avalanchego
avalanchego copied to clipboard
Continuous Staking 0 - P-chain stakers property testing
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
andstate.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
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)
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.