Add crash recovery logic for GigaStorageManager - #4079
Conversation
PR SummaryHigh Risk Overview
Supporting API moves include Extensive tests live in Reviewed by Cursor Bugbot for commit 529d7c3. 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.35% 60.66% -0.70%
==========================================
Files 2187 2127 -60
Lines 191779 185764 -6015
==========================================
- Hits 117673 112692 -4981
+ Misses 62972 62362 -610
+ Partials 11134 10710 -424
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
|
@seidroid review |
There was a problem hiding this comment.
The StateDB/WAL ownership restructure is a genuine improvement and the SC replay path now has the range guard it needed, but sei-db/state_db/giga no longer compiles (its test calls a constructor that does not exist), and three previously reported recovery defects survive: the target-0 path still destroys receipts, an empty receipt store still collapses the target and silently skips all convergence, and CatchUpFrom still stamps the EVM store at a target the WAL never covered. The EVM rollback additionally leaves snapshots above the target in place.
Findings: 5 blocking | 6 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
- 5 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] No test drives
OpenDBWithRecoveryend to end against a home directory whose stores actually disagree. Every new test callsopenStateDB/recoverState/recoverReceiptdirectly, and the onlyfindTargetRecoveryHeighttest is the zero case — so nothing covers a non-zero target computed from a real block-store head, nor the interaction between the receipt rollback and the state rollback. A reopen test (commit N blocks, close, desynchronize one store on disk, reopen throughNewGigaStorageManager, assert every head converged) is what would catch the two target-0 defects below. - [suggestion]
CommitStateChangesstill has// TODO: Commit changes to SS, so SS's head only ever moves during startup recovery and the WAL range it must replay grows with uptime. Because SS joins the prune cycle at version 0, the collector's minimum pins the WAL cut line at 0 as well, so the WAL grows unbounded rather than being pruned out from under the replay. Worth stating in theRollbackTo/recovery godoc while the TODO stands, since theCatchUpFromcoverage check depends on the answer. - [suggestion]
flatkvStateWALNameinsei-db/tools/cmd/seidb/operations/flatkv_open.go:27became dead whenGetRangeswitched to a path argument — its only remaining reference is its own declaration and doc comment. Raised in the previous review and still present. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The restructure that gives giga.StateDB sole ownership of SC, SS and the shared WAL is solid, and every blocker from the previous review is resolved (target-0 guard now at the choke point, an empty receipt store no longer collapses the target, both halves replay through one range-checked replay, SS snapshots above the target are removed, checkpoints are quiesced, and an interrupted restore is healed on open). One crash-ordering defect remains in EVMStateStore.RewindToSnapshotAtOrBelow: the live databases are replaced before the rejected snapshots are removed, so a crash in between leaves a discarded branch that a later rollback can restore as authoritative.
Findings: 1 blocking | 4 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
flatkvStateWALNameinsei-db/tools/cmd/seidb/operations/flatkv_open.go:27is now dead — theGetRangepath-argument change removed its last use, and its only remaining reference is its own doc comment. Reported in both previous reviews and still present. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
All four findings from the previous review are addressed: the SS rewind now removes the discarded snapshots before replacing the databases, RollbackTo verifies both halves landed on the target, the flatkv rewind rejects version 0, and the separate-DB restore hazard is stated plainly. What remains is non-blocking: the SC-side rewind still repoints before it removes (the mirror of the ordering just fixed on SS), and a couple of contracts the new code quietly moves past.
Findings: 0 blocking | 7 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
flatkvStateWALNameinsei-db/tools/cmd/seidb/operations/flatkv_open.go:27is dead — theGetRangepath-argument change removed its last use, and its doc comment ("the offline GetRange used here does not emit any") now describes a call site that no longer passes it. Reported in all three previous reviews and still present. - [suggestion] An interrupted separate-DB restore (
EVMStateStore.restoreSnapshotwithSeparateEVMSubDBs) is now honestly documented as unrecoverable, which resolves the earlier ask, but the code still lets it happen: the head reads as the minimum, recovery classifies the store as merely behind, and replaying forward cannot delete the rows an untouched sub-DB holds above the restored version. Since the mode is off by default, refusing the rewind outright whenseparateDBsis set would trade a documented silent divergence for a loud one. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
CommitStore.Rollback(sei-db/state_db/sc/flatkv/snapshot.go:692) has the same repoint-before-remove ordering as the newRewindToSnapshotAtOrBelow:repointAtSnapshotdemotes the store to the base snapshot, andremoveSnapshotsAboveruns afterwards, so a crash between them leaves a rejected snapshot branch on disk that a later rollback can restore. Present on the base branch; fixing both would want one ordering-safe helper rather than two call sites.
Superseded: latest AI review found no blocking issues.
| // A target of 0 is no height to converge on, and every store is left as it was found: rolling back to | ||
| // it would drop every receipt the node holds along with every block in its WAL. This is the single | ||
| // guard for that, which is why the two rollbacks below it carry none of their own. | ||
| func (m *GigaStorageManager) recoverStores(target int64) error { |
There was a problem hiding this comment.
Should this method also be responsible for rolling back StateWAL?
There was a problem hiding this comment.
Rollback StateWAL will be done in StateDB since WAL lifecycle is managed by StateDB now
| if _, err := os.Stat(littDir); os.IsNotExist(err) { | ||
| return nil | ||
| } | ||
| return rollback.RollbackLittDB([]string{littDir}, func(_ string, key []byte, isPrimary bool) (bool, error) { |
There was a problem hiding this comment.
FYI, the package name for this changes in my PR from rollback to offline. Mentioning this incase the merge conflict makes it unclear what happened.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f191ffa. Configure here.
|
@seidroid review |
There was a problem hiding this comment.
The StateDB ownership restructure and the recovery flow are in good shape, and the receipt/WAL/SS work from earlier rounds is resolved (the dead flatkvStateWALName is live again, the SS rewind removes snapshots before replacing the databases, matchHeight pins the landing height, interrupted restores are healed on open, and there is now real test coverage). One previously-reported defect survives: the new CommitStore.RewindToSnapshotAtOrBelow still repoints before it removes, so a crash inside that window strands a rejected snapshot branch that a later rollback can restore as authoritative.
Findings: 1 blocking | 5 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] An interrupted separate-DB restore (
EVMStateStore.restoreSnapshotwithSeparateEVMSubDBs) is now honestly documented as unrecoverable, but the code still lets it happen: the head reads as the minimum across sub-DBs,recoverSS/rewindSSclassifies the store as merely behind, and replaying forward cannot delete the rows an untouched sub-DB holds above the restored version. The mode is off by default, so refusing the rewind outright whenseparateDBsis set would trade a documented silent divergence for a loud one. (Codex raises the same point.) - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
CommitStore.Rollback(sei-db/state_db/sc/flatkv/snapshot.go:692) has the same repoint-before-remove ordering as the newRewindToSnapshotAtOrBelow, so a crash betweenrepointAtSnapshotandremoveSnapshotsAboveleaves a rejected snapshot branch on disk. Present on the base branch; fixing both would want one ordering-safe helper rather than two call sites.
An interrupted SC rewind leaves the store reading as the snapshot it was repointed at with the abandoned branch still on disk, which rewindSC then skips over, so the cleanup never runs again. A later rollback can seek into that branch and replay over it. Co-authored-by: Cursor <cursoragent@cursor.com>
Neither rewind RollbackTo delegates to is reached at 0, since each skips a store already at or below the target. The steps in between are destructive there: the WAL prune empties the WAL and the snapshot removal takes every snapshot, with the landing check passing because both halves read 0. Co-authored-by: Cursor <cursoragent@cursor.com>
A store asks from its own commit path; a catch-up that drives that path is an asker like any other block. The paths that must not ask are the ones that apply blocks around the commit path, which is what SC's internal replay and SS's ApplyReplayedBlock both do. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
The three blocking/unresolved items from the last round are addressed: RollbackTo now rejects a target of 0, the interrupted SC rewind is healed by an unconditional dropSCSnapshotsAbove/RemoveSnapshotsAbove on every rollback, and the checkpoint-scheduler contract was amended to name a WAL catch-up as an intended asker. What remains is non-blocking: RollbackTo mutates before it establishes that the WAL can reach the target (the property its CommitStore.Rollback sibling documents), a newly enabled SS forces a replay from block 1, and an empty block store still reports recovery success having done nothing.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] An interrupted separate-DB restore (
EVMStateStore.restoreSnapshotwithSeparateEVMSubDBs) is now honestly documented as unrecoverable, but the code still lets it happen: the head reads as the minimum across sub-DBs,rewindSSclassifies the store as merely behind, and replaying forward cannot delete the rows an untouched sub-DB holds above the restored version. The mode is off by default, so refusing the rewind outright whenseparateDBsis set would trade a documented silent divergence for a loud one. (Codex raises the same point.) - [suggestion]
CommitStateChangesstill carries// TODO: Commit changes to SS, so SS's head only ever moves during a recovery replay while SC and the WAL advance every block. That makes the WAL rangecatchUpSSmust cover grow with uptime, and it is what turns thereplayrange check from a corruption detector into the thing that decides whether a node starts. Worth stating onRollbackTo/catchUpSSwhile the TODO stands. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
CommitStore.Rollback(sei-db/state_db/sc/flatkv/snapshot.go:692) repoints at the base snapshot before removing the snapshots above the target, so a crash betweenrepointAtSnapshotandremoveSnapshotsAboveleaves a rejected snapshot branch on disk. The newRewindToSnapshotAtOrBelowhas the same ordering but is now healed by its caller's unconditionaldropSCSnapshotsAbove;Rollbackhas no equivalent, soseid rollbackstill carries the window. Present on the base branch.
Superseded: latest AI review found no blocking issues.
Every step of a rollback is irreversible while the replays that need the WAL run last, so a shortfall found there had already cut the WAL and dropped the snapshots a second attempt would need. CommitStore.Rollback keeps this property; RollbackTo did not. An SS holding nothing the WAL can rebuild is left empty to fill forward from the target, as recoveryTarget already does for an empty receipt store, rather than replayed from block 1 and refused as data loss. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
The recovery flow is in good shape this round: RollbackTo now establishes reachability before it mutates anything, an SS that holds nothing fills forward instead of being called data loss, SC's abandoned snapshot branch is dropped unconditionally, and there is real end-to-end test coverage for the target computation, both rewinds, both replays and the interrupted-restore heal. No blockers; what remains is one mutate-order asymmetry in recoverStores, the block-store-at-zero case that still reports success having done nothing, and a godoc that over-promises on the new SC rewind.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
CommitStateChangesstill carries// TODO: Commit changes to SS, so SS advances only during a recovery replay while SC and the WAL advance every block. That is what makes the WAL rangecatchUpSSmust cover grow with uptime, and it is whyssFillsForward— rather than the replay range check — decides whether a full node starts. Worth stating oncatchUpSS/RollbackTowhile the TODO stands, since the two are now coupled. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
CommitStore.Rollback(sei-db/state_db/sc/flatkv/snapshot.go:692) repoints at the base snapshot before removing the snapshots above the target, so a crash betweenrepointAtSnapshotandremoveSnapshotsAbovestrands a rejected snapshot branch that a later rollback can restore as authoritative. The newRewindToSnapshotAtOrBelowhas the same ordering but is healed by its caller's unconditionaldropSCSnapshotsAbove;Rollback(theseid rollbackpath) has no equivalent. Present on the base branch; Codex flags the same spot.
| if target == 0 { | ||
| return nil | ||
| } | ||
| if err := m.recoverReceipt(target); err != nil { |
There was a problem hiding this comment.
[suggestion] The receipt rollback runs before RollbackTo establishes that state can reach the target, which inverts the property this PR deliberately added on the state side.
requireReachable exists because "every step of one is irreversible… Failing there leaves a node that will not start and no longer holds what a second attempt at a different height would need" (giga/state_db_impl.go). recoverReceipt is exactly such a step — receipt.PruneAfter drops the LittDB bodies, range-deletes the tag index above the target and rewrites m:latest — and it runs first. So a target that SC's surviving snapshots plus the WAL cannot span (e.g. SC at 1000 with its newest snapshot at 900 and a WAL holding 950-1000, block store at 998) prunes receipts 999-1000 and only then refuses with "needs blocks 901-998, but the state WAL only holds 950-1000".
Swapping the two calls fixes it without moving the choke point: RollbackTo touches no receipt, and recoverReceipt still runs before openReceiptStore takes the store's locks. It is also self-healing in the other direction — a recoverReceipt that fails after a successful RollbackTo leaves the next boot deriving the same target and re-pruning.
Codex rates this blocking; I read the incremental data loss as small, since the receipts destroyed sit above the height any successful recovery would have converged on.
| // empty WAL, and converging on a target derived from the other stores would discard it with no WAL left | ||
| // to replay it from. | ||
| func recoveryTarget(blockHeight, stateHeight, receiptHeight uint64) uint64 { | ||
| if blockHeight == 0 || stateHeight == 0 { |
There was a problem hiding this comment.
[suggestion] The comment below now names both halves of this guard but still only justifies the WAL one: "state whose WAL was pruned away behind a snapshot still exists with an empty WAL" is a reason for stateHeight == 0, and there is no equivalent reading for blockHeight == 0.
An empty block store alongside a populated state WAL is not the ambiguous case the comment describes — it is a home directory whose stores disagree in a way recovery cannot fix. Returning 0 makes OpenDBWithRecovery report success on it, consensus then replays from block 1, and CommitStateChanges(1, …) hits enforceWriteOrdering against a WAL whose lastCompletedBlock is N. The node dies at its first commit with an error pointing at the WAL rather than at the missing block ledger, and both invariants in OpenDBWithRecovery's godoc were false while it returned nil. TestRecoveryTarget's "an empty block store yields no target" case pins the behaviour but not that consequence.
Either refuse a block store at 0 when the state WAL holds blocks (a startup error naming both heads), or extend the comment to say why deferring that failure to the first commit is the intended answer. Codex raises the same point.
(Reported on the previous two rounds; the comment grew to mention both stores but still explains only the WAL half.)
| if err != nil { | ||
| return 0, fmt.Errorf("seek snapshot at or below version %d: %w", version, err) | ||
| } | ||
| if baseVersion == s.committedVersion { |
There was a problem hiding this comment.
[suggestion] This early return skips removeSnapshotsAbove, so the method does not do what its godoc — and the LiveStateStore contract this PR adds it to — promises: "discarding committed state and snapshots above that point".
The state it is reachable in is precisely the interrupted rewind the new tests describe: store at 3 with snapshots 3 and 6, RewindToSnapshotAtOrBelow(5) returns 3 and leaves snapshot 6 on disk. RollbackTo is safe because dropSCSnapshotsAbove runs unconditionally right after, and rewindSC only calls this when Version() > target (so base < committedVersion there), but a caller reaching this through the interface gets "landed on 3" with the abandoned branch intact.
The SS mirror has no such early return — it removes and restores every time. Either drop the shortcut and let removeSnapshotsAbove run, or say in the godoc that finishing the snapshot-tree cleanup is the caller's obligation when the store already reads as the base.

Describe your changes and provide context
Adds crash recovery to
GigaStorageManager, so that after an unclean shutdown:Getting there needed one structural change:
giga.StateDBnow owns the state commit store, the EVM state store, and the state WAL those two share. Previously the manager held all four pieces itself and SC opened a WAL of its own, which left SC and the manager both writing the same WAL — a double-append that makes recovery-by-replay impossible. SC and SS are now each constructed without a WAL,StateDBis the only writer, and the replay that brings either half onto a height reads through that one WAL.Recovery flow
OpenDBWithRecoveryopens the block store and the receipt store, computes a target, and brings everything else onto it:findTargetRecoveryHeight— the lowest head among the block store, the state WAL and the receipt store, receipts being skipped when disabled. The WAL tail is read offline throughstatewal.GetRange, with no live WAL open. A target of0means there is no height to converge on, and nothing is moved.recoverReceipt— the store has to be closed to be rewritten, so it closes, rolls back offline throughreceipt.Rollback, and reopens.openStateDB— opens SC, the WAL and SS where it finds them, and puts both halves of state on one checkpoint schedule. It converges nothing: the caller names the height, because only the caller knows what the stores outside the StateDB can serve.recoverState—StateDB.RollbackTo(target).RollbackTois the single convergence primitive, and it is what readies a freshly opened StateDB as much as what rewinds a running one. It:Each half is handled independently of the other, which is what lets them have crashed at different heights.
New store-level APIs
flatkv.CommitStore.RewindToSnapshotAtOrBelow—Rollbackfor a store whose WAL an outer context owns. It moves only between snapshot boundaries, so no WAL crosses the API and replaying forward from the version it returns is the caller's to do. It sharesrepointAtSnapshotwith the existingRollback, which is unchanged.flatkv.StateWALPath/StateWALConfig— how the WAL's owner locates and reopens it without repeating FlatKV's layout convention.evm.EVMStateStore.RewindToSnapshotAtOrBelow/ApplyReplayedBlock— the SS mirror of the split above: the rewind moves between snapshot boundaries and discards the snapshots above the target, and applying one replayed block is the per-block stepStateDB's replay drives. No WAL crosses either, so both halves of state reach the same replay and its missing-blocks check.snapshot.Manager.RemoveSnapshotsAbove— drops the snapshots a rollback has just discarded the history for, repointingcurrentat the newest survivor.receipt.Rollback— offline rollback of the LittDB receipt bodies plus a rewind of the PebbleDB tag index.statewal.GetRange/PruneAfter/VerifyIntegritynow take a directory path rather than a*Config, so they can run against a WAL directory with no live instance open.StateDBcontract gainsRollbackToandClose.gigacontracts moved togiga/typesflatkvandcompositeimportedgigaforLiveStateStoreandStateView, which meantgigacould not importflatkvto construct SC from config. Those interfaces and the EVM value types in their signatures now live insei-db/state_db/giga/types(imported asgigatypes), a package with no implementation. The dependency runs one way again: thegigaimplementation importsflatkv, and both sides depend only on the contract package.Smaller changes
DefaultGigaStorageConfigreturns*GigaStorageConfig, andValidatetakes a pointer receiver and guards nil. Adds theWithValidatorMode/WithFullNodeMode/With{Account,Storage,Code}DBCacheSizebuilders.GigaStorageManageris down to the block store, the receipt store, the StateDB and the collector.SC()/SS()/StateWAL()delegate to the StateDB,prunableStorestakes the state stores from it as a group, andClosereports every store's failure rather than stopping at the first.Testing performed to validate your change
sei-db/bootstrap/recovery_test.gocovers each skew a crash can leave behind:TestRecoverSCReplaysAMissedWALBlockTestRecoverSCRollsBackToTheTargetTestRecoverSCAboveTheWALHeadRewindsToASnapshotAndReplaysTestRecoverSSReplaysEVMChangesetsTestRecoverSSRollsBackToTheTargetTestRecoverReceiptRewindsTheHeadTestRecoverStateDropsWALBlocksAboveTheTargetTestStateDBRollbackToRewindsBothHalvesAndTheWALTestFindTargetRecoveryHeightIsZeroWithoutABlockLedgerTestOpenDBWithoutRecoveryOnAFreshHomestorage_manager_test.goadditionally covers the checkpoint schedule reaching both halves of state, every store joining the prune cycle in order, andCloseon a partial open.TestCatchUpRefusesAWALMissingTheBlocksAStoreNeedscovers both halves refusing a WAL pruned past a store's head,TestRecoverSSRemovesSnapshotsAboveTheTargetthe snapshot tree after a rollback, andTestHealInterruptedRestorea snapshot restore interrupted between its two renames.Verified with
scripts/ramtest.sh ./sei-db/...(56 packages green) andscripts/ramtest.sh ./giga/..., plusmake fmtcheckand golangci-lint v2.8.0 clean ongiga,bootstrapandflatkv.Known gaps, called out for review
None of these are regressions; they are limits of what this PR reaches.
CommitStateChangesstill does not write SS (TODO: Commit changes to SS). SS recovery is implemented and tested, but on the commit path SS only moves when something else populates it, so the SS half of convergence is not yet exercised by ordinary block production.StateDB.WAL()must therefore be re-read after anyRollbackTo, andRollbackTomust not run once the prune cycle holds the WAL. Recovery finishing beforestartGarbageCollectoris what keeps that safe today; it is not enforced.receipt.Rollbacksupports only thelittidxbackend and refuses any other.OpenViewAtpanics — serving a past height needs the historical state DB, which is not wired intoStateDB.