From c11accdc01890ed8cb5948012daa47686f72e3fd Mon Sep 17 00:00:00 2001 From: Alberto Benegiamo Date: Mon, 4 Dec 2023 09:16:01 -0700 Subject: [PATCH] Drop Pending Stakers 0 - De-duplicate staking tx verification (#2335) --- tests/e2e/p/staking_rewards.go | 18 +- vms/platformvm/block/builder/builder.go | 19 +- vms/platformvm/state/staker_test.go | 2 +- vms/platformvm/state/state.go | 25 +-- .../txs/executor/staker_tx_verification.go | 170 ++++++++---------- .../txs/executor/standard_tx_executor.go | 56 +++--- .../txs/executor/tx_mempool_verifier.go | 27 +-- 7 files changed, 151 insertions(+), 166 deletions(-) diff --git a/tests/e2e/p/staking_rewards.go b/tests/e2e/p/staking_rewards.go index f05b3bfbf64a..475c3de261dd 100644 --- a/tests/e2e/p/staking_rewards.go +++ b/tests/e2e/p/staking_rewards.go @@ -103,12 +103,18 @@ var _ = ginkgo.Describe("[Staking Rewards]", func() { betaNodeID, betaPOP, err := betaInfoClient.GetNodeID(e2e.DefaultContext()) require.NoError(err) + pvmClient := platformvm.NewClient(alphaNode.GetProcessContext().URI) + const ( delegationPercent = 0.10 // 10% delegationShare = reward.PercentDenominator * delegationPercent weight = 2_000 * units.Avax ) + ginkgo.By("retrieving supply before inserting validators") + supplyAtValidatorsStart, _, err := pvmClient.GetCurrentSupply(e2e.DefaultContext(), constants.PrimaryNetworkID) + require.NoError(err) + alphaValidatorStartTime := time.Now().Add(e2e.DefaultValidatorStartTimeDiff) alphaValidatorEndTime := alphaValidatorStartTime.Add(validationPeriod) tests.Outf("alpha node validation period starting at: %v\n", alphaValidatorStartTime) @@ -171,6 +177,10 @@ var _ = ginkgo.Describe("[Staking Rewards]", func() { require.NoError(err) }) + ginkgo.By("retrieving supply before inserting delegators") + supplyAtDelegatorsStart, _, err := pvmClient.GetCurrentSupply(e2e.DefaultContext(), constants.PrimaryNetworkID) + require.NoError(err) + gammaDelegatorStartTime := time.Now().Add(e2e.DefaultValidatorStartTimeDiff) tests.Outf("gamma delegation period starting at: %v\n", gammaDelegatorStartTime) @@ -227,8 +237,6 @@ var _ = ginkgo.Describe("[Staking Rewards]", func() { // delegation periods are shorter than the validation periods. time.Sleep(time.Until(betaValidatorEndTime)) - pvmClient := platformvm.NewClient(alphaNode.GetProcessContext().URI) - ginkgo.By("waiting until the alpha and beta nodes are no longer validators") e2e.Eventually(func() bool { validators, err := pvmClient.GetCurrentValidators(e2e.DefaultContext(), constants.PrimaryNetworkID, nil) @@ -270,11 +278,9 @@ var _ = ginkgo.Describe("[Staking Rewards]", func() { require.Len(rewardBalances, len(rewardKeys)) ginkgo.By("determining expected validation and delegation rewards") - currentSupply, _, err := pvmClient.GetCurrentSupply(e2e.DefaultContext(), constants.PrimaryNetworkID) - require.NoError(err) calculator := reward.NewCalculator(rewardConfig) - expectedValidationReward := calculator.Calculate(validationPeriod, weight, currentSupply) - potentialDelegationReward := calculator.Calculate(delegationPeriod, weight, currentSupply) + expectedValidationReward := calculator.Calculate(validationPeriod, weight, supplyAtValidatorsStart) + potentialDelegationReward := calculator.Calculate(delegationPeriod, weight, supplyAtDelegatorsStart) expectedDelegationFee, expectedDelegatorReward := reward.Split(potentialDelegationReward, delegationShare) ginkgo.By("checking expected rewards against actual rewards") diff --git a/vms/platformvm/block/builder/builder.go b/vms/platformvm/block/builder/builder.go index b8476e19b032..f16e9353a8ad 100644 --- a/vms/platformvm/block/builder/builder.go +++ b/vms/platformvm/block/builder/builder.go @@ -126,28 +126,11 @@ func (b *builder) buildBlock() (block.Block, error) { return nil, fmt.Errorf("%w: %s", state.ErrMissingParentState, preferredID) } - timestamp := b.txExecutorBackend.Clk.Time() - if parentTime := preferred.Timestamp(); parentTime.After(timestamp) { - timestamp = parentTime - } - // [timestamp] = max(now, parentTime) - - nextStakerChangeTime, err := txexecutor.GetNextStakerChangeTime(preferredState) + timestamp, timeWasCapped, err := txexecutor.NextBlockTime(preferredState, b.txExecutorBackend.Clk) if err != nil { return nil, fmt.Errorf("could not calculate next staker change time: %w", err) } - // timeWasCapped means that [timestamp] was reduced to - // [nextStakerChangeTime]. It is used as a flag for [buildApricotBlock] to - // be willing to issue an advanceTimeTx. It is also used as a flag for - // [buildBanffBlock] to force the issuance of an empty block to advance - // the time forward; if there are no available transactions. - timeWasCapped := !timestamp.Before(nextStakerChangeTime) - if timeWasCapped { - timestamp = nextStakerChangeTime - } - // [timestamp] = min(max(now, parentTime), nextStakerChangeTime) - return buildBlock( b, preferredID, diff --git a/vms/platformvm/state/staker_test.go b/vms/platformvm/state/staker_test.go index 747f442e5eda..a6faa4ab704f 100644 --- a/vms/platformvm/state/staker_test.go +++ b/vms/platformvm/state/staker_test.go @@ -144,7 +144,7 @@ func TestNewCurrentStaker(t *testing.T) { subnetID := ids.GenerateTestID() weight := uint64(12345) startTime := time.Now() - endTime := time.Now() + endTime := startTime.Add(time.Hour) potentialReward := uint64(54321) currentPriority := txs.SubnetPermissionedValidatorCurrentPriority diff --git a/vms/platformvm/state/state.go b/vms/platformvm/state/state.go index 4627fc706626..82ff0107762a 100644 --- a/vms/platformvm/state/state.go +++ b/vms/platformvm/state/state.go @@ -1448,7 +1448,12 @@ func (s *state) loadCurrentValidators() error { } tx, _, err := s.GetTx(txID) if err != nil { - return err + return fmt.Errorf("failed loading validator transaction txID %s, %w", txID, err) + } + + stakerTx, ok := tx.Unsigned.(txs.Staker) + if !ok { + return fmt.Errorf("expected tx type txs.Staker but got %T", tx.Unsigned) } metadataBytes := validatorIt.Value() @@ -1461,11 +1466,6 @@ func (s *state) loadCurrentValidators() error { return err } - stakerTx, ok := tx.Unsigned.(txs.Staker) - if !ok { - return fmt.Errorf("expected tx type txs.Staker but got %T", tx.Unsigned) - } - staker, err := NewCurrentStaker(txID, stakerTx, metadata.PotentialReward) if err != nil { return err @@ -1498,11 +1498,12 @@ func (s *state) loadCurrentValidators() error { } metadataBytes := subnetValidatorIt.Value() + startTime := stakerTx.StartTime() metadata := &validatorMetadata{ txID: txID, // use the start time as the fallback value // in case it's not stored in the database - LastUpdated: uint64(stakerTx.StartTime().Unix()), + LastUpdated: uint64(startTime.Unix()), } if err := parseValidatorMetadata(metadataBytes, metadata); err != nil { return err @@ -1538,6 +1539,11 @@ func (s *state) loadCurrentValidators() error { return err } + stakerTx, ok := tx.Unsigned.(txs.Staker) + if !ok { + return fmt.Errorf("expected tx type txs.Staker but got %T", tx.Unsigned) + } + metadata := &delegatorMetadata{ txID: txID, } @@ -1546,11 +1552,6 @@ func (s *state) loadCurrentValidators() error { return err } - stakerTx, ok := tx.Unsigned.(txs.Staker) - if !ok { - return fmt.Errorf("expected tx type txs.Staker but got %T", tx.Unsigned) - } - staker, err := NewCurrentStaker(txID, stakerTx, metadata.PotentialReward) if err != nil { return err diff --git a/vms/platformvm/txs/executor/staker_tx_verification.go b/vms/platformvm/txs/executor/staker_tx_verification.go index 7fae0e78a85b..8a0c6046d67f 100644 --- a/vms/platformvm/txs/executor/staker_tx_verification.go +++ b/vms/platformvm/txs/executor/staker_tx_verification.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "math" + "time" "github.com/ava-labs/avalanchego/database" "github.com/ava-labs/avalanchego/ids" @@ -92,7 +93,6 @@ func verifyAddValidatorTx( } duration := tx.Validator.Duration() - switch { case tx.Validator.Wght < backend.Config.MinValidatorStake: // Ensure validator is staking at least the minimum amount @@ -123,16 +123,12 @@ func verifyAddValidatorTx( return outs, nil } - currentTimestamp := chainState.GetTimestamp() - // Ensure the proposed validator starts after the current time - startTime := tx.StartTime() - if !currentTimestamp.Before(startTime) { - return nil, fmt.Errorf( - "%w: %s >= %s", - ErrTimestampNotBeforeStartTime, - currentTimestamp, - startTime, - ) + var ( + currentTimestamp = chainState.GetTimestamp() + startTime = tx.StartTime() + ) + if err := verifyStakerStartTime(currentTimestamp, startTime); err != nil { + return nil, err } _, err := GetValidator(chainState, constants.PrimaryNetworkID, tx.Validator.NodeID) @@ -165,14 +161,9 @@ func verifyAddValidatorTx( return nil, fmt.Errorf("%w: %w", ErrFlowCheckFailed, err) } - // Make sure the tx doesn't start too far in the future. This is done last - // to allow the verifier visitor to explicitly check for this error. - maxStartTime := currentTimestamp.Add(MaxFutureStartTime) - if startTime.After(maxStartTime) { - return nil, ErrFutureStakeTime - } - - return outs, nil + // verifyStakerStartsSoon is checked last to allow + // the verifier visitor to explicitly check for this error. + return outs, verifyStakerStartsSoon(currentTimestamp, startTime) } // verifyAddSubnetValidatorTx carries out the validation for an @@ -203,16 +194,12 @@ func verifyAddSubnetValidatorTx( return nil } - currentTimestamp := chainState.GetTimestamp() - // Ensure the proposed validator starts after the current timestamp - validatorStartTime := tx.StartTime() - if !currentTimestamp.Before(validatorStartTime) { - return fmt.Errorf( - "%w: %s >= %s", - ErrTimestampNotBeforeStartTime, - currentTimestamp, - validatorStartTime, - ) + var ( + currentTimestamp = chainState.GetTimestamp() + startTime = tx.StartTime() + ) + if err := verifyStakerStartTime(currentTimestamp, startTime); err != nil { + return err } _, err := GetValidator(chainState, tx.SubnetValidator.Subnet, tx.Validator.NodeID) @@ -255,14 +242,9 @@ func verifyAddSubnetValidatorTx( return fmt.Errorf("%w: %w", ErrFlowCheckFailed, err) } - // Make sure the tx doesn't start too far in the future. This is done last - // to allow the verifier visitor to explicitly check for this error. - maxStartTime := currentTimestamp.Add(MaxFutureStartTime) - if validatorStartTime.After(maxStartTime) { - return ErrFutureStakeTime - } - - return nil + // verifyStakerStartsSoon is checked last to allow + // the verifier visitor to explicitly check for this error. + return verifyStakerStartsSoon(currentTimestamp, startTime) } // Returns the representation of [tx.NodeID] validating [tx.Subnet]. @@ -372,16 +354,12 @@ func verifyAddDelegatorTx( return outs, nil } - currentTimestamp := chainState.GetTimestamp() - // Ensure the proposed validator starts after the current timestamp - validatorStartTime := tx.StartTime() - if !currentTimestamp.Before(validatorStartTime) { - return nil, fmt.Errorf( - "%w: %s >= %s", - ErrTimestampNotBeforeStartTime, - currentTimestamp, - validatorStartTime, - ) + var ( + currentTimestamp = chainState.GetTimestamp() + startTime = tx.StartTime() + ) + if err := verifyStakerStartTime(currentTimestamp, startTime); err != nil { + return nil, err } primaryNetworkValidator, err := GetValidator(chainState, constants.PrimaryNetworkID, tx.Validator.NodeID) @@ -438,14 +416,9 @@ func verifyAddDelegatorTx( return nil, fmt.Errorf("%w: %w", ErrFlowCheckFailed, err) } - // Make sure the tx doesn't start too far in the future. This is done last - // to allow the verifier visitor to explicitly check for this error. - maxStartTime := currentTimestamp.Add(MaxFutureStartTime) - if validatorStartTime.After(maxStartTime) { - return nil, ErrFutureStakeTime - } - - return outs, nil + // verifyStakerStartsSoon is checked last to allow + // the verifier visitor to explicitly check for this error. + return outs, verifyStakerStartsSoon(currentTimestamp, startTime) } // verifyAddPermissionlessValidatorTx carries out the validation for an @@ -465,16 +438,12 @@ func verifyAddPermissionlessValidatorTx( return nil } - currentTimestamp := chainState.GetTimestamp() - // Ensure the proposed validator starts after the current time - startTime := tx.StartTime() - if !currentTimestamp.Before(startTime) { - return fmt.Errorf( - "%w: %s >= %s", - ErrTimestampNotBeforeStartTime, - currentTimestamp, - startTime, - ) + var ( + currentTimestamp = chainState.GetTimestamp() + startTime = tx.StartTime() + ) + if err := verifyStakerStartTime(currentTimestamp, startTime); err != nil { + return err } validatorRules, err := getValidatorRules(backend, chainState, tx.Subnet) @@ -482,8 +451,10 @@ func verifyAddPermissionlessValidatorTx( return err } - duration := tx.Validator.Duration() - stakedAssetID := tx.StakeOuts[0].AssetID() + var ( + duration = tx.Validator.Duration() + stakedAssetID = tx.StakeOuts[0].AssetID() + ) switch { case tx.Validator.Wght < validatorRules.minValidatorStake: // Ensure validator is staking at least the minimum amount @@ -562,14 +533,9 @@ func verifyAddPermissionlessValidatorTx( return fmt.Errorf("%w: %w", ErrFlowCheckFailed, err) } - // Make sure the tx doesn't start too far in the future. This is done last - // to allow the verifier visitor to explicitly check for this error. - maxStartTime := currentTimestamp.Add(MaxFutureStartTime) - if startTime.After(maxStartTime) { - return ErrFutureStakeTime - } - - return nil + // verifyStakerStartsSoon is checked last to allow + // the verifier visitor to explicitly check for this error. + return verifyStakerStartsSoon(currentTimestamp, startTime) } // verifyAddPermissionlessDelegatorTx carries out the validation for an @@ -589,15 +555,12 @@ func verifyAddPermissionlessDelegatorTx( return nil } - currentTimestamp := chainState.GetTimestamp() - // Ensure the proposed validator starts after the current timestamp - startTime := tx.StartTime() - if !currentTimestamp.Before(startTime) { - return fmt.Errorf( - "chain timestamp (%s) not before validator's start time (%s)", - currentTimestamp, - startTime, - ) + var ( + currentTimestamp = chainState.GetTimestamp() + startTime = tx.StartTime() + ) + if err := verifyStakerStartTime(currentTimestamp, startTime); err != nil { + return err } delegatorRules, err := getDelegatorRules(backend, chainState, tx.Subnet) @@ -605,8 +568,10 @@ func verifyAddPermissionlessDelegatorTx( return err } - duration := tx.Validator.Duration() - stakedAssetID := tx.StakeOuts[0].AssetID() + var ( + duration = tx.Validator.Duration() + stakedAssetID = tx.StakeOuts[0].AssetID() + ) switch { case tx.Validator.Wght < delegatorRules.minDelegatorStake: // Ensure delegator is staking at least the minimum amount @@ -706,14 +671,9 @@ func verifyAddPermissionlessDelegatorTx( return fmt.Errorf("%w: %w", ErrFlowCheckFailed, err) } - // Make sure the tx doesn't start too far in the future. This is done last - // to allow the verifier visitor to explicitly check for this error. - maxStartTime := currentTimestamp.Add(MaxFutureStartTime) - if startTime.After(maxStartTime) { - return ErrFutureStakeTime - } - - return nil + // verifyStakerStartsSoon is checked last to allow + // the verifier visitor to explicitly check for this error. + return verifyStakerStartsSoon(currentTimestamp, startTime) } // Returns an error if the given tx is invalid. @@ -762,3 +722,25 @@ func verifyTransferSubnetOwnershipTx( return nil } + +// Ensure the proposed validator starts after the current time +func verifyStakerStartTime(chainTime, stakerTime time.Time) error { + if !chainTime.Before(stakerTime) { + return fmt.Errorf( + "%w: %s >= %s", + ErrTimestampNotBeforeStartTime, + chainTime, + stakerTime, + ) + } + return nil +} + +func verifyStakerStartsSoon(chainTime, stakerStartTime time.Time) error { + // Make sure the tx doesn't start too far in the future. + maxStartTime := chainTime.Add(MaxFutureStartTime) + if stakerStartTime.After(maxStartTime) { + return ErrFutureStakeTime + } + return nil +} diff --git a/vms/platformvm/txs/executor/standard_tx_executor.go b/vms/platformvm/txs/executor/standard_tx_executor.go index 63069cb5d5d5..083e4f4c75c7 100644 --- a/vms/platformvm/txs/executor/standard_tx_executor.go +++ b/vms/platformvm/txs/executor/standard_tx_executor.go @@ -290,13 +290,11 @@ func (e *StandardTxExecutor) AddValidatorTx(tx *txs.AddValidatorTx) error { return err } - txID := e.Tx.ID() - newStaker, err := state.NewPendingStaker(txID, tx) - if err != nil { + if err := e.putStaker(tx); err != nil { return err } - e.State.PutPendingValidator(newStaker) + txID := e.Tx.ID() avax.Consume(e.State, tx.Ins) avax.Produce(e.State, txID, tx.Outs) @@ -321,16 +319,13 @@ func (e *StandardTxExecutor) AddSubnetValidatorTx(tx *txs.AddSubnetValidatorTx) return err } - txID := e.Tx.ID() - newStaker, err := state.NewPendingStaker(txID, tx) - if err != nil { + if err := e.putStaker(tx); err != nil { return err } - e.State.PutPendingValidator(newStaker) + txID := e.Tx.ID() avax.Consume(e.State, tx.Ins) avax.Produce(e.State, txID, tx.Outs) - return nil } @@ -344,16 +339,13 @@ func (e *StandardTxExecutor) AddDelegatorTx(tx *txs.AddDelegatorTx) error { return err } - txID := e.Tx.ID() - newStaker, err := state.NewPendingStaker(txID, tx) - if err != nil { + if err := e.putStaker(tx); err != nil { return err } - e.State.PutPendingDelegator(newStaker) + txID := e.Tx.ID() avax.Consume(e.State, tx.Ins) avax.Produce(e.State, txID, tx.Outs) - return nil } @@ -444,13 +436,11 @@ func (e *StandardTxExecutor) AddPermissionlessValidatorTx(tx *txs.AddPermissionl return err } - txID := e.Tx.ID() - newStaker, err := state.NewPendingStaker(txID, tx) - if err != nil { + if err := e.putStaker(tx); err != nil { return err } - e.State.PutPendingValidator(newStaker) + txID := e.Tx.ID() avax.Consume(e.State, tx.Ins) avax.Produce(e.State, txID, tx.Outs) @@ -478,16 +468,13 @@ func (e *StandardTxExecutor) AddPermissionlessDelegatorTx(tx *txs.AddPermissionl return err } - txID := e.Tx.ID() - newStaker, err := state.NewPendingStaker(txID, tx) - if err != nil { + if err := e.putStaker(tx); err != nil { return err } - e.State.PutPendingDelegator(newStaker) + txID := e.Tx.ID() avax.Consume(e.State, tx.Ins) avax.Produce(e.State, txID, tx.Outs) - return nil } @@ -511,7 +498,6 @@ func (e *StandardTxExecutor) TransferSubnetOwnershipTx(tx *txs.TransferSubnetOwn txID := e.Tx.ID() avax.Consume(e.State, tx.Ins) avax.Produce(e.State, txID, tx.Outs) - return nil } @@ -539,9 +525,29 @@ func (e *StandardTxExecutor) BaseTx(tx *txs.BaseTx) error { return err } + txID := e.Tx.ID() // Consume the UTXOS avax.Consume(e.State, tx.Ins) // Produce the UTXOS - avax.Produce(e.State, e.Tx.ID(), tx.Outs) + avax.Produce(e.State, txID, tx.Outs) + return nil +} + +// Creates the staker as defined in [stakerTx] and adds it to [e.State]. +func (e *StandardTxExecutor) putStaker(stakerTx txs.Staker) error { + txID := e.Tx.ID() + staker, err := state.NewPendingStaker(txID, stakerTx) + if err != nil { + return err + } + + switch priority := staker.Priority; { + case priority.IsPendingValidator(): + e.State.PutPendingValidator(staker) + case priority.IsPendingDelegator(): + e.State.PutPendingDelegator(staker) + default: + return fmt.Errorf("staker %s, unexpected priority %d", staker.TxID, priority) + } return nil } diff --git a/vms/platformvm/txs/executor/tx_mempool_verifier.go b/vms/platformvm/txs/executor/tx_mempool_verifier.go index f6eff499c2ec..378b742ae484 100644 --- a/vms/platformvm/txs/executor/tx_mempool_verifier.go +++ b/vms/platformvm/txs/executor/tx_mempool_verifier.go @@ -9,6 +9,7 @@ import ( "time" "github.com/ava-labs/avalanchego/ids" + "github.com/ava-labs/avalanchego/utils/timer/mockable" "github.com/ava-labs/avalanchego/vms/platformvm/state" "github.com/ava-labs/avalanchego/vms/platformvm/txs" ) @@ -108,7 +109,7 @@ func (v *MempoolTxVerifier) standardBaseState() (state.Diff, error) { return nil, err } - nextBlkTime, err := v.nextBlockTime(state) + nextBlkTime, _, err := NextBlockTime(state, v.Clk) if err != nil { return nil, err } @@ -123,20 +124,26 @@ func (v *MempoolTxVerifier) standardBaseState() (state.Diff, error) { return state, nil } -func (v *MempoolTxVerifier) nextBlockTime(state state.Diff) (time.Time, error) { +func NextBlockTime(state state.Chain, clk *mockable.Clock) (time.Time, bool, error) { var ( - parentTime = state.GetTimestamp() - nextBlkTime = v.Clk.Time() + timestamp = clk.Time() + parentTime = state.GetTimestamp() ) - if parentTime.After(nextBlkTime) { - nextBlkTime = parentTime + if parentTime.After(timestamp) { + timestamp = parentTime } + // [timestamp] = max(now, parentTime) + nextStakerChangeTime, err := GetNextStakerChangeTime(state) if err != nil { - return time.Time{}, fmt.Errorf("could not calculate next staker change time: %w", err) + return time.Time{}, false, fmt.Errorf("failed getting next staker change time: %w", err) } - if !nextBlkTime.Before(nextStakerChangeTime) { - nextBlkTime = nextStakerChangeTime + + // timeWasCapped means that [timestamp] was reduced to [nextStakerChangeTime] + timeWasCapped := !timestamp.Before(nextStakerChangeTime) + if timeWasCapped { + timestamp = nextStakerChangeTime } - return nextBlkTime, nil + // [timestamp] = min(max(now, parentTime), nextStakerChangeTime) + return timestamp, timeWasCapped, nil }