ACP-226: Apply Min Block Delay to Block Builder #1310
Conversation
ceyonur
changed the title
ceyonur
force-pushed
the
ceyonur/acp-226-min-block-delay-builder
branch
from
f02e5c7 to
aa9a2cc
Compare
| - Set expected block gas cost to 0 in Granite network upgrade, removing block gas cost requirements for block building. | ||
| - Add `timeMilliseconds` (Unix uint64) timestamp to block header for Granite upgrade. | ||
| - Add `minDelayExcess` (uint64) to block header for Granite upgrade. | ||
| - Add minimum block building delays to conform ACP-226 requirements to the block builder. |
There was a problem hiding this comment.
| - Add minimum block building delays to conform ACP-226 requirements to the block builder. | |
| - Add minimum block building delays to conform the block builder to ACP-226 requirements. |
Grammar nit
| // Minimum amount of time to wait after building a block before attempting to build a block | ||
| // a second time without changing the contents of the mempool. | ||
| PreGraniteMinBlockBuildingRetryDelay = 500 * time.Millisecond | ||
| // Minimum amount of time to wait after attempting/build a block before attempting to build another block | ||
| // This is only applied for retrying to build a block after a minimum delay has passed. | ||
| // The initial minimum delay is applied according to parent minDelayExcess (if available) | ||
| // TODO (ceyonur): Decide whether this a correct value. | ||
| PostGraniteMinBlockBuildingRetryDelay = 100 * time.Millisecond |
There was a problem hiding this comment.
Why should this value differ pre- and post-Granite?
IIUC, it only applies for retrying to build a block (after previously not being able to on an initial attempt or prior retry), and the reasons why a retry may be successful after a period of time are equivalent both pre- and post-Granite.
| // 1. If the current header is in Granite, return the remaining ACP-226 delay after the parent block time and the minimum retry delay. | ||
| // 2. If the current header is not in Granite, return 0 and the minimum retry delay. | ||
| func (b *blockBuilder) getMinBlockBuildingDelays(currentHeader *types.Header, config *extras.ChainConfig) (time.Duration, time.Duration, error) { | ||
| // TODO Cleanup (ceyonur): this check can be removed after Granite is activated. |
There was a problem hiding this comment.
| // TODO Cleanup (ceyonur): this check can be removed after Granite is activated. | |
| // TODO (ceyonur): this check can be removed after Granite is activated. (See https://github.com/ava-labs/coreth/issues/1318) |
Did we decide we wanted to link to issues created for TODOs or not? Feel free to ignore if we decided not to of course.
|
|
||
| // getMinBlockBuildingDelays returns the initial min block building delay and the minimum retry delay. | ||
| // It implements the following logic: | ||
| // 1. If the current header is in Granite, return the remaining ACP-226 delay after the parent block time and the minimum retry delay. | ||
| // 2. If the current header is not in Granite, return 0 and the minimum retry delay. | ||
| func (b *blockBuilder) getMinBlockBuildingDelays(currentHeader *types.Header, config *extras.ChainConfig) (time.Duration, time.Duration, error) { |
There was a problem hiding this comment.
| // getMinBlockBuildingDelays returns the initial min block building delay and the minimum retry delay. | |
| // It implements the following logic: | |
| // 1. If the current header is in Granite, return the remaining ACP-226 delay after the parent block time and the minimum retry delay. | |
| // 2. If the current header is not in Granite, return 0 and the minimum retry delay. | |
| func (b *blockBuilder) getMinBlockBuildingDelays(currentHeader *types.Header, config *extras.ChainConfig) (time.Duration, time.Duration, error) { | |
| // minBlockBuildingDelays returns the initial required minimum block building delay and the minimum retry delay. | |
| func (b *blockBuilder) minBlockBuildingDelays(currentHeader *types.Header, config *extras.ChainConfig) (time.Duration, time.Duration, error) { |
Echo'ing Arran's rationale here about not using "get".
The logic I think is self-explanatory and self-documenting in the implementation itself.
If we're able to keep the retry times the same pre- and post-Granite, I think this function will become cleaner as well, since it will only return a single value.
| } | ||
|
|
||
| // calculateBlockBuildingDelay calculates the delay needed before building the next block. | ||
| // It returns the time to wait, a boolean indicating whether to build immediately, and any error. |
There was a problem hiding this comment.
I think the boolean return value here is not necessary. With just (time.Duration, error) return values, buildImmediately = err == nill && time.Duration == 0. Callers should always check for non-nil errors first prior to using any other return values, and it will be cleaner to then just use the single time.Duration IMO.
| lastBuildParentHash common.Hash, | ||
| currentHeader *types.Header, | ||
| ) (time.Duration, bool, error) { | ||
| initialMinBlockBuildingDelay, minBlockBuildingRetryDelay, err := b.getMinBlockBuildingDelays(currentHeader, b.chainConfig) |
There was a problem hiding this comment.
| initialMinBlockBuildingDelay, minBlockBuildingRetryDelay, err := b.getMinBlockBuildingDelays(currentHeader, b.chainConfig) | |
| initialDelay, retryDelay, err := b.getMinBlockBuildingDelays(currentHeader, b.chainConfig) |
I think these will be easier to read, since we're already within a block building context. At the very least, think we should put "initial" and "retry" in the same place, (i.e. initialMinBlockBuildingDelay and retryMinBlockBuilding Delay)
|
|
||
| isRetry := lastBuildParentHash == currentHeader.ParentHash && !lastBuildTime.IsZero() // if last build time is zero, this is not a retry | ||
|
|
||
| timeSinceLastBuildTime := b.clock.Time().Sub(lastBuildTime) |
There was a problem hiding this comment.
I think this logic below feels a bit hard to reason about because we're doing multiple comparisons against the current time (one here and one in getMinBlockBuildingDelays). It'd be more clear if we only compared against the current time once IMO.
Why this should be merged
ACP-226 directly affects the block building time. This PR adds ACP-226 delays to block builder.
How this works
It adds 2 delay system to the block builder:
1- ACP-226 based remaining min delay after parent's time. This is in order to pass the verification (to be added).
2- A constant min retry delay
How this was tested
added UTs
Need to be documented?
no
Need to update RELEASES.md?
updated