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 20 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3907 +/- ##
==========================================
- Coverage 58.97% 57.88% -1.10%
==========================================
Files 2310 2211 -99
Lines 197500 185882 -11618
==========================================
- Hits 116481 107593 -8888
+ Misses 70261 68479 -1782
+ Partials 10758 9810 -948
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 ( Adds Also: Reviewed by Cursor Bugbot for commit dfde9ff. 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}
![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.
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.
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.
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.
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 b6dd4fc. Configure here.
Use latestTraceHeight in the max-lookback check so debug_trace*("latest")
is not rejected when receipt/state stores lag the app tip.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
This revision resolves the substantive findings from earlier passes — the guard families are now collapsed behind a single ensureTraceHeightFunc, guardHistoricalDebugTraceHeight measures lookback against the same safe latest the tags resolve to, the parent-block floor uses genesisInitialHeight(), and each guard takes one Watermarks snapshot. What remains is non-blocking: guard work still runs outside the semaphore and trace timeout (including a 1s uninterruptible retry sleep), the empty-block trace cache fast path is still dropped, and a few dead/latent branches and doc nits persist.
Findings: 0 blocking | 12 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- New intermittent failure on the send-tx → poll-receipt →
debug_traceTransactionflow.guardTraceByTxHashfeedsrcpt.BlockNumberstraight intoEnsureTraceHeightAvailable, whoselatestmins instateStore.GetLatestVersion(), whileeth_getTransactionReceiptapplies no watermark check. When SS lags the tip by a block the trace now fails withrequested height N is not yet available; safe latest is N-1where it previously proceeded. Erroring beats a silent empty trace, but a short bounded retry for heights within a block or two of the tip would keep the common client flow from flapping. - No test asserts the pruned-height error surfaces through the JSON-RPC handler. Every new test calls the guards or
DebugAPImethods directly; since the client-visible error is the point of PLT-975, one handler-level case (pruned height in → explicit JSON-RPC error out) would pin the contract. EnsureTraceHeightAvailablecomputesmax(height-1, m.genesisInitialHeight())twice — once insideensureReplayParentBlockAvailable, once forstateHeight— sogenesisInitialHeight()is resolved twice per guard for a value both legs share. Hoisting it once above the two checks removes the duplication and makes it obvious the parent block and parent state use the same floor.EnsureReceiptHeightAvailablenow wrapsreceipt.ErrReceiptPruned(itself"receipt pruned: %w" ErrNotFound), so the fiveeth_getBlock*/eth_getBlockReceiptscall sites inblock.gosurfacerequested height 100 receipts have been pruned; earliest available is 150: receipt pruned: receipt not foundto clients. The sentinel is the right mechanism; consider makingErrReceiptPruned's own text terse (e.g.errors.Newon a bare marker, or%wwith an empty prefix) so the user-visible message doesn't restate "pruned" and "not found" three times.- 7 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
MsgEVMTransaction.GetAssociateTx(x/evm/types/message_evm_transaction.go:86) callspanic(err)whenUnpackTxDatafails, andIsAssociateTxreaches it on every message. Any undecodable EVM message in a block therefore panics insidefilterTransactions/Backend.BlockByNumberbefore any nil-tx guard can run, so decode failures cannot be handled gracefully by callers. Returning the error and letting both call sites skip would close it at the source.
| if returnErr = api.validateTraceTracer(config); returnErr != nil { | ||
| return nil, returnErr | ||
| } | ||
| if returnErr = api.guardTraceByHash(ctx, "debug_traceBlockByHash", hash, api.ensureTraceHeightAvailable); returnErr != nil { |
There was a problem hiding this comment.
[suggestion] Still unaddressed from the previous pass (the new prepareTraceContext godoc records the trade-off, which is good, but the mechanics are unchanged): this guard runs on the caller's raw ctx, so it is bounded by neither MaxConcurrentTraceCalls nor TraceTimeout.
The concrete cost is inside guardTraceByHash → blockByHashRespectingWatermarks(..., maxRetries: 1) → blockByHashWithRetry, which does time.Sleep(1 * time.Second) (evmrpc/utils.go:171) when the Tendermint client returns an error. time.Sleep ignores context cancellation, so a burst of debug_traceBlockByHash against a struggling client parks one goroutine per request for a full second with no concurrency cap — work the semaphore used to bound.
An unknown hash short-circuits to ErrBlockNotFoundByHash without sleeping, so only genuine client errors hit this. Two small options: pass maxRetries: 0 on the guard path (the retry exists for the receipt/block commit race, which does not apply to a pure availability check), or create the timeout context before the guard and cancel it separately from semaphore release so the guard at least inherits TraceTimeout.
| // 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] Still present after three passes, and Codex flags it again this round.
[] is the correct trace result for a block with no EVM transactions, and the old ([], true) returned it without touching the replay path. Now every debug_traceBlockBy{Number,Hash} on an empty block falls through to tracersAPI.TraceBlockBy* — parent state load, block init, the whole replay setup — to produce the same []. On a chain with sub-second blocks, empty blocks are common enough for this to be a measurable latency regression. (tryBlockResultCache above only covers it when a block-level entry was previously written.)
If the concern is that an empty txHashes can also mean "tx enumeration failed" rather than "block is genuinely empty", that ambiguity lives at the call sites (txHashesOf(block.Transactions()) in tryBlockTraceCacheBy{Number,Hash}), where the block is in hand and the two cases are distinguishable. Resolving it there keeps the fast path for genuinely empty blocks. If the goal is only honest hit accounting, keep the fast path and simply don't count it as a hit.
| func (api *DebugAPI) guardTraceByHash(ctx context.Context, endpoint string, hash common.Hash, ensure ensureTraceHeightFunc) error { | ||
| if api.backend == nil || api.tmClient == nil { | ||
| return nil | ||
| return api.guardTrace(ctx, endpoint, api.latestTraceHeight(ctx), ensure) |
There was a problem hiding this comment.
[suggestion] Two things in this function, both carried over from the previous pass:
- The nil check is narrower than its siblings, and now fails harder.
ensureTraceHeightAvailable/ensureTraceCallHeightAvailable(lines 119-131) treatapi.backend.watermarks == nilas "skip the guard", but this branch only testsapi.backend == nil || api.tmClient == nil. With a backend but no watermark manager, control reachesblockByHashRespectingWatermarks, which returnserrNoHeightSource— and since this revision propagates that error instead of swallowing it,debug_traceBlockByHash/debug_traceCall-by-hash now hard-fail withunable to determine height informationwhere the old code let the trace proceed. Production always wires watermarks (server.go), so this is latent rather than live, but the condition should match its siblings:
if api.backend == nil || api.backend.watermarks == nil || api.tmClient == nil {- The
block == nil || block.Block == nilbranch below is unreachable.blockByHashWithRetrydereferencesblockRes.Blockand returnsErrBlockNotFoundByHashbefore ever returning a nil error with a nil block (evmrpc/utils.go:178), sofmt.Errorf("block %s not found", ...)can never be produced. Dropping it keeps the function honest about what it defends against.
|
cancelling this PR as the issue with debug trace was resolved by another PR. |
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)