Skip to content

fix(evmrpc): fail fast on pruned heights for debug_trace* (PLT-975) - #3907

Open
amir-deris wants to merge 19 commits into
mainfrom
amir/plt-975-fix-debug-trace-issues
Open

fix(evmrpc): fail fast on pruned heights for debug_trace* (PLT-975)#3907
amir-deris wants to merge 19 commits into
mainfrom
amir/plt-975-fix-debug-trace-issues

Conversation

@amir-deris

@amir-deris amir-deris commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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 (single Status snapshot).
  • EnsureTraceCallHeightAvailable — for debug_traceCall: verifies block and state only (no receipt check, since TraceCall never reads receipts). Same single-snapshot watermark fetch.
  • ErrReceiptPruned — new sentinel wrapping ErrNotFound so eth_getTransactionReceipt still returns null for pruned receipts while trace guards can distinguish pruned from missing.
  • latestTraceHeight — resolves latest/pending/safe/finalized tags via the watermark's safe latest instead of the raw app tip, avoiding intermittent errors when receipts/state lag the tip by a block.
  • Guard ordering — retention checks intentionally run before the trace semaphore so pruned requests fail fast without consuming a slot; hash-based endpoints may perform Tendermint block lookups outside the concurrency limiter as part of those guards (reverses the previous semaphore-first invariant).
  • Backend.BlockByNumber — uses shared getBlockNumber + watermark resolution instead of a separate ConvertBlockNumber path; pending now resolves like latest per EVM RPC spec instead of panicking.
  • Minor fixes — empty tx list no longer reports a block trace cache hit (avoids serving a stale empty result when tx enumeration failed); nil EVM tx in block filtering is skipped safely.

Known limitation: tx-by-hash trace guards need a resolvable receipt (or index); once a receipt is fully TTL-reclaimed, ErrNotFound is 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

  • Added unit tests for watermark trace guards (EnsureTraceHeightAvailable, EnsureTraceCallHeightAvailable, parent block floor, genesis initial height, SS-disabled edge cases)
  • Added integration-style tests for trace endpoints rejecting pruned heights before semaphore acquisition
  • Added tests for latest tag guard matching block resolution via safe latest watermark
  • Added tests for receipt lookup error propagation (ErrReceiptPruned, store errors, ErrNotFound fallthrough)
  • Updated receipt store tests to expect ErrReceiptPruned below retention floor
  • Updated trace semaphore ordering tests (TraceBlockByNumber, TraceBlockByHash, TraceCall)

amir-deris and others added 8 commits August 10, 2026 17:21
…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 amir-deris self-assigned this Aug 12, 2026
@amir-deris amir-deris changed the title Amir/plt 975 fix debug trace issues fix(evmrpc): fail fast on pruned heights for debug_trace* (PLT-975) Aug 12, 2026
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 19, 2026, 4:40 PM

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.32%. Comparing base (6debd90) to head (4bd63f4).

Files with missing lines Patch % Lines
evmrpc/tracers.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3907      +/-   ##
==========================================
- Coverage   58.61%   58.32%   -0.29%     
==========================================
  Files        2323     2225      -98     
  Lines      198626   185737   -12889     
==========================================
- Hits       116428   108339    -8089     
+ Misses      71445    67180    -4265     
+ Partials    10753    10218     -535     
Flag Coverage Δ
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
evmrpc/simulate.go 76.08% <ø> (-0.90%) ⬇️
evmrpc/trace_profile.go 67.41% <ø> (+1.48%) ⬆️
evmrpc/tx.go 82.39% <ø> (+1.56%) ⬆️
evmrpc/utils.go 73.55% <ø> (+0.83%) ⬆️
evmrpc/watermark_manager.go 86.44% <ø> (-0.12%) ⬇️
sei-db/ledger_db/receipt/litt_receipt_store.go 59.56% <ø> (-9.24%) ⬇️
sei-db/ledger_db/receipt/receipt_store.go 66.66% <ø> (-0.34%) ⬇️
evmrpc/tracers.go 71.59% <50.00%> (+0.63%) ⬆️

... and 330 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@amir-deris
amir-deris marked this pull request as ready for review August 12, 2026 13:02
@cursor

