diff --git a/prdoc/pr_10239.prdoc b/prdoc/pr_10239.prdoc new file mode 100644 index 0000000000000..b753fe23166aa --- /dev/null +++ b/prdoc/pr_10239.prdoc @@ -0,0 +1,7 @@ +title: '[pallet-revive] fix prestate tracer current address' +doc: +- audience: Runtime Dev + description: Fix prestate tracer not reporting the contract addresses properly. +crates: +- name: pallet-revive + bump: patch diff --git a/substrate/frame/revive/fixtures/contracts/Counter.sol b/substrate/frame/revive/fixtures/contracts/Counter.sol new file mode 100644 index 0000000000000..e4bde49eaab0f --- /dev/null +++ b/substrate/frame/revive/fixtures/contracts/Counter.sol @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.4; +contract Counter { + uint256 public number; + + constructor() { + number = 3; + } + + function setNumber(uint256 newNumber) public returns (uint256) { + number = newNumber; + } + + function increment() public { + number++; + } +} + +contract NestedCounter { + Counter public counter; + uint256 public number; + + + constructor() { + counter = new Counter(); + counter.setNumber(10); + number = 7; + } + + function nestedNumber() public returns (uint256) { + uint256 currentNumber = counter.setNumber(number); + number++; + return currentNumber; + } +} diff --git a/substrate/frame/revive/src/evm/api/debug_rpc_types.rs b/substrate/frame/revive/src/evm/api/debug_rpc_types.rs index b3a547c59eaf4..008dd683bcea2 100644 --- a/substrate/frame/revive/src/evm/api/debug_rpc_types.rs +++ b/substrate/frame/revive/src/evm/api/debug_rpc_types.rs @@ -21,6 +21,7 @@ use codec::{Decode, Encode}; use derive_more::From; use scale_info::TypeInfo; use serde::{ + de::{Error, MapAccess, Visitor}, ser::{SerializeMap, Serializer}, Deserialize, Serialize, }; @@ -166,7 +167,7 @@ pub enum CallType { } /// A Trace -#[derive(TypeInfo, From, Encode, Decode, Serialize, Deserialize, Clone, Debug, Eq, PartialEq)] +#[derive(TypeInfo, Deserialize, Serialize, From, Encode, Decode, Clone, Debug, Eq, PartialEq)] #[serde(untagged)] pub enum Trace { /// A call trace. @@ -176,7 +177,7 @@ pub enum Trace { } /// A prestate Trace -#[derive(TypeInfo, Encode, Decode, Serialize, Deserialize, Clone, Debug, Eq, PartialEq)] +#[derive(TypeInfo, Encode, Serialize, Decode, Clone, Debug, Eq, PartialEq)] #[serde(untagged)] pub enum PrestateTrace { /// The Prestate mode returns the accounts necessary to execute a given transaction @@ -196,6 +197,68 @@ pub enum PrestateTrace { }, } +impl<'de> Deserialize<'de> for PrestateTrace { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct PrestateTraceVisitor; + + impl<'de> Visitor<'de> for PrestateTraceVisitor { + type Value = PrestateTrace; + + fn expecting(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result { + formatter.write_str("a map representing either Prestate or DiffMode") + } + + fn visit_map(self, mut map: A) -> Result + where + A: MapAccess<'de>, + { + let mut pre_map = None; + let mut post_map = None; + let mut account_map = BTreeMap::new(); + + while let Some(key) = map.next_key::()? { + match key.as_str() { + "pre" => { + if pre_map.is_some() { + return Err(Error::duplicate_field("pre")); + } + pre_map = Some(map.next_value::>()?); + }, + "post" => { + if post_map.is_some() { + return Err(Error::duplicate_field("post")); + } + post_map = Some(map.next_value::>()?); + }, + _ => { + let addr: H160 = + key.parse().map_err(|_| Error::custom("Invalid address"))?; + let info = map.next_value::()?; + account_map.insert(addr, info); + }, + } + } + + match (pre_map, post_map) { + (Some(pre), Some(post)) => { + if !account_map.is_empty() { + return Err(Error::custom("Mixed diff and prestate mode")); + } + Ok(PrestateTrace::DiffMode { pre, post }) + }, + (None, None) => Ok(PrestateTrace::Prestate(account_map)), + _ => Err(Error::custom("diff mode: must have both 'pre' and 'post'")), + } + } + } + + deserializer.deserialize_map(PrestateTraceVisitor) + } +} + impl PrestateTrace { /// Returns the pre and post trace info. pub fn state_mut( @@ -223,7 +286,7 @@ pub struct PrestateTraceInfo { #[serde(skip_serializing_if = "Option::is_none")] pub code: Option, /// The storage of the contract account. - #[serde(skip_serializing_if = "is_empty", serialize_with = "serialize_map_skip_none")] + #[serde(default, skip_serializing_if = "is_empty", serialize_with = "serialize_map_skip_none")] pub storage: BTreeMap>, } diff --git a/substrate/frame/revive/src/evm/tracing/prestate_tracing.rs b/substrate/frame/revive/src/evm/tracing/prestate_tracing.rs index e6bdf843be223..ec98539f470c5 100644 --- a/substrate/frame/revive/src/evm/tracing/prestate_tracing.rs +++ b/substrate/frame/revive/src/evm/tracing/prestate_tracing.rs @@ -19,7 +19,10 @@ use crate::{ tracing::Tracing, AccountInfo, Code, Config, ExecReturnValue, Key, Pallet, PristineCode, Weight, }; -use alloc::{collections::BTreeMap, vec::Vec}; +use alloc::{ + collections::{BTreeMap, BTreeSet}, + vec::Vec, +}; use sp_core::{H160, U256}; /// A tracer that traces the prestate. @@ -28,11 +31,14 @@ pub struct PrestateTracer { /// The tracer configuration. config: PrestateTracerConfig, - /// The current address of the contract's which storage is being accessed. - current_addr: H160, + /// Stack of calls. + calls: Vec, + + /// The code used by create transaction + create_code: Option, - /// Whether the current call is a contract creation. - is_create: Option, + /// List of created contracts addresses. + created_addrs: BTreeSet, // pre / post state trace: (BTreeMap, BTreeMap), @@ -49,6 +55,10 @@ where Self { config, ..Default::default() } } + fn current_addr(&self) -> H160 { + self.calls.last().copied().unwrap_or_default() + } + /// Returns an empty trace. pub fn empty_trace(&self) -> PrestateTrace { if self.config.diff_mode { @@ -71,6 +81,14 @@ where }; if self.config.diff_mode { + if include_code { + for addr in &self.created_addrs { + if let Some(info) = post.get_mut(addr) { + info.code = Self::bytecode(addr); + } + } + } + // clean up the storage that are in pre but not in post these are just read pre.iter_mut().for_each(|(addr, info)| { if let Some(post_info) = post.get(addr) { @@ -154,6 +172,22 @@ where info.nonce = if nonce > 0 { Some(nonce) } else { None }; info } + + /// Record a read + fn read_account_prestate(&mut self, addr: H160) { + if self.created_addrs.contains(&addr) { + return + } + + let include_code = !self.config.disable_code; + self.trace.0.entry(addr).or_insert_with_key(|addr| { + Self::prestate_info( + addr, + Pallet::::evm_balance(addr), + include_code.then(|| Self::bytecode(addr)).flatten(), + ) + }); + } } impl Tracing for PrestateTracer @@ -172,7 +206,7 @@ where } fn instantiate_code(&mut self, code: &crate::Code, _salt: Option<&[u8; 32]>) { - self.is_create = Some(code.clone()); + self.create_code = Some(code.clone()); } fn enter_child_span( @@ -185,66 +219,49 @@ where _input: &[u8], _gas: Weight, ) { - let include_code = !self.config.disable_code; - self.trace.0.entry(from).or_insert_with_key(|addr| { - Self::prestate_info( - addr, - Pallet::::evm_balance(addr), - include_code.then(|| Self::bytecode(addr)).flatten(), - ) - }); - - if self.is_create.is_none() { - self.trace.0.entry(to).or_insert_with_key(|addr| { - Self::prestate_info( - addr, - Pallet::::evm_balance(addr), - include_code.then(|| Self::bytecode(addr)).flatten(), - ) - }); + if is_delegate_call { + self.calls.push(self.current_addr()); + } else { + self.calls.push(to); } - if !is_delegate_call { - self.current_addr = to; + if self.create_code.take().is_some() { + self.created_addrs.insert(to); } + self.read_account_prestate(from); + self.read_account_prestate(to); } fn exit_child_span_with_error(&mut self, _error: crate::DispatchError, _gas_used: Weight) { - self.is_create = None; + self.calls.pop(); } fn exit_child_span(&mut self, output: &ExecReturnValue, _gas_used: Weight) { - let create_code = self.is_create.take(); + let current_addr = self.calls.pop().unwrap_or_default(); if output.did_revert() { return } - let code = if self.config.disable_code { - None - } else if let Some(code) = create_code { - match code { - Code::Upload(code) => Some(code.into()), - Code::Existing(code_hash) => - PristineCode::::get(&code_hash).map(|code| Bytes::from(code.to_vec())), - } - } else { - Self::bytecode(&self.current_addr) - }; + let code = if self.config.disable_code { None } else { Self::bytecode(¤t_addr) }; Self::update_prestate_info( - self.trace.1.entry(self.current_addr).or_default(), - &self.current_addr, + self.trace.1.entry(current_addr).or_default(), + ¤t_addr, code, ); } fn storage_write(&mut self, key: &Key, old_value: Option>, new_value: Option<&[u8]>) { + let current_addr = self.current_addr(); + if self.created_addrs.contains(¤t_addr) { + return + } let key = Bytes::from(key.unhashed().to_vec()); let old_value = self .trace .0 - .entry(self.current_addr) + .entry(current_addr) .or_default() .storage .entry(key.clone()) @@ -257,19 +274,24 @@ where if old_value.as_ref().map(|v| v.0.as_ref()) != new_value { self.trace .1 - .entry(self.current_addr) + .entry(current_addr) .or_default() .storage .insert(key, new_value.map(|v| v.to_vec().into())); } else { - self.trace.1.entry(self.current_addr).or_default().storage.remove(&key); + self.trace.1.entry(self.current_addr()).or_default().storage.remove(&key); } } fn storage_read(&mut self, key: &Key, value: Option<&[u8]>) { + let current_addr = self.current_addr(); + if self.created_addrs.contains(¤t_addr) { + return + } + self.trace .0 - .entry(self.current_addr) + .entry(current_addr) .or_default() .storage .entry(key.unhashed().to_vec().into()) @@ -277,6 +299,10 @@ where } fn balance_read(&mut self, addr: &H160, value: U256) { + if self.created_addrs.contains(&addr) { + return + } + let include_code = !self.config.disable_code; self.trace.0.entry(*addr).or_insert_with_key(|addr| { Self::prestate_info(addr, value, include_code.then(|| Self::bytecode(addr)).flatten()) diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index c2a765981489d..f157d5d0ac7ba 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -4240,19 +4240,31 @@ fn prestate_tracing_works() { // redact balance so that tests are resilient to weight changes let alice_redacted_balance = Some(U256::from(1)); - let test_cases: Vec<(Box, _, _)> = vec![ - ( - Box::new(|| { - builder::bare_call(addr) - .data((3u32, addr_callee).encode()) - .build_and_unwrap_result(); + struct TestCase { + description: &'static str, + exec_call: Box, + config: PrestateTracerConfig, + expected_trace: PrestateTrace, + } + + let test_cases: Vec = vec![ + TestCase { + description: "prestate mode with cross-contract call", + exec_call: Box::new({ + let addr = addr; + let addr_callee = addr_callee; + move || { + builder::bare_call(addr) + .data((3u32, addr_callee).encode()) + .build_and_unwrap_result(); + } }), - PrestateTracerConfig { + config: PrestateTracerConfig { diff_mode: false, disable_storage: false, disable_code: false, }, - PrestateTrace::Prestate(BTreeMap::from([ + expected_trace: PrestateTrace::Prestate(BTreeMap::from([ ( ALICE_ADDR, PrestateTraceInfo { @@ -4284,19 +4296,24 @@ fn prestate_tracing_works() { }, ), ])), - ), - ( - Box::new(|| { - builder::bare_call(addr) - .data((3u32, addr_callee).encode()) - .build_and_unwrap_result(); + }, + TestCase { + description: "diff mode with cross-contract call", + exec_call: Box::new({ + let addr = addr; + let addr_callee = addr_callee; + move || { + builder::bare_call(addr) + .data((3u32, addr_callee).encode()) + .build_and_unwrap_result(); + } }), - PrestateTracerConfig { + config: PrestateTracerConfig { diff_mode: true, disable_storage: false, disable_code: false, }, - PrestateTrace::DiffMode { + expected_trace: PrestateTrace::DiffMode { pre: BTreeMap::from([ ( BOB_ADDR, @@ -4332,19 +4349,23 @@ fn prestate_tracing_works() { ), ]), }, - ), - ( - Box::new(|| { - builder::bare_instantiate(Code::Upload(dummy_code.clone())) - .salt(None) - .build_and_unwrap_result(); + }, + TestCase { + description: "diff mode with contract instantiation", + exec_call: Box::new({ + let dummy_code = dummy_code.clone(); + move || { + builder::bare_instantiate(Code::Upload(dummy_code)) + .salt(None) + .build_and_unwrap_result(); + } }), - PrestateTracerConfig { + config: PrestateTracerConfig { diff_mode: true, disable_storage: false, disable_code: false, }, - PrestateTrace::DiffMode { + expected_trace: PrestateTrace::DiffMode { pre: BTreeMap::from([( ALICE_ADDR, PrestateTraceInfo { @@ -4373,10 +4394,10 @@ fn prestate_tracing_works() { ), ]), }, - ), + }, ]; - for (exec_call, config, expected_trace) in test_cases.into_iter() { + for TestCase { description, exec_call, config, expected_trace } in test_cases.into_iter() { let mut tracer = PrestateTracer::::new(config); trace(&mut tracer, || { exec_call(); @@ -4401,7 +4422,7 @@ fn prestate_tracing_works() { }, } - assert_eq!(trace, expected_trace); + assert_eq!(trace, expected_trace, "Trace mismatch for: {description}"); } }); } diff --git a/substrate/frame/revive/src/tests/sol.rs b/substrate/frame/revive/src/tests/sol.rs index 96d3f91033b08..e32058779a858 100644 --- a/substrate/frame/revive/src/tests/sol.rs +++ b/substrate/frame/revive/src/tests/sol.rs @@ -19,21 +19,22 @@ use crate::{ assert_refcount, call_builder::VmBinaryModule, debug::DebugSettings, + evm::{PrestateTrace, PrestateTracer, PrestateTracerConfig}, test_utils::{builder::Contract, ALICE, ALICE_ADDR}, tests::{ builder, test_utils::{contract_base_deposit, ensure_stored, get_contract}, AllowEvmBytecode, DebugFlag, ExtBuilder, RuntimeOrigin, Test, }, + tracing::trace, Code, Config, Error, GenesisConfig, Pallet, PristineCode, }; use alloy_core::sol_types::{SolCall, SolInterface}; use frame_support::{assert_err, assert_ok, traits::fungible::Mutate}; -use pallet_revive_fixtures::{compile_module_with_type, Fibonacci, FixtureType}; +use pallet_revive_fixtures::{compile_module_with_type, Fibonacci, FixtureType, NestedCounter}; use pretty_assertions::assert_eq; -use test_case::test_case; - use revm::bytecode::opcode::*; +use test_case::test_case; mod arithmetic; mod bitwise; @@ -114,7 +115,9 @@ fn basic_evm_flow_tracing_works() { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = trace(&mut tracer, || { - builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract() + builder::bare_instantiate(Code::Upload(code.clone())) + .salt(None) + .build_and_unwrap_contract() }); let contract = get_contract(&addr); @@ -124,7 +127,7 @@ fn basic_evm_flow_tracing_works() { tracer.collect_trace().unwrap(), CallTrace { from: ALICE_ADDR, - call_type: CallType::Create2, + call_type: CallType::Create, to: addr, input: code.into(), output: runtime_code.into(), @@ -302,3 +305,195 @@ fn dust_work_with_child_calls(fixture_type: FixtureType) { assert_eq!(crate::Pallet::::evm_balance(&addr), value); }); } + +#[test] +fn prestate_diff_mode_tracing_works() { + use alloy_core::hex; + + struct TestCase { + config: PrestateTracerConfig, + expected_instantiate_trace_json: &'static str, + expected_call_trace_json: &'static str, + } + + let (counter_code, _) = compile_module_with_type("NestedCounter", FixtureType::Solc).unwrap(); + let (contract_runtime_code, _) = + compile_module_with_type("NestedCounter", FixtureType::SolcRuntime).unwrap(); + let (child_runtime_code, _) = + compile_module_with_type("Counter", FixtureType::SolcRuntime).unwrap(); + + let test_cases = [ + TestCase { + config: PrestateTracerConfig { + diff_mode: false, + disable_storage: false, + disable_code: false, + }, + expected_instantiate_trace_json: r#"{ + "{{ALICE_ADDR}}": { + "balance": "{{ALICE_BALANCE_PRE}}" + } + }"#, + expected_call_trace_json: r#"{ + "{{ALICE_ADDR}}": { + "balance": "{{ALICE_BALANCE_POST}}", + "nonce": 1 + }, + "{{CONTRACT_ADDR}}": { + "balance": "0x0", + "nonce": 2, + "code": "{{CONTRACT_CODE}}", + "storage": { + "0x0000000000000000000000000000000000000000000000000000000000000000": "{{CHILD_ADDR_PADDED}}", + "0x0000000000000000000000000000000000000000000000000000000000000001": "0x0000000000000000000000000000000000000000000000000000000000000007" + } + }, + "{{CHILD_ADDR}}": { + "balance": "0x0", + "nonce": 1, + "code": "{{CHILD_CODE}}", + "storage": { + "0x0000000000000000000000000000000000000000000000000000000000000000": "0x000000000000000000000000000000000000000000000000000000000000000a" + } + } + }"#, + }, + TestCase { + config: PrestateTracerConfig { + diff_mode: true, + disable_storage: false, + disable_code: false, + }, + expected_instantiate_trace_json: r#"{ + "pre": { + "{{ALICE_ADDR}}": { + "balance": "{{ALICE_BALANCE_PRE}}" + } + }, + "post": { + "{{ALICE_ADDR}}": { + "balance": "{{ALICE_BALANCE_POST}}", + "nonce": 1 + }, + "{{CONTRACT_ADDR}}": { + "balance": "0x0", + "nonce": 2, + "code": "{{CONTRACT_CODE}}" + }, + "{{CHILD_ADDR}}": { + "balance": "0x0", + "nonce": 1, + "code": "{{CHILD_CODE}}" + } + } + }"#, + expected_call_trace_json: r#"{ + "pre": { + "{{CONTRACT_ADDR}}": { + "balance": "0x0", + "nonce": 2, + "code": "{{CONTRACT_CODE}}", + "storage": { + "0x0000000000000000000000000000000000000000000000000000000000000001": "0x0000000000000000000000000000000000000000000000000000000000000007" + } + }, + "{{CHILD_ADDR}}": { + "balance": "0x0", + "nonce": 1, + "code": "{{CHILD_CODE}}", + "storage": { + "0x0000000000000000000000000000000000000000000000000000000000000000": "0x000000000000000000000000000000000000000000000000000000000000000a" + } + } + }, + "post": { + "{{CONTRACT_ADDR}}": { + "storage": { + "0x0000000000000000000000000000000000000000000000000000000000000001": "0x0000000000000000000000000000000000000000000000000000000000000008" + } + }, + "{{CHILD_ADDR}}": { + "storage": { + "0x0000000000000000000000000000000000000000000000000000000000000000": "0x0000000000000000000000000000000000000000000000000000000000000007" + } + } + } + }"#, + }, + ]; + + for test_case in test_cases { + ExtBuilder::default().build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 1_000_000_000_000); + + let contract_addr = crate::address::create1(&ALICE_ADDR, 0u64); + let child_addr = crate::address::create1(&contract_addr, 1u64); + + // Compute balances + let alice_balance_pre = Pallet::::convert_native_to_evm( + 1_000_000_000_000 - Pallet::::min_balance(), + ); + + let replace_placeholders = |json: &str| -> String { + let alice_balance_post = Pallet::::evm_balance(&ALICE_ADDR); + + let mut child_addr_bytes = [0u8; 32]; + child_addr_bytes[12..32].copy_from_slice(child_addr.as_bytes()); + + json.replace("{{ALICE_ADDR}}", &format!("{:#x}", ALICE_ADDR)) + .replace("{{CONTRACT_ADDR}}", &format!("{:#x}", contract_addr)) + .replace("{{CHILD_ADDR}}", &format!("{:#x}", child_addr)) + .replace("{{ALICE_BALANCE_PRE}}", &format!("{:#x}", alice_balance_pre)) + .replace("{{ALICE_BALANCE_POST}}", &format!("{:#x}", alice_balance_post)) + .replace( + "{{CONTRACT_CODE}}", + &format!("0x{}", hex::encode(&contract_runtime_code)), + ) + .replace("{{CHILD_CODE}}", &format!("0x{}", hex::encode(&child_runtime_code))) + .replace( + "{{CHILD_ADDR_PADDED}}", + &format!("0x{}", hex::encode(child_addr_bytes)), + ) + }; + + let mut tracer = PrestateTracer::::new(test_case.config.clone()); + let Contract { addr: contract_addr_actual, .. } = trace(&mut tracer, || { + builder::bare_instantiate(Code::Upload(counter_code.clone())) + .salt(None) + .build_and_unwrap_contract() + }); + assert_eq!(contract_addr, contract_addr_actual, "contract address mismatch"); + + let instantiate_trace = tracer.collect_trace(); + + let expected_json = replace_placeholders(test_case.expected_instantiate_trace_json); + let expected_trace: PrestateTrace = serde_json::from_str(&expected_json).unwrap(); + assert_eq!( + instantiate_trace, expected_trace, + "unexpected instantiate trace for {:?}", + test_case.config + ); + + let mut tracer = PrestateTracer::::new(test_case.config.clone()); + trace(&mut tracer, || { + builder::bare_call(contract_addr) + .data( + NestedCounter::NestedCounterCalls::nestedNumber( + NestedCounter::nestedNumberCall {}, + ) + .abi_encode(), + ) + .build_and_unwrap_result(); + }); + + let call_trace = tracer.collect_trace(); + let expected_json = replace_placeholders(test_case.expected_call_trace_json); + let expected_trace: PrestateTrace = serde_json::from_str(&expected_json).unwrap(); + assert_eq!( + call_trace, expected_trace, + "unexpected call trace for {:?}", + test_case.config + ); + }); + } +}