Skip to content

Add crash recovery logic for GigaStorageManager - #4079

Open
yzang2019 wants to merge 21 commits into
mainfrom
yzang/crash-recovery
Open

Add crash recovery logic for GigaStorageManager#4079
yzang2019 wants to merge 21 commits into
mainfrom
yzang/crash-recovery

Conversation

@yzang2019

@yzang2019 yzang2019 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Describe your changes and provide context

Adds crash recovery to GigaStorageManager, so that after an unclean shutdown:

  1. Every store is at or below the block store's height.
  2. Every store other than the block store is on the same height.

Getting there needed one structural change: giga.StateDB now 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, StateDB is the only writer, and the replay that brings either half onto a height reads through that one WAL.

Recovery flow

OpenDBWithRecovery opens 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 through statewal.GetRange, with no live WAL open. A target of 0 means 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 through receipt.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.
  • recoverStateStateDB.RollbackTo(target).

RollbackTo is the single convergence primitive, and it is what readies a freshly opened StateDB as much as what rewinds a running one. It:

  1. rewinds SC to a snapshot boundary at or below the target, when SC sits above it;
  2. truncates the WAL tail to the target, so the next commit is the block after it;
  3. rewinds SS to its newest snapshot at or below the target, when SS sits above it, discarding the snapshots above it;
  4. replays the WAL forward into SC;
  5. replays the WAL forward into SS.

Each half is handled independently of the other, which is what lets them have crashed at different heights.

New store-level APIs

  • flatkv.CommitStore.RewindToSnapshotAtOrBelowRollback for 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 shares repointAtSnapshot with the existing Rollback, 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 step StateDB'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, repointing current at the newest survivor.
  • receipt.Rollback — offline rollback of the LittDB receipt bodies plus a rewind of the PebbleDB tag index.
  • statewal.GetRange / PruneAfter / VerifyIntegrity now take a directory path rather than a *Config, so they can run against a WAL directory with no live instance open.
  • The StateDB contract gains RollbackTo and Close.

giga contracts moved to giga/types

flatkv and composite imported giga for LiveStateStore and StateView, which meant giga could not import flatkv to construct SC from config. Those interfaces and the EVM value types in their signatures now live in sei-db/state_db/giga/types (imported as gigatypes), a package with no implementation. The dependency runs one way again: the giga implementation imports flatkv, and both sides depend only on the contract package.

Smaller changes

  • DefaultGigaStorageConfig returns *GigaStorageConfig, and Validate takes a pointer receiver and guards nil. Adds the WithValidatorMode / WithFullNodeMode / With{Account,Storage,Code}DBCacheSize builders.
  • GigaStorageManager is down to the block store, the receipt store, the StateDB and the collector. SC() / SS() / StateWAL() delegate to the StateDB, prunableStores takes the state stores from it as a group, and Close reports every store's failure rather than stopping at the first.

Testing performed to validate your change

sei-db/bootstrap/recovery_test.go covers each skew a crash can leave behind:

Test Skew
TestRecoverSCReplaysAMissedWALBlock SC behind the WAL
TestRecoverSCRollsBackToTheTarget SC above the target
TestRecoverSCAboveTheWALHeadRewindsToASnapshotAndReplays SC above the WAL head, via a snapshot
TestRecoverSSReplaysEVMChangesets SS behind the WAL
TestRecoverSSRollsBackToTheTarget SS above the target
TestRecoverReceiptRewindsTheHead receipts above the target
TestRecoverStateDropsWALBlocksAboveTheTarget WAL above the target
TestStateDBRollbackToRewindsBothHalvesAndTheWAL both halves and the WAL in one call
TestFindTargetRecoveryHeightIsZeroWithoutABlockLedger target computation
TestOpenDBWithoutRecoveryOnAFreshHome fresh node, every height zero

storage_manager_test.go additionally covers the checkpoint schedule reaching both halves of state, every store joining the prune cycle in order, and Close on a partial open.

TestCatchUpRefusesAWALMissingTheBlocksAStoreNeeds covers both halves refusing a WAL pruned past a store's head, TestRecoverSSRemovesSnapshotsAboveTheTarget the snapshot tree after a rollback, and TestHealInterruptedRestore a snapshot restore interrupted between its two renames.

Verified with scripts/ramtest.sh ./sei-db/... (56 packages green) and scripts/ramtest.sh ./giga/..., plus make fmtcheck and golangci-lint v2.8.0 clean on giga, bootstrap and flatkv.

