add block number to ReadBlockByHash#3703
Conversation
PR SummaryMedium Risk Overview littblock persists blocks as Reviewed by Cursor Bugbot for commit 175dccb. 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 #3703 +/- ##
==========================================
- Coverage 59.30% 58.39% -0.91%
==========================================
Files 2273 2186 -87
Lines 188364 178775 -9589
==========================================
- Hits 111705 104399 -7306
+ Misses 66596 65112 -1484
+ Partials 10063 9264 -799
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.
Clean, well-tested change that adds a GlobalBlockNumber return value to ReadBlockByHash by embedding the number in a newly version-framed on-disk value; the signature change is propagated to both implementations and all (test-only) callers. No blockers, but a doc typo and a documented backward-incompatibility with pre-existing DBs are worth noting.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Backward compatibility: the new version-prefixed decoders reject any pre-existing unversioned LittDB values, and
recoverCursorscallsdecodeQCat startup (litt_block_db.go:119), so reopening a DB written by the previous code would fail to recover. The author documents this as intentional ("this store is not yet in production, so no prior format is supported") in littblock/codec.go, so this is an accepted design decision rather than a blocker — but if any dev/test stores exist they must be wiped on upgrade. (Codex flagged this as a blocker; downgraded here given the explicit documented non-production status.) - Cursor's second-opinion review produced no output (empty file).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // for the block that was passed to WriteBlock. | ||
| // | ||
| // This method returns the block number for the block if it is found. | ||
| // not found, the block number returned is undefined. |
There was a problem hiding this comment.
[nit] Doc typo: this sentence is missing its opening word. It should read "If not found, the block number returned is undefined." (currently "not found, the block number returned is undefined.").
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward API extension with proper versioned framing and thorough tests; the two nits are inline.
Extended reasoning...
Overview
This PR extends BlockDB.ReadBlockByHash to also return the block's GlobalBlockNumber. On the durable littblock side, block values are reframed as [version:1][number:8][proto] so a hash-alias read (which only carries the hash key) can recover the number; QC values also get a 1-byte version prefix. The in-memory memblock mirrors this via a new hashEntry struct. Interface docs, callers, and tests are updated in lock-step.
Security risks
None material. This touches only the block DB serialization layer — no auth, crypto, permissions, or user-facing input handling. The new decode paths validate length and reject unknown version bytes with explicit errors rather than partial values, and short-buffer rejection is covered by tests.
Level of scrutiny
Low. The change is mechanical and self-contained: an API extension plus a versioned framing bump for on-disk values. The littblock store is explicitly documented as pre-production ("no prior format is supported"), so the on-disk format change carries no migration risk. Tests cover round-trip with the embedded number, unknown-version rejection, garbage rejection, and hash-lookup correctness across both implementations and gapped writes.
Other factors
The two findings surfaced by the bug hunter are both pure documentation nits (a misleading version-constant comment and a missing If in a godoc sentence) with no runtime impact — worth fixing but not blocking. The QC-before-block invariant, watermark semantics, and iterator snapshot semantics are preserved. Shared test suite assertBlocksReadable now asserts hash-lookup returns the correct number for both impls, giving strong contract coverage.
| // diverge. Decode rejects any other version outright: this store is not yet in | ||
| // production, so no prior format is supported. | ||
| const blockSerializationVersion byte = 1 | ||
| const qcSerializationVersion byte = 1 |
There was a problem hiding this comment.
why do we need qcSerializationVersion to return a block number?
There was a problem hiding this comment.
We don't. I just added a 1 byte serialization version to the data schema since it's good practice to encode serialization versions (and it was originally missing). This way, if we ever decide to alter the serialization schema for a QC in the future, we will be able to do so with little headache.
| // is framed as [version:1][GlobalBlockNumber:8 big-endian][proto(Block)]. The | ||
| // number is embedded so a by-hash lookup — which reaches this same shared value | ||
| // through a secondary key that carries only the hash — can still recover it. | ||
| func encodeBlock(n types.GlobalBlockNumber, blk *types.Block) []byte { |
There was a problem hiding this comment.
I thought we didn't have to encode block number into the block because it was the key. Would this hurt the performance? I guess not.
There was a problem hiding this comment.
It is in the primary key, but it's not in the secondary key. LittDB keeps no map from secondary keys to primary keys, and so if we want this data when we get by block hash, it needs to go into the value data.
Technically yes, this does add some overhead. But in practical terms, this overhead is negligible. We're adding 8 bytes onto a 2 megabyte block (this is the size of the blocks Greg asked me to simulate in my benchmark).
There was a problem hiding this comment.
A focused, well-tested change that adds the block's GlobalBlockNumber to ReadBlockByHash; the interface and both implementations are updated consistently. The only point worth confirming is that the change alters the on-disk value format for both blocks and QCs (adding a version prefix), which would reject any pre-existing littblock data on upgrade — likely a non-issue since the store appears to be pre-production, but worth verifying.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- On-disk format compatibility (raised by Codex): decodeBlock/decodeQC now require a leading version byte, and the block value additionally embeds an 8-byte number, so the previous raw-protobuf values become undecodable after upgrade. This affects both blocks AND QCs, so recoverCursors (which decodes the newest persisted QC in NewBlockDB) would fail to open any store containing legacy values. I could not find littblock.NewBlockDB wired into any production path (only tests and blocksim), so this is most likely pre-production with no persisted data to migrate — please confirm; if any node has persisted littblock data, a migration or legacy decoder is required before merge.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
- The new GlobalBlockNumber return value of ReadBlockByHash is not yet consumed by any production caller (grep finds no non-test/non-impl callers); this is a forward-looking API addition, which is fine but worth noting.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if len(value) < blockValuePrefixLen { | ||
| return 0, nil, fmt.Errorf("block value too short: %d bytes", len(value)) | ||
| } | ||
| if value[0] != blockSerializationVersion { |
There was a problem hiding this comment.
[suggestion] This introduces a versioned on-disk value format ([version][number][proto] for blocks, and [version][proto] for QCs at line 122). Any block/QC value persisted by a prior build was raw protobuf with no version byte, so this check will reject it on upgrade — and since recoverCursors in NewBlockDB decodes the newest persisted QC, an existing store would fail to open. This looks safe only because littblock.NewBlockDB doesn't appear to be wired into any production path yet. Please confirm there is no persisted littblock data in the field; otherwise a migration or a legacy (unversioned) decode fallback is needed.
There was a problem hiding this comment.
no production use yet
There was a problem hiding this comment.
LGTM — small, well-tested API extension; prior doc nits are fixed.
Extended reasoning...
Overview
This PR adds a GlobalBlockNumber return value to BlockDB.ReadBlockByHash so callers can recover a block's number from a hash lookup. To support this on the durable littblock backend (where the by-hash secondary key does not map to the by-number primary key), the block value's on-disk format changes: it is now framed as [version:1][number:8 big-endian][proto(Block)]. QC values pick up a matching 1-byte version prefix as a schema-evolution hook (not functionally required for this PR). All three implementations (interface, litt, mem), their tests, and the block iterator's decode call site are updated in lockstep.
Security risks
None material. The storage format change is a breaking bytes-on-disk change, but the author confirmed on the thread that this store is not yet in production, so there is no migration concern. Decode does bounds- and version-check before reading, so malformed input surfaces an error rather than a panic or partial value.
Level of scrutiny
Low. This is a self-contained API/serialization tweak in a pre-production ledger store, with unit tests covering the round-trip, version rejection, and the memblock/littblock parity. The interface signature is threaded correctly to every caller (verified via grep — only 4 files reference ReadBlockByHash, all included in the PR).
Other factors
Both nits from my prior review — the missing If in the interface godoc, and the misleading "must never share a version number" comment on the two version constants — were addressed in the latest commit. Reviewer questions from wen-coding were answered by the author (QC version byte is defensive, per-block overhead is 8 bytes on a 2 MB block). No outstanding concerns.
|
|
||
| // blockValuePrefixLen is the fixed header preceding a block's proto bytes: one | ||
| // version byte followed by the 8-byte big-endian GlobalBlockNumber. | ||
| const blockValuePrefixLen = 1 + 8 |
There was a problem hiding this comment.
nit: since Block is already a protobuf, would it be easier for the version and blockNumber be in a protobuf wrap as well?
| rng := utils.TestRngFromSeed(1) | ||
| for range 16 { | ||
| for i := range 16 { | ||
| n := types.GlobalBlockNumber(i * 7) |
There was a problem hiding this comment.
nit: why i*7? in most cases we should have consecutive blocks.
| } | ||
|
|
||
| func getBlock(table littdb.Table, key []byte) (utils.Option[*types.Block], error) { | ||
| func getBlock(table littdb.Table, key []byte) (utils.Option[*types.Block], types.GlobalBlockNumber, error) { |
There was a problem hiding this comment.
nit: would it be more natural to return utils.Option[(*types.Block, types.GlobalBlockNumber)]
2 participants
Describe your changes and provide context
Add an additional return value to
ReadBlockByHash()that provides the block's number.Testing performed to validate your change
unit tests