Bound GetNextStakerChangeTime for ACP-77

vms/platformvm/block/builder/builder.go

@@ -31,9 +31,15 @@ import (
 	txexecutor "github.com/ava-labs/avalanchego/vms/platformvm/txs/executor"
 )
 
-// targetBlockSize is maximum number of transaction bytes to place into a
-// StandardBlock
-const targetBlockSize = 128 * units.KiB
+const (
+	// targetBlockSize is maximum number of transaction bytes to place into a
+	// StandardBlock
+	targetBlockSize = 128 * units.KiB
+
+	// maxTimeToSleep is the maximum time to sleep between checking if a block
+	// should be produced.
+	maxTimeToSleep = time.Hour
+)
 
 var (
 	_ Builder = (*builder)(nil)
@@ -174,12 +180,13 @@ func (b *builder) durationToSleep() (time.Duration, error) {
 		return 0, fmt.Errorf("%w: %s", errMissingPreferredState, preferredID)
 	}
 
-	nextStakerChangeTime, err := state.GetNextStakerChangeTime(preferredState)
+	now := b.txExecutorBackend.Clk.Time()
+	maxTimeToAwake := now.Add(maxTimeToSleep)
+	nextStakerChangeTime, err := state.GetNextStakerChangeTime(preferredState, maxTimeToAwake)
 	if err != nil {
 		return 0, fmt.Errorf("%w of %s: %w", errCalculatingNextStakerTime, preferredID, err)
 	}
 
-	now := b.txExecutorBackend.Clk.Time()
 	return nextStakerChangeTime.Sub(now), nil
 }
 

vms/platformvm/block/executor/proposal_block_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/ava-labs/avalanchego/utils/crypto/bls"
 	"github.com/ava-labs/avalanchego/utils/crypto/secp256k1"
 	"github.com/ava-labs/avalanchego/utils/iterator/iteratormock"
+	"github.com/ava-labs/avalanchego/utils/timer/mockable"
 	"github.com/ava-labs/avalanchego/vms/components/avax"
 	"github.com/ava-labs/avalanchego/vms/components/gas"
 	"github.com/ava-labs/avalanchego/vms/platformvm/block"