Known gaps, called out for review

None of these are regressions; they are limits of what this PR reaches.

  • CommitStateChanges still 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.
  • Rolling SS back needs a snapshot at or below the target. SC always has one, so its rewind cannot fail this way; SS has one only where a checkpoint landed at or below the target, and there is no fallback to replaying from empty.
  • The checkpoint schedule is live during recovery replay, so a replay that crosses a boundary snapshots there. That costs some recovery time and is the reason the schedule is documented as being in place before the StateDB is put on a height.
  • A WAL truncation replaces the handle, and nothing re-hands the new one out. StateDB.WAL() must therefore be re-read after any RollbackTo, and RollbackTo must not run once the prune cycle holds the WAL. Recovery finishing before startGarbageCollector is what keeps that safe today; it is not enforced.
  • receipt.Rollback supports only the littidx backend and refuses any other.
  • OpenViewAt panics — serving a past height needs the historical state DB, which is not wired into StateDB.

@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes startup storage convergence, WAL truncation, and multi-store rollback/replay—any bug can cause data loss or a node that refuses to start after a crash.

Overview
Implements unclean-shutdown recovery for Giga nodes by replacing the no-op CrashRecover with OpenDBWithRecovery, wired into NewGigaStorageManager. On startup it picks a target height (minimum of block store, state WAL, and receipt heads, with guards so an empty receipt store does not collapse the target), then aligns receipts, SC, SS, and the state WAL on that height before opening the receipt store last.

giga.StateDB is now the owner of FlatKV (SC), optional EVM SS, the shared state WAL, and the shared checkpoint scheduler. The manager no longer opens those pieces separately or double-writes the WAL. RollbackTo is the convergence primitive: preflight reachability, snapshot rewinds, WAL tail truncation/reopen, snapshot cleanup above the target, and WAL replay into SC and SS (with special handling when SS is empty or the WAL no longer reaches block 1).

Supporting API moves include gigatypes (sei-db/state_db/giga/types) for StateDB / LiveStateStore / StateView contracts, RewindToSnapshotAtOrBelow and RemoveSnapshotsAbove on FlatKV and SS, offline receipt.GetLatestBlock / receipt.PruneAfter for receipt rewind, StateWALConfig export for WAL layout, and DefaultGigaStorageConfig returning a pointer plus WithValidatorMode / WithFullNodeMode / cache-size builders. AGENTS.md now tells contributors to use scripts/ramtest.sh for store-backed tests.

Extensive tests live in recovery_test.go and related store tests; CommitStateChanges still does not commit SS on the live path (existing TODO).

Reviewed by Cursor Bugbot for commit 529d7c3. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 4, 2026, 1:28 AM

@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 34.95298% with 415 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.66%. Comparing base (2153e61) to head (529d7c3).

Files with missing lines Patch % Lines
sei-db/state_db/giga/state_db_impl.go 11.73% 192 Missing and 11 partials ⚠️
sei-db/state_db/ss/evm/recovery.go 16.04% 65 Missing and 3 partials ⚠️
sei-db/state_db/sc/flatkv/snapshot.go 29.41% 27 Missing and 9 partials ⚠️
sei-db/bootstrap/recovery.go 64.38% 14 Missing and 12 partials ⚠️
sei-db/config/giga_config.go 13.04% 19 Missing and 1 partial ⚠️
sei-db/state_db/ss/snapshot/manager.go 60.00% 8 Missing and 8 partials ⚠️
sei-db/state_db/ss/evm/checkpoint.go 0.00% 15 Missing ⚠️
sei-db/state_db/ss/evm/store.go 67.64% 7 Missing and 4 partials ⚠️
sei-db/bootstrap/storage_manager.go 64.28% 6 Missing and 4 partials ⚠️
giga/evmonly/memory_store.go 60.00% 6 Missing ⚠️
... and 1 more
Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
sei-chain-pr 64.52% <58.22%> (?)
sei-db 69.80% <ø> (ø)
sei-db-state-db ?
sei-db-state-db-pr 70.82% <27.29%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
giga/evmonly/executor.go 86.94% <100.00%> (ø)
giga/evmonly/giga_store.go 90.69% <ø> (ø)
sei-db/controller/checkpoint_scheduler.go 100.00% <ø> (ø)
sei-db/state_db/sc/composite/store.go 71.29% <100.00%> (ø)
sei-db/state_db/sc/flatkv/state_view.go 76.36% <100.00%> (ø)
sei-db/state_db/sc/flatkv/store.go 78.75% <100.00%> (ø)
sei-db/state_db/sc/flatkv/store_meta.go 79.64% <100.00%> (ø)
sei-db/state_db/sc/flatkv/store_read.go 62.31% <100.00%> (ø)
sei-db/state_db/sc/flatkv/verify.go 48.06% <100.00%> (ø)
sei-db/state_db/sc/flatkv/wal_glue.go 100.00% <100.00%> (ø)
... and 17 more

