Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P-chain Add UTs around stakers persistence in platformvm state #2505

Merged
merged 31 commits into from
Jan 17, 2024

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Dec 19, 2023

Why this should be merged

These are the UTs I wish I had before venturing in refactoring writeCurrentStakers in #2074 and following
Whenever a staker is added or deleted a whole bunch of data structures are updated and possibly stored.
We don't currently assert the behaviour of these data structures. The added UTs cover the gap.

This should have been the very first change I made (crate the test you wish you had).
I didn't and I had other review on the PRs that actually change to code.
So I didnt' rebase the prod-code changing PRs over this testing PR.
I only listed them under the same issue #2512

How this works

There a 16 combinations. A staker can be:

  • validator or delegator
  • primary network or subnet one
  • current or pending
  • being added or removed

For each combination the PR set the state and checks that the ancillary data structures are update correctly.
Moreover it rebuild the state from the db and carries out the very same checks, to make sure that state is persisted correctly.

How this was tested

Just add UTs

@abi87 abi87 marked this pull request as ready for review December 19, 2023 18:37
@abi87 abi87 self-assigned this Dec 19, 2023
@abi87 abi87 added the testing This primarily focuses on testing label Dec 19, 2023
@abi87 abi87 requested a review from danlaine as a code owner December 19, 2023 18:38
@abi87 abi87 linked an issue Dec 19, 2023 that may be closed by this pull request
Copy link
Collaborator

@danlaine danlaine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an improvement over what currently exists, so I'm approving this, but I do have some reservations about the readability of these tests. I think that's more of a reflection of the platformvm/state package than anything else. Right now, I think there are a lot of implicit invariants about how *state is called, which are checked in tests, which makes reasoning more difficult.

For example, we do something like this in a bunch of places:

				s.PutCurrentValidator(staker)
				s.AddTx(addPermValTx, status.Committed) // this is currently needed to reload the staker

Now, it isn't explicitly mentioned anywhere that you need to call AddTx along with PutCurrentValidator, but we implicitly expect that usage and use it in the test. Just an example of what I mean.

Anyway, other than my comment about the test naming duplicates, I think this is alright.

vms/platformvm/state/state_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dhrubabasu dhrubabasu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share the same concerns as @danlaine... state's hidden invariants are tough to reason about

Thanks for adding these tests ❤️

@StephenButtolph StephenButtolph added this to the v1.10.18 milestone Jan 17, 2024
@StephenButtolph StephenButtolph merged commit 4819bb9 into dev Jan 17, 2024
17 checks passed
@StephenButtolph StephenButtolph deleted the stakers_persist_unit_tests branch January 17, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing This primarily focuses on testing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Simplify Stakers handling in P-chain state
4 participants