avalanchego icon indicating copy to clipboard operation
avalanchego copied to clipboard

Continuous Staking 0 - P-chain stakers property testing

Open abi87 opened this issue 1 year ago • 2 comments

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:

  1. 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.
  2. 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 avatar Mar 28 '23 17:03 abi87

would it be possible to move all the property based tests to a separate dir?

They used to be in a different package. What is annoying is that property tests are made to test the staker section of state package. If properties are in a different package (and we keep stakers in state) we won't get the test coverage. Looking at test coverage was very helpful for me to spot untested code path and duly cover them or simplify them. This is especially true for the staker update ops PR which will come.

I guess the best solution would be to repackage the stakers section of state and their properties together. I am in favour of it but I'd do that in a different PR (also the state.load section will get slightly more complex since we load staker from genesis in an ad-hoc way)

abi87 avatar May 24 '23 07:05 abi87

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.

github-actions[bot] avatar Mar 31 '24 00:03 github-actions[bot]