Repository navigation
Receipt DB iteration - #4222
Receipt DB iteration#4222
Conversation
PR SummaryMedium Risk Overview littidx behavior is tightened for iteration: Configuration: Reviewed by Cursor Bugbot for commit 9561ac2. 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.
Reviewed by Cursor Bugbot for commit bbe7e38. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new IterateReceipts walk is a reasonable design, but adding it to the exported ReceiptStore interface breaks two evmrpc test fakes so ./evmrpc/... no longer builds, and the IteratorAt not-found fallback silently restarts the walk at the oldest key in the store, returning receipts below startBlock (and below the retention floor) on any store written by an earlier build.
Findings: 2 blocking | 2 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The
ReceiptIterator/IterateReceiptscontract omits two properties a caller depends on: litt iterators capture a snapshot of the keys present at creation (blocks written afterwards are never yielded, so a follower cannot hold one open and expect it to track the head), and the documented "ascending block order" holds only because litt iterates in insertion order and blocks are normally written in ascending order — a part appended for an older block after a newer one (the legacy-migration shape the store doc already calls out) would be yielded out of order. Worth stating both in the interface godoc. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4222 +/- ##
==========================================
- Coverage 67.01% 65.98% -1.04%
==========================================
Files 2179 2060 -119
Lines 167367 156660 -10707
==========================================
- Hits 112163 103370 -8793
+ Misses 55063 53149 -1914
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@seidroid review |
There was a problem hiding this comment.
The iterator design is sound and the previous round's two blockers (missing interface implementations, the not-found fallback walking below startBlock) are genuinely fixed by seekBlock. But the new write-side "no skipped block" invariant turns a legitimately lagging receipt head into a permanent, unrecoverable node panic loop on a path documented as must-not-error, and the iterator still never enforces its own lower bound.
Findings: 1 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The
ReceiptIteratorgodoc still omits the snapshot property it inherits from litt: the iterator captures the keys present at creation (litt.Table.Iterator, table.go:160), so blocks written after it is opened are never yielded. A caller that holds one open expecting it to track the head silently sees nothing. Worth stating onIterateReceiptsalongside the ordering guarantee. - [suggestion] No test exercises the property
seekBlockexists for — skipping receipts that sit belowstarton the whole-store fallback.TestReceiptIteratorStartsAtOldestStoredBlockreacheswalkFromOldest, but withstartBlock == 0there is nothing below it to wrongly yield, so the seek would pass even if it returned the first primary key unconditionally. The new write guard makes that store shape unreachable through the public API, so pinning it needs an internal test that writes the part keys directly (the package already has*_internal_test.gofiles). - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The iterator design is sound and the empty-part write makes positioning work, but the new "no skipped block" write guard is still a latching, pre-commit-fatal refusal on a configuration the PR only documents as Giga-only rather than enforcing, and the iterator still never enforces its own lower bound.
Findings: 1 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] No test exercises the property
seekBlockexists for — skipping part keys belowstarton thewalkFromOldestfallback.TestReceiptIteratorStartsAtOldestStoredBlockreaches that path withstartBlock == 0, so there is nothing below it to wrongly yield, andTestReceiptIteratorStartsAtRetentionFloorfinds the part key at the clamped floor and never falls through. The new write guard makes the fall-through store shape unreachable through the public API, so pinning it needs an internal test that writes part keys directly (the package already has*_internal_test.gofiles). - [suggestion]
writeReceiptsderives the blocks it records from each record's ownReceipt.BlockNumber, not fromheight, so the new "every block the store accepted has a key of its own" invariant only holds while the two agree — only the empty-batch path falls back toheight.flushTransientReceiptsexplicitly documents that the transient store can hold receipts for a block other than the current height (x/evm/keeper/receipt.go:146-150); such a write leavesheightunrecorded and the head short, and the next block then tripsrequireNoSkippedBlock. Recordingheightalongside the record-derived blocks, or checking that they agree, would make the guard measure what its doc claims. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The iterator design is sound, and refusing rs-backend = "littidx" from app.toml genuinely closes the previous round's blocker: the littidx store is now reachable only through GigaStorageManager, whose OpenDBWithRecovery converges every store onto min(block, state, receipt) before opening, so the fatal requireNoSkippedBlock refusal sits behind an invariant that is established by construction (evmonly's FinalizeBlock → giga_store.go writes every height in order, empty blocks included). Remaining findings are non-blocking: a stale rationale on the cryptosim write, an iterator contract still enforced only by write-ordering convention, an app.toml key that can no longer reach any store, and a missing CHANGELOG/compat note.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No test exercises the property
seekBlockexists for — skipping part keys belowstarton thewalkFromOldestfallback.TestReceiptIteratorStartsAtOldestStoredBlockreaches that path withstartBlock == 0(nothing below it to wrongly yield),TestReceiptIteratorStartsAtRetentionFloorfinds the part key at the clamped floor and never falls through, andTestReceiptIteratorStartsInAnEmptyStretchfinds the empty-block part key. The write guard makes the fall-through shape unreachable through the public API, so pinning the GC-race case thewalkFromOldestcomment describes needs an internal test that writes part keys directly (the package already has*_internal_test.gofiles). - [suggestion]
rs-backend = "littidx"was a previously accepted app.toml value (the old error advertisedsupported: pebbledb, littidx); it now failsReadReceiptConfig, andapp.go:653panics on that error, so a node carrying the key cannot start until app.toml is edited — and after switching to pebbledb its receipt history underdata/ledger/receipt/littidxis no longer served. That is the intended change, but there is no## UnreleasedCHANGELOG entry, even though the repo records exactly this class of config-compatibility change there (e.g. #4191'sapi.swaggerno-op, #4032's telemetry default). Worth an Improvements/Upgrade-guide line so the startup panic is not an operator's first notice. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| if len(records) > 0 { | ||
| sdkCtx := sdk.NewContext(nil, tmproto.Header{Height: int64(blockNumber)}, false) //nolint:gosec | ||
| // Every block is written, including one that produced no receipts: the store records each block | ||
| // it is given so a walk can position at any of them, and refuses a write that skips one. |
There was a problem hiding this comment.
[suggestion] This comment's rationale doesn't hold for this store. NewRecieptStoreSimulator opens the receipt store with Backend: "pebbledb" (same file, ~line 141), and the pebble backend has no part keys, no walk to position, and no skipped-block refusal — receiptStore.SetReceipts just applies the (here empty) changeset and stamps the version. Writing every block is still the right change, but for a different reason: production's flushTransientReceipts (x/evm/keeper/receipt.go:166) calls SetReceipts on every block whether or not it produced receipts, so the simulator now matches the production write cadence.
Worth noting the measurement effect too: RecordReceiptBlockWriteDuration and ReportReceiptsWritten(0) now fire for receipt-less blocks, so write-duration percentiles from this benchmark are no longer comparable with runs from before this change.
| } | ||
| continue | ||
| } | ||
| r.txHash = common.BytesToHash(key) |
There was a problem hiding this comment.
[suggestion] Next still never checks the lower bound, so "at or above startBlock, in ascending block order" holds only while parts are appended in ascending block order — a write-ordering convention, not something the iterator establishes. requireNoSkippedBlock doesn't establish it either: it rejects only blockNumber > head+1, so any write at or below the head is accepted (TestLittIdxAcceptsRepeatedBlock pins that). Write blocks 1, 2, 3 then a second part for block 1 and, because litt iterates in insertion order, IterateReceipts(2) yields block 2, block 3, then block 1's receipts — below startBlock, out of order, and when 2 is the retention floor it re-exposes receipts GetReceiptFromStore refuses via belowRetentionFloor.
Not a blocker this round: with littidx now refused from app.toml, the only writer is the Giga path, which executes heights in order, has no legacy-migration multi-part flush (recovery.go:87), and rolls the receipt tail back with PruneAfter before reopening — so no live path produces that shape. It's still worth putting the guarantee on the iterator rather than on the callers, since there is no caller yet to learn the convention from: keep start on receiptIterator and skip in Next while r.block < start, which covers both the IteratorAt and walkFromOldest entry paths.
| # Backend defines the receipt store backend. | ||
| # Supported backends: pebble (aka pebbledb) | ||
| # The littidx backend is opened by Giga through its own storage config and is | ||
| # refused here. |
There was a problem hiding this comment.
[suggestion] With littidx refused here, the log-filter-parallelism key further down this template (line ~218: "Applies only when rs-backend = littidx") can no longer reach any store that reads it: Giga builds its receipt config in DefaultGigaStorageConfig from DefaultReceiptStoreConfig() and never consults app.toml, and the pebble backend ignores the value. So this file now documents a tunable that is inert wherever it is set. Either adjust that key's comment to say receipt tuning for the Giga-opened store isn't read from app.toml, or wire Giga's ReceiptDBConfig from the app.toml-read config so the key keeps meaning.
Superseded: latest AI review found no blocking issues.
|
|
||
| # Backend defines the receipt store backend. | ||
| # Supported backends: pebble (aka pebbledb) | ||
| # The littidx backend is opened by Giga through its own storage config and is |
There was a problem hiding this comment.
We added that for migration purpose, so I dont think anyone has used littidx yet. We don't need to mention this comment since the above comment already state the supported backend is pebble
Addresses a seidroid review finding: recomputeBlockStats took each receipt by tx hash alone, trusting that the returned receipt actually belonged to the block being recomputed. Now that #4222 has landed, prefer ReceiptStore.IterateReceipts(height) — each receipt it yields is scoped to height by the iterator's own BlockNumber(), not by a tx-hash lookup that could answer from any block. - recomputeBlockStats delegates to receiptRecordsForHeight, which tries IterateReceipts first and falls back to decoding the block and fetching receipts by hash (receiptRecordsFromBlock, the prior behavior) only on ErrRangeQueryNotSupported — the littidx backend supports iteration, but pebble and MemoryReceiptStore do not. - Added stubIteratingReceiptStore/fakeReceiptIterator test doubles and a test proving the iterator path is used when available (backend.Block is left nil, so a fall-through to the old path would panic).
3 participants
Adds the ability to iterate receipts in the RecieptDB.