Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid creating extra journal entries #7493

Merged
merged 4 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions crates/evm/core/src/backend/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use revm::{
precompile::{PrecompileSpecId, Precompiles},
primitives::{
Account, AccountInfo, Bytecode, CreateScheme, Env, EnvWithHandlerCfg, HashMap as Map, Log,
ResultAndState, SpecId, StorageSlot, TransactTo, KECCAK_EMPTY,
ResultAndState, SpecId, State, StorageSlot, TransactTo, KECCAK_EMPTY,
},
Database, DatabaseCommit, Inspector, JournaledState,
};
Expand Down Expand Up @@ -1888,7 +1888,19 @@ fn commit_transaction<I: Inspector<Backend>>(
};
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<DB: Database>(state: &mut State, db: &mut DB) -> Result<(), DB::Error> {
for (addr, acc) in state.iter_mut() {
acc.info = db.basic(*addr)?.unwrap_or_default();
for (key, val) in acc.storage.iter_mut() {
val.present_value = db.storage(*addr, *key)?;
}
}

Ok(())
}

Expand All @@ -1898,19 +1910,12 @@ fn apply_state_changeset(
state: Map<revm::primitives::Address, Account>,
journaled_state: &mut JournaledState,
fork: &mut Fork,
) {
let changed_accounts = state.keys().copied().collect::<Vec<_>>();
) -> Result<(), DatabaseError> {
// 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)?;

Ok(())
}
35 changes: 21 additions & 14 deletions crates/evm/evm/src/inspectors/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@ 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::{
interpreter::{
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};
Expand Down Expand Up @@ -228,16 +231,6 @@ macro_rules! call_inspectors_adjust_depth {
};
}

/// Helper method which updates data in the state with the data from the database.
fn update_state<DB: DatabaseExt>(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<Log>,
Expand Down Expand Up @@ -500,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 {
Expand Down
2 changes: 2 additions & 0 deletions crates/forge/tests/it/repros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
22 changes: 22 additions & 0 deletions testdata/default/repros/Issue7481.t.sol
Original file line number Diff line number Diff line change
@@ -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");
}
}
Loading