cursor Bot commented Aug 12, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches debug/trace RPC paths, receipt retention semantics, and guard ordering; behavior changes for pruned data and latest-tag resolution are user-visible but covered by extensive tests.

Overview
debug_trace* now validates block, receipt, and state retention before acquiring the trace semaphore, returning explicit errors when data is pruned instead of empty traces or blocking on concurrency limits.

Replay endpoints (debug_traceTransaction, block traces, etc.) use EnsureTraceHeightAvailable (block, parent block, receipts, state at height−1). debug_traceCall uses EnsureTraceCallHeightAvailable (block and state only—no receipt check). Guards are unified under guardTrace* with an endpoint-specific ensure function; latest/safe/finalized resolve via latestTraceHeight (watermark safe latest, not the raw app tip).

The receipt layer adds ErrReceiptPruned (wrapped in watermark and litt store errors) so trace guards can distinguish pruned receipts from missing txs while eth_getTransactionReceipt still returns null for ErrNotFound. Missing receipts are detected with errors.Is instead of substring checks.

Backend.BlockByNumber drops ConvertBlockNumber in favor of shared getBlockNumber + watermark resolution. Block trace cache no longer treats an empty tx list as a hit. ErrReceiptPruned propagates through GetReceipt on the litt store.

Reviewed by Cursor Bugbot for commit ff4e273. Bugbot is set up for automated code reviews on this repo. Configure here.

@amir-deris
amir-deris requested a review from masih August 12, 2026 13:07
Comment thread evmrpc/watermark_manager.go Outdated
@amir-deris
amir-deris requested a review from bdchatham August 12, 2026 13:07

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md is empty), so this review reflects only the Codex pass plus my own analysis.
  • Guard/semaphore ordering is deliberately inverted: the deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup (and its panicHashLookupClient) encoded the invariant that no Tendermint hash lookup happens before the trace semaphore is acquired. Now every debug_traceBlockByHash/debug_traceCall performs a block-by-hash index lookup plus 3-4 Status calls 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_traceCall no longer has any test asserting it returns errTraceConcurrencyLimit when the semaphore is full (TestTraceBlockByNumberRejectsConcurrencyLimitAfterGuard only covers TraceBlockByNumber).
  • Guard duplication: guardTraceRequest{,ByNumber,ByHash,ByNumberOrHash} and guardTraceCallRequest{,ByNumber,ByHash,ByNumberOrHash} are eight methods that are byte-identical except for the terminal EnsureTrace…HeightAvailable call. 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 the ensure 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:358 still uses strings.Contains(err.Error(), "not found") on a receipt lookup while this PR converted the three sibling call sites in tx.go to errors.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: TestEnsureTraceCallHeightAvailableIgnoresReceipts and TestTraceReceiptFloorBoundary (both in historical_debug_trace_test.go) build the same fixture and largely assert the same thing; and in TestEnsureTraceCallHeightAvailable, rs.earliest = 150 is a no-op since the fake was constructed with earliest: 150.
  • 8 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread evmrpc/tracers.go
Comment thread evmrpc/watermark_manager.go
Comment thread evmrpc/watermark_manager.go Outdated
Comment thread evmrpc/tracers.go
Comment thread evmrpc/tracers.go
Comment thread evmrpc/tracers.go
Comment thread evmrpc/utils.go
Comment thread evmrpc/simulate.go
seidroid[bot]
seidroid Bot previously requested changes Aug 13, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md is empty — the Cursor pass produced no output, so this review reflects only the Codex finding (confirmed, see the inline comment on watermark_manager.go:237) plus my own.
  • No test covers a trace at the chain's initial height or the earliest block tag. TestEnsureTraceHeightAvailableParentBlockFloor pins the pruned-floor case at height 150 but never the genesis case, which is exactly the gap that lets the max(height-1, 0) bug through. Worth adding: EarliestBlockHeight: 1, EnsureTraceHeightAvailable(ctx, 1) → expect no error.
  • TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup covered both TraceBlockByHash and TraceCall; the three replacement tests cover TraceBlockByNumber and TraceBlockByHash only. debug_traceCall now has no test asserting its guard runs before the semaphore.
  • In guardTraceRequestByHash (tracers.go:157) and guardTraceCallRequestByHash (tracers.go:198), the block == nil || block.Block == nil branch is unreachable: blockByHashRespectingWatermarks already dereferences block.Block.Height before 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.

