Continuous Staking 0 - P-chain stakers property testing #1259

abi87 · 2023-03-28T17:01:57Z

Why this should be merged

This PR introduces property testing for the P-chain stakers state management, increasing test coverage for state package from 42.2% to 50.3%.

The PR does also introduces minor production code changes:

it removes an accidental difference among state.Diff and state.State around stakers removal, thus marginally reducing complexity. Property tests now ensures that state.Diff and state.State are really interchangeable.
it allows staker addition and immediate removal.

The second invariants is not strictly required since it can't happen in prod (would require a zero-lenght staker which is forbidden). Still it makes testing easier and, more importantly, it goes in the directions of making P-chain stakers state much less reliant on txs validation and more aligned with what one may expect from a DB.

How this works

This PR introduced property testing to avalanchego.
Property testing allow devs to specify only relevent input attributes in a test and let other attributes be randomly generated by the testing framework. This avoids tests overfitting and allows exploration of input domain in different test runs.
Also property testing forces dev to describe more precisely properties that should be asserted on outputs or subjets-under-test, since dev does not fully control input anymore.

How this was tested

New property tests + CI

abi87 · 2023-03-30T09:41:33Z

vms/platformvm/state/stakers.go

 		s.addedStakers = btree.NewG(defaultTreeDegree, (*Staker).Less)
 	}
 	s.addedStakers.ReplaceOrInsert(staker)
+	delete(s.deletedStakers, staker.TxID)


I would like to assert the property that "it's fine deleting a validator not yet inserted, and then inserting it".
Our dbs have this property, state.State has this property, but state.Diff currently does not.
The change above brings the property to state.Diff.
Note that without the line inserted, a deleted-than-added staker would be marked as deleted, not added, in lower layer diff when diff.Apply is called. This is because diff.Apply implementation loops first among added stakers then among deleted ones.

Note that in our system it does not currently happen that a staker is deleted then inserted (we'll be able to update it in near future, but still no insert-and-delete).
Still having state.State and state.Diff behave uniformly, without accidental differences, even on currently unexposed situation reduces the whole complexity.

abi87 · 2023-03-30T10:19:13Z

vms/platformvm/state/stakers.go

 		validatorDiff.addedDelegators = btree.NewG(defaultTreeDegree, (*Staker).Less)
 	}
 	validatorDiff.addedDelegators.ReplaceOrInsert(staker)
+	delete(validatorDiff.deletedDelegators, staker.TxID)


see comment for delete-than-insert staker on validator

abi87 · 2023-03-30T10:19:19Z

vms/platformvm/state/stakers.go

 		s.addedStakers = btree.NewG(defaultTreeDegree, (*Staker).Less)
 	}
 	s.addedStakers.ReplaceOrInsert(staker)
+	delete(s.deletedStakers, staker.TxID)


see comment for delete-than-insert staker on validator

Signed-off-by: Alberto Benegiamo <[email protected]>

…stakers_model

github-actions · 2024-03-31T00:00:53Z

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

… engine-driven target updates - Add Coordinator to orchestrate dynamic state sync, enforce pivot cadence, and manage queue execution. - Introduce engine hook OnEngineAccept to enqueue accepted blocks and advance the sync target. - Implement pivot policy (every N blocks) and idempotence (skip behind/equal, allow same-height reorgs). resolves #1259 Signed-off-by: Tsvetan Dimitrov ([email protected])

When UpdateSyncTarget is called, remove all queued blocks with height <= new target height since they will never be executed. This prevents processing blocks that the sync has already advanced past. - Add RemoveBlocksBelowHeight method to blockQueue to filter stale blocks. - Call RemoveBlocksBelowHeight in UpdateSyncTarget after pivot check. - Support accept/reject/verify operations in block queue. - Add OnEngineReject and OnEngineVerify handlers to sync client. - Propagate context through ApplyQueuedBatch for proper cancellation. - Remove unnecessary defer vm.versiondb.Abort() from Accept. - Prevent recursion during batch execution via state check. - Make dequeueBatch private to reduce API surface. resolves #1259 Signed-off-by: Tsvetan Dimitrov ([email protected])