... and 63 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

seidroid[bot]
seidroid Bot previously requested changes Sep 2, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 OpenDBWithRecovery against a home directory whose stores actually disagree. Every new test drives the private helpers directly on a freshly opened manager, and TestOpenDBWithoutRecoveryOnAFreshHome only 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.go now holds an ss field that CommitStateChanges never 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] flatkvStateWALName in sei-db/tools/cmd/seidb/operations/flatkv_open.go is now dead after the GetRange signature change — its only remaining reference is its own declaration.
  • 3 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread sei-db/bootstrap/recovery.go Outdated
Comment thread sei-db/bootstrap/recovery.go Outdated
Comment thread sei-db/bootstrap/recovery.go
Comment thread sei-db/state_db/ss/evm/recovery.go Outdated
Comment thread sei-db/state_db/ss/evm/recovery.go
Comment thread sei-db/ledger_db/receipt/rollback.go Outdated
Comment thread sei-db/state_db/giga/state_db_impl.go Outdated
Comment thread sei-db/bootstrap/recovery.go
Comment thread sei-db/bootstrap/recovery.go Outdated
* main:
  Add Giga checkpoint mechanism to EVM SS (#4073)
  fix(seidb): refuse a corrupted changelog in digest replay instead of repairing it (#3983)
Comment thread sei-db/bootstrap/recovery.go
Comment thread sei-db/state_db/ss/evm/recovery.go
@yzang2019

Copy link
Copy Markdown
Contributor Author

@seidroid review

Comment thread sei-db/state_db/giga/state_db_impl.go
seidroid[bot]
seidroid Bot previously requested changes Sep 3, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 OpenDBWithRecovery end to end against a home directory whose stores actually disagree. Every new test calls openStateDB/recoverState/recoverReceipt directly, and the only findTargetRecoveryHeight test 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 through NewGigaStorageManager, assert every head converged) is what would catch the two target-0 defects below.
  • [suggestion] CommitStateChanges still 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 the RollbackTo/recovery godoc while the TODO stands, since the CatchUpFrom coverage check depends on the answer.
  • [suggestion] flatkvStateWALName in sei-db/tools/cmd/seidb/operations/flatkv_open.go:27 became dead when GetRange switched 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.

Comment thread sei-db/state_db/giga/state_db_impl_test.go Outdated
Comment thread sei-db/bootstrap/recovery.go Outdated
Comment thread sei-db/bootstrap/recovery.go
Comment thread sei-db/state_db/ss/evm/recovery.go Outdated
Comment thread sei-db/state_db/ss/evm/recovery.go
Comment thread sei-db/state_db/ss/evm/recovery.go
Comment thread sei-db/state_db/ss/evm/recovery.go
Comment thread sei-db/ledger_db/receipt/rollback.go Outdated
@yzang2019

Copy link
Copy Markdown
Contributor Author

@seidroid review

seidroid[bot]
seidroid Bot previously requested changes Sep 3, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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] flatkvStateWALName in sei-db/tools/cmd/seidb/operations/flatkv_open.go:27 is now dead — the GetRange path-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.

Comment thread sei-db/state_db/ss/evm/recovery.go
Comment thread sei-db/state_db/ss/evm/recovery.go
Comment thread sei-db/state_db/giga/state_db_impl.go
Comment thread sei-db/state_db/sc/flatkv/snapshot.go
Comment thread sei-db/state_db/statewal/state_wal_impl.go Outdated
Comment thread sei-db/state_db/statewal/state_wal_impl.go Outdated
@yzang2019

Copy link
Copy Markdown
Contributor Author

