fix(backup): rebuild the reversion bitsets on restore#1592
Conversation
The FSM's already-reverted gate reads the per-ledger reversion bitset (ZonePerLedger/SubPLReversions), which only the FSM hot path persists. The restore rebuild replays RevertedTransaction logs into the per-tx RevertedByTransaction markers but not into the bitset, so every revert inside the exported delta is forgotten by the gate: a second revert of the same transaction passes it and, when the account is still solvent, SUCCEEDS — a double refund. Otherwise it surfaces as a wrong rejection (insufficient funds / account-type mismatch) instead of TRANSACTION_ALREADY_REVERTED, which is how the model test caught it right after a restore cycle. The test reverts a funded transaction pre-backup, keeps the account solvent with an independent deposit, restores, and asserts a second revert is still rejected with "already reverted" and that the account's volumes are unchanged. Currently fails at Phase 3 (the double revert succeeds); goes green once the rebuild reconstructs the bitset.
The FSM's already-reverted gate reads the per-ledger reversion bitset (ZonePerLedger/SubPLReversions), persisted only by the FSM hot path and loaded verbatim on boot (query.ReadReversions). The restore rebuild replayed RevertedTransaction logs into the tx rows' RevertedByTransaction markers but never into the bitset, so a restored node forgot every revert inside the exported delta: a second revert of the same transaction passed the gate and, on a solvent account, SUCCEEDED — a double refund. On an insolvent one it surfaced as a wrong rejection (insufficient funds / account-type mismatch instead of TRANSACTION_ALREADY_REVERTED), which is how the model test caught it right after a restore cycle. The replay writer now seeds the bitsets from the checkpoint rows, folds every replayed revert into them (SetRevertedBy), resets a ledger's fold on DeleteLedger, and flushes touched ledgers' nonzero words in the FSM's own layout (state.SaveReversionWord, exported for this). Checker strengthening: the persisted bitset was an unverified projection — checkReversionInvariants only validates the log stream's internal consistency. The new compareReversions pass compares the stored bitsets against the audit-derived reverted set (baseline tx-row markers + replayed reverts, complete across archived chapters), exact equality both ways; cleanup-pending ledgers are skipped since their rows linger until the covering purge. The e2e added in the previous commit now passes.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Approve — automated reviewThe reversion-bitset rebuild and restore logic looks sound. The previously identified major issue (stale ZonePerLedger/SubPLReversions rows left in Pebble after a DeleteLedger replay event) was acknowledged and resolved in an earlier revision, confirmed by the prior discussion. The post-restore checker logic and invariant coverage for the new projection pass are consistent with the FSM path. Patch coverage sits around 70%, which is noted but not a blocker. The documentation follow-up noted in the prior discussion remains open but is a non-blocking concern. No new concrete findings were raised by the current round of reviews. No findings. |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (63.71%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1592 +/- ##
================================================
- Coverage 74.80% 74.79% -0.02%
================================================
Files 430 430
Lines 45697 45789 +92
================================================
+ Hits 34185 34249 +64
- Misses 8463 8477 +14
- Partials 3049 3063 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The test engine mimics WriteSet.Merge but kept reverted status in a per-proposal in-memory map, never persisting the reversion words — so compareReversions correctly flagged every synthetic revert as a missing bit. The engine now folds reversions into per-ledger bitsets that survive across proposals (GetReverted consults them, matching the FSM gate) and persists the touched words at commit via SaveReversionWord, like the real dirty-word flush.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1592 (comment)
Unlike the rest of the per-ledger data (deferred to the covering purge), the live path deletes the SubPLReversions rows at DeleteLedger apply (WriteSet.Merge). The replay only dropped its in-memory fold, so a restored store kept a deleted ledger's checkpoint-time reversion rows and boot resurrected them into Registry.Reversions — a restored-vs-live divergence until the purge. Mirror the live path with the now-exported state.DeleteReversionsByLedger. Flagged by NumaryBot on #1592.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).
Summary: #1592 (comment)
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
I found one blocking issue in the new reversion projection checker.
compareReversionsmust not skip based on the persisted pending-cleanup marker, and it should also account for stored rows for non-live ledgers.
The checker derives knownLedgers from the audit stream, but then compareReversions skips a ledger whenever pendingCleanupLedgers[name] exists. That marker is itself a Pebble projection, so a stale or forged pending-cleanup row for an audit-live ledger suppresses both missing-bit and unaudited-bit errors. The current TestCompareReversions_CleanupPendingSkipped locks that behavior in: it marks ledger-a as live, writes an unaudited reversion bit, adds a pending cleanup marker, and expects no error.
This exception needs to be driven by audit-derived deletion state, not by the projection being checked. With this PR, the live FSM path deletes SubPLReversions rows immediately on DeleteLedger, and the rebuild path mirrors that, so stale reversion rows for deleted/non-live ledgers should not be treated as legitimate lingering cleanup data either. Otherwise a regression in the DeleteLedger replay cleanup, or a tampered pending-cleanup marker on a live ledger, can pass Check() cleanly while Registry.Reversions would be wrong on recovery.
Please make the checker compare the decoded stored reversion rows against the audit-derived live/deleted state in both directions, and adjust the cleanup test so a pending-cleanup marker alone cannot hide mismatches for an audit-live ledger.
- Related hardening: the checker now uses
query.ReadReversionsas an integrity source, but that reader silently skips malformed keys/short values and does not returniter.Error(). For a checker pass, malformed rows underZonePerLedger/SubPLReversionsshould be surfaced as an error/event instead of being dropped before comparison.
|
Documentation follow-up: this PR adds a new restore/checker mechanism for |
Review feedback on #1592 (gfyrag): 1. The pass skipped ledgers with a persisted pending-cleanup marker — itself an unverified Pebble projection, so a stale or forged marker on an audit-live ledger suppressed both mismatch directions. The skip is gone: audit-live ledgers are always compared. Stored rows for non-live ledgers are now flagged instead of ignored — DeleteLedger deletes them at apply on both the live path and the replay, so nothing legitimately lingers. 2. query.ReadReversions silently dropped malformed keys/short values and never checked iter.Error(). It now returns malformed rows alongside the bitsets and surfaces iterator errors (also fixed in ReadReversionBitset). The checker reports each malformed row as a REVERTED_MISMATCH event, recovery logs them loudly, and the restore rebuild refuses to seed from a corrupt checkpoint. Docs: checker.md gains the compareReversions pass (and stops claiming checkReversionInvariants verified the stored bitset); attributes.md's reversion key layout corrected to [zone][sub][ledgerName padded 64B][wordIndex] (was documented with a 4-byte ledger id) and the in-memory map key corrected to the ledger name; backup-restore.md moves reversions from the attributes zone to the per-ledger zone and lists them among the state RebuildDelta reconstructs.
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Approved current head 88cd73b. I rechecked the previous requested-change items: compareReversions is now driven by audit-derived state rather than pending-cleanup markers, stored rows for non-live ledgers are flagged, malformed reversion rows surface instead of narrowing the comparison silently, and restore rebuild now seeds/folds/flushes the reversion bitsets in the FSM layout.
Caveats: GitHub currently reports this PR as conflicting with release/v3.0, so it is not merge-ready until rebased/resolved. codecov/patch is also failing, but I did not find a remaining code blocker in this pass.
3 participants
Found by the model test's restore cycle, immediately after a backup→restore: a revert the model knew was already applied got re-attempted by the server and rejected with
insufficient fundsinstead ofTRANSACTION_ALREADY_REVERTED. Same mechanism as an earlier unexplained finding where the incidental rejection wasaccount does not match any account type pattern.Bug
The FSM's already-reverted gate reads the per-ledger reversion bitset (
ZonePerLedger/SubPLReversions) — not the tx rows'RevertedByTransactionmarkers. That bitset is persisted only by the FSM hot path and loaded verbatim on boot (query.ReadReversions). The restore rebuild replayedRevertedTransactionlogs into the tx-row markers but never into the bitset, so a restored node forgot every revert inside the exported delta:Evidence from the failing run: the restored store's log stream held 30 reverts while the
0301key range held zero rows; the re-attempted revert targeted a transaction whose own row saidrevertedByTransaction: 41.Fix
The replay writer seeds the bitsets from the checkpoint rows, folds every replayed revert into them (
SetRevertedBy), resets a ledger's fold onDeleteLedger, and flushes touched ledgers' nonzero words in the FSM's own layout (state.SaveReversionWord, exported for this).Checker strengthening
The persisted bitset was an unverified projection (invariant 8):
checkReversionInvariantsonly validates the log stream's internal consistency. The newcompareReversionspass compares stored bitsets against the audit-derived reverted set (baseline tx-row markers + replayed reverts, complete across archived chapters), exact equality both ways — a lost bit re-admits a double revert, an unaudited bit silently blocks a legitimate one. Cleanup-pending ledgers are skipped (rows linger until the covering purge).Tests
tests/e2e/cluster/restore_reversion_test.go: reverts a funded transaction pre-backup, keeps the account solvent, restores, and asserts a second revert is still rejected with "already reverted" and the account volumes are unchanged. Without the fix commit, the double revert succeeds.compareReversions(match / missing bit / unaudited bit / cleanup-pending skip).Behavioral note
Stores restored with the old rebuild are missing delta reverts from their bitsets and will now correctly fail
Check()withREVERTED_MISMATCH— and genuinely accept double reverts until re-restored with the fix, so worth flagging to operators of existing restored environments.