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 31 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
Config / ops: autobahn JSON can set optional Reviewed by Cursor Bugbot for commit 780b5a3. 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 59.91% 59.07% -0.85%
==========================================
Files 2288 2201 -87
Lines 190180 179946 -10234
==========================================
- Hits 113951 106301 -7650
+ Misses 66063 64320 -1743
+ Partials 10166 9325 -841
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.
|
|
||
| // evictExecuted drops cached blocks/QCs in [evictedQC, nextAppProposal-1). | ||
| // Keeps nextAppProposal-1 for nextToExecute. Caller must hold inner's lock. | ||
| func evictExecuted(inner *inner, evictedQC types.GlobalBlockNumber) types.GlobalBlockNumber { |
There was a problem hiding this comment.
seems redundant with inner.pruneFirst
There was a problem hiding this comment.
pruneFirst removed
| return evictedQC | ||
| } | ||
|
|
||
| func (s *State) runPruning(ctx context.Context, after time.Duration) error { |
There was a problem hiding this comment.
this is yet another pruning task, which we don't need any more.
| // hashvault, and the validator's consensus persister in sibling subdirs). | ||
| // If None, persistence is disabled and the node runs fully in-memory. | ||
| PersistentStateDir utils.Option[string] | ||
| // BlockDB optionally overrides littblock defaults when PersistentStateDir |
There was a problem hiding this comment.
let's get the littDB cofig resolved outside of giga_router.
| func (n *nodeImpl) OnStart(ctx context.Context) error { | ||
| func (n *nodeImpl) OnStart(ctx context.Context) (err error) { | ||
| // If Start fails before giga is spawned, BaseService does not call OnStop | ||
| // and never cancels SpawnCritical — so BlockDB would otherwise leak. |
There was a problem hiding this comment.
how about we just move the giga spawning to be the first call in OnStart?
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>
There was a problem hiding this comment.
A large but carefully engineered refactor replacing the two-file DataWAL with a LittDB-backed BlockDB for Autobahn block/QC persistence, with sound lifecycle management, in-memory eviction + DB read-fallback, and comprehensive rewritten tests. No correctness blockers found; only minor non-blocking notes.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion inputs were empty:
codex-review.md,cursor-review.md, andREVIEW_GUIDELINES.mdall contained no content, so neither the Codex nor Cursor pass produced findings and no repo-specific guidelines could be applied. - Documented behavior (PR description + PruneBefore comment): after restart, blocks pruned before shutdown may briefly reappear until BlockDB GC advances. This is reasoned to be safe (resurrected data is QC-covered committed data, and the littblock read watermark plus the
bn.Number < inner.firstguard inglobalBlockByHashFromDBprevent serving stranded records). Acknowledged and self-healing on the next prune cycle — flagging only for visibility, not as a defect. - Nice incidental fix:
makeCloserpreviously usedif len(errs) >= 0(always true, returning a non-nil error with an empty message even on success); now> 0and covered byTestMakeCloser_NoErrorsReturnsNil. - Large surface area: the PR removes the entire
consensus/persistglobal-block/full-commitqc WAL machinery and its tests. Worth a focused CI run of the sei-tendermint autobahn packages (data, p2p/giga, node) with-raceto confirm the new eviction/persist goroutine interactions before merge.
Fall through to BlockDB naturally when the hash is absent or entries are evicted. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
This PR cleanly replaces the two-file DataWAL with a LittDB-backed BlockDB for Autobahn block/QC persistence, with strong test coverage and careful lifecycle ownership (nodeImpl owns the BlockDB, closed once after giga.Run via sync.Once). The main concern is the on-disk format change: there is no migration from, or cleanup of, the previous globalblocks//fullcommitqcs/ WALs, which can silently discard a running node's persisted consensus state on upgrade.
Findings: 1 blocking | 4 non-blocking | 0 posted inline
Blockers
- Persistence format change has no migration path (merges Codex P1). Persistence moves from
<persistent_state_dir>/{globalblocks,fullcommitqcs}(DataWAL) to<persistent_state_dir>/blockdb(littblock) in giga_router_common.go BuildDataState / setup.go preparePersistentStateDir, but nothing reads or migrates the old WALs and nothing deletes them. An autobahn node that previously ran with a persistent_state_dir starts from an empty BlockDB, so NewState replays nothing and inner.first resets to genesis. Because data.State only accepts contiguous QCs and the network gossips current-height QCs (not from genesis), the node cannot fill the gap between genesis and the current app height without statesync, so runExecute can block indefinitely in PushAppHash waiting for a height that never arrives — a coordinated validator upgrade could stall. Please add a migration (or an explicit, documented format reset that also removes the stale dirs) before merge. If the DataWAL format was never used with persistence on a live network, say so explicitly in the PR so the risk is on record; the impact hinges on that deployment fact.
Non-blocking
- Even if migration is deemed unnecessary, the old
globalblocks/andfullcommitqcs/directories are left orphaned under persistent_state_dir after upgrade (wasted disk). Consider removing them during startup. - The
makeCloserchange in setup.go fromlen(errs) >= 0tolen(errs) > 0is a genuine bug fix — the old predicate was always true, so every closer chain returned a spurious non-nil error even on clean shutdown. Good catch; the added TestMakeCloser_NoErrorsReturnsNil covers it. Flagging only to confirm no callers depended on the previous always-error behavior. - gen_autobahn_config's
--blockdb-retentiondefaults to 30s (documented as local/docker-only). Worth double-checking operators can't accidentally use configs generated by this helper for real clusters, where 30s retention would GC recent blocks aggressively. - Second-opinion inputs: cursor-review.md was empty (no Cursor findings to merge) and REVIEW_GUIDELINES.md was empty/absent (no repo-specific standards applied). Codex produced exactly one finding, incorporated as the blocker above.
…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>
There was a problem hiding this comment.
A well-structured, thoroughly-tested migration from the two-WAL DataWAL to a LittDB-backed BlockDB with careful lifecycle ownership in nodeImpl. The one blocking concern is the absence of a DataWAL→BlockDB upgrade path: an already-advanced node that upgrades starts with an empty blockdb/ and blocks indefinitely in PushAppHash.
Findings: 1 blocking | 4 non-blocking | 0 posted inline
Blockers
- Upgrade/migration gap (matches Codex P1). BuildDataState opens BlockDB at <persistent_state_dir>/blockdb and never imports the previous DataWAL (globalblocks/ + fullcommitqcs/). A node that already has DataWAL state and a committed app height
lastwill, on restart, recover an empty BlockDB (loadFromBlockDB leaves nextAppProposal/nextBlockToPersist at registry.FirstBlock()), then runExecute calls data.PushAppHash(ctx, last, …), which WaitUntil(n < nextBlockToPersist) — for last ≫ FirstBlock this blocks until every historical block/QC is re-fetched from peers, which can hang forever once peers have pruned that history. The old DataWAL dirs are also left orphaned on disk. Please confirm whether persistent Autobahn deployments exist; if so, add a migration (import DataWAL into BlockDB, or seed the recovery cursors from the app's committed height) or an explicit, documented reset/compatibility strategy. If no such deployments exist yet, please state that assumption in the PR so the reset is a deliberate decision.
Non-blocking
- memblock.PruneBefore now intentionally leaves byHash entries in place (deletes only byNumber) so ReadBlockByHash can classify below-watermark hits as ErrPruned. Since nothing ever removes these entries, byHash grows unbounded for the lifetime of the process. memblock is only used when PersistentStateDir is unset (already an “UNSAFE” degraded mode), so impact is limited, but a long-running in-memory node will leak memory proportional to total blocks seen. Consider capping/GC-ing byHash or documenting the leak.
- Second-opinion tooling: cursor-review.md was empty (no Cursor output); REVIEW_GUIDELINES.md was also empty. Codex contributed exactly one finding (the P1 above), which is incorporated as the blocker.
- Nice latent-bug fix in node/setup.go makeCloser: the previous
if len(errs) >= 0was always true, so makeCloser returned a non-nil empty error on the success path; the new> 0plus TestMakeCloser_NoErrorsReturnsNil is a good catch. - The data.ErrNotFound/ErrPruned sentinels are now aliases of types.ErrNotFound/ErrPruned, changing the error strings (e.g. “not found” → “block: not found”). errors.Is callers are unaffected, but confirm no code/log path string-matches the old messages.
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>
| // evictExecuted drops cached blocks/QCs/AppProposals in [evictedQC, nextAppProposal-1). | ||
| // Keeps nextAppProposal-1 for nextToExecute. Caller must hold inner's lock. | ||
| func evictExecuted(inner *inner, evictedQC types.GlobalBlockNumber) types.GlobalBlockNumber { | ||
| evictBelow := evictedQC | ||
| if inner.nextAppProposal > evictedQC { | ||
| evictBelow = inner.nextAppProposal - 1 | ||
| } | ||
| for ; evictedQC < evictBelow; evictedQC++ { | ||
| if b, ok := inner.blocks[evictedQC]; ok { | ||
| delete(inner.blockHashes, b.Header().Hash()) | ||
| delete(inner.blocks, evictedQC) | ||
| } | ||
| delete(inner.qcs, evictedQC) | ||
| delete(inner.appProposals, evictedQC) | ||
| } | ||
| return evictedQC | ||
| } |
There was a problem hiding this comment.
🔴 evictExecuted (state.go:791-807) keeps only the single most-recently-executed AppProposal in memory, replacing the previous PruneAfter grace-period mechanism, and AppProposals have no BlockDB fallback. A lagging/reconnecting AppVotesRecv session (used by peers to collect this validator's AppVotes toward AppQC quorum) permanently skips any evicted height instead of retrying, which can leave some heights without enough votes to ever reach quorum. Recommend restoring a grace/backlog window (or gating eviction on confirmed vote consumption) sized to realistic AppQC quorum-collection latency.
Extended reasoning...
The mechanism. evictExecuted deletes inner.appProposals[n] (and the backing blocks/qcs entries) for every n in [evictedQC, nextAppProposal-1), i.e. it keeps only the single most-recently-executed height's AppProposal. It runs from runPersist on essentially every loop iteration (persistOrEvictReady triggers as soon as nextAppProposal > evictedQC+1). AppProposals are never written to BlockDB — only WriteBlock/WriteQC persist — so State.AppProposal has no fallback once an entry is evicted: it returns ErrPruned permanently for that height.
How this surfaces. avail/subscriptions.go's AppVotesRecv.Recv is the read path serverStreamAppVotes (p2p/giga/avail.go) uses to stream this validator's AppVotes to peers, which is how those votes make it into another node's AppQC quorum tally (avail/app_votes.go's pushVote, bucketed by q.q[GlobalNumber].pushVote). Recv's error handling treats ErrPruned as "this height is gone, move on" (r.next += 1; continue) rather than retrying — it cannot tell a genuinely-retention-pruned height apart from one merely evicted from a size-1 cache. This replaces the old Config.PruneAfter + runPruning mechanism, which the PR removes entirely; that mechanism gave appProposals a deliberate, configurable time window to be read before disappearing.
Why this can matter even without a lagging peer. Block execution and AppQC formation are decoupled by design — runPropose embeds LastAppQC() opportunistically without blocking on it, so nextAppProposal can run ahead of the last formed AppQC by more than one height. Any AppVotesRecv session — including this validator's own outbound stream to an otherwise-healthy peer — only needs to be delayed by roughly one execution step (a scheduling hiccup, backpressure on the RPC stream, a burst of back-to-back block execution) to have the height it wants evicted before it reads it. Because that height's AppQC requires quorum-weight votes bucketed at that exact GlobalNumber (pushVote keys on the vote's hash/height, not "any height at or above"), losing enough validators' votes for one specific height can leave it permanently short of quorum.
Why this is less severe than a hard consensus stall (addressing the refutation). The refutation is right that the system doesn't require every height to get its own AppQC: PushAppVote's inner.prune(committee, appQC, commitQC) discards lower, still-un-quorate rounds once any higher RoadIndex reaches quorum, and every AppVotesRecv session resets to LastAppQC.GlobalNumber()+1 the moment a new AppQC lands — so validators that skipped a height are automatically resynced to try again at the next one. This "leapfrog and abandon" design is genuine and pre-existing, and it means an occasional skipped height is tolerated by design, not a correctness violation. Where the refutation overreaches is concluding no validator ever loses anything real: quorum for a given height still requires enough validators to vote for that exact height before it's evicted, and under correlated lag (a load spike or partition-heal affecting many validators near-simultaneously) the same narrowed window that causes one height to be skipped applies to the "reset" heights too, so the self-healing convergence can itself be repeatedly defeated rather than failing over cleanly after one skip.
Net effect and fix. Practically this reads as reduced AppQC-formation throughput/robustness under load or after reconnects/partitions, not a guaranteed permanent stall — but it's a real, unforced regression from a previously deliberate, configurable grace period down to an unconditional single-height cache with no fallback, in a consensus-critical path. Restoring some backlog tolerance (e.g. don't evict above the highest height any actively-lagging-but-connected AppVotesRecv session still needs, or reintroduce a bounded time/size grace window covering realistic quorum-collection latency) would remove the regression without reverting the intended memory-decoupling goal of this PR.
Step-by-step proof. (1) Validator executes height n, PushAppHash stores appProposals[n]. (2) Validator executes height n+1 before any AppVotesRecv session (including its own outbound stream) has read appProposals[n] — plausible under back-to-back execution or scheduling delay. (3) runPersist's next iteration calls evictExecuted, which now deletes appProposals[n] (since nextAppProposal-1 = n+1 is kept, n is not). (4) Any session with r.next == n calls AppProposal(ctx, n), gets ErrPruned, and does r.next += 1; continue — that session never emits a vote for n. (5) If quorum-weight worth of validators independently hit step 4 for the same n (e.g. simultaneous load spike), q.q[n] never reaches AppQuorum(), and n's AppQC never forms — abandoned once a later height succeeds, per the leapfrog design, but that specific height's finality vote is permanently lost with no retry path.
| if ceiling := min(s.latestQCStartBlock, s.lastBlockNumber); n > ceiling { | ||
| n = ceiling | ||
| } | ||
| for num, blk := range s.byNumber { | ||
| if n > s.pruneWatermark { | ||
| s.pruneWatermark = n | ||
| } | ||
| // Drop number-indexed blocks below the watermark. Leave byHash entries so | ||
| // ReadBlockByHash can still resolve the number and return ErrPruned | ||
| // (mirrors littblock refusing below-watermark hash hits before GC). | ||
| for num := range s.byNumber { | ||
| if num < n { | ||
| delete(s.byNumber, num) | ||
| delete(s.byHash, blk.Header().Hash()) | ||
| } | ||
| } | ||
| for lower, e := range s.qcsByLower { |
There was a problem hiding this comment.
🔴 memblock.PruneBefore only removes stale entries from byNumber/qcsByLower; it never deletes from byHash, so every block ever written stays pinned in memory (full *types.Block objects) for the life of the process. Since this PR wires memblock into the production BuildDataState path whenever PersistentStateDir is unset, any long-running no-persistence node or loadtest now leaks memory unboundedly instead of being bounded to the retention window. Fix: delete the corresponding byHash entry inside the PruneBefore loop, restoring the pre-PR lockstep-delete behavior (or track hash→number without pinning the full block).
Extended reasoning...
mem_block_db.go:WriteBlock unconditionally inserts a hashEntry{blk: blk, n: n} into s.byHash on every call (s.byHash[blk.Header().Hash()] = hashEntry{blk: blk, n: n}), and hashEntry.blk is a full *types.Block pointer, not a lightweight index. PruneBefore's eviction loop only ranges over s.byNumber (deleting stale number-indexed entries) and s.qcsByLower; it does not touch s.byHash at all. The removed pre-PR code used to run delete(s.byNumber, num); delete(s.byHash, blk.Header().Hash()) together in the same loop, so byHash shrank in lockstep with byNumber. This PR deletes that second statement and adds a comment claiming the retention mirrors littblock's watermark-gated hash keys ("Leave byHash entries so ReadBlockByHash can still resolve the number and return ErrPruned").
That justification does not hold for memblock specifically, because littblock's below-watermark hash keys are eventually physically reclaimed by its background GC — the watermark gate is a temporary refusal layer on top of eventual reclamation, not a permanent retention. memblock has no GC of any kind (Close is a no-op, there is no background goroutine, no TTL, nothing). So for memblock, "leave byHash entries so ReadBlockByHash returns ErrPruned" in practice means "never free byHash entries, ever." Since WriteBlock inserts unconditionally on every committed block and nothing removes an entry afterward, len(s.byHash) grows monotonically with the total number of blocks the process has ever written, not with the retained/prune window. Each entry pins a full block (header + payload/txs), so this is a genuine unbounded memory leak, not merely a "slightly stale index."
This regression is newly reachable in production by this same PR: before this change, memblock.NewBlockDB() was only used from test/benchmark fixtures. This PR wires it into BuildDataState (giga_router_common.go) as the BlockDB implementation whenever PersistentStateDir is unset — confirmed directly in review by wen-coding: "We dont use None as BlockDB any more, using memory block db if we skip disk." So any long-running in-memory/no-persistence deployment (loadtest harness, or a misconfigured/intentionally ephemeral node) now leaks one full block per commit, indefinitely, until the process OOMs.
The leak is also functionally pointless for the one real consumer of ReadBlockByHash: data.State.globalBlockByHashFromDB treats ErrPruned and ErrNotFound identically (both map to None), so preserving the ability to distinguish "pruned" from "never written" via a retained byHash entry buys nothing for that caller — the memory is being spent for a distinction nothing observes.
Step-by-step proof of the leak:
WriteBlock(n, blk)is called for block n →s.byHash[blk.Header().Hash()] = hashEntry{blk: blk, n: n}is set.len(s.byHash)== 1.- Many more blocks are written, e.g. up to block N →
len(s.byHash)== N (every hash ever seen is present, each holding a full block pointer). PruneBefore(N-10)is called (e.g. after RetainHeight/prune-window logic indata.State.PruneBefore) → thefor num := range s.byNumber { if num < n { delete(s.byNumber, num) } }loop removes entries< N-10froms.byNumberonly.s.byHashis untouched —len(s.byHash)is still N.- Blocks N+1..N+1000 are written and immediately pruned again (steady-state operation of a long-running node/loadtest) →
s.byNumberstays roughly bounded (correct steady-state behavior), buts.byHashkeeps growing to N+1000, N+2000, etc., without bound, because nothing ever callsdelete(s.byHash, ...). - Given enough runtime (a multi-hour or multi-day loadtest, or any long-lived no-persistence deployment),
s.byHashretains every block ever committed, and the process eventually OOMs.
Fix: In the PruneBefore loop, resolve the block's hash before deleting it from byNumber and also delete it from byHash, exactly like the pre-PR code did — e.g. iterate s.byNumber, and for each num < n, do delete(s.byHash, s.byNumber[num].Header().Hash()) before delete(s.byNumber, num). If preserving the below-watermark ErrPruned classification via ReadBlockByHash is still desired, keep a separate lightweight hash -> number map (without the *types.Block pointer) instead of the full hashEntry.blk, so the classification survives without pinning block payloads.
There was a problem hiding this comment.
Well-structured, heavily-tested refactor replacing the two-file DataWAL with a LittDB-backed BlockDB; the Option→error API migration, recovery/eviction logic, and BlockDB lifecycle across failure paths all check out. No blocking issues found; one minor memblock memory-leak concern and a note that the Cursor second-opinion review produced no output.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex reported "No material issues found" (its focused tests could not run because the sandbox Go module cache was read-only), so it did not fully verify either.
- memblock ReadBlockByHash: PruneBefore now deletes from byNumber/qcsByLower but intentionally leaves byHash entries in place forever, so byHash grows unbounded in a long-running in-memory node (PersistentStateDir=None). The distinction this preserves (ErrPruned vs ErrNotFound on hash lookup) is not observable to the only caller — data.State.globalBlockByHashFromDB maps both ErrPruned and ErrNotFound to None — so the retained entries buy no behavior while leaking memory. Consider deleting the byHash entry on prune (as the old code did) or, if the ErrPruned distinction must be kept, capping/aging the index. Low impact since in-memory mode is flagged as unsafe/non-production.
- Minor: globalBlockByHashFromDB and globalBlockFromDB perform two separate BlockDB reads (block then QC) outside any lock, so a concurrent prune/GC between them could surface ErrPruned on the QC after the block read succeeded. littblock's below-watermark gate makes this effectively unreachable (a served block always has a readable covering QC), so it's not a correctness bug today, but worth a comment noting the reliance.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if n > s.pruneWatermark { | ||
| s.pruneWatermark = n | ||
| } | ||
| // Drop number-indexed blocks below the watermark. Leave byHash entries so |
There was a problem hiding this comment.
[suggestion] PruneBefore leaves byHash entries in place indefinitely (only byNumber is trimmed here, and qcsByLower below), so byHash grows without bound in a long-running in-memory node. The retained entries only serve to make ReadBlockByHash return ErrPruned instead of ErrNotFound for below-watermark hits — but the sole caller (data.State.globalBlockByHashFromDB) treats ErrPruned and ErrNotFound identically (both → None), so this buys no observable behavior while leaking memory. Suggest deleting the byHash entry on prune as before, or aging/capping the index if the distinction is truly needed.
Superseded: latest AI review found no blocking issues.
Avoid unbounded growth of the hash index; callers already treat ErrPruned and ErrNotFound the same for hash lookups. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
This PR replaces the two-WAL DataWAL with a LittDB-backed BlockDB for autobahn block/QC persistence, changes the BlockDB read API from utils.Option to explicit ErrPruned/ErrNotFound errors, and moves in-memory maps to an evict-after-execute model with fall-through reads to BlockDB. The change is large but well-documented and extensively tested; I found no blocking correctness issues, only a few edge-case/behavioral notes worth confirming.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Prune eventual-consistency (Codex P1, confirmed): PruneBefore now only advances the BlockDB watermark and caps to nextAppProposal; it no longer evicts the in-memory maps synchronously (evictExecuted does that later in runPersist). So immediately after PruneBefore returns, a height below the new watermark that is still cached in
inner.blocks/inner.qcswill still be served by Block/TryBlock/QC/GlobalBlock (they consult memory before BlockDB). The served data is still valid and the window is bounded by runPersist, but it does relax the previous 'immediately unavailable after prune' semantics. This matches the PR's documented eventual-consistency note; worth a one-line comment on PruneBefore making the relaxed contract explicit for callers. - Below-genesis read error type (Codex P2, confirmed as an edge case): removing the
n < inner.firstguards means a read for a height below a non-default genesis FirstBlock() (e.g. a chain started via statesync at height > 0) falls through to an empty BlockDB and returns types.ErrNotFound instead of ErrPruned. gigaRouterCommon.BlockByNumber only maps data.ErrPruned to coretypes.WrapErrHeightNotAvailable; an ErrNotFound is surfaced as a raw wrapped error (data.GlobalBlock(%v): ...) rather than the clean height-not-available RPC error. Normal post-prune reads still correctly return ErrPruned via the littblock/memblock watermark, so impact is limited to the pre-genesis edge; consider mapping ErrNotFound at the RPC boundary too, or documenting the intended distinction. - AppProposal eviction aggressiveness (my note, please confirm): evictExecuted now drops appProposals/blocks/qcs for executed heights down to nextAppProposal-1, far more aggressively than the old RetainHeight-based pruning. AppProposal(n) returns ErrPruned once evicted, and the only in-tree consumer (avail/subscriptions.go AppVotesRecv) reacts by advancing past that height. Under normal operation the vote goroutine should keep pace with execution/persist, but it would be good to confirm there is no scenario (restart catch-up, slow signer) where a local AppVote is skipped before its AppQC has formed, since that could affect AppQC liveness.
- Second-opinion coverage: the Cursor review file (cursor-review.md) and REVIEW_GUIDELINES.md were both empty/missing, so no Cursor pass or repo-specific guidelines were incorporated; Codex's two findings are reflected above.
- Minor positive: setup.go makeCloser's guard was corrected from
len(errs) >= 0tolen(errs) > 0, fixing a pre-existing bug where the closer always returned a non-nil (empty-message) error even on success.
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>
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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit df7ab64. Configure here.
| if n, ok := truncateTo.Get(); ok { | ||
| return s.dataWAL.TruncateBefore(n) | ||
| for inner := range s.inner.Lock() { | ||
| return s.blockDB.PruneBefore(min(retainFrom, inner.nextAppProposal)) |
There was a problem hiding this comment.
PruneBefore leaves memory serving pruned
Medium Severity
PruneBefore only advances the BlockDB retention watermark and no longer trims the in-memory maps. TryBlock, Block, and related paths still return cached blocks when inner.blocks[n] is present, without consulting BlockDB, so heights below the new retain bound can keep being served even though blockDB.ReadBlockByNumber would return ErrPruned. That diverges from the documented TryBlock behavior and from the old path that pruned RAM together with persistence.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit df7ab64. Configure here.
There was a problem hiding this comment.
This PR cleanly swaps Autobahn's two-file DataWAL for a LittDB/memblock-backed BlockDB, migrating the Read* API from Option to ErrPruned/ErrNotFound and moving BlockDB lifecycle ownership into nodeImpl; the refactor is well-tested and internally consistent, but the restart/recovery path has a plausible crash-loop edge case worth resolving before merge. Codex and Cursor second-opinion passes both produced no findings.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Behavioral change acknowledged in the PR description (pruned blocks may reappear on restart until GC catches up): in the reappear window, a height that was pruned but physically GC'd returns wrapped ErrNotFound from blockFromDB/qcFromDB rather than ErrPruned, so gigaRouterCommon.BlockByNumber (which only maps data.ErrPruned to WrapErrHeightNotAvailable) would surface a raw error to RPC/ops callers instead of the expected height-not-available. Consider mapping ErrNotFound for below-nextBlock heights the same way, or documenting it.
- data.State.PruneBefore now calls blockDB.PruneBefore while holding the inner Watch lock, whereas the old code deliberately did WAL truncation outside the lock. littblock.PruneBefore is metadata-only (just advances an atomic watermark) so this is fine today, but it's a latent constraint — a future BlockDB doing I/O in PruneBefore would hold the state lock during disk work. Worth a comment noting the requirement.
- Second-opinion coverage was thin: codex-review.md reports 'No material issues found' and cursor-review.md is empty (no output). Neither surfaced the recovery edge case below, so it has had limited independent scrutiny.
- Nice incidental fix: setup.go makeCloser had
if len(errs) >= 0(always true, so it always returned a non-nil error even on clean close); now corrected to> 0and covered by TestMakeCloser_NoErrorsReturnsNil.
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>
There was a problem hiding this comment.
Replaces the two-WAL DataWAL with a LittDB-backed BlockDB for block/QC persistence, moving Read* to a (value, error) contract with ErrPruned/ErrNotFound and giving GigaRouter/nodeImpl explicit ownership of the BlockDB lifecycle. The change is large but predominantly mechanical test churn around the new API, the core recovery/eviction logic is carefully reasoned and well covered by tests, and lifecycle/close ownership is handled correctly. No blocking issues found.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex raised one P1 (data/state.go loadFromBlockDB: recovery errors when the first retained block != first retained QC's GlobalRange.First, claiming a straddling QC can lose its leading blocks to GC). I do not believe this is a real bug: littblock stores blocks and QCs in a single shard, always writes a QC before the blocks it covers, and GC reclaims only a contiguous write-order prefix of segments. A surviving (straddling) QC therefore pins its own segment and every later segment in its range, so the lowest surviving QC's First block is always physically present after GC, and recoveryFloor == firstBlock holds. This is pinned by TestLittblockPartiallyPrunedQCRangeRetained and TestLittblockStrandedBlockNotServedAfterRestart at the littblock layer.
- Testing gap tied to the above: there is no data.State-level regression that exercises write multiple QCs+blocks -> PruneBefore straddling an older QC -> ForceGC -> restart -> NewState. TestRecoveryAfterPruneNoGC deliberately skips GC, and TestRecoveryAfterPruning simulates whole-QC removal rather than a straddle. Adding a state-level test with real GC after a straddling prune would turn the cross-component invariant recovery relies on into an explicit regression and directly close out Codex's concern.
- cursor-review.md and REVIEW_GUIDELINES.md were empty; the Cursor second-opinion pass produced no output to merge, and no repo-specific review guidelines were available.
- OnStart now spawns giga.Run via SpawnCritical before EventBus/indexer/handshake/router.Start, relying on the service-context being cancelled once OnStart returns an error so the SpawnCritical wrapper closes BlockDB. Worth double-confirming that an OnStart failure after gigaSpawned=true actually cancels that context (otherwise giga.Run keeps running and BlockDB is never closed); the sync.Once guard prevents double-close but not a leak if cancellation never fires.
- Nice catch/fix: node/setup.go makeCloser changed len(errs) >= 0 to > 0, fixing a latent bug where makeCloser always returned a non-nil (empty-message) error even on success.
Superseded: latest AI review found no blocking issues.
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>
There was a problem hiding this comment.
Beyond the inline nits, this run also checked two additional candidates raised against the new BlockDB-fallback recovery/read paths and ruled them out: whether NewState recovery incorrectly rejects a legitimate post-GC state where a QC survives intact but its own earliest covered blocks were reclaimed (the recovery floor still aligns with the QC start in that case), and whether TryBlock's widened BlockDB error contract causes runBlockFetcher to silently abandon retries on a real (non-sentinel) error (the fetcher's retry loop only continues on ErrNotFound, so it is unaffected).
Extended reasoning...
These two candidates concern the same recovery/read-fallback logic in state.go touched by the inline findings, but concern different code paths (block-gap floor alignment on recovery, and runBlockFetcher's retry condition in giga/data.go) than what's flagged inline. Verifier agents traced both through the current code and found no divergence from the documented contract, so they are not being carried forward as open issues; this note is purely a record of what was already examined for this run, not a guarantee that the surrounding logic is bug-free.
| func (s *State) globalBlockByHashFromDB(hash types.BlockHeaderHash) (utils.Option[*types.GlobalBlock], error) { | ||
| bn, err := s.blockDB.ReadBlockByHash(hash) | ||
| if err != nil { | ||
| if errors.Is(err, ErrNotFound) || errors.Is(err, ErrPruned) { | ||
| return utils.None[*types.GlobalBlock](), nil | ||
| } | ||
| return utils.None[*types.GlobalBlock](), fmt.Errorf("blockDB.ReadBlockByHash: %w", err) | ||
| } | ||
| qc, err := s.qcFromDB(bn.Number) | ||
| if err != nil { | ||
| return utils.None[*types.GlobalBlock](), err | ||
| } | ||
| return utils.Some(assembleGlobalBlock(bn.Number, bn.Block, qc)), nil | ||
| } |
There was a problem hiding this comment.
🟡 globalBlockByHashFromDB (data/state.go:556-569) only translates ReadBlockByHash's ErrNotFound/ErrPruned into (None, nil); the subsequent qcFromDB(bn.Number) error path returns the wrapped error verbatim. Since a concurrent PruneBefore (called after every committed block) can advance BlockDB's watermark between the two unsynchronized reads, this can leak a raw error out of GlobalBlockByHash, which then surfaces as a spurious RPC error from gigaRouterCommon.BlockByHash instead of the documented nil-block response. Fix: apply the same errors.Is(err, ErrNotFound) || errors.Is(err, ErrPruned) translation to the qcFromDB error path.
Extended reasoning...
The bug. globalBlockByHashFromDB (sei-tendermint/internal/autobahn/data/state.go:556-569) does two sequential, independently-checked BlockDB reads:
bn, err := s.blockDB.ReadBlockByHash(hash)
if err != nil {
if errors.Is(err, ErrNotFound) || errors.Is(err, ErrPruned) {
return utils.None[*types.GlobalBlock](), nil
}
return utils.None[*types.GlobalBlock](), fmt.Errorf("blockDB.ReadBlockByHash: %w", err)
}
qc, err := s.qcFromDB(bn.Number)
if err != nil {
return utils.None[*types.GlobalBlock](), err // <-- no ErrNotFound/ErrPruned translation
}The first read's error is correctly mapped to (None, nil) for the two "block just isn't retained" sentinels, matching the function's documented contract ("returns the finalized GlobalBlock ... or None if no such block is currently retained"). The second read (qcFromDB → blockDB.ReadQCByBlockNumber) has no such mapping — it just returns the wrapped error as-is, even though qcFromDB wraps with %w so errors.Is would still work if it were checked.
Why the race is reachable, not just theoretical. GlobalBlockByHash releases s.inner's lock before falling through to globalBlockByHashFromDB:
func (s *State) GlobalBlockByHash(hash types.BlockHeaderHash) (utils.Option[*types.GlobalBlock], error) {
for inner := range s.inner.Lock() {
... // in-memory check
}
return s.globalBlockByHashFromDB(hash) // runs with no lock held
}So the two BlockDB calls inside globalBlockByHashFromDB are unsynchronized with respect to each other and with respect to State.PruneBefore, which runs from runExecute after every committed block in normal steady-state operation — this isn't a rare admin action, it's continuous. PruneBefore calls s.blockDB.PruneBefore(retain), which advances littblock's internal atomic watermark (s.watermark.Load()), checked independently by ReadBlockByHash and ReadQCByBlockNumber.
The failure window: ReadBlockByHash succeeds because bn.Number >= watermark at that instant. Before qcFromDB's ReadQCByBlockNumber runs, a concurrent PruneBefore advances the watermark past bn.Number. ReadQCByBlockNumber now returns ErrPruned (or ErrNotFound once async GC also reclaims the segment). qcFromDB wraps it and returns it up, and because the check here doesn't exist, globalBlockByHashFromDB returns it as a real, non-nil error.
Why nothing catches this today. This is a genuinely new failure mode introduced by this PR. Before this change, GlobalBlockByHash was a single atomic in-memory map lookup under s.inner.Lock() with no BlockDB fallback and (per the removed TODO) "never errors." The two-step, unsynchronized BlockDB fallback is new in this PR, and the asymmetric error handling (ReadBlockByHash's branch exists, qcFromDB's does not) looks like an oversight rather than an intentional decision, since the same translation was clearly the intended contract for this exact function.
Downstream impact. One layer up, gigaRouterCommon.BlockByHash (giga_router_common.go:141-155) has no ErrPruned/ErrNotFound translation at all (unlike BlockByNumber, which explicitly maps data.ErrPruned → WrapErrHeightNotAvailable), and its own doc comment states it "Matches CometBFT semantics for unknown hashes: returns &ResultBlock{Block: nil} with no error." On this race, it instead returns fmt.Errorf("data.GlobalBlockByHash: %w", err) — a spurious error response to an eth_getBlockByHash-style caller for what should just look like "block not currently available."
Step-by-step proof:
- Block
nis executed, persisted to BlockDB, and evicted frominner.blocks/inner.qcs/inner.blockHashesbyevictExecuted(or byPruneBefore's in-RAM cache trim) — soGlobalBlockByHashfor its hash now falls through toglobalBlockByHashFromDB. - A caller (e.g. an RPC
eth_getBlockByHashhandler) callsGlobalBlockByHash(hash). The in-memory index misses, the lock is released, andglobalBlockByHashFromDBrunsblockDB.ReadBlockByHash(hash). At this instantn >= watermark, so the read succeeds and returnsbn.Number = n. - Concurrently,
runExecutefinishes committing a later block and callsdata.State.PruneBefore(retainFrom)withretainFrom > n, which callsblockDB.PruneBefore(retain)and advances littblock's watermark pastn. globalBlockByHashFromDBnow callss.qcFromDB(n)→blockDB.ReadQCByBlockNumber(n). Since the watermark has moved pastn, this returnstypes.ErrPruned(wrapped via%winqcFromDB).- The
if err != nil { return utils.None[...](), err }branch here has noerrors.Ischeck, so the wrappedErrPrunedpropagates as a genuine, non-nil error out ofGlobalBlockByHash. gigaRouterCommon.BlockByHashreceives this non-nil error, has no translation for it, and returnsfmt.Errorf("data.GlobalBlockByHash: %w", err)to the RPC layer — violating its own documented "returns &ResultBlock{Block: nil} with no error" contract for what is, from the caller's perspective, just an unavailable/unknown block.
Fix. Apply the identical errors.Is(err, ErrNotFound) || errors.Is(err, ErrPruned) => (None, nil) translation to the qcFromDB error branch in globalBlockByHashFromDB, mirroring the ReadBlockByHash branch immediately above it.
Severity. All three independent verifiers who examined this converged on nit: the trigger is a narrow race window at the exact prune boundary, the failure is a self-healing transient RPC error (not a crash, data loss, or consensus-safety issue), and the fix is a trivial one-line addition mirroring existing code in the same function.
| // | ||
| // Persistence cursors are seeded from BlockDB.Status(), not from | ||
| // inner.nextQC/nextBlock. Those inner cursors can advance via PushQC between | ||
| // NewState and Run; the BlockDB tips stay put until this loop Writes, so we do | ||
| // not skip heights that are only in memory. | ||
| func (s *State) runPersist(ctx context.Context) error { | ||
| // Initialize from nextBlockToPersist, not nextQC/nextBlock. PushQC may | ||
| // have been called before Run() (race between p2p startup and Run), | ||
| // advancing nextQC/nextBlock beyond what's been persisted. Starting | ||
| // from nextBlockToPersist ensures we don't skip unpersisted data. | ||
| var persistedQC, persistedBlock types.GlobalBlockNumber | ||
| first := s.cfg.Registry.FirstBlock() | ||
| tips := s.blockDB.Status() | ||
| persistedQC := first | ||
| if n, ok := tips.LastQCNext.Get(); ok { | ||
| persistedQC = n | ||
| } | ||
| persistedBlock := first | ||
| if n, ok := tips.LastBlockNumber.Get(); ok { | ||
| persistedBlock = n + 1 | ||
| } | ||
| // Eviction cursor tracks the lowest height still cached in memory. Seed from | ||
| // nextAppProposal (recovery floor at Run start; AppProposals are not | ||
| // recovered): after restart the retain window is fully loaded in RAM and | ||
| // already durable, so we must walk from that floor once PushAppHash advances. | ||
| var evictedQC types.GlobalBlockNumber | ||
| for inner := range s.inner.Lock() { | ||
| persistedQC = inner.nextBlockToPersist | ||
| persistedBlock = inner.nextBlockToPersist | ||
| evictedQC = inner.nextAppProposal | ||
| } |
There was a problem hiding this comment.
🟡 runPersist seeds its eviction cursor evictedQC by reading inner.nextAppProposal live inside its own goroutine (sei-tendermint/internal/autobahn/data/state.go:705-728), intending it to equal the recovery floor set by loadFromBlockDB. Because data.State.Run (which spawns runPersist) and runExecute are launched as independent, unsynchronized goroutines, a racing runExecute restart call to PushAppHash(ctx, last, ...) can advance nextAppProposal past the floor before this read executes, permanently excluding the recovered window [floor, last) from eager eviction by runPersist for the life of the process. This is a pre-existing memory-decoupling regression only on the restart path; impact is bounded and self-healing via the independent RetainHeight-driven PruneBefore path, so it is not correctness- or crash-affecting.
Extended reasoning...
What the bug is. runPersist (state.go:721-728) seeds its eviction cursor with a single read: for inner := range s.inner.Lock() { evictedQC = inner.nextAppProposal }, with the comment "Seed from nextAppProposal (recovery floor at Run start)." This is only correct if that read executes before any concurrent call advances nextAppProposal past the recovery floor established by loadFromBlockDB during NewState. Nothing in the code enforces that ordering.
The race. data.State.Run (which spawns runPersist via scope.Run -> scope.SpawnNamed) and runExecute are launched as independent sibling goroutines from gigaRouterCommon.Run (s.SpawnNamed("data", ...) / s.SpawnNamed("execute", ...)), with no cross-ordering between them. On a restart with last > 0, runExecute's restart branch calls r.data.PushAppHash(ctx, last, info.LastBlockAppHash) before entering its main loop. PushAppHash walks for inner.nextAppProposal <= n { ...; inner.nextAppProposal += 1 }, advancing nextAppProposal from the recovery floor to last+1 in a single call. If that call completes before runPersist's three-line seed read executes, evictedQC is seeded to last instead of the recovery floor.
Why nothing else catches it. evictExecuted (called from runPersist) only ever evicts entries in [evictedQC, nextAppProposal-1), and evictedQC only increases from its seed value. Once seeded above the floor, the recovered window [floor, last) — already fully loaded into RAM by loadFromBlockDB at NewState time — is never eagerly evicted by runPersist for the remainder of the process. Nothing re-seeds or corrects the cursor afterward.
Why this is bounded and self-healing, not a correctness bug. Three independent aspects limit the blast radius: (1) peak memory is unaffected — the recovered window is already resident in RAM at NewState regardless of the race outcome; (2) State.PruneBefore, called from runExecute after every committed block and driven by the app's RetainHeight policy, is completely independent of evictedQC and will eventually evict the same RAM entries once retention advances past those heights; (3) there is no crash, incorrect read, or data-loss consequence — reads still resolve correctly via the in-memory maps (or BlockDB fallback) either way.
Addressing the trigger-plausibility objection. One reviewer argued the race is effectively unreachable because runExecute's path to PushAppHash requires opening the hashvault Pebble DB and one ABCI Info() round-trip (millisecond-scale I/O) before it can advance nextAppProposal, while runPersist's path to the seed read is only a few in-memory goroutine hops. That asymmetry is real and does make the race unlikely to lose in practice on a typical node — this matches the synthesis description's own characterization ("in practice runPersist usually wins, but nothing enforces this ordering"). But "usually wins under typical scheduling" is not the same as "cannot lose": goroutine scheduling latency, GC pauses, a slow/cold filesystem for the hashvault open, or a differently-ordered startup sequence in a future refactor could all close that gap without any test catching it, since the recovery tests exercise data.State directly and don't reproduce the runExecute/hashvault race at all. The finding is explicitly an unenforced-ordering bug, not a claim that it fires on every restart — and an unenforced invariant that happens to hold today due to incidental timing is exactly the kind of thing that silently regresses when nearby code changes.
Step-by-step proof. (1) Node restarts with info.LastBlockHeight = last > 0; NewState/loadFromBlockDB replays the full retained window [floor, last] into inner.blocks/inner.qcs, setting inner.nextAppProposal = floor. (2) gigaRouterCommon.Run spawns data.State.Run (-> scope.Run -> SpawnNamed("runPersist", ...)) and runExecute as independent goroutines. (3) runExecute's restart branch performs buildHashVault (opens Pebble DB) then app.Info(ctx), then calls PushAppHash(ctx, last, ...), which loops nextAppProposal++ up to last+1. (4) If step 3 completes before runPersist's goroutine reaches evictedQC = inner.nextAppProposal, that read observes last+1 instead of floor. (5) From then on, evictExecuted never evicts anything below last+1, so [floor, last) stays cached in inner.blocks/inner.qcs in memory until PruneBefore (driven by app RetainHeight) independently reclaims it — which, with a large or disabled retention window, may not happen for a long time.
Fix. Capture the recovery-floor value once inside NewState/loadFromBlockDB, before any goroutines are spawned, and pass it explicitly into runPersist instead of re-reading inner.nextAppProposal from within the concurrently-scheduled goroutine. This removes the race entirely rather than relying on incidental scheduling asymmetry between the two startup paths.
| // OnStart starts the Node. It implements service.Service. | ||
| func (n *nodeImpl) OnStart(ctx context.Context) error { | ||
| // Start giga first so runPersist is active before inbound giga connections | ||
| // 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 { | ||
| defer func() { _ = n.closeGigaBlockDB() }() | ||
| return giga.Run(ctx) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🟡 The lifecycle comment above makeNode (node.go:266-271) lists an 'OnStart defer if Start fails before giga is spawned' BlockDB close path that no longer exists — giga is spawned as the very first statement of OnStart, so no such window remains, and OnStart has no BlockDB-closing defer anywhere. On any later OnStart failure (EventBus/indexer/handshake/router/reactor/RPC), BaseService.Start() cancels and returns immediately without waiting for the giga SpawnCritical goroutine's deferred closeGigaBlockDB() to run, so the close is real but asynchronous/unsynchronized on that path. Suggest updating the comment to describe the async close (or adding an explicit synchronous defer in OnStart).
Extended reasoning...
This is a documentation-accuracy defect with a narrow, non-production behavioral corollary, introduced by this PR's giga-first reordering in OnStart.
What the comment claims vs. what the code does. The comment above makeNode (node.go:266-271) lists three BlockDB close paths: (1) makeNode defer on construct failure, (2) "OnStart defer if Start fails before giga is spawned", and (3) the SpawnCritical wrapper after giga.Run returns. Reading the full body of OnStart (node.go:468-604), there is no BlockDB-closing defer anywhere in it — only the wrapper defer inside the SpawnCritical("giga", ...) closure (path 3). Moreover, this PR moves that SpawnCritical("giga", ...) call to the very first statement of OnStart, so there is no longer any "before giga is spawned" window within OnStart for path (2) to describe. The bullet is dead documentation.
Why the close becomes asynchronous on later failures. service.BaseService.Start() (sei-tendermint/libs/service/service.go:109-135) calls bs.impl.OnStart(ctx) synchronously; if it returns an error, Start() calls cancel() and returns the error immediately (line 120-123) — it does not spawn the goroutine that runs OnStop() + inner.wg.Wait() (that goroutine is only launched after a successful OnStart, lines 125-133). SpawnCritical (lines 172-186) enrolls the giga task in inner.wg and runs it against inner.ctx. Because giga is now spawned as literally the first line of OnStart, every one of the many later failure returns (EventBus.Start, indexerService.Start, the handshake, router.Start, reactor.Start, RPC StartService, etc.) triggers this path: cancel() fires, inner.ctx is cancelled, giga.Run eventually observes cancellation and returns, and only then does the defer func() { _ = n.closeGigaBlockDB() }() wrapper actually close the LittDB-backed BlockDB — but Start() has already returned to its caller by that point, with nothing awaiting inner.wg.
Step-by-step proof. (1) OnStart runs n.SpawnCritical("giga", ...) first, enrolling the giga goroutine in inner.wg against inner.ctx. (2) OnStart proceeds to, say, indexerService.Start(ctx), which returns a non-nil error. (3) OnStart returns that error. (4) BaseService.Start() sees the error, calls cancel() (cancelling inner.ctx), and returns the error to its caller — synchronously, without spawning the OnStop/inner.wg.Wait() goroutine. (5) Some time later (asynchronously, on a different goroutine), the giga goroutine observes inner.ctx cancellation, giga.Run returns, and the deferred closeGigaBlockDB() runs, releasing the LittDB file lock. (6) Any code path that calls Start() and, on error, immediately retries constructing a fresh node against the same BlockDB directory in the same process can race step (5) and hit a transient file-lock/"resource busy" error.
Why this doesn't rise above nit. BlockDB is not leaked — the async close in step (5) still runs (this is exactly the pre-existing SpawnCritical wrapper path, working as designed for the cancel case). In the realistic production failure mode — a fatal Start() error causing the process to exit — the OS reclaims the file lock on exit regardless of whether the async close has completed, so there is no user-visible failure. The concrete risk (a same-process retry racing the still-closing handle) requires a caller that reconstructs the node against the same BlockDB dir within the same process after a Start() failure; no such retry-in-place pattern exists in this codebase today, so the risk is real only for hypothetical future test harnesses or supervisor loops, not for anything currently reachable. The one clearly wrong, immediately fixable artifact is the stale comment bullet, which is a one-line documentation fix (or, if preferred, an explicit synchronous defer in OnStart covering the failure path) — not something that should block merge.
Distinguish pruned by-number reads with ErrPruned while representing missing records as None, matching the shared BlockDB contract. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
I didn't find any bugs, but this PR makes substantial changes to consensus-critical persistence code — BlockDB open/close lifecycle and node startup ordering (to avoid races between giga.Run and inbound connections), plus new pruning/error semantics (types.ErrPruned) across littblock, memblock, and the autobahn data.State recovery path. Given the size (37 files) and criticality, this is worth a human look.
Extended reasoning...
Overview
This PR spans sei-db (littblock/memblock BlockDB) and sei-tendermint (autobahn consensus, giga p2p router, node lifecycle). Key changes: (1) node.go/setup.go now open BlockDB in buildGigaRouter and thread ownership through createRouter into nodeImpl, with a sync.Once close path invoked from three distinct points (construct-failure defer, OnStart failure defer, SpawnCritical wrapper after giga.Run returns) to avoid closing while persist/execute goroutines are still running; (2) giga.Run is now spawned before other OnStart steps so runPersist is active before inbound connections can PushQC; (3) littblock/memblock ReadBlockByNumber/ReadQCByBlockNumber now return a typed types.ErrPruned instead of a bare None for below-watermark reads, and memblock gained a pruneWatermark field mirroring littblock's stranding guard; (4) data/state.go dropped its own DataWAL/ErrPruned and now aliases types.ErrPruned, plus a new ErrBlockGap for BlockDB contiguity checks in NewState (recovery path).
Security risks
No injection/authn/authz surface here. The main risk category is correctness/availability: a bug in the BlockDB close-ordering or the new ErrPruned/ErrBlockGap semantics could cause a validator or fullnode to crash, deadlock, or silently misinterpret pruned state as absent state during crash recovery — an availability/consensus-liveness risk rather than a classic vulnerability class.
Level of scrutiny
This touches consensus-critical lifecycle and persistence code (validator BlockDB, autobahn data state recovery, node startup sequencing) — the highest-scrutiny category in this repo. The lifecycle reordering in node.go is subtle (comments explicitly call out race avoidance with BaseService.OnStop/SpawnCritical), and error-semantics changes (None → ErrPruned) ripple into callers across littblock, memblock, and data/state — exactly the kind of large-scale design decision that should get a human pass even absent concrete findings.
Other factors
The bug hunting system reported no findings, and the diffs I inspected (node.go, setup.go, giga_router.go, littblock/memblock, data/state.go) are consistent and well-commented, with test files updated alongside (block_db_test.go, littblock_crash_test.go, state_recovery_test.go, etc.). PR timeline metadata was unavailable to this run, so no prior review context could be checked. Given scope and criticality, I am deferring rather than approving.
There was a problem hiding this comment.
Replaces the two-WAL DataWAL with a LittDB-backed BlockDB for Autobahn block/QC persistence, moving replay into NewState and BlockDB lifecycle ownership into nodeImpl. The core recovery, persistence-cursor, eviction, and ErrPruned-vs-ErrNotFound paths are correct and well-tested; I found no blocking issues. Notes below, including that both second-opinion review files were empty.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes produced no output: both codex-review.md and cursor-review.md are empty, and REVIEW_GUIDELINES.md is empty/missing. This synthesis therefore reflects only my own review, without Codex/Cursor input or repo-specific guidelines.
- Test-coverage gap: every recovery test prunes at an exact QC boundary (e.g. PruneBefore(gr2.First)). In production the prune height is the app-defined RetainHeight, which is QC-boundary-agnostic and will commonly land strictly inside a multi-block QC range. That path is correct (on restart the littblock watermark resets down to the oldest present QC's First and the straddling QC pins its block prefix on disk), but it is only exercised at the littblock layer (TestLittblockPartiallyPrunedQCRangeRetained), not through data.State.NewState. Consider a data.State restart test that prunes mid-QC-range then reopens, to lock in loadFromBlockDB's floor logic against the documented 'first block != first QC start' guard.
- Documented behavioral change: after restart, blocks that were pruned but not yet GC'd can briefly reappear (watermark resets down to the oldest present QC). Worth confirming downstream consumers (block-sync, evmrpc, ops tooling) tolerate a height momentarily reappearing and then being re-pruned once GC advances.
- Minor: gen_autobahn_config's default --blockdb-retention=30s is intended only for local/docker clusters. It's clearly documented as such and this command is not production node bring-up, but an operator reusing the helper for a real node would get very aggressive pruning; the empty-to-omit escape hatch mitigates this.
- Drive-by fix in node/setup.go makeCloser (len(errs) >= 0 → > 0) is correct and worthwhile — the old condition always returned a non-nil (empty-message) error even when every closer succeeded.
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.