Add codecs for ReceiptDB with txIndex#3799
Conversation
PR SummaryMedium Risk Overview New On-disk format change for the litt receipt table only (pebble/legacy KV unchanged). Pre-prefix receipts fail decode until they age out under retention; offset/length are plumbed but typically zero until block-store wiring lands. Reviewed by Cursor Bugbot for commit e8ff39b. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2120e9c. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a versioned metadata prefix to littidx receipt values with a clean, well-tested codec; all litt read/write paths are updated consistently and the change is internally sound. No blockers — a couple of non-blocking notes about currently-unused fields and format-compatibility confirmation.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Backward-compat (Codex P1, downgraded): decodeReceiptData accepts only the v1 (0x01) prefix, so any receipt written by the pre-PR littidx code as raw protobuf would fail to decode. This is not a blocker because littidx is opt-in (default backend is pebble) and the entire litt store landed in #3707 — this PR's base — so it has never been in a tagged release and holds no production data; the version byte also produces a clean 'unknown version' error (old protobuf starts 0x08, not 0x01) rather than silent corruption. Worth a one-line confirmation that no early-adopter/test-node littidx data needs to survive the format change.
- TxOffset/TxLength are added to ReceiptRecord and written into every receipt's metadata prefix, but nothing in the repo populates them yet, so they are always zero. The 17-byte prefix is therefore currently pure storage/CPU overhead per receipt with no functional benefit until a writer sets these fields — acceptable as incremental plumbing, but confirm the follow-up that wires them is planned.
- Cursor's second-opinion pass (cursor-review.md) produced no output.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // into the receipt value's metadata prefix so a receipt lookup can find the | ||
| // transaction bytes. Only meaningful when block compression is off; zero when | ||
| // unknown. | ||
| TxOffset uint32 |
There was a problem hiding this comment.
[nit] TxOffset/TxLength are plumbed through into the receipt metadata prefix, but no caller populates them anywhere in the repo yet, so they are always encoded as zero. Fine as forward-looking plumbing — just flagging that the prefix adds ~17 bytes per receipt with no functional payoff until a writer sets these; confirm the follow-up wiring is planned.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3799 +/- ##
==========================================
- Coverage 60.16% 59.21% -0.96%
==========================================
Files 2327 2236 -91
Lines 194614 184141 -10473
==========================================
- Hits 117089 109035 -8054
+ Misses 66951 65384 -1567
+ Partials 10574 9722 -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}
There was a problem hiding this comment.
Looks good — self-contained codec change with solid test coverage.
Extended reasoning...
Overview
Adds a versioned metadata prefix (receiptData v1: version + blockNumber + txOffset + txLength + body) to receipt values stored in the LittDB-backed littidx receipt store, and threads new TxOffset/TxLength fields through ReceiptRecord into the encoded value. GetReceiptFromStore and candidateBlockLogs were updated to decode the wrapper and unmarshal only the body.
Security risks
None identified. This is an internal on-disk encoding change for an auxiliary, non-consensus RPC data store (receipts/logs), not user-facing input parsing, auth, or crypto.
Level of scrutiny
Light-to-moderate is appropriate. The littidx receipt backend is opt-in — the default ReceiptStoreConfig.Backend is pebbledb — so this format change doesn't affect the default path. The new codec is small, self-contained, and covered by round-trip, wire-layout-pinning, and error-path tests in codec_test.go. The only current caller of ReceiptRecord (x/evm/keeper/receipt.go) doesn't yet populate TxOffset/TxLength, consistent with the doc comment that they're "zero when unknown" — this looks like infrastructure landing ahead of the code that will populate it, which is a normal sequencing pattern in this repo.
Other factors
The bug-hunting system found no bugs. A candidate concern about breaking decode of pre-existing litt receipt values (no migration) was raised and ruled out — reasonable given this backend isn't the default and there's no indication of production data in this format yet. The offset variable was also sensibly renamed (offset → partOffset) to avoid confusion with the new TxOffset field.
There was a problem hiding this comment.
A clean, well-tested versioned-codec refactor for LittDB receipts, but the new decoder rejects every pre-PR receipt value (raw protobuf, no version prefix) with a hard error, causing eth_getTransactionReceipt / eth_getLogs to fail over any retained pre-upgrade data until it ages out. The PR acknowledges the format change but the error-propagation behavior is worse than a graceful age-out.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (cursor-review.md) produced no output; only Codex's review was available for cross-referencing.
- Consider a follow-up test that seeds a legacy raw-protobuf value into the litt table and asserts the desired read behavior (fallback / not-found vs. error), so the chosen compatibility strategy is locked in.
- TxOffset/TxLength are plumbed through ReceiptRecord and the codec but always written as 0 in this PR (populated in a follow-up). This is fine and matches the PR description, but the wire format now permanently reserves 8 bytes per receipt that carry no information yet — acceptable given the stated design-doc plan.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if !exists { | ||
| return nil, ErrNotFound | ||
| } | ||
| data, err := decodeReceiptData(val) |
There was a problem hiding this comment.
[suggestion] Backward-compatibility regression (confirms Codex's High finding, and it's worse than a graceful age-out). Pre-PR receipt values are raw protobufs with no version prefix. A valid protobuf's first byte can never be 0x01 (that encodes field number 0, which is invalid), so decodeReceiptData reliably falls into the default branch and returns "unknown receipt value version" for every legacy value.
Two consequences here:
GetReceiptFromStorereturns that error, and inGetReceiptit is notErrNotFound, so the code neither masks it as not-found nor falls back tolegacyReceiptFromKVStore— it propagates as a hard error toeth_getTransactionReceipt.- The decode happens before the
belowRetentionFloorcheck, so even legacy values that are already below the retention floor (previously masked as not-found) now surface as errors instead.
The PR description frames legacy values as simply "aging out within the KeepRecent/TTL window," but during that window reads error rather than degrade gracefully. Suggest adding the legacy-tolerant decode branch mentioned in the PR notes (treat a non-v1 prefix as a raw protobuf body with a zero TxHeader), or at minimum map an undecodable value to ErrNotFound so the KV fallback and retention masking still apply.
| if !exists { | ||
| continue | ||
| } | ||
| data, err := decodeReceiptData(bz) |
There was a problem hiding this comment.
[suggestion] Same legacy-decode issue on the eth_getLogs path: a single pre-PR (unprefixed) receipt value in the queried range makes decodeReceiptData error and fails the entire candidateBlockLogs call, so eth_getLogs returns an error for any range spanning pre-upgrade blocks until they age out. Whatever compatibility strategy is chosen at the GetReceiptFromStore decode site should be applied here too.
There was a problem hiding this comment.
Clean, well-documented additive codec with good unit tests, but the read path now hard-errors on any pre-existing litt receipt value, and the stated "they age out via TTL" mitigation doesn't hold under the default KeepRecent == 0 config (no TTL is set at all). A few-line legacy-tolerant decode branch resolves it; remaining notes are perf/naming/doc nits.
Findings: 2 blocking | 8 non-blocking | 5 posted inline
Blockers
- Cursor's second-opinion review file (
cursor-review.md) is empty — that pass produced no output, so this review reflects only Claude + Codex. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- PR description lists
docs/tx-location-index.mdanddocs/figures/as part of the change, but the diff contains only the 5 Go files (no docs are present on the branch). Either add the design doc or update the description. - Test coverage:
codec_test.gois thorough for the codec in isolation, but there is no store-level test that a record with non-zeroTxOffset/TxLengthsurvivesSetReceipts→ read (the header isn't surfaced by any read API yet), and no test pinning the behavior for a legacy raw-proto value in the litt table. Adding the latter would lock in whatever compatibility decision you make above. ReceiptRecord.TxOffset/TxLengthare on the shared record struct but only the littidx backend persists them; the pebble backend and legacy KV path silently drop them, and the only in-tree producer (x/evm/keeper/receipt.go:165) never sets them. Worth stating in the field comment so a future caller doesn't assume they round-trip on every backend.- Header duplicates
blockNumber, which already exists in the receipt body (proto field 7). That's justified for a future header-onlygetTxByHashread, but a short note incodec.gosaying so would preempt the "why store it twice" question. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| if !exists { | ||
| return nil, ErrNotFound | ||
| } | ||
| data, err := decodeReceiptData(val) |
There was a problem hiding this comment.
[blocker] Reads now unconditionally require the new version prefix, so every receipt already written to the litt table by a prior binary becomes unreadable — and it fails as a hard error, not a miss. Two consequences:
decodeReceiptDatareturnsunknown receipt value version 8(legacy values are raw protobuf, first byte0x08fortx_type). That error is notErrNotFound, soGetReceipt(line 216) skips the legacy-KV fallback andeth_getTransactionReceiptreturns an internal error instead of "not found".- The PR notes say stale-format values "age out within the
KeepRecent/TTL window", butKeepRecentdefaults to 0 (DefaultReceiptStoreConfig), andnewLittReceiptStoreonly callsSetTTLwhenKeepRecent > 0(lines 138-143). With the default/archive config there is no TTL at all, so those receipts never expire and stay permanently broken.
Since the version byte 1 can never collide with a protobuf tag byte, a legacy-tolerant branch in decodeReceiptData is a few lines and fully unambiguous — treat a non-receiptDataV1 first byte as receiptData{Header: TxHeader{}, Body: raw}. Codex flagged this as High too. If littidx has no existing on-disk data anywhere yet (it's opt-in, default is pebbledb, and no in-repo config enables it), this is waivable — but please say so explicitly rather than relying on the TTL argument, which doesn't hold under the default config.
| if !exists { | ||
| continue | ||
| } | ||
| data, err := decodeReceiptData(bz) |
There was a problem hiding this comment.
[suggestion] A single undecodable receipt value now aborts the entire eth_getLogs query for the whole block range, whereas the line above tolerates a missing value (if !exists { continue }). Combined with the legacy-format issue, one old receipt anywhere in the range makes every getLogs over that range fail. Consider logging and continue-ing on decode failure to match the missing-value handling.
| return err | ||
| } | ||
| offset := uint32(len(value)) //nolint:gosec // block regions fit within uint32 | ||
| bz := encodeReceiptData(receiptData{ |
There was a problem hiding this comment.
[suggestion] encodeReceiptData allocates a fresh buffer and copies the whole body for every receipt, on the per-block commit path, only to immediately append it into value and discard it. You can avoid the extra alloc + copy per tx by appending the 17-byte header directly into value and then appending body (e.g. an appendReceiptHeader(dst []byte, h TxHeader) []byte helper reused by encodeReceiptData). Also, value := make([]byte, 0) on line 305 could be preallocated now that the per-record size is known to grow by receiptDataV1HeaderLen.
| }, | ||
| Body: raw[receiptDataV1HeaderLen:], | ||
| }, nil | ||
| default: |
There was a problem hiding this comment.
[suggestion] This is the natural place for the legacy branch: because a raw marshaled Receipt always starts with a valid protobuf tag byte (0x08, 0x10, 0x1a, …) and never 0x01, default: can safely be interpreted as "pre-versioned value" and returned as receiptData{Body: raw} with a zero header. Worth a comment recording that invariant either way, since it's what makes the version dispatch unambiguous.
|
|
||
| // Field widths of the v1 metadata prefix. | ||
| versionLen = 1 // version byte | ||
| blockNumberLen = 8 // uint64 block number |
There was a problem hiding this comment.
[nit] blockNumberLen = 8 duplicates the existing package-level blockNumLen = 8 (tx_hash_index.go:45). Two near-identically named constants for the same width in one package invites picking the wrong one later — reuse blockNumLen or rename to something clearly codec-scoped (e.g. hdrBlockNumberLen).
2 participants
Summary
Adds a versioned metadata prefix to receipt values in the LittDB-backed
ReceiptStoreso that each stored receipt records where its transaction livesin the block store —
(blockNumber, txOffset, txLength). Since ReceiptDB isalready keyed by tx hash, this lays the groundwork for a cheap future
getTxByHash(direct pointer into BlockDB, no full-block decode, no separate txindex).
This PR is the foundational, strictly-additive step.
txOffset/txLengthareplumbed but currently written as
0; populating them from the block store andthe
getTxByHashread path are follow-ups (see design doc).Changes
receipt/codec.go— versioned receipt value codec:[version:1][blockNumber:8 BE][txOffset:4 BE][txLength:4 BE][receiptBody].decodeReceiptDatadispatches on the version byte; unknown/short/empty valueserror.
receipt_store.go—ReceiptRecordgainsTxOffset/TxLength(thetx's byte range within its block-store block value).
litt_receipt_store.go— write path wraps each receipt body withencodeReceiptData;GetReceiptFromStoredecodes then unmarshals the body.litt_tag_index.go— theeth_getLogsread path decodes beforeunmarshaling.
codec_test.go— round-trip, exact wire-layout pinning, and error-branchunit tests.
docs/tx-location-index.md+docs/figures/— design doc for the tx-indexwiring and future
getTxByHash, with diagrams.Notes / constraints
getReceiptByHashround-trips exactly asbefore; the prefix is transparent.
this change have no version prefix; they age out within the
KeepRecent/TTLwindow. (A legacy-tolerant decode branch can be added if a zero-downtime
cutover is needed.)
raw receipt protos.
sub-range secondary keys, unrelated to this metadata).
Test plan
go test ./sei-db/ledger_db/receipt/...(incl. newcodec_test.go)gofmt/goimports/go vetcleangetTxByHashtest once block-store wiring lands