fix(evmrpc): honor trace_timeout in profiled debug_traceBlock path (PLT-989) - #3914
Conversation
…LT-989) Stop CosmWasm metadata replay loops from ignoring the trace context after timeout, so parallelized block traces release their semaphore slot instead of running until restart. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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 #3914 +/- ##
==========================================
- Coverage 59.48% 58.49% -1.00%
==========================================
Files 2325 2229 -96
Lines 198647 188027 -10620
==========================================
- Hits 118160 109977 -8183
+ Misses 69258 67655 -1603
+ Partials 11229 10395 -834
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
Tests cover sequential (metadata + EVM-only) and parallel metadata loops, plus export test hooks for the parallel path. Reviewed by Cursor Bugbot for commit e921de0. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
The fix is correct and minimal: the ctx reaching profiledTraceBlock* is the prepareTraceContext timeout context, and the new per-iteration ctx.Err() checks match the existing ReplayTransactionTillIndex pattern, closing the gap where md.TraceRunnable replays kept running past trace_timeout. No blockers; the notes concern the untested parallel half of the diff and the divergent timeout return semantics between the sequential and parallel paths.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage covers only
profiledTraceBlockSequential. Both new checks inprofiledTraceBlockParallel(EVM-only and metadata branches) are untested — roughly half the behavioral change. The parallel path is reachable whenevermin(NumCPU, tracedCount) > 1, i.e. the common case on real nodes, so it is the path most requests actually take; it is also the one with the surprising return contract (partialresults, nil), so a test pinning that is worth more than the sequential ones. - On cancellation the parallel path fills untraced slots with
"state advancement failed at prior tx: %v"(block_trace_profiled.go:301 and :312 — unchanged lines, so not inline-anchorable). That message is now emitted forcontext.DeadlineExceeded, where nothing about state advancement failed; an operator debugging atrace_timeoutsees a misattributed error string. Consider branching the message onerrors.Is(failed, context.DeadlineExceeded) || errors.Is(failed, context.Canceled). - Second-opinion passes: Codex reported "No material findings in the PR diff."
cursor-review.mdis empty — the Cursor pass produced no output, so treat this review as covering Claude + Codex only. - Nit:
testProfiledTraceBlock(block_trace_profiled_test.go:77) is a fixture builder, not a test; a name likenewProfiledTraceBlockFixturereads better next to theTestProfiledTraceBlock*functions it sits above. - Note: I could not execute
go test ./evmrpc/orgo vetin this environment (command approval denied), so the findings below are from static reading of the diff and surrounding code, not from a test run. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| block = gethtypes.NewBlock(block.Header(), body, nil, trie.NewStackTrie(nil)) | ||
|
|
||
| ctx, cancel := context.WithCancel(t.Context()) | ||
| cancel() |
There was a problem hiding this comment.
[nit] Cancelling before the call means this only exercises the guard on iteration 0 — it proves the loop never starts, not that it stops. The scenario the PR is fixing is cancellation mid-loop, which the metadata test above covers properly via cancel() inside the first TraceRunnable.
Also worth noting: the pre-fix failure mode here is a nil-interface panic in profiledTraceTx (statedb.GetNonce on the nil passed at line 144) rather than a clean assertion failure. Both points are addressed by giving iteration 0 something real to do and cancelling after it — e.g. a stub statedb — so the test fails on require.ErrorIs rather than on a panic.
… (PLT-989) Return a top-level RPC error from profiledTraceBlockParallel on block-level timeout/cancellation so callers get consistent behavior regardless of CPU count. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Follow-up to PLT-986 for the opt-in profiled
debug_traceBlock*path (evm.enable_parallelized_block_trace = true, default struct logger).When tracing CosmWasm-heavy blocks, non-EVM txs are replayed via
TraceBlockMetadata.TraceRunnable→DeliverTx. The profiled loops did not checkctx.Err()between metadata iterations, so aftertrace_timeoutthe handler could keep replaying Cosmos/Wasm txs and hold a trace semaphore slot until restart.This adds per-iteration context checks in
profiledTraceBlockSequentialandprofiledTraceBlockParallel, matching the pattern used in go-ethereum PR #94 andReplayTransactionTillIndex. Both paths now return a top-level RPC error on block-level timeout/cancellation (nil, ctx.Err()), so behavior no longer depends on CPU count or traced-tx count.Changes
profiledTraceBlockSequential: return onctx.Err()at the start of each EVM-only and metadata-replay iterationprofiledTraceBlockParallel: break onctx.Err()at the start of each iteration; propagatecontext.DeadlineExceeded/context.Canceledas a top-level RPC error (same as sequential) instead of partialresults, nilblock_trace_profiled_test.goblock_trace_profiled_export_test.go(via export test hooks inexport_test.go)Out of scope
DeliverTxalready blocked inside Wasm executionTest plan
go test ./evmrpc/ -run ProfiledTraceBlock -count=1References