@seidroid review

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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] flatkvStateWALName in sei-db/tools/cmd/seidb/operations/flatkv_open.go:27 is dead — the GetRange path-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.restoreSnapshot with SeparateEVMSubDBs) 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 when separateDBs is 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 new RewindToSnapshotAtOrBelow: repointAtSnapshot demotes the store to the base snapshot, and removeSnapshotsAbove runs 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.

Comment thread sei-db/state_db/sc/flatkv/snapshot.go
Comment thread sei-db/state_db/giga/state_db_impl.go Outdated
Comment thread sei-db/state_db/statewal/state_wal.go Outdated
Comment thread sei-db/bootstrap/recovery.go
@seidroid
seidroid Bot dismissed stale reviews from themself September 3, 2026 17:23

Superseded: latest AI review found no blocking issues.

Comment thread sei-db/state_db/giga/state_db_impl.go
Comment thread sei-db/state_db/sc/flatkv/snapshot.go
Comment thread sei-db/bootstrap/recovery.go Outdated
Comment thread sei-db/bootstrap/recovery.go Outdated
// 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this method also be responsible for rolling back StateWAL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rollback StateWAL will be done in StateDB since WAL lifecycle is managed by StateDB now

Comment thread sei-db/bootstrap/recovery.go Outdated
Comment thread sei-db/ledger_db/receipt/rollback.go Outdated
if _, err := os.Stat(littDir); os.IsNotExist(err) {
return nil
}
return rollback.RollbackLittDB([]string{littDir}, func(_ string, key []byte, isPrimary bool) (bool, error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread sei-db/ledger_db/receipt/rollback.go Outdated
@yzang2019

Copy link
Copy Markdown
Contributor Author

@seidroid review

seidroid[bot]
seidroid Bot previously requested changes Sep 4, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.restoreSnapshot with SeparateEVMSubDBs) is now honestly documented as unrecoverable, but the code still lets it happen: the head reads as the minimum across sub-DBs, recoverSS/rewindSS classifies 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 when separateDBs is 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 new RewindToSnapshotAtOrBelow, so a crash between repointAtSnapshot and removeSnapshotsAbove leaves a rejected snapshot branch on disk. Present on the base branch; fixing both would want one ordering-safe helper rather than two call sites.

Comment thread sei-db/state_db/sc/flatkv/snapshot.go
Comment thread sei-db/state_db/giga/state_db_impl.go
Comment thread sei-db/bootstrap/recovery.go
Comment thread sei-db/state_db/giga/state_db_impl.go Outdated
yzang2019 and others added 3 commits September 3, 2026 17:33
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>
@yzang2019

Copy link
Copy Markdown
Contributor Author

@seidroid review

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.restoreSnapshot with SeparateEVMSubDBs) is now honestly documented as unrecoverable, but the code still lets it happen: the head reads as the minimum across sub-DBs, rewindSS classifies 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 when separateDBs is set would trade a documented silent divergence for a loud one. (Codex raises the same point.)
  • [suggestion] CommitStateChanges still 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 range catchUpSS must cover grow with uptime, and it is what turns the replay range check from a corruption detector into the thing that decides whether a node starts. Worth stating on RollbackTo/catchUpSS while 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 between repointAtSnapshot and removeSnapshotsAbove leaves a rejected snapshot branch on disk. The new RewindToSnapshotAtOrBelow has the same ordering but is now healed by its caller's unconditional dropSCSnapshotsAbove; Rollback has no equivalent, so seid rollback still carries the window. Present on the base branch.

Comment thread sei-db/state_db/giga/state_db_impl.go
Comment thread sei-db/state_db/giga/state_db_impl.go
Comment thread sei-db/bootstrap/recovery.go
@seidroid
seidroid Bot dismissed their stale review September 4, 2026 00:49

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>
@yzang2019

Copy link
Copy Markdown
Contributor Author

@seidroid review

@yzang2019
yzang2019 enabled auto-merge September 4, 2026 01:38
@yzang2019
yzang2019 disabled auto-merge September 4, 2026 01:38

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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] CommitStateChanges still 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 range catchUpSS must cover grow with uptime, and it is why ssFillsForward — rather than the replay range check — decides whether a full node starts. Worth stating on catchUpSS/RollbackTo while 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 between repointAtSnapshot and removeSnapshotsAbove strands a rejected snapshot branch that a later rollback can restore as authoritative. The new RewindToSnapshotAtOrBelow has the same ordering but is healed by its caller's unconditional dropSCSnapshotsAbove; Rollback (the seid rollback path) 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants