-
Notifications
You must be signed in to change notification settings - Fork 158
ACP-226: Apply Min Block Delay to Block Builder #1310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
ef12dfb
ef76b58
cf66f59
9c0d64c
da7a8cb
5402155
a35d6fb
4f5b78b
27da7a6
3c2d944
67c62cd
eef3650
49d1a78
e998652
ac83a0c
10e3bc0
6d05981
3385ef1
a2a6946
76111f5
049aefa
ce38349
aa9a2cc
29aee71
1503d87
cf2697e
d96886b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,6 +10,7 @@ import ( | |||||
|
||||||
"github.com/ava-labs/avalanchego/snow" | ||||||
"github.com/ava-labs/avalanchego/utils/lock" | ||||||
"github.com/ava-labs/avalanchego/utils/timer/mockable" | ||||||
"github.com/ava-labs/libevm/common" | ||||||
"github.com/ava-labs/libevm/core/types" | ||||||
"github.com/ava-labs/libevm/log" | ||||||
|
@@ -18,17 +19,29 @@ import ( | |||||
|
||||||
"github.com/ava-labs/coreth/core" | ||||||
"github.com/ava-labs/coreth/core/txpool" | ||||||
"github.com/ava-labs/coreth/params/extras" | ||||||
"github.com/ava-labs/coreth/plugin/evm/customheader" | ||||||
"github.com/ava-labs/coreth/plugin/evm/customtypes" | ||||||
"github.com/ava-labs/coreth/plugin/evm/extension" | ||||||
|
||||||
commonEng "github.com/ava-labs/avalanchego/snow/engine/common" | ||||||
) | ||||||
|
||||||
// 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. | ||||||
const MinBlockBuildingRetryDelay = 500 * time.Millisecond | ||||||
const ( | ||||||
// 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 | ||||||
) | ||||||
|
||||||
type blockBuilder struct { | ||||||
ctx *snow.Context | ||||||
clock *mockable.Clock | ||||||
ctx *snow.Context | ||||||
chainConfig *extras.ChainConfig | ||||||
|
||||||
txPool *txpool.TxPool | ||||||
extraMempool extension.BuilderMempool | ||||||
|
@@ -51,10 +64,12 @@ type blockBuilder struct { | |||||
func (vm *VM) NewBlockBuilder(extraMempool extension.BuilderMempool) *blockBuilder { | ||||||
b := &blockBuilder{ | ||||||
ctx: vm.ctx, | ||||||
chainConfig: vm.chainConfigExtra(), | ||||||
txPool: vm.txPool, | ||||||
extraMempool: extraMempool, | ||||||
shutdownChan: vm.shutdownChan, | ||||||
shutdownWg: &vm.shutdownWg, | ||||||
clock: vm.clock, | ||||||
} | ||||||
b.pendingSignal = lock.NewCond(&b.buildBlockLock) | ||||||
return b | ||||||
|
@@ -64,7 +79,7 @@ func (vm *VM) NewBlockBuilder(extraMempool extension.BuilderMempool) *blockBuild | |||||
func (b *blockBuilder) handleGenerateBlock(currentParentHash common.Hash) { | ||||||
b.buildBlockLock.Lock() | ||||||
defer b.buildBlockLock.Unlock() | ||||||
b.lastBuildTime = time.Now() | ||||||
b.lastBuildTime = b.clock.Time() | ||||||
b.lastBuildParentHash = currentParentHash | ||||||
} | ||||||
|
||||||
|
@@ -124,20 +139,19 @@ func (b *blockBuilder) waitForEvent(ctx context.Context, currentHeader *types.He | |||||
if err != nil { | ||||||
return 0, err | ||||||
} | ||||||
timeSinceLastBuildTime := time.Since(lastBuildTime) | ||||||
isRetry := lastBuildParentHash == currentHeader.ParentHash | ||||||
// 1. if this is not a retry | ||||||
// 2. if this is the first time we try to build a block | ||||||
// 3. if the time since the last build is greater than the minimum retry delay | ||||||
// then we can build a block immediately. | ||||||
if !isRetry || lastBuildTime.IsZero() || timeSinceLastBuildTime >= MinBlockBuildingRetryDelay { | ||||||
b.ctx.Log.Debug("Last time we built a block was long enough ago or this is not a retry, no need to wait", | ||||||
zap.Duration("timeSinceLastBuildTime", timeSinceLastBuildTime), | ||||||
zap.Bool("isRetry", isRetry), | ||||||
) | ||||||
timeUntilNextBuild, err := b.calculateBlockBuildingDelay( | ||||||
lastBuildTime, | ||||||
lastBuildParentHash, | ||||||
currentHeader, | ||||||
) | ||||||
if err != nil { | ||||||
return 0, err | ||||||
} | ||||||
if timeUntilNextBuild == 0 { | ||||||
b.ctx.Log.Debug("Last time we built a block was long enough ago or this is not a retry, no need to wait") | ||||||
return commonEng.PendingTxs, nil | ||||||
} | ||||||
timeUntilNextBuild := MinBlockBuildingRetryDelay - timeSinceLastBuildTime | ||||||
|
||||||
b.ctx.Log.Debug("Last time we built a block was too recent, waiting", | ||||||
zap.Duration("timeUntilNextBuild", timeUntilNextBuild), | ||||||
) | ||||||
|
@@ -161,3 +175,66 @@ func (b *blockBuilder) waitForNeedToBuild(ctx context.Context) (time.Time, commo | |||||
} | ||||||
return b.lastBuildTime, b.lastBuildParentHash, nil | ||||||
} | ||||||
|
||||||
// currentMinBlockBuildingDelays 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) currentMinBlockBuildingDelays(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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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. |
||||||
currentTimestamp := b.clock.Unix() | ||||||
if !config.IsGranite(currentTimestamp) { | ||||||
return 0, PreGraniteMinBlockBuildingRetryDelay, nil // Pre-Granite: no initial delay | ||||||
} | ||||||
|
||||||
acp226DelayExcess, err := customheader.MinDelayExcess(config, currentHeader, currentTimestamp, nil) | ||||||
if err != nil { | ||||||
return 0, 0, err | ||||||
} | ||||||
acp226Delay := time.Duration(acp226DelayExcess.Delay()) * time.Millisecond | ||||||
|
||||||
// Calculate initial delay: time since parent minus ACP-226 delay (clamped to 0) | ||||||
parentBlockTime := customtypes.BlockTime(currentHeader) | ||||||
timeSinceParentBlock := b.clock.Time().Sub(parentBlockTime) | ||||||
// TODO question (ceyonur): should we just use acp226Delay if timeSinceParentBlock is negative? | ||||||
initialMinBlockBuildingDelay := acp226Delay - timeSinceParentBlock | ||||||
if initialMinBlockBuildingDelay < 0 { | ||||||
initialMinBlockBuildingDelay = 0 | ||||||
} | ||||||
|
||||||
return initialMinBlockBuildingDelay, PostGraniteMinBlockBuildingRetryDelay, nil | ||||||
} | ||||||
|
||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the boolean return value here is not necessary. With just |
||||||
// It implements the following logic: | ||||||
// 1. If there is no initial min block building delay | ||||||
// 2. if this is not a retry | ||||||
// 3. if the time since the last build is greater than the minimum retry delay | ||||||
// then we can build a block immediately. | ||||||
func (b *blockBuilder) calculateBlockBuildingDelay( | ||||||
lastBuildTime time.Time, | ||||||
lastBuildParentHash common.Hash, | ||||||
currentHeader *types.Header, | ||||||
) (time.Duration, error) { | ||||||
initialDelay, retryDelay, err := b.currentMinBlockBuildingDelays(currentHeader, b.chainConfig) | ||||||
if err != nil { | ||||||
return 0, err | ||||||
} | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are two different comparisons:
If you say it's easier to read, I can collect all delay logic in the same function |
||||||
var remainingMinDelay time.Duration | ||||||
if retryDelay > timeSinceLastBuildTime { | ||||||
remainingMinDelay = retryDelay - timeSinceLastBuildTime | ||||||
} | ||||||
|
||||||
if initialDelay > 0 { | ||||||
remainingMinDelay = max(initialDelay, remainingMinDelay) | ||||||
} else if !isRetry || remainingMinDelay == 0 { | ||||||
return 0, nil // Build immediately | ||||||
} | ||||||
|
||||||
return remainingMinDelay, nil // Need to wait | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
package evm | ||
|
||
import ( | ||
"testing" | ||
"time" | ||
|
||
"github.com/ava-labs/avalanchego/utils/timer/mockable" | ||
"github.com/ava-labs/avalanchego/vms/evm/acp226" | ||
"github.com/ava-labs/libevm/common" | ||
"github.com/ava-labs/libevm/core/types" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/ava-labs/coreth/params/extras" | ||
"github.com/ava-labs/coreth/plugin/evm/customtypes" | ||
) | ||
|
||
func TestCalculateBlockBuildingDelay(t *testing.T) { | ||
now := time.UnixMilli(10000) | ||
nowSecUint64 := uint64(now.Unix()) | ||
nowMilliUint64 := uint64(now.UnixMilli()) | ||
clock := &mockable.Clock{} | ||
clock.Set(now) | ||
tests := []struct { | ||
name string | ||
config *extras.ChainConfig | ||
currentHeader *types.Header | ||
lastBuildTime time.Time | ||
lastBuildParentHash common.Hash | ||
expectedTimeToWait time.Duration | ||
}{ | ||
{ | ||
name: "pre_granite_returns_build_immediately_zero_time", | ||
config: extras.TestFortunaChainConfig, // Pre-Granite config | ||
currentHeader: &types.Header{ | ||
ParentHash: common.Hash{1}, | ||
Time: nowSecUint64, | ||
}, | ||
lastBuildTime: time.Time{}, // Zero time means not a retry | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: 0, | ||
}, | ||
{ | ||
name: "pre_granite_returns_build_immediately_different_parent_hash", | ||
config: extras.TestFortunaChainConfig, // Pre-Granite config | ||
currentHeader: &types.Header{ | ||
ParentHash: common.Hash{2}, | ||
Time: nowSecUint64, | ||
}, | ||
lastBuildTime: now, | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: 0, | ||
}, | ||
{ | ||
name: "pre_granite_returns_build_delays_with_same_parent_hash", | ||
config: extras.TestFortunaChainConfig, // Pre-Granite config | ||
currentHeader: &types.Header{ | ||
ParentHash: common.Hash{1}, | ||
Time: nowSecUint64, | ||
}, | ||
lastBuildTime: now, | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: PreGraniteMinBlockBuildingRetryDelay, | ||
}, | ||
{ | ||
name: "pre_granite_returns_build_returns_immediately_if_enough_time_passed", | ||
config: extras.TestFortunaChainConfig, // Pre-Granite config | ||
currentHeader: &types.Header{ | ||
ParentHash: common.Hash{1}, | ||
Time: nowSecUint64, | ||
}, | ||
lastBuildTime: now.Add(-PreGraniteMinBlockBuildingRetryDelay), // Less than retry delay ago | ||
lastBuildParentHash: common.Hash{1}, // Same as current parent | ||
expectedTimeToWait: 0, | ||
}, | ||
{ | ||
name: "pre_granite_returns_build_delays_only_remaining_min_delay", | ||
config: extras.TestFortunaChainConfig, // Pre-Granite config | ||
currentHeader: &types.Header{ | ||
ParentHash: common.Hash{1}, | ||
Time: nowSecUint64, | ||
}, | ||
lastBuildTime: now.Add(-PreGraniteMinBlockBuildingRetryDelay / 2), // Less than retry delay ago | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: PreGraniteMinBlockBuildingRetryDelay / 2, | ||
}, | ||
{ | ||
name: "granite_block_with_now_time", | ||
config: extras.TestGraniteChainConfig, | ||
currentHeader: createGraniteTestHeader(common.Hash{1}, nowMilliUint64, acp226.InitialDelayExcess), | ||
lastBuildTime: time.Time{}, | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: 2000 * time.Millisecond, // should wait for initial delay | ||
}, | ||
{ | ||
name: "granite_block_with_2_seconds_before_clock_no_retry", | ||
config: extras.TestGraniteChainConfig, | ||
currentHeader: createGraniteTestHeader(common.Hash{1}, nowMilliUint64-2000, acp226.InitialDelayExcess), | ||
lastBuildTime: time.Time{}, // Zero time means not a retry | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: 0, // should not wait for initial delay | ||
}, | ||
{ | ||
name: "granite_block_with_2_seconds_before_clock_with_retry", | ||
config: extras.TestGraniteChainConfig, | ||
currentHeader: createGraniteTestHeader(common.Hash{1}, nowMilliUint64-2000, acp226.InitialDelayExcess), | ||
lastBuildTime: now, | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: PostGraniteMinBlockBuildingRetryDelay, | ||
}, | ||
{ | ||
name: "granite_with_2_seconds_before_clock_only_waits_for_retry_delay", | ||
config: extras.TestGraniteChainConfig, | ||
currentHeader: createGraniteTestHeader(common.Hash{1}, nowMilliUint64-2000, 0), // 0 means min delay excess which is 1 | ||
lastBuildTime: now, | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: PostGraniteMinBlockBuildingRetryDelay, | ||
}, | ||
{ | ||
name: "granite_with_2_seconds_before_clock_only_waits_for_remaining_retry_delay", | ||
config: extras.TestGraniteChainConfig, | ||
currentHeader: createGraniteTestHeader(common.Hash{1}, nowMilliUint64-2000, 0), // 0 means min delay excess which is 1 | ||
lastBuildTime: now.Add(-PostGraniteMinBlockBuildingRetryDelay / 2), // Less than retry delay ago | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: PostGraniteMinBlockBuildingRetryDelay / 2, | ||
}, | ||
{ | ||
name: "granite_with_2_seconds_after_clock", | ||
config: extras.TestGraniteChainConfig, | ||
currentHeader: createGraniteTestHeader(common.Hash{1}, nowMilliUint64+2000, acp226.InitialDelayExcess), | ||
lastBuildTime: time.Time{}, // Zero time means not a retry | ||
lastBuildParentHash: common.Hash{1}, | ||
expectedTimeToWait: 4000 * time.Millisecond, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
b := &blockBuilder{ | ||
clock: clock, | ||
chainConfig: tt.config, | ||
} | ||
|
||
timeToWait, err := b.calculateBlockBuildingDelay( | ||
tt.lastBuildTime, | ||
tt.lastBuildParentHash, | ||
tt.currentHeader, | ||
) | ||
|
||
require.NoError(t, err) | ||
require.Equal(t, tt.expectedTimeToWait, timeToWait) | ||
}) | ||
} | ||
} | ||
|
||
func createGraniteTestHeader(parentHash common.Hash, timeMilliseconds uint64, minDelayExcess acp226.DelayExcess) *types.Header { | ||
header := &types.Header{ | ||
Time: timeMilliseconds / 1000, | ||
} | ||
header.ParentHash = parentHash | ||
|
||
extra := &customtypes.HeaderExtra{ | ||
TimeMilliseconds: &timeMilliseconds, | ||
MinDelayExcess: &minDelayExcess, | ||
} | ||
customtypes.SetHeaderExtra(header, extra) | ||
|
||
return header | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PostGranite using 500 ms can be too much and can miss the next block if the chain is configured with less than 0.5ms. We should probably decrease the delay so that we can actually have 0.5ms blocks. I just did not want to change the current behaviour but we can surely use a single delay constant with 100 ms and such. cc @StephenButtolph