From 892aedcb39f939d4a621fc2391f165a299cdbac8 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Wed, 25 Sep 2024 12:16:56 -0400 Subject: [PATCH 1/7] Remove stale expiries in AdvanceTimeTo --- vms/platformvm/txs/executor/state_changes.go | 22 ++++ .../txs/executor/state_changes_test.go | 110 ++++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/vms/platformvm/txs/executor/state_changes.go b/vms/platformvm/txs/executor/state_changes.go index 55f940fca509..e11b1c049040 100644 --- a/vms/platformvm/txs/executor/state_changes.go +++ b/vms/platformvm/txs/executor/state_changes.go @@ -183,6 +183,28 @@ func AdvanceTimeTo( changes.SetFeeState(feeState) } + // Remove all expiries whose timestamp now implies they can never be + // re-issued. + // + // Recall that the expiry timestamp is the time at which it is no longer + // valid, so any expiry with a timestamp less than or equal to the new chain + // time can be removed. + expiryIterator, err := parentState.GetExpiryIterator() + if err != nil { + return false, err + } + defer expiryIterator.Release() + + newChainTimeUnix := uint64(newChainTime.Unix()) + for expiryIterator.Next() { + expiry := expiryIterator.Value() + if expiry.Timestamp > newChainTimeUnix { + break + } + + changes.DeleteExpiry(expiry) + } + changes.SetTimestamp(newChainTime) return changed, changes.Apply(parentState) } diff --git a/vms/platformvm/txs/executor/state_changes_test.go b/vms/platformvm/txs/executor/state_changes_test.go index 5588f4b7da73..fac281d44313 100644 --- a/vms/platformvm/txs/executor/state_changes_test.go +++ b/vms/platformvm/txs/executor/state_changes_test.go @@ -9,9 +9,12 @@ import ( "github.com/stretchr/testify/require" + "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/upgrade/upgradetest" + "github.com/ava-labs/avalanchego/utils/iterator" "github.com/ava-labs/avalanchego/vms/components/gas" "github.com/ava-labs/avalanchego/vms/platformvm/config" + "github.com/ava-labs/avalanchego/vms/platformvm/genesis/genesistest" "github.com/ava-labs/avalanchego/vms/platformvm/state" "github.com/ava-labs/avalanchego/vms/platformvm/state/statetest" ) @@ -98,3 +101,110 @@ func TestAdvanceTimeTo_UpdatesFeeState(t *testing.T) { }) } } + +func TestAdvanceTimeTo_RemovesStaleExpiries(t *testing.T) { + var ( + currentTime = genesistest.DefaultValidatorStartTime + newTime = currentTime.Add(3 * time.Second) + newTimeUnix = uint64(newTime.Unix()) + ) + + tests := []struct { + name string + initialExpiries []state.ExpiryEntry + expectedExpiries []state.ExpiryEntry + }{ + { + name: "no expiries", + }, + { + name: "unexpired expiry", + initialExpiries: []state.ExpiryEntry{ + { + Timestamp: newTimeUnix + 1, + ValidationID: ids.ID{1}, + }, + }, + expectedExpiries: []state.ExpiryEntry{ + { + Timestamp: newTimeUnix + 1, + ValidationID: ids.ID{1}, + }, + }, + }, + { + name: "unexpired expiry at new time", + initialExpiries: []state.ExpiryEntry{ + { + Timestamp: newTimeUnix, + ValidationID: ids.GenerateTestID(), + }, + }, + }, + { + name: "unexpired expiry at previous time", + initialExpiries: []state.ExpiryEntry{ + { + Timestamp: newTimeUnix - 1, + ValidationID: ids.GenerateTestID(), + }, + }, + }, + { + name: "limit expiries removed", + initialExpiries: []state.ExpiryEntry{ + { + Timestamp: newTimeUnix, + ValidationID: ids.GenerateTestID(), + }, + { + Timestamp: newTimeUnix + 1, + ValidationID: ids.ID{1}, + }, + }, + expectedExpiries: []state.ExpiryEntry{ + { + Timestamp: newTimeUnix + 1, + ValidationID: ids.ID{1}, + }, + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var ( + require = require.New(t) + s = statetest.New(t, statetest.Config{}) + ) + + // Ensure the invariant that [newTime <= nextStakerChangeTime] on + // AdvanceTimeTo is maintained. + nextStakerChangeTime, err := state.GetNextStakerChangeTime(s) + require.NoError(err) + require.False(newTime.After(nextStakerChangeTime)) + + for _, expiry := range test.initialExpiries { + s.PutExpiry(expiry) + } + + validatorsModified, err := AdvanceTimeTo( + &Backend{ + Config: &config.Config{ + UpgradeConfig: upgradetest.GetConfig(upgradetest.Latest), + }, + }, + s, + newTime, + ) + require.NoError(err) + require.False(validatorsModified) + + expiryIterator, err := s.GetExpiryIterator() + require.NoError(err) + require.Equal( + test.expectedExpiries, + iterator.ToSlice(expiryIterator), + ) + }) + } +} From e6dfa27c59816ba78ad3348c19b96d00af760ad7 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Wed, 25 Sep 2024 12:23:47 -0400 Subject: [PATCH 2/7] add ref --- vms/platformvm/txs/executor/state_changes.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/vms/platformvm/txs/executor/state_changes.go b/vms/platformvm/txs/executor/state_changes.go index e11b1c049040..7e6ae53b812d 100644 --- a/vms/platformvm/txs/executor/state_changes.go +++ b/vms/platformvm/txs/executor/state_changes.go @@ -186,9 +186,11 @@ func AdvanceTimeTo( // Remove all expiries whose timestamp now implies they can never be // re-issued. // - // Recall that the expiry timestamp is the time at which it is no longer - // valid, so any expiry with a timestamp less than or equal to the new chain - // time can be removed. + // The expiry timestamp is the time at which it is no longer valid, so any + // expiry with a timestamp less than or equal to the new chain time can be + // removed. + // + // Ref: https://github.com/avalanche-foundation/ACPs/tree/main/ACPs/77-reinventing-subnets#registersubnetvalidatortx expiryIterator, err := parentState.GetExpiryIterator() if err != nil { return false, err From 2ca8774792d4f44502cbb3caab9b75355212f112 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Wed, 25 Sep 2024 13:09:12 -0400 Subject: [PATCH 3/7] nits --- vms/platformvm/txs/executor/state_changes.go | 2 ++ .../txs/executor/state_changes_test.go | 27 +++++++++++-------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/vms/platformvm/txs/executor/state_changes.go b/vms/platformvm/txs/executor/state_changes.go index c2119a127f76..d663e081d448 100644 --- a/vms/platformvm/txs/executor/state_changes.go +++ b/vms/platformvm/txs/executor/state_changes.go @@ -209,6 +209,8 @@ func AdvanceTimeTo( // removed. // // Ref: https://github.com/avalanche-foundation/ACPs/tree/main/ACPs/77-reinventing-subnets#registersubnetvalidatortx + // + // The expiry iterator is sorted in order of increasing timestamp. expiryIterator, err := parentState.GetExpiryIterator() if err != nil { return false, err diff --git a/vms/platformvm/txs/executor/state_changes_test.go b/vms/platformvm/txs/executor/state_changes_test.go index 80b80d1832ed..15576b306ec9 100644 --- a/vms/platformvm/txs/executor/state_changes_test.go +++ b/vms/platformvm/txs/executor/state_changes_test.go @@ -108,6 +108,11 @@ func TestAdvanceTimeTo_RemovesStaleExpiries(t *testing.T) { currentTime = genesistest.DefaultValidatorStartTime newTime = currentTime.Add(3 * time.Second) newTimeUnix = uint64(newTime.Unix()) + + unexpiredTime = newTimeUnix + 1 + expiredTime = newTimeUnix + previouslyExpiredTime = newTimeUnix - 1 + validationID = ids.GenerateTestID() ) tests := []struct { @@ -122,14 +127,14 @@ func TestAdvanceTimeTo_RemovesStaleExpiries(t *testing.T) { name: "unexpired expiry", initialExpiries: []state.ExpiryEntry{ { - Timestamp: newTimeUnix + 1, - ValidationID: ids.ID{1}, + Timestamp: unexpiredTime, + ValidationID: validationID, }, }, expectedExpiries: []state.ExpiryEntry{ { - Timestamp: newTimeUnix + 1, - ValidationID: ids.ID{1}, + Timestamp: unexpiredTime, + ValidationID: validationID, }, }, }, @@ -137,7 +142,7 @@ func TestAdvanceTimeTo_RemovesStaleExpiries(t *testing.T) { name: "unexpired expiry at new time", initialExpiries: []state.ExpiryEntry{ { - Timestamp: newTimeUnix, + Timestamp: expiredTime, ValidationID: ids.GenerateTestID(), }, }, @@ -146,7 +151,7 @@ func TestAdvanceTimeTo_RemovesStaleExpiries(t *testing.T) { name: "unexpired expiry at previous time", initialExpiries: []state.ExpiryEntry{ { - Timestamp: newTimeUnix - 1, + Timestamp: previouslyExpiredTime, ValidationID: ids.GenerateTestID(), }, }, @@ -155,18 +160,18 @@ func TestAdvanceTimeTo_RemovesStaleExpiries(t *testing.T) { name: "limit expiries removed", initialExpiries: []state.ExpiryEntry{ { - Timestamp: newTimeUnix, + Timestamp: expiredTime, ValidationID: ids.GenerateTestID(), }, { - Timestamp: newTimeUnix + 1, - ValidationID: ids.ID{1}, + Timestamp: unexpiredTime, + ValidationID: validationID, }, }, expectedExpiries: []state.ExpiryEntry{ { - Timestamp: newTimeUnix + 1, - ValidationID: ids.ID{1}, + Timestamp: unexpiredTime, + ValidationID: validationID, }, }, }, From 37f6108f7fec1601e9c1841aa264aa7e30169bcd Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Wed, 25 Sep 2024 13:15:12 -0400 Subject: [PATCH 4/7] Fix race --- vms/platformvm/txs/executor/state_changes.go | 38 ++++++++++++++------ 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/vms/platformvm/txs/executor/state_changes.go b/vms/platformvm/txs/executor/state_changes.go index d663e081d448..9ddadf75f1f1 100644 --- a/vms/platformvm/txs/executor/state_changes.go +++ b/vms/platformvm/txs/executor/state_changes.go @@ -77,12 +77,30 @@ func VerifyNewChainTime( // AdvanceTimeTo applies all state changes to [parentState] resulting from // advancing the chain time to [newChainTime]. +// // Returns true iff the validator set changed. func AdvanceTimeTo( backend *Backend, parentState state.Chain, newChainTime time.Time, ) (bool, error) { + diff, changed, err := advanceTimeTo(backend, parentState, newChainTime) + if err != nil { + return false, err + } + return changed, diff.Apply(parentState) +} + +// advanceTimeTo returns the state diff on top of parentState resulting from +// advancing the chain time to newChainTime. It also returns a boolean +// indicating if the validator set changed. +// +// parentState is not modified. +func advanceTimeTo( + backend *Backend, + parentState state.Chain, + newChainTime time.Time, +) (state.Diff, bool, error) { // We promote pending stakers to current stakers first and remove // completed stakers from the current staker set. We assume that any // promoted staker will not immediately be removed from the current staker @@ -93,12 +111,12 @@ func AdvanceTimeTo( changes, err := state.NewDiffOn(parentState) if err != nil { - return false, err + return nil, false, err } pendingStakerIterator, err := parentState.GetPendingStakerIterator() if err != nil { - return false, err + return nil, false, err } defer pendingStakerIterator.Release() @@ -116,7 +134,7 @@ func AdvanceTimeTo( if stakerToRemove.Priority == txs.SubnetPermissionedValidatorPendingPriority { if err := changes.PutCurrentValidator(&stakerToAdd); err != nil { - return false, err + return nil, false, err } changes.DeletePendingValidator(stakerToRemove) changed = true @@ -125,12 +143,12 @@ func AdvanceTimeTo( supply, err := changes.GetCurrentSupply(stakerToRemove.SubnetID) if err != nil { - return false, err + return nil, false, err } rewards, err := GetRewardsCalculator(backend, parentState, stakerToRemove.SubnetID) if err != nil { - return false, err + return nil, false, err } potentialReward := rewards.Calculate( @@ -147,7 +165,7 @@ func AdvanceTimeTo( switch stakerToRemove.Priority { case txs.PrimaryNetworkValidatorPendingPriority, txs.SubnetPermissionlessValidatorPendingPriority: if err := changes.PutCurrentValidator(&stakerToAdd); err != nil { - return false, err + return nil, false, err } changes.DeletePendingValidator(stakerToRemove) @@ -156,7 +174,7 @@ func AdvanceTimeTo( changes.DeletePendingDelegator(stakerToRemove) default: - return false, fmt.Errorf("expected staker priority got %d", stakerToRemove.Priority) + return nil, false, fmt.Errorf("expected staker priority got %d", stakerToRemove.Priority) } changed = true @@ -165,7 +183,7 @@ func AdvanceTimeTo( // Remove any current stakers whose [EndTime] <= [newChainTime]. currentStakerIterator, err := parentState.GetCurrentStakerIterator() if err != nil { - return false, err + return nil, false, err } defer currentStakerIterator.Release() @@ -213,7 +231,7 @@ func AdvanceTimeTo( // The expiry iterator is sorted in order of increasing timestamp. expiryIterator, err := parentState.GetExpiryIterator() if err != nil { - return false, err + return nil, false, err } defer expiryIterator.Release() @@ -228,7 +246,7 @@ func AdvanceTimeTo( } changes.SetTimestamp(newChainTime) - return changed, changes.Apply(parentState) + return changes, changed, nil } func GetRewardsCalculator( From 58fc67a53d748866d3ec565a10057fc91ff2fbc1 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Wed, 25 Sep 2024 13:23:39 -0400 Subject: [PATCH 5/7] Document invariant --- vms/platformvm/txs/executor/state_changes.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/vms/platformvm/txs/executor/state_changes.go b/vms/platformvm/txs/executor/state_changes.go index 9ddadf75f1f1..0dda483228de 100644 --- a/vms/platformvm/txs/executor/state_changes.go +++ b/vms/platformvm/txs/executor/state_changes.go @@ -114,6 +114,11 @@ func advanceTimeTo( return nil, false, err } + // Promote any pending stakers to current if [StartTime] <= [newChainTime]. + // + // Invariant: It is not safe to modify the state while iterating over it, + // so we use the parentState's iterator rather than the changes iterator. + // ParentState must not be modified before this iterator is released. pendingStakerIterator, err := parentState.GetPendingStakerIterator() if err != nil { return nil, false, err @@ -121,7 +126,6 @@ func advanceTimeTo( defer pendingStakerIterator.Release() var changed bool - // Promote any pending stakers to current if [StartTime] <= [newChainTime]. for pendingStakerIterator.Next() { stakerToRemove := pendingStakerIterator.Value() if stakerToRemove.StartTime.After(newChainTime) { @@ -181,6 +185,10 @@ func advanceTimeTo( } // Remove any current stakers whose [EndTime] <= [newChainTime]. + // + // Invariant: It is not safe to modify the state while iterating over it, + // so we use the parentState's iterator rather than the changes iterator. + // ParentState must not be modified before this iterator is released. currentStakerIterator, err := parentState.GetCurrentStakerIterator() if err != nil { return nil, false, err @@ -229,6 +237,10 @@ func advanceTimeTo( // Ref: https://github.com/avalanche-foundation/ACPs/tree/main/ACPs/77-reinventing-subnets#registersubnetvalidatortx // // The expiry iterator is sorted in order of increasing timestamp. + // + // Invariant: It is not safe to modify the state while iterating over it, + // so we use the parentState's iterator rather than the changes iterator. + // ParentState must not be modified before this iterator is released. expiryIterator, err := parentState.GetExpiryIterator() if err != nil { return nil, false, err From f31ea171c51926b6f3be6800529a78ed137267ca Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Wed, 25 Sep 2024 13:36:19 -0400 Subject: [PATCH 6/7] fix unit test --- .../block/executor/standard_block_test.go | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/vms/platformvm/block/executor/standard_block_test.go b/vms/platformvm/block/executor/standard_block_test.go index 065bd2098616..8e62937c9239 100644 --- a/vms/platformvm/block/executor/standard_block_test.go +++ b/vms/platformvm/block/executor/standard_block_test.go @@ -18,7 +18,7 @@ import ( "github.com/ava-labs/avalanchego/upgrade/upgradetest" "github.com/ava-labs/avalanchego/utils/constants" "github.com/ava-labs/avalanchego/utils/crypto/secp256k1" - "github.com/ava-labs/avalanchego/utils/iterator/iteratormock" + "github.com/ava-labs/avalanchego/utils/iterator" "github.com/ava-labs/avalanchego/vms/components/avax" "github.com/ava-labs/avalanchego/vms/components/gas" "github.com/ava-labs/avalanchego/vms/platformvm/block" @@ -118,22 +118,19 @@ func TestBanffStandardBlockTimeVerification(t *testing.T) { nextStakerTime := chainTime.Add(executor.SyncBound).Add(-1 * time.Second) // store just once current staker to mark next staker time. - currentStakerIt := iteratormock.NewIterator[*state.Staker](ctrl) - currentStakerIt.EXPECT().Next().Return(true).AnyTimes() - currentStakerIt.EXPECT().Value().Return( - &state.Staker{ - NextTime: nextStakerTime, - Priority: txs.PrimaryNetworkValidatorCurrentPriority, - }, - ).AnyTimes() - currentStakerIt.EXPECT().Release().Return().AnyTimes() - onParentAccept.EXPECT().GetCurrentStakerIterator().Return(currentStakerIt, nil).AnyTimes() + onParentAccept.EXPECT().GetCurrentStakerIterator().DoAndReturn(func() (iterator.Iterator[*state.Staker], error) { + return iterator.FromSlice( + &state.Staker{ + NextTime: nextStakerTime, + Priority: txs.PrimaryNetworkValidatorCurrentPriority, + }, + ), nil + }).AnyTimes() // no pending stakers - pendingIt := iteratormock.NewIterator[*state.Staker](ctrl) - pendingIt.EXPECT().Next().Return(false).AnyTimes() - pendingIt.EXPECT().Release().Return().AnyTimes() - onParentAccept.EXPECT().GetPendingStakerIterator().Return(pendingIt, nil).AnyTimes() + onParentAccept.EXPECT().GetPendingStakerIterator().Return(iterator.Empty[*state.Staker]{}, nil).AnyTimes() + // no expiries + onParentAccept.EXPECT().GetExpiryIterator().Return(iterator.Empty[state.ExpiryEntry]{}, nil).AnyTimes() onParentAccept.EXPECT().GetTimestamp().Return(chainTime).AnyTimes() onParentAccept.EXPECT().GetFeeState().Return(gas.State{}).AnyTimes() From c6b18ec4069ab5263e14e75b8279be5d65be1555 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Wed, 25 Sep 2024 13:49:07 -0400 Subject: [PATCH 7/7] fix unit test --- .../block/executor/proposal_block_test.go | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/vms/platformvm/block/executor/proposal_block_test.go b/vms/platformvm/block/executor/proposal_block_test.go index 41553c151309..ec9fa0afab93 100644 --- a/vms/platformvm/block/executor/proposal_block_test.go +++ b/vms/platformvm/block/executor/proposal_block_test.go @@ -20,6 +20,7 @@ import ( "github.com/ava-labs/avalanchego/utils/constants" "github.com/ava-labs/avalanchego/utils/crypto/bls" "github.com/ava-labs/avalanchego/utils/crypto/secp256k1" + "github.com/ava-labs/avalanchego/utils/iterator" "github.com/ava-labs/avalanchego/utils/iterator/iteratormock" "github.com/ava-labs/avalanchego/utils/timer/mockable" "github.com/ava-labs/avalanchego/vms/components/avax" @@ -205,24 +206,21 @@ func TestBanffProposalBlockTimeVerification(t *testing.T) { nextStakerTxID := nextStakerTx.ID() onParentAccept.EXPECT().GetTx(nextStakerTxID).Return(nextStakerTx, status.Processing, nil) - currentStakersIt := iteratormock.NewIterator[*state.Staker](ctrl) - currentStakersIt.EXPECT().Next().Return(true).AnyTimes() - currentStakersIt.EXPECT().Value().Return(&state.Staker{ - TxID: nextStakerTxID, - EndTime: nextStakerTime, - NextTime: nextStakerTime, - Priority: txs.PrimaryNetworkValidatorCurrentPriority, + onParentAccept.EXPECT().GetCurrentStakerIterator().DoAndReturn(func() (iterator.Iterator[*state.Staker], error) { + return iterator.FromSlice( + &state.Staker{ + TxID: nextStakerTxID, + EndTime: nextStakerTime, + NextTime: nextStakerTime, + Priority: txs.PrimaryNetworkValidatorCurrentPriority, + }, + ), nil }).AnyTimes() - currentStakersIt.EXPECT().Release().AnyTimes() - onParentAccept.EXPECT().GetCurrentStakerIterator().Return(currentStakersIt, nil).AnyTimes() + onParentAccept.EXPECT().GetPendingStakerIterator().Return(iterator.Empty[*state.Staker]{}, nil).AnyTimes() + onParentAccept.EXPECT().GetExpiryIterator().Return(iterator.Empty[state.ExpiryEntry]{}, nil).AnyTimes() onParentAccept.EXPECT().GetDelegateeReward(constants.PrimaryNetworkID, unsignedNextStakerTx.NodeID()).Return(uint64(0), nil).AnyTimes() - pendingStakersIt := iteratormock.NewIterator[*state.Staker](ctrl) - pendingStakersIt.EXPECT().Next().Return(false).AnyTimes() // no pending stakers - pendingStakersIt.EXPECT().Release().AnyTimes() - onParentAccept.EXPECT().GetPendingStakerIterator().Return(pendingStakersIt, nil).AnyTimes() - env.mockedState.EXPECT().GetUptime(gomock.Any).Return( time.Microsecond, /*upDuration*/ time.Time{}, /*lastUpdated*/