Fix Morph trace replay on Cancun-active chains#57
Conversation
📝 WalkthroughWalkthroughThis pull request adds tracing RPC endpoint support for Morph L2 nodes by introducing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/rpc/src/eth/mod.rs (1)
368-378: Consider documenting the trace accuracy trade-offs.The no-op implementation correctly avoids requiring
parent_beacon_block_rooton Cancun-active L2 chains, but it also skips setup thatMorphBlockExecutor::apply_pre_execution_changes(crates/evm/src/block/mod.rs:227-275) performs:
- State clear flag for Spurious Dragon
- L1 Gas Price Oracle cache warming
- Curie hardfork system contract initialization
For most trace scenarios this is fine since actual execution already applied these. However, traces for blocks at the Curie activation boundary or transactions depending on L1 fee oracle reads may differ from actual execution. Consider adding a doc comment noting this is intentional L2 behavior and its scope.
📝 Suggested documentation addition
fn apply_pre_execution_changes<DB: Send + reth_evm::Database + revm::DatabaseCommit>( &self, _block: &RecoveredBlock<ProviderBlock<Self::Provider>>, _db: &mut DB, _evm_env: &reth_evm::EvmEnvFor<Self::Evm>, ) -> Result<(), Self::Error> { // Morph L2 does not execute Ethereum pre-block system calls such as // EIP-2935/EIP-4788 during block replay. Using the upstream default here // incorrectly requires `parent_beacon_block_root` on Cancun-active chains. + // + // Note: This intentionally skips L1 Gas Price Oracle cache warming and + // Curie hardfork initialization that MorphBlockExecutor performs. For + // already-executed blocks this is safe; trace results should match + // actual execution in all but edge cases (Curie transition blocks). Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/src/eth/mod.rs` around lines 368 - 378, Add a doc comment above the apply_pre_execution_changes function in crates/rpc/src/eth/mod.rs that explains this no-op is intentional for L2 to avoid requiring parent_beacon_block_root, but that it skips the same setup MorphBlockExecutor::apply_pre_execution_changes performs (state clear flag for Spurious Dragon, L1 Gas Price Oracle cache warming, Curie hardfork system contract initialization), and that traces at Curie activation boundaries or transactions relying on L1 fee-oracle reads may differ from on-chain execution; reference the function name apply_pre_execution_changes and MorphBlockExecutor::apply_pre_execution_changes for context and clearly state the scope where this trade-off is acceptable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/node/tests/debug_trace.rs`:
- Around line 134-135: Replace the post-default field assignment on the
GethDebugTracingCallOptions instance with struct update syntax: instead of
creating opts via GethDebugTracingCallOptions::default() and then setting
opts.tx_index = Some(0), construct opts using GethDebugTracingCallOptions {
tx_index: Some(0), ..Default::default() } so the tx_index is set during
initialization and avoids the clippy field-reassign-with-default lint.
---
Nitpick comments:
In `@crates/rpc/src/eth/mod.rs`:
- Around line 368-378: Add a doc comment above the apply_pre_execution_changes
function in crates/rpc/src/eth/mod.rs that explains this no-op is intentional
for L2 to avoid requiring parent_beacon_block_root, but that it skips the same
setup MorphBlockExecutor::apply_pre_execution_changes performs (state clear flag
for Spurious Dragon, L1 Gas Price Oracle cache warming, Curie hardfork system
contract initialization), and that traces at Curie activation boundaries or
transactions relying on L1 fee-oracle reads may differ from on-chain execution;
reference the function name apply_pre_execution_changes and
MorphBlockExecutor::apply_pre_execution_changes for context and clearly state
the scope where this trade-off is acceptable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c73ddef2-7ea2-4f87-abac-b66c12044097
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlcrates/node/Cargo.tomlcrates/node/tests/debug_trace.rscrates/rpc/src/eth/mod.rs
chengwenxi
left a comment
There was a problem hiding this comment.
Code review
No issues found. Checked for bugs and CLAUDE.md compliance.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
Summary
Test Plan
Summary by CodeRabbit
New Features
Chores