Add GetSubrange for LittDB - #3815
Conversation
PR SummaryMedium Risk Overview The read path is wired through
Tests cover disk-table variants, cache parity/hits, compressed fallback, block framing, and subrange behavior before/after flush. Reviewed by Cursor Bugbot for commit f0a4720. 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 #3815 +/- ##
==========================================
- Coverage 60.23% 59.28% -0.96%
==========================================
Files 2332 2240 -92
Lines 195168 184745 -10423
==========================================
- Hits 117564 109529 -8035
+ Misses 66958 65422 -1536
+ Partials 10646 9794 -852
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds litt.Table.GetSubrange threaded through cachedTable → DiskTable → Segment.ReadSubrange, plus littblock.(*blockDB).GetTxByOffset and three new test files. I found no correctness bugs: bounds checks are consistently computed in uint64, the compressed fallback is correct, zero-length reads return non-nil slices, and only two types implement litt.Table/ManagedTable so the interface addition doesn't break other implementers. Remaining notes are about the cache-bypass rationale, a dangling doc reference, an unreachable-from-outside API, and test-coverage/description accuracy.
Findings: 0 blocking | 11 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage gap vs. PR description: the description says
disk_table_subrange_test.gois table-driven across "mem/pebble × cached/uncached", buttableBuilders(disk_table_test.go:32-49) only builds rawDiskTables — there is no cached variant, andsei-db/db_engine/litt/dbcache/has no test files at all.cachedTable.GetSubrangeis only exercised indirectly via littblock, where the defaultTableConfigsetsWriteCacheSize/ReadCacheSizeto 0, so the caches are always empty. The one deliberate behavioral decision in this PR (cache bypass) therefore has no direct test. Worth adding a small dbcache test that populates the read cache viaGet, then assertsGetSubrangestill returns the right bytes (and, if you keep the bypass, that a base-table-evicted-but-cached key behaves as documented). GetTxByOffsetis unreachable from outside the package:blockDBis unexported andNewBlockDBreturnstypes.BlockDB, so no consumer can type-assert to it. The follow-up producer PR will have to add an exported interface anyway — consider defining it now (e.g.type TxRangeReader interface { GetTxByOffset(types.GlobalBlockNumber, uint32, uint32) (utils.Option[[]byte], error) }) so the method is callable and the extension point is explicit. Right now the only callers are the new tests.- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty). Codex reported "No material issues found in the PR diff." So the Cursor perspective is effectively missing from this synthesis. - Minor duplication: the same "subrange [%d, %d) is out of bounds for value of length %d" bounds check and message is written three times (disk_table.go unflushed path, segment.go compressed path, segment.go uncompressed path). A small shared helper would keep the three in sync if the semantics ever change.
- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| // - The read and write caches are keyed by the full key and store the full value. A subrange read cannot | ||
| // safely populate them (it would store a partial value under a key that Get is entitled to treat as the | ||
| // whole value), and it does not consult them either: even on a cache hit the whole point of a subrange | ||
| // read is to avoid materializing the full value, and slicing a cached full value would defeat the I/O |
There was a problem hiding this comment.
[suggestion] The rationale for not consulting the caches is inverted. On a cache hit the full value is already in memory, so slicing it costs zero I/O — it can't "defeat the I/O savings," it strictly beats the base-table read. Not populating the caches is well justified (a partial value under a key Get treats as whole); not reading them means that for any table configured with non-zero WriteCacheSize/ReadCacheSize, GetSubrange can be slower than plain Get for exactly the hot, recently-written values the tx-index use case targets.
Consulting both caches would also be semantically safe here: Put/PutBatch store value[sk.Offset:sk.Offset+sk.Length] under a secondary key, and readCache stores whatever Get returned, so a cached entry is always the same logical value the disk path would slice — offsets stay relative to the same region.
A related consequence worth documenting either way: because the caches are bypassed, a key that has been reclaimed from the base table (GC/TTL) but is still resident in readCache/writeCache will return exists=true from Get and exists=false from GetSubrange.
| defer func() { | ||
| if exists && value != nil { | ||
| // hot is always false: subrange reads never come from a cache (see the cache policy above). | ||
| c.metrics.ReportReadOperation(c.Name(), time.Since(start), uint64(len(value)), false) |
There was a problem hiding this comment.
[nit] Subrange reads are folded into the same ReportReadOperation series as Get, but report len(value) = subrange length. That will pull the table's average read size and throughput down in a way that isn't a real change in read cost, and makes the two access patterns indistinguishable in metrics. Consider a separate counter/histogram (or at least a distinguishing attribute) for subrange reads.
|
|
||
| // The value starts at dataAddress.Offset(); the sub-range starts offset bytes further in. Both operands | ||
| // and their sum are bounded by the value file size (< 2^32), so the uint32 addition cannot overflow. | ||
| value, err := values.read(dataAddress.Offset()+offset, length) |
There was a problem hiding this comment.
[nit] dataAddress.Offset()+offset is the only arithmetic here done in uint32 rather than uint64. The comment above asserts the sum is bounded by the value file size (< 2^32), but valueFile.write only refuses to start a value once v.size > MaxUint32, so a value that begins at, say, 4.0e9 and is 1e9 long ends past 2^32. If that ever happened, the addition would wrap to a small index, valueFile.read's bounds check (computed in uint64 against flushedSize) would pass, and you'd silently return the wrong bytes rather than an error. Practically unreachable with current segment size limits, but since end is already computed in uint64 two lines up, it costs nothing to compute the start in uint64 too and reject > MaxUint32 explicitly, converting a silent-wrong-data mode into an error.
| // without decoding the block. offset and length identify a byte range within the block's stored value — | ||
| // the exact bytes encodeBlock produced: [version:1][GlobalBlockNumber:8][proto(Block)] — typically the | ||
| // (offset, length) of one transaction recorded at write time (see the tx-location-index design under | ||
| // sei-db/ledger_db/receipt/docs). offset is measured from the start of that stored value (i.e. it includes |
There was a problem hiding this comment.
[nit] Dangling doc reference: sei-db/ledger_db/receipt/docs does not exist in the tree (the only docs directory under sei-db is sei-db/db_engine/litt/docs), and line 441's "see the compression note in the design doc" points at the same non-existent doc. Either land the design doc in this PR or reference something that exists.
| return utils.None[[]byte](), types.ErrPruned | ||
| } | ||
|
|
||
| value, exists, err := s.table.GetSubrange(blockKey(n), offset, length) |
There was a problem hiding this comment.
[suggestion] Two things worth adding to the contract here:
- The returned slice aliases LittDB-internal memory (before flush it is a sub-slice of the live
unflushedDataCacheentry for the whole block), so it must not be mutated. That's a meaningful difference fromReadBlockByNumber, which hands back a freshly decoded block, and thelitt.Table.GetSubrange"NOT safe to mutate" warning isn't restated on this method. - There's no validation that
value[0] == blockSerializationVersion. Since offsets are recorded at write time and persisted elsewhere (receipt records), any future bump toblockSerializationVersionshifts the layout and previously recorded offsets would silently return wrong bytes rather than erroring. Consider reading the version byte (or at least requiringoffset >= blockValuePrefixLen) so a layout mismatch fails loudly.
| // genBlockWithTxs returns a random block guaranteed to carry at least one transaction, so the | ||
| // tx-extraction assertions below are meaningful. | ||
| func genBlockWithTxs(rng utils.Rng) *types.Block { | ||
| for { |
There was a problem hiding this comment.
[nit] Unbounded retry loop — if GenBlock ever stops producing blocks with transactions this hangs until the test timeout rather than failing with a clear message. A bounded loop (for i := 0; i < 100; i++ { ... }; t.Fatal("no block with txs generated")) fails fast and self-describes.
| // length-delimited `repeated bytes` element), so locating one gives a valid (offset, length) — | ||
| // exactly what a writer would record. Extracting that range must return the transaction. | ||
| for _, tx := range blk.Payload().Txs() { | ||
| idx := bytes.Index(stored, tx) |
There was a problem hiding this comment.
[nit] Locating the tx range with bytes.Index is a bit fragile as a stand-in for what a real writer computes: it can match a coincidental earlier occurrence (harmless, since the bytes compare equal, so the assertion can pass without actually exercising the intended offset), and if a generated tx is empty it degenerates to idx == 0 with a require.Equal(nil-vs-empty) risk. Consider a synthetic block with distinctive, non-empty tx payloads and offsets derived from the encoding, so the test pins the real coordinate space the follow-up producer will compute.
cody-littley
left a comment
cody-littley
left a comment
There was a problem hiding this comment.
Straight forward change, LGTM
There was a problem hiding this comment.
GetSubrange is threaded correctly and consistently through every layer (interface → cachedTable → DiskTable → Segment), with bounds checks widened to uint64 at each level and good test coverage across all four disk-table configs plus the compressed fallback; I found no correctness or security blockers. Remaining notes are defensive/documentation polish: sub-range slices returned from the in-memory paths carry spare capacity into live cached data, and two doc references point at a directory that doesn't exist.
Findings: 0 blocking | 9 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Cursor's review file (
cursor-review.md) is empty — that pass produced no output. Codex reported "No material issues found." So all findings below come from this pass only. - Verified the overflow claim in
Segment.ReadSubrange:dataAddress.Offset()+offsetgenuinely cannot wrap, becausecontrol_loop.go:498rolls to a fresh segment whenGetMaxShardSize()+onDiskLen > math.MaxUint32, guaranteeingOffset()+ValueSize() <= 2^32-1. Worth citing that guard in the comment rather than asserting "bounded by the value file size (< 2^32)" —valueFile.writealone only bounds the start offset, so the invariant isn't locally evident. GetTxByOffsetships as effectively test-only code: the receiver*blockDBis unexported andNewBlockDBreturnstypes.BlockDB, so no production caller can reach it. The PR description acknowledges this and defers the exported access point to the producer PR — fine to land, but note the offset convention has no non-test consumer validating it yet.- The
GetTxByOffsetoffset space includes the 9-byte[version:1][GlobalBlockNumber:8]prefix, so a producer computing offsets againstproto(Block)is silently off byblockValuePrefixLen. It is documented, but consider making the follow-up producer share a single constant (or accept proto-relative offsets) so the two sides can't drift. - I could not run
go build/go testin this environment (those commands required approval), so this review is static. Symbol-level checks did pass:litt.Tablehas exactly two implementations in-tree (cachedTable,DiskTable), both updated, so the interface addition is not a breaking change; and all helpers the new tests rely on (strandingConfig,writeSyntheticBatches,encodeBlock,GenBlock,GenFullCommitQCRange,TestRngFromSeed) exist in the same packages. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil, false, fmt.Errorf( | ||
| "subrange [%d, %d) is out of bounds for value of length %d", offset, end, len(full)) | ||
| } | ||
| return full[offset:end], true, nil |
There was a problem hiding this comment.
[suggestion] full[offset:end] keeps the spare capacity of the unflushed value, so the returned slice's backing array extends past end into bytes that are still pending a write to disk. A caller that appends to the returned sub-range (the natural thing to do with a small byte range) would silently overwrite the tail of the not-yet-flushed value, and that corruption becomes durable. The post-flush path is immune because valueFile.read allocates make([]byte, length) (cap == len), so the hazard only shows up before a flush — hard to reproduce. Suggest a three-index slice: full[offset:end:end].
The interface contract does say the result is not safe to mutate, but append doesn't read as mutation, and capping is free.
| return nil, false, fmt.Errorf( | ||
| "subrange [%d, %d) is out of bounds for value of length %d", offset, end, len(value)) | ||
| } | ||
| return value[offset:end], true, nil |
There was a problem hiding this comment.
[suggestion] Same capacity concern as the disk-table unflushed path: value[offset:end] aliases the cached full value with spare capacity, so a caller appending to the sub-range writes into the entry still held by writeCache/readCache and visible to every later Get. value[offset:end:end] removes the foot-gun and keeps cache-hit results shaped like the exact-capacity buffers the disk path returns.
| // without decoding the block. offset and length identify a byte range within the block's stored value — | ||
| // the exact bytes encodeBlock produced: [version:1][GlobalBlockNumber:8][proto(Block)] — typically the | ||
| // (offset, length) of one transaction recorded at write time (see the tx-location-index design under | ||
| // sei-db/ledger_db/receipt/docs). offset is measured from the start of that stored value (i.e. it includes |
There was a problem hiding this comment.
[nit] sei-db/ledger_db/receipt/docs does not exist in the tree (the only docs directory under sei-db is sei-db/db_engine/litt/docs), so this pointer and the "see the compression note in the design doc" reference on line 441 are both dangling. Either add the design doc in this PR or drop/reword the references so the comment stands on its own.
| } | ||
| } | ||
|
|
||
| // getSubrangeParityTest runs against every implementation, cached and uncached, and pins the property |
There was a problem hiding this comment.
[nit] The comment claims this pins that a sub-range read is "indistinguishable between the cached and the base path," but for the cached builders it only ever exercises the cache-hit path: the 19-byte value stays in the 500-byte writeCache (Flush doesn't evict it), so c.base.GetSubrange is never reached on a cached table. The assertions are still valuable across builders, and DiskTable.GetSubrange is covered directly in the disktable tests — but if you want the stated property, add a case where the caches can't answer (value larger than the cache weight, or a table built with size-0 caches) so the fall-through is actually compared against the cache hit.
There was a problem hiding this comment.
This PR adds a nontrivial read-path primitive (GetSubrange) across LittDB's core storage layers (cache, disk table, segment) plus a new block-store method built on it, so I think it warrants a human look before merging.
What was reviewed: the cache consult-but-never-populate policy in cachedTable.GetSubrange and its interaction with Get; the uncompressed bounded-read vs. compressed decompress-then-slice paths in Segment.ReadSubrange; and GetTxByOffset's watermark/pruning behavior in littblock. Several open review threads from other reviewers (slice-capacity aliasing that could let an append corrupt not-yet-flushed/cached data, a uint32 offset-overflow edge case, dangling doc references) do not appear to have been addressed by new commits yet.
Extended reasoning...
This PR introduces litt.Table.GetSubrange, threaded through cachedTable, DiskTable, and Segment, plus a new littblock.(*blockDB).GetTxByOffset built on top of it. It touches core storage read paths used throughout the block/ledger database, including cache semantics and raw segment I/O.
No new bugs were flagged by the bug hunting system in this run, and my own prior finding (a dangling doc reference) is already posted as an inline comment. However, other reviewers (seidroid, cursor) raised several substantive points against the current diff that look like real correctness/robustness concerns rather than nits — most notably that full[offset:end] and value[offset:end] slices returned from the unflushed-data path and the cache-hit path retain spare capacity from their backing arrays, so a caller that appends to a returned sub-range (a natural thing to do with a small byte range) could silently corrupt not-yet-flushed or cached data. There is also a uint32-arithmetic edge case in Segment.ReadSubrange's offset computation that is practically unreachable today but would silently return wrong bytes if segment size limits ever changed.
None of these have visible follow-up commits addressing them as of this review. Given this is core storage-engine code (not a leaf feature), and the open threads point at a real (if currently low-probability) data-corruption vector, I think this is squarely in human-review territory rather than something to wave through, even though the automated bug hunter did not surface a new finding this run.
There was a problem hiding this comment.
GetSubrange is threaded correctly through all three layers — the unflushed/keymap/segment split in DiskTable, the bounded uncompressed read and the compressed full-read-then-slice fallback in Segment.ReadSubrange, and the consult-but-never-populate cache policy in cachedTable all agree on offset framing (including for secondary keys, whose unflushed/cached entries and on-disk addresses both cover only the aliased region), and GetTxByOffset correctly reads against encodeBlock's primary-key value. No blockers; the notes below are a test-coverage gap on the cached wrapper's miss path, a couple of nits, and the fact that no reviewer in this run could execute the tests.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes:
cursor-review.mdis empty — the Cursor pass produced no output. Codex reported no material issues but could not run tests (Go 1.25.6 download needs network). I also could not invoke the Go toolchain in this environment, so compilation and the four new/expanded test files are unverified locally; rely on CI (go test -race+ golangci-lint v2.8.0) being green before merge. (*blockDB).GetTxByOffsethas no non-test caller and is unreachable outsidelittblock(unexported receiver,NewBlockDBreturnstypes.BlockDB). The PR description acknowledges this and defers the exported access point to the producer PR — fine to land, but it is dead production code until then, so keep the follow-up close behind. (unusedis not enabled in.golangci.yml, so lint won't complain.)- The change is motivated entirely by I/O savings, but nothing in the PR measures them.
sei-db/db_engine/litt/benchmarkalready exists — a benchmark comparingGetvsGetSubrangeon a large uncompressed value would pin the win (and would surface thatvalueFile.readdoes a freshos.OpenFile+seek per call, so the saving is in bytes read, not syscalls). - For the follow-up producer: consider recording the serialization version alongside each
(offset, length), or asserting the stored value's version byte before trusting an offset. The doc argues correctly that an already-recorded pair can't shift, but ablockValuePrefixLenchange plus any in-place value rewrite would silently return shifted bytes rather than error. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| return subrangeOf(cached, offset, length) | ||
| } | ||
|
|
||
| value, exists, err = c.base.GetSubrange(key, offset, length) |
There was a problem hiding this comment.
[suggestion] This fall-through (and the exists == false return from the base table) is never exercised through the cached wrapper by the new tests. In TestGetSubrangeParity the cached builders create a 500-weight write cache that Put fills with the 19-byte value, and cachedTable.Flush only delegates to base.Flush() — nothing evicts — so every read in that test, before and after flush, is a write-cache hit. TestGetSubrangeServedFromCache deliberately tests hits only. Since DefaultTableConfig sets both cache sizes to 0, this miss path is precisely the production path through the wrapper. Worth adding a case with a zero/tiny-weight cache (or a key that was never written) so the base delegation and the not-found return are covered end-to-end.
|
|
||
| // The value starts at dataAddress.Offset(); the sub-range starts offset bytes further in. Both operands | ||
| // and their sum are bounded by the value file size (< 2^32), so the uint32 addition cannot overflow. | ||
| value, err := values.read(dataAddress.Offset()+offset, length) |
There was a problem hiding this comment.
[nit] The no-overflow argument in the comment holds for every case that matters, but not unconditionally: when length > 0 the bounds check above pins Offset()+offset+length <= Offset()+ValueSize() <= fileSize, so no wrap. The one case it doesn't cover is length == 0 with offset == ValueSize() and Offset()+ValueSize() == 2^32, where Offset()+offset wraps to 0 and the read is issued at file offset 0 — harmless, since a zero-length read returns an empty slice either way. Computing the start in uint64 (or short-circuiting length == 0) would make the invariant hold by construction instead of by argument.
| // GetTxByOffset reads a single transaction's raw bytes out of the block stored at GlobalBlockNumber n, | ||
| // without decoding the block. offset and length identify a byte range within the block's stored value — | ||
| // the exact bytes encodeBlock produced: [version:1][GlobalBlockNumber:8][proto(Block)] — typically the | ||
| // (offset, length) of one transaction recorded at write time. Offset is measured from the start of that stored value (i.e. it includes |
There was a problem hiding this comment.
[nit] This comment line is ~135 columns, well past the ~110-col wrapping the rest of this doc block (and file) uses. Rewrap.
There was a problem hiding this comment.
Beyond the inline capacity-aliasing finding, this run also examined three other candidates raised as suggestions on the thread and ruled them out: the uint32 offset-arithmetic overflow in ReadSubrange (unreachable given current segment size limits, so it can't silently return wrong bytes today), GetSubrange reads being folded into Get's metrics series (an observability nit, not a correctness bug), and GetTxByOffset skipping a block-serialization-version check (an (offset, length) pair is always recorded against the exact value bytes it indexes into and never rewritten, so a future version bump can't retroactively invalidate an already-persisted pair).
Extended reasoning...
This run's bug hunting independently investigated three candidate issues that had already been raised as suggestions/nits elsewhere on the PR thread (uint32 overflow in ReadSubrange's offset arithmetic, GetSubrange read metrics being folded into Get's series, and GetTxByOffset omitting a block-serialization-version check) and concluded none of them are real correctness bugs, for the reasons summarized in the message. This is offered as a record of what was checked, not as a signal to skip re-checking in a future pass. The one inline finding from this run (unbounded spare capacity on GetSubrange's pre-flush/cache-hit return slices) already speaks for itself.
There was a problem hiding this comment.
A well-scoped, carefully documented addition of GetSubrange through the litt read stack plus a GetTxByOffset block-store primitive; the cache-consult-never-populate invariant holds against every writer of those caches, bounds checks are consistently done in uint64, and every return path uses capped three-index slices. No correctness or security blockers found — only ordering, performance-caveat, and doc-drift notes.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
sei-db/db_engine/litt/README.md:118-133mirrors theTableinterface method set and is now stale — it does not listGetSubrange. Worth adding so the doc keeps matchingtable.go.GetTxByOffsetis currently unreachable outside thelittblockpackage (*blockDBis unexported andNewBlockDBreturnstypes.BlockDB), so it is production code exercised only by the in-package tests. The PR description acknowledges this and defers the exported access point to the producer PR; nothing breaks in CI (unusedis not among the enabled linters), but it does mean the primitive ships unvalidated by any real caller.- No benchmark backs the stated I/O win.
valueFile.readstill doesos.OpenFile+Seek+io.ReadFullper call, so a sub-range read saves bytes transferred but pays the same per-read syscall/open overhead as a fullRead. For small ranges (a single tx) that overhead may dominate. A quick benchmark comparingGet+ slice vsGetSubrangeon realistic block sizes would be worth having before the receipt-side producer is built on the assumption. - Test nit (
sei-db/ledger_db/block/littblock/litt_block_db_test.go:63): offsets are derived withbytes.Index, which returns the first occurrence. A tx whose bytes also appear earlier in the encoded payload (or a zero-length tx) would yield a range that is not the actual proto field, so the assertion could pass or fail for the wrong reason. It is deterministic under the fixed seed so it will not flake today, but assertingbytes.Count(...) == 1— or using the writer-side offsets once the producer exists — would be sturdier. - Cursor's review file (
cursor-review.md) is empty, so that pass produced no output; only Codex's single P3 note was available to merge (it is reflected inline below). - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // means the caller measured against the wrong frame — most likely the body instead of the whole stored | ||
| // value. Reject it rather than return plausible-looking but shifted bytes. A change to the value | ||
| // layout has to revisit this bound along with encodeBlock/decodeBlock. | ||
| if offset < blockValuePrefixLen { |
There was a problem hiding this comment.
[suggestion] Argument validation runs before the watermark check, so a below-watermark block asked for with an offset inside the prefix returns the offset error instead of types.ErrPruned. The doc comment above lists ErrPruned as the outcome for n below the watermark, and ReadBlockByNumber checks the watermark first — moving the watermark check ahead of the prefix check keeps the two consistent and makes retention state, rather than argument shape, the thing a caller observes first.
(Also raised by Codex as P3.)
| return subrangeOf(cached, offset, length) | ||
| } | ||
|
|
||
| value, exists, err = c.base.GetSubrange(key, offset, length) |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
No new bugs found in this pass. The previously-flagged slice-capacity issue (unflushed/cached values returned without capping capacity, risking silent corruption via append) and the dangling design-doc reference are both fixed in the current code (three-index slices in disk_table.go, cached_table.go, and segment.go; the doc comment no longer points at a non-existent file). Given this touches a hot storage read path (caching, disk table, segment/compression fallback) and already went through several rounds of real correctness fixes, I'd still like a human to sign off rather than auto-approve.
Extended reasoning...
This PR adds GetSubrange to LittDB's Table interface and threads it through cachedTable, DiskTable, and Segment, plus a GetTxByOffset primitive on littblock's blockDB built on top of it. No security-sensitive surface (auth/crypto) is touched, but it is a storage hot-path change with subtle invariants: cache consult-never-populate semantics, slice aliasing/capacity across unflushed vs. flushed vs. compressed reads, and offset arithmetic bounds checks.
Across this review's history, several real issues were found and fixed: a slice-capacity bug where GetSubrange's unflushed/cache-hit paths returned two-index slices that could let a caller's append() silently corrupt shared LittDB-internal memory (fixed with three-index slices in disk_table.go, cached_table.go, and segment.go), and a dangling doc-comment reference to a design doc that doesn't exist (removed). I verified both fixes are present in the current tree.
Remaining open items are minor nits (metrics conflating subrange reads into the same series as full Get, a uint32-vs-uint64 arithmetic edge case in segment.go bounded by an already-unreachable overflow condition, and a couple of test-coverage gaps around the cache-miss fallthrough path) — none of these are correctness bugs in the shipped behavior.
Given the complexity of the read-path semantics and the fact that this already required multiple correction rounds to reach a safe state, I think it's worth a final human pass rather than a shadow approval, even though this run's bug-hunting pass found nothing new.
There was a problem hiding this comment.
GetSubrange is threaded cleanly through table → cachedTable → DiskTable → Segment with consistent bounds checks, uniform capped-slice returns on all four paths, and good test coverage (including the compressed fallback and per-segment compression mixing); I found no correctness or security blockers. Remaining notes are design/test-hardening suggestions, chiefly the offset-frame ambiguity in GetTxByOffset and the fact that the whole feature is currently unreachable from outside littblock.
Findings: 0 blocking | 10 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the reviewed diff";
cursor-review.mdis empty, so the Cursor pass produced no output for this PR. GetTxByOffsetis on the unexported*blockDBand not ontypes.BlockDB, andNewBlockDBreturns the interface — so the method (and therefore the wholeGetSubrangestack) has no production caller and is exercised only by tests. That's acknowledged in the PR description, but it means this API ships without a single real consumer; worth confirming the follow-up producer PR is actually queued, otherwise the exportedlitt.Tablesurface grows for nothing.- Nothing pins the performance contract that motivates the change: no test or benchmark asserts that a sub-range read scales with
lengthrather than the full value size. A benchmark comparingGetvsGetSubrangeon a multi-MB value would catch a future refactor that silently routes sub-range reads through a full read (e.g. someone "simplifying"Segment.ReadSubrangeto always callRead), which no existing test would notice since all of them only check byte equality. GetSubrangewas added directly to thelitt.Tableinterface, obliging every future implementation to provide it. OnlyDiskTableandcachedTableimplementlitt.Table/ManagedTabletoday (verified by grep over allGet(key []byte) (…[]byte, bool, error)signatures), so nothing breaks — but an optional type-asserted interface would keepTablenarrower for implementations that cannot slice values (the same argument the PR uses to keepGetTxByOffsetofftypes.BlockDB).- Worth calling out as done well: the aliasing hazard is handled uniformly with three-index capped slices on all four paths (unflushed, write cache, read cache, compressed fallback) and asserted in tests; and
Segment.ReadSubrangekeys the compressed/uncompressed decision off per-segmentIsCompressed(), which is correct for a table whoseCompressionsetting changed mid-life and left a mix of segment types. - Verification caveat:
go build/go testwere not runnable in this review environment, so all findings above are from static reading of the diff plus the surrounding read paths (DiskTable.Get,Segment.Read,valueFile.read,FIFOCache,PutBatch's unflushed-cache population), not from executing the new tests. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // means the caller measured against the wrong frame — most likely the body instead of the whole stored | ||
| // value. Reject it rather than return plausible-looking but shifted bytes. A change to the value | ||
| // layout has to revisit this bound along with encodeBlock/decodeBlock. | ||
| if offset < blockValuePrefixLen { |
There was a problem hiding this comment.
[suggestion] This guard only catches the frame mistake when the caller's offset happens to land in the first 9 bytes; a body-relative offset of, say, 40 is indistinguishable from a valid value-relative one and silently returns bytes shifted by blockValuePrefixLen. Since the producer naturally computes tx positions against the marshalled proto body (that's what it has in hand when it locates each tx), consider making the parameter body-relative and adding blockValuePrefixLen inside this method. That removes the whole class of confusion, drops the need for this heuristic, and also makes recorded offsets independent of the prefix layout — which is exactly the coupling the doc comment above (lines 448-451) has to hedge about.
| func (s *Segment) ReadSubrange(key []byte, dataAddress types.Address, offset uint32, length uint32) ([]byte, error) { | ||
| // A compressed value cannot be partially read from disk: decompress the whole thing, then slice. The | ||
| // bounds check is against the decompressed length, since dataAddress.ValueSize() is the compressed size. | ||
| if s.IsCompressed() { |
There was a problem hiding this comment.
[suggestion] The compressed fallback is correct, but note the cost profile for the intended use case: reading N transactions out of one block means N full reads + N decompressions of the same value, i.e. strictly worse than one Get plus N slices. The interface doc says "without the I/O savings", which undersells it. Worth stating in the litt.Table.GetSubrange doc that a caller extracting multiple ranges from the same value on a compressed table should use Get and slice instead — or add a GetSubranges batch entry point when the receipt-side producer lands.
| // genBlockWithTxs returns a random block guaranteed to carry at least one transaction, so the | ||
| // tx-extraction assertions below are meaningful. | ||
| func genBlockWithTxs(rng utils.Rng) *types.Block { | ||
| for { |
There was a problem hiding this comment.
[nit] Unbounded retry loop. GenPayload uses utils.GenSlice, so a zero-tx payload is possible and the seeds here are fixed, but if GenSlice's distribution ever changes this hangs the test instead of failing it. Bound it (e.g. 100 attempts, then t.Fatal) — that needs a *testing.T parameter.
| // exactly what a writer would record. Extracting that range must return the transaction. The | ||
| // search is scoped to the payload because that is the only place a tx can legitimately live. | ||
| for _, tx := range blk.Payload().Txs() { | ||
| idx := bytes.Index(stored[blockValuePrefixLen:], tx) |
There was a problem hiding this comment.
[nit] bytes.Index can match a coincidental earlier occurrence of the tx bytes (txs here are 10 random bytes, so unlikely but not structurally excluded), in which case the assertion below still passes — the test reduces to "a byte-range read equals slicing the same range", which TestGetSubrange already covers. To actually pin the coordinate system the producer will use, derive the offset from the proto encoding (walk the length-delimited repeated bytes elements) rather than searching for the payload.
There was a problem hiding this comment.
No new bugs found this run. The latest commit addresses the concerns raised in earlier review passes (mine and others'): the pre-flush/cache-hit sub-range slices are now capacity-capped (three-index slices), GetTxByOffset checks the retention watermark before validating the offset (matching ReadBlockByNumber's ErrPruned-first behavior), and the dangling design-doc reference was removed from the doc comment.
That said, this still touches a production-critical read path (LittDB's cache/disktable/segment layers plus a new block-store primitive) with subtle correctness properties — cache/disk parity, slice aliasing, compressed-segment fallback, bounds checking — so I'd like a human to look it over rather than rubber-stamp it.
Extended reasoning...
Overview
This PR adds litt.Table.GetSubrange (a bounded byte-range read) and threads it through cachedTable, DiskTable, and Segment, then builds littblock.(*blockDB).GetTxByOffset on top of it to extract a transaction's bytes from a stored block without a full decode. It's a read-path optimization, not a schema or wire-format change.
Security risks
No auth/crypto/permission surface. The main risk class is memory-safety-adjacent: returned slices alias internal LittDB buffers (unflushed values, cache entries), so an incorrect capacity bound could let an append by a caller corrupt shared state. That specific issue (spare capacity on the pre-flush/cache-hit paths) was raised in an earlier pass and is now fixed with three-index slices in both disk_table.go and cached_table.go.
Level of scrutiny
This warrants more than a mechanical-change level of review: it's new code on a hot read path in a component (LittDB) that backs block/QC storage, with several interacting correctness properties — cache-vs-disk parity, compressed vs. uncompressed fallback behavior, bounds checking against the right length in each case, and the watermark-vs-argument-validation ordering in GetTxByOffset. The test suite is thorough (parity tests across cached/uncached and compressed/uncompressed configurations, cache-hit-only and cache-disabled variants), which lowers risk, but the surface area is large enough that I'd still prefer a human sign-off.
Other factors
Two of my own prior inline findings (capacity capping, dangling doc reference to a nonexistent design doc) and one of seidroid's (watermark check ordering in GetTxByOffset) have been addressed in the current commit. A handful of remaining seidroid nits (uint32 arithmetic in a practically-unreachable overflow case, subrange reads sharing Get's metrics series, some test-robustness suggestions) are minor and don't block review on their own.
There was a problem hiding this comment.
Clean, well-documented addition of a sub-range read primitive through the LittDB read stack (interface → cachedTable → DiskTable → Segment) plus a littblock consumer; I found no correctness or security blockers — the cache consult-never-populate policy, the capped three-index slices, the compressed-segment fallback, and the watermark semantics all check out and are faithfully mirrored from the existing Get path. Remaining notes are hardening/naming/benchmarking suggestions, plus the fact that Cursor's review file was empty.
Findings: 0 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the PR diff.";
cursor-review.mdis empty, so the Cursor pass produced no output and contributed nothing to this synthesis. - Nothing in production can reach the new code yet, on either end:
GetTxByOffsetis a method on the unexported*blockDBwhileNewBlockDBreturnstypes.BlockDB, and the cachedTable consult path is inert becauseDefaultTableConfigsetsWriteCacheSize/ReadCacheSizeto 0. Both are acknowledged in the PR description; worth keeping in mind that the whole stack ships exercised only by tests until the follow-up producer lands. - No benchmark accompanies the claimed I/O win. Since the follow-up (per-tx
(offset, length)inReceiptRecord) is justified by that win, aGetSubrangevsGetbenchmark over realistic block-sized values would be worth adding here so the follow-up rests on measured numbers rather than the reasoning in the doc comments. - Minor test-coverage gaps: the compressed fallback is only exercised on the single-shard mem-keymap builder (
buildCompressedMemKeyDiskTable), not the multi-shard configurations thatTestGetSubrangecovers for the uncompressed path; andTestGetTxByOffset/TestGetTxByOffsetPrunedomitt.Parallel()while every other test added in this PR uses it. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| "the stored value, not of the proto body", offset, blockValuePrefixLen) | ||
| } | ||
|
|
||
| value, exists, err := s.table.GetSubrange(blockKey(n), offset, length) |
There was a problem hiding this comment.
[suggestion] Unlike ReadBlockByNumber (which goes through decodeBlock and validates value[0] == blockSerializationVersion and recovers the stored GlobalBlockNumber), this path performs no integrity check on the value it slices — it returns whatever bytes sit at a caller-supplied offset and the caller treats them as a transaction. The offset < blockValuePrefixLen guard above only catches a body-relative offset in [0, 9); a body-relative offset of, say, 40 silently yields bytes shifted by 9, and a stored value written under a future blockSerializationVersion with a different prefix width would do the same.
A cheap defense that preserves the point of the primitive: read the 9-byte prefix (GetSubrange(blockKey(n), 0, blockValuePrefixLen)) and assert the version byte and that the encoded block number equals n before returning the range — one extra bounded read, no unmarshal. Worth doing before the receipt-side producer starts feeding it recorded offsets, since a mismatch there fails silently rather than loudly.
| // - None with a nil error: no block is present at n (never written, or not yet written). | ||
| // - a non-nil error: offset lands inside the fixed prefix, the range is out of bounds for the block | ||
| // value, or the read failed. | ||
| func (s *blockDB) GetTxByOffset( |
There was a problem hiding this comment.
[suggestion] The name promises more than the method delivers: it returns an arbitrary byte range with no transaction framing validation, and the PR description already anticipates using it to read a receipt's fixed-position index field. Now — while there are no callers outside this package — is the cheapest moment to rename it to something layout-accurate like GetBlockValueSubrange, leaving "tx" to whatever wrapper the follow-up adds on top.
| // means the caller measured against the wrong frame — most likely the body instead of the whole stored | ||
| // value. Reject it rather than return plausible-looking but shifted bytes. A change to the value | ||
| // layout has to revisit this bound along with encodeBlock/decodeBlock. | ||
| if offset < blockValuePrefixLen { |
There was a problem hiding this comment.
[nit] length == 0 is accepted here and returns Some(empty slice). No transaction (or receipt index field) has zero length, so this is the same class of caller mistake the prefix check above is designed to reject loudly. Consider rejecting length == 0 too, or documenting it explicitly in the result list above alongside the other error cases.
|
|
||
| // The value starts at dataAddress.Offset(); the sub-range starts offset bytes further in. Both operands | ||
| // and their sum are bounded by the value file size (< 2^32), so the uint32 addition cannot overflow. | ||
| value, err := values.read(dataAddress.Offset()+offset, length) |
There was a problem hiding this comment.
[nit] valueFile.read opens the value file (os.OpenFile), seeks, ReadFulls, and closes on every call, so a sub-range read saves bytes transferred but still pays the same per-read open/seek/close as a full Read. For the target case (one tx out of a block) the fixed syscall cost may well dominate the savings, which makes the benchmark suggested elsewhere in this review worth having before the receipt producer is built on the assumption that "cost scales with length" — as written, only the transfer component does.
| // GetTxByOffset reads a single transaction's raw bytes out of the block stored at GlobalBlockNumber n, | ||
| // without decoding the block. offset and length identify a byte range within the block's stored value — | ||
| // the exact bytes encodeBlock produced: [version:1][GlobalBlockNumber:8][proto(Block)] — typically the | ||
| // (offset, length) of one transaction recorded at write time. Offset is measured from the start of that stored value (i.e. it includes |
There was a problem hiding this comment.
[nit] This line runs well past the ~110-column wrap the rest of this doc comment (and the file) keeps to — easy reflow.
There was a problem hiding this comment.
A well-tested, additive read primitive: GetSubrange is threaded consistently through litt.Table, cachedTable, DiskTable, and Segment, offset semantics agree across the unflushed / cache / disk / compressed paths, bounds and overflow checks are correct, and the tests cover the interesting edges. No blocking issues; only maintainability/perf-validation notes.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex explicitly reported no material issues. - No benchmark accompanies a performance-motivated primitive.
valueFile.readstill does a fullos.OpenFile+Seekper call, so the win is purely in bytes transferred; for modest block sizes the syscall overhead may dominate. AGetvsGetSubrangebenchmark on a realistic block value would substantiate the claim in the PR description before the follow-up producer PR builds on it. GetTxByOffsetis currently unreachable from outsidelittblock(unexported receiver,NewBlockDBreturnstypes.BlockDB), so it is production-dead code exercised only by tests until the follow-up lands. This is acknowledged in the PR description and is acceptable, but worth confirming the follow-up is actually queued so it doesn't linger.- Consider extending whatever concurrent/GC-stress coverage exists for
Getto also issueGetSubrangecalls: it reserves segments on the samecontrolLoop.getReservedSegmentpath, and the new tests are all single-threaded and quiescent (no read racing a flush, segment rotation, or GC). - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| return utils.None[[]byte](), fmt.Errorf( | ||
| "tx offset %d is too large to address within a block value", offset) | ||
| } | ||
| valueOffset := offset + blockValuePrefixLen |
There was a problem hiding this comment.
[suggestion] blockValuePrefixLen is applied unconditionally, so this is only correct while blockSerializationVersion == 1. The doc comment above correctly identifies this as the place a future variable-width prefix would have to be resolved, but nothing fails loudly if that day comes — decodeBlock rejects an unrecognized version byte, whereas this path would silently return bytes shifted by the prefix delta (i.e. a wrong-but-plausible transaction). Since the returned bytes will feed a receipt/tx lookup, silent misalignment is the worst failure mode here. Cheapest guards, in rough order of cost: a compile-time/init assertion tied to blockSerializationVersion, or reading [0,1) and comparing before the ranged read.
|
|
||
| // subrangeOf slices a cached full value, applying the same bounds check (and reporting it the same way) | ||
| // as the base table, so a cache hit and a cache miss are indistinguishable to the caller. | ||
| func subrangeOf(value []byte, offset uint32, length uint32) ([]byte, bool, error) { |
There was a problem hiding this comment.
[nit] This bounds check + capped three-index slice is now duplicated verbatim in three places (here, DiskTable.GetSubrange, and the compressed branch of Segment.ReadSubrange), including the identical error string. The parity property TestGetSubrangeParity pins depends on all three staying byte-identical, so extracting one exported helper (e.g. in litt/util) would make drift impossible rather than merely tested-against.
| // Caching sub-ranges themselves, under specially structured keys, would speed up misses as well, but that | ||
| // adds complexity we do not yet know we need. Revisit if profiling shows subrange misses are hot enough to | ||
| // justify it. | ||
| func (c *cachedTable) GetSubrange(key []byte, offset uint32, length uint32) (value []byte, exists bool, err error) { |
There was a problem hiding this comment.
[suggestion] The consult-but-never-populate policy is well argued for safety, but note the perf consequence for the motivating workload: extracting N transactions from one block on a cache-enabled table costs N keymap lookups + N file opens, where a single Get + N in-memory slices would cost one. So GetSubrange is pessimal exactly where the caller reads several ranges from the same value. Moot today (DefaultTableConfig sets both cache sizes to 0), and the doc already flags subrange-keyed caching as future work — but worth a line in that comment saying multi-range callers should prefer one Get while the caches can't help them, so the follow-up producer doesn't loop this per tx.
There was a problem hiding this comment.
Well-scoped, thoroughly documented and well-tested addition of a sub-range read primitive threaded cleanly through litt.Table → cachedTable → DiskTable → Segment, plus a block-store wrapper. I found no correctness or security blockers; the remaining points are polish, forward-compat hardening, and test hygiene.
Findings: 0 blocking | 10 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only Claude's and Codex's findings. Codex reported "no material issues found." - The same bounds-check + capped-slice logic is now written three times with three separately-maintained copies of the identical error string:
dbcache.subrangeOf,DiskTable.GetSubrange's unflushed branch (disk_table.go:993-1004), andSegment.ReadSubrange's compressed branch (segment.go:734-745). Consider one exported helper (e.g. inlitt/utilorlitt/types) so the semantics — and the[offset:end:end]capping in particular — cannot drift apart later. ReadBlockSubrangeis currently unreachable from outsidelittblock(*blockDBis unexported andNewBlockDBreturnstypes.BlockDB), so it is dead code apart from tests until the follow-up producer PR lands. The PR description acknowledges this; worth confirming the follow-up is tracked so this doesn't sit unused.- No benchmark accompanies the optimization.
valueFile.readdoes a freshos.OpenFile+Seek+ReadFull+Closeon every call, so for the small ranges this is aimed at (a single tx) the syscall overhead likely dominates and the win is bytes-transferred only. SinceDefaultTableConfigdisables both caches andGetSubrangedeliberately never populates the read cache, every sub-range read pays that full syscall path. A microbenchmark comparingGet+ slice vsGetSubrangeon a realistic block would validate the premise before the receipt-side producer is built on top of it. README.md:161says "Get,Exists,KeyCount, and TTL all treat secondary keys identically to primary keys" —GetSubrangenow does too (anddisk_table_subrange_test.go/TestGetSubrangeParityassert it), so it belongs in that sentence alongside the interface-listing line the PR already updated.- Test coverage gap: no case exercises
cachedTablewrapping a compressed base table.TestGetSubrangeCompressedcovers the compressed segment via the bareDiskTable, andTestGetSubrangeParitycovers the cached wrapper over uncompressed tables, but the combination (cache holds the plaintext, base must decompress-then-slice) is untested. Low risk since both halves are covered, but it's the one path where the cached and base frames could diverge unnoticed. - Minor style inconsistency across the new comments: "subrange", "sub-range", and "sub range" are used interchangeably (e.g.
table.go:96"subrange read" vstable.go:81"[offset, offset+length) byte range" vscached_table.go:83"sub-range"). Picking one spelling would make the new docs easier to grep. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| return utils.None[[]byte](), fmt.Errorf( | ||
| "tx offset %d is too large to address within a block value", offset) | ||
| } | ||
| valueOffset := offset + blockValuePrefixLen |
There was a problem hiding this comment.
[suggestion] This adds blockValuePrefixLen without ever reading the stored version byte, so the method silently assumes every stored block is blockSerializationVersion. decodeBlock (codec.go:96) does reject an unexpected version, so ReadBlockByNumber fails loudly on a version mismatch while ReadBlockSubrange would quietly return bytes shifted by the prefix-width delta — the worst failure mode for a primitive whose whole job is returning uninterpreted bytes.
The doc comment above already names this ("Should the prefix width ever vary by block version, this method is where the block's version would have to be resolved"), and TestBlockValueFraming pins the layout for version 1, so this is forward-compat hardening rather than a live bug. Still, a cheap guard would make the assumption enforceable rather than aspirational — e.g. a switch blockSerializationVersion { case 1: default: panic("prefix width must be resolved per version") } style compile-time-ish assertion, or a // note at the blockSerializationVersion declaration in codec.go pointing back here as a second site to update. That way bumping the version can't leave this arithmetic stale without something failing.
|
|
||
| // subrangeOf slices a cached full value, applying the same bounds check (and reporting it the same way) | ||
| // as the base table, so a cache hit and a cache miss are indistinguishable to the caller. | ||
| func subrangeOf(value []byte, offset uint32, length uint32) ([]byte, bool, error) { |
There was a problem hiding this comment.
[nit] Two small things here:
-
The out-of-bounds error from this helper is returned to the caller unwrapped (line 155/160:
return subrangeOf(...)), while a base-table error gets wrapped with"failed to get subrange from base table"(line 165). Since the identical error text is also produced byDiskTable.GetSubrangeandSegment.ReadSubrange, an operator reading a log line can't tell which of the three layers rejected the range. Wrapping the cache-hit path with something like"cached value:"would disambiguate at no cost. -
The doc contract in
table.go:104promises a zero-length range "returns an empty (non-nil) slice when the key exists". That holds for every production path (PutBatchrejects nil values atdisk_table.go:1049, and the disk path allocates viamake), but this helper would returnnilfor anilcached value — which is exactly the shapeTestGetSubrangeServedFromCacheconstructs by seeding the cache directly. Not reachable throughPuttoday; noting it in case the seeding pattern gets reused.
| // genBlockWithTxs returns a random block guaranteed to carry at least one transaction, so the | ||
| // tx-extraction assertions below are meaningful. | ||
| func genBlockWithTxs(rng utils.Rng) *types.Block { | ||
| for { |
There was a problem hiding this comment.
[nit] Unbounded retry loop: if types.GenBlock ever changes to produce tx-less blocks for this seed (or always), the test hangs until the Go test timeout rather than failing with a useful message. Bounding it — e.g. for i := 0; i < 100; i++ { ... } followed by t.Fatal("GenBlock produced no block with transactions") — turns a 10-minute CI timeout into an immediate, readable failure.
Separately, bytes.Index(body, tx) in TestReadBlockSubrange can locate a coincidental earlier match for a short tx (and returns 0 for an empty tx). The assertion still passes in that case because it compares against the same bytes, so the test remains valid — just slightly weaker than "the tx is at the offset a writer would record". Worth a require.NotEmpty(t, tx) if you want to rule out the degenerate case.
There was a problem hiding this comment.
I didn't find any new issues in this pass, but this PR adds a new read primitive threaded through every layer of LittDB's read stack (cachedTable, DiskTable, Segment) plus a new littblock method built on top of it, so it's worth a human look before merging.
Reviewed the offset/length bounds checks and capacity-capping (three-index slices) across the cache, disktable, and segment layers; the ReadBlockSubrange prefix-overflow guard and watermark-before-argument-validation ordering; and the compressed-segment fallback path. Two issues I raised on earlier revisions (a dangling doc reference and a capacity/append hazard on the unflushed and cached-value paths) have since been fixed in the current diff.
Extended reasoning...
Overview
This PR adds litt.Table.GetSubrange (a bounded byte-range read) threaded through cachedTable, DiskTable, and Segment, plus littblock.(*blockDB).ReadBlockSubrange, a block-store primitive built on top of it. It touches core read paths of the LittDB storage engine (cache consultation policy, disk/segment I/O, compressed-segment fallback) and introduces a new offset/length contract that several other subsystems (a planned receipt-side producer) will depend on.
Security risks
No auth/crypto/permissions surface. The main correctness risks are memory-safety-adjacent: returning slices that alias internal buffers (unflushed values, cache entries) without capacity capping, which could let a caller's append corrupt shared state. This was actually flagged in an earlier revision (by me) and has since been fixed with three-index slices in both disk_table.go and cached_table.go.
Level of scrutiny
This warrants a higher level of scrutiny than a typical PR: it's a new read primitive in a storage engine used for ledger data, threaded through multiple layers with subtle invariants (cache-consult-never-populate, compressed vs. uncompressed fallback cost/correctness, prefix-offset arithmetic with overflow checks). The PR has already been through multiple rounds of detailed review (from both this system and a bot reviewer) with several rounds of fixes applied, which is a good sign, but the sheer surface area and number of still-open design suggestions (naming, metrics separation, benchmark validation of the "cost scales with length" claim, code duplication of the bounds-check logic across three files) make this a case where a human maintainer's sign-off adds real value beyond what's been automatically checked.
Other factors
No bugs were found by the bug-hunting system in this run. Test coverage is extensive (dedicated parity, cache-hit, and compressed-fallback tests across every table implementation). The PR description's "Not in scope" section appropriately defers the receipt-side producer wiring, keeping this PR's blast radius to unused-but-tested primitives (no in-tree caller outside littblock yet).
2 participants
Summary
Adds a sub-range read primitive to LittDB and a block-store primitive built on top of it, so a single transaction can eventually be read out of a stored block (or a receipt's fixed-position index field) without paying for a full-value read + full protobuf decode.
litt.Table.GetSubrange(key, offset, length)— reads only[offset, offset+length)of a value instead of the whole thing.littblock.(*blockDB).ReadBlockSubrange(n, offset, length)— reads a byte range out of a stored block's marshalled body viaGetSubrange, skipping the full-block disk read and protobuf unmarshal.GetSubrangeimplementationThreaded through every layer of the read stack:
litt.Table(interface)GetSubrangemethod, fully documented (semantics, bounds, caching note).dbcache.cachedTableGetdoes, slicing the requested range out of a cached value; falls through to the base table on a miss. Never populates either cache (rationale below).disktable.DiskTablesegment.Segment.ReadSubrangevalue.Offset()+offsetand reads exactlylengthbytes (cost scales withlength, not the full value). For a compressed segment, a compressed blob can't be sliced on disk, so it falls back toRead(full read + decompress) and slices the plaintext — still correct, just without the I/O win.Cache policy: consult, never populate
Geta partial value it is entitled to treat as the whole thing.Putstoresvalue[sk.Offset:sk.Offset+sk.Length]under a secondary key, the base table addresses that secondary at the same aliased region on disk, and the read cache holds whateverGetreturned. A hit therefore slices exactly the bytes the base table would have read.GetSubrangeslower than plainGetfor precisely the hot, recently-written values a sub-range read is meant to serve.Because hits come from the same caches
Getuses, the two agree on whether a key exists — including for a key already reclaimed from the base table by GC or TTL expiry but still cache-resident.Returned slices are capacity-capped
A sub-range may alias a buffer holding the whole value — the live unflushed entry, or a cache entry shared by every later reader. Every path therefore returns a three-index slice, so
cap == lenand anappendby the caller allocates instead of overwriting whatever follows the range in that shared buffer. "NOT safe to mutate" alone didn't coverappend, so the guarantee is now stated in thelitt.Tablecontract and asserted in the tests.Caching sub-ranges themselves, under specially structured keys, would speed up misses as well, but that adds complexity we don't yet know we need; it's documented as a revisit-if-profiling-says-so. Worth noting that every table currently runs with caching disabled (
DefaultTableConfigsets both cache sizes to 0), so cache consultation only starts paying off once a caller enables a cache on the table it reads.ReadBlockSubrangeAdded as a method on littblock's concrete
*blockDB(alongsideReadBlockByNumber,Close, etc.) rather than on thetypes.BlockDBinterface, since the offset space is defined by this implementation's serialization ([version:1][GlobalBlockNumber:8][proto(Block)]) and wouldn't be portable tomemblock(which holds blocks by pointer with no byte layout). It mirrorsReadBlockByNumber's watermark/pruning semantics (types.ErrPrunedbelow the watermark,Nonefor an absent block), and the watermark is checked before argument validation so retention state is what a caller observes for a pruned block either way.Offsets are body-relative.
offsetis measured from the start of the marshalledproto(Block)body, and the method adds the fixed prefix itself. That is the frame a writer naturally computes in — it locates each transaction inside the marshalled body — so there is no conversion step to forget, and the framing stays a private detail oflittblockinstead of being baked into every offset persisted in another subsystem. The prefix addition is overflow-checked: an offset near the top of theuint32range would otherwise wrap and quietly return bytes from the start of the block.It returns bytes, not transactions. The range is handed back verbatim with no framing validation, so the name says what it does; a tx-shaped wrapper belongs with whatever layer holds recorded transaction locations.
Testing
disktable/disk_table_subrange_test.go— table-driven across all four disk-table configurations (mem/pebble keymap × single/multi shard), before and after flush: full range, assorted sub-ranges, zero-length reads, composition with secondary keys, out-of-bounds errors, missing-key handling.test/table_test.go— exercises thelitt.Tablesurface including the cached wrappers:TestGetSubrangeParitypins that a sub-range read is indistinguishable between the cached and the base path — primary key, secondary key (offsets relative to its own aliased region), zero-length range, out-of-bounds against both the full value and the shorter alias, missing key — and that a sub-range read never poisons a later fullGet. It also runs against a cache-disabled wrapper (zero-max-weight caches, i.e. theDefaultTableConfigproduction shape), because the existing cached builders keep every written value resident and so would only ever exercise hits, leaving the wrapper's fall-through to the base table untested.TestGetSubrangeServedFromCacheproves the caches are genuinely consulted, by seeding a cache entry for a key the base table has never seen so that only a cache hit can answer it. Also covers the reclaimed-but-still-cached case, whereGetSubrangeandGetmust agree that the key exists, and asserts that appending to a returned range leaves the cached value untouched.disktable/compression_test.go(TestGetSubrangeCompressed) — proves the compressed fallback path is correct for both a compressible (S2-tagged) and an incompressible (CompressionNone-tagged) value on the same compressed segment.littblock/litt_block_db_test.go— round-trips a block's whole body, extracts real transactions by locating their range within the body, and verifies the overflow guard, out-of-bounds ranges, absent blocks, andErrPrunedbelow the watermark.Not in scope
Wiring the receipt-side producer — computing per-tx
(offset, length)duringWriteBlockand threading it intoReceiptRecord— is left for a follow-up. That work also depends on resolving the open block-numbering question (EVM height vs.GlobalBlockNumber).Since
*blockDBis unexported andNewBlockDBreturnstypes.BlockDB,ReadBlockSubrangeis currently reachable only from within thelittblockpackage; the follow-up producer PR is where the exported access point (an interface to type-assert for, or a package-level helper) belongs.