@@ -279,7 +280,7 @@ func TestBanffProposalBlockTimeVerification(t *testing.T) {
 
 		block := env.blkManager.NewBlock(statelessProposalBlock)
 		err = block.Verify(context.Background())
-		require.ErrorIs(err, errChildBlockEarlierThanParent)
+		require.ErrorIs(err, executor.ErrChildBlockEarlierThanParent)
 	}
 
 	{
@@ -1377,7 +1378,7 @@ func TestAddValidatorProposalBlock(t *testing.T) {
 
 	// Advance time until next staker change time is [validatorEndTime]
 	for {
-		nextStakerChangeTime, err := state.GetNextStakerChangeTime(env.state)
+		nextStakerChangeTime, err := state.GetNextStakerChangeTime(env.state, mockable.MaxTime)
 		require.NoError(err)
 		if nextStakerChangeTime.Equal(validatorEndTime) {
 			break

vms/platformvm/block/executor/standard_block_test.go

@@ -212,7 +212,7 @@ func TestBanffStandardBlockTimeVerification(t *testing.T) {
 		require.NoError(err)
 		block := env.blkManager.NewBlock(banffChildBlk)
 		err = block.Verify(context.Background())
-		require.ErrorIs(err, errChildBlockEarlierThanParent)
+		require.ErrorIs(err, executor.ErrChildBlockEarlierThanParent)
 	}
 
 	{

vms/platformvm/block/executor/verifier.go

@@ -27,7 +27,6 @@ var (
 	errApricotBlockIssuedAfterFork           = errors.New("apricot block issued after fork")
 	errBanffStandardBlockWithoutChanges      = errors.New("BanffStandardBlock performs no state changes")
 	errIncorrectBlockHeight                  = errors.New("incorrect block height")
-	errChildBlockEarlierThanParent           = errors.New("proposed timestamp before current chain time")
 	errOptionBlockTimestampNotMatchingParent = errors.New("option block proposed timestamp not matching parent block one")
 )
 
@@ -278,26 +277,11 @@ func (v *verifier) banffNonOptionBlock(b block.BanffBlock) error {
 	}
 
 	newChainTime := b.Timestamp()
-	parentChainTime := parentState.GetTimestamp()
-	if newChainTime.Before(parentChainTime) {
-		return fmt.Errorf(
-			"%w: proposed timestamp (%s), chain time (%s)",
-			errChildBlockEarlierThanParent,
-			newChainTime,
-			parentChainTime,
-		)
-	}
-
-	nextStakerChangeTime, err := state.GetNextStakerChangeTime(parentState)
-	if err != nil {
-		return fmt.Errorf("could not verify block timestamp: %w", err)
-	}
-
 	now := v.txExecutorBackend.Clk.Time()
 	return executor.VerifyNewChainTime(
 		newChainTime,
-		nextStakerChangeTime,
 		now,
+		parentState,
 	)
 }
 

vms/platformvm/state/chain_time_helpers.go

@@ -7,7 +7,7 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/ava-labs/avalanchego/database"
+	"github.com/ava-labs/avalanchego/utils/iterator"
 	"github.com/ava-labs/avalanchego/utils/timer/mockable"
 	"github.com/ava-labs/avalanchego/vms/components/gas"
 	"github.com/ava-labs/avalanchego/vms/platformvm/config"
@@ -24,7 +24,10 @@ func NextBlockTime(state Chain, clk *mockable.Clock) (time.Time, bool, error) {
 	}
 	// [timestamp] = max(now, parentTime)
 
-	nextStakerChangeTime, err := GetNextStakerChangeTime(state)
+	// If the NextStakerChangeTime is after timestamp, then we shouldn't return
+	// that the time was capped.
+	nextStakerChangeTimeCap := timestamp.Add(time.Second)
+	nextStakerChangeTime, err := GetNextStakerChangeTime(state, nextStakerChangeTimeCap)
 	if err != nil {
 		return time.Time{}, false, fmt.Errorf("failed getting next staker change time: %w", err)
 	}
@@ -39,37 +42,33 @@ func NextBlockTime(state Chain, clk *mockable.Clock) (time.Time, bool, error) {
 }
 
 // GetNextStakerChangeTime returns the next time a staker will be either added
-// or removed to/from the current validator set.
-func GetNextStakerChangeTime(state Chain) (time.Time, error) {
-	currentStakerIterator, err := state.GetCurrentStakerIterator()
+// or removed to/from the current validator set. If the next staker change time
+// is further in the future than [defaultTime], then [defaultTime] is returned.
+func GetNextStakerChangeTime(state Chain, defaultTime time.Time) (time.Time, error) {
+	currentIterator, err := state.GetCurrentStakerIterator()
 	if err != nil {
 		return time.Time{}, err
 	}
-	defer currentStakerIterator.Release()
+	defer currentIterator.Release()
 
-	pendingStakerIterator, err := state.GetPendingStakerIterator()
+	pendingIterator, err := state.GetPendingStakerIterator()
 	if err != nil {
 		return time.Time{}, err
 	}
-	defer pendingStakerIterator.Release()
+	defer pendingIterator.Release()
 
-	hasCurrentStaker := currentStakerIterator.Next()
-	hasPendingStaker := pendingStakerIterator.Next()
-	switch {
-	case hasCurrentStaker && hasPendingStaker:
-		nextCurrentTime := currentStakerIterator.Value().NextTime
-		nextPendingTime := pendingStakerIterator.Value().NextTime
-		if nextCurrentTime.Before(nextPendingTime) {
-			return nextCurrentTime, nil
+	for _, it := range []iterator.Iterator[*Staker]{currentIterator, pendingIterator} {
+		// If the iterator is empty, skip it
+		if !it.Next() {
+			continue
+		}
+
+		time := it.Value().NextTime
+		if time.Before(defaultTime) {
+			defaultTime = time
 		}
-		return nextPendingTime, nil
-	case hasCurrentStaker:
-		return currentStakerIterator.Value().NextTime, nil
-	case hasPendingStaker:
-		return pendingStakerIterator.Value().NextTime, nil
-	default:
-		return time.Time{}, database.ErrNotFound
 	}
+	return defaultTime, nil
 }
 
 // PickFeeCalculator creates either a static or a dynamic fee calculator,

vms/platformvm/state/chain_time_helpers_test.go

@@ -5,16 +5,131 @@ package state
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 
 	"github.com/ava-labs/avalanchego/database/memdb"
 	"github.com/ava-labs/avalanchego/genesis"
+	"github.com/ava-labs/avalanchego/ids"
 	"github.com/ava-labs/avalanchego/upgrade/upgradetest"
+	"github.com/ava-labs/avalanchego/utils/constants"
+	"github.com/ava-labs/avalanchego/utils/timer/mockable"
 	"github.com/ava-labs/avalanchego/vms/platformvm/config"
+	"github.com/ava-labs/avalanchego/vms/platformvm/genesis/genesistest"
+	"github.com/ava-labs/avalanchego/vms/platformvm/txs"
 	"github.com/ava-labs/avalanchego/vms/platformvm/txs/fee"
 )
 
+func TestNextBlockTime(t *testing.T) {
+	tests := []struct {
+		name           string
+		chainTime      time.Time
+		now            time.Time
+		expectedTime   time.Time
+		expectedCapped bool
+	}{
+		{
+			name:           "parent time is after now",
+			chainTime:      genesistest.DefaultValidatorStartTime,
+			now:            genesistest.DefaultValidatorStartTime.Add(-time.Second),
+			expectedTime:   genesistest.DefaultValidatorStartTime,
+			expectedCapped: false,
+		},
+		{
+			name:           "parent time is before now",
+			chainTime:      genesistest.DefaultValidatorStartTime,
+			now:            genesistest.DefaultValidatorStartTime.Add(time.Second),
+			expectedTime:   genesistest.DefaultValidatorStartTime.Add(time.Second),
+			expectedCapped: false,
+		},
+		{
+			name:           "now is at next staker change time",
+			chainTime:      genesistest.DefaultValidatorStartTime,
+			now:            genesistest.DefaultValidatorEndTime,
+			expectedTime:   genesistest.DefaultValidatorEndTime,
+			expectedCapped: true,
+		},
+		{
+			name:           "now is after next staker change time",
+			chainTime:      genesistest.DefaultValidatorStartTime,
+			now:            genesistest.DefaultValidatorEndTime.Add(time.Second),
+			expectedTime:   genesistest.DefaultValidatorEndTime,
+			expectedCapped: true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var (
+				require = require.New(t)
+				s       = newTestState(t, memdb.New())
+				clk     mockable.Clock
+			)
+
+			s.SetTimestamp(test.chainTime)
+			clk.Set(test.now)
+
+			actualTime, actualCapped, err := NextBlockTime(s, &clk)
+			require.NoError(err)
+			require.Equal(test.expectedTime.Local(), actualTime.Local())
+			require.Equal(test.expectedCapped, actualCapped)
+		})
+	}
+}
+
+func TestGetNextStakerChangeTime(t *testing.T) {
+	tests := []struct {
+		name     string
+		pending  []*Staker
+		maxTime  time.Time
+		expected time.Time
+	}{
+		{
+			name:     "only current validators",
+			maxTime:  mockable.MaxTime,
+			expected: genesistest.DefaultValidatorEndTime,
+		},
+		{
+			name: "current and pending validators",
+			pending: []*Staker{
+				{
+					TxID:      ids.GenerateTestID(),
+					NodeID:    ids.GenerateTestNodeID(),
+					PublicKey: nil,
+					SubnetID:  constants.PrimaryNetworkID,
+					Weight:    1,
+					StartTime: genesistest.DefaultValidatorStartTime.Add(time.Second),
+					EndTime:   genesistest.DefaultValidatorEndTime,
+					NextTime:  genesistest.DefaultValidatorStartTime.Add(time.Second),
+					Priority:  txs.PrimaryNetworkValidatorPendingPriority,
+				},
+			},
+			maxTime:  mockable.MaxTime,
+			expected: genesistest.DefaultValidatorStartTime.Add(time.Second),
+		},
+		{
+			name:     "restricted timestamp",
+			maxTime:  genesistest.DefaultValidatorStartTime,
+			expected: genesistest.DefaultValidatorStartTime,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var (
+				require = require.New(t)
+				s       = newTestState(t, memdb.New())
+			)
+			for _, staker := range test.pending {
+				require.NoError(s.PutPendingValidator(staker))
+			}
+
+			actual, err := GetNextStakerChangeTime(s, test.maxTime)
+			require.NoError(err)
+			require.Equal(test.expected.Local(), actual.Local())
+		})
+	}
+}
+
 func TestPickFeeCalculator(t *testing.T) {
 	var (
 		createAssetTxFee = genesis.LocalParams.CreateAssetTxFee

vms/platformvm/txs/executor/advance_time_test.go

@@ -99,12 +99,13 @@ func TestAdvanceTimeTxUpdatePrimaryNetworkStakers(t *testing.T) {
 	require.True(ok)
 }
 
-// Ensure semantic verification fails when proposed timestamp is at or before current timestamp
+// Ensure semantic verification fails when proposed timestamp is before the
+// current timestamp
 func TestAdvanceTimeTxTimestampTooEarly(t *testing.T) {
 	require := require.New(t)
 	env := newEnvironment(t, upgradetest.ApricotPhase5)
 
-	tx, err := newAdvanceTimeTx(t, env.state.GetTimestamp())
+	tx, err := newAdvanceTimeTx(t, env.state.GetTimestamp().Add(-time.Second))
 	require.NoError(err)
 
 	onCommitState, err := state.NewDiff(lastAcceptedID, env)
@@ -122,10 +123,11 @@ func TestAdvanceTimeTxTimestampTooEarly(t *testing.T) {
 		Tx:            tx,
 	}
 	err = tx.Unsigned.Visit(&executor)
-	require.ErrorIs(err, ErrChildBlockNotAfterParent)
+	require.ErrorIs(err, ErrChildBlockEarlierThanParent)
 }
 
-// Ensure semantic verification fails when proposed timestamp is after next validator set change time
+// Ensure semantic verification fails when proposed timestamp is after next
+// validator set change time
 func TestAdvanceTimeTxTimestampTooLate(t *testing.T) {
 	require := require.New(t)
 	env := newEnvironment(t, upgradetest.ApricotPhase5)
@@ -166,8 +168,8 @@ func TestAdvanceTimeTxTimestampTooLate(t *testing.T) {
 	env.ctx.Lock.Lock()
 	defer env.ctx.Lock.Unlock()
 
-	// fast forward clock to 10 seconds before genesis validators stop validating
-	env.clk.Set(genesistest.DefaultValidatorEndTime.Add(-10 * time.Second))
+	// fast forward clock to when genesis validators stop validating
+	env.clk.Set(genesistest.DefaultValidatorEndTime)
 
 	{
 		// Proposes advancing timestamp to 1 second after genesis validators stop validating

vms/platformvm/txs/executor/proposal_tx_executor.go

@@ -34,7 +34,6 @@ var (
 
 	ErrRemoveStakerTooEarly          = errors.New("attempting to remove staker before their end time")
 	ErrRemoveWrongStaker             = errors.New("attempting to remove wrong staker")
-	ErrChildBlockNotAfterParent      = errors.New("proposed timestamp not after current chain time")
 	ErrInvalidState                  = errors.New("generated output isn't valid state")
 	ErrShouldBePermissionlessStaker  = errors.New("expected permissionless staker")
 	ErrWrongTxType                   = errors.New("wrong transaction type")
@@ -270,34 +269,17 @@ func (e *ProposalTxExecutor) AdvanceTimeTx(tx *txs.AdvanceTimeTx) error {
 		)
 	}
 
-	parentChainTime := e.OnCommitState.GetTimestamp()
-	if !newChainTime.After(parentChainTime) {
-		return fmt.Errorf(
-			"%w, proposed timestamp (%s), chain time (%s)",
-			ErrChildBlockNotAfterParent,
-			parentChainTime,
-			parentChainTime,
-		)
-	}
-
-	// Only allow timestamp to move forward as far as the time of next staker
-	// set change time
-	nextStakerChangeTime, err := state.GetNextStakerChangeTime(e.OnCommitState)
-	if err != nil {
-		return err
-	}
-
 	now := e.Clk.Time()
 	if err := VerifyNewChainTime(
 		newChainTime,
-		nextStakerChangeTime,
 		now,
+		e.OnCommitState,
 	); err != nil {
 		return err
 	}
 
 	// Note that state doesn't change if this proposal is aborted
-	_, err = AdvanceTimeTo(e.Backend, e.OnCommitState, newChainTime)
+	_, err := AdvanceTimeTo(e.Backend, e.OnCommitState, newChainTime)
 	return err
 }