feat(api): Implement debug_traceTransaction API#6685
Draft
akaladarshi wants to merge 16 commits intomainfrom
Draft
feat(api): Implement debug_traceTransaction API#6685akaladarshi wants to merge 16 commits intomainfrom
akaladarshi wants to merge 16 commits intomainfrom
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:
WalkthroughImplements a debug_traceTransaction RPC (Forest.EthDebugTraceTransaction) with Call, FlatCall, and PreState tracer modes, Geth-style call-frame construction, EVM-aware state-diffing, tipset-based replay for prestate capture, and a TipsetExecutor utility used to centralize VM creation and replay. Changes
Sequence DiagramsequenceDiagram
actor Client
participant RPC as RPC Handler<br/>(debug_traceTransaction)
participant SM as StateManager
participant TE as TipsetExecutor
participant VM as Filecoin VM
participant Builder as Frame Builder<br/>(Geth/PreState)
Client->>RPC: debug_traceTransaction(txHash, options)
RPC->>SM: resolve tx → target MCID
RPC->>SM: replay_for_prestate(tipset, target_mcid)
SM->>TE: new(tipset, chain_config, ...)
TE->>TE: prepare_parent_state()
SM->>VM: create_vm(parent_state, epoch, trace=true)
SM->>VM: apply_messages(up to target)
VM-->>SM: pre_state_root, invoc_result, post_state_root
SM-->>RPC: (pre_root, invoc_result, post_root)
alt tracer == "call"
RPC->>Builder: build_geth_call_frame(invoc_result, cfg)
Builder-->>RPC: GethCallFrame tree
else tracer == "prestate"
RPC->>Builder: build_prestate_frame(pre_root, post_root, cfg)
Builder-->>RPC: PreStateFrame / StateDiff
else tracer == "flatcall"
RPC->>SM: execute_tipset_traces(...) → per-entry traces
SM-->>RPC: flat entry traces
end
RPC-->>Client: tracer-formatted response (Call/PreState/FlatCall)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
akaladarshi
requested review from
LesnyRumcajs,
hanabi1224 and
sudo-shashank
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/interpreter/vm.rs (1)
524-531:⚠️ Potential issue | 🟡 MinorDocument
reward_messagenow that it is crate-visible.This helper became
pub(crate)without rustdoc. Please document its invariants and the meaning of theOptionreturn, since callers outside this module can now rely on it.As per coding guidelines, "Document all public functions and structs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interpreter/vm.rs` around lines 524 - 531, Add a rustdoc comment to the now crate-visible function reward_message describing its purpose (builds an optional miner reward Message for the given epoch), documenting each parameter (epoch: ChainEpoch, miner: Address, win_count, penalty: TokenAmount, gas_reward: TokenAmount), the invariants (e.g., win_count >= 0, penalty and gas_reward are non-negative TokenAmount, epoch must be valid for the chain context), the semantics of the Option return (None means no reward message should be emitted for this call; Some(Message) contains the constructed reward message ready for dispatch), the Result error conditions (what causes an Err to be returned), and any side effects or assumptions callers must honor when using reward_message so crate consumers can rely on its contract.src/rpc/methods/eth/trace.rs (1)
110-115:⚠️ Potential issue | 🟠 MajorFix the EVM/EAM predicate before using these exit-code mappings.
trace_is_evm_or_eam()currently uses!=for the EAM actor id, so it matches almost every non-EAM actor and excludes the EAM itself. That makes this branch label unrelated actor failures as EVM errors while real EAM failures fall through toactor error: ....Suggested fix
fn trace_is_evm_or_eam(trace: &ExecutionTrace) -> bool { if let Some(invoked_actor) = &trace.invoked_actor { is_evm_actor(&invoked_actor.state.code) - || invoked_actor.id != Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap() + || invoked_actor.id == Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap() } else { false } }Also applies to: 138-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace.rs` around lines 110 - 115, trace_is_evm_or_eam currently returns true for almost every non-EAM actor because it uses "!="; change the predicate so it returns true when the invoked actor is either an EVM actor or the EAM actor by replacing the invoked_actor.id != Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap() check with invoked_actor.id == Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap(), keeping the existing is_evm_actor(&invoked_actor.state.code) check; make the same correction in the other identical predicate block around the 138-156 region to ensure EAM failures are classified as EAM/EVM traces.
🤖 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 144-149: The doc currently routes a raw CID directly to
eth_getTransactionByHash; split the flows so readers know to first call
eth_getTransactionHashByCid when they have a Filecoin message CID and only then
use the returned EthHash with eth_getTransactionByHash and
debug_traceTransaction, and conversely if the value from CAST is already a
0x-prefixed EthHash skip the CID step; update the examples and the corresponding
text (also at the later section around lines 153-163) to show both paths and
clarify that HASH_FROM_CAST must be a 0x-hash before calling
eth_getTransactionByHash/debug_traceTransaction or otherwise must be converted
via eth_getTransactionHashByCid.
In `@docs/docs/users/knowledge_base/rpc/debug_trace_transaction.md`:
- Around line 332-335: Update the section heading text from "Filecoin Specific
Behavior" to the hyphenated form "Filecoin-Specific Behavior" so it reads as a
compound modifier; locate the heading string in the document (the current
heading line begins with "## Filecoin Specific Behavior") and replace it with
"## Filecoin-Specific Behavior".
In `@src/rpc/methods/eth.rs`:
- Around line 3491-3498: The code resolves a tipset for every transaction even
when get_eth_transaction_by_hash returned a mempool/pending tx, causing wrong
tipset resolution; modify the logic after get_eth_transaction_by_hash
(referencing eth_txn and its block_number) to detect pending/mempool
transactions (e.g., default/None block metadata or a pending flag) and
immediately return a clear "transaction pending/not traceable yet" error instead
of calling TipsetResolver::new and tipset_by_block_number_or_hash; only
construct the resolver and resolve the tipset when eth_txn has valid on-chain
block metadata.
- Around line 3391-3405: The msg_idx used for TipsetTraceEntry.msg_position is
being incremented before storing, making positions one-based; change the logic
in the loop (over raw_traces) so you assign the current msg_idx to
TipsetTraceEntry.msg_position for non-system messages and only then increment
msg_idx (or increment at loop end), ensuring transaction_position remains
zero-based; update the loop around msg_idx, the conditional that skips
system::ADDRESS, and the construction of TipsetTraceEntry to use the
pre-increment value.
- Around line 3380-3389: The call to ctx.state_manager.execution_trace(ts)
inside execute_tipset_traces is performing synchronous, CPU-heavy tipset replay
on the async executor; change this to run on the blocking pool by wrapping the
execution_trace call in tokio::task::spawn_blocking and await its JoinHandle
(propagating errors), then continue using the returned state_root to call
ctx.state_manager.get_state_tree(&state_root); ensure execute_tipset_traces
preserves its Result error conversion and types when moving the execution into
spawn_blocking so the async RPC task is not blocked by VM execution.
- Around line 3481-3488: The code currently defaults
opts.tracer.clone().unwrap_or(GethDebugBuiltInTracerType::Call) which returns a
callTracer when tracer is omitted; change this to either (A) default to the
struct logger used by Geth (e.g., use GethDebugBuiltInTracerType::Struct or the
enum variant representing the opcode/struct logger and ensure the struct logger
implementation is invoked instead of callTracer) or (B) explicitly reject
requests that omit tracer by checking if opts.tracer is None and returning an
error rather than falling back to Call; update the tracer selection logic around
opts.tracer, remove the unwrap_or(GethDebugBuiltInTracerType::Call) usage, and
ensure downstream handling (the branch that returns GethTrace::Noop/NoopFrame
and any code that expects callTracer output) correctly handles the chosen
behavior.
In `@src/rpc/methods/eth/types.rs`:
- Around line 684-701: The current call_config and prestate_config silently
swallow deserialization errors via .ok().unwrap_or_default(), so update both
GethDebugTracingOptions::call_config and ::prestate_config to return
Result<CallTracerConfig, serde_json::Error> (or a crate-specific error) instead
of the default directly; attempt serde_json::from_value(c.0.clone()) and
propagate the error if deserialization fails (do not convert errors to
defaults), and also annotate the target structs (CallTracerConfig and
PreStateConfig) with #[serde(deny_unknown_fields)] so unknown keys cause
errors—this ensures malformed or unsupported tracerConfig payloads are reported
to callers rather than quietly no-oping.
In `@src/state_manager/mod.rs`:
- Around line 934-943: In replay_for_prestate keep the same sanity check as
VM::apply_block_messages: after creating the reward message with
vm.reward_message and applying it via vm.apply_implicit_message (the variables
rew_msg and ret), inspect the reward receipt's exit code in addition to
failure_info() and bail if the exit code indicates non-success; in other words,
after the apply_implicit_message call check ret.failure_info() OR
ret.exit_code() (or the receipt's exit code field) for a non-success value and
return an error with context "failed to apply reward message for miner
{block.miner}" if either condition is met so replay cannot advance past a state
regular execution would reject.
In `@src/tool/subcommands/api_cmd/test_snapshots_ignored.txt`:
- Line 86: The ignored snapshot entry uses "Forest.EthDebugTraceTransaction" but
the RPC registers EthDebugTraceTransaction::NAME as
"Filecoin.EthDebugTraceTransaction", so update the snapshot name to match the
registered RPC name (change the snapshot entry from
Forest.EthDebugTraceTransaction to Filecoin.EthDebugTraceTransaction) or
alternatively change EthDebugTraceTransaction::NAME to
"Forest.EthDebugTraceTransaction" if the intention is to keep the snapshot;
ensure the string in the test snapshot exactly matches
EthDebugTraceTransaction::NAME.
---
Outside diff comments:
In `@src/interpreter/vm.rs`:
- Around line 524-531: Add a rustdoc comment to the now crate-visible function
reward_message describing its purpose (builds an optional miner reward Message
for the given epoch), documenting each parameter (epoch: ChainEpoch, miner:
Address, win_count, penalty: TokenAmount, gas_reward: TokenAmount), the
invariants (e.g., win_count >= 0, penalty and gas_reward are non-negative
TokenAmount, epoch must be valid for the chain context), the semantics of the
Option return (None means no reward message should be emitted for this call;
Some(Message) contains the constructed reward message ready for dispatch), the
Result error conditions (what causes an Err to be returned), and any side
effects or assumptions callers must honor when using reward_message so crate
consumers can rely on its contract.
In `@src/rpc/methods/eth/trace.rs`:
- Around line 110-115: trace_is_evm_or_eam currently returns true for almost
every non-EAM actor because it uses "!="; change the predicate so it returns
true when the invoked actor is either an EVM actor or the EAM actor by replacing
the invoked_actor.id != Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap()
check with invoked_actor.id ==
Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap(), keeping the existing
is_evm_actor(&invoked_actor.state.code) check; make the same correction in the
other identical predicate block around the 138-156 region to ensure EAM failures
are classified as EAM/EVM traces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63878b5c-8e53-4b21-9623-fa0d7300fcab
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/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 (11)
docs/dictionary.txtdocs/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.rssrc/rpc/methods/eth/types.rssrc/rpc/methods/eth/utils.rssrc/rpc/mod.rssrc/state_manager/mod.rssrc/tool/subcommands/api_cmd/test_snapshots_ignored.txt
Member
|
no green checkmark, no review ❌ Also, make sure to address/resolve 🐰 comments |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/rpc/methods/eth/types.rs (1)
684-709:⚠️ Potential issue | 🟠 MajorDon't turn an invalid
tracerConfiginto a successful trace.
parse_tracer_config()still swallows deserialization failures and falls back to defaults, so a malformed payload quietly no-ops instead of telling the caller their config was ignored. The config structs also need strict decoding, otherwise unsupported keys keep slipping through as "success".Suggested change
- pub fn call_config(&self) -> CallTracerConfig { - parse_tracer_config::<CallTracerConfig>(&self.tracer_config) + pub fn call_config(&self) -> anyhow::Result<CallTracerConfig> { + parse_tracer_config::<CallTracerConfig>(&self.tracer_config) + .context("invalid callTracer tracerConfig") } - pub fn prestate_config(&self) -> PreStateConfig { - parse_tracer_config::<PreStateConfig>(&self.tracer_config) + pub fn prestate_config(&self) -> anyhow::Result<PreStateConfig> { + parse_tracer_config::<PreStateConfig>(&self.tracer_config) + .context("invalid prestateTracer tracerConfig") } } -fn parse_tracer_config<T: Default + serde::de::DeserializeOwned>(raw: &Option<TracerConfig>) -> T { +fn parse_tracer_config<T: Default + serde::de::DeserializeOwned>( + raw: &Option<TracerConfig>, +) -> anyhow::Result<T> { let Some(cfg) = raw.as_ref().filter(|c| !c.0.is_null()) else { - return T::default(); + return Ok(T::default()); }; - serde_json::from_value(cfg.0.clone()).unwrap_or_else(|e| { - tracing::warn!( - error = %e, - "invalid tracerConfig — using defaults" - ); - T::default() - }) + serde_json::from_value(cfg.0.clone()).map_err(Into::into) }#[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct CallTracerConfig { #[serde(default, skip_serializing_if = "Option::is_none")] pub only_top_call: Option<bool>, }#[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct PreStateConfig { #[serde(default, skip_serializing_if = "Option::is_none")] pub diff_mode: Option<bool>, #[serde(default, skip_serializing_if = "Option::is_none")] pub disable_code: Option<bool>, #[serde(default, skip_serializing_if = "Option::is_none")] pub disable_storage: Option<bool>, }As per coding guidelines, "Use anyhow::Result for most operations and add context with
.context()when errors occur".Also applies to: 713-738
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/types.rs` around lines 684 - 709, parse_tracer_config currently swallows deserialization errors and returns defaults, letting malformed tracerConfig silently no-op; change it to return anyhow::Result<T> (e.g. pub fn parse_tracer_config<T: Default + DeserializeOwned>(raw: &Option<TracerConfig>) -> anyhow::Result<T>) and propagate serde_json::from_value(...) errors instead of unwrap_or_else by using .context("failed to deserialize tracerConfig for <type>"); update callers GethDebugTracingOptions::call_config and ::prestate_config to return anyhow::Result<CallTracerConfig>/anyhow::Result<PreStateConfig> and propagate the error; additionally enforce strict decoding on the config structs (CallTracerConfig, PreStateConfig) by adding #[serde(deny_unknown_fields)] so unknown keys fail deserialization. Ensure all error returns use .context(...) to add helpful context per guidelines.
🧹 Nitpick comments (1)
src/state_manager/mod.rs (1)
921-921: Consider computingMessageGasCostinstead of using default.The
replay_blockingmethod (line 834) computesMessageGasCost::new(ctx.message.message(), ctx.apply_ret)?to populate gas cost details. Here,MessageGasCost::default()is used, which will return empty/zero gas cost information. If the API consumer needs gas cost breakdown for debugging, this could be a gap.♻️ Suggested fix
msg_rct: Some(ret.msg_receipt()), error: ret.failure_info().unwrap_or_default(), duration: duration.as_nanos().clamp(0, u64::MAX as u128) as u64, - gas_cost: MessageGasCost::default(), + gas_cost: MessageGasCost::new(msg.message(), &ret)?, execution_trace: structured::parse_events(ret.exec_trace())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state_manager/mod.rs` at line 921, The code sets gas_cost: MessageGasCost::default() which yields empty/zero data; instead compute the real gas breakdown the same way replay_blocking does by calling MessageGasCost::new(ctx.message.message(), ctx.apply_ret) (or equivalent variables in scope) and propagate errors as needed (e.g., using ?), replacing MessageGasCost::default() with the computed result so callers receive accurate gas cost information.
🤖 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/users/knowledge_base/rpc/debug_trace_transaction.md`:
- Around line 41-45: The docs table for the prestateTracer config is missing the
existing flags; update the table for `prestateTracer` to include `disableCode`
and `disableStorage` (both boolean, default `false`) alongside `diffMode`, and
add concise descriptions such as "`disableCode`: when `true`, do not collect
contract code in pre/post states" and "`disableStorage`: when `true`, do not
collect contract storage in pre/post states" so the documented request shape
matches the implemented `prestateTracer` options.
In `@src/rpc/methods/eth.rs`:
- Around line 3540-3549: The current logic uses
EthAddress::from_filecoin_address on invoc_result.msg.from()/to(), which misses
native f1/f3 endpoints; instead, after creating pre_state via
StateTree::new_from_root, lookup the actor for each message endpoint in that
pre_state (e.g., call StateTree::get_actor or equivalent on
invoc_result.msg.from()/to()), and resolve the EthAddress from the actor data
(derive the corresponding 0x address for account/BLS native actors) before
inserting into touched; keep using
extract_touched_eth_addresses(&execution_trace) but replace the direct
EthAddress::from_filecoin_address calls with state-based resolution so
non-delegated f1/f3 senders are included in touched (use the existing
pre_state/post_state variables and the invoc_result.msg.from()/to() identifiers
to find the actors).
- Around line 3499-3506: The early return for GethDebugBuiltInTracerType::Noop
currently precedes parsing and validating the tx hash and fetching the
transaction, allowing debug_traceTransaction to return Noop for malformed or
missing hashes; move the noop short-circuit below the validation steps: first
call EthHash::from_str(&tx_hash) and await get_eth_transaction_by_hash(&ctx,
ð_hash, None) (and handle pending/unknown tx error) to validate the input,
then, after successful validation, check if tracer ==
GethDebugBuiltInTracerType::Noop and return GethTrace::Noop(NoopFrame {}) only
after the transaction is confirmed valid.
In `@src/rpc/methods/eth/trace.rs`:
- Around line 685-690: The function build_geth_call_frame currently accepts a
CallTracerConfig but ignores the withLog flag, so callers requesting logs get
the same result as without logs; add an explicit guard at the start of
build_geth_call_frame that checks tracer_cfg.with_log (or withLog) and returns
an error (e.g., via anyhow::bail or Err(anyhow::anyhow!(...))) when true,
referencing that logs are not yet supported, otherwise continue to call
build_geth_frame_recursive as before; mention build_geth_call_frame,
CallTracerConfig, and build_geth_frame_recursive in your change so reviewers can
find the spot.
In `@src/rpc/methods/eth/types.rs`:
- Around line 331-335: The code must reject requests where both `input` and
`data` are set but differ instead of silently preferring `input`; modify
`effective_input` (or add a small validator) to detect when `self.input` and
`self.data` are both Some and not equal and return an error (e.g., an
RPC/bad-request error) in that case, otherwise return the chosen calldata; then
update callers (the site that calls `effective_input()` and the other usage
around the `convert_data_to_message_params`/`EthCallMessage` conversion) to
handle the Result (use `?` or propagate the error) so a conflicting
`data`+`input` produces a clear RPC error rather than silently using one field.
- Around line 766-803: from_parity_call_type currently maps any unrecognized
parity string to GethCallType::Call, losing fidelity; change its signature to
return an Option<GethCallType> (or Result<GethCallType, ParseError>) and only
map known strings ("staticcall", "delegatecall", "call" if present) to variants,
returning None/Err for unknown values. Update the function declaration
from_parity_call_type to return the fallible type, implement explicit matches
for recognized lowercase parity strings, and update all call sites that used the
old infallible function (e.g., wherever from_parity_call_type is invoked) to
handle the None/Err case appropriately (propagate the error or skip/record as
unknown) so unknown parity call types are not silently coerced to Call. Ensure
GethCallType::as_str/is_static_call remain unchanged.
---
Duplicate comments:
In `@src/rpc/methods/eth/types.rs`:
- Around line 684-709: parse_tracer_config currently swallows deserialization
errors and returns defaults, letting malformed tracerConfig silently no-op;
change it to return anyhow::Result<T> (e.g. pub fn parse_tracer_config<T:
Default + DeserializeOwned>(raw: &Option<TracerConfig>) -> anyhow::Result<T>)
and propagate serde_json::from_value(...) errors instead of unwrap_or_else by
using .context("failed to deserialize tracerConfig for <type>"); update callers
GethDebugTracingOptions::call_config and ::prestate_config to return
anyhow::Result<CallTracerConfig>/anyhow::Result<PreStateConfig> and propagate
the error; additionally enforce strict decoding on the config structs
(CallTracerConfig, PreStateConfig) by adding #[serde(deny_unknown_fields)] so
unknown keys fail deserialization. Ensure all error returns use .context(...) to
add helpful context per guidelines.
---
Nitpick comments:
In `@src/state_manager/mod.rs`:
- Line 921: The code sets gas_cost: MessageGasCost::default() which yields
empty/zero data; instead compute the real gas breakdown the same way
replay_blocking does by calling MessageGasCost::new(ctx.message.message(),
ctx.apply_ret) (or equivalent variables in scope) and propagate errors as needed
(e.g., using ?), replacing MessageGasCost::default() with the computed result so
callers receive accurate gas cost information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e7a1c4b-9cf5-4051-a2e8-e93955ea1396
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/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 (6)
docs/dictionary.txtdocs/docs/users/knowledge_base/rpc/debug_trace_transaction.mdsrc/rpc/methods/eth.rssrc/rpc/methods/eth/trace.rssrc/rpc/methods/eth/types.rssrc/state_manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/dictionary.txt
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/rpc/methods/eth.rs (2)
3499-3514:⚠️ Potential issue | 🟠 MajorValidate the transaction before honoring
noopTracer.This early return still lets malformed, unknown, or pending hashes succeed with
{}whenevernoopTraceris selected. The noop short-circuit should happen only after hash parsing, lookup, and pending rejection.Suggested reorder
- if tracer == GethDebugBuiltInTracerType::Noop { - return Ok(GethTrace::Noop(NoopFrame {})); - } - let eth_hash = EthHash::from_str(&tx_hash).context("invalid transaction hash")?; let eth_txn = get_eth_transaction_by_hash(&ctx, ð_hash, None) .await? .ok_or(ServerError::internal_error("transaction not found", None))?; @@ if eth_txn.block_hash == EthHash::default() { return Err(ServerError::invalid_params( "no trace for pending transactions", None, )); } + + if tracer == GethDebugBuiltInTracerType::Noop { + return Ok(GethTrace::Noop(NoopFrame {})); + }🤖 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 3499 - 3514, The noop tracer early-return is happening before validating the transaction, allowing malformed/unknown/pending tx hashes to return NoopFrame; move the GethDebugBuiltInTracerType::Noop short-circuit to after parsing and lookup: first call EthHash::from_str(&tx_hash), then await get_eth_transaction_by_hash(&ctx, ð_hash, None) and reject if eth_txn.block_hash == EthHash::default(), and only then return Ok(GethTrace::Noop(NoopFrame {})) when tracer == GethDebugBuiltInTracerType::Noop so malformed/unknown/pending transactions are validated before honoring noop.
3398-3412:⚠️ Potential issue | 🟠 MajorKeep
transaction_positionzero-based.
msg_idxis incremented before it is recorded, so the first non-system transaction gets position1. Every trace emitted through this helper ends up off by one.Minimal fix
for ir in raw_traces { if ir.msg.from == system::ADDRESS.into() { continue; } - msg_idx += 1; let tx_hash = EthGetTransactionHashByCid::handle(ctx.clone(), (ir.msg_cid,), ext).await?; let tx_hash = tx_hash .with_context(|| format!("cannot find transaction hash for cid {}", ir.msg_cid))?; entries.push(TipsetTraceEntry { tx_hash, msg_position: msg_idx, invoc_result: ir, }); + msg_idx += 1; }🤖 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 3398 - 3412, The msg index is being incremented before recording, making the first non-system transaction position 1 instead of zero; in the loop over raw_traces (variables: msg_idx, raw_traces) ensure TipsetTraceEntry.msg_position (transaction_position) uses a zero-based index by using the current msg_idx when pushing the entry and then incrementing msg_idx afterward (or decrementing before assignment) so the first recorded non-system message has position 0.src/rpc/methods/eth/types.rs (1)
684-698:⚠️ Potential issue | 🟠 MajorReject invalid
tracerConfiginstead of tracing with defaults.Malformed or unsupported tracer config currently degrades to a successful trace with default settings.
parse_tracer_configswallows deserialization errors, and both config structs still accept unknown keys, so callers get no signal that their config was ignored.One way to make config handling strict
impl GethDebugTracingOptions { /// Extracts and validates the `callTracer` config. /// Returns an error if an unsupported flag (e.g. `withLog`) is set to true. pub fn call_config(&self) -> anyhow::Result<CallTracerConfig> { - let cfg = parse_tracer_config::<CallTracerConfig>(&self.tracer_config); + let cfg = parse_tracer_config::<CallTracerConfig>(&self.tracer_config)?; if cfg.with_log.unwrap_or(false) { anyhow::bail!("callTracer: withLog is not yet supported"); } Ok(cfg) } /// Extracts the `prestateTracer` config, defaulting to no-op values when absent. - pub fn prestate_config(&self) -> PreStateConfig { + pub fn prestate_config(&self) -> anyhow::Result<PreStateConfig> { parse_tracer_config::<PreStateConfig>(&self.tracer_config) } } /// Parses a tracer-specific config from the opaque [`TracerConfig`] JSON blob. -/// Returns `T::default()` when the config is absent or null, and logs a warning -/// if the config is present but fails to deserialize. -fn parse_tracer_config<T: Default + serde::de::DeserializeOwned>(raw: &Option<TracerConfig>) -> T { +/// Returns `T::default()` when the config is absent or null. +fn parse_tracer_config<T: Default + serde::de::DeserializeOwned>( + raw: &Option<TracerConfig>, +) -> anyhow::Result<T> { let Some(cfg) = raw.as_ref().filter(|c| !c.0.is_null()) else { - return T::default(); + return Ok(T::default()); }; - serde_json::from_value(cfg.0.clone()).unwrap_or_else(|e| { - tracing::warn!( - error = %e, - "invalid tracerConfig — using defaults" - ); - T::default() - }) + Ok(serde_json::from_value(cfg.0.clone())?) } #[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct CallTracerConfig { @@ #[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct PreStateConfig {You'd also need to propagate
?from theprestate_config()call site insrc/rpc/methods/eth.rs.Also applies to: 701-715, 717-747
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/types.rs` around lines 684 - 698, The code currently swallows malformed/unsupported tracerConfig and falls back to defaults; change GethDebugTracingOptions::call_config and prestate_config to validate and reject invalid tracer_config instead of silently returning defaults by making parse_tracer_config return a Result and propagating errors: update call_config to call parse_tracer_config::<CallTracerConfig>(&self.tracer_config)? and keep the with_log check, and change prestate_config to return anyhow::Result<PreStateConfig> and call parse_tracer_config::<PreStateConfig>(&self.tracer_config)? so deserialization errors are surfaced; also ensure CallTracerConfig and PreStateConfig deserialization is strict (deny unknown fields or equivalent) so unknown keys produce errors that will be propagated to callers (and then update call sites to propagate the ? as noted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3499-3514: The noop tracer early-return is happening before
validating the transaction, allowing malformed/unknown/pending tx hashes to
return NoopFrame; move the GethDebugBuiltInTracerType::Noop short-circuit to
after parsing and lookup: first call EthHash::from_str(&tx_hash), then await
get_eth_transaction_by_hash(&ctx, ð_hash, None) and reject if
eth_txn.block_hash == EthHash::default(), and only then return
Ok(GethTrace::Noop(NoopFrame {})) when tracer ==
GethDebugBuiltInTracerType::Noop so malformed/unknown/pending transactions are
validated before honoring noop.
- Around line 3398-3412: The msg index is being incremented before recording,
making the first non-system transaction position 1 instead of zero; in the loop
over raw_traces (variables: msg_idx, raw_traces) ensure
TipsetTraceEntry.msg_position (transaction_position) uses a zero-based index by
using the current msg_idx when pushing the entry and then incrementing msg_idx
afterward (or decrementing before assignment) so the first recorded non-system
message has position 0.
In `@src/rpc/methods/eth/types.rs`:
- Around line 684-698: The code currently swallows malformed/unsupported
tracerConfig and falls back to defaults; change
GethDebugTracingOptions::call_config and prestate_config to validate and reject
invalid tracer_config instead of silently returning defaults by making
parse_tracer_config return a Result and propagating errors: update call_config
to call parse_tracer_config::<CallTracerConfig>(&self.tracer_config)? and keep
the with_log check, and change prestate_config to return
anyhow::Result<PreStateConfig> and call
parse_tracer_config::<PreStateConfig>(&self.tracer_config)? so deserialization
errors are surfaced; also ensure CallTracerConfig and PreStateConfig
deserialization is strict (deny unknown fields or equivalent) so unknown keys
produce errors that will be propagated to callers (and then update call sites to
propagate the ? as noted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ccc5f453-d735-4905-9155-624194237d92
📒 Files selected for processing (5)
.config/forest.dicdocs/docs/users/knowledge_base/rpc/debug_trace_transaction.mdmise.tomlsrc/rpc/methods/eth.rssrc/rpc/methods/eth/types.rs
💤 Files with no reviewable changes (1)
- mise.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/docs/users/knowledge_base/rpc/debug_trace_transaction.md
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
akaladarshi
force-pushed
the
akaladarshi/add-debug-trace-txn-api
branch
from
f1b44a5 to
542a6ad
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/rpc/methods/eth.rs (1)
3399-3412:⚠️ Potential issue | 🟠 MajorKeep
transaction_positionzero-based.
msg_idxis incremented before it is stored, so the first non-system transaction is reported as position1. That shiftstransactionPositionfor every trace built from this helper, includingtrace_block,trace_replayBlockTransactions, anddebug_traceTransactionflat traces.🔧 Minimal fix
for ir in raw_traces { if ir.msg.from == system::ADDRESS.into() { continue; } - msg_idx += 1; let tx_hash = EthGetTransactionHashByCid::handle(ctx.clone(), (ir.msg_cid,), ext).await?; let tx_hash = tx_hash .with_context(|| format!("cannot find transaction hash for cid {}", ir.msg_cid))?; entries.push(TipsetTraceEntry { tx_hash, msg_position: msg_idx, invoc_result: ir, }); + msg_idx += 1; }🤖 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 3399 - 3412, The loop that builds TipsetTraceEntry increments msg_idx before storing it, making msg_position one-based; update the logic in the loop that iterates over raw_traces (using variables msg_idx, ir, and building TipsetTraceEntry with msg_position) so that msg_idx remains zero-based when assigned to TipsetTraceEntry (e.g., assign msg_idx to msg_position before incrementing, or increment msg_idx after pushing the entry) to preserve zero-based transaction_position across trace helpers like trace_block, trace_replayBlockTransactions, and debug_traceTransaction.
🤖 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 201: Summary: The phrase "Filecoin specific differences" should be
hyphenated. Fix: locate the sentence containing "When comparing
`debug_traceTransaction` output from Forest and Anvil, expect these Filecoin
specific differences:" and update it to read "Filecoin-specific differences"
(hyphenate "Filecoin-specific") so the phrase becomes "expect these
Filecoin-specific differences:". Ensure only the hyphenation is changed and
punctuation/formatting remains the same.
In `@docs/docs/users/knowledge_base/rpc/debug_trace_transaction.md`:
- Around line 31-39: Update the tracer docs to document the omitted-tracer
behavior: explicitly state that when the tracer option is omitted, Forest
defaults to callTracer (GethDebugBuiltInTracerType::Call) rather than Geth's
struct/opcode (StructLog) tracer; mention this is implemented in
debug_traceTransaction (src/rpc/methods/eth.rs) and justify briefly that FVM
cannot support opcode-level StructLog tracing, and note the planned alternative
(return an error if Geth-compliant behavior is requested) so users understand
the difference in default response shape.
---
Duplicate comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3399-3412: The loop that builds TipsetTraceEntry increments
msg_idx before storing it, making msg_position one-based; update the logic in
the loop that iterates over raw_traces (using variables msg_idx, ir, and
building TipsetTraceEntry with msg_position) so that msg_idx remains zero-based
when assigned to TipsetTraceEntry (e.g., assign msg_idx to msg_position before
incrementing, or increment msg_idx after pushing the entry) to preserve
zero-based transaction_position across trace helpers like trace_block,
trace_replayBlockTransactions, and debug_traceTransaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e97f8f3-9caa-4829-a4b6-c3e45dd10b3e
📒 Files selected for processing (5)
docs/docs/developers/guides/debug_trace_transaction_guide.mddocs/docs/users/knowledge_base/rpc/debug_trace_transaction.mdmise.tomlsrc/rpc/methods/eth.rssrc/rpc/methods/eth/types.rs
💤 Files with no reviewable changes (1)
- mise.toml
akaladarshi
force-pushed
the
akaladarshi/add-debug-trace-txn-api
branch
from
916fac3 to
a4f765e
Compare
6 tasks
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
2 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:
Debug_TraceTransactionAPIReference issue to close (if applicable)
Closes #6675
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Documentation