fix(levm): handle storage collisions when creating contracts

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<Option<AccountInfo>, EvmError> {
+    fn get_account_state(&self, address: Address) -> Result<Option<AccountState>, 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()))
     }
 

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<Option<AccountInfo>, GuestProgramStateError> {
+    ) -> Result<Option<AccountState>, 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.

crates/l2/common/src/state_diff.rs

@@ -538,12 +538,12 @@ pub fn get_nonce_diff(
     account_update: &AccountUpdate,
     db: &impl VmDatabase,
 ) -> Result<u16, StateDiffError> {
-    // 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,
     };
 

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<AccountInfo, DatabaseError> {
+    fn get_account_state(&self, address: CoreAddress) -> Result<AccountState, DatabaseError> {
         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<AccountInfo, DatabaseError> {
-        let acc_info = <dyn VmDatabase>::get_account_info(self.as_ref(), address)
+    fn get_account_state(&self, address: CoreAddress) -> Result<AccountState, DatabaseError> {
+        let acc_state = <dyn VmDatabase>::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(

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<Option<AccountInfo>, EvmError>;
+    fn get_account_state(&self, address: Address) -> Result<Option<AccountState>, EvmError>;
     fn get_storage_slot(&self, address: Address, key: H256) -> Result<Option<U256>, EvmError>;
     fn get_block_hash(&self, block_number: u64) -> Result<H256, EvmError>;
     fn get_chain_config(&self) -> Result<ChainConfig, EvmError>;

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,43 +13,65 @@ 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<H256, U256>,
+    /// 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<AccountInfo> 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<GenesisAccount> for LevmAccount {
     fn from(genesis: GenesisAccount) -> Self {
-        let storage = genesis
+        let storage: BTreeMap<H256, U256> = genesis
             .storage
             .into_iter()
             .map(|(key, value)| (H256::from(key.to_big_endian()), value))
             .collect();
 
         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<AccountState> 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 {

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(())

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<dyn Database>,
         current_accounts_state: BTreeMap<Address, Account>,
@@ -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))
             }

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<AccountInfo, DatabaseError>;
+    fn get_account_state(&self, address: Address) -> Result<AccountState, DatabaseError>;
     fn get_storage_value(&self, address: Address, key: H256) -> Result<U256, DatabaseError>;
     fn get_block_hash(&self, block_number: u64) -> Result<H256, DatabaseError>;
     fn get_chain_config(&self) -> Result<ChainConfig, DatabaseError>;

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,

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()))?;

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,
         },

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<Option<AccountInfo>, EvmError> {
+    fn get_account_state(&self, address: Address) -> Result<Option<AccountState>, 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()))
     }
 

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",

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.

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}",
             )))