diff --git a/etherlink/kernel_latest/revm/src/code_storage.rs b/etherlink/kernel_latest/revm/src/code_storage.rs index 7e4ca9cc18872caaf8f9bfe17512b5fc6c58c895..020a697ff4934bbd003c7ef1cd82d33a165af31b 100644 --- a/etherlink/kernel_latest/revm/src/code_storage.rs +++ b/etherlink/kernel_latest/revm/src/code_storage.rs @@ -101,15 +101,16 @@ impl CodeStorage { } } - #[cfg(test)] - pub fn decrement_code_usage(&self, host: &mut impl Runtime) -> Result { - let number_reference = self.get_ref_count(host)?; - let number_reference = number_reference.saturating_sub(1u64); - self.set_ref_count(host, number_reference)?; + fn decrement_code_usage(&self, host: &mut impl Runtime) -> Result { + let mut number_reference = self.get_ref_count(host)?; + if number_reference != 0 { + // Condition avoids an unnecessary write access + number_reference = number_reference.saturating_sub(1u64); + self.set_ref_count(host, number_reference)?; + } Ok(number_reference) } - #[cfg(test)] pub fn delete(host: &mut impl Runtime, code_hash: &B256) -> Result<(), Error> { let code = Self::new(code_hash)?; if code.exists(host)? { diff --git a/etherlink/kernel_latest/revm/src/database.rs b/etherlink/kernel_latest/revm/src/database.rs index 218d4a9f0b4178aabd9d7e1cee6cdf89e0556dd4..f3bd685608bb4bc821624aa628164cd095382561 100644 --- a/etherlink/kernel_latest/revm/src/database.rs +++ b/etherlink/kernel_latest/revm/src/database.rs @@ -17,7 +17,7 @@ use crate::{ use revm::{ primitives::{Address, HashMap, StorageKey, StorageValue, B256, U256}, - state::{Account, AccountInfo, AccountStatus, Bytecode, EvmStorageSlot}, + state::{Account, AccountInfo, Bytecode, EvmStorage, EvmStorageSlot}, Database, DatabaseCommit, }; use tezos_crypto_rs::hash::{ContractKt1Hash, HashTrait}; @@ -42,6 +42,11 @@ pub struct EtherlinkVMDB<'a, Host: Runtime> { withdrawals: Vec, } +enum AccountState { + Touched((AccountInfo, EvmStorage)), + SelfDestructed(B256), +} + // See: https://github.com/rust-lang/rust-clippy/issues/5787 #[allow(clippy::needless_lifetimes)] impl<'a, Host: Runtime> EtherlinkVMDB<'a, Host> { @@ -81,10 +86,6 @@ pub trait PrecompileDatabase: Database { } impl EtherlinkVMDB<'_, Host> { - pub fn abort(&mut self) { - *self.commit_status = false; - } - pub fn commit_status(&self) -> bool { *self.commit_status } @@ -106,6 +107,79 @@ impl EtherlinkVMDB<'_, Host> { .rollback_transaction(self.host) .map_err(|err| Error::Custom(err.to_string())) } + + fn abort(&mut self) { + *self.commit_status = false; + } + + fn update_account(&mut self, address: Address, account_state: AccountState) { + match self.get_or_create_account(address) { + Ok(mut storage_account) => match account_state { + AccountState::Touched((info, storage)) => { + if let Err(err) = storage_account.set_info(self.host, info) { + self.abort(); + log!( + self.host, + LogError, + "DatabaseCommit `set_info` error: {err:?}" + ); + } + + for ( + key, + EvmStorageSlot { + original_value, + present_value, + .. + }, + ) in storage + { + if original_value != present_value { + if let Err(err) = storage_account.set_storage( + self.host, + &key, + &present_value, + ) { + self.abort(); + log!( + self.host, + LogError, + "DatabaseCommit `set_storage` error: {err:?}" + ); + } + } + } + } + AccountState::SelfDestructed(code_hash) => { + if let Err(err) = storage_account.clear_info(self.host, &code_hash) { + self.abort(); + log!( + self.host, + LogError, + "DatabaseCommit `clear_info` error: {err:?}" + ); + } + + if let Err(err) = storage_account.clear_storage(self.host) { + self.abort(); + log!( + self.host, + LogError, + "DatabaseCommit `clear_storage` error: {err:?}" + ); + } + } + }, + Err(err) => { + self.abort(); + log!( + self.host, + LogError, + "DatabaseCommit `get_or_create_account` error: {err:?}" + ) + } + } + } } impl PrecompileDatabase for EtherlinkVMDB<'_, Host> { @@ -198,65 +272,28 @@ impl Database for EtherlinkVMDB<'_, Host> { impl DatabaseCommit for EtherlinkVMDB<'_, Host> { fn commit(&mut self, changes: HashMap) { - for ( - address, - Account { - info, - storage, - status, - .. - }, - ) in changes - { - // The account is marked as touched, the changes should be commited + for (address, account) in changes { + // The account isn't marked as touched, the changes are not commited // to the database. - if status.contains(AccountStatus::Touched) { - match self.get_or_create_account(address) { - Ok(mut storage_account) => { - if let Err(err) = storage_account.set_info(self.host, info) { - self.abort(); - log!( - self.host, - LogError, - "DatabaseCommit `set_info` error: {err:?}" - ); - } + if !account.is_touched() { + continue; + } - for ( - key, - EvmStorageSlot { - original_value, - present_value, - .. - }, - ) in storage - { - if original_value != present_value { - if let Err(err) = storage_account.set_storage( - self.host, - &key, - &present_value, - ) { - self.abort(); - log!( - self.host, - LogError, - "DatabaseCommit `set_storage` error: {err:?}" - ); - } - } - } - } - Err(err) => { - self.abort(); - log!( - self.host, - LogError, - "DatabaseCommit `get_or_create_account` error: {err:?}" - ) - } - } + // The account is touched and marked as selfdestructed, we clear the + // account. + if account.is_selfdestructed() { + self.update_account( + address, + AccountState::SelfDestructed(account.info.code_hash), + ); + continue; } + + // The account is touched, the changes are naturally commited to the database. + self.update_account( + address, + AccountState::Touched((account.info, account.storage)), + ); } } } diff --git a/etherlink/kernel_latest/revm/src/lib.rs b/etherlink/kernel_latest/revm/src/lib.rs index 355b42dab3060ec5655dcd05c05b800dd460e147..5402fab26c21710bc9915785af7f2f1acf51877e 100644 --- a/etherlink/kernel_latest/revm/src/lib.rs +++ b/etherlink/kernel_latest/revm/src/lib.rs @@ -7,6 +7,7 @@ use crate::{database::PrecompileDatabase, send_outbox_message::Withdrawal}; use database::EtherlinkVMDB; use precompile_provider::EtherlinkPrecompiles; use revm::context::result::EVMError; +use revm::context::tx::TxEnvBuilder; use revm::{ context::{ result::ExecutionResult, transaction::AccessList, BlockEnv, CfgEnv, ContextTr, @@ -113,25 +114,28 @@ fn tx_env<'a, Host: Runtime>( .map_err(|err| Error::Custom(err.to_string()))?; let nonce = storage_account.nonce(host)?; - Ok(TxEnv { - // Setting the transaction type from scratch seems irrelevant - // in the execution. We just set it to legacy be default as - // it's the other parameters that matter here. - tx_type: 0, - caller, - gas_limit, - gas_price, - kind, - value, - data, - nonce, - chain_id: Some(chain_id), - access_list, - gas_priority_fee: None, - blob_hashes: vec![], - max_fee_per_blob_gas: 0, - authorization_list: vec![], - }) + // Using the transaction environment builder helps to + // derive the transaction type directly from the different + // fields of the transaction. + let tx_env = TxEnvBuilder::new() + .caller(caller) + .gas_limit(gas_limit) + .gas_price(gas_price) + .kind(kind) + .value(value) + .data(data) + .nonce(nonce) + .chain_id(Some(chain_id)) + .access_list(access_list) + .build() + .map_err(|err| { + Error::Custom(format!( + "Building the transaction environment failed with: {:?}", + err + )) + })?; + + Ok(tx_env) } type EvmContext<'a, Host> = Evm< diff --git a/etherlink/kernel_latest/revm/src/world_state_handler.rs b/etherlink/kernel_latest/revm/src/world_state_handler.rs index eee4359bd1ca6fba781db695872e78704684b3ce..80b827e2cd4d326382bdf0aaee74b9c5bcb44ced 100644 --- a/etherlink/kernel_latest/revm/src/world_state_handler.rs +++ b/etherlink/kernel_latest/revm/src/world_state_handler.rs @@ -217,6 +217,37 @@ impl StorageAccount { Ok(()) } + fn delete_code( + &mut self, + host: &mut impl Runtime, + code_hash: &B256, + ) -> Result<(), Error> { + if code_hash != &KECCAK_EMPTY { + CodeStorage::delete(host, code_hash)?; + let code_hash_path = concat(&self.path, &CODE_HASH_PATH)?; + if host.store_has(&code_hash_path)?.is_some() { + host.store_delete(&code_hash_path)? + } + } + Ok(()) + } + + pub fn clear_info( + &mut self, + host: &mut impl Runtime, + code_hash: &B256, + ) -> Result<(), Error> { + // If nothing was ever stored state-wise, we have nothing + // to clear, it means the storage account was created and + // destructed within the same transaction. + if host.store_has(&self.path)?.is_some() { + self.set_balance(host, U256::ZERO)?; + self.set_nonce(host, 0)?; + self.delete_code(host, code_hash)?; + } + Ok(()) + } + pub fn storage_path(&self, index: &U256) -> Result { let storage_path = concat(&self.path, &STORAGE_ROOT_PATH)?; let index_path = path_from_u256(index)?; @@ -240,6 +271,14 @@ impl StorageAccount { Ok(host.store_write_all(&path, &value_bytes)?) } + pub fn clear_storage(&mut self, host: &mut impl Runtime) -> Result<(), Error> { + let path = concat(&self.path, &STORAGE_ROOT_PATH)?; + if host.store_has(&path)?.is_some() { + host.store_delete(&path)? + } + Ok(()) + } + fn read_ticket_balance( &self, host: &impl Runtime, diff --git a/etherlink/tezt/tests/evm_rollup.ml b/etherlink/tezt/tests/evm_rollup.ml index ea4861d11099ca086ef5cdf15b6c64eaf25a0f75..255ef22dac00e9392fded6f1de6e453e923d1554 100644 --- a/etherlink/tezt/tests/evm_rollup.ml +++ b/etherlink/tezt/tests/evm_rollup.ml @@ -603,8 +603,8 @@ let register_proxy ~title ~tags ?kernels ?additional_uses ?additional_config ?admin ?commitment_period ?challenge_window ?eth_bootstrap_accounts ?da_fee_per_byte ?minimum_base_fee_per_gas ?whitelist ?rollup_operator_key ?maximum_allowed_ticks ?maximum_gas_per_transaction ?restricted_rpcs - ?websockets ?enable_fast_withdrawal ?evm_version ?register_revm f protocols - = + ?websockets ?enable_fast_withdrawal ?evm_version ?(enable_revm = false) f + protocols = let register ~enable_dal ~enable_multichain ~enable_revm : unit = register_test ~title @@ -633,8 +633,8 @@ let register_proxy ~title ~tags ?kernels ?additional_uses ?additional_config ~enable_revm ~setup_mode:Setup_proxy in - if register_revm = Some true then - register ~enable_dal:false ~enable_multichain:false ~enable_revm:true ; + if enable_revm then + register ~enable_dal:false ~enable_multichain:false ~enable_revm ; register ~enable_dal:false ~enable_multichain:false ~enable_revm:false ; register ~enable_dal:true ~enable_multichain:false ~enable_revm:false ; register ~enable_dal:false ~enable_multichain:true ~enable_revm:false ; @@ -646,8 +646,8 @@ let register_sequencer ?(return_sequencer = false) ~title ~tags ?kernels ?minimum_base_fee_per_gas ?time_between_blocks ?whitelist ?rollup_operator_key ?maximum_allowed_ticks ?maximum_gas_per_transaction ?restricted_rpcs ?max_blueprints_ahead ?websockets ?evm_version - ?genesis_timestamp ?enable_tx_queue f protocols = - let register ~enable_dal ~enable_multichain : unit = + ?genesis_timestamp ?enable_tx_queue ?(enable_revm = false) f protocols = + let register ~enable_dal ~enable_multichain ~enable_revm : unit = register_test ~title ~tags @@ -672,6 +672,7 @@ let register_sequencer ?(return_sequencer = false) ~title ~tags ?kernels protocols ~enable_dal ~enable_multichain + ~enable_revm ~setup_mode: (Setup_sequencer { @@ -682,17 +683,19 @@ let register_sequencer ?(return_sequencer = false) ~title ~tags ?kernels genesis_timestamp; }) in - register ~enable_dal:false ~enable_multichain:false ; - register ~enable_dal:true ~enable_multichain:false ; - register ~enable_dal:false ~enable_multichain:true ; - register ~enable_dal:true ~enable_multichain:true + if enable_revm then + register ~enable_dal:false ~enable_multichain:false ~enable_revm ; + register ~enable_dal:false ~enable_multichain:false ~enable_revm:false ; + register ~enable_dal:true ~enable_multichain:false ~enable_revm:false ; + register ~enable_dal:false ~enable_multichain:true ~enable_revm:false ; + register ~enable_dal:true ~enable_multichain:true ~enable_revm:false let register_both ~title ~tags ?kernels ?additional_uses ?additional_config ?admin ?commitment_period ?challenge_window ?eth_bootstrap_accounts ?da_fee_per_byte ?minimum_base_fee_per_gas ?time_between_blocks ?whitelist ?rollup_operator_key ?maximum_allowed_ticks ?maximum_gas_per_transaction - ?restricted_rpcs ?max_blueprints_ahead ?websockets ?evm_version f protocols - : unit = + ?restricted_rpcs ?max_blueprints_ahead ?websockets ?evm_version + ?(enable_revm = false) f protocols : unit = register_proxy ~title ~tags @@ -712,6 +715,7 @@ let register_both ~title ~tags ?kernels ?additional_uses ?additional_config ?restricted_rpcs ?websockets ?evm_version + ~enable_revm f protocols ; register_sequencer @@ -735,6 +739,7 @@ let register_both ~title ~tags ?kernels ?additional_uses ?additional_config ?max_blueprints_ahead ?websockets ?evm_version + ~enable_revm f protocols @@ -2608,7 +2613,7 @@ let test_deposit_and_withdraw = ~admin ~commitment_period ~challenge_window - ~register_revm:true + ~enable_revm:true @@ fun ~protocol:_ ~evm_setup: { @@ -2692,7 +2697,7 @@ let test_withdraw_amount = ~tags:["evm"; "withdraw"; "wei"; "mutez"] ~title:"Minimum amount to withdraw" ~admin - ~register_revm:true + ~enable_revm:true @@ fun ~protocol:_ ~evm_setup:{endpoint; produce_block; _} -> let sender = Eth_account.bootstrap_accounts.(0) in (* Minimal amount of Wei fails with revert. *) @@ -2747,7 +2752,7 @@ let test_withdraw_via_calls = ~tags:["evm"; "withdraw"; "call"; "staticcall"; "delegatecall"; "callcode"] ~title:"Withdrawal via different kind of calls" ~admin - ~register_revm:true + ~enable_revm:true @@ fun ~protocol:_ ~evm_setup:({endpoint; produce_block; evm_version; _} as evm_setup) -> @@ -5327,6 +5332,7 @@ let test_l2_call_selfdetruct_contract_in_same_transaction_and_separate_transacti register_both ~tags:["evm"; "l2_call"; "selfdestruct"; "cancun"] ~title:"Check SELFDESTRUCT's behavior as stated by Cancun's EIP-6780" + ~enable_revm:true @@ fun ~protocol:_ ~evm_setup:({endpoint; produce_block; evm_version; _} as evm_setup) -> diff --git a/etherlink/tezt/tests/evm_sequencer.ml b/etherlink/tezt/tests/evm_sequencer.ml index 5ce0d0bf3d99d073ce29943740c85d201a905a44..2d445e1ffe4bf1629293c634cbec26a4bddb44de 100644 --- a/etherlink/tezt/tests/evm_sequencer.ml +++ b/etherlink/tezt/tests/evm_sequencer.ml @@ -13064,6 +13064,7 @@ let test_eip2930_storage_access = ~title:"Check EIP-2930's semantic correctness" ~da_fee:Wei.zero ~time_between_blocks:Nothing + ~use_revm:activate_revm_registration @@ fun {sequencer; evm_version; _} _protocol -> let whale = Eth_account.bootstrap_accounts.(0) in let* eip2930_storage_access =