Add crash recovery logic for GigaStorageManager - #4079
Conversation
PR SummaryHigh Risk Overview Recovery picks a target as the minimum head among the block store, offline state WAL tail, and receipt store (when enabled). It then rolls receipts back offline ( Supporting API changes: Adds Reviewed by Cursor Bugbot for commit e7f0fac. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4079 +/- ##
==========================================
- Coverage 61.30% 60.44% -0.87%
==========================================
Files 2178 2101 -77
Lines 190826 182015 -8811
==========================================
- Hits 116990 110021 -6969
+ Misses 62807 61659 -1148
+ Partials 11029 10335 -694
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The crash-recovery implementation has several correctness problems: the commit store is now constructed with the same live state WAL that giga.stateDB already writes (which the WAL's own ordering check rejects), a computed target of 0 runs the destructive recovery steps instead of short-circuiting, and CatchUpFrom stamps the EVM store at the target even when the WAL cannot cover the replay range. The refactors around statewal path arguments and EVMStateStore.openDBs/closeDBs are clean.
Findings: 4 blocking | 6 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 4 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] No test exercises
OpenDBWithRecoveryagainst a home directory whose stores actually disagree. Every new test drives the private helpers directly on a freshly opened manager, andTestOpenDBWithoutRecoveryOnAFreshHomeonly covers the fresh case. A reopen test (commit N blocks, close the manager, desynchronize one store on disk, reopen and assert every head converged) is what would have caught the SC/WAL ownership change and the target-0 path. - [suggestion]
giga/state_db_impl.gonow holds anssfield thatCommitStateChangesnever writes (// TODO: Commit changes to SS). The EVM state store therefore only ever advances during startup recovery, so the state WAL must retain every block back to the previous recovery point for the next restart to be correct — a constraint nothing in the prune cycle enforces. Worth stating in the recovery godoc while the TODO stands. - [suggestion]
flatkvStateWALNameinsei-db/tools/cmd/seidb/operations/flatkv_open.gois now dead after theGetRangesignature change — its only remaining reference is its own declaration. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := m.openStateWal(); err != nil { | ||
| return err | ||
| } | ||
| if err := m.openSC(ctx, m.stateWAL); err != nil { |
There was a problem hiding this comment.
[blocker] Passing the live m.stateWAL into NewCommitStore hands FlatKV the WAL write path, but giga.stateDB still writes the same instance itself, so every block is written twice.
CommitStore.Commit does wal.Write / SignalEndOfBlock / Flush whenever its wal field is non-nil (store_write.go:87, and the field doc at store.go:148 — "non-nil ⇒ FlatKV writes/replays/prunes it"). stateDB.CommitStateChanges already does wal.Write(blockNum, …) + SignalEndOfBlock() before delegating to sc.CommitStateChanges. The second Write for the same height reaches enforceWriteOrdering with currentBlockEnded == true and returns block number N has already ended; cannot write more changes to it, so the very first commit fails.
This is exactly the invariant NewStateDB's godoc states ("one holding a WAL would record every block twice"), and storage_manager.go:34 still documents SC as "opened with no WAL of its own; StateDB writes it" — both are now stale.
There is a second hazard even if the write path is fixed: CommitStore.Close closes whatever WAL instance it holds and reopenWAL may replace it, while GigaStorageManager.Close also closes m.stateWAL and giga.stateDB keeps writing through its own copy of the pointer. Two owners of one WAL handle.
SC only needs the WAL so recoverSC's LoadLatest can replay; consider injecting it for the recovery window only, or moving the WAL write out of giga.stateDB so FlatKV is the single writer.
| if err := m.recoverReceipt(targetHeight); err != nil { | ||
| return err | ||
| } | ||
| if err := m.truncateStateWAL(targetHeight); err != nil { |
There was a problem hiding this comment.
[blocker] The godoc on line 19 says "A target of 0 means a fresh node, and nothing is moved", but nothing short-circuits on 0 — every destructive step below still runs.
With a target of 0 and populated stores (see the receipt-head case on line 145, or a block store at 0 while the state WAL is not):
truncateStateWAL(0)callsstatewal.PruneAfter(path, 0)and drops the entire state WAL;recoverSC(0)then callssc.Rollback(0), whichrollbackBaseVersionrejects (snapshot.go:611: "rollback target 0 is invalid: version 0 means no state");recoverSS(0)would likewise hitRollbackTo'starget <= 0guard.
So the node destroys its WAL and then refuses to start. Return early from OpenDBWithRecovery when the target is 0, or treat a 0 target as an error when any store is non-empty, before anything is written.
| if err != nil { | ||
| return 0, fmt.Errorf("read receipt store head: %w", err) | ||
| } | ||
| target = min(target, receiptHeight) |
There was a problem hiding this comment.
[blocker] blockHeight and stateHeight each get a == 0 → fresh node guard on line 136, but receiptHeight does not, so an empty receipt store silently collapses target to 0 and triggers the destructive path described on line 34.
This is reachable whenever receipts are enabled on a node that already has block/state history — receipts newly turned on, or the receipt directory recreated after corruption. Either give receiptHeight == 0 the same treatment as the other two, or make "one store is at 0 while others are not" an explicit error rather than a target.
| return err | ||
| } | ||
| } | ||
| if got := s.GetLatestVersion(); got < target { |
There was a problem hiding this comment.
[blocker] This unconditional tail bump marks the store as caught up even when the WAL never supplied the blocks.
wal.Iterator(head+1, target) only rejects endIndex > last (seiwal_impl.go:588); a startIndex below the WAL's first stored block is accepted, and the iterator just yields records from the WAL's actual floor. So when the WAL has been pruned past head+1, the loop applies [walFirst, target], leaves [head+1, walFirst) unapplied, and then SetLatestVersion(target) stamps the EVM store as being at target — silent state divergence instead of a startup failure.
The FlatKV side already does the check this is missing: CommitStore.rollbackBaseVersion reads GetStoredRange() and errors with "blocks %d-%d are needed, but the WAL only holds %d-%d" when the range is short (snapshot.go:639). Mirror that here — verify the stored range covers [head+1, target] before replaying, and drop the unconditional SetLatestVersion (or gate it on having actually consumed every block in the range).
This is load-bearing because stateDB.CommitStateChanges still has // TODO: Commit changes to SS, so the store's head only ever moves at startup and the distance it must replay grows with uptime.
| if err := utils.ClonePebbleDir(src, tmp); err != nil { | ||
| return err | ||
| } | ||
| if err := os.Rename(dst, bak); err != nil && !os.IsNotExist(err) { |
There was a problem hiding this comment.
[suggestion] An interrupted restore is indistinguishable from a fresh store, and nothing ever consumes the leftovers.
A crash between this rename and the next one leaves no dst: the next startup's openDBs creates an empty Pebble directory, GetLatestVersion() reports 0, and recoverSS classifies the store as merely behind and calls CatchUpFrom from block 1 — which, per the range issue above, silently stamps target over a store that holds almost nothing. .restore-bak and .restore-tmp are only removed by the next replacePebbleDir call, so they are never inspected on open.
Add a step on open (or at the top of restoreSnapshot) that promotes a leftover bak when dst is missing, or write a marker recovery can read. The separateDBs loop in restoreSnapshot has the same problem one level up: a failure after the first sub-DB leaves the sub-DBs at mixed versions with no record of it.
| if normalizeReceiptBackend(cfg.Backend) != receiptBackendLittIdx { | ||
| return fmt.Errorf("receipt store rollback is not supported for backend %q", cfg.Backend) | ||
| } | ||
| if err := rollbackLittBodies(cfg, uint64(target)); err != nil { //nolint:gosec // target >= 0 |
There was a problem hiding this comment.
[suggestion] Bodies are dropped before the index and head are rewound, so a crash in between leaves m:latest above the newest surviving body plus tag entries pointing at receipts that no longer exist. Recovery re-derives the same target from the block/state heads on the next boot, so the common case self-heals, but this is the crash-recovery path — rewinding the head first and deleting bodies second makes the intermediate state under-promise rather than over-promise, which is the safe direction.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| if err := m.openSC(ctx, m.stateWAL); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
SC shares manager-owned WAL
High Severity
openSC now injects the manager-owned WAL into CommitStore, but StateDB still writes that same WAL and Close still closes it. Every live commit therefore writes the block twice, Close closes the WAL twice, and recoverSC rollback replaces s.wal while leaving m.stateWAL pointing at the closed instance.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 78e8216. Configure here.
| } | ||
| if err := m.truncateStateWAL(targetHeight); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Zero target still wipes stores
High Severity
A recovery target of 0 is documented as a fresh node that should not move data, but OpenDBWithRecovery still truncates the WAL and attempts rollbacks. findTargetRecoveryHeight also returns 0 when only the block store, WAL, or receipt head is empty, including genesis (GetLatestBlock reports 0).
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 78e8216. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e7f0fac. Configure here.
| return 0, fmt.Errorf("read receipt store head: %w", err) | ||
| } | ||
| target = min(target, receiptHeight) | ||
| } |
There was a problem hiding this comment.
Empty receipts wipe recovery target
High Severity
findTargetRecoveryHeight treats a missing block or WAL head as "nothing to converge on", but still folds a receipt head of 0 into min(). Receipts are enabled by default and can be unwritten while the block store and WAL already have height, so the target becomes 0. OpenDBWithRecovery then always calls truncateStateWAL(0), which drops every WAL block, and recoverSS cannot roll back to 0 because RollbackTo rejects that target.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit e7f0fac. Configure here.
| } | ||
| if err := os.Rename(tmp, dst); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
SS restore is not crash-safe
Medium Severity
replacePebbleDir renames the live directory to .restore-bak before swapping in the clone. If that second rename fails or the process crashes between the two, the live path is gone and openDBs cannot reopen the store. RollbackTo also calls closeDBs without stopCheckpoints, so an in-flight snapshot can still be using the directories being replaced.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e7f0fac. Configure here.
1 participant
Describe your changes and provide context
Adds crash recovery to
GigaStorageManagerso that, after recovery:OpenDBWithRecoveryopens the block store and receipt store, then computes a recovery target and brings the rest of the stores onto it:findTargetRecoveryHeight— the target is the lowest head among the block store, the state WAL, and the receipt store (receipts skipped when disabled). It reads the WAL tail offline viastatewal.GetRangewithout opening a live WAL. A target of0means a fresh node with nothing to converge on, andOpenDBWithoutRecoveryjust loads the commit store as it sits.truncateStateWAL— drops every WAL block above the target offline viastatewal.PruneAfter, so the first live write after startup is the block after the target.recoverSC— loads the commit store from the truncated WAL, then rolls it back if a snapshot left it above the target.recoverSS— brings the EVM state store onto the target, replaying the WAL when it is behind (CatchUpFrom) and restoring the newest snapshot <= target and replaying onto it when it is ahead (RollbackTo).recoverReceipt— rolls the receipt store back to the target. The store must be closed for the rollback to rewrite it, so it closes, rolls back offline viareceipt.Rollback, and reopens.Supporting changes:
statewal.GetRange/PruneAfter/VerifyIntegritynow take a directory path instead of a*Config, so they can run offline against a WAL directory with no live instance open.receipt.Rollback,EVMStateStore.CatchUpFrom/RollbackTo.GigaStorageManagerretains its config for the reopen/offline paths, andClosereports every store's failure rather than stopping at the first.Testing performed to validate your change
sei-db/bootstrap/recovery_test.gocovering each skew: SC behind the WAL, SC ahead (rollback), SS behind, SS ahead, receipts ahead, WAL truncation, target-height computation, and a fresh node where every height is zero.go test ./sei-db/bootstrap/...and./sei-db/state_db/sc/flatkv/...pass (viascripts/ramtest.sh).