- Add context parameter to finishSync() and propagate through stateSyncStatic/Dynamic - Add context parameter to FinalizeVM callback in Coordinator - Add context parameter to ProcessQueuedBlockOperations (renamed from ApplyQueuedBatch) - Add context parameter to executeBlockOperationBatch (moved from blockQueue) - Propagate context through ProcessQueue operations - Add cancellation checks before expensive operations in finishSync() using declarative operation list pattern with runWithCancellationCheck helper. - Add cancellation checks in ProcessQueuedBlockOperations before state transitions. - Add cancellation checks in executeBlockOperationBatch loop using select pattern. - Improve error messages to include operation index and type for better debugging. Refactoring: - Move block operation processing logic from blockQueue to Coordinator (executeBlockOperationBatch) for better separation of concerns. - Simplify blockQueue to be a pure data structure (enqueue, dequeueBatch, removeBelowHeight). - Rename pivot.go to pivot_policy.go for clarity. - Remove cancel function from Coordinator struct, pass as parameter to finish(). Pivot Policy: - Add defaultPivotInterval constant (10000 blocks) in pivot_policy.go. - Apply default pivot interval when WithPivotInterval is not explicitly called. - Update newPivotPolicy to use default when interval is 0. This change enables graceful shutdown of state sync operations and ensures that cancellation signals propagate correctly through all layers of the dynamic state sync orchestration. resolves #1259 Signed-off-by: Tsvetan Dimitrov ([email protected])

Addresses my comments on #1259.

Enabled all proof tests, which discovered an introduced bug when MaybePersisted was introduced. The node being returned was the wrong one, which caused some proof errors when constructing proofs, notably the wrong node was being returned if the node wasn't yet persisted. Since the tests run with unpersisted but hashed nodes, this caused some failures when the tests were re-enabled. Some tests still do not work with ethhash enabled. Created #1261 for tracking. Also restructured the tests. Proof tests are separated now from range proof tests. Range proof tests were moved but not enabled in this PR (will be done next).

… engine-driven target updates - Add Coordinator to orchestrate dynamic state sync, enforce pivot cadence, and manage queue execution. - Introduce engine hook OnEngineAccept to enqueue accepted blocks and advance the sync target. - Implement pivot policy (every N blocks) and idempotence (skip behind/equal, allow same-height reorgs). resolves #1259 Signed-off-by: Tsvetan Dimitrov ([email protected])

When UpdateSyncTarget is called, remove all queued blocks with height <= new target height since they will never be executed. This prevents processing blocks that the sync has already advanced past. - Add RemoveBlocksBelowHeight method to blockQueue to filter stale blocks. - Call RemoveBlocksBelowHeight in UpdateSyncTarget after pivot check. - Support accept/reject/verify operations in block queue. - Add OnEngineReject and OnEngineVerify handlers to sync client. - Propagate context through ApplyQueuedBatch for proper cancellation. - Remove unnecessary defer vm.versiondb.Abort() from Accept. - Prevent recursion during batch execution via state check. - Make dequeueBatch private to reduce API surface. resolves #1259 Signed-off-by: Tsvetan Dimitrov ([email protected])

- Add context parameter to finishSync() and propagate through stateSyncStatic/Dynamic - Add context parameter to FinalizeVM callback in Coordinator - Add context parameter to ProcessQueuedBlockOperations (renamed from ApplyQueuedBatch) - Add context parameter to executeBlockOperationBatch (moved from blockQueue) - Propagate context through ProcessQueue operations - Add cancellation checks before expensive operations in finishSync() using declarative operation list pattern with runWithCancellationCheck helper. - Add cancellation checks in ProcessQueuedBlockOperations before state transitions. - Add cancellation checks in executeBlockOperationBatch loop using select pattern. - Improve error messages to include operation index and type for better debugging. Refactoring: - Move block operation processing logic from blockQueue to Coordinator (executeBlockOperationBatch) for better separation of concerns. - Simplify blockQueue to be a pure data structure (enqueue, dequeueBatch, removeBelowHeight). - Rename pivot.go to pivot_policy.go for clarity. - Remove cancel function from Coordinator struct, pass as parameter to finish(). Pivot Policy: - Add defaultPivotInterval constant (10000 blocks) in pivot_policy.go. - Apply default pivot interval when WithPivotInterval is not explicitly called. - Update newPivotPolicy to use default when interval is 0. This change enables graceful shutdown of state sync operations and ensures that cancellation signals propagate correctly through all layers of the dynamic state sync orchestration. resolves #1259 Signed-off-by: Tsvetan Dimitrov ([email protected])

