fix(evmrpc): fail fast on pruned heights for debug_trace* (PLT-975) - #3907
fix(evmrpc): fail fast on pruned heights for debug_trace* (PLT-975)#3907amir-deris wants to merge 13 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>
- 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>
amir-deris
changed the title
|
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 #3907 +/- ##
==========================================
- Coverage 59.47% 58.38% -1.09%
==========================================
Files 2323 2226 -97
Lines 198389 186928 -11461
==========================================
- Hits 117982 109129 -8853
+ Misses 69198 67505 -1693
+ Partials 11209 10294 -915
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Replay endpoints use
Reviewed by Cursor Bugbot for commit 8e4b72f. 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}
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 e7399d0. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-tested hardening of the debug_trace* retention guards: the new EnsureTraceHeightAvailable/EnsureTraceCallHeightAvailable split matches what each path actually reads, and the strings.Contains("not found") → errors.Is migration is a real improvement. No blocking correctness or security defects found, but there is a coverage gap in the by-tx-hash pruned path (Codex's finding), several efficiency/duplication issues in the new guard layer, and a deliberately reversed guard/semaphore ordering invariant worth confirming.
Findings: 0 blocking | 14 non-blocking | 8 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 reflects only the Codex pass plus my own analysis. - Guard/semaphore ordering is deliberately inverted: the deleted
TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup(and itspanicHashLookupClient) encoded the invariant that no Tendermint hash lookup happens before the trace semaphore is acquired. Now everydebug_traceBlockByHash/debug_traceCallperforms a block-by-hash index lookup plus 3-4Statuscalls outside the concurrency limiter. Failing fast is the right goal, but the limiter no longer bounds that work — please confirm this trade-off is intended, and consider noting it in the PR description since it reverses a previously test-enforced property. - Test coverage regression: with that test removed,
debug_traceCallno longer has any test asserting it returnserrTraceConcurrencyLimitwhen the semaphore is full (TestTraceBlockByNumberRejectsConcurrencyLimitAfterGuardonly coversTraceBlockByNumber). - Guard duplication:
guardTraceRequest{,ByNumber,ByHash,ByNumberOrHash}andguardTraceCallRequest{,ByNumber,ByHash,ByNumberOrHash}are eight methods that are byte-identical except for the terminalEnsureTrace…HeightAvailablecall. Per AGENTS.md ("guard at the choke point, never at each caller"), consider one resolution helper (resolveTraceHeight(ctx, endpoint, blockNrOrHash) (int64, error)) plus a single guard parameterized by theensure func(context.Context, int64) error— halves the surface and makes a future third guard variant a one-liner instead of four new copies. - Consistency:
evmrpc/block.go:358still usesstrings.Contains(err.Error(), "not found")on a receipt lookup while this PR converted the three sibling call sites intx.gotoerrors.Is(err, receiptpkg.ErrNotFound). Behavior is unchanged today (the new pruned message still contains "not found"), but leaving one string-matcher behind is exactly the fragility the rest of the PR removes. - Test hygiene:
TestEnsureTraceCallHeightAvailableIgnoresReceiptsandTestTraceReceiptFloorBoundary(both inhistorical_debug_trace_test.go) build the same fixture and largely assert the same thing; and inTestEnsureTraceCallHeightAvailable,rs.earliest = 150is a no-op since the fake was constructed withearliest: 150. - 8 suggestion(s)/nit(s) flagged inline on specific lines.
| if api.keeper != nil { | ||
| rcpt, err := api.keeper.GetReceipt(api.ctxProvider(LatestCtxHeight), hash) | ||
| if err != nil { | ||
| if errors.Is(err, receipt.ErrReceiptPruned) || !errors.Is(err, receipt.ErrNotFound) { |
There was a problem hiding this comment.
[suggestion] Pruned receipts are still indistinguishable from unknown tx hashes on the main by-hash path (raised by the Codex pass as High; I agree it's real, though I'd scope it as a coverage gap rather than a regression).
ErrReceiptPruned is only produced when the record is physically still present but below the retention floor:
littReceiptStore.GetReceiptFromStorereturns it only after a successfulreceipts.Get; once litt's TTL actually reclaims the value,!exists→ plainErrNotFound(litt_receipt_store.go:263).- The default
receiptStore.GetReceipthas no retention-floor check at all and always returns plainErrNotFound(receipt_store.go:220).
In both cases this falls through to guardHistoricalDebugTraceHeight(ctx, endpoint, api.latestTraceHeight(ctx)), which passes, so debug_traceTransaction / debug_traceStateAccess / debug_traceTransactionProfile on a pruned tx do not fail fast — the stated goal of the PR — and instead proceed into the trace path.
An unknown hash genuinely has no resolvable height, so you can't fully close this. But you could make the fall-through informative: when errors.Is(err, receipt.ErrNotFound) and api.backend.watermarks.receiptStore.EarliestVersion() > blockEarliest, return something like "receipt not found; receipts below height %d have been pruned" rather than silently continuing. At minimum, worth documenting the limitation here.
| // for debug_trace* replay endpoints (transaction/block). Replay loads parent | ||
| // block validators and parent state (height-1) and reads receipts at the | ||
| // requested height. | ||
| func (m *WatermarkManager) EnsureTraceHeightAvailable(ctx context.Context, height int64) error { |
There was a problem hiding this comment.
[suggestion] EnsureTraceHeightAvailable recomputes Watermarks() three times (via EnsureBlockHeightAvailable, ensureReplayParentBlockAvailable → EnsureBlockHeightAvailable, and EnsureStateHeightAvailable), and each Watermarks() call does a tmClient.Status(), a ctxProvider(LatestCtxHeight), a receiptStore.LatestVersion(), and two stateStore version reads. For debug_traceBlockByHash it's four Status calls per request (the guard's blockByHashRespectingWatermarks adds one more) — all now before the trace semaphore.
Two consequences: unnecessary per-request cost on an unauthenticated endpoint, and the three snapshots can disagree if the tip advances mid-guard.
Suggest fetching once and validating against the snapshot:
blockEarliest, stateEarliest, latest, err := m.Watermarks(ctx)
if err != nil { return err }
if err := ensureWithinWatermarks(height, blockEarliest, latest); err != nil { return err }
// ... parent, receipt, state checks against the same tripleThe same applies to EnsureTraceCallHeightAvailable (two calls).
| // ensureReplayParentBlockAvailable verifies the parent block height replay | ||
| // tracing loads for validator set lookup in initializeBlock. | ||
| func (m *WatermarkManager) ensureReplayParentBlockAvailable(ctx context.Context, height int64) error { | ||
| parentBlockHeight := max(height-1, 0) |
There was a problem hiding this comment.
[suggestion] Two issues with the parent-block floor:
-
The
max(height-1, 0)clamp is ineffective.EnsureBlockHeightAvailable(0)always fails, sinceblockEarliestcomes from Tendermint'sEarliestBlockHeightand is>= 1. So on a chain started at initial height 1,debug_traceBlockByNumber(1)is always rejected. If the intent was to make the genesis block traceable, clamp tom.genesisInitialHeight()(asEnsureTraceHeightAvailablealready does forstateHeight) or skip the parent check whenheight <= genesisInitialHeight. -
The error message names the wrong height. Tracing the earliest retained block surfaces
"requested height 149 has been pruned; earliest available is 150"to a caller who asked for 150 — confusing, andTestEnsureTraceHeightAvailableParentBlockFloorlocks that message in. Consider wrapping:fmt.Errorf("cannot trace height %d: parent block unavailable: %w", height, err).
| } | ||
| } | ||
| return api.guardHistoricalDebugTraceHeight(ctx, endpoint, int64(receipt.BlockNumber)) //nolint:gosec | ||
| return api.guardHistoricalDebugTraceHeight(ctx, endpoint, api.latestTraceHeight(ctx)) |
There was a problem hiding this comment.
[nit] Two notions of "latest" are now mixed: this passes api.latestTraceHeight(ctx) (the watermark safe latest) into guardHistoricalDebugTraceHeight, which internally compares against api.ctxProvider(LatestCtxHeight).BlockHeight() (the raw app tip). When the watermark lags the tip — which the latestTraceHeight doc comment says is the normal case — the lookback check sees blockHeight < latest, so with max_trace_lookback_blocks set to 0 or 1 a plain debug_traceBlockByNumber("latest") would be rejected with "block number 99 is beyond max lookback of 0".
Harmless at the default of 10000, but the fix is small: have guardHistoricalDebugTraceHeight use api.latestTraceHeight(ctx) as its reference too, so both sides agree on what "latest" means. Same applies at lines 146/170/217.
| // make EnsureTraceHeightAvailable reject the most common trace requests. | ||
| func (api *DebugAPI) latestTraceHeight(ctx context.Context) int64 { | ||
| if api.backend != nil && api.backend.watermarks != nil { | ||
| if latest, err := api.backend.watermarks.LatestHeight(ctx); err == nil { |
There was a problem hiding this comment.
[nit] The watermark error is silently discarded and the raw app tip is returned instead. That tip is then handed to guardTraceRequest, whose EnsureTraceHeightAvailable calls Watermarks() again and fails with the same underlying error — so the fallback buys nothing and hides the cause. Either propagate the error (latestTraceHeight(ctx) (int64, error)) or add a comment explaining why the tip is a safe substitute here.
| // 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] This makes a genuinely empty block always miss the cache and fall through to the full trace path, where the correct answer is the empty slice this used to return cheaply. Neither the diff nor the PR description explains the motivation. If the concern is that an empty txHashes can also mean "we failed to load the tx list", that ambiguity is better resolved at the call site (txHashesOf(block.Transactions()) in tryBlockTraceCacheBy{Number,Hash}) than by making empty blocks permanently uncacheable — and the renamed test ("empty tx list is not a cache hit") then documents the intent rather than just the mechanism.
| continue | ||
| } | ||
| ethtx, _ := m.AsTransaction() | ||
| if ethtx == nil { |
There was a problem hiding this comment.
[nit] Good catch on the nil deref, but the check papers over a discarded error two lines up. m.AsTransaction() returns (nil, err) on a decode failure; testing the error directly says why the tx is skipped and won't silently break if AsTransaction ever returns a non-nil tx alongside an error:
ethtx, err := m.AsTransaction()
if err != nil || ethtx == nil {
continue
}Also: this path isn't covered by any test in the PR — a decode-failure case in filterTransactions would pin the fix.
| 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] Undocumented behavior change worth a line in the PR description: the removed ConvertBlockNumber panicked with "tracing on pending block is not supported" for rpc.PendingBlockNumber, whereas getBlockNumber maps pending to nil → the watermark safe latest. Replacing a panic with a sensible resolution is an improvement, but debug_traceBlockByNumber("pending") now silently traces the latest block instead of erroring, and no test covers the pending tag on this path.
There was a problem hiding this comment.
Solid, well-tested hardening of the debug_trace* retention guards, but the replay parent-block check floors at 0 instead of the chain's initial height, which makes tracing the genesis block (and the earliest tag) fail on any node. Several non-blocking notes on guard cost, the reversed semaphore-ordering invariant, and the empty-block cache fast path.
Findings: 1 blocking | 9 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review reflects only the Codex finding (confirmed, see the inline comment onwatermark_manager.go:237) plus my own.- No test covers a trace at the chain's initial height or the
earliestblock tag.TestEnsureTraceHeightAvailableParentBlockFloorpins the pruned-floor case at height 150 but never the genesis case, which is exactly the gap that lets themax(height-1, 0)bug through. Worth adding:EarliestBlockHeight: 1,EnsureTraceHeightAvailable(ctx, 1)→ expect no error. TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookupcovered bothTraceBlockByHashandTraceCall; the three replacement tests coverTraceBlockByNumberandTraceBlockByHashonly.debug_traceCallnow has no test asserting its guard runs before the semaphore.- In
guardTraceRequestByHash(tracers.go:157) andguardTraceCallRequestByHash(tracers.go:198), theblock == nil || block.Block == nilbranch is unreachable:blockByHashRespectingWatermarksalready dereferencesblock.Block.Heightbefore returning a nil error, so it would have panicked first. Either drop the check or move the nil handling into that helper. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // ensureReplayParentBlockAvailable verifies the parent block height replay | ||
| // tracing loads for validator set lookup in initializeBlock. | ||
| func (m *WatermarkManager) ensureReplayParentBlockAvailable(ctx context.Context, height int64) error { | ||
| parentBlockHeight := max(height-1, 0) |
There was a problem hiding this comment.
[blocker] Off-by-one at the chain's initial height. For height == genesisInitialHeight (1 on most chains) this checks height 0, which is always below blockEarliest (status.SyncInfo.EarliestBlockHeight, ≥ 1), so ensureWithinWatermarks rejects it with the misleading message requested height 0 has been pruned; earliest available is 1.
Concretely, on an archive node with EarliestBlockHeight: 1, both debug_traceBlockByNumber("0x1") and debug_traceBlockByNumber("earliest") (which resolveDebugTraceBlockNumber maps to Genesis.InitialHeight) now fail, as does debug_traceTransaction for any tx in the genesis block. Same for chains started above height 1.
The state leg two lines up already gets this right — stateHeight := max(height-1, m.genesisInitialHeight()). Use the same floor here:
parentBlockHeight := max(height-1, m.genesisInitialHeight())(Also flagged by Codex as P1.)
| 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] Moving the guard ahead of prepareTraceContext deliberately reverses the invariant that the now-deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup (and its panicHashLookupClient) existed to enforce. Two consequences worth being explicit about in the PR description:
- The Tendermint block-by-hash lookup and the watermark reads now run with unbounded concurrency —
MaxConcurrentTraceCallsno longer caps them. A flood ofdebug_traceBlockByHashwith unknown hashes is no longer throttled. - The guard runs on the caller's
ctx, not thetraceTimeoutcontext created byprepareTraceContext, so this pre-work is no longer bounded byTraceTimeout.
The fail-fast goal is right; the trade-off just deserves a note, and ideally the guard should still run under the trace timeout (create the timeout context first, acquire the semaphore after).
| // for debug_trace* replay endpoints (transaction/block). Replay loads parent | ||
| // block validators and parent state (height-1) and reads receipts at the | ||
| // requested height. | ||
| func (m *WatermarkManager) EnsureTraceHeightAvailable(ctx context.Context, height int64) error { |
There was a problem hiding this comment.
[suggestion] EnsureTraceHeightAvailable calls Watermarks three times (via EnsureBlockHeightAvailable, ensureReplayParentBlockAvailable, and EnsureStateHeightAvailable), each of which issues a tmClient.Status plus receipt/state version reads. For a latest-tagged request latestTraceHeight adds a fourth. Since this now runs before the semaphore (unbounded concurrency), the amplification is on the un-throttled path.
Fetching once and validating all legs against that snapshot would be both cheaper and internally consistent — right now the three checks can observe different watermark snapshots mid-flight:
blockEarliest, stateEarliest, latest, err := m.Watermarks(ctx)
if err != nil { return err }
// validate height, parent, receipt floor, and stateHeight against this snapshot| // 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.
[suggestion] This turns an empty block from an instant ([], true) into a full cache miss, so debug_traceBlockBy{Number,Hash} on an empty block now falls through to the complete replay path (parent state load, block init) only to produce []. On a chain with sub-second blocks, empty blocks are common enough that this is a real cost.
The PR describes it as a fix, but [] is the correct trace result for a block with no transactions. If the concern is masking a case where block.Transactions() came back empty because decoding failed rather than because the block is genuinely empty, the guard should distinguish those two rather than dropping the fast path for both.
| if api.keeper != nil { | ||
| rcpt, err := api.keeper.GetReceipt(api.ctxProvider(LatestCtxHeight), hash) | ||
| if err != nil { | ||
| if errors.Is(err, receipt.ErrReceiptPruned) || !errors.Is(err, receipt.ErrNotFound) { |
There was a problem hiding this comment.
[suggestion] This also propagates receipt.ErrNotConfigured (asserted by the new ErrNotConfigured subtest), which changes behavior on any node whose keeper has no receipt store: debug_traceTransaction / debug_traceStateAccess / debug_traceTransactionProfile previously fell through to the lookback check and now hard-fail with receipt store not configured.
If a nil receipt store is a supported deployment (rather than test-only), ErrNotConfigured should fall through like ErrNotFound does. If it isn't reachable in production, a one-line comment saying so would make the intent clear — right now the test reads as if it's a supported configuration being deliberately broken.
| return api.guardTraceRequest(ctx, endpoint, api.latestTraceHeight(ctx)) | ||
| } | ||
|
|
||
| func (api *DebugAPI) guardTraceCallRequest(ctx context.Context, endpoint string, height int64) error { |
There was a problem hiding this comment.
[nit] guardTraceCallRequest{,ByNumber,ByHash,ByNumberOrHash} are line-for-line duplicates of the guardTraceRequest* family above, differing only in which Ensure* they call. Per AGENTS.md's "guard at the choke point" guidance, this is the shape that invites the two families to drift — a future guard added to one is easy to forget in the other.
One family parameterized by the availability check would collapse eight methods to four:
type heightAvailability func(context.Context, int64) error
func (api *DebugAPI) guardTraceRequestByHash(ctx context.Context, endpoint string, hash common.Hash, ensure heightAvailability) errorwith TraceCall passing watermarks.EnsureTraceCallHeightAvailable and the replay endpoints passing EnsureTraceHeightAvailable.
There was a problem hiding this comment.
The retention guards are well-decomposed and the guard-before-semaphore reordering is the right fix for PLT-975; no correctness blockers found. Remaining notes are efficiency (3 redundant Watermarks()/Status calls per trace request, now unbounded pre-semaphore), duplication across six near-identical guard wrappers, one unreachable defensive check, and a new intermittent "not yet available" failure mode for concrete tip heights.
Findings: 0 blocking | 13 non-blocking | 6 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 synthesis merges only Claude's and Codex's findings. - Guard-before-semaphore intentionally reverses a previously-asserted invariant: the deleted
TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookupasserted "hash lookup should not happen before trace context setup." Now everydebug_traceBlockByHash/debug_traceCall-by-hash request performs a Tendermint block-by-hash lookup plus 3-4Statuscalls outsideMaxConcurrentTraceCalls. Spamming random hashes therefore drives unbounded concurrent blockstore work that the semaphore used to cap. Worth confirming another rate limit fronts the debug namespace, and ideally worth a comment recording that the ordering trade-off is deliberate so it isn't "fixed" back later. - New intermittent failure mode for concrete tip heights.
latestTraceHeightclampslatest/pending/safe/finalizedto the safe watermark, but the by-tx-hash path feedsrcpt.BlockNumberstraight intoEnsureTraceHeightAvailable. Sincelatestmins instateStore.GetLatestVersion()(async SS writes) whileeth_getTransactionReceipthas no watermark check, the common flow — send tx, poll for receipt, thendebug_traceTransaction— can return "requested height N is not yet available; safe latest is N-1" whenever SS lags a block. Erroring beats a silently-empty trace, but a bounded retry/short wait for heights within a block or two of the tip would keep that flow from flapping. - Partial migration off string matching:
evmrpc/block.go:367(eth_getBlockReceipts) still doesstrings.Contains(err.Error(), "not found")on a receipt lookup. It happens to keep working only becauseErrReceiptPruned's message ends in "receipt not found"; converting it toerrors.Is(err, receiptpkg.ErrNotFound)alongside the threetx.gosites would remove that accidental coupling. - Coverage gap in the reordering tests: the new tests assert pre-semaphore rejection for
TraceBlockByNumberandTraceBlockByHash, but the deleted test also coveredTraceCallby hash, and nothing replaces it.guardTraceCallRequestByNumberOrHash(the only caller ofEnsureTraceCallHeightAvailablein the endpoint path) has no test asserting it runs beforeprepareTraceContext. - No test asserts the pruned-height error actually surfaces through the JSON-RPC handler — all new tests call the guards or
DebugAPImethods directly. Given the point of the PR is the client-visible error, one handler-level case (pruned height in, explicit JSON-RPC error out) would pin the contract. - Coverage limit of the fix worth noting in the PR body:
ErrReceiptPrunedis only produced while litt still physically holds the value (lazy TTL). Once the value actually expires,GetReceiptreturnsErrNotFound, the tx-hash guard has no height to check, and the user gets "transaction not found" rather than a retention error for a tx that did exist. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| // for debug_trace* replay endpoints (transaction/block). Replay loads parent | ||
| // block validators and parent state (height-1) and reads receipts at the | ||
| // requested height. | ||
| func (m *WatermarkManager) EnsureTraceHeightAvailable(ctx context.Context, height int64) error { |
There was a problem hiding this comment.
[suggestion] This resolves watermarks three separate times per call: EnsureBlockHeightAvailable, ensureReplayParentBlockAvailable → EnsureBlockHeightAvailable, and EnsureStateHeightAvailable each invoke Watermarks(ctx), which is a Tendermint Status RPC plus ctxProvider + receipt-store + state-store reads — and genesisInitialHeight() is called twice on top. Because the guard now runs before the trace semaphore, that cost is paid on every trace request with no concurrency bound.
Taking one snapshot and validating all three legs against it would cut it to a single fetch and also remove the cross-call inconsistency (each Watermarks call can observe a different tip/earliest, so the parent-block leg can be evaluated against a snapshot the block leg never saw):
blockEarliest, stateEarliest, latest, err := m.Watermarks(ctx)
if err != nil {
return err
}
if err := ensureWithinWatermarks(height, blockEarliest, latest); err != nil {
return err
}
parent := max(height-1, m.genesisInitialHeight())
if err := ensureWithinWatermarks(parent, blockEarliest, latest); err != nil {
return err
}
// ... receipt leg, then state leg against stateEarliest/latestSame applies to EnsureTraceCallHeightAvailable, which resolves watermarks twice.
| return api.guardTraceRequest(ctx, endpoint, api.latestTraceHeight(ctx)) | ||
| } | ||
|
|
||
| func (api *DebugAPI) guardTraceCallRequest(ctx context.Context, endpoint string, height int64) error { |
There was a problem hiding this comment.
[suggestion] guardTraceCallRequest* and guardTraceRequest* are six functions whose bodies are identical apart from which Ensure* they call. AGENTS.md asks that a correction leave the code readable as a sequence of named steps rather than accumulating parallel copies; a single set parameterized by the availability check would halve this and make it impossible for a future endpoint to wire up the by-number variant of one family and the by-hash variant of the other:
type traceAvailability func(context.Context, int64) error
func (api *DebugAPI) traceReplayAvailability() traceAvailability { ... }
func (api *DebugAPI) traceCallAvailability() traceAvailability { ... }
func (api *DebugAPI) guardTraceHeight(ctx context.Context, endpoint string, height int64, ensure traceAvailability) error
func (api *DebugAPI) guardTraceByHash(ctx context.Context, endpoint string, hash common.Hash, ensure traceAvailability) error
func (api *DebugAPI) guardTraceByNumberOrHash(...) errorThe two Ensure* methods stay as the meaningful distinction, which is where the replay-vs-call difference actually lives.
| return api.guardTraceRequest(ctx, endpoint, height) | ||
| } | ||
|
|
||
| func (api *DebugAPI) guardTraceRequestByHash(ctx context.Context, endpoint string, hash common.Hash) error { |
There was a problem hiding this comment.
[suggestion] The nil check here is narrower than the one in guardTraceRequest just above: that one tolerates api.backend.watermarks == nil, but this path falls through to blockByHashRespectingWatermarks, which returns errNoHeightSource when wm == nil. So a DebugAPI with a backend but no watermark manager fails debug_traceBlockByHash outright with "unable to determine height information", where the old code swallowed that error and let the trace proceed. Production always wires watermarks (server.go:89/294), so this is latent rather than live, but the condition should match its sibling:
if api.backend == nil || api.backend.watermarks == nil || api.tmClient == nil {Same in guardTraceCallRequestByHash.
| continue | ||
| } | ||
| ethtx, _ := m.AsTransaction() | ||
| if ethtx == nil { |
There was a problem hiding this comment.
[suggestion] This guard can't fire. AsTransaction returns nil only when UnpackTxData(msg.Data) fails — and m.IsAssociateTx() three lines up calls GetAssociateTx, which panic(err)s on exactly that failure. So an undecodable EVM message panics before reaching here, and the PR description's "nil EVM tx in block filtering is skipped safely" doesn't hold for the case it targets.
If the goal is to survive an undecodable MsgEVMTransaction, the fix belongs in GetAssociateTx/IsAssociateTx (x/evm/types/message_evm_transaction.go:85) — return the decode failure instead of panicking, and let both call sites skip. If the panic is genuinely unreachable and this is belt-and-braces, dropping it keeps the loop honest about what it defends against.
| } | ||
|
|
||
| func (api *DebugAPI) guardHistoricalDebugTraceByNumber(ctx context.Context, endpoint string, number rpc.BlockNumber) error { | ||
| // latestTraceHeight resolves the height debug_trace* should use for latest-ish |
There was a problem hiding this comment.
[nit] Per AGENTS.md, godoc says what a thing is, not why it came to be or how it works inside — "It prefers ... since the tip can outrun ... and would otherwise make EnsureTraceHeightAvailable reject" is rationale, which belongs in an inline comment at the line that needs it. (Same class of thing in EnsureTraceCallHeightAvailable's and EnsureTraceHeightAvailable's godocs in watermark_manager.go, which describe the replay mechanism and name StateAndHeaderByNumberOrHash.) Suggest rewriting as a contract:
// latestTraceHeight returns the height debug_trace* resolves latest, pending,
// safe, and finalized to: the watermark's safe latest, or the app tip when no
// watermark is available.The "tip outruns the stores by a block" rationale is load-bearing, so keep it — as an inline comment on the LatestHeight call. (Finding also raised by Codex.)
| // 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 confirming the intent: for an empty tx list the old code returned ([], true), which short-circuited to the same [] the tracer produces for a block with no EVM txs — so this changes no result, it only stops counting a trivially-satisfiable assembly as a cache hit. The cost is that debug_traceBlockBy* on every empty block now falls through to a full replay setup instead of returning immediately, which on a chain with many empty blocks is a real latency regression for a metrics-honesty gain. If the goal is accurate hit accounting, keeping the fast path and not recording it as a hit would get both.
Superseded: latest AI review found no blocking issues.
…e test. Fetch watermarks once per EnsureTrace* guard to avoid redundant Status calls, restore debug_traceCall semaphore coverage, and document the guard-before-semaphore ordering. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid, well-tested change: retention guards now run before the trace semaphore, debug_trace* fails fast with explicit errors on pruned block/receipt/state data, and the brittle strings.Contains("not found") checks are replaced with sentinel-error matching. One real inconsistency remains — latest-ish tags resolve to the watermark's safe latest while the max-lookback guard still compares against the raw app tip — plus a few non-blocking notes about guard work now running outside the semaphore and trace timeout.
Findings: 0 blocking | 9 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review reflects only the Codex findings plus my own.- Hash-based endpoints now do the Tendermint block-by-hash lookup twice per request: once in
guardTraceRequestByHash/guardTraceCallRequestByHashand again intryBlockTraceCacheByHash/tracersAPI.TraceBlockByHash. Worth caching the resolved height on the request path if this shows up in latency. - Guard cost per by-hash request is two
Statussnapshots, not one as the description implies:blockByHashRespectingWatermarkscallsEnsureBlockHeightAvailable(→Watermarks) and thenguardTraceRequestcallsEnsureTraceHeightAvailable(→Watermarksagain). - The deleted
TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookupencoded the opposite invariant. The new ordering is covered forTraceBlockByHash(TestTraceBlockByHashChecksAvailabilityBeforeSemaphore), but there is no equivalent guard-before-semaphore test forTraceCallwith a block hash — only with a block number. Worth adding so the reversed invariant is pinned on both hash paths. - The known limitation in the description (fully TTL-reclaimed receipts are indistinguishable from an unknown tx hash, so
debug_traceTransactionproceeds into the trace path) is accurately stated and not blocking, but consider a follow-up issue so it doesn't get lost — it's the one remaining path where a pruned request still consumes a semaphore slot. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| switch number { | ||
| case rpc.SafeBlockNumber, rpc.FinalizedBlockNumber, rpc.LatestBlockNumber, rpc.PendingBlockNumber: | ||
| return api.ctxProvider(LatestCtxHeight).BlockHeight(), nil | ||
| return api.latestTraceHeight(ctx), nil |
There was a problem hiding this comment.
[suggestion] Latest-ish tags now resolve to the watermark's safe latest, but guardHistoricalDebugTraceHeight (line 236) still measures the lookback window against the raw app tip (api.ctxProvider(LatestCtxHeight).BlockHeight()). Whenever the receipt/state stores lag the tip — the exact condition latestTraceHeight exists to paper over — the two disagree by the lag:
- With
max_trace_lookback_blocks = 0("tip only"),debug_trace*("latest")resolves totip-1, andisHistoricalDebugTraceBlock(tip-1, tip, 0)is true → the request is rejected withblock number N is beyond max lookback of 0. That is the intermittent failure this PR set out to remove. - With the default 10000, a client that computes
eth_blockNumber - 10000(andeth_blockNumberalso returns the safe latest, seeevmrpc/info.go:49) asks forsafeLatest-10000, which is< tip-10000→ rejected at the documented boundary.
Suggest threading the already-resolved latest into the lookback guard (e.g. have guardHistoricalDebugTraceHeight take the latest it should compare against, defaulting to latestTraceHeight(ctx)) so both halves of the guard use one height. TestTraceLatestTagGuardMatchesBlockResolution would have caught this if it called guardTraceRequestByNumber instead of resolveDebugTraceBlockNumber directly — its DebugAPI already has maxBlockLookback at the zero value. (Also reported by Codex.)
| 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] Moving the guard ahead of prepareTraceContext is the right call for fail-fast, but note the guard work is now bounded by neither the trace semaphore nor traceTimeout: it runs on the caller's raw ctx, and blockByHashRespectingWatermarks(..., maxRetries: 1) sleeps a full second before retrying when the Tendermint client returns an error (an unknown hash short-circuits to ErrBlockNotFoundByHash without sleeping, so only genuine client errors hit this). Consider either creating the timeout context before the guard and cancelling it separately from semaphore release, or passing maxRetries: 0 on the guard path — the retry exists for the receipt/block commit race, which doesn't apply to a pure availability check.
|
|
||
| // EnsureStateHeightAvailable verifies that historical state for the given block | ||
| // height has not been pruned from the state store. | ||
| func (m *WatermarkManager) EnsureStateHeightAvailable(ctx context.Context, height int64) error { |
There was a problem hiding this comment.
[nit] EnsureStateHeightAvailable has no production caller — it's exercised only by watermark_manager_test.go. Either wire it into EnsureTraceCallHeightAvailable/EnsureTraceHeightAvailable (both currently inline the same ensureWithinWatermarks(height, stateEarliest, latest) check) or drop it, so the exported surface stays load-bearing.
| // tags. It prefers the watermark's safe latest over the raw app tip, since the | ||
| // tip can outrun the receipt/state stores by a block or so and would otherwise | ||
| // make EnsureTraceHeightAvailable reject the most common trace requests. | ||
| func (api *DebugAPI) latestTraceHeight(ctx context.Context) int64 { |
There was a problem hiding this comment.
[nit] The watermark error is swallowed and silently replaced with the raw app tip. On the guardTraceRequestByNumber path that's harmless — EnsureTraceHeightAvailable re-fetches and surfaces the same error. But on the guardTraceRequestByTxHash fallback (line 128) there's no re-check, so a Status failure quietly degrades the lookback guard to the unvalidated tip. Returning (int64, error) and letting callers propagate would make that explicit.
1 participant
Describe your changes and provide context
debug_trace*endpoints could silently return empty results or panic when block, receipt, or state data had been pruned. Guards also ran after the trace semaphore was acquired, so pruned requests could block on concurrency limits instead of failing immediately.This PR adds retention checks before trace work begins and returns explicit errors when data is unavailable:
EnsureTraceHeightAvailable— for replay endpoints (debug_traceTransaction,debug_traceBlockByNumber, etc.): verifies block, parent block (height−1 for validator/state replay), receipt, and state retention. Fetches watermarks once per guard (singleStatussnapshot).EnsureTraceCallHeightAvailable— fordebug_traceCall: verifies block and state only (no receipt check, since TraceCall never reads receipts). Same single-snapshot watermark fetch.ErrReceiptPruned— new sentinel wrappingErrNotFoundsoeth_getTransactionReceiptstill returnsnullfor pruned receipts while trace guards can distinguish pruned from missing.latestTraceHeight— resolveslatest/pending/safe/finalizedtags via the watermark's safe latest instead of the raw app tip, avoiding intermittent errors when receipts/state lag the tip by a block.Backend.BlockByNumber— uses sharedgetBlockNumber+ watermark resolution instead of a separateConvertBlockNumberpath;pendingnow resolves likelatestper EVM RPC spec instead of panicking.Known limitation: tx-by-hash trace guards need a resolvable receipt (or index); once a receipt is fully TTL-reclaimed,
ErrNotFoundis indistinguishable from an unknown hash and the request proceeds into the trace path before failing downstream.Fixes PLT-975.
Testing performed to validate your change
EnsureTraceHeightAvailable,EnsureTraceCallHeightAvailable, parent block floor, genesis initial height, SS-disabled edge cases)latesttag guard matching block resolution via safe latest watermarkErrReceiptPruned, store errors,ErrNotFoundfallthrough)ErrReceiptPrunedbelow retention floorTraceBlockByNumber,TraceBlockByHash,TraceCall)