Fix delegator state inconsistency on put/delete pairs#5331
Conversation
b511283 to
e710f54
Compare
e75533e to
30598bd
Compare
There was a problem hiding this comment.
Pull request overview
Defensive fix to keep delegator diffs internally consistent when the same delegator is both put and deleted within a single diff cycle, ensuring pending writes and iterators don’t disagree (and persistence doesn’t accidentally delete or re-add rows).
Changes:
- Cancel out inverse
PutDelegator/DeleteDelegatorpairs inbaseStakerspending writes. - Apply the same cancel-out behavior in
diffStakersto keepGetDelegatorIterator/GetStakerIteratorcorrect under delete-then-readd and put-then-delete sequences. - Add regression tests covering iterator correctness and persisted DB row behavior; fix iterator lifecycle management in existing diff tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
vms/platformvm/state/stakers.go |
Adds cancel-out logic for delegator put/delete pairs in both base and diff staker trackers to prevent inconsistent added/deleted sets. |
vms/platformvm/state/stakers_test.go |
Adds tests for delegator cancel-out behavior at diff and persistence layers (but currently has iterator double-release issues). |
vms/platformvm/state/diff_test.go |
Ensures iterators are properly released in delegator iterator tests. |
vms/platformvm/state/BUILD.bazel |
Adds the linkeddb dependency needed by the new persistence test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
11bda51 to
a0ba33e
Compare
yacovm
left a comment
There was a problem hiding this comment.
I don't think this can be triggered in reality because we can't add and remove and vice versa the same delegator in a block, so it should be fine adding this as a sanity check for future P-chain development.
|
if we want to support this, we need to cover the situation where we delete + put, but we put a delegator with updated fields, for example a different weight. This test is failing: |
f883b2b to
80b421a
Compare
| if _, ok := validatorDiff.deletedDelegators[staker.TxID]; ok { | ||
| // Cancel a prior delete only when the re-added staker is identical. | ||
| // A mismatch (e.g. different weight) is recorded as a replacement. | ||
| if existing, ok := validatorDiff.deletedDelegators[staker.TxID]; ok && existing.Equals(staker) { |
There was a problem hiding this comment.
when I revert this change, the tests still pass 🫠
Why this should be merged
Defensive fix for an internal consistency issue in
baseStakersanddiffStakers. The pending writes don't cancel out matchingPutDelegator/DeleteDelegatorpairs, so bothaddedDelegatorsanddeletedDelegatorscan end up populated for the sameTxID, inconsistent with the in-memory btree.There's no way to trigger this normally. The bug requires the same delegator (same
staker.TxID) to be both added and removed within a single diff, which no current code path does.How this works
Cancel out matching put/delete pairs in two places:
baseStakers.PutDelegator/DeleteDelegator: when a call is the inverse of an earlier call in the pending writes, undo the prior call instead of recording the new one.diffStakers.PutDelegator/DeleteDelegator: same shape applied to the in-flight diff. Without this,GetDelegatorIteratoron a delete-then-readd diff would filter the re-added delegator out.How this was tested
TestDiffStakersDelegatorCancelOut— diff-level read correctness viaGetDelegatorIterator/GetStakerIteratorfor bothdelete-then-putandput-then-delete. Thedelete-then-putsubtest fails without the fix:deletedDelegatorsstill contains the staker, so the iterator filters out the parent-supplied entry and returnsnil.TestBaseStakersDelegatorCancelOutPersistence— writes the diff throughwriteCurrentDelegatorDiffinto a memdb-backedlinkeddband checks the persisted row for both orderings. Thedelete-then-putsubtest fails without the fix:writeCurrentDelegatorDiffsees the staker indeletedDelegatorsand removes the DB row.Need to be documented in RELEASES.md?
No