feat(data): integrate LittDB-backed BlockDB into autobahn data layer (CON-272)#3707
feat(data): integrate LittDB-backed BlockDB into autobahn data layer (CON-272)#3707wen-coding wants to merge 30 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
PR SummaryHigh Risk Overview
Node/config: optional Tests are largely rewritten around BlockDB recovery, pruning, and pre- Reviewed by Cursor Bugbot for commit 88a10c4. 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).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3707 +/- ##
==========================================
- Coverage 60.14% 59.17% -0.97%
==========================================
Files 2303 2211 -92
Lines 192032 181694 -10338
==========================================
- Hits 115491 107516 -7975
+ Misses 66230 64722 -1508
+ Partials 10311 9456 -855
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.
Solid, well-tested migration from the bespoke WAL persist layer to a single LittDB-backed BlockDB; the NewState recovery logic (QC skipTo/insertQC pass, block gap/coverage guards) is carefully constructed and matches the BlockDB ascending-iterator and QC-before-block contracts. No blockers, but a couple of non-blocking issues: the empty-string PersistentStateDir guard is defeated by rootify, and the new State.Close() is never wired into shutdown.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- cursor-review.md is empty — the Cursor second-opinion pass produced no output. Codex produced one finding (the PersistentStateDir guard, addressed inline).
- Doc-comment detachment nit: inserting
newTestBlockDBinconsensus/inner_test.goorphans the// seedPersistedInner ...comment above it, and adding a blank line beforenewTestStateinconsensus/state_test.godetaches that function's godoc comment. Harmless but slightly misleading; consider moving the helper below or removing the blank line. - Persistence is now mandatory (the
utils.Option[string]→stringchange removes the oldNone= in-memory/no-persistence mode). This is an intentional behavior change worth calling out in release notes for operators who previously ran with--persistent-state-dir=to disable persistence. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| func rootifyPersistentStateDir(rootDir string, c *p2p.GigaRouterCommonConfig) { | ||
| if dir, ok := c.PersistentStateDir.Get(); ok && !filepath.IsAbs(dir) { | ||
| c.PersistentStateDir = utils.Some(filepath.Join(rootDir, dir)) | ||
| if !filepath.IsAbs(c.PersistentStateDir) { |
There was a problem hiding this comment.
[suggestion] rootifyPersistentStateDir treats an empty string as a relative path: filepath.IsAbs("") is false, so filepath.Join(rootDir, "") rewrites PersistentStateDir to rootDir. Because this runs at config-load time (setup.go:314/325) before buildDataState, the new cfg.PersistentStateDir == "" guard in giga_router_common.go:70 can never fire on the node path. An omitted or explicitly empty persistent_state_dir therefore silently creates BlockDB at <home>/blockdb instead of producing the explicit startup error the PR describes (and instead of the old "persistence disabled" behavior). Consider special-casing empty here (leave it empty so the guard fires) or dropping the now-unreachable guard and documenting the new default. (Matches Codex's P1, downgraded to a suggestion since the flag defaults to a non-empty "data/autobahn".)
| } | ||
|
|
||
| // Close releases resources held by the BlockDB backing this State. | ||
| func (s *State) Close() error { |
There was a problem hiding this comment.
[suggestion] State.Close() (which closes the backing LittDB) is only ever called from tests — the validator/fullnode routers never invoke r.data.Close() on shutdown, so LittDB's resources (background GC, file locks, mmaps) aren't released on graceful shutdown. Note hashVault is explicitly closed on runExecute exit, so there's precedent; wiring data.State.Close() into the router teardown (e.g. via scope/defer) would make shutdown symmetric and avoid a stale-lock risk on the next start.
There was a problem hiding this comment.
Solid, well-tested refactor swapping the WAL persistence layer for a LittDB-backed BlockDB with clean restart recovery; no blocking correctness issues, but a few non-blocking items: a stale CLI flag help string, a constructor-path resource leak, and a note about prune durability across restarts.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Prune is not durable across restart (Codex P1, downgraded):
PruneBeforeonly advances the in-memorywatermark(atomic.Uint64 reset to 0 on reopen) and GC is asynchronous, so a restart before GC physically reclaims pruned entries makesNewStatere-load previously-pruned blocks, temporarily regressinginner.first/the retain window until the next prune cycle re-advances. This appears consistent with littblock's documented TTL-failsafe design and is self-healing (data stays valid and QC-covered; no consensus/correctness break), so it reads as acceptable rather than a bug — but worth confirming the widened retain window after restart is intended. - Validator constructor leaks the BlockDB on an error path: in
NewGigaValidatorRouter, ifconsensus.NewStatefails, the already-constructeddataState(holding an open LittDB with file locks) is never closed.buildDataStatewas fixed to close the DB whendata.NewStatefails, but the caller does not closedataStateon the subsequentconsensus.NewStateerror. This is a pre-existing pattern (the old DataWAL had the same gap) but is easy to fix while this area is being reworked. (The fullnode constructor has no subsequent fallible step, so it is unaffected.) - cursor-review.md is empty — the Cursor second-opinion pass produced no output; only Codex provided findings (both of which are reflected here).
- Minor: the new
_ = r.data.Close()in both routers'Runswallows the close error; consider at least logging it so a flush/close failure on shutdown is observable. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
The PR cleanly swaps the ad-hoc dual-WAL persistence (fullcommitqcs/globalblocks) for a single LittDB-backed BlockDB and simplifies PersistentStateDir from Option[string] to string; the refactor is well-organized and tests are migrated. However, there is a real durability gap in the prune/restart path (also flagged by Codex) that the new tests do not actually exercise, so it should be resolved or explicitly justified before merge.
Findings: 3 blocking | 4 non-blocking | 2 posted inline
Blockers
- PruneBefore's watermark is not durable, so restart-before-GC re-exposes 'pruned' data (Codex High). data.State.PruneBefore delegates to blockDB.PruneBefore(n), which per the BlockDB contract only advances an in-memory async watermark (LittDB reclaims lazily on GC, gated by a retention TTL). On restart before reclamation, NewState iterates blockDB.Blocks(false)/QCs(false), which still yield below-watermark records, and sets inner.first to the lowest surviving block (state.go:282). This moves inner.first backward relative to the pre-crash retain window, so Block/QC/GlobalBlockByHash serve blocks that were logically pruned instead of returning ErrPruned — contradicting the guarantee TestRecoveryAfterPruning claims to verify. It also risks a first/nextAppProposal mismatch on restart (reloaded old blocks below where consensus resumes app-proposing). Please either persist a durable prune anchor (or force/await reclamation on prune) so recovery starts at the intended watermark, or document why re-exposing finalized blocks after restart is safe and confirm runPruning re-establishes the watermark without wedging.
- TestRecoveryAfterPruning does not exercise the scenario in the blocker above: instead of calling PruneBefore then reopening the same BlockDB, it manually writes only qc2/qc3 (pre-assuming GC already reclaimed qc1). No test drives the real path (write → PruneBefore → close → reopen before GC), so the un-reclaimed-restart behavior is untested and the ErrPruned assertion gives false confidence. Add a test that prunes via State.PruneBefore and then constructs a new State over the same on-disk BlockDB.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Config compatibility: AutobahnFileConfig.PersistentStateDir changed from utils.Option[string] to plain string. Option[string] marshals absent as JSON null; a plain string field unmarshals null to "", which buildDataState now rejects ("must not be empty"). Nodes whose existing autobahn config has persistent_state_dir: null (previously = disable persistence / in-memory) will fail to start. This appears intentional (the empty-string guard makes missing config explicit), but call it out in release notes/upgrade guidance.
- cursor-review.md was empty — the Cursor second-opinion pass produced no output; only Codex's review was available to merge.
- Minor style: the new newTestBlockDB / newTestState helpers in inner_test.go, consensus/inner_test.go, and consensus/state_test.go have a stray blank line between the preceding doc comment and the function, detaching the comment from the symbol it documents.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| PersistentStateDir: utils.Some(cfg.PersistentStateDir), | ||
| }, dataState) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("consensus.NewState(): %w", err) |
There was a problem hiding this comment.
[suggestion] Resource leak on this error path: buildDataState opened a durable BlockDB (LittDB/Pebble) but if consensus.NewState fails here, dataState is dropped without dataState.Close(), leaking file handles/goroutines. The successful path only closes it in Run(). Add _ = dataState.Close() before returning the error.
5c04a9a to
acdb1ac
Compare
The contract with BlockDB is that they will always return consistent state upon restart. It could be that the pruning of LittDB is behind the pruning of in-memory state, which may make old QCs/blocks reappear after returning errPruned, which is okay behavior. We don't have guarantee that it's wrong to returning actual data after returning errPruned previously. It's only wrong to miss the QCs/blocks we advertised newer than the BlockDB watermark (which should always be older or equal to the in-memory watermark).
And this inconsistency window is small, on next prune the same AppQC should cause the same blocks to be pruned.
There was a problem hiding this comment.
A well-scoped refactor swapping the WAL persist layer for a single LittDB-backed BlockDB and making PersistentStateDir mandatory; logic and recovery paths are sound, with one notable (non-blocking) semantic change around pruning durability and a couple of minor config-migration notes.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Config format change:
AutobahnFileConfig.PersistentStateDirchanged fromutils.Option[string]to plainstring, so the JSON representation ofpersistent_state_dirchanged. Any autobahn config file serialized by a prior build may need regeneration viagen_autobahn_config; worth calling out in release notes. gen_autobahn_confignow unconditionally setscfg.PersistentStateDir = persistentStateDireven for--persistent-state-dir=(empty). The flag help says 'must not be empty', but an empty value is written to the config and only rejected later at node startup inbuildDataState. Consider validating (rejecting empty) at generation time for a clearer, earlier failure.- Good cleanup: the mandatory-persistence change lets the PR remove the stale 'None ⇒ DANGEROUS/SLASHING' warnings and no-op-persister branches; the fail-fast empty-string guard in
buildDataStateand the addeddataState.Close()/r.data.Close()cleanup paths are solid improvements. - Cursor produced no review output (empty file); Codex produced a single finding (pruning durability at data/state.go:636), addressed inline.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // Prune BlockDB outside the lock to avoid holding it during disk I/O. | ||
| if n, ok := truncateTo.Get(); ok { | ||
| return s.dataWAL.TruncateBefore(n) | ||
| return s.blockDB.PruneBefore(n) |
There was a problem hiding this comment.
[suggestion] Pruning durability regression vs. the old WAL path (also flagged by Codex). blockDB.PruneBefore(n) only advances an in-memory atomic.Uint64 watermark in littblock — it is not persisted, and GC is asynchronous. On restart the watermark resets to 0 and NewState's Blocks()/QCs() iterators surface every record GC hasn't yet physically reclaimed, so recovery rebuilds inner.first/nextQC from below-watermark data and inner.first lands lower than the last watermark — i.e. pruned blocks/QCs reappear after a restart-before-GC. The old DataWAL.TruncateBefore truncated on disk synchronously, so this is a behavioral change.
Impact is bounded (resurrected data is valid committed data → no consensus-safety issue; the 24h Retention TTL still bounds retention durably; runPruning re-prunes after restart so it self-heals), so this is a suggestion rather than a blocker. But note the new recovery tests (TestRecoveryAfterPruning, TestPruningKeepsLastQCRange) simulate the post-GC on-disk state (writing only surviving QCs to a fresh DB) instead of exercising real PruneBefore → reopen, so this exact path is currently untested. Consider a test that prunes, reopens the same DB without ForceGC, and asserts the expected inner.first, plus a comment here documenting that prune durability relies on the TTL failsafe rather than on PruneBefore being immediately durable.
wen-coding
changed the title
There was a problem hiding this comment.
Careful refactor replacing the no-op-capable WAL persistence layer with a required LittDB-backed BlockDB, threading PersistentStateDir from Option[string] to a plain string, and fixing a BlockDB handle leak on the consensus.NewState error path. The core recovery/persist logic matches the BlockDB ordering contract and the config change is JSON-backward-compatible; only minor test-naming/comment nits remain.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes produced nothing actionable: cursor-review.md is empty, and codex-review.md reports 'No material findings' but notes it could not run tests (no Go 1.25.6 / no network in its sandbox), so its clean result is not test-backed.
- The dataState.Close() leak fix in NewGigaValidatorRouter is correct and the fullnode path is safe (buildDataState is the only fallible step and closes blockDB internally), and Run() now closes r.data after scope.Run drains all goroutines — good lifecycle handling; no double-close since a failed constructor never returns a router.
- BlockByHash still delegates to the in-memory hash index (pruned per RetainHeight) rather than BlockDB.ReadBlockByHash (retained per TTL), so hashes below the in-memory watermark read as 'unknown' even though BlockDB may still hold them; this matches prior behavior and is documented via TODO, but worth tracking as an intended follow-up.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
Comments that couldn't be anchored to the diff
sei-tendermint/internal/autobahn/consensus/persist/commitqcs_test.go:242-- [nit] Same as the blocks test:TestNoOpCommitQCPersisternow passest.TempDir(), so it no longer tests a no-op persister and overlapsTestNewCommitQCPersisterEmptyDir. Rename or fold in.
There was a problem hiding this comment.
Solid refactor replacing the temporary WAL data-persist layer with a LittDB-backed BlockDB; recovery logic honors the BlockDB write contract, tests are thorough, and it fixes a BlockDB handle leak. No correctness blockers found — only upgrade/config-compatibility notes.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output.
- Upgrade/migration story should be documented: nodes upgrading with existing
globalblocks//fullcommitqcs/WAL data will not migrate it into the newblockdb/store and will start the data layer empty (relying on state-sync/re-sync). Given the data layer only holds QC-covered committed blocks and Autobahn appears pre-production, this is likely acceptable, but the removal of the old WALs plus the switch toblockdb/warrants an explicit upgrade note (mirrors the Codex finding). - The consensus persister (PersistedInner) remains WAL-based via persist.NewPersister, so consensus state still recovers across restart even though the data layer does not migrate — worth calling out in the PR description so the asymmetry is intentional and understood.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Solid refactor replacing the dual-WAL persist layer with a single LittDB-backed BlockDB, and eliminating the risky in-memory "no persistence" mode (which could cause slashing on restart). Recovery logic conforms to the documented BlockDB contract and is well-tested; the only notable issue is silently dropped BlockDB Close() errors on the router shutdown path.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — no findings were produced by that pass. Codex produced one finding (the swallowed Close() errors), reflected inline below.
- REVIEW_GUIDELINES.md in the base branch is empty, so no repo-specific standards were applied beyond AGENTS.md conventions.
- runPersist now writes QCs then blocks then a single Flush per batch (correctly matching the BlockDB QC-before-block contract). Consider a brief test asserting a crash between WriteBlock and Flush leaves nextBlockToPersist unadvanced so no committed block is skipped on restart — the existing restart tests cover the happy path but not the mid-batch flush boundary.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| s.SpawnNamed("service", func() error { return r.service.Run(ctx) }) | ||
| return nil | ||
| }) | ||
| _ = r.data.Close() |
There was a problem hiding this comment.
[suggestion] r.data.Close() now closes the LittDB-backed BlockDB, but its error is discarded. If scope.Run returns nil (clean shutdown) while Close fails (e.g. a flush-on-close failure), that failure is silently lost with no log. Prefer surfacing it: return errors.Join(err, r.data.Close()). (Flagged by Codex.)
| s.SpawnNamed("service", func() error { return r.service.Run(ctx) }) | ||
| return nil | ||
| }) | ||
| _ = r.data.Close() |
There was a problem hiding this comment.
[suggestion] Same as the validator router: the BlockDB Close() error is swallowed. On a clean scope.Run return a Close/durability failure would go unreported. Use return errors.Join(err, r.data.Close()) (or at least log the close error). (Flagged by Codex.)
There was a problem hiding this comment.
A well-structured migration from the WAL persist layer to a LittDB-backed BlockDB that also removes the dangerous in-memory/no-op persistence mode and fixes several resource leaks. No blocking issues found; the main discussion point is the non-durable prune watermark that Codex flagged, which the author has documented as a deliberate, self-healing tradeoff.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Config compatibility:
AutobahnFileConfig.PersistentStateDirchanged fromutils.Option[string]to plainstring, andbuildDataState/gen_autobahn_confignow hard-fail on an empty value. This is intentional (making missing config explicit rather than silently running in-memory), but any hand-written config JSON that omits the field, or was serialized with the old Option encoding, will now fail node startup with "PersistentStateDir must not be empty". Worth a migration/release note. - Cursor's second-opinion review (
cursor-review.md) was empty — that pass produced no output. Codex's single P2 finding is addressed below. - Test coverage for the new BlockDB-backed recovery is solid (restart-from-persisted, gap detection, corrupt data), but there is no test exercising the specific Codex scenario: PruneBefore advances the watermark, the process restarts before async GC reclaims, and NewState reloads below-watermark data (verifying inner.first moves back and the state still self-heals). Consider adding one to lock in the documented "safe/self-healing" behavior.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Solid, well-documented refactor swapping the WAL persistence layer for a LittDB-backed BlockDB, with careful restart-restore logic, correct resource cleanup, and thorough test updates. No blockers; the main note is a deliberate behavior change (non-persisted prune watermark + 24h default retention) whose restart-replay footprint should be confirmed acceptable for high-throughput production nodes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Restart-replay volume regression (raised by Codex, confirmed): PruneBefore now only advances an in-memory watermark and BlockDB reclamation is async under a 24h default Retention TTL (littblock DefaultConfig, Retention: 24*time.Hour). Unlike the old WAL TruncateBefore, which physically truncated to the retain window, NewState's block/QC iterators replay every not-yet-reclaimed entry on startup, so a node restarted before GC runs can load up to ~24h of blocks/QCs into memory and set inner.first below the pre-crash watermark. It is correctness-safe (resurrected data is QC-covered and self-heals on the next runPruning cycle) and explicitly documented, but on a high-throughput chain this could mean slow startup or elevated memory pressure. Worth confirming the 24h Retention default is appropriate for production, or bounding startup replay.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no findings to incorporate.
- PersistentStateDir semantics changed: an empty/absent value now hard-errors at startup (gen_autobahn_config and buildDataState) instead of falling back to in-memory mode. JSON back-compat is fine (Option[string] marshals to a bare string or null, both parse into the plain string field), but any existing config that relied on null/empty to run in-memory will now fail to start. This appears intentional (in-memory mode removed) — just flagging for release notes/operator awareness.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Clean, well-tested refactor replacing the WAL-based Autobahn persistence layer with a single LittDB-backed BlockDB; the recovery logic is consistent with the documented BlockDB durability/ordering contract and the config-type change is backward-compatible. No blocking issues found; only minor lifecycle observations.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported no material findings (noting only that it could not run tests because the sandbox has Go 1.24.13 and downloading Go 1.25.6 was blocked); the Cursor review file was empty (no output produced).
- BlockDB lifecycle: data.State (and thus blockDB) is only closed inside gigaFullnodeRouter.Run / gigaValidatorRouter.Run via errors.Join(err, r.data.Close()). A router that is successfully constructed but never Run (e.g. node setup aborts between NewGigaXRouter and Run) would leak the BlockDB handle. Low priority and largely matches the prior pattern, but worth confirming the node always reaches Run once a router is built.
- The QC-recovery block treats a block whose number is >= nextQC as fatal 'no QC coverage' corruption. This is correct given the BlockDB contract guarantees blocks are always QC-covered, but it removes the old reconcile path (case 7) that tolerated blocks-ahead-of-QCs by truncating; if any future BlockDB implementation could durably surface a block past its QC, startup would fail permanently. Fine under the current contract — flagging only as a design assumption to keep in mind.
There was a problem hiding this comment.
A large, well-tested refactor that replaces the WAL-based data persistence layer with a single LittDB-backed BlockDB and makes PersistentStateDir mandatory (removing the slashing-risky in-memory mode). No hard blockers found, but the switch from durable WAL truncation to BlockDB's asynchronous, non-persisted prune watermark is a real behavioral change worth confirming.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Pruning durability (raised by Codex): PruneBefore/runPruning now advance an in-memory-only, asynchronously-GC'd watermark on BlockDB, whereas the old DataWAL truncation was durable. On a restart before GC reclaims entries, NewState can see below-watermark blocks/QCs and set inner.first lower than the pre-crash watermark. The code comment claims this 'self-heals on the next runPruning cycle', but on any restart inner.appProposals is not restored and nextAppProposal is reset to inner.first, so both PruneBefore (firstToKeep = min(retainFrom, nextAppProposal) <= first) and runPruning (guarded by inner.first+1 < inner.nextAppProposal) are paused until block execution re-progresses through the resurrected region and repopulates appProposals. Please confirm the execute path reliably resumes from inner.first and repopulates appProposals on restart; otherwise reclamation is deferred and consumers may be served below-retain blocks instead of ErrPruned. (See inline note on PruneBefore.)
- Lifecycle nit: data.State/BlockDB is only closed via Run() (errors.Join(err, r.data.Close())). A router constructed successfully but whose Run() is never invoked (e.g. node startup aborts after construction) leaks the BlockDB handle. Consider documenting the 'Run owns Close' contract or closing on a dedicated shutdown path.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output; Codex's single finding is incorporated above.
- Positive: consolidating to one BlockDB with a single Flush per batch eliminates the previous cross-WAL reconcile() complexity and the dangerous no-op persistence mode, and making PersistentStateDir mandatory closes a real slashing footgun. Config JSON stays backward-compatible (Option[string] marshaled as bare string; null now maps to empty and is explicitly rejected).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| truncateTo = utils.Some(inner.first) | ||
| } | ||
| // Truncate WALs outside the lock to avoid holding it during disk I/O. | ||
| // Prune BlockDB outside the lock to avoid holding it during disk I/O. |
There was a problem hiding this comment.
[suggestion] This is the crux of Codex's concern and a real behavioral change from the old durable WAL truncation. Because BlockDB's prune watermark is in-memory and GC is asynchronous/not persisted, a restart before GC can resurrect below-watermark (but QC-covered) data, and NewState will set inner.first below the pre-crash watermark. The 'self-heals on the next runPruning cycle' claim relies on appProposals being repopulated — but NewState does not restore appProposals and resets nextAppProposal to inner.first, so both PruneBefore (firstToKeep <= inner.first → early return) and runPruning (guarded by inner.first+1 < inner.nextAppProposal) stay paused until block execution re-advances through the resurrected region. Please confirm execution reliably resumes from inner.first after restart (repopulating appProposals) so pruning re-advances; otherwise reclamation is deferred indefinitely and RPC consumers get below-retain blocks instead of ErrPruned.
There was a problem hiding this comment.
Clean, well-tested refactor replacing the data-layer WAL (fullcommitqcs/globalblocks) with a LittDB-backed BlockDB and dropping the no-op/Option[string] persistence mode; PersistentStateDir becomes a required non-empty string. No hard blockers, but there's an unaddressed upgrade/migration gap for the data layer and a stale doc comment.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Upgrade/migration gap (raised by Codex as P1): buildDataState opens a fresh /blockdb and data.NewState now restores QCs/blocks only from BlockDB. A node upgraded from a build that persisted the old data-layer WALs (fullcommitqcs/globalblocks) will not import that data, so its previously-finalized blocks/QCs are ignored after the upgrade. Practical impact is limited — the data layer holds committed/finalized data that a node can re-obtain via block-sync from peers, and the anti-slashing consensus state (persistedInner) is unaffected — but it's worth confirming that no operator node has already persisted data-layer WALs (the removed code was labeled 'temporary'), and ideally noting the one-time re-sync in the PR/upgrade notes.
- Both routers now call r.data.Close() at the end of Run(), and two tests also register t.Cleanup(router.data.Close()). Confirm littblock BlockDB.Close() (and data.State.Close()) is idempotent so a future test that both runs the router and relies on cleanup can't double-close; today no test does both, so this is only defensive.
- Tooling: REVIEW_GUIDELINES.md is empty/missing and cursor-review.md produced no output, so this synthesis reflects only Claude's and Codex's findings; the Codex pass noted its tests could not run (Go 1.25.6 toolchain download blocked, local Go 1.24.13).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // loadPersistedState creates persisters for the given directory and loads | ||
| // any existing state from disk. When a prune anchor is present, stale commitQCs | ||
| // and blocks below the anchor are filtered out before returning. | ||
| // Returns None when the directory contains no persisted state (fresh start). |
There was a problem hiding this comment.
[nit] Stale doc: this function no longer returns None on a fresh start. Previously the None result came from the removed if _, ok := dir.Get(); !ok (no-op) branch; now, on an empty directory it falls through and always returns utils.Some(loaded) with empty commitQCs/blocks (line 141). newInner treats Some(empty) and None equivalently, so behavior is correct — but the comment should be updated (e.g. drop this line or say it returns Some with empty state on fresh start).
There was a problem hiding this comment.
This PR cleanly replaces the WAL-based consensus/data persistence layer with a LittDB-backed BlockDB, promotes PersistentStateDir from Option[string] to a required string, and fixes a BlockDB handle leak on the validator constructor error path. The recovery, persist, and pruning logic is well-reasoned and correct; I found no blocking issues, only informational notes (including that tests could not be executed in this environment and the Cursor pass produced no output).
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behavioral change worth calling out in release notes: persistence can no longer be disabled. An empty PersistentStateDir now hard-fails at startup (buildDataState / gen_autobahn_config) instead of silently running in-memory. This is safer (removes the slashing-prone no-persistence mode), and the flag still defaults to "data/autobahn", but any operator whose config sets persistent_state_dir to empty/null (previously → in-memory) will now fail to boot rather than degrade silently.
- Implicit directory-creation ordering: the consensus (inner A/B files) and avail persisters call persist.NewPersister(cfg.PersistentStateDir), which os.Stat()s the root and errors if it's absent. This only works because buildDataState constructs the BlockDB at /blockdb first (its MkdirAll creates the root as a side effect) before consensus.NewState runs. This dependency is correct today but undocumented and fragile; an explicit os.MkdirAll(PersistentStateDir) in buildDataState would make it robust to future reordering.
- Both giga routers close r.data (the BlockDB) via errors.Join(err, r.data.Close()) only after Run() returns. A router that is successfully constructed but never Run() would leak the BlockDB handle. This matches the typical lifecycle and is minor, but a defensive Close on any post-buildDataState construction failure (as already done for the validator's consensus.NewState path) would fully close the gap.
- Verification gap: the Codex pass reported it could not run
go testbecause the Go 1.25.6 toolchain was unavailable in its sandbox, and I hit the same constraint here (build/gofmt/test blocked in this environment). This is a consensus-critical crash-recovery change — ensure CI'sgo test -raceon Go 1.25.6 (go-test.yml) and golangci-lint (golangci.yml) pass green before merge. - The Cursor second-opinion review file was empty (no output produced); only the Codex pass returned content, and it found no material issues.
Open BlockDB in setup before the transport starts, and close it after giga.Run returns via SpawnCritical so Close does not race BaseService's OnStop-before-wg.Wait ordering. Routers no longer open or close BlockDB. Co-authored-by: Cursor <cursoragent@cursor.com>
t.Fatalf/require are unsafe from non-main test goroutines; panic matches the helpers that were already written that way. Co-authored-by: Cursor <cursoragent@cursor.com>
Detect the first retained block via len(blocks)==0 instead, matching the QC-side first==nextQC style without a one-shot bool. nextBlock cannot be used here because insertBlock does not advance it until after the loop. Co-authored-by: Cursor <cursoragent@cursor.com>
Add TODOs for pushing view checks into BlockDB and removing snapshotted persist cursors, drop inaccurate prune "disk I/O" comments, split recovery tests into their own file, and run temporary state.Run under scope.Run. Co-authored-by: Cursor <cursoragent@cursor.com>
PruneBefore only advances an async in-memory watermark (no disk I/O), so there is no reason to release the lock first. Annotate that explicitly. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose WriteHighWaterMarks on BlockDB and drop State's one-shot persist cursor snapshots so runPersist reads the durable tip once under lock. Co-authored-by: Cursor <cursoragent@cursor.com>
…iled Start pruneFirst no longer assumes blocks[first] is still in memory after runPersist eviction. OnStart closes BlockDB if Start fails before giga is spawned, and regression tests cover eviction+prune and PushQC-before-Run persistence. Co-authored-by: Cursor <cursoragent@cursor.com>
Seed the eviction cursor from inner.first and wake runPersist when PushAppHash advances nextAppProposal over already-durable heights, so restart no longer keeps the full retain window pinned in memory. Co-authored-by: Cursor <cursoragent@cursor.com>
Gate GlobalBlockByHash on inner.first so hash lookups match number-based ErrPruned, and propagate ErrPruned from TryBlock's BlockDB fallback so runBlockFetcher does not retry forever on reclaimed heights. Co-authored-by: Cursor <cursoragent@cursor.com>
Spell out Retention/GCPeriod/Fsync defaults from littblock.DefaultConfig, warn that fsync=false is unsafe in production, and clarify the block_db field overlays AutobahnBlockDBConfig onto those defaults. Co-authored-by: Cursor <cursoragent@cursor.com>
Point absent fields at littblock.DefaultConfig instead of copying Retention/ GCPeriod/Fsync values that may drift when LittDB changes. Co-authored-by: Cursor <cursoragent@cursor.com>
JSON Some("") no longer rootifies to the node home (in-memory mode), and
makeCloser returns nil when every closer succeeds so construct-failure
errors stay unwrap-able.
Co-authored-by: Cursor <cursoragent@cursor.com>
Autobahn must not ship with fsync disabled. Crash can lose acknowledged writes. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove TestEvictedReadsFallBackToBlockDB and TestRestartEvictsRecoveredWindow; coverage is not worth the sleep-poll patterns. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the outer Option wrapper; empty overrides are the zero value and omit from JSON. Co-authored-by: Cursor <cursoragent@cursor.com>
Centralize defaults/overlays/fsync in AutobahnBlockDBConfig.LittBlockConfig, applied after rootifying persistent_state_dir in preparePersistentStateDir. Co-authored-by: Cursor <cursoragent@cursor.com>
Reject first-block/QC mismatches instead of normalizing straddling prefixes. Also rename blockEntry.blk, tighten insertBlock bounds, and simplify avail test helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
Fall through to BlockDB naturally when the hash is absent or entries are evicted. Co-authored-by: Cursor <cursoragent@cursor.com>
…tFound Stop tracking a separate in-memory prune watermark in data.State — memory is cleared by eviction after execution, and PruneBefore only advances BlockDB. BlockDB Read APIs now return typed errors so callers can distinguish pruned from missing without remapping Option/None. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename BlockDB status accessors, open BlockDB in node setup, and start giga before transport so persistence is active before inbound data traffic. Co-authored-by: Cursor <cursoragent@cursor.com>
The n < nextBlock guard already excludes every height eviction can remove, so qcs[n] is always present in the admitted range. Co-authored-by: Cursor <cursoragent@cursor.com>
PruneBefore now clears cached blocks, QCs, and app proposals below the retained bound so memory cannot serve data that BlockDB would reject as pruned. Co-authored-by: Cursor <cursoragent@cursor.com>
Use ErrBlockGap with errors.Is in recovery tests, and drop the pre-giga OnStart close defer now that giga always owns BlockDB lifecycle. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore hash-path mapping of pruned/missing QC to None, and update partial-QC prune tests for cohort-atomic watermark clamping. Co-authored-by: Cursor <cursoragent@cursor.com>
aba862a to
674d934
Compare
There was a problem hiding this comment.
Solid, well-tested migration from the two-file DataWAL to a LittDB-backed BlockDB, with correct eviction-sentinel handling and BlockDB ownership/close paths. The one blocking concern (also raised by Codex) is that giga.Run was moved to the very top of OnStart, starting autobahn InitChain/consensus/production before the genesis-time wait and EventBus/indexer init.
Findings: 1 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review (./cursor-review.md) was empty — that pass produced no output. Codex contributed exactly one finding (the OnStart reordering), which this review incorporates.
- PR description acknowledges that on restart, blocks pruned before the previous shutdown may briefly reappear until BlockDB GC catches up. This is exercised by TestRecoveryAfterPruneNoGC, but it means RPC/ops read paths (BlockByNumber/BlockByHash) can transiently serve heights that were logically pruned — worth a note in operator-facing docs since the old WAL path did not resurface pruned data.
- The persist package (globalblocks.go, fullcommitqcs.go, testonly.go and their tests) is fully removed and the reconcile test-suite replaced with BlockDB recovery tests; coverage looks comprehensive (empty, normal, after-prune, blocks-behind, partial-QC-prefix, no-GC, QCs-without-blocks, block-gap). No gap noticed, but confirm CI runs the new sei-db/ledger_db/block Status test and data/state_recovery_test under -race.
| // can advance inner cursors via PushQC, and so BaseService cancels | ||
| // SpawnCritical (closing BlockDB via the wrapper) if later OnStart steps fail. | ||
| if giga, ok := n.giga.Get(); ok { | ||
| n.SpawnCritical("giga", func(ctx context.Context) error { |
There was a problem hiding this comment.
[blocker] Moving giga.Run to the very top of OnStart starts more than persistence. giga.Run spawns runExecute (which calls App.InitChain on a fresh start and drives block execution) plus the validator consensus/producer loops, and giga does its own outbound dialing independent of n.router. This now runs before the genesis-time wait (~120 lines below) and before EventBus/indexerService are started, so a fresh node with a future-dated genesis will call InitChain and begin autobahn consensus/production ahead of genesis time, and early execution can outrun indexer init.
The race this reordering fixes is only that inbound giga connections (dispatched via n.router) could reach PushQC before runPersist is active — and inbound connections can't arrive until n.router.Start(ctx). So the spawn only needs to move ahead of n.router.Start(ctx), not ahead of the genesis wait / EventBus / indexer. Suggest placing this SpawnCritical("giga", ...) block immediately before n.router.Start(ctx) (after the genesis-time sleep), which preserves the runPersist-before-inbound ordering while respecting genesis timing and indexer initialization.
Keep runPersist ahead of inbound PushQC without starting Autobahn before EventBus, indexer, or a future-dated genesis time. Co-authored-by: Cursor <cursoragent@cursor.com>
| break | ||
| } | ||
| n := it.Number() | ||
| qc, err := db2.ReadQCByBlockNumber(n) | ||
| require.NoError(t, err) | ||
| require.True(t, qc.IsPresent(), "block %d survived but its covering QC was lost", n) | ||
| _, err = db2.ReadQCByBlockNumber(n) | ||
| require.NoError(t, err, "block %d survived but its covering QC was lost", n) | ||
| present++ | ||
| } |
There was a problem hiding this comment.
🟡 TestLittblockNoBlockWithoutQCAfterTornTail and TestLittblockFlushSurvivesHardKill dropped their require.True(t, qc.IsPresent(), ...) assertion, leaving only require.NoError on the result of ReadQCByBlockNumber. Per the BlockDB contract, a block with no covering QC returns (None, nil) — a nil error — so both tests now pass even if a block survives crash recovery without its covering QC, which is exactly the invariant they exist to verify.
Extended reasoning...
What changed. In sei-db/ledger_db/block/littblock_crash_test.go, both TestLittblockNoBlockWithoutQCAfterTornTail (torn-tail recovery) and TestLittblockFlushSurvivesHardKill (hard-kill recovery) previously did:
qc, err := db2.ReadQCByBlockNumber(n)
require.NoError(t, err)
require.True(t, qc.IsPresent(), "block %d survived but its covering QC was lost", n)and now do:
_, err = db2.ReadQCByBlockNumber(n)
require.NoError(t, err, "block %d survived but its covering QC was lost", n)Why this is vacuous. Per the BlockDB.ReadQCByBlockNumber contract (sei-tendermint/autobahn/types/block_db.go:262-277), when a block number is at or above the retention watermark but has no covering QC, the method returns utils.None with a nil error — err is only non-nil for genuine failures (ErrPruned, I/O/decode errors). The littblock implementation (litt_block_db.go around line 410-416) matches this contract exactly: when s.table.Get(qcKey(n)) reports !exists, it returns (utils.None, nil).
In both of these crash-recovery tests, the watermark stays at genesis (no pruning occurs in either torn-tail or hard-kill scenarios), so every block number iterated is at or above the watermark. That means a block which survives crash recovery without its covering QC — exactly the failure mode these tests are named to catch — now produces (None, nil) from ReadQCByBlockNumber, and require.NoError(t, err, ...) passes without complaint. The dropped require.True(t, qc.IsPresent(), ...) was the only assertion that actually checked for QC presence.
Why nothing else catches this. These are the only two assertions in the codebase directly verifying the 'every persisted block is still covered by a persisted QC' invariant after a simulated crash (torn WAL tail / hard kill). No other test path exercises this specific recovery scenario, so weakening these two removes the only regression guard for this invariant.
Step-by-step proof:
- Suppose littblock's recovery logic regresses and, after a torn-tail or hard-kill event, block
nsurvives on disk but its covering QC does not. db2.ReadQCByBlockNumber(n)is called. Sincenis at or above the (genesis) watermark and no QC covers it, the method returns(utils.None, nil)per contract.- The test executes
_, err = db2.ReadQCByBlockNumber(n)thenrequire.NoError(t, err, ...). Sinceerris nil, this assertion passes. - The loop increments
present++and continues; the test function returns without failing. - The regression is silently swallowed — the test suite reports green despite a real violation of the crash-safety invariant the test's own doc comment claims to guarantee ('every persisted block is still covered by a persisted QC ... never the reverse').
Impact and fix. This is test-only: no production/runtime/consensus code path is affected, and the underlying littblock recovery logic itself is unchanged by this PR (all three verifiers independently confirmed the current implementation still satisfies the invariant in practice). The regression is purely in the tests' ability to detect a future violation. The fix is to restore the dropped assertion, e.g.:
qc, err := db2.ReadQCByBlockNumber(n)
require.NoError(t, err)
require.True(t, qc.IsPresent(), "block %d survived but its covering QC was lost", n)at both call sites (lines ~85 and ~232 pre-diff).
There was a problem hiding this comment.
Large, well-tested refactor swapping Autobahn's DataWAL for a LittDB-backed BlockDB, with careful recovery, RAM-eviction, and BlockDB lifecycle handling. No correctness blockers found; a few non-blocking observations around documented design tradeoffs.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (cursor-review.md) was empty — that pass produced no output. Codex reported no material issues but noted its targeted tests could not run because the Go 1.25.6 toolchain download was network-blocked, so its 'no issues' verdict is based on static reading only.
- Recovery (
loadFromBlockDB) hard-couples to littblock's cohort-atomic GC guarantee: it requires the first block-iterator entry to equal the first retained QC'sGlobalRange().Firstand returns an error (NewState fails, node won't start) on any partial QC prefix. This is intentional and well-covered by tests (TestRecoveryPartialQCPrefix, TestRecoveryAfterPruneNoGC), but it means a future change to littblock's block-vs-QC pruning independence would turn into a startup failure rather than graceful normalization. Worth keeping the invariant documented at the littblock layer too. - node.go OnStart: when giga is already spawned and a later startup step fails, BlockDB is deliberately NOT closed by the defer — it relies on the SpawnCritical giga wrapper closing it after the service context is cancelled once OnStart returns. The reasoning is documented, but this correctness depends on BaseService cancelling the critical-goroutine context on OnStart failure; worth confirming that framework behavior so BlockDB can't leak on a partial-start error.
- gen_autobahn_config.go defaults
--blockdb-retentionto30s. It's documented as a local/docker helper (not production bring-up), but generating a config for a longer-lived node with this tool would silently bake in a 30s retention TTL; consider making the local-only intent even louder or requiring an explicit opt-in for such a short default. - PushBlock now drops the second-lock
n < inner.firstre-check (relying on insertBlock'sn < nextBlockguard). This is correct given eviction stays below nextBlock, but it's a subtle invariant worth a brief inline note at the insertBlock guard to guard against future regressions.
Superseded: latest AI review found no blocking issues.
3 participants
Summary
DataWAL(two WAL files) with a LittDB-backedBlockDBfor block and QC persistenceNewState(cfg, blockDB)replays persisted state at construction time;Nonedisables persistenceGigaRouterowns theBlockDBlifecycle: open in constructor, defer close inRunBehavioral note: On restart, blocks pruned before the previous shutdown may briefly reappear until BlockDB GC catches up.