diff --git a/RELEASES.md b/RELEASES.md index 32065ccfe2..917cdf7696 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -9,6 +9,7 @@ - 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 the block builder to ACP-226 requirements. + - Add minimum delay verification. ## [v0.7.9](https://github.com/ava-labs/subnet-evm/releases/tag/v0.7.9) diff --git a/miner/worker.go b/miner/worker.go index c669efaaaf..db37610f67 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -139,13 +139,13 @@ func (w *worker) commitNewWork(predicateContext *precompileconfig.PredicateConte w.mu.RLock() defer w.mu.RUnlock() var ( - parent = w.chain.CurrentBlock() - tstart = w.clock.Time() - chainExtra = params.GetExtra(w.chainConfig) + parent = w.chain.CurrentBlock() + chainExtra = params.GetExtra(w.chainConfig) + tstart = customheader.GetNextTimestamp(parent, w.clock.Time()) + timestamp = uint64(tstart.Unix()) + timestampMS = uint64(tstart.UnixMilli()) ) - timestamp, timestampMS := customheader.GetNextTimestamp(parent, tstart) - header := &types.Header{ ParentHash: parent.Hash(), Number: new(big.Int).Add(parent.Number, common.Big1), diff --git a/plugin/evm/customheader/time.go b/plugin/evm/customheader/time.go index 702a802ca4..ac1c15ff61 100644 --- a/plugin/evm/customheader/time.go +++ b/plugin/evm/customheader/time.go @@ -24,30 +24,23 @@ var ( ErrTimeMillisecondsRequired = errors.New("TimeMilliseconds is required after Granite activation") ErrTimeMillisecondsMismatched = errors.New("TimeMilliseconds does not match header.Time") ErrTimeMillisecondsBeforeGranite = errors.New("TimeMilliseconds should be nil before Granite activation") + ErrMinDelayNotMet = errors.New("minimum block delay not met") + ErrGraniteClockBehindParent = errors.New("current timestamp is not allowed to be behind than parent timestamp in Granite") ) -// GetNextTimestamp calculates the timestamp (in seconds and milliseconds) for the next child block based on the parent's timestamp and the current time. -// First return value is the timestamp in seconds, second return value is the timestamp in milliseconds. -func GetNextTimestamp(parent *types.Header, now time.Time) (uint64, uint64) { - var ( - timestamp = uint64(now.Unix()) - timestampMS = uint64(now.UnixMilli()) - ) - // Note: in order to support asynchronous block production, blocks are allowed to have - // the same timestamp as their parent. This allows more than one block to be produced - // per second. +// GetNextTimestamp calculates the time for the next header based on the parent's timestamp and the current time. +// This can return the parent time if now is before the parent time and TimeMilliseconds is not set (pre-Granite). +func GetNextTimestamp(parent *types.Header, now time.Time) time.Time { parentExtra := customtypes.GetHeaderExtra(parent) - if parent.Time >= timestamp || - (parentExtra.TimeMilliseconds != nil && *parentExtra.TimeMilliseconds >= timestampMS) { - timestamp = parent.Time - // If the parent has a TimeMilliseconds, use it. Otherwise, use the parent time * 1000. - if parentExtra.TimeMilliseconds != nil { - timestampMS = *parentExtra.TimeMilliseconds - } else { - timestampMS = parent.Time * 1000 // TODO: establish minimum time - } + // In Granite, there is a minimum delay enforced, so we cannot adjust the time with the parent's timestamp. + // Instead we should have waited enough time before calling this function and before the block building. + // We return the current time instead regardless and defer the verification to VerifyTime. + if parent.Time < uint64(now.Unix()) || parentExtra.TimeMilliseconds != nil { + return now } - return timestamp, timestampMS + + // In pre-Granite, blocks are allowed to have the same timestamp as their parent. + return time.Unix(int64(parent.Time), 0) } // VerifyTime verifies that the header's Time and TimeMilliseconds fields are @@ -58,26 +51,34 @@ func GetNextTimestamp(parent *types.Header, now time.Time) (uint64, uint64) { // - Time matches TimeMilliseconds/1000 after Granite activation // - Time/TimeMilliseconds is not too far in the future // - Time/TimeMilliseconds is non-decreasing -// - (TODO) Minimum block delay is enforced +// - Minimum block delay is enforced func VerifyTime(extraConfig *extras.ChainConfig, parent *types.Header, header *types.Header, now time.Time) error { var ( headerExtra = customtypes.GetHeaderExtra(header) parentExtra = customtypes.GetHeaderExtra(parent) ) + // These two variables are backward-compatible with Time (seconds) fields. + headerTimeMS := customtypes.HeaderTimeMilliseconds(header) + parentTimeMS := customtypes.HeaderTimeMilliseconds(parent) + // Verify the header's timestamp is not earlier than parent's - // it does include equality(==), so multiple blocks per second is ok - if header.Time < parent.Time { - return fmt.Errorf("%w: %d < parent %d", errBlockTooOld, header.Time, parent.Time) + // This includes equality(==), so multiple blocks per milliseconds is ok + // pre-Granite. + if headerTimeMS < parentTimeMS { + return fmt.Errorf("%w: %d < parent %d", errBlockTooOld, headerTimeMS, parentTimeMS) } - // Do all checks that apply only before Granite - if !extraConfig.IsGranite(header.Time) { - // Make sure the block isn't too far in the future - if maxBlockTime := uint64(now.Add(MaxFutureBlockTime).Unix()); header.Time > maxBlockTime { - return fmt.Errorf("%w: %d > allowed %d", ErrBlockTooFarInFuture, header.Time, maxBlockTime) - } + // Verify if the header's timestamp is not too far in the future + if maxBlockTimeMS := uint64(now.Add(MaxFutureBlockTime).UnixMilli()); headerTimeMS > maxBlockTimeMS { + return fmt.Errorf("%w: %d > allowed %d", + ErrBlockTooFarInFuture, + headerTimeMS, + maxBlockTimeMS, + ) + } + if !extraConfig.IsGranite(header.Time) { // This field should not be set yet. if headerExtra.TimeMilliseconds != nil { return ErrTimeMillisecondsBeforeGranite @@ -99,22 +100,23 @@ func VerifyTime(extraConfig *extras.ChainConfig, parent *types.Header, header *t ) } - // Verify TimeMilliseconds is not earlier than parent's TimeMilliseconds - // TODO: Ensure minimum block delay is enforced - if parentExtra.TimeMilliseconds != nil && *headerExtra.TimeMilliseconds < *parentExtra.TimeMilliseconds { - return fmt.Errorf("%w: %d < parent %d", - errBlockTooOld, - *headerExtra.TimeMilliseconds, - *parentExtra.TimeMilliseconds, - ) + // Verify minimum block delay is enforced + // Parent might not have a min delay excess if this is the first Granite block + // in this case we cannot verify the min delay, + // Otherwise parent should have been verified in VerifyMinDelayExcess + if parentExtra.MinDelayExcess == nil { + return nil } - // Verify TimeMilliseconds is not too far in the future - if maxBlockTimeMillis := uint64(now.Add(MaxFutureBlockTime).UnixMilli()); *headerExtra.TimeMilliseconds > maxBlockTimeMillis { - return fmt.Errorf("%w: %d > allowed %d", - ErrBlockTooFarInFuture, - *headerExtra.TimeMilliseconds, - maxBlockTimeMillis, + // This should not be underflow as we have verified that the parent's + // TimeMilliseconds is earlier than the header's TimeMilliseconds above. + actualDelayMS := headerTimeMS - parentTimeMS + minRequiredDelayMS := parentExtra.MinDelayExcess.Delay() + if actualDelayMS < minRequiredDelayMS { + return fmt.Errorf("%w: actual delay %dms < required %dms", + ErrMinDelayNotMet, + actualDelayMS, + minRequiredDelayMS, ) } diff --git a/plugin/evm/customheader/time_test.go b/plugin/evm/customheader/time_test.go index 06b4500695..3dd974c0a4 100644 --- a/plugin/evm/customheader/time_test.go +++ b/plugin/evm/customheader/time_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/ava-labs/avalanchego/vms/evm/acp226" "github.com/ava-labs/libevm/core/types" "github.com/stretchr/testify/require" @@ -15,17 +16,6 @@ import ( "github.com/ava-labs/subnet-evm/utils" ) -func generateHeader(timeSeconds uint64, timeMilliseconds *uint64) *types.Header { - return customtypes.WithHeaderExtra( - &types.Header{ - Time: timeSeconds, - }, - &customtypes.HeaderExtra{ - TimeMilliseconds: timeMilliseconds, - }, - ) -} - func TestVerifyTime(t *testing.T) { var ( now = time.Unix(1714339200, 123_456_789) @@ -138,6 +128,110 @@ func TestVerifyTime(t *testing.T) { parentHeader: generateHeader(timeSeconds, nil), extraConfig: extras.TestGraniteChainConfig, }, + // Min delay verification tests + { + name: "pre_granite_no_min_delay_verification", + header: generateHeader(timeSeconds, nil), + parentHeader: generateHeader(timeSeconds, nil), + extraConfig: extras.TestFortunaChainConfig, + }, + { + name: "granite_first_block_no_parent_min_delay_excess", + header: generateHeaderWithMinDelayExcessAndTime( + timeSeconds, + utils.NewUint64(timeMillis), + utils.NewUint64(acp226.InitialDelayExcess), + ), + parentHeader: generateHeader(timeSeconds-1, nil), // Pre-Granite parent + extraConfig: extras.TestGraniteChainConfig, + }, + { + name: "granite_initial_delay_met", + header: generateHeaderWithMinDelayExcessAndTime( + timeSeconds, + utils.NewUint64(timeMillis), + utils.NewUint64(acp226.InitialDelayExcess), + ), + parentHeader: generateHeaderWithMinDelayExcessAndTime( + timeSeconds-1, + utils.NewUint64(timeMillis-2000), // 2000 ms is the exact initial delay + utils.NewUint64(acp226.InitialDelayExcess), + ), + extraConfig: extras.TestGraniteChainConfig, + }, + { + name: "granite_initial_delay_not_met", + header: generateHeaderWithMinDelayExcessAndTime( + timeSeconds, + utils.NewUint64(timeMillis), + utils.NewUint64(acp226.InitialDelayExcess), + ), + parentHeader: generateHeaderWithMinDelayExcessAndTime( + timeSeconds-1, + utils.NewUint64(timeMillis-1999), // 1 ms less than required + utils.NewUint64(acp226.InitialDelayExcess), + ), + extraConfig: extras.TestGraniteChainConfig, + expectedErr: ErrMinDelayNotMet, + }, + { + name: "granite_future_timestamp_within_limits", + header: generateHeaderWithMinDelayExcessAndTime( + timeSeconds+5, // 5 seconds in future + utils.NewUint64(timeMillis+5000), + utils.NewUint64(acp226.InitialDelayExcess), + ), + parentHeader: generateHeaderWithMinDelayExcessAndTime( + timeSeconds-1, + utils.NewUint64(timeMillis-2000), + utils.NewUint64(acp226.InitialDelayExcess), + ), + extraConfig: extras.TestGraniteChainConfig, + }, + { + name: "granite_future_timestamp_abuse", + header: generateHeaderWithMinDelayExcessAndTime( + timeSeconds+15, // 15 seconds in future, exceeds MaxFutureBlockTime + utils.NewUint64(timeMillis+15000), + utils.NewUint64(acp226.InitialDelayExcess), + ), + parentHeader: generateHeaderWithMinDelayExcessAndTime( + timeSeconds-1, + utils.NewUint64(timeMillis-2000), + utils.NewUint64(acp226.InitialDelayExcess), + ), + extraConfig: extras.TestGraniteChainConfig, + expectedErr: ErrBlockTooFarInFuture, + }, + { + name: "granite_zero_delay_excess", + header: generateHeaderWithMinDelayExcessAndTime( + timeSeconds, + utils.NewUint64(timeMillis), + utils.NewUint64(0), + ), + parentHeader: generateHeaderWithMinDelayExcessAndTime( + timeSeconds, + utils.NewUint64(timeMillis-1), // 1ms delay, meets zero requirement + utils.NewUint64(0), // Parent has zero delay excess + ), + extraConfig: extras.TestGraniteChainConfig, + }, + { + name: "granite_zero_delay_excess_but_zero_delay", + header: generateHeaderWithMinDelayExcessAndTime( + timeSeconds, + utils.NewUint64(timeMillis), + utils.NewUint64(0), + ), + parentHeader: generateHeaderWithMinDelayExcessAndTime( + timeSeconds, + utils.NewUint64(timeMillis), // Same timestamp, zero delay + utils.NewUint64(0), // Parent has zero delay excess + ), + extraConfig: extras.TestGraniteChainConfig, + expectedErr: ErrMinDelayNotMet, + }, } for _, test := range tests { @@ -188,14 +282,7 @@ func TestGetNextTimestamp(t *testing.T) { expectedMillis: nowSeconds * 1000, // parent.Time * 1000 }, { - name: "current_time_equals_parent_time_with_milliseconds", - parent: generateHeader(nowSeconds, utils.NewUint64(nowMillis)), - now: now, - expectedSec: nowSeconds, - expectedMillis: nowMillis, // parent's TimeMilliseconds - }, - { - name: "current_time_before_parent_time", + name: "current_time_before_parent_time_no_milliseconds", parent: generateHeader(nowSeconds+10, nil), now: now, expectedSec: nowSeconds + 10, @@ -205,23 +292,60 @@ func TestGetNextTimestamp(t *testing.T) { name: "current_time_before_parent_time_with_milliseconds", parent: generateHeader(nowSeconds+10, utils.NewUint64(nowMillis)), now: now, - expectedSec: nowSeconds + 10, - expectedMillis: nowMillis, // parent's TimeMilliseconds + expectedSec: nowSeconds, + expectedMillis: nowMillis, }, { name: "current_time_milliseconds_before_parent_time_milliseconds", parent: generateHeader(nowSeconds, utils.NewUint64(nowMillis+10)), now: now, expectedSec: nowSeconds, - expectedMillis: nowMillis + 10, // parent's TimeMilliseconds + expectedMillis: nowMillis, + }, + { + name: "current_time_equals_parent_time_with_milliseconds_granite", + parent: generateHeader(nowSeconds, utils.NewUint64(nowMillis)), + now: now, + expectedSec: nowSeconds, + expectedMillis: nowMillis, + }, + { + name: "current_timesec_equals_parent_time_with_less_milliseconds", + parent: generateHeader(nowSeconds, utils.NewUint64(nowMillis-10)), + now: now, + expectedSec: nowSeconds, + expectedMillis: nowMillis, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - sec, millis := GetNextTimestamp(test.parent, test.now) - require.Equal(t, test.expectedSec, sec) - require.Equal(t, test.expectedMillis, millis) + time := GetNextTimestamp(test.parent, test.now) + require.Equal(t, test.expectedSec, uint64(time.Unix())) + require.Equal(t, test.expectedMillis, uint64(time.UnixMilli())) }) } } + +func generateHeader(timeSeconds uint64, timeMilliseconds *uint64) *types.Header { + return customtypes.WithHeaderExtra( + &types.Header{ + Time: timeSeconds, + }, + &customtypes.HeaderExtra{ + TimeMilliseconds: timeMilliseconds, + }, + ) +} + +func generateHeaderWithMinDelayExcessAndTime(timeSeconds uint64, timeMilliseconds *uint64, minDelayExcess *uint64) *types.Header { + return customtypes.WithHeaderExtra( + &types.Header{ + Time: timeSeconds, + }, + &customtypes.HeaderExtra{ + TimeMilliseconds: timeMilliseconds, + MinDelayExcess: (*acp226.DelayExcess)(minDelayExcess), + }, + ) +} diff --git a/plugin/evm/vm_test.go b/plugin/evm/vm_test.go index bb1ca13e91..d68925be16 100644 --- a/plugin/evm/vm_test.go +++ b/plugin/evm/vm_test.go @@ -4097,12 +4097,8 @@ func TestDelegatePrecompile_BehaviorAcrossUpgrades(t *testing.T) { data := crypto.Keccak256([]byte("delegateSendHello()"))[:4] nonce := vm.txPool.Nonce(testEthAddrs[0]) signedTx := newSignedLegacyTx(t, vm.chainConfig, testKeys[0].ToECDSA(), nonce, &contractAddr, big.NewInt(0), 100000, tt.txGasPrice, data) - for _, err := range vm.txPool.AddRemotesSync([]*types.Transaction{signedTx}) { - require.NoError(t, err) - } - - blk, err := vm.BuildBlock(ctx) + blk, err := IssueTxsAndSetPreference([]*types.Transaction{signedTx}, vm) if !tt.wantIncluded { // On subnet-evm, InvalidateExecution causes the transaction to be excluded from the block. // BuildBlock will create a block but it will fail verification because it's empty @@ -4111,13 +4107,11 @@ func TestDelegatePrecompile_BehaviorAcrossUpgrades(t *testing.T) { require.ErrorContains(t, err, "empty block", "Should fail with empty block error") return } - require.NoError(t, err) - require.NoError(t, blk.Verify(ctx)) - require.NoError(t, vm.SetPreference(ctx, blk.ID())) require.NoError(t, blk.Accept(ctx)) ethBlock := blk.(*chain.BlockWrapper).Block.(*wrappedBlock).ethBlock + require.Len(t, ethBlock.Transactions(), 1) receipts := vm.blockChain.GetReceiptsByHash(ethBlock.Hash()) require.Len(t, receipts, 1)