refactor: prepare for the changes in debug trace transaction#6731
refactor: prepare for the changes in debug trace transaction#6731akaladarshi wants to merge 2 commits intomainfrom
Conversation
WalkthroughRefactors Ethereum trace generation to a two-stage flow: execution now returns a StateTree plus TipsetTraceEntry items; parity-format traces are built later from those entries. Adds TipsetExecutor to centralize VM/tipset preparation and moves trace-related types into types.rs with new helpers in trace.rs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RPC as RPC Handler
participant Exec as execute_tipset_traces
participant SM as StateManager/TipsetExecutor
participant Entry as TipsetTraceEntry
participant Builder as Parity Trace Builder
RPC->>Exec: request execute_tipset_traces(ctx, ts, ext)
Exec->>SM: spawn blocking task → run tipset execution
SM-->>Exec: (StateTree, Vec<TipsetTraceEntry>)
Exec-->>RPC: return (StateTree, entries)
loop per entry
RPC->>Entry: entry.build_parity_traces(&StateTree)
Entry->>Builder: derive parity-style EthTrace items
Builder-->>Entry: Vec<EthTrace>
Entry-->>RPC: EthTrace results
end
RPC->>RPC: assemble EthBlockTrace from EthTrace results
RPC-->>Client: return trace response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/rpc/methods/eth/trace.rs (1)
1488-1521: Make the helper tests fail when fixture setup is missing.The
if let Some(actor)guards make these tests pass vacuously if the EVM actor bundle metadata or actor-state setup is unavailable. Replacing them withexpect(...)keeps the new coverage meaningful.🤖 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 1488 - 1521, The tests use `if let Some(actor) = create_evm_actor_with_bytecode(...)` which allows them to pass vacuously when fixtures are missing; replace those `if let Some(...)` guards with direct unwraps via `expect("...")` (or `unwrap()` with a clear message) so the tests fail if the EVM actor fixture or actor-state setup is not created; update the three tests that call `create_evm_actor_with_bytecode` (and the one that uses `create_test_actor` if applicable) to call `create_evm_actor_with_bytecode(...).expect("failed to create EVM actor fixture")` or `create_test_actor(...).expect("failed to create test actor")`, leaving assertions that call `actor_bytecode(...)` and `actor_nonce(...)` unchanged.src/state_manager/mod.rs (1)
1897-1967: Add phase/tipset context to the new helper's fallible calls.
TipsetExecutornow owns parent-tipset loading, circulating-supply lookup, VM creation, and block-message loading, but those errors still bubble up bare. When this path fails during state replay or tracing, it's hard to tell which stage or epoch failed. Wrapping the new fallible calls with.context(...)would make these failures much easier to diagnose.♻️ Example tightening
- let circ_supply = self.genesis_info.get_vm_circulating_supply( + let circ_supply = self.genesis_info.get_vm_circulating_supply( epoch, self.chain_index.db(), &state_root, - )?; - VM::new( + ) + .with_context(|| format!("failed to compute circulating supply for tipset={}, epoch={epoch}", self.tipset.key()))?; + VM::new( ExecutionContext { heaviest_tipset: self.tipset.clone(), state_tree_root: state_root, @@ - trace, - ) + trace, + ) + .with_context(|| format!("failed to create VM for tipset={}, epoch={epoch}", self.tipset.key())) @@ - let mut parent_state = *self.tipset.parent_state(); - let parent_epoch = - Tipset::load_required(self.chain_index.db(), self.tipset.parents())?.epoch(); + let mut parent_state = *self.tipset.parent_state(); + let parent_epoch = Tipset::load_required(self.chain_index.db(), self.tipset.parents()) + .with_context(|| format!("failed to load parent tipset for {}", self.tipset.key()))? + .epoch(); @@ - let block_messages = BlockMessages::for_tipset(self.chain_index.db(), &self.tipset)?; + let block_messages = BlockMessages::for_tipset(self.chain_index.db(), &self.tipset) + .with_context(|| format!("failed to load block messages for {}", self.tipset.key()))?;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/state_manager/mod.rs` around lines 1897 - 1967, The prepare_parent_state path performs several fallible operations without context; wrap each call that returns anyhow::Result with .context(...) to indicate phase/tipset/epoch (e.g., annotate Tipset::load_required(self.chain_index.db(), self.tipset.parents())? with context like "loading parent tipset for tipset={:?}" including self.tipset, annotate get_vm_circulating_supply(...) in create_vm, the create_vm(...) call and vm.run_cron(...) invocation inside the loop (include epoch_i), run_state_migrations(...)? return, and BlockMessages::for_tipset(...) with clear messages identifying the phase and epoch/tipset); update prepare_parent_state, create_vm, and the loop so errors bubble up with .context(...) so failures show which stage (parent-load, circ-supply, vm-creation, cron, migration, or block-message load) and the relevant tipset/epoch.
🤖 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.rs`:
- Around line 3412-3425: The loop uses msg_idx (initialized to 0) but increments
it before creating each TipsetTraceEntry, making msg_position one-based; change
the logic so transaction positions remain zero-based by using the current
msg_idx when constructing TipsetTraceEntry and only incrementing msg_idx after
pushing the entry (or start msg_idx at 0 and increment after push) in the loop
that iterates raw_traces where EthGetTransactionHashByCid::handle(...) is called
and TipsetTraceEntry { tx_hash, msg_position: msg_idx, ... } is created.
In `@src/rpc/methods/eth/trace.rs`:
- Around line 678-689: The function actor_nonce currently masks evm::State::load
failures by returning actor.sequence (producing wrong EVM nonces); change
actor_nonce to return anyhow::Result<EthUint64> and replace the unwrap_or
fallback with propagating the load error using .context("failed to load EVM
state for actor nonce") so genuine read errors surface. Update callers such as
build_account_diff to handle the Result (use ? to propagate) and adjust any
signature to return anyhow::Result as needed, ensuring all paths propagate the
error instead of fabricating a nonce value.
---
Nitpick comments:
In `@src/rpc/methods/eth/trace.rs`:
- Around line 1488-1521: The tests use `if let Some(actor) =
create_evm_actor_with_bytecode(...)` which allows them to pass vacuously when
fixtures are missing; replace those `if let Some(...)` guards with direct
unwraps via `expect("...")` (or `unwrap()` with a clear message) so the tests
fail if the EVM actor fixture or actor-state setup is not created; update the
three tests that call `create_evm_actor_with_bytecode` (and the one that uses
`create_test_actor` if applicable) to call
`create_evm_actor_with_bytecode(...).expect("failed to create EVM actor
fixture")` or `create_test_actor(...).expect("failed to create test actor")`,
leaving assertions that call `actor_bytecode(...)` and `actor_nonce(...)`
unchanged.
In `@src/state_manager/mod.rs`:
- Around line 1897-1967: The prepare_parent_state path performs several fallible
operations without context; wrap each call that returns anyhow::Result with
.context(...) to indicate phase/tipset/epoch (e.g., annotate
Tipset::load_required(self.chain_index.db(), self.tipset.parents())? with
context like "loading parent tipset for tipset={:?}" including self.tipset,
annotate get_vm_circulating_supply(...) in create_vm, the create_vm(...) call
and vm.run_cron(...) invocation inside the loop (include epoch_i),
run_state_migrations(...)? return, and BlockMessages::for_tipset(...) with clear
messages identifying the phase and epoch/tipset); update prepare_parent_state,
create_vm, and the loop so errors bubble up with .context(...) so failures show
which stage (parent-load, circ-supply, vm-creation, cron, migration, or
block-message load) and the relevant tipset/epoch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43ffd8a5-45fb-4395-ba7c-2bb99cee88ad
⛔ 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 (4)
src/rpc/methods/eth.rssrc/rpc/methods/eth/trace.rssrc/rpc/methods/eth/types.rssrc/state_manager/mod.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| /// Returns the effective nonce for an actor: EVM nonce for EVM actors, sequence otherwise. |
There was a problem hiding this comment.
I think some of those methods could be potentially used outside of the trace module; would it make sense to add them to some utils module? Also, it might be slightly nicer to actually add a trait an implement it for ActorState instead of relying on some free functions.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.rs`:
- Around line 689-700: Change actor_bytecode to return
anyhow::Result<Option<EthBytes>> and stop collapsing errors into None: call
evm::State::load(store, actor.code, actor.state) and the subsequent
store.get(...) using ? (propagating errors) and add context() messages for both
failure points so callers (e.g., build_account_diff) can distinguish "no code"
(Ok(None)) from IO/deserialization failures; update the other similar helper at
the second occurrence (the lines referenced around 751-752) to the same
signature and error-handling pattern, and adjust callers (build_account_diff) to
handle the Result<Option<...>> accordingly.
- Around line 136-153: The current guard uses trace_is_evm_or_eam(trace) which
in turn treats any actor with invoked_actor.id != ETHEREUM_ACCOUNT_MANAGER_ACTOR
as EVM, causing non-EVM exits to be misclassified; update the guard/helper so it
only returns true for actual EVM or EAM actors (e.g., change trace_is_evm_or_eam
to check invoked_actor.id equals the EVM actor ID or equals
ETHEREUM_ACCOUNT_MANAGER_ACTOR, or better check the invoked actor's code/type
explicitly), then keep the mapping of evm12::EVM_CONTRACT_* codes (the match in
trace.rs that returns PARITY_* constants) so it only runs for true EVM/EAM
actors.
- Around line 1486-1519: The three tests (test_actor_nonce_evm,
test_actor_bytecode_evm, test_actor_bytecode_evm_no_bytecode) currently use
conditional `if let Some(actor) = create_evm_actor_with_bytecode(...)` which
silently skips assertions when the fixture is missing; change each to
unwrap/expect the fixture (e.g., let actor =
create_evm_actor_with_bytecode(...).expect("failed to create EVM actor
fixture")) so the test fails loudly if the EVM code CID/manifest cannot be
created, and update test_actor_bytecode_evm and
test_actor_bytecode_evm_no_bytecode similarly to use expect instead of `if let
Some(...)`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ad969f4b-6e2e-4353-b3f1-3b26428b4a1a
📒 Files selected for processing (1)
src/rpc/methods/eth/trace.rs
| if trace_is_evm_or_eam(trace) { | ||
| match code.into() { | ||
| evm12::EVM_CONTRACT_REVERTED => return Some("Reverted".into()), // capitalized for compatibility | ||
| evm12::EVM_CONTRACT_INVALID_INSTRUCTION => return Some("invalid instruction".into()), | ||
| evm12::EVM_CONTRACT_REVERTED => return Some(PARITY_TRACE_REVERT_ERROR.into()), // capitalized for compatibility | ||
| evm12::EVM_CONTRACT_INVALID_INSTRUCTION => { | ||
| return Some(PARITY_EVM_INVALID_INSTRUCTION.into()); | ||
| } | ||
| evm12::EVM_CONTRACT_UNDEFINED_INSTRUCTION => { | ||
| return Some("undefined instruction".into()); | ||
| return Some(PARITY_EVM_UNDEFINED_INSTRUCTION.into()); | ||
| } | ||
| evm12::EVM_CONTRACT_STACK_UNDERFLOW => return Some("stack underflow".into()), | ||
| evm12::EVM_CONTRACT_STACK_OVERFLOW => return Some("stack overflow".into()), | ||
| evm12::EVM_CONTRACT_STACK_UNDERFLOW => return Some(PARITY_EVM_STACK_UNDERFLOW.into()), | ||
| evm12::EVM_CONTRACT_STACK_OVERFLOW => return Some(PARITY_EVM_STACK_OVERFLOW.into()), | ||
| evm12::EVM_CONTRACT_ILLEGAL_MEMORY_ACCESS => { | ||
| return Some("illegal memory access".into()); | ||
| return Some(PARITY_EVM_ILLEGAL_MEMORY_ACCESS.into()); | ||
| } | ||
| evm12::EVM_CONTRACT_BAD_JUMPDEST => return Some(PARITY_EVM_BAD_JUMPDEST.into()), | ||
| evm12::EVM_CONTRACT_SELFDESTRUCT_FAILED => { | ||
| return Some(PARITY_EVM_SELFDESTRUCT_FAILED.into()); | ||
| } |
There was a problem hiding this comment.
Fix the EVM/EAM guard before extending this mapping.
This branch still relies on trace_is_evm_or_eam(), and that helper currently returns true for every non-EAM actor because it uses invoked_actor.id != ETHEREUM_ACCOUNT_MANAGER_ACTOR. Any unrelated actor exit code that collides numerically with an EVM code can therefore be mislabeled as a Parity EVM error here.
🛠️ Minimal fix
fn trace_is_evm_or_eam(trace: &ExecutionTrace) -> bool {
if let Some(invoked_actor) = &trace.invoked_actor {
+ let eam_actor_id = Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR
+ .id()
+ .expect("EAM actor address should be an ID address");
is_evm_actor(&invoked_actor.state.code)
- || invoked_actor.id != Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap()
+ || invoked_actor.id == eam_actor_id
} else {
false
}
}🤖 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 136 - 153, The current guard uses
trace_is_evm_or_eam(trace) which in turn treats any actor with invoked_actor.id
!= ETHEREUM_ACCOUNT_MANAGER_ACTOR as EVM, causing non-EVM exits to be
misclassified; update the guard/helper so it only returns true for actual EVM or
EAM actors (e.g., change trace_is_evm_or_eam to check invoked_actor.id equals
the EVM actor ID or equals ETHEREUM_ACCOUNT_MANAGER_ACTOR, or better check the
invoked actor's code/type explicitly), then keep the mapping of
evm12::EVM_CONTRACT_* codes (the match in trace.rs that returns PARITY_*
constants) so it only runs for true EVM/EAM actors.
| /// Returns the deployed bytecode of an EVM actor, or `None` for non-EVM actors. | ||
| fn actor_bytecode<DB: Blockstore>(store: &DB, actor: &ActorState) -> Option<EthBytes> { | ||
| if !is_evm_actor(&actor.code) { | ||
| return None; | ||
| } | ||
| let evm_state = evm::State::load(store, actor.code, actor.state).ok()?; | ||
| store | ||
| .get(&evm_state.bytecode()) | ||
| .ok() | ||
| .flatten() | ||
| .map(EthBytes) | ||
| } |
There was a problem hiding this comment.
Don’t collapse bytecode/state read failures into “no code”.
actor_bytecode() swallows both evm::State::load() and store.get() errors with ok()?, so build_account_diff() can emit unchanged/added/removed code deltas even when the underlying EVM state or bytecode read failed. This should return anyhow::Result<Option<EthBytes>> and only treat Ok(None) as “no deployed bytecode.”
🛠️ Proposed fix
-fn actor_bytecode<DB: Blockstore>(store: &DB, actor: &ActorState) -> Option<EthBytes> {
+fn actor_bytecode<DB: Blockstore>(
+ store: &DB,
+ actor: &ActorState,
+) -> anyhow::Result<Option<EthBytes>> {
if !is_evm_actor(&actor.code) {
- return None;
+ return Ok(None);
}
- let evm_state = evm::State::load(store, actor.code, actor.state).ok()?;
- store
- .get(&evm_state.bytecode())
- .ok()
- .flatten()
- .map(EthBytes)
+ let evm_state = evm::State::load(store, actor.code, actor.state)
+ .context("failed to load EVM state for bytecode")?;
+ Ok(
+ store
+ .get(&evm_state.bytecode())
+ .context("failed to load EVM bytecode")?
+ .map(EthBytes),
+ )
}- let pre_code = pre_actor.and_then(|a| actor_bytecode(store, a));
- let post_code = post_actor.and_then(|a| actor_bytecode(store, a));
+ let pre_code = pre_actor.map(|a| actor_bytecode(store, a)).transpose()?.flatten();
+ let post_code = post_actor.map(|a| actor_bytecode(store, a)).transpose()?.flatten();As per coding guidelines: Use anyhow::Result<T> for most operations and add context with .context() when errors occur.
Also applies to: 751-752
🤖 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 689 - 700, Change actor_bytecode
to return anyhow::Result<Option<EthBytes>> and stop collapsing errors into None:
call evm::State::load(store, actor.code, actor.state) and the subsequent
store.get(...) using ? (propagating errors) and add context() messages for both
failure points so callers (e.g., build_account_diff) can distinguish "no code"
(Ok(None)) from IO/deserialization failures; update the other similar helper at
the second occurrence (the lines referenced around 751-752) to the same
signature and error-handling pattern, and adjust callers (build_account_diff) to
handle the Result<Option<...>> accordingly.
| fn test_actor_nonce_evm() { | ||
| let store = Arc::new(MemoryDB::default()); | ||
| if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 7, Some(&[0x60])) { | ||
| let nonce = actor_nonce(store.as_ref(), &actor).unwrap(); | ||
| // EVM actors use the EVM nonce field, not the actor sequence | ||
| assert_eq!(nonce.0, 7); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_actor_bytecode_non_evm() { | ||
| let store = MemoryDB::default(); | ||
| let actor = create_test_actor(1000, 0); | ||
| assert!(actor_bytecode(&store, &actor).is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_actor_bytecode_evm() { | ||
| let store = Arc::new(MemoryDB::default()); | ||
| let bytecode = &[0x60, 0x80, 0x60, 0x40, 0x52]; | ||
| if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 1, Some(bytecode)) { | ||
| let result = actor_bytecode(store.as_ref(), &actor); | ||
| assert_eq!(result, Some(EthBytes(bytecode.to_vec()))); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_actor_bytecode_evm_no_bytecode() { | ||
| let store = Arc::new(MemoryDB::default()); | ||
| if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 1, None) { | ||
| // No bytecode stored => None (Cid::default() won't resolve to raw data) | ||
| let result = actor_bytecode(store.as_ref(), &actor); | ||
| assert!(result.is_none()); | ||
| } |
There was a problem hiding this comment.
Make these tests fail if the EVM fixture can't be created.
Each if let Some(actor) turns a missing EVM code CID/manifest into a vacuous pass, so these tests can stop asserting anything without going red. Please convert fixture creation to expect(...) so bundle regressions fail immediately.
🧪 Tighten the assertions
let store = Arc::new(MemoryDB::default());
- if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 7, Some(&[0x60])) {
- let nonce = actor_nonce(store.as_ref(), &actor).unwrap();
- // EVM actors use the EVM nonce field, not the actor sequence
- assert_eq!(nonce.0, 7);
- }
+ let actor = create_evm_actor_with_bytecode(&store, 1000, 0, 7, Some(&[0x60]))
+ .expect("expected EVM actor fixture");
+ let nonce = actor_nonce(store.as_ref(), &actor).unwrap();
+ // EVM actors use the EVM nonce field, not the actor sequence
+ assert_eq!(nonce.0, 7);Apply the same pattern to test_actor_bytecode_evm and test_actor_bytecode_evm_no_bytecode.
🤖 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 1486 - 1519, The three tests
(test_actor_nonce_evm, test_actor_bytecode_evm,
test_actor_bytecode_evm_no_bytecode) currently use conditional `if let
Some(actor) = create_evm_actor_with_bytecode(...)` which silently skips
assertions when the fixture is missing; change each to unwrap/expect the fixture
(e.g., let actor = create_evm_actor_with_bytecode(...).expect("failed to create
EVM actor fixture")) so the test fails loudly if the EVM code CID/manifest
cannot be created, and update test_actor_bytecode_evm and
test_actor_bytecode_evm_no_bytecode similarly to use expect instead of `if let
Some(...)`.
| pub value: Option<EthBigInt>, | ||
| // Some clients use `input`, others use `data`. We have to support both. | ||
| #[serde(alias = "input", skip_serializing_if = "Option::is_none", default)] | ||
| // Some clients use `input`, others use `data`; both accepted, `input` takes precedence. |
| TraceResult::Call(r) => r.output.clone(), | ||
| TraceResult::Create(r) => r.code.clone(), |
There was a problem hiding this comment.
Could we avoid cloning given we're grabbing traces ownership anyway?
| } | ||
|
|
||
| async fn eth_trace_block<DB>( | ||
| struct TipsetTraceEntry { |
There was a problem hiding this comment.
If we're refactoring things, I think it'd make sense to move trace-related types to trace/types.rs. This module is already huge.
| let mut vm = create_vm(parent_state, epoch, tipset.min_timestamp())?; | ||
| let mut vm = exec.create_vm(parent_state, epoch, tipset.min_timestamp(), enable_tracing)?; | ||
|
|
||
| // step 4: apply tipset messages |
Summary of changes
Changes introduced in this pull request:
DebugTraceTransactionAPI implementationReference issue to close (if applicable)
Contains the part of this PR: #6685
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests