From fe3ef5ef7a12f05b26ec3f530ed88e89648e3274 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 26 Mar 2024 03:49:24 +0400 Subject: [PATCH 1/4] fix: avoid creating extra journal entries --- crates/evm/core/src/backend/mod.rs | 26 +++++++++++++------------- crates/evm/evm/src/inspectors/stack.rs | 14 ++------------ 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/crates/evm/core/src/backend/mod.rs b/crates/evm/core/src/backend/mod.rs index f55259f4d1e0..3e44f3ccb3af 100644 --- a/crates/evm/core/src/backend/mod.rs +++ b/crates/evm/core/src/backend/mod.rs @@ -16,8 +16,7 @@ use revm::{ inspectors::NoOpInspector, precompile::{PrecompileSpecId, Precompiles}, primitives::{ - Account, AccountInfo, Bytecode, CreateScheme, Env, EnvWithHandlerCfg, HashMap as Map, Log, - ResultAndState, SpecId, StorageSlot, TransactTo, KECCAK_EMPTY, + Account, AccountInfo, Bytecode, CreateScheme, Env, EnvWithHandlerCfg, HashMap as Map, Log, ResultAndState, SpecId, State, StorageSlot, TransactTo, KECCAK_EMPTY }, Database, DatabaseCommit, Inspector, JournaledState, }; @@ -1892,6 +1891,16 @@ fn commit_transaction>( Ok(()) } +/// Helper method which updates data in the state with the data from the database. +pub fn update_state(state: &mut State, db: &mut DB) where DB::Error: core::fmt::Debug { + for (addr, acc) in state.iter_mut() { + acc.info = db.basic(*addr).unwrap().unwrap_or_default(); + for (key, val) in acc.storage.iter_mut() { + val.present_value = db.storage(*addr, *key).unwrap(); + } + } +} + /// Applies the changeset of a transaction to the active journaled state and also commits it in the /// forked db fn apply_state_changeset( @@ -1899,18 +1908,9 @@ fn apply_state_changeset( journaled_state: &mut JournaledState, fork: &mut Fork, ) { - let changed_accounts = state.keys().copied().collect::>(); // commit the state and update the loaded accounts fork.db.commit(state); - for addr in changed_accounts { - // reload all changed accounts by removing them from the journaled state and reloading them - // from the now updated database - if journaled_state.state.remove(&addr).is_some() { - let _ = journaled_state.load_account(addr, &mut fork.db); - } - if fork.journaled_state.state.remove(&addr).is_some() { - let _ = fork.journaled_state.load_account(addr, &mut fork.db); - } - } + update_state(&mut journaled_state.state, &mut fork.db); + update_state(&mut fork.journaled_state.state, &mut fork.db); } diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index ea0af6bbc873..002323690e7d 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -3,7 +3,7 @@ use super::{ StackSnapshotType, TracingInspector, TracingInspectorConfig, }; use alloy_primitives::{Address, Bytes, Log, U256}; -use foundry_evm_core::{backend::DatabaseExt, debug::DebugArena}; +use foundry_evm_core::{backend::{update_state, DatabaseExt}, debug::DebugArena}; use foundry_evm_coverage::HitMaps; use foundry_evm_traces::CallTraceArena; use revm::{ @@ -11,7 +11,7 @@ use revm::{ CallInputs, CallOutcome, CallScheme, CreateInputs, CreateOutcome, Gas, InstructionResult, Interpreter, InterpreterResult, }, - primitives::{BlockEnv, Env, EnvWithHandlerCfg, ExecutionResult, Output, State, TransactTo}, + primitives::{BlockEnv, Env, EnvWithHandlerCfg, ExecutionResult, Output, TransactTo}, DatabaseCommit, EvmContext, Inspector, }; use std::{collections::HashMap, sync::Arc}; @@ -228,16 +228,6 @@ macro_rules! call_inspectors_adjust_depth { }; } -/// Helper method which updates data in the state with the data from the database. -fn update_state(state: &mut State, db: &mut DB) { - for (addr, acc) in state.iter_mut() { - acc.info = db.basic(*addr).unwrap().unwrap_or_default(); - for (key, val) in acc.storage.iter_mut() { - val.present_value = db.storage(*addr, *key).unwrap(); - } - } -} - /// The collected results of [`InspectorStack`]. pub struct InspectorData { pub logs: Vec, From f10bdb0392577659f26eea482d96850cf3f78a8c Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 26 Mar 2024 04:17:54 +0400 Subject: [PATCH 2/4] add test --- crates/forge/tests/it/repros.rs | 2 ++ testdata/default/repros/Issue7481.t.sol | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 testdata/default/repros/Issue7481.t.sol diff --git a/crates/forge/tests/it/repros.rs b/crates/forge/tests/it/repros.rs index 38cf76331619..63592108ac6c 100644 --- a/crates/forge/tests/it/repros.rs +++ b/crates/forge/tests/it/repros.rs @@ -328,3 +328,5 @@ test_repro!(6634; |config| { cheats_config.always_use_create_2_factory = true; config.runner.cheats_config = std::sync::Arc::new(cheats_config); }); + +test_repro!(7481); \ No newline at end of file diff --git a/testdata/default/repros/Issue7481.t.sol b/testdata/default/repros/Issue7481.t.sol new file mode 100644 index 000000000000..d9fbc91511f1 --- /dev/null +++ b/testdata/default/repros/Issue7481.t.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity 0.8.18; + +import "ds-test/test.sol"; +import "cheats/Vm.sol"; + +// https://github.com/foundry-rs/foundry/issues/7481 +// This test ensures that we don't panic +contract Issue7481Test is DSTest { + Vm constant vm = Vm(HEVM_ADDRESS); + + function testFailTransact() public { + vm.createSelectFork("rpcAlias", 19514903); + + // Transfer some funds to sender of tx being transacted to ensure that it appears in journaled state + payable(address(0x5C60cD7a3D50877Bfebd484750FBeb245D936dAD)).call{value: 1}(""); + vm.transact(0xccfd66fc409a633a99b5b75b0e9a2040fcf562d03d9bee3fefc1a5c0eb49c999); + + // Revert the current call to ensure that revm can revert state journal + revert("HERE"); + } +} From 98b24298de3c1307816b8ec718bc0e938ee28d0d Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 26 Mar 2024 04:23:13 +0400 Subject: [PATCH 3/4] fmt --- crates/evm/core/src/backend/mod.rs | 8 ++++++-- crates/evm/evm/src/inspectors/stack.rs | 5 ++++- crates/forge/tests/it/repros.rs | 2 +- testdata/default/repros/Issue7481.t.sol | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/evm/core/src/backend/mod.rs b/crates/evm/core/src/backend/mod.rs index 3e44f3ccb3af..8a2db2c5fcc0 100644 --- a/crates/evm/core/src/backend/mod.rs +++ b/crates/evm/core/src/backend/mod.rs @@ -16,7 +16,8 @@ use revm::{ inspectors::NoOpInspector, precompile::{PrecompileSpecId, Precompiles}, primitives::{ - Account, AccountInfo, Bytecode, CreateScheme, Env, EnvWithHandlerCfg, HashMap as Map, Log, ResultAndState, SpecId, State, StorageSlot, TransactTo, KECCAK_EMPTY + Account, AccountInfo, Bytecode, CreateScheme, Env, EnvWithHandlerCfg, HashMap as Map, Log, + ResultAndState, SpecId, State, StorageSlot, TransactTo, KECCAK_EMPTY, }, Database, DatabaseCommit, Inspector, JournaledState, }; @@ -1892,7 +1893,10 @@ fn commit_transaction>( } /// Helper method which updates data in the state with the data from the database. -pub fn update_state(state: &mut State, db: &mut DB) where DB::Error: core::fmt::Debug { +pub fn update_state(state: &mut State, db: &mut DB) +where + DB::Error: core::fmt::Debug, +{ for (addr, acc) in state.iter_mut() { acc.info = db.basic(*addr).unwrap().unwrap_or_default(); for (key, val) in acc.storage.iter_mut() { diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 002323690e7d..36bdf24e808a 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -3,7 +3,10 @@ use super::{ StackSnapshotType, TracingInspector, TracingInspectorConfig, }; use alloy_primitives::{Address, Bytes, Log, U256}; -use foundry_evm_core::{backend::{update_state, DatabaseExt}, debug::DebugArena}; +use foundry_evm_core::{ + backend::{update_state, DatabaseExt}, + debug::DebugArena, +}; use foundry_evm_coverage::HitMaps; use foundry_evm_traces::CallTraceArena; use revm::{ diff --git a/crates/forge/tests/it/repros.rs b/crates/forge/tests/it/repros.rs index 63592108ac6c..02f1aa15eea2 100644 --- a/crates/forge/tests/it/repros.rs +++ b/crates/forge/tests/it/repros.rs @@ -329,4 +329,4 @@ test_repro!(6634; |config| { config.runner.cheats_config = std::sync::Arc::new(cheats_config); }); -test_repro!(7481); \ No newline at end of file +test_repro!(7481); diff --git a/testdata/default/repros/Issue7481.t.sol b/testdata/default/repros/Issue7481.t.sol index d9fbc91511f1..eb568dc94666 100644 --- a/testdata/default/repros/Issue7481.t.sol +++ b/testdata/default/repros/Issue7481.t.sol @@ -8,7 +8,7 @@ import "cheats/Vm.sol"; // This test ensures that we don't panic contract Issue7481Test is DSTest { Vm constant vm = Vm(HEVM_ADDRESS); - + function testFailTransact() public { vm.createSelectFork("rpcAlias", 19514903); From f573cda1590603a9342c78f56418bdfb25d34653 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 26 Mar 2024 16:36:12 +0400 Subject: [PATCH 4/4] graceful error handling --- crates/evm/core/src/backend/mod.rs | 21 +++++++++++---------- crates/evm/evm/src/inspectors/stack.rs | 18 ++++++++++++++++-- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/crates/evm/core/src/backend/mod.rs b/crates/evm/core/src/backend/mod.rs index 8a2db2c5fcc0..ca6cbb1ef8bd 100644 --- a/crates/evm/core/src/backend/mod.rs +++ b/crates/evm/core/src/backend/mod.rs @@ -1888,21 +1888,20 @@ fn commit_transaction>( }; trace!(elapsed = ?now.elapsed(), "transacted transaction"); - apply_state_changeset(res.state, journaled_state, fork); + apply_state_changeset(res.state, journaled_state, fork)?; Ok(()) } /// Helper method which updates data in the state with the data from the database. -pub fn update_state(state: &mut State, db: &mut DB) -where - DB::Error: core::fmt::Debug, -{ +pub fn update_state(state: &mut State, db: &mut DB) -> Result<(), DB::Error> { for (addr, acc) in state.iter_mut() { - acc.info = db.basic(*addr).unwrap().unwrap_or_default(); + acc.info = db.basic(*addr)?.unwrap_or_default(); for (key, val) in acc.storage.iter_mut() { - val.present_value = db.storage(*addr, *key).unwrap(); + val.present_value = db.storage(*addr, *key)?; } } + + Ok(()) } /// Applies the changeset of a transaction to the active journaled state and also commits it in the @@ -1911,10 +1910,12 @@ fn apply_state_changeset( state: Map, journaled_state: &mut JournaledState, fork: &mut Fork, -) { +) -> Result<(), DatabaseError> { // commit the state and update the loaded accounts fork.db.commit(state); - update_state(&mut journaled_state.state, &mut fork.db); - update_state(&mut fork.journaled_state.state, &mut fork.db); + update_state(&mut journaled_state.state, &mut fork.db)?; + update_state(&mut fork.journaled_state.state, &mut fork.db)?; + + Ok(()) } diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 36bdf24e808a..6f83bf43f867 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -493,8 +493,22 @@ impl InspectorStack { ecx.db.commit(res.state.clone()); // Update both states with new DB data after commit. - update_state(&mut ecx.journaled_state.state, &mut ecx.db); - update_state(&mut res.state, &mut ecx.db); + if let Err(e) = update_state(&mut ecx.journaled_state.state, &mut ecx.db) { + let res = InterpreterResult { + result: InstructionResult::Revert, + output: Bytes::from(e.to_string()), + gas, + }; + return (res, None) + } + if let Err(e) = update_state(&mut res.state, &mut ecx.db) { + let res = InterpreterResult { + result: InstructionResult::Revert, + output: Bytes::from(e.to_string()), + gas, + }; + return (res, None) + } // Merge transaction journal into the active journal. for (addr, acc) in res.state {