fix(evmrpc): return pruned errors from debug_trace* at unavailable heights (PLT-975) - #3888
fix(evmrpc): return pruned errors from debug_trace* at unavailable heights (PLT-975)#3888amir-deris wants to merge 5 commits into
Conversation
…ights (PLT-975) Guard all trace endpoints against block, receipt, and state retention before acquiring the trace semaphore so pruned heights fail fast with explicit errors instead of silent empty results or internal panics. Co-authored-by: Cursor <cursoragent@cursor.com>
amir-deris
changed the title
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview
Receipt layer: Reviewed by Cursor Bugbot for commit 1845b87. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3888 +/- ##
==========================================
- Coverage 59.45% 58.46% -1.00%
==========================================
Files 2319 2225 -94
Lines 198379 187933 -10446
==========================================
- Hits 117946 109871 -8075
+ Misses 69235 67694 -1541
+ Partials 11198 10368 -830
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.
The unified trace guard is the right shape and closes real holes (silent [] on pruned receipts, the state-pruned panic, the tx-hash bypass), but three issues block merge: latest-tag traces can now fail transiently because the guard compares the app tip against a lagging watermark, the state leg checks height where replay needs height-1, and the new ErrReceiptPruned sentinel bypasses the "not found" checks in eth_getTransactionReceipt/eth_getTransactionByHash/eth_getBlockReceipts. Codex's point about debug_traceCall not needing receipts is included; Cursor produced no output.
Findings: 3 blocking | 13 non-blocking | 10 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) is empty — no output from that reviewer. Codex's single finding (traceCall does not need receipts) is included below. - SS-disabled nodes:
WatermarkssetsstateEarliest = latestwhenstateStore == nil, so the newEnsureStateHeightAvailableleg makesEnsureTraceHeightAvailablereject every height below the tip. On a node with state store disabled, all historicaldebug_trace*now return "has been pruned". The new unit test (nil state store uses latest as earliest from Watermarks) pins this, so it looks intentional and consistent withResolveHeight/eth_call— but it is a user-visible narrowing that deserves a line in the PR description / release notes. - The retention floor that produces
ErrReceiptPrunedonly exists inlittReceiptStore. The non-littreceiptStore(sei-db/ledger_db/receipt/receipt_store.go) enforces no floor, so the "tx-hash guard hole" is only closed on the litt backend; on the other backenddebug_traceTransactionfor a pruned tx still falls through to the latest-height lookback check. Worth stating explicitly (or asserting the litt store is the only production path). - Nit:
evmrpc/tracers.gonow imports the package asreceipt, but two functions in the same file declare local variables namedreceipt(tryTraceCachearea,isPanicOrSyntheticTx). It compiles, butevmrpc/tx.goalready aliases this package asreceiptpkg; matching that avoids a shadowing trap for the next edit. - No test covers the new error path in
guardTraceRequestByHash(unknown hash now returnsblock %s not found/ the underlying watermark error instead ofnil). That is a user-visible change fordebug_traceBlockByHashanddebug_traceCall-by-hash and is currently unasserted. - Test plan's Tier-2 item (docker localnet with aggressive
min-retain-blocks) is still unchecked — that is the one check that would have surfaced the latest-tag and parent-state boundary issues below. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| // EnsureTraceHeightAvailable verifies block, receipt, and state availability | ||
| // for debug_trace* endpoints. All three stores must retain the height. | ||
| func (m *WatermarkManager) EnsureTraceHeightAvailable(ctx context.Context, height int64) error { | ||
| if err := m.EnsureBlockHeightAvailable(ctx, height); err != nil { |
There was a problem hiding this comment.
[suggestion] EnsureTraceHeightAvailable resolves watermarks twice (EnsureBlockHeightAvailable and EnsureStateHeightAvailable each call Watermarks, each of which does a tmClient.Status). On the by-hash path blockByHashRespectingWatermarks adds a third. Since the guard now runs before the semaphore, that is 3 Status calls per request under unbounded concurrency.
Call Watermarks(ctx) once and run the three ensureWithinWatermarks/floor comparisons against that snapshot — it is also more correct, since the current version can mix watermarks from two different reads.
| if returnErr = api.validateTraceTracer(config); returnErr != nil { | ||
| return nil, returnErr | ||
| } | ||
| if returnErr = api.guardTraceRequestByHash(ctx, "debug_traceBlockByHash", hash); returnErr != nil { |
There was a problem hiding this comment.
[suggestion] This reverses the invariant that the deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup (with its panicHashLookupClient) existed to pin: no Tendermint hash lookup before the semaphore is acquired. Two consequences worth stating explicitly rather than leaving implicit in a test rename:
BlockByHash+ up to 3Statuscalls now run outsideMaxConcurrentTraceCalls, so that knob no longer bounds the pre-trace work an attacker can drive withdebug_traceBlockByHash.- The guard now runs on the raw request context, so it is no longer bounded by
traceTimeout(prepareTraceContextis what creates that deadline).
Guard-before-wait is the right call for the pruned-height case, so I'm not asking to revert it — but please record the trade-off in the PR body/commit, and consider whether the pre-semaphore lookup needs its own bound.
| rcpt, err := api.keeper.GetReceipt(api.ctxProvider(LatestCtxHeight), hash) | ||
| if err != nil { | ||
| if errors.Is(err, receipt.ErrReceiptPruned) { | ||
| return err |
There was a problem hiding this comment.
[suggestion] Only ErrReceiptPruned is propagated; every other store error is swallowed and execution falls through to the latest-height lookback check at line 125, which then lets the trace proceed into the same panic path the PR is closing. Prefer returning any error that is not ErrNotFound:
rcpt, err := api.keeper.GetReceipt(api.ctxProvider(LatestCtxHeight), hash)
switch {
case err != nil && !errors.Is(err, receipt.ErrNotFound):
return err
case err == nil && rcpt != nil:
return api.guardTraceRequest(ctx, endpoint, int64(rcpt.BlockNumber))
}| if err == nil { | ||
| return receipt, nil | ||
| } | ||
| if errors.Is(err, ErrReceiptPruned) { |
There was a problem hiding this comment.
[nit] This early return is placed ahead of the legacyReceiptFromKVStore fallback, so a below-floor hit no longer consults the legacy KV store. In practice MigrateLegacyReceiptsBatch deletes the legacy key after writing to litt, so the fallback is usually already dead for these hashes — but that makes the interaction worth a word in the comment, and it is the mechanism behind the legacy-receipt amplifier noted on ErrReceiptPruned.
| continue | ||
| } | ||
| ethtx, _ := m.AsTransaction() | ||
| if ethtx == nil { |
There was a problem hiding this comment.
[nit] The nil guard is the right fix for the panic, but it leans on the discarded error one line up. Prefer making the failure explicit — ethtx, err := m.AsTransaction(); if err != nil || ethtx == nil { continue } — so a malformed EVM message is skipped for a stated reason rather than via a nil that reads as accidental.
| // blockTraceCacheGet assembles a per-tx hit; returns (nil, false) if any miss. | ||
| func blockTraceCacheGet(cache *keeper.TraceDB, height int64, txHashes []common.Hash, config *tracers.TraceConfig) ([]*tracers.TxTraceResult, bool) { | ||
| if cache == nil { | ||
| if cache == nil || len(txHashes) == 0 { |
There was a problem hiding this comment.
[nit] Correct fix for the false hit, with a side effect worth noting: a genuinely empty block (no EVM txs) can now never be served from this cache and pays a full trace on every request. tryBlockResultCache still covers it if a whole-block entry was baked, so this is likely acceptable — just confirm empty blocks do get block-level entries, otherwise this is a small permanent regression on a common case.
- Resolve latest/pending/safe/finalized trace tags via the watermark's safe latest instead of the raw app tip, so debug_trace* no longer intermittently errors while receipts/state lag the tip. - Check the parent height (height-1) against state retention, matching how initializeBlock actually replays a traced block. - Wrap ErrReceiptPruned around ErrNotFound so eth_getTransactionReceipt and friends keep returning null for pruned receipts instead of an RPC error, while trace guards can still react to it specifically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 19a24a5. Configure here.
There was a problem hiding this comment.
The unified trace guard is the right shape (single choke point, checked before semaphore acquisition), but the state-availability leg treats a disabled state store as "everything below the tip is pruned", which rejects essentially all debug_trace* requests on SS-disabled nodes and appears to contradict the PR's own TestGuardTraceRequestByHashUsesTendermintHeight assertion. Several smaller issues: an unmatched sentinel-less "block not found" error, a skipped legacy-receipt fallback, redundant Watermarks recomputation, and the residual tx-hash gap Codex flagged.
Findings: 3 blocking | 13 non-blocking | 9 posted inline
Blockers
evmrpc/testsandevmrpcunit tests could not be executed in this environment, so the failure predicted forTestGuardTraceRequestByHashUsesTendermintHeight(see inline comments onevmrpc/watermark_manager.goandevmrpc/historical_debug_trace_test.go) is from reading the code rather than a run. Please confirmgo test ./evmrpc/... ./sei-db/ledger_db/receipt/...is green before merging — if it is, that means the nil-stateStorepath behaves differently than I read it and the analysis should be rechecked rather than dismissed.- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty — that review pass produced no output, so this synthesis reflects only Claude's and Codex's findings. - The guard now runs before
prepareTraceContext, so the block-by-hash lookup plus up to threeWatermarkscomputations (each antmClient.Statuscall + store version reads) happen outside the trace semaphore on everydebug_trace*request. The deletedTestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookupexisted to pin the opposite ordering; the reversal is intentional and justified here, but the concurrency-bounding property it protected is now gone. Worth a note in the PR description (or a cheap pre-check) so the next person doesn't re-reverse it. - The pruned-receipt signal only exists in the litt backend.
receiptStore.GetReceipt(sei-db/ledger_db/receipt/receipt_store.go:203) has no retention-floor check at all and can only ever returnErrNotFound, so on nodes using that backend the tx-hash guard hole this PR closes stays fully open. Either state that asymmetry in the commit/PR body or lift the floor check into the shared layer. - No test covers
latestTraceHeight's fallback branches (nilbackend/watermarks, orLatestHeightreturning an error), norguardTraceRequestByHashpropagating an unknown-hash error, nor the reordering ondebug_traceCallspecifically (onlyTraceBlockByHashandTraceBlockByNumbergot before-semaphore tests). These are the paths the PR actually changed from lenient to strict. evmrpc/AGENTS.mddocumentsdebug_trace*semantics (faithful replay, tracer gating) but not the new availability invariant. Adding a line — "all three of block/receipt/state must retain the height; the guard runs before semaphore acquisition" — would keep the module guide the source of truth for this contract, per the repo's nested-guide convention.- Drive-by scope: the
filterTransactionsnil-guard and theblockTraceCacheGetempty-list change are unrelated to pruning. They're small and defensible, but calling them out as separate concerns in the PR body (or splitting them) would make the pruning change easier to revert in isolation. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| return err | ||
| } | ||
| return api.guardHistoricalDebugTraceHeight(ctx, endpoint, block.Block.Height) | ||
| if block == nil || block.Block == nil { |
There was a problem hiding this comment.
[suggestion] This branch is unreachable, and the error it returns isn't matchable. blockByHashRespectingWatermarks → blockByHashWithRetry already converts blockRes.Block == nil into ErrBlockNotFoundByHash (evmrpc/utils.go:179), so a (nil-block, nil-error) return can't occur.
More importantly, if it ever did, a bare fmt.Errorf("block %s not found") can't be recognised by callers — the rest of the package keys off the ErrBlockNotFoundByHash sentinel (e.g. blockByHashOrNullForJSONRPC maps it to JSON null). Either drop the branch or return fmt.Errorf("block %s: %w", hash.Hex(), ErrBlockNotFoundByHash).
| func (api *DebugAPI) guardTraceRequestByTxHash(ctx context.Context, endpoint string, hash common.Hash) error { | ||
| if api.keeper != nil { | ||
| rcpt, err := api.keeper.GetReceipt(api.ctxProvider(LatestCtxHeight), hash) | ||
| if err != nil { |
There was a problem hiding this comment.
[suggestion] Two things worth tightening here.
- Every non-
ErrReceiptPrunederror fromGetReceiptis silently swallowed — including genuine store failures — and execution falls through to the latest-height lookback check, which passes. That's the same "lookup failed, so skip the guard" lenience this PR is fixing on the by-hash path. - The fallback calls
guardHistoricalDebugTraceHeightdirectly rather thanguardTraceRequest, so no availability check runs at all when the receipt is unknown. That's defensible (there's no height to check), but it's load-bearing and non-obvious — a one-line doc comment on the function stating "receipt unknown ⇒ lookback-only, availability cannot be evaluated" would keep a later reader from "fixing" it intoguardTraceRequest(latestTraceHeight)and rejecting unknown-hash traces.
| if err == nil { | ||
| return receipt, nil | ||
| } | ||
| if errors.Is(err, ErrReceiptPruned) { |
There was a problem hiding this comment.
[suggestion] This early return also skips the legacyReceiptFromKVStore fallback below. Previously a below-floor receipt returned ErrNotFound and the legacy KV store was still consulted; now it short-circuits. The overlap case (a receipt present in litt and below the litt floor and also in legacy KV) should be rare — pre-litt receipts aren't in litt at all — but the ordering change is silent. A brief comment stating that a pruned litt entry is authoritative and deliberately does not fall through to legacy would pin the intent.
| if s.belowRetentionFloor(r.BlockNumber) { | ||
| return nil, ErrNotFound | ||
| earliest := s.earliestVersion.Load() | ||
| return nil, fmt.Errorf("requested height %d receipts have been pruned; earliest available is %d: %w", |
There was a problem hiding this comment.
[suggestion] Agreeing with Codex's P1, though I'd scope it as a documented limitation rather than a blocker: this only fires while litt's lazily-expired value is still physically present. Once litt actually deletes it, s.receipts.Get reports !exists, GetReceiptFromStore returns plain ErrNotFound, and guardTraceRequestByTxHash falls through to the latest-height lookback guard — so debug_traceTransaction on a long-pruned hash still reports "not found" instead of "pruned". The tx-hash hole is narrowed, not closed.
A store-level fix isn't possible (the block number is gone with the value), but the guard could close it: on ErrNotFound, if receiptStore.EarliestVersion() is above the earliest traceable height, report "receipt not found; receipts below height X have been pruned" instead of a bare not-found. At minimum, note the residual gap in the PR description so PR 2 of PLT-975 picks it up.
|
|
||
| // EnsureTraceHeightAvailable verifies block, receipt, and state availability | ||
| // for debug_trace* endpoints. All three stores must retain the height. | ||
| func (m *WatermarkManager) EnsureTraceHeightAvailable(ctx context.Context, height int64) error { |
There was a problem hiding this comment.
[suggestion] A single guarded trace call now recomputes Watermarks up to three times: once via latestTraceHeight → LatestHeight, once in EnsureBlockHeightAvailable, once in EnsureStateHeightAvailable. Each does a tmClient.Status round trip plus receipt/state version reads, and — since the guard was deliberately moved ahead of the semaphore — this is now unbounded under trace load.
Fetching once and applying the three comparisons inline would be both cheaper and more consistent (today the block and state legs can observe different latest values mid-flight):
blockEarliest, stateEarliest, latest, err := m.Watermarks(ctx)
if err != nil { return err }
if err := ensureWithinWatermarks(height, blockEarliest, latest); err != nil { return err }
if err := m.EnsureReceiptHeightAvailable(height); err != nil { return err }
// ... state leg against stateEarliest/latestThis also reads better against the "guard at the choke point" rule in AGENTS.md: one watermark snapshot, three named checks.
| // blockTraceCacheGet assembles a per-tx hit; returns (nil, false) if any miss. | ||
| func blockTraceCacheGet(cache *keeper.TraceDB, height int64, txHashes []common.Hash, config *tracers.TraceConfig) ([]*tracers.TxTraceResult, bool) { | ||
| if cache == nil { | ||
| if cache == nil || len(txHashes) == 0 { |
There was a problem hiding this comment.
[nit] Worth a comment on why an empty tx list must not be a hit. As written this reads like a perf regression — an empty block used to short-circuit to [] and now walks the full trace path to produce the same []. The actual reason is that the old code reported a hit without ever consulting the cache, which let an unavailable height answer [] with HTTP 200 (the silent-[] row in your PR table). Naming that in the comment stops someone reverting it as a pointless miss.
| continue | ||
| } | ||
| ethtx, _ := m.AsTransaction() | ||
| if ethtx == nil { |
There was a problem hiding this comment.
[nit] The guard fixes a real nil-deref, but pairing it with the discarded error on the line above makes a malformed message vanish with no trace. Since you're already touching this, consider ethtx, err := m.AsTransaction(); if err != nil || ethtx == nil { continue } — same control flow, but the error is at least available to log or count if this ever fires.
There was a problem hiding this comment.
The unified trace-availability guard is well-targeted and the guard/execution height alignment (replacing ConvertBlockNumber with the shared getBlockNumber) is a genuine correctness improvement, backed by focused unit tests. No blocking defects found; the notes below are gaps in the fix's reach (pebble receipt backend, parent-block retention, legacy receipt fallback), a deliberately reversed concurrency invariant that isn't called out, and duplication that the repo's structural guidelines would push back on.
Findings: 0 blocking | 13 non-blocking | 9 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review is Claude + Codex only. - Codex flagged as High that
EnsureTraceCallHeightAvailable/latestTraceHeightinherit the receipt-cappedlatestfromWatermarks()even thoughdebug_traceCallneeds no receipts. Keeping it for visibility, but I disagree on severity/novelty:StateAndHeaderByNumberOrHash→getBlockByNumberOrHash→blockByNumberRespectingWatermarksalready applied the samelatestcap before this PR, andlatest-tag resolution goes through the samewm.LatestHeight, so the guard and the executed height agree and no request is newly rejected. If a receipts-independent safe-latest is wanted for state-only endpoints, that is a separate change toWatermarks(). - Test coverage stops at the unit boundary for the two behaviours the description headlines. There is no test that
guardTraceRequestByTxHashactually propagatesErrReceiptPrunedout ofdebug_traceTransaction/debug_traceStateAccess(only the litt store-level test atlittidx_test.go), and none covering the "state pruned → panic → -32603" case the summary table lists as fixed — the state leg is exercised only throughWatermarkManagerdirectly. - The Tier 2 item in the test plan (docker localnet with aggressive
min-retain-blocks, comparing trace errors against theeth_getBlockTransactionCountByNumbercontrol) is still unchecked. Given the fix is specifically about behaviour at retention floors, that is the check most likely to surface the parent-height and backend-coverage gaps noted inline. - 9 suggestion(s)/nit(s) flagged inline on specific lines.
| if returnErr = api.validateTraceTracer(config); returnErr != nil { | ||
| return nil, returnErr | ||
| } | ||
| if returnErr = api.guardTraceRequestByHash(ctx, "debug_traceBlockByHash", hash); returnErr != nil { |
There was a problem hiding this comment.
[suggestion] This reverses an invariant that was previously pinned on purpose. The deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup used a client whose BlockByHash panicked with "hash lookup should not happen before trace context setup", i.e. hash resolution was deliberately deferred until after the MaxConcurrentTraceCalls semaphore. After this change every debug_traceBlockByHash / debug_traceCall (and, via latestTraceHeight, every by-number trace) performs a tmClient.Status plus a Tendermint BlockByHash lookup outside the concurrency limit.
The trade-off looks defensible — a pruned request shouldn't have to win a semaphore slot to learn it's pruned, and these are cheap in-process reads next to an actual trace — but the PR description frames it only as "reorder so guard precedes prepareTraceContext" and doesn't mention that a pinned protection was removed. Worth stating the reasoning explicitly here or in the description, since the next reader will find the deleted test in history and not know it was intentional.
| // SS disabled: trace replay uses SC via ctxProvider, not SS retention. | ||
| return nil | ||
| } | ||
| stateHeight := max(height-1, m.genesisInitialHeight()) |
There was a problem hiding this comment.
[suggestion] The state leg correctly accounts for replay reading parent state at height-1, but the block leg only checks height. initializeBlock also calls b.tmClient.Validators(ctx, &prevBlockHeight, ...) at height-1, so at the Tendermint block-retention boundary (height == blockEarliest) this guard passes and tracing then fails downstream with "failed to load validators for block %d from tendermint" — a generic error, not the pruned error this PR exists to produce. Same class of issue Codex raised.
Narrow window (exactly one height), and it errors rather than panics, so not blocking. But the asymmetry is surprising given stateHeight already does the height-1 reasoning: consider EnsureBlockHeightAvailable(ctx, max(height-1, genesisInitialHeight())) here so the parent-block requirement is expressed in the same place, and drop the TestTraceReceiptFloorBoundary comment claiming the guard "applies at the requested height only" if that changes.
| // the store's retention floor. It wraps ErrNotFound so existing not-found | ||
| // handling (e.g. eth_getTransactionReceipt returning null) still applies, | ||
| // while errors.Is(err, ErrReceiptPruned) lets trace guards react to it specifically. | ||
| ErrReceiptPruned = fmt.Errorf("receipt pruned: %w", ErrNotFound) |
There was a problem hiding this comment.
[suggestion] ErrReceiptPruned is only ever produced by littReceiptStore. The pebble backend (receiptBackendPebble, selectable via ReceiptStoreConfig.Backend) also prunes — newReceiptBackend calls startReceiptPruning — but receiptStore.GetReceipt/GetReceiptFromStore enforce no retention floor and return plain ErrNotFound (lines 219, 241).
So the "close the tx-hash guard hole" part of this PR only lands on litt nodes; on pebble nodes guardTraceRequestByTxHash still falls through to the lookback-only check and debug_traceTransaction on a pruned tx degrades to a downstream transaction not found. The height-based guards (EnsureReceiptHeightAvailable via EarliestVersion()) do still cover the by-number/by-hash paths on both backends, so this is a reach gap rather than a regression — but either mirror the floor check in receiptStore or say in the doc comment that the sentinel is litt-only, otherwise the next reader will assume backend parity.
| if err == nil { | ||
| return receipt, nil | ||
| } | ||
| if errors.Is(err, ErrReceiptPruned) { |
There was a problem hiding this comment.
[suggestion] This early return skips the legacy KV fallback below. Previously a below-floor hit surfaced as ErrNotFound from GetReceiptFromStore and fell through to legacyReceiptFromKVStore; now it errors out immediately. For a node whose legacy KV store still holds receipts for a height that litt has aged past, that's a served receipt turning into an error.
The overlap is probably empty in practice (legacy receipts predate litt, so they shouldn't have litt entries at all), which is why I'm not calling it blocking — but the safer ordering is to attempt legacyReceiptFromKVStore first and only return the ErrReceiptPruned wrap if that also misses. That keeps "pruned" meaning "unavailable everywhere", which is what the trace guard actually wants to assert.
| if err := m.EnsureBlockHeightAvailable(ctx, height); err != nil { | ||
| return err | ||
| } | ||
| if m.stateStore == nil { |
There was a problem hiding this comment.
[suggestion] The stateStore == nil short-circuit and its comment are duplicated verbatim in EnsureTraceHeightAvailable (line 221). AGENTS.md's "guard at the choke point, never at each caller" applies: a third trace guard added later has to remember this, and the identical comment in two places is the tell. Hoisting it into one named helper — something like ensureReplayStateAvailable(ctx, height) whose doc comment carries the why (SS disabled ⇒ replay reads state via SC/ctxProvider, so SS watermarks don't apply) — would leave both Ensure* methods reading as a clean sequence of steps.
Related: EnsureStateHeightAvailable is exported and, with stateStore == nil, Watermarks() sets stateEarliest = latest, so it reports every historical height as pruned — pinned by TestEnsureStateHeightAvailable's "nil state store" subtest. That's a trap for a future caller who reaches for it directly. Worth a doc-comment sentence saying it reports SS retention only and is not meaningful when SS is disabled.
| func (api *DebugAPI) guardHistoricalDebugTraceByTxHash(ctx context.Context, endpoint string, hash common.Hash) error { | ||
| if api.keeper == nil { | ||
| return nil | ||
| func (api *DebugAPI) guardTraceRequest(ctx context.Context, endpoint string, height int64) error { |
There was a problem hiding this comment.
[suggestion] Two things about the new guard layer:
-
guardTraceRequest{,ByNumber,ByHash,ByNumberOrHash,ByTxHash}andguardTraceCallRequest{,ByNumber,ByHash,ByNumberOrHash}are nine functions where the by-number/by-hash/by-number-or-hash trios are byte-for-byte identical apart from whichEnsure*method the leaf calls. Threading the availability check through instead — e.g. one family takingensure func(context.Context, int64) error, withEnsureTraceHeightAvailable/EnsureTraceCallHeightAvailablepassed at the entry points — would halve this without losing the replay-vs-call distinction the PR is careful to draw. -
Ordering side effect: the watermark check now runs before
guardHistoricalDebugTraceHeight, sorecordHistoricalDebugTraceAttemptno longer fires for a height that is both pruned and beyondmaxBlockLookback. If that metric is used to sizeMaxTraceLookbackBlocks, it now undercounts on pruning-heavy nodes. Probably fine, but it's a silent observability change not mentioned in the description.
| // blockTraceCacheGet assembles a per-tx hit; returns (nil, false) if any miss. | ||
| func blockTraceCacheGet(cache *keeper.TraceDB, height int64, txHashes []common.Hash, config *tracers.TraceConfig) ([]*tracers.TxTraceResult, bool) { | ||
| if cache == nil { | ||
| if cache == nil || len(txHashes) == 0 { |
There was a problem hiding this comment.
[nit] The what is clear but the why is the load-bearing part and it's only in the test name. The reason an empty txHashes must not be a hit is that Backend.BlockByNumber drops txs whose receipts aren't found (if !found { continue }), so an empty list is indistinguishable from "receipts pruned" — which is exactly the silent []/HTTP 200 failure mode in the PR's table. Per AGENTS.md, that belongs in the doc comment above (currently just "assembles a per-tx hit; returns (nil, false) if any miss").
Also worth a word that a genuinely empty block now always falls through to the full trace path; harmless (no txs to replay, and tryBlockResultCache still covers the block-level entry), but it reads like an oversight without the note.
| func (b Backend) BlockByNumber(ctx context.Context, bn rpc.BlockNumber) (*ethtypes.Block, []tracersutils.TraceBlockMetadata, error) { | ||
| blockNum := b.ConvertBlockNumber(bn) | ||
| tmBlock, err := blockByNumberRespectingWatermarks(ctx, b.tmClient, b.watermarks, &blockNum, 1) | ||
| blockNumberPtr, err := getBlockNumber(ctx, b.tmClient, bn) |
There was a problem hiding this comment.
[nit] Swapping ConvertBlockNumber for getBlockNumber also changes pending handling: the old code did panic("tracing on pending block is not supported"), while getBlockNumber maps PendingBlockNumber to nil (= latest). resolveDebugTraceBlockNumber maps it to latestTraceHeight too, so guard and execution stay consistent and this is a strict improvement over panicking — but it's a user-visible semantic change (debug_traceBlockByNumber("pending") now traces latest instead of erroring) that the description doesn't list. Worth a line in the PR body, since it's the kind of thing an integrator notices before we do.
| require.NoError(t, wm.EnsureTraceCallHeightAvailable(t.Context(), 175)) | ||
|
|
||
| // Receipts pruned below 150; replay guard fails, TraceCall guard does not check receipts. | ||
| rs.earliest = 150 |
There was a problem hiding this comment.
[nit] rs.earliest = 150 is a no-op — rs is constructed with earliest: 150 five lines up. The comment above ("Receipts pruned below 150") reads as though this line establishes that state, so a later reader may adjust the constructor and assume this line re-pins it. Either drop the assignment or initialise rs with earliest: 1 so the mutation is what actually moves the floor (as TestEnsureTraceHeightAvailable at line 192 does).
Superseded: latest AI review found no blocking issues.
1 participant
Summary
Fixes PLT-975 (PR 1 of 2). Historical
debug_trace*reads block, receipt, and state stores with independent retention, but only block retention was checked before tracing. That mismatch caused:[](HTTP 200)-32603This PR adds a unified trace guard at the RPC choke point so pruned heights return explicit errors — consistent with
eth_getBlockTransactionCountByNumberand theevmrpc/AGENTS.mdhistorical-consistency invariant.Key changes:
EnsureTraceHeightAvailable(block + receipt + state) andEnsureStateHeightAvailableonWatermarkManagerdebug_trace*entry points (TraceTransaction,TraceBlockBy*,TraceCall,TraceStateAccess,TraceTransactionProfile)TraceBlockByHash/TraceCallso guard precedesprepareTraceContextErrReceiptPrunedinstead of skipping checks when receipt lookup failsErrReceiptPrunedsentinel in litt receipt store (distinct fromErrNotFound)blockTraceCacheGettreating empty tx list as a cache hitAsTransaction()infilterTransactionsdebug_traceCallfrom replay tracing: addEnsureTraceCallHeightAvailable(block + state only, no receipts) andguardTraceCallRequest*variants, sinceTraceCallreads state at the requested height directly and never touches receipts — unlike replay tracing, which reads receipts and replays from the parent (height-1) stateBackend.BlockByNumberguarded one height (from the ad-hocConvertBlockNumberresolution oflatest/safe/finalized/earliest) but executed against another. Replaced it with the sharedgetBlockNumberhelper already used by the rest ofevmrpcso the guarded height and the executed height are always the sameEnsureTraceCallHeightAvailable/EnsureTraceHeightAvailable: when SS is disabled, trace replay reads state via SC (ctxProvider), not SS retention, so the guard now short-circuits instead of evaluating watermarks against a nil storeFollow-up (separate future PR): go-ethereum
trace_timeoutfix for full concurrency relief (PLT-975 PR 2).Test plan
EnsureTraceHeightAvailable/EnsureStateHeightAvailable/EnsureTraceCallHeightAvailablewatermark cases, including SS-disabled (nil state store)BlockByNumber)blockTraceCacheGetempty-list false-hit regressionErrReceiptPrunedbelow retention floordebug_traceCallguarded against block+state only (no receipt check) at both current and historical heightsBackend.BlockByNumberresolveslatest/safe/finalized/earliestvia the same path used for guarding, so guard and execution heights agreemin-retain-blocks— confirm trace errors match tx-count control height