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 15 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.49% 58.34% -1.15%
==========================================
Files 2325 2226 -99
Lines 198659 186690 -11969
==========================================
- Hits 118187 108924 -9263
+ Misses 69230 67471 -1759
+ Partials 11242 10295 -947
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 cc9a49b. 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.
| } | ||
| } | ||
| 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.
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.
| // 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.
| 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.
| 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.
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>
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)