Comment thread evmrpc/watermark_manager.go Outdated
Comment thread evmrpc/tracers.go Outdated
Comment thread evmrpc/watermark_manager.go
Comment thread evmrpc/tracers.go
Comment thread evmrpc/tracers.go
Comment thread evmrpc/tracers.go Outdated

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md is empty), so this synthesis merges only Claude's and Codex's findings.
  • Guard-before-semaphore intentionally reverses a previously-asserted invariant: the deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup asserted "hash lookup should not happen before trace context setup." Now every debug_traceBlockByHash/debug_traceCall-by-hash request performs a Tendermint block-by-hash lookup plus 3-4 Status calls outside MaxConcurrentTraceCalls. 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. latestTraceHeight clamps latest/pending/safe/finalized to the safe watermark, but the by-tx-hash path feeds rcpt.BlockNumber straight into EnsureTraceHeightAvailable. Since latest mins in stateStore.GetLatestVersion() (async SS writes) while eth_getTransactionReceipt has no watermark check, the common flow — send tx, poll for receipt, then debug_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 does strings.Contains(err.Error(), "not found") on a receipt lookup. It happens to keep working only because ErrReceiptPruned's message ends in "receipt not found"; converting it to errors.Is(err, receiptpkg.ErrNotFound) alongside the three tx.go sites would remove that accidental coupling.
  • Coverage gap in the reordering tests: the new tests assert pre-semaphore rejection for TraceBlockByNumber and TraceBlockByHash, but the deleted test also covered TraceCall by hash, and nothing replaces it. guardTraceCallRequestByNumberOrHash (the only caller of EnsureTraceCallHeightAvailable in the endpoint path) has no test asserting it runs before prepareTraceContext.
  • No test asserts the pruned-height error actually surfaces through the JSON-RPC handler — all new tests call the guards or DebugAPI methods 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: ErrReceiptPruned is only produced while litt still physically holds the value (lazy TTL). Once the value actually expires, GetReceipt returns ErrNotFound, 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.

Comment thread evmrpc/watermark_manager.go
Comment thread evmrpc/tracers.go Outdated
Comment thread evmrpc/tracers.go Outdated
Comment thread evmrpc/utils.go
Comment thread evmrpc/tracers.go
Comment thread evmrpc/tracers.go
@seidroid
seidroid Bot dismissed their stale review August 13, 2026 11:56

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>

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md is 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/guardTraceCallRequestByHash and again in tryBlockTraceCacheByHash / 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 Status snapshots, not one as the description implies: blockByHashRespectingWatermarks calls EnsureBlockHeightAvailable (→ Watermarks) and then guardTraceRequest calls EnsureTraceHeightAvailable (→ Watermarks again).
  • The deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup encoded the opposite invariant. The new ordering is covered for TraceBlockByHash (TestTraceBlockByHashChecksAvailabilityBeforeSemaphore), but there is no equivalent guard-before-semaphore test for TraceCall with 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_traceTransaction proceeds 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.

Comment thread evmrpc/tracers.go
Comment thread evmrpc/tracers.go Outdated
Comment thread evmrpc/watermark_manager.go
Comment thread evmrpc/tracers.go

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread evmrpc/tracers.go
amir-deris and others added 5 commits August 14, 2026 11:57
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>
@amir-deris

Copy link
Copy Markdown
Contributor Author

