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 46 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
Giga validator/fullnode routers open and own BlockDB in Restart behavior: pruned data may still exist on disk until BlockDB GC runs, so Reviewed by Cursor Bugbot for commit a9f6242. 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.85% 58.93% -0.92%
==========================================
Files 2278 2191 -87
Lines 189140 179344 -9796
==========================================
- Hits 113202 105696 -7506
+ Misses 65861 64375 -1486
+ Partials 10077 9273 -804
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.
wen-coding
force-pushed
the
wen/blockdb_integrate_with_data
branch
from
5c04a9a to
acdb1ac
Compare
wen-coding
dismissed
seidroid[bot]’s stale review
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.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
…rsistentStateDir Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Clean migration from the two-WAL DataWAL to a LittDB-backed BlockDB: NewState is now infallible, Init replays persisted state and snapshots write-cursors before Run spawns any goroutines, and BlockDB lifecycle is owned by the router's Run. No blocking issues found; the one Codex finding is a false positive, and the change is well covered by new recovery tests.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex flagged a potential Init-ordering race (validator peer pushing data before Init snapshots cursors). I traced the paths and believe this does NOT hold: Init runs synchronously at the top of giga *Router.Run (giga_router_validator.go:68-81 / giga_router_fullnode.go) before scope.Run spawns anything. All data.State mutators run in-scope post-Init — the giga/data.go client handlers (RunClient/RunBlockSyncClient) and avail/state.go:639 PushQC (inside avail.State.Run, spawned by consensus.State.Run). Inbound connections dispatched by the outer router before giga.Run only drive server/read handlers and feed the consensus/avail layer, never data.State directly. So Init's cursor snapshot always reflects only BlockDB-loaded state. Recommend confirming this ordering guarantee is documented so it isn't accidentally broken later.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output.
- The BlockDB open/defer-close/Init boilerplate at the top of Run is duplicated almost verbatim between gigaValidatorRouter.Run and gigaFullnodeRouter.Run; consider factoring it into a helper on gigaRouterCommon to keep the two lifecycles in sync.
- REVIEW_GUIDELINES.md in the base branch is empty, so no repo-specific standards were applied beyond AGENTS.md.
…lockDB in persist-error test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Clean, well-structured refactor replacing the two-WAL DataWAL (plus its cross-WAL reconcile logic) with a single LittDB-backed BlockDB whose ordering/contiguity invariants are enforced by the store itself; the replay, persist, prune, and lifecycle wiring all look correct and the test suite was comprehensively migrated. No blocking issues found.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- cursor-review.md is empty — the Cursor second-opinion pass produced no output, so it contributed no findings to this synthesis.
- codex-review.md reports "No material findings" but explicitly notes it could not run
go testbecause the environment only has Go 1.24.13 and cannot fetch the required Go 1.25.6 toolchain. I hit the same limitation (build/test invocation blocked). Neither automated tool nor this review compiled or ran the changed packages — ensure CI (Go 1.25.6) green-lights the build and the migrated data/recovery tests before merge. - Lifecycle ownership is correct: in both giga_router_fullnode.go and giga_router_validator.go, BlockDB is opened,
defer Close()is registered,data.Initruns beforescope.Run, andr.data.Runis spawned inside that scope — so Close only fires after all persistence goroutines have exited. Nice. - The QC-then-block-then-Flush ordering in runPersist and the straddling-QC handling in loadFromBlockDB (dropping unreachable qcs entries below the first retained block,
n >= nextQCno-QC-coverage guard) correctly match the documented BlockDB contract and the PruneBefore "resurrected below-watermark data self-heals" reasoning; no correctness gap spotted. - Verified no dangling references to the removed symbols (NewDataWAL, GlobalBlockPersister, FullCommitQCPersister, LoadedGlobalBlock, SetLoadedForTest) remain in the tree, and state.go's
errors/scopeimports are still used. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Well-structured refactor replacing the two-file DataWAL with a LittDB-backed BlockDB, splitting data.State construction into infallible NewState + Init(blockDB)/Run, with thorough test coverage. The one substantive concern is the absence of any migration or rejection path for pre-existing on-disk DataWAL state on upgrade.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Migration gap (raised by Codex, confirmed): this PR points the data layer at a fresh
<PersistentStateDir>/blockdband leaves any pre-existingglobalblocks/andfullcommitqcs/WAL dirs orphaned, with no migration or startup rejection. The consensus layer still persists itsinnerstate under<PersistentStateDir>/inner(unchanged), so a node upgraded from a build that had DataWAL state will resume consensus at an advanced height whiledata.Initreplays an empty BlockDB — a silent consensus/data mismatch that can leave execution stuck waiting for block data that was ignored. If no node with persisted DataWAL state exists yet (pre-production autobahn/giga), this is moot; otherwise consider failing fast when the legacy dirs are present, or migrating them. See inline comment. - Second-opinion inputs:
cursor-review.mdis empty (Cursor produced no output);REVIEW_GUIDELINES.mdis also empty, so no repo-specific guidelines were applied. Codex produced one finding, incorporated above. - No prompt-injection attempts were found in the PR title, description, or diff.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil, fmt.Errorf("data.NewState(): %w", err) | ||
| var blockCfgOpt utils.Option[*littblock.LittBlockConfig] | ||
| if dir, ok := cfg.PersistentStateDir.Get(); ok { | ||
| blockCfg, err := littblock.DefaultConfig(filepath.Join(dir, "blockdb")) |
There was a problem hiding this comment.
[suggestion] This switches the data layer to a fresh <PersistentStateDir>/blockdb but nothing migrates or rejects the old globalblocks//fullcommitqcs/ WAL dirs that prior builds wrote here. Because the consensus layer still persists its inner state under the same PersistentStateDir (unchanged by this PR), an upgraded node with existing state resumes consensus at an advanced height while data.Init sees an empty BlockDB — a silent mismatch that can wedge execution waiting for blocks that were dropped. If any node may already hold legacy DataWAL state, consider detecting the old dirs and failing fast (or migrating) rather than starting empty. If autobahn/giga has no persisted deployments yet, a comment noting that assumption would help.
…s in loadFromBlockDB Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit db07f65. Configure here.
| // PushAppHash only unblocks after data is crash-durable. | ||
| if err := db.Flush(); err != nil { | ||
| return fmt.Errorf("flush BlockDB: %w", err) | ||
| } |
There was a problem hiding this comment.
In-memory-only runPersist silently advances cursors without persistence
Medium Severity
When blockDB is None (in-memory mode via Init(nil)), runPersist collects batches of QCs and blocks from inner state, skips all writes (the if db, ok := s.blockDB.Get() block is false), but still advances persistedQC/persistedBlock and nextBlockToPersist. This means PushAppHash unblocks as if data were durably persisted, which is the intended behavior for in-memory mode. However, the batch collection still locks inner and copies all QC/block references into b.qcs and b.blocks slices every iteration, doing unnecessary allocations and lock contention in a mode explicitly designed for load tests where performance matters. More critically, the WaitUntil in the batch loop holds the lock while waiting, and the no-op path still pays the full snapshot cost. This is a design concern rather than a correctness bug — the in-memory path works correctly but does unnecessary work.
Reviewed by Cursor Bugbot for commit db07f65. Configure here.
| } | ||
| n := it2.Number() | ||
| if n >= in.nextQC { | ||
| return fmt.Errorf("block %d in BlockDB has no QC coverage (nextQC=%d)", n, in.nextQC) |
There was a problem hiding this comment.
Recovery rejects blocks from pre-GC pruned range
Medium Severity
In loadFromBlockDB, the block iterator check n >= in.nextQC returns a hard error when a block in BlockDB has no QC coverage. However, as discussed in the PR, LittDB's PruneBefore + Flush doesn't guarantee immediate GC — pruned blocks can reappear on restart. If a block beyond the last QC's range was written before a crash (blocks persisted ahead of their QC in runPersist), and the QC write or flush didn't complete, the block survives without a covering QC. On recovery this causes a fatal error instead of gracefully skipping the orphaned block tail.
Reviewed by Cursor Bugbot for commit db07f65. Configure here.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Solid migration from the two-WAL DataWAL to a LittDB-backed BlockDB, with the persistence/replay/prune paths cleanly rewritten and tests updated. However, moving state replay from constructor time into Init-inside-Run introduces a restart-window regression where the transport can serve inbound peers against an empty data.State, and the prune watermark is now non-durable across restarts.
Findings: 2 blocking | 4 non-blocking | 2 posted inline
Blockers
- Restart-window regression: previously
buildDataStatereplayed persisted blocks/QCs at construction (insidebuildGigaRouter, beforen.router.Start). Now replay happens indata.State.Init, called at the top ofgiga.Run, whichnode/node.go(~L560-571) spawns AFTERn.router.Start(ctx). The transport dispatches inbound giga connections togigaRouterCommon.RunInboundConn, which serves GetBlock/StreamFullCommitQCs fromr.datawith no 'initialized' gate. During the window between transport start and Init completing (which can be long while loadFromBlockDB reads a large DB), a restarting node serves empty/not-found block-sync responses for data it actually holds on disk. Consider performing Init (or opening+replaying BlockDB) beforen.router.Start, or gating RunInboundConn until Init completes. (Also raised by Codex.) - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Non-durable prune watermark (Codex #2):
PruneBeforenow advances an in-memory BlockDB watermark whose GC is async and not persisted, so a crash before GC resurrects previously-pruned blocks/QCs on reopen andloadFromBlockDBmay setinner.firstbelow the pre-crash watermark. The PR documents this instate.goPruneBefore as safe (resurrected data is QC-covered committed data) and self-healing on the next runPruning cycle, with durable bounds coming from the BlockDB retention TTL. This is a reasonable, documented tradeoff, but confirm no downstream caller relies on durable ErrPruned/retention semantics immediately after restart. ./REVIEW_GUIDELINES.mdand./cursor-review.mdwere empty, so no repo-specific guidelines were applied and the Cursor second-opinion pass produced no output. Only the Codex pass contributed external findings.- Large amount of persister code and tests deleted (fullcommitqcs.go, globalblocks.go, testonly.go and their tests) — confirm no other package still imports
consensus/persistsymbols likeGlobalBlockPersister/FullCommitQCPersister/LoadedGlobalBlockthat were removed. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| func (r *gigaFullnodeRouter) Run(ctx context.Context) error { | ||
| var initErr error |
There was a problem hiding this comment.
[blocker] Replay (data.Init) now runs at the top of Run, but node.go spawns giga.Run after n.router.Start(ctx). The transport can therefore dispatch inbound peers to RunInboundConn → service.RunInbound and serve block/QC requests against the still-empty data.State during the window before Init finishes. Previously replay happened at construction (before transport start), so this is a restart-behavior regression. Same pattern in giga_router_validator.go Run. Consider opening/replaying BlockDB before the transport starts, or gating inbound serving until Init completes.
| @@ -647,41 +610,40 @@ func (s *State) PruneBefore(retainFrom types.GlobalBlockNumber) error { | |||
| if firstToKeep <= inner.first { | |||
There was a problem hiding this comment.
[suggestion] The prune watermark is in-memory and GC is async/non-persisted, so a crash before GC resurrects pruned blocks/QCs and can lower inner.first below the pre-crash watermark on restart. The comment argues this is safe and self-healing (durable bounds come from retention TTL), which is reasonable — but worth confirming no caller depends on durable ErrPruned/retention immediately after restart.
wen-coding
force-pushed
the
wen/blockdb_integrate_with_data
branch
2 times, most recently
from
7bd0823 to
4e5115b
Compare
There was a problem hiding this comment.
A well-structured, thoroughly-tested migration from the two-file DataWAL to a single LittDB-backed BlockDB for Autobahn block/QC persistence, with the BlockDB lifecycle now owned by the router's Run. No blocking issues found; one minor defensive nit and a couple of behavioral notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's second-opinion review reported no material findings (it noted it could not run tests because the workspace lacked Go 1.25.6 / toolchain network access). Cursor's review file was empty — that pass produced no output. Both were considered; nothing material to merge in.
- Behavioral change worth calling out in the PR description / ops notes: PruneBefore now only advances an in-memory watermark and relies on BlockDB retention TTL for durable reclamation (GC is async, not persisted). On restart before GC runs, below-watermark committed data can be resurrected. The code comment argues this is safe (resurrected data is QC-covered) and self-heals on the next pruning cycle, which is reasonable, but it is a real semantics change from the old durable WAL truncation and deserves visibility.
- loadFromBlockDB assumes blockDB.QCs(false)/Blocks(false) iterate in ascending, gap-free order and relies on that for the contiguity/gap checks. This is an implicit contract on the littblock iterator; a one-line comment stating the required iteration order (or an assertion) would make the dependency explicit for future BlockDB implementations.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| ds, err := data.NewState(&data.Config{Committee: committee}, blockDB) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("data.NewState(): %w", err) | ||
| _ = blockDB.Close() |
There was a problem hiding this comment.
[nit] Minor/defensive: this blockDB.Close() is called on the atypes.BlockDB interface value without a nil guard, unlike the guarded if blockDB != nil { _ = blockDB.Close() } pattern used in NewGigaValidatorRouter. It is safe today only because data.NewState returns a non-nil error exclusively from inside its if blockDB != nil branch, so err != nil here implies blockDB is non-nil. If NewState ever grows an error path that runs with a nil blockDB (e.g. validating cfg), this would nil-panic. Consider guarding it (if blockDB != nil) for consistency and future-proofing.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
wen-coding
force-pushed
the
wen/blockdb_integrate_with_data
branch
2 times, most recently
from
81943af to
957a955
Compare
There was a problem hiding this comment.
Well-structured, thoroughly-tested migration from the two-WAL DataWAL to a LittDB-backed BlockDB for autobahn block/QC persistence. Production logic (QC-first persist ordering, straddling-QC recovery, BlockDB lifecycle) is correct and no use-after-close was found; only minor non-blocking notes.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes produced no findings: cursor-review.md is empty, and codex-review.md reports no material findings but explicitly could not build/run tests (sandbox had Go 1.24.13, network blocked, so the Go 1.25.6 toolchain couldn't be fetched). CI should be relied on to confirm the build/tests pass on Go 1.25.6.
- BlockDB lifecycle is owned by Run's deferred Close. A router that is successfully constructed but never Run (e.g. node startup aborts after NewGiga*Router returns) leaks the open BlockDB handle. This matches the pre-existing DataWAL ownership model and the process typically exits anyway, so risk is low — but a Close() on the router (or closing in the constructor's later error paths only, as done) leaves this one gap documented only implicitly.
- PruneBefore now only advances an in-memory/async watermark (GC is deferred and not persisted), so on restart before GC runs, NewState may observe below-watermark blocks/QCs and set inner.first lower than the pre-crash watermark. The code documents this as safe (resurrected data is QC-covered committed data) and self-healing on the next runPruning cycle; flagging for awareness since it is a behavioral change from the old WAL-truncation semantics.
wen-coding
force-pushed
the
wen/blockdb_integrate_with_data
branch
from
957a955 to
850806e
Compare
There was a problem hiding this comment.
A large, carefully-structured refactor swapping the DataWAL persistence layer for a LittDB-backed BlockDB in the autobahn data layer; recovery/persistence logic is thorough and well-tested, and I found no correctness blockers. Only minor, non-blocking notes remain.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex flagged goimports non-compliance in sei-tendermint/internal/autobahn/avail/inner_test.go (project import
pbordered before stdlibtesting). This is PRE-EXISTING — the import block is byte-identical on the base branch (242a9c2) and this PR does not modify those lines, so it is out of scope and won't be flagged as a new issue by CI. Optional: since the PR already touches this file, a maintainer could rungoimports -won it to clean it up. - Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty/missing, so no repo-specific standards were applied beyond AGENTS.md.
- Minor resource-leak window: if a router is constructed successfully (BlockDB opened in buildDataState) but Run is never called because node setup fails later, the opened BlockDB handle is never closed (Close only runs in Run's defer). This is a startup-only path where the process exits anyway, so impact is negligible.
- Recovery correctness has a documented coupling to littblock's GC behavior: TestRecoveryAfterPruneNoGC and code comments rely on blocks and QCs sharing the same GC filter so a below-watermark block never outlives its QC. A future BlockDB implementation that GC'd blocks and QCs independently would exercise the 'block has no QC coverage' guard (a loud error, not silent corruption), but the interface contract doesn't explicitly require this shared-GC invariant.
- Behavioral note from the PR description (pruned blocks may briefly reappear on restart until BlockDB GC catches up) is an intentional, documented design trade-off — the resurrected data is QC-covered committed data and self-heals on the next runPruning cycle. Flagged only for reviewer awareness.
wen-coding
force-pushed
the
wen/blockdb_integrate_with_data
branch
from
850806e to
acac20d
Compare
There was a problem hiding this comment.
This PR replaces the autobahn data layer's WAL-based persistence (GlobalBlockPersister/FullCommitQCPersister) with a littblock BlockDB backend, deleting the old persist WALs and rewiring NewState/runPersist/pruning around BlockDB, plus creating the persistent state dir at setup. The change is coherent and well-documented; no confirmed blocking bug, but two design/upgrade concerns (raised by Codex and confirmed here) are worth resolving before merge.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Upgrade/migration path (Codex #1, plausible): the new code opens
<PersistentStateDir>/blockdband ignores any pre-existingglobalblocks/andfullcommitqcs/WAL dirs from the previous version. A node upgraded from a build that ran the old WALs would start the data layer empty from genesis while orphaning committed WAL data, and any consensus persister state (sibling subdir) referencing committed heights could then be inconsistent with an empty data.State. Please confirm no released node ran with the old WAL persistence; if one could have, add a migration, a compatibility read path, or an explicit fail-fast when legacy WAL dirs are present. If autobahn/giga persistence has never shipped, this is a non-issue — just document that. - Prune durability (Codex #2, confirmed): littblock
PruneBeforeonly advances an in-memoryatomic.Uint64watermark (litt_block_db.go:197) that is not persisted and resets to 0 on restart; GC is asynchronous. After a restart before GC reclaims entries,NewStatereplays below-watermark blocks/QCs and setsinner.firstbelow the pre-crash prune point, so previously-pruned blocks are readable again via the RPC BlockByNumber/BlockByHash path until the nextrunPruningcycle re-establishes the watermark. The author documents this as safe (resurrected data is QC-covered/committed) and self-healing, and defers durable bounds to the BlockDB retention TTL — reasonable, but it means RetainHeight/RPC-pruning semantics can be temporarily violated across restarts (not merely "briefly"), especially if the node restarts before GC or before a new block is executed. Worth confirming this is acceptable for the RPC contract. - Cursor second-opinion review (
cursor-review.md) was empty — no Cursor findings were available to merge. - Minor: the raw
blockDBhandle is closed only viadeferinside each router'sRun. IfNewGigaFullnodeRouter/NewGigaValidatorRoutersucceeds butRunis never called, the handle leaks. Construction-time error paths already close it correctly (buildDataState and validator consensus-failure path), so this is a low-risk edge case. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
NewState(cfg, blockDB) replaces the two-step NewState()+Init(blockDB) pattern. Callers pass a BlockDB (or nil) at construction time; the constructor replays persisted state and captures the persist cursors before any goroutines start, eliminating the startup race where a concurrent PushQC could advance inner.nextQC/nextBlock past unpersisted entries before Run's runPersist loop read the watermarks. The BlockDB lifecycle is now owned by the caller (Run defers Close). preparePersistentStateDir merges the former rootify+mkdir helpers. buildDataState returns the raw BlockDB handle alongside *data.State so the router structs can defer-close it in Run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
wen-coding
force-pushed
the
wen/blockdb_integrate_with_data
branch
from
acac20d to
a9f6242
Compare
There was a problem hiding this comment.
A clean, well-tested refactor replacing the two-file DataWAL with a single LittDB-backed BlockDB in the autobahn data layer; the write-ordering, replay, and lifecycle logic are carefully reasoned and match the documented BlockDB contract, with only minor non-blocking observations.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage was thin: cursor-review.md is empty (that pass produced no output), and codex-review.md reports no material findings but notes its compile check was skipped because the sandbox could not download the Go 1.25.6 toolchain. Neither provided independent verification of the change.
- blockDB lifecycle: the handle is opened in the constructor (buildDataState) but only closed in Run's deferred cleanup. Constructors correctly close it on their own error paths (data.NewState / consensus.NewState failures), but if a router is constructed successfully and Run is never invoked (e.g. node startup aborts later for an unrelated reason), the BlockDB handle leaks. This is still a net improvement over the prior code, which did not close DataWAL from these Run methods at all — worth a follow-up if a construct-without-Run path exists.
- The PR body acknowledges that on restart, blocks pruned before shutdown may briefly reappear until BlockDB GC catches up. The PruneBefore comment argues this is safe (resurrected data is QC-covered committed data and self-heals on the next runPruning cycle). Reasonable, but worth confirming downstream consumers of GlobalBlock/BlockByNumber tolerate transiently seeing below-watermark blocks after a restart.
4 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.