feat(debug-trace-server): erc7562Tracer support; no-record preceding replay - #186
feat(debug-trace-server): erc7562Tracer support; no-record preceding replay#186flyq wants to merge 1 commit into
Conversation
… without recording erc7562Tracer becomes the sixth supported builtin: a TracerKind variant built on revm-inspectors' from_geth_erc7562_config / geth_erc7562_traces, response- cached under a parsed-config key (the ignored-opcode list folds into a 256-bit set so permutations collapse onto one entry), classified/gated like the other config-reading builtins (null config defaults, type-malformed rejected with -32602 before any block data is fetched). The Unsupported request shape from the upgrade branch is gone with its last user. debug_traceTransaction's preceding-transaction replay stops recording traces it discards: the JS path seeds a hook-less tracer (no step/enter/exit — the user's JS runs only for the target transaction) and the mux path seeds an empty mux inspector, matching the none-config TracingInspector the other tx paths already use.
Claude review status
Last reviewed: head New this round: 1 finding(s), 1 question(s) · Resolved this round: 0 · Open questions: 1 Open questions awaiting an answer:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf9540a212
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let replay_inspector = js_inspector( | ||
| "{result: function() { return null; }, fault: function() {}}".to_string(), | ||
| serde_json::Value::Null, | ||
| make_tx_ctx(&info), | ||
| )?; | ||
| setup_executor!(&env, &mut state, replay_inspector => executor); |
There was a problem hiding this comment.
For debug_traceTransaction with a JS tracer and tx_index > 0, this now constructs only the stub replay inspector before replay_preceding_txs!, so the user-supplied JS is not parsed until after all preceding transactions have executed. If that replay hits a witness/data error, an actually malformed JS request is reported as TraceError::Data and can evict the block data; even on healthy data, invalid JS spends the full replay cost before being rejected. Construct the real JsInspector up front (as before), then install it after the no-record replay.
Useful? React with 👍 / 👎.
| /// onto one cache entry. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)] | ||
| pub struct Erc7562ConfigKey { | ||
| stack_top_items_size: Option<u64>, |
There was a problem hiding this comment.
When debug_traceBlock* requests use erc7562Tracer, callers can vary stackTopItemsSize to produce a distinct ResponseVariant here, but the revm-inspectors 0.40.1 path we invoke (from_geth_erc7562_config/geth_erc7562_traces) never reads that field, so the serialized trace is identical. That lets one block fill the response cache with duplicate large entries and defeats the intended equivalent-config key space; omit this field from the cache key until it affects output, or bypass the shape.
Useful? React with 👍 / 👎.
![mega-maxwell[bot]](https://avatars.githubusercontent.com/u/138558126?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 blocking · 0 should-fix · 1 suggestion(s) · 1 open question(s)
Reviewed head cf9540a2.
Findings without inline anchors:
bin/debug-trace-server/src/tracing_executor.rs:821— [Minor] Malformed JS tracer wastes preceding-tx replay before rejection For anydebug_traceTransactionrequest with a malformed JS tracer andtx_index > 0, the server pays the full preceding-transaction replay cost before returning the-32602-equivalent request error. A careless or adversarial client can burn substantial CPU per request (JS traces already bypass the response cache, so nothing amortises it), and if the replay itself trips a witness/data error along the way the failure surfaces asTraceError::Dataand evicts the block-data cache entry — the real fault (bad JS) is never reported. Suggested fix: Restore the up-front construction: build the realJsInspector(fromcode/config_json/make_tx_ctx(&info)) first so parsing/compilation errors fail before any executor setup; then build the hook-less stub, run the no-record replay, and install the real inspector via*executor.inspector_mut() = real_inspectorfor the target transaction.
Open questions — answer them in a reply on this PR. Each one is marked answered here once a later review round confirms the answer, so this list stays current:
❓ **Open question · Medium confidence**
- Does revm-inspectors 0.40.1's
TracingInspectorConfig::from_geth_erc7562_config/geth_erc7562_tracesactually readErc7562Config::stack_top_items_size? If not, thestack_top_items_sizecomponent ofErc7562ConfigKey(bin/debug-trace-server/src/response_cache.rs:138) discriminates on a field that never affects the serialized trace, letting callers pollute the response cache with byte-identical duplicate entries for one block by varying only that field. - Why it matters: The cache is bounded and shared across all clients; a single block hit with N distinct
stackTopItemsSizevalues would occupy N slots of identical large JSON bodies, evicting hot legitimate entries. The cache-key contract promises "equivalent configs collapse onto one entry," which is violated the moment a key field is inert in the execution path. - How to verify: Read
TracingInspectorConfig::from_geth_erc7562_configand thegeth_erc7562_tracesbuilder inrevm-inspectorsv0.40.1 and confirm whetherstack_top_items_sizeis threaded through either the inspector config or the frame builder. If neither reads it, drop the field fromErc7562ConfigKey(or fold it to a constant) until upstream honours it, and add a test asserting two configs differing only instackTopItemsSizeproduce identicalResponseVariants.
3 participants
Summary
Two follow-ups from the #184 review pass:
erc7562Tracergraduates from "explicitly unsupported" to the sixth supported builtin (revm-inspectors 0.40 ships all the primitives), anddebug_traceTransaction's preceding-transaction replay stops recording traces it immediately discards on the JS and mux paths.erc7562Tracer support
A new
TracerKind::Erc7562variant rides the existing dispatch:TracingInspectorConfig::from_geth_erc7562_configbuilds the inspector andgeth_erc7562_tracesextracts the frame, on both the block and single-transaction paths. The response cache stores it under a parsed-config key like the other config-reading builtins — the ignored-opcode list folds into a 256-bit set, so permutations and duplicates of the same opcodes collapse onto one entry. Classification and gating follow the established contract: a nulltracerConfigis the default config (alloy'sinto_*_configconversions do this for the older builtins;into_erc7562_configmirrors it), and a type-malformed one is rejected with-32602before any block data is fetched. TheUnsupportedrequest shape introduced on the upgrade branch is removed with its last user.Preceding replay without recording
Tracing one transaction first replays its predecessors purely to rebuild state, on the same single executor (preserving the DynamicGasCost bucket cache). The tx-level
TracingInspectorpaths already seed that replay with a none-config inspector; this extends the same idea to the two remaining paths: the JS path seeds a hook-less tracer (nostep/enter/exit— the inspector never re-enters Boa during replay, so the user's JS runs only for the target transaction) and the mux path seeds an empty mux inspector. For a JS trace of a late-block transaction this removes per-opcode JS callback execution for every preceding transaction.Testing
New tests: an end-to-end erc7562 trace over the fixture block on both paths (asserting
Erc7562Frameoutput), the erc7562 malformed-config case in the executor's defense-in-depth matrix, gate classification tests (malformed →-32602, well-formed → cacheable), and the label-conformance case moves from the unsupported tail into the cacheable set.cargo test --workspace: 375 passed / 0 failed; clippy / fmt / cargo-sort clean.Notes
Based on #184 (
liquan/chore/upgrade-reth-2.3) — merge after it lands. Sibling PR #185 (witness sharing) is independent; whichever lands second rebases trivially.