@seidroid review

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_traceTransaction flow. guardTraceByTxHash feeds rcpt.BlockNumber straight into EnsureTraceHeightAvailable, whose latest mins in stateStore.GetLatestVersion(), while eth_getTransactionReceipt applies no watermark check. When SS lags the tip by a block the trace now fails with requested height N is not yet available; safe latest is N-1 where 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 DebugAPI methods 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.
  • EnsureTraceHeightAvailable computes max(height-1, m.genesisInitialHeight()) twice — once inside ensureReplayParentBlockAvailable, once for stateHeight — so genesisInitialHeight() 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.
  • EnsureReceiptHeightAvailable now wraps receipt.ErrReceiptPruned (itself "receipt pruned: %w" ErrNotFound), so the five eth_getBlock* / eth_getBlockReceipts call sites in block.go surface requested height 100 receipts have been pruned; earliest available is 150: receipt pruned: receipt not found to clients. The sentinel is the right mechanism; consider making ErrReceiptPruned's own text terse (e.g. errors.New on a bare marker, or %w with 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) calls panic(err) when UnpackTxData fails, and IsAssociateTx reaches it on every message. Any undecodable EVM message in a block therefore panics inside filterTransactions / Backend.BlockByNumber before 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.

Comment thread evmrpc/tracers.go
if returnErr = api.validateTraceTracer(config); returnErr != nil {
return nil, returnErr
}
if returnErr = api.guardTraceByHash(ctx, "debug_traceBlockByHash", hash, api.ensureTraceHeightAvailable); returnErr != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 guardTraceByHashblockByHashRespectingWatermarks(..., 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.

Comment thread evmrpc/tracers.go
// 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread evmrpc/tracers.go
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Two things in this function, both carried over from the previous pass:

  1. The nil check is narrower than its siblings, and now fails harder. ensureTraceHeightAvailable / ensureTraceCallHeightAvailable (lines 119-131) treat api.backend.watermarks == nil as "skip the guard", but this branch only tests api.backend == nil || api.tmClient == nil. With a backend but no watermark manager, control reaches blockByHashRespectingWatermarks, which returns errNoHeightSource — and since this revision propagates that error instead of swallowing it, debug_traceBlockByHash / debug_traceCall-by-hash now hard-fail with unable to determine height information where 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 {
  1. The block == nil || block.Block == nil branch below is unreachable. blockByHashWithRetry dereferences blockRes.Block and returns ErrBlockNotFoundByHash before ever returning a nil error with a nil block (evmrpc/utils.go:178), so fmt.Errorf("block %s not found", ...) can never be produced. Dropping it keeps the function honest about what it defends against.


// 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] EnsureStateHeightAvailable still has no production caller — grep finds only watermark_manager_test.go. Both trace guards inline the same ensureWithinWatermarks(height, stateEarliest, latest) check against their own snapshot rather than calling it (and they must, since they need the single-snapshot property). Either drop it, or give it a caller, so the exported surface stays load-bearing.

Comment thread evmrpc/tracers.go
// 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The watermark error is still swallowed and silently replaced with the raw app tip. On the guardTraceByNumber path that is harmless — ensureTraceHeightAvailable re-fetches and surfaces the same error. But this revision also routes guardHistoricalDebugTraceHeight (line 220) and the guardTraceByTxHash fallback (line 153) through it, and neither re-checks: a Status failure quietly degrades the lookback guard to an unvalidated tip. Returning (int64, error) and letting callers propagate would make that explicit; a comment stating why the tip is a safe substitute would at least record the intent.

Comment thread evmrpc/tracers.go
return api.guardHistoricalDebugTraceHeight(ctx, endpoint, api.latestTraceHeight(ctx))
}

// latestTraceHeight resolves the height debug_trace* should use for latest-ish

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Per AGENTS.md, a 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.

// 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 — keep it, as an inline comment on the LatestHeight call.

Comment thread evmrpc/utils.go
continue
}
ethtx, _ := m.AsTransaction()
if ethtx == nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This guard can't fire, and my earlier suggestion to test the second return value was wrong — AsTransaction() returns (*ethtypes.Transaction, ethtx.TxData), not an error, so _ is the tx data.

The reachability point stands: 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 (x/evm/types/message_evm_transaction.go:86). An undecodable EVM message panics before reaching here, so the PR description's "nil EVM tx in block filtering is skipped safely" doesn't hold for the case it targets. simulate.go:454 already carries the same unreachable guard, so this makes the two consistent rather than fixing anything.

If the goal is to survive an undecodable MsgEVMTransaction, the fix belongs in GetAssociateTx — return the decode failure instead of panicking, and let both call sites skip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant