feat(api): Implement debug_traceTransaction API#6751
Merged
akaladarshi merged 9 commits intomainfrom Mar 25, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Geth-compatible debug_traceTransaction RPC: new RPC method and registry entry, tracer option types, Geth/PreState trace builders, state-replay support in StateManager, state-diff and trace utilities, test helpers and unit tests, plus developer and user documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as "RPC Handler\n(Forest.EthDebugTraceTransaction)"
participant StateMan as "State Manager\n(replay_for_prestate)"
participant TraceBuilder as "Trace Builder\n(geth/parity builders)"
participant DB as "State DB / Blockstore"
Client->>RPC: debug_traceTransaction(txHash, opts)
RPC->>DB: Resolve tx by hash (ensure included in block)
alt PreState tracer
RPC->>StateMan: replay_for_prestate(tipset, target_msg_cid)
StateMan->>DB: Load parent state, execute messages up to target
StateMan-->>RPC: (pre_root, ApiInvocResult, post_root)
RPC->>TraceBuilder: build_prestate_frame(pre_state, post_state, touched_addrs)
TraceBuilder-->>RPC: PreStateFrame
else Call/FlatCall tracer
RPC->>StateMan: replay tipset to collect ExecutionTrace[]
StateMan-->>RPC: ExecutionTrace[]
RPC->>TraceBuilder: build_geth_call_frame(execution_trace)
TraceBuilder-->>RPC: GethCallFrame / Flat frames
end
RPC->>RPC: Construct GethTrace variant (PreState/Call/Flat/Noop)
RPC-->>Client: JSON GethTrace response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
akaladarshi
force-pushed
the
akaladarshi/debug-trace-txn
branch
from
0b8c563 to
38cb7df
Compare
akaladarshi
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/methods/eth.rs (1)
3700-3716:⚠️ Potential issue | 🟠 MajorResolve touched subcall addresses from actor metadata, not raw message addresses.
This is fine for the top-level message, but internal
ExecutionTrace.msg.from/msg.tovalues can be ID addresses. Converting those directly yields masked IDs, sobuild_prestate_frame/build_state_diffcan emit snapshots under masked addresses even though the trace builders already usesrc/rpc/methods/eth/trace/utils.rs:10-17trace_to_address(...)to resolve delegated 0x addresses when actor metadata is available.🔧 Suggested direction
-fn extract_touched_eth_addresses(trace: &crate::rpc::state::ExecutionTrace) -> HashSet<EthAddress> { +fn extract_touched_eth_addresses(trace: &crate::rpc::state::ExecutionTrace) -> HashSet<EthAddress> { let mut addresses = HashSet::default(); - let mut stack = vec![trace]; + let mut stack = vec![(trace, None)]; - while let Some(current) = stack.pop() { - if let Ok(eth_addr) = EthAddress::from_filecoin_address(¤t.msg.from) { - addresses.insert(eth_addr); - } - if let Ok(eth_addr) = EthAddress::from_filecoin_address(¤t.msg.to) { - addresses.insert(eth_addr); - } - stack.extend(¤t.subcalls); + while let Some((current, parent_caller)) = stack.pop() { + let caller = parent_caller + .or_else(|| EthAddress::from_filecoin_address(¤t.msg.from).ok()); + let callee = current + .invoked_actor + .as_ref() + .map(trace_to_address) + .or_else(|| EthAddress::from_filecoin_address(¤t.msg.to).ok()); + + if let Some(addr) = caller { + addresses.insert(addr); + } + if let Some(addr) = callee { + addresses.insert(addr); + for subcall in ¤t.subcalls { + stack.push((subcall, Some(addr))); + } + } } addresses }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 3700 - 3716, extract_touched_eth_addresses currently converts subcall msg.from/msg.to directly via EthAddress::from_filecoin_address which yields masked IDs; instead resolve internal addresses using the trace_to_address helper (from src/rpc/methods/eth/trace/utils.rs) so delegated 0x addresses from actor metadata are used; update extract_touched_eth_addresses to call trace_to_address(current, ...) for subcall "from" and "to" resolution (falling back to EthAddress::from_filecoin_address only if trace_to_address returns None) while keeping the top-level behavior intact.src/rpc/methods/eth/trace/state_diff.rs (1)
206-243:⚠️ Potential issue | 🟠 MajorDon't collapse storage read failures into an empty diff.
If an EVM actor's state or KAMT can't be read, this helper currently returns
{}and the callers treat that as “no storage touched”. That can produce false-negativestateDiff/prestateTraceroutput instead of surfacing the underlying corruption or read failure. Keep theNone/ non-EVM fast path, but propagate real load/iteration errors with context.🔧 Suggested direction
-pub fn extract_evm_storage_entries<DB: Blockstore>( +pub fn extract_evm_storage_entries<DB: Blockstore>( store: &DB, actor: Option<&ActorState>, -) -> HashMap<[u8; 32], U256> { +) -> anyhow::Result<HashMap<[u8; 32], U256>> { let actor = match actor { Some(a) if is_evm_actor(&a.code) => a, - _ => return HashMap::default(), + _ => return Ok(HashMap::default()), }; - let evm_state = match evm::State::load(store, actor.code, actor.state) { - Ok(state) => state, - Err(e) => { - debug!("failed to load EVM state for storage extraction: {e}"); - return HashMap::default(); - } - }; + let evm_state = evm::State::load(store, actor.code, actor.state) + .context("failed to load EVM state for storage extraction")?; // keep the explicit empty-storage fast path here if needed let mut entries = HashMap::default(); - if let Err(e) = kamt.for_each(|key, value| { + kamt.for_each(|key, value| { entries.insert(key.to_big_endian(), *value); Ok(()) - }) { - debug!("failed to iterate storage KAMT: {e}"); - return HashMap::default(); - } + }) + .context("failed to iterate storage KAMT")?; - entries + Ok(entries) }As per coding guidelines "Use anyhow::Result for most operations and add context with
.context()when errors occur".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/state_diff.rs` around lines 206 - 243, Change extract_evm_storage_entries to return anyhow::Result<HashMap<[u8;32], U256>> and keep the non-EVM fast path returning Ok(HashMap::default()); for actual I/O/load failures propagate errors instead of returning an empty map by using .context(...) (or anyhow::Error::new) on the calls that can fail: evm::State::load(store, actor.code, actor.state), Kamt::load_with_config(&storage_cid, store, config) and kamt.for_each(...), returning Err(...) with clear context strings like "loading EVM state", "loading storage KAMT", and "iterating storage KAMT"; update callers to handle Result accordingly.
🧹 Nitpick comments (3)
src/rpc/methods/eth/trace/utils.rs (1)
11-26: Add doc comments to the new public trace helpers.
trace_to_address,extract_revert_reason, andu256_to_eth_hashare new public helpers and currently undocumented. Brief doc comments would make their contracts and edge-case behavior much easier to follow.As per coding guidelines "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/utils.rs` around lines 11 - 26, Add concise Rust doc comments to each public helper: trace_to_address, extract_revert_reason, and u256_to_eth_hash. For trace_to_address describe what address is derived (delegated_address preferred, fallback to ActorID via from_actor_id) and any edge cases (when delegated_address is absent or conversion fails). For extract_revert_reason document the expected input (EthBytes from a reverted transaction), what parse_eth_revert returns, and that it returns None or Some(String) only when the parsed reason does not start with "0x" (i.e., non-hex reasons). For u256_to_eth_hash state that it converts a U256 into an EthHash by writing big-endian bytes into an ethereum_types::H256 and note any assumptions about byte ordering and fixed-size padding. Keep each comment short, use /// doc comment style, and place them immediately above the respective function definitions.src/rpc/methods/eth/trace/types.rs (2)
180-191: Document the new public helper methods.These helpers are public API surface but still lack doc comments:
PreStateConfig::{is_diff_mode,is_code_disabled,is_storage_disabled},GethCallType::{as_str,is_static_call},AccountState::is_empty, andEthTrace::is_success.As per coding guidelines, "Document public functions and structs with doc comments".
Also applies to: 219-232, 312-317, 430-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/types.rs` around lines 180 - 191, Several newly-added public helper methods lack doc comments; add concise Rustdoc for PreStateConfig::is_diff_mode, PreStateConfig::is_code_disabled, PreStateConfig::is_storage_disabled, GethCallType::as_str, GethCallType::is_static_call, AccountState::is_empty, and EthTrace::is_success describing what each returns, the semantics (e.g. what true/false means), and any edge cases (like unwrap_or default behavior); place the /// comments immediately above each function or enum method, using brief one-line summary plus a short note if needed about defaults or behavior when underlying Option is None.
204-248: UsestrumforGethCallTypestring conversions.This enum now keeps Serde renames,
as_str(), and a manualDisplayimpl in sync by hand. Deriving the string conversions withstrumwould remove that duplication and reduce drift.As per coding guidelines, "Use
strumcrate for enum string conversions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/types.rs` around lines 204 - 248, Replace the hand-rolled string conversion and Display for GethCallType with strum derives: add strum_macros derives (e.g., EnumString, AsRefStr and Display) to the GethCallType enum, add #[strum(serialize = "...")] on each variant using the current uppercase tokens ("CALL", "STATICCALL", etc.) to preserve as_str/Display behavior, keep the existing #[serde(rename = "...")] attributes, remove the manual as_str() and the std::fmt::Display impl, and simplify from_parity_call_type to parse the parity string (e.g., call_type.parse().unwrap_or(Self::Call)) or map via EnumString serializations for lowercase parity inputs using appropriate #[strum(serialize = "staticcall")] entries on the matching variants; update any callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/developers/guides/debug_trace_transaction_guide.md`:
- Around line 119-120: Replace the literal Anvil private key value assigned to
the ANVIL_KEY export with a symbolic placeholder to avoid committing secrets;
update the line that sets ANVIL_KEY so it references a descriptive placeholder
(e.g., "ANVIL_PRIVATE_KEY_PLACEHOLDER" or instruct readers to set their own key
in their environment) and keep the ANVIL_RPC export as-is; ensure the guide text
around ANVIL_KEY explains to replace the placeholder with their own key or load
it from a secure source.
In `@docs/docs/users/knowledge_base/rpc/debug_trace_transaction.md`:
- Around line 127-134: The doc text contradicts itself about which accounts
appear in diff-mode: update the Diff mode behavior to state that "pre" contains
the state before execution for all accounts that were touched by the transaction
(including read-only touches that existed before execution), while "post"
contains only the fields that changed after execution; make explicit that
read-only touched accounts therefore appear in "pre" but not in "post", and
clarify that accounts that were neither touched nor changed are omitted from
both "pre" and "post". Reference the "pre" and "post" labels in that paragraph
so readers clearly understand read-only touches appear only in "pre".
In `@src/interpreter/vm.rs`:
- Line 524: Add a Rustdoc comment for the crate-public function reward_message
describing its purpose and contract: explain what reward_message does, the
meaning of its parameters and return value (and any side effects or error/panic
conditions) so other crate modules can use it correctly; place the doc comment
(/// ...) immediately above the pub(crate) fn reward_message declaration and
keep it concise and informative.
In `@src/rpc/methods/eth/trace/geth.rs`:
- Around line 171-188: The code currently always calls
extract_evm_storage_entries and diff_entry_keys even when storage tracing is
disabled; update the logic to check the tracer option (e.g.,
config.disableStorage or the tracer disable flag) and skip the expensive KAMT
walks when true: only call extract_evm_storage_entries for pre_actor/post_actor
and diff_entry_keys when storage is enabled, otherwise set
pre_entries/post_entries/changed_keys to None or empty and pass that through to
build_account_snapshot_from_entries so build_account_snapshot_from_entries is
invoked without attempting to read storage; apply the same conditional change
for the second occurrence that mirrors the block using pre_entries/post_entries
at the later lines.
- Around line 171-173: The diff drops pure storage clears because when a slot
goes nonzero->0 the post snapshot is empty and subsequent retain/remove logic
eliminates those cleared slots; update the logic around
extract_evm_storage_entries/diff_entry_keys handling so that for each key in
changed_keys missing from post_entries you insert a zero-valued entry into the
post structure (or otherwise preserve the pre entry for that key) before
building post_map/post_snap and before calling
pre_map.retain(...)/retain_changed, ensuring cleared slots are represented in
post_map and the account isn’t removed; reference extract_evm_storage_entries,
diff_entry_keys, pre_entries, post_entries, changed_keys, post_snap, pre_map,
post_map, and retain_changed when making the change.
- Around line 119-124: The code silently drops errors by calling .ok() on
actor.eth_nonce(store) and actor.eth_bytecode(store), which hides state decoding
failures; update build_prestate_frame to propagate these errors (it already
returns anyhow::Result) by replacing .ok()/flatten() with the try operator (?)
and add context() to both calls (e.g., when calling actor.eth_nonce(store) and
actor.eth_bytecode(store)) so failures return an Err with a helpful message;
keep honoring config.is_code_disabled() for skipping bytecode retrieval.
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 113-144: Change parse_tracer_config to return anyhow::Result<T>
(instead of T) and fail on JSON deserialization errors using
serde_json::from_value(...).context("invalid tracerConfig") rather than
defaulting to T::default(); update GethDebugTracingOptions::call_config to
propagate that Result (it already returns anyhow::Result<CallTracerConfig>) and
ensure it still checks cfg.with_log after successful parsing, and change
GethDebugTracingOptions::prestate_config to return
anyhow::Result<PreStateConfig> (propagating parse_tracer_config errors) so
callers of prestate_config and any upstream call sites (e.g. trace entrypoints)
are updated to handle the Result.
In `@src/state_manager/mod.rs`:
- Around line 907-912: The current code calls vm.flush() and traced_vm.flush()
while the VM is bound to the canonical DB (TipsetExecutor::create_vm /
chain_index.db()), causing non-canonical per-request roots to be persisted;
instead, create a temporary/overlay blockstore (in-memory or overlay layer) and
bind both the pre-execution VM and the traced VM to that temporary store so
their flush() calls write only to the overlay and are discarded after the trace.
Specifically, replace the use of the canonical-bound VMs when calling vm.flush()
and traced_vm.flush() by constructing VMs via create_vm (or the VM constructor
used here) that accept a temporary store or overlay, run apply_message on the
traced VM, compute pre_root/post_root against that temp store, then drop the
overlay without writing to chain_index.db(); ensure TipsetExecutor::create_vm
remains used for canonical state changes only.
---
Outside diff comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3700-3716: extract_touched_eth_addresses currently converts
subcall msg.from/msg.to directly via EthAddress::from_filecoin_address which
yields masked IDs; instead resolve internal addresses using the trace_to_address
helper (from src/rpc/methods/eth/trace/utils.rs) so delegated 0x addresses from
actor metadata are used; update extract_touched_eth_addresses to call
trace_to_address(current, ...) for subcall "from" and "to" resolution (falling
back to EthAddress::from_filecoin_address only if trace_to_address returns None)
while keeping the top-level behavior intact.
In `@src/rpc/methods/eth/trace/state_diff.rs`:
- Around line 206-243: Change extract_evm_storage_entries to return
anyhow::Result<HashMap<[u8;32], U256>> and keep the non-EVM fast path returning
Ok(HashMap::default()); for actual I/O/load failures propagate errors instead of
returning an empty map by using .context(...) (or anyhow::Error::new) on the
calls that can fail: evm::State::load(store, actor.code, actor.state),
Kamt::load_with_config(&storage_cid, store, config) and kamt.for_each(...),
returning Err(...) with clear context strings like "loading EVM state", "loading
storage KAMT", and "iterating storage KAMT"; update callers to handle Result
accordingly.
---
Nitpick comments:
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 180-191: Several newly-added public helper methods lack doc
comments; add concise Rustdoc for PreStateConfig::is_diff_mode,
PreStateConfig::is_code_disabled, PreStateConfig::is_storage_disabled,
GethCallType::as_str, GethCallType::is_static_call, AccountState::is_empty, and
EthTrace::is_success describing what each returns, the semantics (e.g. what
true/false means), and any edge cases (like unwrap_or default behavior); place
the /// comments immediately above each function or enum method, using brief
one-line summary plus a short note if needed about defaults or behavior when
underlying Option is None.
- Around line 204-248: Replace the hand-rolled string conversion and Display for
GethCallType with strum derives: add strum_macros derives (e.g., EnumString,
AsRefStr and Display) to the GethCallType enum, add #[strum(serialize = "...")]
on each variant using the current uppercase tokens ("CALL", "STATICCALL", etc.)
to preserve as_str/Display behavior, keep the existing #[serde(rename = "...")]
attributes, remove the manual as_str() and the std::fmt::Display impl, and
simplify from_parity_call_type to parse the parity string (e.g.,
call_type.parse().unwrap_or(Self::Call)) or map via EnumString serializations
for lowercase parity inputs using appropriate #[strum(serialize = "staticcall")]
entries on the matching variants; update any callers accordingly.
In `@src/rpc/methods/eth/trace/utils.rs`:
- Around line 11-26: Add concise Rust doc comments to each public helper:
trace_to_address, extract_revert_reason, and u256_to_eth_hash. For
trace_to_address describe what address is derived (delegated_address preferred,
fallback to ActorID via from_actor_id) and any edge cases (when
delegated_address is absent or conversion fails). For extract_revert_reason
document the expected input (EthBytes from a reverted transaction), what
parse_eth_revert returns, and that it returns None or Some(String) only when the
parsed reason does not start with "0x" (i.e., non-hex reasons). For
u256_to_eth_hash state that it converts a U256 into an EthHash by writing
big-endian bytes into an ethereum_types::H256 and note any assumptions about
byte ordering and fixed-size padding. Keep each comment short, use /// doc
comment style, and place them immediately above the respective function
definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a4153233-bbd4-42d7-bc3e-632757f9ac1b
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
docs/docs/developers/guides/debug_trace_transaction_guide.mddocs/docs/users/knowledge_base/rpc/debug_trace_transaction.mdsrc/interpreter/vm.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/trace/geth.rssrc/rpc/methods/eth/trace/mod.rssrc/rpc/methods/eth/trace/parity.rssrc/rpc/methods/eth/trace/state_diff.rssrc/rpc/methods/eth/trace/test_helpers.rssrc/rpc/methods/eth/trace/types.rssrc/rpc/methods/eth/trace/utils.rssrc/rpc/methods/eth/utils.rssrc/rpc/mod.rssrc/state_manager/mod.rssrc/tool/subcommands/api_cmd/test_snapshots_ignored.txt
akaladarshi
force-pushed
the
akaladarshi/debug-trace-txn
branch
from
38cb7df to
a15e446
Compare
akaladarshi
commented
Mar 18, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/methods/eth/trace/state_diff.rs (1)
206-243:⚠️ Potential issue | 🟠 MajorDon’t silently convert storage read failures into empty diffs.
Returning
HashMap::default()on KAMT/state load errors can hide real failures and emit incorrectdebug_traceTransactionstate diffs.Suggested direction
+use anyhow::Context as _; -pub fn extract_evm_storage_entries<DB: Blockstore>( +pub fn extract_evm_storage_entries<DB: Blockstore>( store: &DB, actor: Option<&ActorState>, -) -> HashMap<[u8; 32], U256> { +) -> anyhow::Result<HashMap<[u8; 32], U256>> { let actor = match actor { Some(a) if is_evm_actor(&a.code) => a, - _ => return HashMap::default(), + _ => return Ok(HashMap::default()), }; - let evm_state = match evm::State::load(store, actor.code, actor.state) { - Ok(state) => state, - Err(e) => { - debug!("failed to load EVM state for storage extraction: {e}"); - return HashMap::default(); - } - }; + let evm_state = evm::State::load(store, actor.code, actor.state) + .context("failed to load EVM state for storage extraction")?; let storage_cid = evm_state.contract_state(); let config = evm_kamt_config(); - let kamt: EvmStorageKamt<&DB> = match Kamt::load_with_config(&storage_cid, store, config) { - Ok(k) => k, - Err(e) => { - debug!("failed to load storage KAMT: {e}"); - return HashMap::default(); - } - }; + let kamt: EvmStorageKamt<&DB> = Kamt::load_with_config(&storage_cid, store, config) + .context("failed to load storage KAMT")?; let mut entries = HashMap::default(); - if let Err(e) = kamt.for_each(|key, value| { + kamt.for_each(|key, value| { entries.insert(key.to_big_endian(), *value); Ok(()) - }) { - debug!("failed to iterate storage KAMT: {e}"); - return HashMap::default(); - } + }) + .context("failed to iterate storage KAMT")?; - entries + Ok(entries) }As per coding guidelines: “Use
anyhow::Result<T>for most operations and add context with.context()when errors occur”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/state_diff.rs` around lines 206 - 243, The function extract_evm_storage_entries currently swallows errors from evm::State::load, Kamt::load_with_config and kamt.for_each and returns an empty HashMap; change its signature to return anyhow::Result<HashMap<[u8; 32], U256>> and propagate errors instead of returning HashMap::default(), adding context() on failures (e.g. on evm::State::load, Kamt::load_with_config and kamt.for_each) to provide meaningful messages; update callers to handle the Result and stop converting real storage/load/iterate failures into silent empty diffs; keep the same key/value extraction logic but return Ok(entries) on success.src/rpc/methods/eth/trace/parity.rs (1)
43-48:⚠️ Potential issue | 🟡 MinorThe new rustdoc describes the opposite of the Lotus-parity guard.
This predicate intentionally returns
truefor more than just EVM/EAM invocations, so the current comment will mislead anyone treating it as a contract. Please document the parity quirk explicitly instead of describing the idealized behavior.Based on learnings, "In Forest's
trace_is_evm_or_eam()helper insrc/rpc/methods/eth/trace/parity.rs, the current guard (invoked_actor.id != ETHEREUM_ACCOUNT_MANAGER_ACTOR) intentionally mirrors Lotus behavior even though it can returntruefor non-EVM actors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/parity.rs` around lines 43 - 48, Update the rustdoc for trace_is_evm_or_eam to explicitly state it mirrors the Lotus/Parity guard and can return true for non-EVM actors (i.e., it is intentionally permissive), rather than describing an idealized “EVM or EAM only” predicate. Mention the parity quirk and the exact check used (the is_evm_actor(&invoked_actor.state.code) || invoked_actor.id != Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap() condition) so readers know the != ETHEREUM_ACCOUNT_MANAGER_ACTOR branch is deliberate and may cause non-EVM traces to be treated as true.
♻️ Duplicate comments (1)
src/state_manager/mod.rs (1)
907-913:⚠️ Potential issue | 🟠 MajorAvoid persisting trace-only pre/post roots into the canonical blockstore.
At Line 908 and Line 912,
flush()runs on VMs created byTipsetExecutor::create_vm, which is backed byself.chain_index.db()(Line 2002). That persists per-request intermediate roots for a read RPC, and repeated tracing can grow node storage unnecessarily. Please execute this pre/post replay on an overlay/in-memory blockstore and discard it after building the response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/developers/guides/debug_trace_transaction_guide.md`:
- Line 120: Add an inline gitleaks allow marker to the known Anvil test private
key export so scanners won't flag it; edit the line that defines the ANVIL_KEY
export (the line containing export ANVIL_KEY="0xac0974...f2ff80") and append the
comment marker `#gitleaks`:allow on the same line to explicitly permit this
test-only key.
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 459-485: The current construction of GethCallFrame sets output
using (!result.output.is_empty()).then_some(...) which collapses a successful
empty return into None; update the logic in the GethCallFrame creation (the
output field) to always set Some(result.output.clone()) for successful
calls/creates (even when empty) and only set output = None when the trace
actually failed (the existing !is_success branch should clear the output on
failure). Do the analogous change for create frames that use result.code so
successful empty code remains Some(EthBytes::default()) and only clear on
failure; locate the output/code assignments near the GethCallFrame creation and
the later !is_success handling and adjust them accordingly.
---
Outside diff comments:
In `@src/rpc/methods/eth/trace/parity.rs`:
- Around line 43-48: Update the rustdoc for trace_is_evm_or_eam to explicitly
state it mirrors the Lotus/Parity guard and can return true for non-EVM actors
(i.e., it is intentionally permissive), rather than describing an idealized “EVM
or EAM only” predicate. Mention the parity quirk and the exact check used (the
is_evm_actor(&invoked_actor.state.code) || invoked_actor.id !=
Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap() condition) so readers know
the != ETHEREUM_ACCOUNT_MANAGER_ACTOR branch is deliberate and may cause non-EVM
traces to be treated as true.
In `@src/rpc/methods/eth/trace/state_diff.rs`:
- Around line 206-243: The function extract_evm_storage_entries currently
swallows errors from evm::State::load, Kamt::load_with_config and kamt.for_each
and returns an empty HashMap; change its signature to return
anyhow::Result<HashMap<[u8; 32], U256>> and propagate errors instead of
returning HashMap::default(), adding context() on failures (e.g. on
evm::State::load, Kamt::load_with_config and kamt.for_each) to provide
meaningful messages; update callers to handle the Result and stop converting
real storage/load/iterate failures into silent empty diffs; keep the same
key/value extraction logic but return Ok(entries) on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 239365ef-26eb-4e64-a9ac-e2c2b84bc4dd
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
docs/docs/developers/guides/debug_trace_transaction_guide.mddocs/docs/users/knowledge_base/rpc/debug_trace_transaction.mdsrc/interpreter/vm.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/trace/geth.rssrc/rpc/methods/eth/trace/mod.rssrc/rpc/methods/eth/trace/parity.rssrc/rpc/methods/eth/trace/state_diff.rssrc/rpc/methods/eth/trace/test_helpers.rssrc/rpc/methods/eth/trace/types.rssrc/rpc/methods/eth/trace/utils.rssrc/rpc/methods/eth/utils.rssrc/rpc/mod.rssrc/state_manager/mod.rssrc/tool/subcommands/api_cmd/test_snapshots_ignored.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- src/interpreter/vm.rs
- src/rpc/methods/eth/trace/mod.rs
- src/rpc/methods/eth/trace/test_helpers.rs
- docs/docs/users/knowledge_base/rpc/debug_trace_transaction.md
hanabi1224
reviewed
Mar 23, 2026
hanabi1224
previously approved these changes
Mar 23, 2026
Contributor
hanabi1224
left a comment
hanabi1224
left a comment
There was a problem hiding this comment.
Some NITs, otherwise LGTM.
1 task
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/state_manager/mod.rs (1)
907-912:⚠️ Potential issue | 🟠 MajorKeep pre/post replay roots out of the shared blockstore.
Line 908 and Line 912 create roots that
debug_trace_transactionlater reopens fromctx.store_owned(), so this read RPC is persisting non-canonical intermediate states into the node DB. Repeated traces can grow durable state indefinitely. Please run this replay on a temporary/overlay store and load the pre/postStateTrees from that store instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state_manager/mod.rs` around lines 907 - 912, The current replay writes intermediate pre/post roots into the shared node DB because vm.flush() and traced_vm.flush() persist to the default store, so change the replay to use a temporary/overlay blockstore instead: create an ephemeral in-memory or overlay store and pass it into exec.create_vm(...) (and any VM construction that currently uses ctx.store_owned()), call vm.flush() and traced_vm.flush() against that temp store so pre_root/post_root are created there, and then load the pre/post StateTree from that temporary store when debug_trace_transaction reopens them (avoid calling ctx.store_owned() for these roots). Ensure you reference vm.flush(), exec.create_vm, traced_vm.flush(), debug_trace_transaction and StateTree to locate and update the VM creation and flush paths.
🧹 Nitpick comments (1)
src/rpc/methods/eth/trace/types.rs (1)
176-188: Add rustdoc to the new public helper methods.These helpers extend the exported tracer/RPC surface, but several of them are still undocumented (
PreStateConfig::is_*,GethCallType::as_str/is_static_call,AccountState::is_empty,EthTrace::is_success,StateDiff::new). Please document them before this API solidifies.As per coding guidelines, "Document public functions and structs with doc comments".
Also applies to: 215-228, 308-313, 426-428, 719-722
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/types.rs` around lines 176 - 188, Add Rustdoc comments to the new public helper methods so the exported tracer/RPC API is documented: add concise triple-slash (///) docs above PreStateConfig::is_diff_mode, PreStateConfig::is_code_disabled, PreStateConfig::is_storage_disabled and likewise for GethCallType::as_str, GethCallType::is_static_call, AccountState::is_empty, EthTrace::is_success, and StateDiff::new that explain what the method does, any important semantics or edge cases, and what the return value represents; keep descriptions short, use imperative style and mention if the method is a convenience wrapper (e.g., unwrap_or default), and include examples or panics only if relevant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 489-514: In the TraceAction::Create / TraceResult::Create arm
(inside into_geth_frame), detect the invalid case where is_success is true but
result.address is None and bail early instead of building a success-shaped
GethCallFrame; return an error (e.g., a TraceError::InvalidTrace or similar
existing error variant) when is_success && result.address.is_none() so
successful CREATE traces always include a deployed address before constructing
the GethCallFrame.
- Around line 116-121: call_config currently rejects CallTracerConfig when
with_log is true; update the handling so the error is explicit and actionable:
in call_config (using parse_tracer_config::<CallTracerConfig> and the
CallTracerConfig.with_log field) replace the generic anyhow::bail!("callTracer:
withLog is not yet supported") with an error that names the unsupported option
and points callers to the public docs/CHANGELOG entry (or a link/marker)
explaining the incompatibility, and ensure the repository's
documentation/release notes are updated to declare that Forest's callTracer does
not support withLog (unless you instead implement with_log handling here and the
associated log-collection behavior).
- Around line 338-360: Remove deserialization support for the response-only
GethTrace enum by deleting the Deserialize derive from the GethTrace declaration
(leave Serialize) and removing any lotus_json_with_self! invocation that
registers GethTrace for deserialization; keep PreStateFrame and other types
unchanged but ensure GethTrace remains #[derive(PartialEq, Debug, Clone,
Serialize, JsonSchema)] and untagged so it only serializes. This eliminates the
unreachable Noop variant during deserialization while preserving response
serialization for debug_trace_transaction.
---
Duplicate comments:
In `@src/state_manager/mod.rs`:
- Around line 907-912: The current replay writes intermediate pre/post roots
into the shared node DB because vm.flush() and traced_vm.flush() persist to the
default store, so change the replay to use a temporary/overlay blockstore
instead: create an ephemeral in-memory or overlay store and pass it into
exec.create_vm(...) (and any VM construction that currently uses
ctx.store_owned()), call vm.flush() and traced_vm.flush() against that temp
store so pre_root/post_root are created there, and then load the pre/post
StateTree from that temporary store when debug_trace_transaction reopens them
(avoid calling ctx.store_owned() for these roots). Ensure you reference
vm.flush(), exec.create_vm, traced_vm.flush(), debug_trace_transaction and
StateTree to locate and update the VM creation and flush paths.
---
Nitpick comments:
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 176-188: Add Rustdoc comments to the new public helper methods so
the exported tracer/RPC API is documented: add concise triple-slash (///) docs
above PreStateConfig::is_diff_mode, PreStateConfig::is_code_disabled,
PreStateConfig::is_storage_disabled and likewise for GethCallType::as_str,
GethCallType::is_static_call, AccountState::is_empty, EthTrace::is_success, and
StateDiff::new that explain what the method does, any important semantics or
edge cases, and what the return value represents; keep descriptions short, use
imperative style and mention if the method is a convenience wrapper (e.g.,
unwrap_or default), and include examples or panics only if relevant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc16da0e-cb17-45b3-a190-c987a903247d
📒 Files selected for processing (3)
src/rpc/methods/eth.rssrc/rpc/methods/eth/trace/types.rssrc/state_manager/mod.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
3493-3495: Consider usinginvalid_paramsfor "transaction not found".The error "transaction not found" typically indicates invalid client input (non-existent hash) rather than an internal server error. However, I note that
EthTraceTransaction::handle(line 3757) uses the sameinternal_errorpattern, so this is consistent with existing code.💡 Optional: Use invalid_params for consistency with RPC semantics
let eth_txn = get_eth_transaction_by_hash(&ctx, ð_hash, None) .await? - .ok_or(ServerError::internal_error("transaction not found", None))?; + .ok_or(ServerError::invalid_params("transaction not found", None))?;If you adopt this change, consider applying it to
EthTraceTransaction::handleas well for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 3493 - 3495, Replace the ServerError::internal_error used when get_eth_transaction_by_hash returns None with ServerError::invalid_params("transaction not found", None) to reflect that a missing transaction is a client/input error; update the expression that constructs eth_txn (the call to get_eth_transaction_by_hash and its .ok_or(...)? chain) to return invalid_params instead of internal_error, and consider making the same change in EthTraceTransaction::handle to keep RPC error semantics consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3493-3495: Replace the ServerError::internal_error used when
get_eth_transaction_by_hash returns None with
ServerError::invalid_params("transaction not found", None) to reflect that a
missing transaction is a client/input error; update the expression that
constructs eth_txn (the call to get_eth_transaction_by_hash and its .ok_or(...)?
chain) to return invalid_params instead of internal_error, and consider making
the same change in EthTraceTransaction::handle to keep RPC error semantics
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77e5f559-119b-44c1-8c53-715fdd5afaf2
📒 Files selected for processing (4)
src/interpreter/vm.rssrc/rpc/methods/eth.rssrc/rpc/mod.rssrc/state_manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/interpreter/vm.rs
- src/state_manager/mod.rs
hanabi1224
approved these changes
Mar 25, 2026
This was referenced Mar 25, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6675
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Documentation
Tests