Drop Pending Stakers 0 - De-duplicate staking tx verification #2335

abi87 · 2023-11-19T12:06:14Z

Why this should be merged

Nits to remove some minor code duplcation and simplify diffs down the line.

How this works

Slicing #2175 up.
Extract some duiplicated pieces of code. Minor cleanup nits

How this was tested

CI (it's a pure refactoring)

abi87 · 2023-11-19T12:11:10Z

vms/platformvm/block/builder/builder.go

 	preferredState, ok := b.blkManager.GetState(preferredID)
 	if !ok {
 		return nil, fmt.Errorf("%w: %s", state.ErrMissingParentState, preferredID)
 	}

-	timestamp := b.txExecutorBackend.Clk.Time()


The logic to pick next block time is duplicated here and it the mempool tx verification. This change removes the duplication

abi87 · 2023-11-19T12:11:44Z

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)


why not, feels cleaner. We don't allow zero-lenght staking periods in prod.

abi87 · 2023-11-19T12:12:21Z

vms/platformvm/state/state.go

@@ -1447,7 +1447,11 @@ func (s *state) loadCurrentValidators() error {
 		}
 		tx, _, err := s.GetTx(txID)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed loading validator transaction txID %v, %w", txID, err)


moved this check up of a few line, to reduce diffs later on.
This error should never happen, but always better failing fast

abi87 · 2023-11-19T12:12:57Z

vms/platformvm/txs/executor/staker_tx_verification.go

@@ -123,16 +128,8 @@ func verifyAddValidatorTx(
 		return outs, nil
 	}

-	currentTimestamp := chainState.GetTimestamp()


encapsulated this check to reduce diffs down the line. We also spare some code duplication

abi87 · 2023-11-19T12:13:35Z

vms/platformvm/txs/executor/staker_tx_verification.go

-	// 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


encapsulated this check to reduce diffs down the line. We also spare some code duplication

abi87 · 2023-11-19T12:14:09Z

vms/platformvm/txs/executor/standard_tx_executor.go