patrick-ogrady added vm This involves virtual machines testing This primarily focuses on testing labels Mar 29, 2023

abi87 self-assigned this Mar 29, 2023

abi87 force-pushed the stakers_model branch from b174d40 to 9c03c99 Compare March 30, 2023 09:09

abi87 commented Mar 30, 2023

View reviewed changes

abi87 marked this pull request as ready for review March 30, 2023 10:34

abi87 requested review from StephenButtolph and danlaine as code owners March 30, 2023 10:34

abi87 requested review from gyuho and joshua-kim as code owners April 7, 2023 13:58

abi87 changed the base branch from master to dev April 7, 2023 13:59

abi87 requested a review from coffeeavax as a code owner April 13, 2023 14:22

abi87 added 16 commits April 13, 2023 18:46

Introduced Stakers model

4aed100

introduced staker generator

94ddc25

stakers store model property tests

43cf22a

chain state property tests

d0ea2a0

diff property tests + fix

245fa58

reduced code duplication in tests

2e9fd5f

fix insert after delete delegator in diff

cbd246b

extended test coverage

e7691da

nit

94ecc46

nit

cf91599

cleanup

382148d

wip: stakers storage stateful testing

8c119d5

wip: more stakers storage stateful testing

0e48267

wip: more stakers storage stateful testing

3bead39

wip: more stakers storage stateful testing

f6356f2

merge fix

13c5256

abi87 and others added 7 commits October 17, 2023 12:40

Merge branch 'dev' into stakers_model

d57e692

Merge branch 'dev' into stakers_model

460046e

Merge branch 'dev' into stakers_model

2cac774

Signed-off-by: Alberto Benegiamo <[email protected]>

fixed merge

801e2ea

Merge remote-tracking branch 'upstream/dev' into stakers_model

2255c6e

Merge branch 'dev' into stakers_model

acd1e7f

Signed-off-by: Alberto Benegiamo <[email protected]>

Merge branch 'dev' into stakers_model

0427428

abi87 force-pushed the stakers_model branch from 8f24523 to 0427428 Compare November 2, 2023 17:16

Dan Laine and others added 9 commits November 3, 2023 12:57

Merge remote-tracking branch 'upstream/dev' into stakers_model

d744fbf

Merge branch 'stakers_model' of github.com:ava-labs/avalanchego into …

3c88bb7

…stakers_model

Merge branch 'dev' into stakers_model

c3750f3

Merge branch 'dev' into stakers_model

879e2b6

Merge branch 'dev' into stakers_model

9b9f802

Merge branch 'dev' into stakers_model

4392421

Merge branch 'dev' into stakers_model

267bd34

Merge branch 'dev' into stakers_model

f5cb8ab

Merge branch 'dev' into stakers_model

0c51db3

github-actions bot added the lifecycle/stale Inactive for 60 days label Mar 31, 2024

aaronbuchwald closed this Dec 2, 2024

maru-ava pushed a commit that referenced this pull request Dec 3, 2025

feat: explicit impl of PartialEq/Eq on HashOrRlp (#1260)

28b0498

Addresses my comments on #1259.

JonathanOppenheimer added acp236 Continuous Staking and removed continuous staking labels Dec 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Continuous Staking 0 - P-chain stakers property testing #1259

Continuous Staking 0 - P-chain stakers property testing #1259

Uh oh!

abi87 commented Mar 28, 2023 •

edited

Loading

Uh oh!

abi87 Mar 30, 2023 •

edited

Loading

Uh oh!

abi87 Mar 30, 2023

Uh oh!

abi87 Mar 30, 2023

Uh oh!

github-actions bot commented Mar 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Continuous Staking 0 - P-chain stakers property testing #1259

Continuous Staking 0 - P-chain stakers property testing #1259

Uh oh!

Conversation

abi87 commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Uh oh!

abi87 Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abi87 Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

abi87 Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

abi87 commented Mar 28, 2023 •

edited

Loading

abi87 Mar 30, 2023 •

edited

Loading