-
Notifications
You must be signed in to change notification settings - Fork 594
Enable trace methods #2294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
tmeinlschmidt
wants to merge
18
commits into
0xPolygon:develop
Choose a base branch
from
tmeinlschmidt:enable-trace-methods
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Enable trace methods #2294
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
fb8c82e
chore: ignore local env files and Claude local overrides
a7fa1ae
feat(tracers): add Parity/OpenEthereum trace_* RPC methods
85954a3
feat(server): gate the trace RPC namespace behind --rpc.enabletrace
c3456e4
docs: document trace_* API flag and known limitations in README
81c2c4f
eth/tracers: add TLOAD to vmTraceOpPushCount return-1 set
2c200b1
docs: fix TOML casing for enabletrace in README example
b7a77c8
eth/tracers: restore 5s defaultTraceTimeout, add 60s parity-specific …
d7cfef3
eth/tracers: use active precompile set for trace filter, covering BLS…
fd201ce
eth/tracers: fix comment — non-revert error emits result null, not om…
122845d
eth/tracers: fix potential integer overflow in buildParityStateDiff m…
f9ac0ab
docs: regenerate CLI docs to include rpc.enabletrace flag
4576885
eth/tracers: update tests for precompile-set parameter in convertCall…
6166712
eth/tracers: add tests for pruned node, underfunded sender, and trace…
505c9eb
eth/tracers: fix SonarCloud quality gate issues
6c0cd6f
eth/tracers: reduce duplication — parityExecInput struct and buildRep…
b99944c
eth/tracers: dedupe Parity block-replay setup into shared helper
c1123a3
eth/tracers: restructure parity code for CI quality gates
13b462b
eth/tracers: dedupe parity test harnesses into shared helpers
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The README's
--rpc.evmtimeout ≥ 60s recommendedline under the trace namespace is misleading:--rpc.evmtimeoutmaps toRPCEVMTimeout, which is only read byeth_call/eth_simulate/ graphql — never by anything undereth/tracers/. Setting it has no effect ontrace_*methods (they use their owndefaultParityTraceTimeout = 60sinternally). Consider dropping the line entirely (the 60s trace-phase default already gives operators what the README claims to want), or replacing it with the knob that actually bounds a long trace call's wall-clock — the HTTP server write timeout (--http.timeout.write, default 30s).Extended reasoning...
What is wrong
README.md:69 tells operators enabling the new Parity trace namespace to set
--rpc.evmtimeout ≥ 60s. That flag is wired toethconfig.RPCEVMTimeoutand is consumed only by:internal/ethapi/api.go(DoCall—eth_call,eth_estimateGas)internal/ethapi/simulate.go(eth_simulate)graphql/graphql.gogrep -r RPCEVMTimeout eth/tracers/returns zero hits. Neitherdebug_trace*nor the newtrace_*methods read it.How the trace path actually times out
The tracers ship their own constants:
eth/tracers/api.go:54—defaultTraceTimeout = 5 * time.Second(used bydebug_trace*when the caller does not setTraceConfig.Timeout).eth/tracers/parity.go:28—defaultParityTraceTimeout = 60 * time.Second(used byparityTraceConfigfor everytrace_*call, since the Parity RPC methods do not exposeTimeoutto the caller).So the trace phase is already capped at 60s internally, regardless of
--rpc.evmtimeout. The only external knob that governs a longtrace_*call is the HTTP server write timeout (--http.timeout.write, default 30s), which will cut the RPC response off well before the 60s tracer cap on default settings.Step-by-step proof
bor server --rpc.enabletrace --rpc.evmtimeout 60s --gcmode archive.--rpc.evmtimeout 60ssetsJsonRPC.RPCEVMTimeout = 60sin the config, threaded intoethconfig.Config.RPCEVMTimeout.trace_replayBlockTransactionsfor a heavy historical block.TraceAPI.ReplayBlockTransactions→replayBlockTransactions→buildReplayResult→parityTraceTx→traceTx(cfg = parityTraceConfig(userCfg)).parityTraceConfig(parity.go:184),cfg.Timeoutis set todefaultParityTraceTimeout.String()(60s).RPCEVMTimeoutis not read anywhere on this path.traceTxthe timeout comes fromcfg.Timeout— the 60s constant — not fromapi.b.RPCEVMTimeout().jsonrpc.timeouts.write); if the reply is not written in time the client sees a broken connection well before the 60s tracer cap.The operator has set a flag that changes literally nothing about their trace timeouts.
Impact
Documentation-only. The trace namespace works correctly out of the box because the internal 60s default is already what the README claims to be recommending. Nothing crashes, no wrong data is returned; the advice is simply a no-op for the namespace it targets. The practical harm is that an operator debugging a hung
trace_*call will look at the flag they set (--rpc.evmtimeout) instead of the flag that actually matters (--http.timeout.write).How to fix
Two good options in README.md (and in the PR description, which repeats the same line under "Recommended companion flags"):
--rpc.evmtimeoutbullet entirely. The internaldefaultParityTraceTimeout = 60salready gives the intended behavior.--http.timeout.write(default 30s), which is the real ceiling on a longtrace_*call. Suggested wording: "Sufficient HTTP write timeout for deep blocks (--http.timeout.write≥ 60s recommended, since heavy trace_replay* calls can take longer than the 30s default before the internal 60s trace cap trips)."Severity
nit — documentation error, not a code-correctness bug. Merging as-is does not break the feature. Worth fixing before release notes go out so operators are not chasing a no-op flag.