From 53c082957c26bc0c20b135b0d89150d730ca75d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jerem=C3=ADas=20Salom=C3=B3n?= <48994069+JereSalo@users.noreply.github.com> Date: Fri, 20 Dec 2024 22:05:06 -0300 Subject: [PATCH] fix(levm): fix interaction with cache and db (#1531) **Motivation** **Description** TLDR: We don't want to interact directly with the db without consulting before with the cache. This won't fix nor break tests because in every EFTest we send the cache empty, but it is a necessary fix for executing multiple transactions within a block. As a side effect we have to move contract creation and insertion to cache to `transact()`, so that we can revert if address is already occupied. Idea: - Move contract creation and insertion to cache to `transact()`, not to `new()`. - This is because we really want to check if the contract already exists both in the cache and in the db, and if when you want to create it you see that it already exists in one of those 2 there you revert. Currently we only check it in the db but the tests work because they test only one transaction each, so the cache always starts empty. - The idea is to stop using `db.get_account_info()` in the `VM::new()`, we want to query the cache first and then the DB. For this we have the `get_account()` method in VM, but in `VM::new()` it doesn't work because there is no vm created yet. The idea is to grab that behavior and take it to a `get_account(address, mut cache, db)` function that has the same behavior as now. The VM get_account method can be a handrail to call this, which does `get_account(address, self.cache, self.db)`. Additional Changes: - Remove unnecessary stuff in `VM::new()` - Remove receiver account every time a transaction reverts, independently of it's type. Closes #issue_number --- crates/vm/levm/src/vm.rs | 104 ++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 62 deletions(-) diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index a40780931..846d98a05 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -158,20 +158,14 @@ impl VM { TxKind::Call(address_to) => { default_touched_accounts.insert(address_to); - // add address_to to cache - let recipient_account_info = db.get_account_info(address_to); - cache::insert_account( - &mut cache, - address_to, - Account::from(recipient_account_info.clone()), - ); + let bytecode = get_account(&mut cache, &db, address_to).info.bytecode; // CALL tx let initial_call_frame = CallFrame::new( env.origin, address_to, address_to, - recipient_account_info.bytecode, + bytecode, value, calldata.clone(), false, @@ -201,26 +195,14 @@ impl VM { TxKind::Create => { // CREATE tx - let new_contract_address = - VM::calculate_create_address(env.origin, db.get_account_info(env.origin).nonce) - .map_err(|_| { - VMError::Internal(InternalError::CouldNotComputeCreateAddress) - })?; + let sender_nonce = get_account(&mut cache, &db, env.origin).info.nonce; + let new_contract_address = VM::calculate_create_address(env.origin, sender_nonce) + .map_err(|_| { + VMError::Internal(InternalError::CouldNotComputeCreateAddress) + })?; default_touched_accounts.insert(new_contract_address); - // Since we are in a CREATE transaction, we need to check if the address is already occupied. - // If it is, we should not continue with the transaction. We will handle the revert in the next step. - let new_account = db.get_account_info(new_contract_address); - let balance = value - .checked_add(new_account.balance) - .ok_or(VMError::BalanceOverflow)?; - - if !new_account.has_code() && !new_account.has_nonce() { - let created_contract = Account::new(balance, Bytes::new(), 1, HashMap::new()); - cache::insert_account(&mut cache, new_contract_address, created_contract); - } - let initial_call_frame = CallFrame::new( env.origin, new_contract_address, @@ -871,10 +853,8 @@ impl VM { // 1. Undo value transfer if the transaction was reverted if let TxResult::Revert(_) = report.result { - // msg_value was not increased in the receiver account when is a create transaction. - if !self.is_create() { - self.decrease_account_balance(receiver_address, initial_call_frame.msg_value)?; - } + // We remove the receiver account from the cache, like nothing changed in it's state. + remove_account(&mut self.cache, &receiver_address); self.increase_account_balance(sender_address, initial_call_frame.msg_value)?; } @@ -939,21 +919,28 @@ impl VM { self.prepare_execution(&mut initial_call_frame)?; - // The transaction should be reverted if: - // - The transaction is a CREATE transaction and - // - The address is already in the database and - // - The address is not empty + // In CREATE type transactions: + // Add created contract to cache, reverting transaction if the address is already occupied if self.is_create() { - let new_address_acc = self.db.get_account_info(initial_call_frame.to); - if new_address_acc.has_code() || new_address_acc.has_nonce() { + let new_contract_address = initial_call_frame.to; + let new_account = self.get_account(new_contract_address); + + let value = initial_call_frame.msg_value; + let balance = new_account + .info + .balance + .checked_add(value) + .ok_or(InternalError::ArithmeticOperationOverflow)?; + + if new_account.has_code_or_nonce() { return self.handle_create_non_empty_account(&initial_call_frame); } + + let created_contract = Account::new(balance, Bytes::new(), 1, HashMap::new()); + cache::insert_account(&mut self.cache, new_contract_address, created_contract); } let mut report = self.execute(&mut initial_call_frame)?; - if self.is_create() && !report.is_success() { - remove_account(&mut self.cache, &initial_call_frame.to); - } self.post_execution_changes(&initial_call_frame, &mut report)?; // There shouldn't be any errors here but I don't know what the desired behavior is if something goes wrong. @@ -1038,18 +1025,6 @@ impl VM { Ok(()) } - pub fn cache_from_db(&mut self, address: Address) { - let acc_info = self.db.get_account_info(address); - cache::insert_account( - &mut self.cache, - address, - Account { - info: acc_info.clone(), - storage: HashMap::new(), - }, - ); - } - /// Accesses to an account's information. /// /// Accessed accounts are stored in the `touched_accounts` set. @@ -1199,18 +1174,7 @@ impl VM { /// Gets account, first checking the cache and then the database (caching in the second case) pub fn get_account(&mut self, address: Address) -> Account { - match cache::get_account(&self.cache, &address) { - Some(acc) => acc.clone(), - None => { - let account_info = self.db.get_account_info(address); - let account = Account { - info: account_info, - storage: HashMap::new(), - }; - cache::insert_account(&mut self.cache, address, account.clone()); - account - } - } + get_account(&mut self.cache, &self.db, address) } fn handle_create_non_empty_account( @@ -1251,3 +1215,19 @@ fn get_number_of_topics(op: Opcode) -> Result { Ok(number_of_topics) } + +/// Gets account, first checking the cache and then the database (caching in the second case) +pub fn get_account(cache: &mut CacheDB, db: &Arc, address: Address) -> Account { + match cache::get_account(cache, &address) { + Some(acc) => acc.clone(), + None => { + let account_info = db.get_account_info(address); + let account = Account { + info: account_info, + storage: HashMap::new(), + }; + cache::insert_account(cache, address, account.clone()); + account + } + } +}