diff --git a/crates/blockchain/vm.rs b/crates/blockchain/vm.rs index 41787942199..a90fdd2ae49 100644 --- a/crates/blockchain/vm.rs +++ b/crates/blockchain/vm.rs @@ -2,7 +2,7 @@ use bytes::Bytes; use ethrex_common::{ Address, H256, U256, constants::EMPTY_KECCACK_HASH, - types::{AccountInfo, BlockHash, BlockNumber, ChainConfig}, + types::{AccountState, BlockHash, BlockNumber, ChainConfig}, }; use ethrex_storage::Store; use ethrex_vm::{EvmError, VmDatabase}; @@ -43,9 +43,9 @@ impl StoreVmDatabase { impl VmDatabase for StoreVmDatabase { #[instrument(level = "trace", name = "Account read", skip_all)] - fn get_account_info(&self, address: Address) -> Result, EvmError> { + fn get_account_state(&self, address: Address) -> Result, EvmError> { self.store - .get_account_info_by_hash(self.block_hash, address) + .get_account_state_by_hash(self.block_hash, address) .map_err(|e| EvmError::DB(e.to_string())) } diff --git a/crates/common/types/block_execution_witness.rs b/crates/common/types/block_execution_witness.rs index 4901be62207..500afa84336 100644 --- a/crates/common/types/block_execution_witness.rs +++ b/crates/common/types/block_execution_witness.rs @@ -6,7 +6,7 @@ use crate::types::Block; use crate::{ H160, constants::EMPTY_KECCACK_HASH, - types::{AccountInfo, AccountState, AccountUpdate, BlockHeader, ChainConfig}, + types::{AccountState, AccountUpdate, BlockHeader, ChainConfig}, utils::{decode_hex, keccak}, }; use bytes::Bytes; @@ -355,12 +355,12 @@ impl GuestProgramState { .ok_or(GuestProgramStateError::MissingParentHeaderOf(block_number)) } - /// Retrieves the account info based on what is stored in the state trie. + /// Retrieves the account state from the state trie. /// Returns an error if the state trie is not rebuilt or if decoding the account state fails. - pub fn get_account_info( + pub fn get_account_state( &mut self, address: Address, - ) -> Result, GuestProgramStateError> { + ) -> Result, GuestProgramStateError> { let state_trie = self .state_trie .as_ref() @@ -380,11 +380,7 @@ impl GuestProgramState { GuestProgramStateError::Database("Failed to get decode account from trie".to_string()) })?; - Ok(Some(AccountInfo { - balance: state.balance, - code_hash: state.code_hash, - nonce: state.nonce, - })) + Ok(Some(state)) } /// Fetches the block hash for a specific block number. diff --git a/crates/l2/common/src/state_diff.rs b/crates/l2/common/src/state_diff.rs index 7a50fa110a7..bd2dddc42a1 100644 --- a/crates/l2/common/src/state_diff.rs +++ b/crates/l2/common/src/state_diff.rs @@ -538,12 +538,12 @@ pub fn get_nonce_diff( account_update: &AccountUpdate, db: &impl VmDatabase, ) -> Result { - // Get previous account_info either from store or cache - let account_info = db.get_account_info(account_update.address)?; + // Get previous account_state either from store or cache + let account_state = db.get_account_state(account_update.address)?; // Get previous nonce - let prev_nonce = match account_info { - Some(info) => info.nonce, + let prev_nonce = match account_state { + Some(state) => state.nonce, None => 0, }; diff --git a/crates/vm/backends/levm/db.rs b/crates/vm/backends/levm/db.rs index 55050263891..8c835c6e6ef 100644 --- a/crates/vm/backends/levm/db.rs +++ b/crates/vm/backends/levm/db.rs @@ -1,6 +1,6 @@ use ethrex_common::U256 as CoreU256; use ethrex_common::constants::EMPTY_KECCACK_HASH; -use ethrex_common::types::AccountInfo; +use ethrex_common::types::AccountState; use ethrex_common::{Address as CoreAddress, H256 as CoreH256}; use ethrex_levm::db::Database as LevmDatabase; @@ -32,18 +32,18 @@ impl DatabaseLogger { } impl LevmDatabase for DatabaseLogger { - fn get_account_info(&self, address: CoreAddress) -> Result { + fn get_account_state(&self, address: CoreAddress) -> Result { self.state_accessed .lock() .map_err(|_| DatabaseError::Custom("Could not lock mutex".to_string()))? .entry(address) .or_default(); - let info = self + let state = self .store .lock() .map_err(|_| DatabaseError::Custom("Could not lock mutex".to_string()))? - .get_account_info(address)?; - Ok(info) + .get_account_state(address)?; + Ok(state) } fn get_storage_value( @@ -101,12 +101,12 @@ impl LevmDatabase for DatabaseLogger { } impl LevmDatabase for DynVmDatabase { - fn get_account_info(&self, address: CoreAddress) -> Result { - let acc_info = ::get_account_info(self.as_ref(), address) + fn get_account_state(&self, address: CoreAddress) -> Result { + let acc_state = ::get_account_state(self.as_ref(), address) .map_err(|e| DatabaseError::Custom(e.to_string()))? .unwrap_or_default(); - Ok(acc_info) + Ok(acc_state) } fn get_storage_value( diff --git a/crates/vm/db.rs b/crates/vm/db.rs index 0aa9d402356..1e9d51d2a3e 100644 --- a/crates/vm/db.rs +++ b/crates/vm/db.rs @@ -3,11 +3,11 @@ use bytes::Bytes; use dyn_clone::DynClone; use ethrex_common::{ Address, H256, U256, - types::{AccountInfo, ChainConfig}, + types::{AccountState, ChainConfig}, }; pub trait VmDatabase: Send + Sync + DynClone { - fn get_account_info(&self, address: Address) -> Result, EvmError>; + fn get_account_state(&self, address: Address) -> Result, EvmError>; fn get_storage_slot(&self, address: Address, key: H256) -> Result, EvmError>; fn get_block_hash(&self, block_number: u64) -> Result; fn get_chain_config(&self) -> Result; diff --git a/crates/vm/levm/src/account.rs b/crates/vm/levm/src/account.rs index 919a94f8711..4b75d94c83f 100644 --- a/crates/vm/levm/src/account.rs +++ b/crates/vm/levm/src/account.rs @@ -1,9 +1,8 @@ -use ethrex_common::{H256, utils::keccak}; -use ethrex_common::{ - U256, - constants::EMPTY_KECCACK_HASH, - types::{AccountInfo, GenesisAccount}, -}; +use ethrex_common::H256; +use ethrex_common::constants::EMPTY_TRIE_HASH; +use ethrex_common::types::{AccountState, GenesisAccount}; +use ethrex_common::utils::keccak; +use ethrex_common::{U256, constants::EMPTY_KECCACK_HASH, types::AccountInfo}; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; @@ -14,27 +13,34 @@ use std::collections::BTreeMap; /// - We'll fetch the code only if we need to, this means less accesses to the database. /// - If there's duplicate code between accounts (which is pretty common) we'll store it in memory only once. /// - We'll be able to make better decisions without relying on external structures, based on the current status of an Account. e.g. If it was untouched we skip processing it when calculating Account Updates, or if the account has been destroyed and re-created with same address we know that the storage on the Database is not valid and we shouldn't access it, etc. -#[derive(Clone, Default, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct LevmAccount { pub info: AccountInfo, pub storage: BTreeMap, + /// If true it means that attempting to create an account with this address it would at least collide because of storage. + /// We just care about this kind of collision if the account doesn't have code or nonce. Otherwise its value doesn't matter. + /// For more information see EIP-7610: https://eips.ethereum.org/EIPS/eip-7610 + /// Warning: This attribute should only be used for handling create collisions as it's not necessary appropriate for every scenario. Read the caveat below. + /// + /// How this works: + /// - When getting an account from the DB this is set to true if the account has non-empty storage root. + /// - Upon destruction of an account this is set to false because storage is emptied for sure. + /// + /// **Important Caveat** + /// This only works for accounts of these characteristics that have been created in the past, we consider that accounts with storage + /// but no nonce or code cannot be created anymore, otherwise the fix would need to be more complex because we should keep track of the + /// storage root of an account during execution instead of just keeping track of it when fetching it from the Database or updating it when + /// destroying it. The EIP that adds to the spec this check did it because there are 28 accounts with these characteristics already deployed + /// in mainnet (back when they were deployed with nonce 0), but they cannot be created intentionally anymore. + pub has_storage: bool, /// Current status of the account. pub status: AccountStatus, } -impl From for LevmAccount { - fn from(info: AccountInfo) -> Self { - LevmAccount { - info, - storage: BTreeMap::new(), - status: AccountStatus::Unmodified, - } - } -} - +// This is used only in state_v2 runner, storage is already fully filled in the genesis account. impl From for LevmAccount { fn from(genesis: GenesisAccount) -> Self { - let storage = genesis + let storage: BTreeMap = genesis .storage .into_iter() .map(|(key, value)| (H256::from(key.to_big_endian()), value)) @@ -42,15 +48,30 @@ impl From for LevmAccount { LevmAccount { info: AccountInfo { + code_hash: keccak(genesis.code), balance: genesis.balance, nonce: genesis.nonce, - code_hash: keccak(&genesis.code), }, + has_storage: !storage.is_empty(), storage, status: AccountStatus::Unmodified, } } } +impl From for LevmAccount { + fn from(state: AccountState) -> Self { + LevmAccount { + info: AccountInfo { + code_hash: state.code_hash, + balance: state.balance, + nonce: state.nonce, + }, + storage: BTreeMap::new(), + status: AccountStatus::Unmodified, + has_storage: state.storage_root != *EMPTY_TRIE_HASH, + } + } +} impl LevmAccount { pub fn has_nonce(&self) -> bool { @@ -61,8 +82,8 @@ impl LevmAccount { self.info.code_hash != *EMPTY_KECCACK_HASH } - pub fn has_code_or_nonce(&self) -> bool { - self.has_code() || self.has_nonce() + pub fn create_would_collide(&self) -> bool { + self.has_code() || self.has_nonce() || self.has_storage } pub fn is_empty(&self) -> bool { diff --git a/crates/vm/levm/src/call_frame.rs b/crates/vm/levm/src/call_frame.rs index 87a41a71dc2..65db7c119cf 100644 --- a/crates/vm/levm/src/call_frame.rs +++ b/crates/vm/levm/src/call_frame.rs @@ -291,6 +291,7 @@ impl CallFrameBackup { info: account.info.clone(), storage: BTreeMap::new(), status: account.status.clone(), + has_storage: account.has_storage, }); Ok(()) diff --git a/crates/vm/levm/src/db/gen_db.rs b/crates/vm/levm/src/db/gen_db.rs index 3afc4671896..720c978f53f 100644 --- a/crates/vm/levm/src/db/gen_db.rs +++ b/crates/vm/levm/src/db/gen_db.rs @@ -47,6 +47,7 @@ impl GeneralizedDatabase { } } + /// Only used within Levm Runner, where the accounts already have all the storage pre-loaded, not used in real case scenarios. pub fn new_with_account_state( store: Arc, current_accounts_state: BTreeMap, @@ -77,8 +78,8 @@ impl GeneralizedDatabase { match self.current_accounts_state.entry(address) { Entry::Occupied(entry) => Ok(entry.into_mut()), Entry::Vacant(entry) => { - let info = self.store.get_account_info(address)?; - let account = LevmAccount::from(info); + let state = self.store.get_account_state(address)?; + let account = LevmAccount::from(state); self.initial_accounts_state.insert(address, account.clone()); Ok(entry.insert(account)) } diff --git a/crates/vm/levm/src/db/mod.rs b/crates/vm/levm/src/db/mod.rs index d598fb9955f..cd0bcca3295 100644 --- a/crates/vm/levm/src/db/mod.rs +++ b/crates/vm/levm/src/db/mod.rs @@ -2,13 +2,13 @@ use crate::errors::DatabaseError; use bytes::Bytes; use ethrex_common::{ Address, H256, U256, - types::{AccountInfo, ChainConfig}, + types::{AccountState, ChainConfig}, }; pub mod gen_db; pub trait Database: Send + Sync { - fn get_account_info(&self, address: Address) -> Result; + fn get_account_state(&self, address: Address) -> Result; fn get_storage_value(&self, address: Address, key: H256) -> Result; fn get_block_hash(&self, block_number: u64) -> Result; fn get_chain_config(&self) -> Result; diff --git a/crates/vm/levm/src/execution_handlers.rs b/crates/vm/levm/src/execution_handlers.rs index 86864d3679c..9bd514961c9 100644 --- a/crates/vm/levm/src/execution_handlers.rs +++ b/crates/vm/levm/src/execution_handlers.rs @@ -110,7 +110,7 @@ impl<'a> VM<'a> { let new_contract_address = self.current_call_frame.to; let new_account = self.get_account_mut(new_contract_address)?; - if new_account.has_code_or_nonce() { + if new_account.create_would_collide() { return Ok(Some(ContextResult { result: TxResult::Revert(ExceptionalHalt::AddressAlreadyOccupied.into()), gas_used: self.env.gas_limit, diff --git a/crates/vm/levm/src/opcode_handlers/system.rs b/crates/vm/levm/src/opcode_handlers/system.rs index 67a338a967b..2ae0fa57f58 100644 --- a/crates/vm/levm/src/opcode_handlers/system.rs +++ b/crates/vm/levm/src/opcode_handlers/system.rs @@ -665,7 +665,7 @@ impl<'a> VM<'a> { // Deployment will fail (consuming all gas) if the contract already exists. let new_account = self.get_account_mut(new_address)?; - if new_account.has_code_or_nonce() { + if new_account.create_would_collide() { self.current_call_frame.stack.push1(FAIL)?; self.tracer .exit_early(gas_limit, Some("CreateAccExists".to_string()))?; diff --git a/crates/vm/levm/src/utils.rs b/crates/vm/levm/src/utils.rs index eb602b3d39a..693ee9fae7d 100644 --- a/crates/vm/levm/src/utils.rs +++ b/crates/vm/levm/src/utils.rs @@ -581,10 +581,12 @@ impl<'a> VM<'a> { } /// Converts Account to LevmAccount +/// The problem with this is that we don't have the storage root. pub fn account_to_levm_account(account: Account) -> (LevmAccount, Bytes) { ( LevmAccount { info: account.info, + has_storage: !account.storage.is_empty(), // This is used in scenarios in which the storage is already all in the account. For the Levm Runner storage: account.storage, status: AccountStatus::Unmodified, }, diff --git a/crates/vm/witness_db.rs b/crates/vm/witness_db.rs index c32dfa57db3..28457cb5255 100644 --- a/crates/vm/witness_db.rs +++ b/crates/vm/witness_db.rs @@ -3,7 +3,7 @@ use bytes::Bytes; use ethrex_common::{ Address, H256, U256, types::{ - AccountInfo, AccountUpdate, Block, BlockHeader, ChainConfig, + AccountState, AccountUpdate, Block, BlockHeader, ChainConfig, block_execution_witness::{GuestProgramState, GuestProgramStateError}, }, }; @@ -67,10 +67,10 @@ impl VmDatabase for GuestProgramStateWrapper { .map_err(|_| EvmError::DB("Failed to get account code".to_string())) } - fn get_account_info(&self, address: Address) -> Result, EvmError> { + fn get_account_state(&self, address: Address) -> Result, EvmError> { self.lock_mutex() .map_err(|_| EvmError::DB("Failed to lock db".to_string()))? - .get_account_info(address) + .get_account_state(address) .map_err(|_| EvmError::DB("Failed to get account info".to_string())) } diff --git a/tooling/ef_tests/blockchain/tests/all.rs b/tooling/ef_tests/blockchain/tests/all.rs index f135a264a2a..0e55dfeeab5 100644 --- a/tooling/ef_tests/blockchain/tests/all.rs +++ b/tooling/ef_tests/blockchain/tests/all.rs @@ -10,14 +10,6 @@ const TEST_FOLDER: &str = "vectors/"; // Base skips shared by all runs. const SKIPPED_BASE: &[&str] = &[ - // These tests contain accounts without nonce or code but have storage, which is a virtually impossible scenario. That's why we fail, but that's okay. - // When creating an account we don't check the storage root but just if it has nonce or code. - // Fix is on its way on https://github.com/lambdaclass/ethrex/pull/4813 - "InitCollisionParis", - "RevertInCreateInInitCreate2Paris", - "create2collisionStorageParis", - "dynamicAccountOverwriteEmpty_Paris", - "RevertInCreateInInit_Paris", // Skip because they take too long to run, but they pass "static_Call50000_sha256", "CALLBlake2f_MaxRounds", diff --git a/tooling/ef_tests/state/parser.rs b/tooling/ef_tests/state/parser.rs index 40328e3133d..ca356115c41 100644 --- a/tooling/ef_tests/state/parser.rs +++ b/tooling/ef_tests/state/parser.rs @@ -21,17 +21,12 @@ pub enum EFTestParseError { } const IGNORED_TESTS: &[&str] = &[ - "static_Call50000_sha256.json", // Skip because it takes longer to run than some tests, but not a huge deal. - "CALLBlake2f_MaxRounds.json", // Skip because it takes extremely long to run, but passes. - "ValueOverflow.json", // Skip because it tries to deserialize number > U256::MAX - "ValueOverflowParis.json", // Skip because it tries to deserialize number > U256::MAX - "loopMul.json", // Skip because it takes too long to run - "dynamicAccountOverwriteEmpty_Paris.json", // Skipped because the scenario described is extremely unlikely, since it implies doing EXTCODEHASH on an empty account that is then created - "RevertInCreateInInitCreate2Paris.json", // Skipped because it's not worth implementing since the scenario of the test is virtually impossible. See https://github.com/lambdaclass/ethrex/issues/1555 - "RevertInCreateInInit_Paris.json", // Skipped because it's not worth implementing since the scenario of the test is virtually impossible. See https://github.com/lambdaclass/ethrex/issues/1555 - "create2collisionStorageParis.json", // Skipped because it's not worth implementing since the scenario of the test is virtually impossible. See https://github.com/lambdaclass/ethrex/issues/1555 - "InitCollisionParis.json", // Skip because it fails on REVM - "InitCollision.json", // Skip because it fails on REVM + "ValueOverflow.json", // Skip because it tries to deserialize number > U256::MAX + "ValueOverflowParis.json", // Skip because it tries to deserialize number > U256::MAX + // Skip because they take too long to run: + "static_Call50000_sha256.json", + "CALLBlake2f_MaxRounds.json", + "loopMul.json", ]; // One .json can have multiple tests, sometimes we want to skip one of those. diff --git a/tooling/ef_tests/state/runner/levm_runner.rs b/tooling/ef_tests/state/runner/levm_runner.rs index 9e3f762751c..24fc308709d 100644 --- a/tooling/ef_tests/state/runner/levm_runner.rs +++ b/tooling/ef_tests/state/runner/levm_runner.rs @@ -224,7 +224,7 @@ pub fn prepare_vm_for_tx<'a>( pub fn ensure_pre_state(evm: &VM, test: &EFTest) -> Result<(), EFTestRunnerError> { let world_state = &evm.db.store; for (address, pre_value) in &test.pre.0 { - let account_info = world_state.get_account_info(*address).map_err(|e| { + let account_info = world_state.get_account_state(*address).map_err(|e| { EFTestRunnerError::Internal(InternalError::Custom(format!( "Failed to read account {address:#x} from world state: {e}", ))) diff --git a/tooling/ef_tests/state/runner/revm_db.rs b/tooling/ef_tests/state/runner/revm_db.rs index b1ae385b369..b8d4a8e7b83 100644 --- a/tooling/ef_tests/state/runner/revm_db.rs +++ b/tooling/ef_tests/state/runner/revm_db.rs @@ -2,9 +2,9 @@ use ethrex_common::types::{AccountInfo, AccountUpdate, ChainConfig}; use ethrex_common::{Address as CoreAddress, BigEndianHash, H256, U256}; use ethrex_vm::{DynVmDatabase, EvmError, VmDatabase}; use revm::context::DBErrorMarker; -use revm::database::states::{AccountStatus, bundle_state::BundleRetention}; +use revm::database::states::{bundle_state::BundleRetention, AccountStatus}; use revm::primitives::{ - Address as RevmAddress, B256 as RevmB256, Bytes as RevmBytes, U256 as RevmU256, + Address as RevmAddress, Bytes as RevmBytes, B256 as RevmB256, U256 as RevmU256, }; use revm::state::{AccountInfo as RevmAccountInfo, Bytecode as RevmBytecode}; @@ -54,7 +54,7 @@ impl revm::Database for RevmDynVmDatabase { type Error = RevmError; fn basic(&mut self, address: RevmAddress) -> Result, Self::Error> { - let acc_info = match ::get_account_info( + let acc_info = match ::get_account_state( self.0.as_ref(), CoreAddress::from(address.0.as_ref()), )? { @@ -97,7 +97,7 @@ impl revm::DatabaseRef for RevmDynVmDatabase { type Error = RevmError; fn basic_ref(&self, address: RevmAddress) -> Result, Self::Error> { - let acc_info = match ::get_account_info( + let acc_info = match ::get_account_state( self.0.as_ref(), CoreAddress::from(address.0.as_ref()), )? {