special error when reading below watermark#3773
Conversation
PR SummaryMedium Risk Overview Prune behavior is tightened so a QC’s blocks change readability atomically: Tests are extended (including Reviewed by Cursor Bugbot for commit c203dc6. 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3773 +/- ##
==========================================
- Coverage 59.99% 59.03% -0.97%
==========================================
Files 2290 2201 -89
Lines 190586 180317 -10269
==========================================
- Hits 114344 106450 -7894
+ Misses 66052 64509 -1543
+ Partials 10190 9358 -832
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.
Adds a distinct types.ErrPruned for below-watermark by-number reads and wires it consistently through both the durable (littblock) and in-memory (memblock) BlockDB implementations, with thorough test updates. Clean and well-scoped; one documentation/contract accuracy concern about the "never readable again" guarantee across restarts.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (
cursor-review.md) is empty — that pass produced no output. - The new
types.ErrPrunedhas no production consumer yet (grep shows only the two implementations produce it;data.Statedoes not consume BlockDB reads, and the p2p/avail layers check a separatedata.ErrPruned). This PR is groundwork; a follow-up will need to translate BlockDBtypes.ErrPrunedinto thedata.ErrPrunedthose callers already handle, otherwise the new 'stop retrying' signal never reaches them. - Nice consistency win: memblock now tracks a watermark and refuses sub-watermark reads even when a straddling QC physically covers the height, matching littblock's by-number gate (TestMemblockPruneStraddlingQC updated accordingly).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| defer func() { _ = db.Close() }() | ||
| writeAll(t, db, batches) | ||
|
|
||
| watermark := batches[1].first |
There was a problem hiding this comment.
nit: can you prune in the middle of a QC to make the watermark case more interesting?
There was a problem hiding this comment.
This test was missing. When I added it, it exposed a real bug where we would allow the prune watermark in memory be in the middle of a QC's range. I also fixed the bug.
| // ErrPruned is returned by the by-number read methods (ReadBlockByNumber and | ||
| // ReadQCByBlockNumber) when the requested GlobalBlockNumber is strictly below | ||
| // the retention watermark: the record has been pruned (or is being reclaimed) | ||
| // and will never become readable again. It is distinct from a utils.None |
There was a problem hiding this comment.
nit: we don't write the watermark onto disk, so never say never...
| // on its next GC pass, and only after a retention-TTL floor). The | ||
| // watermark guarantees nothing below n is removed before n is | ||
| // reached, but does NOT bound when eligible data is actually | ||
| // reclaimed — pruned entries may remain readable for a while. |
There was a problem hiding this comment.
do we need to update this comment?
There was a problem hiding this comment.
This is still technically accurate. If there is a QC spanning blocks 100-110 and you ask to PruneBefore(105), internally it will clamp the watermark to block 100.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM - the one finding is a minor doc-wording nit, not a correctness issue.
Extended reasoning...
Overview: This PR introduces types.ErrPruned, a sentinel error returned by ReadBlockByNumber/ReadQCByBlockNumber in both littblock and memblock when a query falls strictly below the retention watermark, distinguishing pruned from never-written. memblock gains its own watermark field to match littblock's existing behavior for straddling QCs. The interface doc comment in block_db.go is updated to describe the three-way Some/ErrPruned/None outcome. Tests are updated across both implementations to assert ErrPruned where a plain None was previously expected, plus one new contract test.
Security risks: None. This is an internal storage-layer contract change with no auth, crypto, or externally-controlled input involved. The change is purely about which sentinel value/error is returned from an existing gate check that already existed (n below watermark) - no new gating logic is introduced, just a differentiated return value.
Level of scrutiny: Low-to-moderate. The change is mechanical: existing below-watermark branches that returned None with a nil error now return None with ErrPruned, plus a new watermark field added to memblock mirroring the pattern already proven out in littblock. No production caller yet consumes types.ErrPruned (grepped - only the pre-existing, unrelated data.ErrPruned is used elsewhere), so this PR only changes the contract surface, not runtime behavior of any live caller. Test coverage is thorough and updated consistently across both implementations.
Other factors: One nit-level finding was surfaced: the new doc language about a pruned record never becoming readable again is not durable across a process restart for littblock, because recoverReadWatermark re-derives the watermark from the lowest surviving QC key rather than persisting the exact PruneBefore value, which can be lower when a straddling QC survives GC. This does not violate any data-integrity invariant (a re-served block still has a readable covering QC) and is scoped to precision of the doc comment, not correctness of the gating logic itself. Given no current caller depends on the literal wording, and the underlying safety property holds, this does not rise to a level requiring a hold on approval.
There was a problem hiding this comment.
Introduces a distinct types.ErrPruned for below-watermark reads across both BlockDB backends, with strong tests and docs; sound and low-risk since no production caller reads these methods yet. One latent consistency concern: littblock's prune clamp trusts the QC's embedded GlobalRange().First while WriteQC/memblock use the supplied index bounds, with no validation that they match.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- littblock.clampPruneBoundary derives the rounded watermark from the QC's embedded GlobalRange().First, whereas memblock rounds using the supplied WriteQC bounds (e.lower/e.upper). The BlockDB.WriteQC contract does not require lowerBound == qc.GlobalRange().First and neither backend validates it, so a mismatched QC would make the two backends disagree and could push littblock's watermark above the requested height (even past the newest block, over-pruning). Not production-reachable today (only blocksim/tests call these), but worth hardening: validate lowerBound==GlobalRange().First / upperBound==Next in WriteQC, or clamp from the index bounds for parity with memblock. (Also raised by Codex as P1.)
- There are now two distinct ErrPruned sentinels — the new types.ErrPruned and the pre-existing data/state.go ErrPruned (errors.New("pruned")). Harmless now but should be reconciled/mapped when BlockDB is wired into data.State.
- cursor-review.md was empty — the Cursor second-opinion pass produced no output.
- Codex's second-opinion review flagged only the clampPruneBoundary/WriteQC consistency issue above; no additional findings from it.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if err != nil { | ||
| return 0, fmt.Errorf("failed to decode covering QC for %d: %w", blockHeight, err) | ||
| } | ||
| return qc.QC().GlobalRange().First, nil |
There was a problem hiding this comment.
[suggestion] This rounds the watermark to the QC's embedded GlobalRange().First, but WriteQC indexes the QC under the separately-supplied lowerBound/upperBound and never validates they match the embedded range (nor does the interface contract require it). memblock's clamp instead uses the stored index bounds (e.lower). If a QC's embedded First ever differs from its index position, the two backends diverge and littblock can move the watermark above the requested height — potentially past the newest block, over-pruning. Consider validating lowerBound == qc.GlobalRange().First in WriteQC, or clamping from the index bounds here for parity with memblock.
There was a problem hiding this comment.
LGTM — the only finding from this run is a nit on test-only helper code (GenFullCommitQCRange) with no effect on any current code path, and the earlier round-trip/restart concerns I and other reviewers raised have already been addressed (godoc updated, watermark-rounding bug found by the new test and fixed).
Extended reasoning...
Overview
This PR adds a distinct types.ErrPruned error for ReadBlockByNumber/ReadQCByBlockNumber when a requested block/QC is below the retention watermark, replacing the previous ambiguous utils.None. It also tightens PruneBefore so the watermark always rounds down to a QC's GlobalRange().First (littblock via the new clampPruneBoundary, memblock via an equivalent in-memory scan), guaranteeing a QC's covered blocks change readability atomically. Touches littblock, memblock, the types.BlockDB interface docs, and test infrastructure (GenFullCommitQCRange replacing GenFullCommitQCN).
Security risks
None. This is internal storage/retention logic in the consensus block store, not exposed to external input or untrusted data.
Level of scrutiny
Medium — this changes read/prune semantics of a durable store consumed by consensus-critical code (data.State), so correctness of the watermark/cohort-rounding logic matters, but no callers currently consume ErrPruned yet (per the PR's own docs), which limits blast radius today. The PR has strong test coverage (both littblock and memblock exercise the same scenarios, including a new test that itself caught and fixed a real watermark-rounding bug), and prior review rounds (mine, cursor/seidroid/wen-coding, cody-littley) already surfaced and resolved the restart/permanence-of-ErrPruned concerns via doc wording and a bug fix.
Other factors
The one remaining finding this run (GenFullCommitQCRange's Next not surviving a protobuf round-trip) is confined to test-only code and has no current caller relying on it — a nit, not a blocker. The author has been responsive throughout the thread, addressing every prior reviewer comment (including my own) with either a code fix or a doc clarification.
| // GenFullCommitQCRange generates a FullCommitQC whose GlobalRange is | ||
| // [first, next) and which is internally consistent (First + len(headers) == | ||
| // Next), for tests that index a QC by its own range. littblock does not verify | ||
| // signatures, so the QC need not be otherwise well-formed. | ||
| func GenFullCommitQCRange(rng utils.Rng, first GlobalBlockNumber, next GlobalBlockNumber) *FullCommitQC { | ||
| qc := GenCommitQC(rng) | ||
| qc.vote.Msg().Proposal().globalRange = GlobalRange{First: first, Next: next} | ||
| headers := utils.GenSliceN(rng, int(next-first), GenBlockHeader) //nolint:gosec // small test range | ||
| return NewFullCommitQC(qc, headers) |
There was a problem hiding this comment.
🟡 GenFullCommitQCRange's doc comment promises the returned QC's GlobalRange stays internally consistent (First + len(headers) == Next), but that only holds for the freshly-constructed in-memory object. Because it builds the QC via GenCommitQC/ProposalAt with zero lane ranges and then pokes globalRange directly, any protobuf encode/decode round-trip (which littblock performs on every WriteQC/Read) recomputes Next as First + sum(lane-range lengths) and silently collapses it back to First. This is test-only code (testonly.go) and no current test or production code path reads a round-tripped QC's .Next off this helper, so nothing in this PR is broken today, but it's a genuine footgun in the PR's own new public test API for a future littblock/memblock test that asserts .Next after a read-back.
Extended reasoning...
GenFullCommitQCRange (sei-tendermint/autobahn/types/testonly.go:409-417) is new test-helper API added by this PR to replace GenFullCommitQCN. It builds a CommitQC via GenCommitQC → CommitQCAt → ProposalAt, which constructs a Proposal with an empty laneRanges map, and then directly overwrites the private field qc.vote.Msg().Proposal().globalRange = GlobalRange{First: first, Next: next}. The doc comment states the returned QC "is internally consistent (First + len(headers) == Next)".
That claim is true only of the freshly-constructed, in-memory object. Proposal.GlobalRange() (proposal.go:209-211) just returns the struct field, so reading it back immediately after construction reflects the injected value. But ProposalConv (proposal.go) only ever serializes GlobalFirst on the wire — Encode never writes Next at all. Decode reconstructs the proposal via newProposal (proposal.go:175-191), which recomputes Next as First + sum(laneRange lengths). Since the underlying proposal built by ProposalAt has zero lane ranges, that sum is always 0, so any encode→decode round trip collapses Next back down to First, silently discarding the value passed into GenFullCommitQCRange.
This isn't a hypothetical serialization path — it's the exact one littblock performs on every WriteQC/read. encodeQC calls types.FullCommitQCConv.Marshal(qc) and decodeQC calls types.FullCommitQCConv.Unmarshal(...), both of which delegate down through CommitQCConv → CommitVoteConv → ProposalConv. Three independent verifiers reproduced this concretely: calling types.GenFullCommitQCRange(rng, 100, 110) yields GlobalRange{First:100, Next:110} in memory (10 headers), but after types.FullCommitQCConv.Marshal + Unmarshal — littblock's exact round trip — it reads back as GlobalRange{First:100, Next:100}, with the 10 headers still present but the range now zero-length.
Why nothing in this PR currently observes it: clampPruneBoundary and every current caller of a decoded QC only ever read GlobalRange().First (which is serialized and survives). recoverCursors derives lastQCNext from lowerBound + len(qc.Headers()) (the header count, which survives in full), never from GlobalRange().Next. The new litt_block_stranding_test.go tests that use this helper (writeSyntheticBatches) only ever assert .First after a round trip. So the refutation is right that no code path in this PR is broken by this — I'm not claiming it is.
Why it's still worth flagging (as a nit, not a blocker): the helper is the PR's own new exported test API (types.GenFullCommitQCRange), specifically built to feed littblock (a store that always round-trips through protobuf), and its doc comment makes an unqualified consistency promise that doesn't survive that exact round trip. A future littblock/memblock test — or a reviewer extending testPrunedDistinctFromNotFound-style coverage — asserting .GlobalRange().Next on a QC obtained via ReadQCByBlockNumber built with this helper would silently get a collapsed, wrong zero-length range instead of the range requested, with no error or panic to flag the mistake. That's a real footgun in new test infrastructure even though it causes zero harm today.
Step-by-step proof:
- Call
qc := types.GenFullCommitQCRange(rng, 100, 110). - In memory:
qc.QC().GlobalRange()returns{First: 100, Next: 110}, andlen(qc.Headers()) == 10, matching the doc's claimed invariant. - Encode:
bytes := types.FullCommitQCConv.Marshal(qc)(what littblock'sencodeQCcalls). Internally this delegates toProposalConv.Encode, which serializes onlyGlobalFirst = 100—Nextis never written to the wire. - Decode:
qc2 := types.FullCommitQCConv.Unmarshal(bytes)(what littblock'sdecodeQCcalls). This callsnewProposal, which recomputesNext = First + sum(laneRange lengths) = 100 + 0 = 100, since the proposal has no lane ranges. - Result:
qc2.QC().GlobalRange()is now{First: 100, Next: 100}— a zero-length range — whilelen(qc2.Headers())is still 10. The invariantFirst + len(headers) == Next(100 + 10 == 110) that held in step 2 is now false (100 + 10 != 100).
Suggested fix: either build the proposal with real lane ranges sized to match [first, next) so Next is derived correctly and survives serialization, or add a doc caveat noting the invariant only holds for the in-memory object returned by the constructor and does not survive a protobuf round trip (e.g. through littblock).
2 participants
Describe your changes and provide context
Return a special error when reading a block below the watermark.