-	txID := e.Tx.ID()
-	newStaker, err := state.NewPendingStaker(txID, tx)
-	if err != nil {
+	if err := e.addStakerFromStakerTx(tx); err != nil {


encapsulated this check to reduce diffs down the line, when we'll change the way staking start time is picked

abi87 · 2023-11-19T12:19:33Z

tests/e2e/p/staking_rewards.go

 		const (
 			delegationPercent = 0.10 // 10%
 			delegationShare   = reward.PercentDenominator * delegationPercent
 			weight            = 2_000 * units.Avax
 		)

+		ginkgo.By("retrieving supply before inserting validators")


This is actually a fix of this e2e test that is currently passing. Stakers reward is a function of the avax supply at the time they are inserted. The tests changes reflect this facts.
Test still pass rn, but won't down the line when pending stakers will be dropped and we'll start stakers immediately

abi87 · 2023-11-19T22:23:22Z

vms/platformvm/txs/executor/tx_mempool_verifier.go

+	timestamp := clk.Time()
+	if parentTime := state.GetTimestamp(); 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("could not calculate next staker change time: %w", err)
 	}
-	if !nextBlkTime.Before(nextStakerChangeTime) {
-		nextBlkTime = nextStakerChangeTime
+
+	// 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
 	}
-	return nextBlkTime, nil
+	// [timestamp] = min(max(now, parentTime), nextStakerChangeTime)
+	return timestamp, timeWasCapped, nil


this is basically code from block builder since it's more rich in comments

abi87 · 2023-11-19T22:24:09Z

vms/platformvm/txs/executor/standard_tx_executor.go

@@ -545,3 +528,29 @@ func (e *StandardTxExecutor) BaseTx(tx *txs.BaseTx) error {
 	avax.Produce(e.State, e.Tx.ID(), tx.Outs)
 	return nil
 }
+
+// addStakerFromStakerTx creates the staker and adds it to state.
+func (e *StandardTxExecutor) addStakerFromStakerTx(stakerTx txs.Staker) error {


refactor here since down the line, when stakers will be promoted to current immediately, this is the only code that will change

marun

e2e changes lgtm

abi87 · 2023-11-20T12:21:23Z

vms/platformvm/state/state.go

@@ -1460,11 +1464,6 @@ func (s *state) loadCurrentValidators() error {
 			return err
 		}

-		stakerTx, ok := tx.Unsigned.(txs.Staker)


this is where the check was

abi87 · 2023-11-20T12:21:54Z

vms/platformvm/state/state.go

+			stakerTx, ok := tx.Unsigned.(txs.Staker)
+			if !ok {
+				return fmt.Errorf("expected tx type txs.Staker but got %T", tx.Unsigned)
+			}
+


moved this check up of a few line, to reduce diffs later on.
This error should never happen, but always better failing fast

abi87 · 2023-11-20T12:22:00Z

vms/platformvm/state/state.go

@@ -1545,11 +1550,6 @@ func (s *state) loadCurrentValidators() error {
 				return err
 			}

-			stakerTx, ok := tx.Unsigned.(txs.Staker)


this is where the check was

vms/platformvm/txs/executor/staker_tx_verification.go

vms/platformvm/block/builder/builder.go

vms/platformvm/txs/executor/tx_mempool_verifier.go

vms/platformvm/txs/executor/standard_tx_executor.go

vms/platformvm/block/builder/builder.go

vms/platformvm/state/state.go

vms/platformvm/txs/executor/staker_tx_verification.go

vms/platformvm/block/builder/builder.go

dhrubabasu

LGTM, put some optional stylistic nits here (and resolved the merge conflict): #2375

Maybe we should rename this PR to "De-duplicate staking tx verification" before merging?

vms/platformvm/state/state.go

vms/platformvm/txs/executor/standard_tx_executor.go

vms/platformvm/txs/executor/tx_mempool_verifier.go

commit b36416d Author: Alberto Benegiamo <alberto.benegiamo@gmail.com> Date: Wed Dec 6 12:12:11 2023 -0700 Drop Pending Stakers 1 - introduced ScheduledStaker txs (#2323) Co-authored-by: Stephen Buttolph <stephen@avalabs.org> commit 7df1f3a Author: Stephen Buttolph <stephen@avalabs.org> Date: Wed Dec 6 13:46:56 2023 -0500 Restrict GOPROXY (#2434) commit 21b7ab8 Author: Stephen Buttolph <stephen@avalabs.org> Date: Tue Dec 5 19:43:00 2023 -0500 Fix platformvm.SetPreference (#2429) commit ada692a Author: Stephen Buttolph <stephen@avalabs.org> Date: Tue Dec 5 17:40:03 2023 -0500 Update minimum golang version to v1.20.12 (#2427) commit 004a23e Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com> Date: Tue Dec 5 12:56:25 2023 -0500 `vms/platformvm`: Add `decisionTxs` parameter to `NewBanffProposalBlock` (#2411) Co-authored-by: Dan Laine <daniel.laine@avalabs.org> commit 439dc1e Author: Alberto Benegiamo <alberto.benegiamo@gmail.com> Date: Tue Dec 5 10:46:13 2023 -0700 ProposerVM Extend windows 0 - Cleanup (#2404) Co-authored-by: Stephen Buttolph <stephen@avalabs.org> commit 477157d Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com> Date: Tue Dec 5 11:10:56 2023 -0500 `vms/platformvm`: Cleanup some block tests (#2422) Co-authored-by: Dan Laine <daniel.laine@avalabs.org> commit b6700c9 Author: Stephen Buttolph <stephen@avalabs.org> Date: Mon Dec 4 22:24:34 2023 -0500 Update slices dependency to use Compare (#2424) Co-authored-by: James Walker <jim.walker@smartcontract.com> commit 5d9e482 Author: Dan Laine <daniel.laine@avalabs.org> Date: Mon Dec 4 19:27:29 2023 -0500 allow user of `build_fuzz.sh` to specify a directory to fuzz in (#2414) commit 2e32281 Author: Stephen Buttolph <stephen@avalabs.org> Date: Mon Dec 4 18:30:41 2023 -0500 Fix duplicated bootstrapper engine termination (#2334) Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Co-authored-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> commit b741c19 Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com> Date: Mon Dec 4 17:49:15 2023 -0500 `vms/platformvm`: Move `VerifyUniqueInputs` from `verifier` to `backend` (#2410) commit 6aa20fc Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com> Date: Mon Dec 4 17:32:07 2023 -0500 `vms/platformvm`: Initialize txs in `Transactions` field for `BanffProposalBlock` (#2419) commit c11accd Author: Alberto Benegiamo <alberto.benegiamo@gmail.com> Date: Mon Dec 4 09:16:01 2023 -0700 Drop Pending Stakers 0 - De-duplicate staking tx verification (#2335) commit 05ce366 Author: marun <maru.newby@avalabs.org> Date: Sat Dec 2 13:33:18 2023 -0800 testing: Update to latest version of ginkgo (#2390) commit 04af33e Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com> Date: Sat Dec 2 16:33:07 2023 -0500 `vms/platformvm`: Cleanup block builder tests (#2406) Co-authored-by: Stephen Buttolph <stephen@avalabs.org> commit 7623ffd Author: Stephen Buttolph <stephen@avalabs.org> Date: Fri Dec 1 19:39:15 2023 -0500 Update versions for v1.10.17 (#2394) commit be1a2ad Author: aaronbuchwald <aaron.buchwald56@gmail.com> Date: Fri Dec 1 13:31:49 2023 -0500 Add more descriptive formatted error (#2403) Co-authored-by: Stephen Buttolph <stephen@avalabs.org> commit 9b85141 Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com> Date: Thu Nov 30 12:42:36 2023 -0800 `vms/platformvm`: Move `GetRewardUTXOs`, `GetSubnets`, and `GetChains` to `State` interface (#2402) commit de3b16c Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu Nov 30 12:49:53 2023 -0500 Add `p2p.Network` component (#2283) Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Co-authored-by: Stephen Buttolph <stephen@avalabs.org> commit 0ab2046 Author: marun <maru.newby@avalabs.org> Date: Thu Nov 30 09:18:59 2023 -0800 Rename `testnet` fixture to `tmpnet` (#2307) commit 907b34c Author: Stephen Buttolph <stephen@avalabs.org> Date: Wed Nov 29 21:32:43 2023 -0500 Update bootstrap IPs (#2396) commit 96d451d Author: Stephen Buttolph <stephen@avalabs.org> Date: Wed Nov 29 18:15:47 2023 -0500 Periodically PullGossip only from connected validators (#2399) commit 0da5bcc Author: Dan Laine <daniel.laine@avalabs.org> Date: Wed Nov 29 16:32:16 2023 -0500 Remove method `CappedList` from `set.Set` (#2395) Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

abi87 added 2 commits November 19, 2023 12:55

nits to reduce future diffs

4c4c011

nit

905e55e

abi87 self-assigned this Nov 19, 2023

abi87 commented Nov 19, 2023

View reviewed changes

abi87 marked this pull request as ready for review November 19, 2023 12:14

abi87 requested review from danlaine and StephenButtolph as code owners November 19, 2023 12:14

improved e2e test

89094cd

abi87 requested review from gyuho and marun as code owners November 19, 2023 12:17

abi87 commented Nov 19, 2023

View reviewed changes

abi87 linked an issue Nov 19, 2023 that may be closed by this pull request

Drop Pending Stakers #2336

Closed

abi87 commented Nov 19, 2023

View reviewed changes

marun approved these changes Nov 20, 2023

View reviewed changes

abi87 commented Nov 20, 2023

View reviewed changes

Merge branch 'dev' into post_durango_nits_0

698af90

dhrubabasu reviewed Nov 21, 2023

View reviewed changes

nits

c3e3670

abi87 requested a review from dhrubabasu November 21, 2023 19:27

dhrubabasu reviewed Nov 21, 2023

View reviewed changes

Merge branch 'dev' into post_durango_nits_0

9606b77

abi87 force-pushed the post_durango_nits_0 branch from 5d0d2bd to 9599087 Compare November 22, 2023 08:51

abi87 requested a review from dhrubabasu November 22, 2023 08:55

dhrubabasu reviewed Nov 22, 2023

View reviewed changes

vms/platformvm/block/builder/builder.go Show resolved Hide resolved

dhrubabasu mentioned this pull request Nov 23, 2023

vms/platformvm: Remove MempoolTxVerifier #2362

Merged

abi87 added 3 commits November 23, 2023 01:21

Merge branch 'dev' into post_durango_nits_0

6ea5add

Merge branch 'dev' into post_durango_nits_0

8260546

nit

73d5c2f

abi87 requested a review from dhrubabasu November 27, 2023 10:57

dhrubabasu approved these changes Nov 27, 2023

View reviewed changes

abi87 added 2 commits November 28, 2023 08:38

Merge branch 'dev' into post_durango_nits_0

84321ef

nits

c353c33

abi87 changed the title ~~Drop Pending Stakers 0 - Nits to reduce forward diffs~~ De-duplicate staking tx verification Nov 28, 2023

abi87 changed the title ~~De-duplicate staking tx verification~~ Drop Pending Stakers 0 - De-duplicate staking tx verification Nov 28, 2023

abi87 mentioned this pull request Nov 28, 2023

nits #2375

Closed

abi87 added 4 commits November 28, 2023 10:54

Merge branch 'dev' into post_durango_nits_0

9bd9d60

Merge branch 'dev' into post_durango_nits_0

a99a5f7

Merge branch 'dev' into post_durango_nits_0

5bb301b

Merge branch 'dev' into post_durango_nits_0

e6463b7

StephenButtolph reviewed Nov 30, 2023

View reviewed changes

abi87 added 2 commits December 1, 2023 03:07

Merge branch 'dev' into post_durango_nits_0

15d7871

nits

5e10688

abi87 requested a review from StephenButtolph December 1, 2023 10:44

Merge branch 'dev' into post_durango_nits_0

a5b3875

StephenButtolph approved these changes Dec 4, 2023

View reviewed changes

StephenButtolph added this to the v1.10.18 milestone Dec 4, 2023

StephenButtolph added this pull request to the merge queue Dec 4, 2023

StephenButtolph added the cleanup Code quality improvement label Dec 4, 2023

Merged via the queue into dev with commit c11accd Dec 4, 2023
16 checks passed

StephenButtolph deleted the post_durango_nits_0 branch December 4, 2023 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Pending Stakers 0 - De-duplicate staking tx verification #2335

Drop Pending Stakers 0 - De-duplicate staking tx verification #2335

abi87 commented Nov 19, 2023 •

edited

Loading

abi87 Nov 19, 2023

abi87 Nov 19, 2023

abi87 Nov 19, 2023

abi87 Nov 19, 2023 •

edited

Loading

abi87 Nov 19, 2023

abi87 Nov 19, 2023

abi87 Nov 19, 2023

abi87 Nov 19, 2023

abi87 Nov 19, 2023

marun left a comment

abi87 Nov 20, 2023

abi87 Nov 20, 2023

abi87 Nov 20, 2023

dhrubabasu left a comment •

edited

Loading

Drop Pending Stakers 0 - De-duplicate staking tx verification #2335

Drop Pending Stakers 0 - De-duplicate staking tx verification #2335

Conversation

abi87 commented Nov 19, 2023 • edited Loading

Why this should be merged

How this works

How this was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abi87 Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubabasu left a comment • edited Loading

Choose a reason for hiding this comment

abi87 commented Nov 19, 2023 •

edited

Loading

abi87 Nov 19, 2023 •

edited

Loading

dhrubabasu left a comment •

edited

Loading