BlockDB: no orphaned blocks#3734
Conversation
PR SummaryMedium Risk Overview littblock treats the prune watermark as a read gate (not only GC eligibility):
The Reviewed by Cursor Bugbot for commit fc8af7a. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3734 +/- ##
==========================================
- Coverage 59.88% 58.96% -0.93%
==========================================
Files 2283 2196 -87
Lines 189429 179586 -9843
==========================================
- Hits 113431 105884 -7547
+ Misses 65906 64412 -1494
+ Partials 10092 9290 -802
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.
The read-watermark gate that refuses below-watermark blocks/QCs and re-derives the watermark on restart is correct and well-tested, upholding the new "a readable block always has a readable covering QC" invariant. However, the newly added doc comment contains a misspelling that the enabled misspell linter will reject, failing CI.
Findings: 2 blocking | 4 non-blocking | 2 posted inline
Blockers
gofmt/misspell: the new doc block in sei-tendermint/autobahn/types/block_db.go (lines 76-77) will fail CI — "gauranteed" trips the enabledmisspelllinter and the trailing whitespace on line 76 is removed bygofmt. Fix both before merge (raised by Codex).- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Method-level docs are now stale relative to the new top-level guarantee: ReadBlockByNumber (lines 199-206), ReadBlockByHash (lines 214-217), and ReadQCByBlockNumber (lines 226-231) still say a below-watermark block/QC "may remain readable until it is actually reclaimed." Both implementations now never serve below-watermark blocks (littblock refuses via the read gate; memblock deletes synchronously in PruneBefore), so these docs should be tightened to match. (Codex point #2, adjusted — the docs are a permissive superset so not strictly wrong, but misleading.)
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output.
- Minor: in qcIterator.Next, decodeNumberKey(key) is computed twice for a straddling QC (lines 96 and 103); could be hoisted to a local. Negligible.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped, correct fix that gates below-watermark reads/iterators in the durable littblock BlockDB so blocks whose covering QC was asynchronously GC'd are never served, with a re-derived on-open watermark and strong white-box regression tests. No blocking issues found; only minor/informational notes.
Findings: 0 blocking | 3 non-blocking | 0 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 findings and could not run the Go 1.25.6 tests in its sandbox.
- recoverReadWatermark's correctness depends on litt forward iteration being insertion order combined with WriteQC's ascending-contiguous enforcement (so the first primary QC encountered is the lowest surviving one). This is correct but implicit; a one-line note tying the scan to the insertion-order/contiguity guarantee would help future readers. Also, the 'no QC survives' path does a full-table scan on open — acceptable since it only happens when every QC has been reclaimed, but worth being aware of.
- Behavior change: below-watermark reads/iterators now reliably return None rather than 'maybe'. This is within the (updated) interface contract and no production callers rely on the old behavior (only blocksim/tests consume Blocks()/QCs()), so it is safe — flagging only for awareness.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
| } | ||
|
|
||
| // No QC survives. Any block still present is therefore stranded, so refuse | ||
| // every one by setting the watermark just past the newest block. |
There was a problem hiding this comment.
The contract at consensus level is that we should always keep at least one (block, qc) pair when pruning. So if you reach here, something is broken. Maybe the user accidentally removed the QC WAL file? If that's the case, is the right behavior:
- just panic and exit
or - just remove all blocks?
I don't know how much I should trust the remaining blocks at this point.
There was a problem hiding this comment.
I mean, we have code in consensus to guarantee that, so it's up to you whether you want to double-enforce it inside Block DB.
There was a problem hiding this comment.
This PR adds a read-watermark gate to the littblock BlockDB (plus a never-empty prune clamp shared with memblock) so that stranded below-watermark blocks are never served, upholding the newly documented "a readable block always has a readable covering QC" invariant; the change is well-tested and cleanly documented. However, PruneBefore on an empty littblock store records an unclamped watermark that then silently hides (and lets GC delete) subsequently written, QC-covered blocks — violating the PR's own "pruning never empties the store" guarantee and diverging from memblock.
Findings: 1 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Iterator watermark snapshot (Codex Medium):
Blocks/QCscaptures.watermarkat creation, and the underlying litt iterator is itself a point-in-time snapshot, so a block yielded by a pre-prune iterator was covered by a QC in that same snapshot — internally consistent and by design. The only observable oddity is cross-referencing a long-lived pre-prune iterator with a fresh point read (ReadQCByBlockNumber), which uses the current, higher watermark and may return None for a block the iterator still yields. Worth a one-line note in the iterator doc that its watermark is fixed at creation and may lag a later prune. REVIEW_GUIDELINES.mdandcursor-review.mdare both empty (no repo-specific guidelines and no Cursor second-opinion output were available for this pass); the Codex review was present and its two findings are addressed above/inline.- Nice test coverage:
testPruneQCAheadOfBlocks,testPruneNeverEmpties, and the littblock stranding tests directly exercise the clamp/gate edge cases; the empty-store prune case (see inline blocker) is the one uncovered gap. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| if s.hasBlocks { |
There was a problem hiding this comment.
[blocker] Empty-store prune records an unclamped watermark that hides later valid blocks (matches Codex's High finding). The clamp only runs when s.hasBlocks, so PruneBefore(N) on an empty store stores watermark = N directly. If blocks below N are then written (each with a covering QC, which the write path accepts — it never checks the watermark), ReadBlockByNumber/ReadBlockByHash/the iterators all refuse them (n < watermark), and gcFilter marks their block+QC keys reclaimable, so GC can physically delete freshly written, QC-covered data. This violates the PR's own newly documented guarantee ("Once a block has been written, at least one block ... always remains readable") and diverges from memblock, which has no read-gate and serves such writes normally. Consider clamping/lowering the watermark to available data on write, or not advancing the watermark past the newest written block regardless of hasBlocks.
| hasQC bool | ||
| lastQCNext types.GlobalBlockNumber | ||
|
|
||
| // latestQCStartBlock is therecently written QC's starting block number. |
There was a problem hiding this comment.
[nit] Typo: "is therecently written QC's" → "is the most recently written QC's" (missing space and word).
| hasQC bool | ||
| lastQCNext types.GlobalBlockNumber | ||
|
|
||
| // latestQCStartBlock is therecently written QC's starting block number. |
There was a problem hiding this comment.
🟡 Nit: docstring typo on the newly added latestQCStartBlock field — "is therecently written QC's starting block number" is missing a space (and a word). Should read "is the most recently written QC's starting block number" to match the parallel comment on the same field in memblock/mem_block_db.go. Sibling to the gauranteed typo already caught on this PR.
Extended reasoning...
What's wrong
At sei-db/ledger_db/block/littblock/litt_block_db.go:45, the doc comment for the newly added latestQCStartBlock field reads:
// latestQCStartBlock is therecently written QC'\''s starting block number.
latestQCStartBlock types.GlobalBlockNumbertherecently is a broken glue-word — a missing space between the and recently, and semantically it should be "the most recently written" (matching the parallel comment on the same field in memblock/mem_block_db.go, which does read "the most recently written QC'''s starting block number").
Why the linters won'''t catch it
The repo'''s misspell linter (.golangci.yml) only flags known dictionary misspellings — the same reason gauranteed → guaranteed was caught but this one isn'''t. therecently is not in misspell'''s dictionary; the and recently are both individually valid tokens, so no automated tool will surface this. It needs a human eye.
Impact
Pure docstring polish on a newly introduced private field — no functional or behavioral impact whatsoever, and no CI failure. It'''s user-visible only when someone reads the source (godoc excludes private fields). The reason it'''s still worth catching in the same touch: the sibling memblock field carries the correctly-worded version, so the two-line comment mismatch will read as a maintenance smell to the next reader ("which docstring is authoritative?"). Fixing it now is one edit; leaving it costs someone a moment of confusion later.
Step-by-step proof
git grep -n therecently sei-db/ledger_db/block/returns a single hit:littblock/litt_block_db.go:45.- Compare to the parallel field'''s comment in
sei-db/ledger_db/block/memblock/mem_block_db.go(see the diff, mem_block_db.go around line 46):// latestQCStartBlock is the most recently written QC'\''s starting block number —— well-formed, and clearly the intended wording. - Run misspell (or trigger
make lint):therecentlyis not flagged, confirming the linter cannot backstop this.
How to fix
One-line edit at sei-db/ledger_db/block/littblock/litt_block_db.go:45:
- // latestQCStartBlock is therecently written QC'\''s starting block number.
+ // latestQCStartBlock is the most recently written QC'\''s starting block number.That aligns the two implementations''' comments and closes the last of the docstring polish issues on this PR (following the already-fixed gauranteed and trailing-whitespace bugs in block_db.go).
3 participants
Describe your changes and provide context
Don't return blocks that have had their QC messages pruned.
Testing performed to validate your change
unit tests