From 74a51d87f9157318c7e92600bd36d4829a2911b6 Mon Sep 17 00:00:00 2001 From: JereSalo Date: Thu, 5 Dec 2024 10:49:20 -0300 Subject: [PATCH 1/4] change validate transaction name --- crates/vm/levm/src/vm.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index 7a2a58a43..cff7da7be 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -435,12 +435,12 @@ impl VM { /// ## Description /// This method performs validations and returns an error if any of the validations fail. - /// It also makes initial changes alongside the validations: + /// It also makes pre-execution changes: /// - It increases sender nonce /// - It substracts up-front-cost from sender balance. (Not doing this for now) /// - It calculates and adds intrinsic gas to the 'gas used' of callframe and environment. /// See 'docs' for more information about validations. - fn validate_transaction(&mut self, initial_call_frame: &mut CallFrame) -> Result<(), VMError> { + fn prepare_execution(&mut self, initial_call_frame: &mut CallFrame) -> Result<(), VMError> { //TODO: This should revert the transaction, not throw an error. And I don't know if it should be done here... // if self.is_create() { // // If address is already in db, there's an error @@ -587,7 +587,7 @@ impl VM { .ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?; let cache_before_execution = self.cache.clone(); - self.validate_transaction(&mut initial_call_frame)?; + self.prepare_execution(&mut initial_call_frame)?; // Maybe can be done in validate_transaction let sender = initial_call_frame.msg_sender; From 761c58cd786a1045a15b190af73b223b72f218d1 Mon Sep 17 00:00:00 2001 From: JereSalo Date: Thu, 5 Dec 2024 12:16:23 -0300 Subject: [PATCH 2/4] refactor gas consumption order and add some things --- crates/vm/levm/src/errors.rs | 4 +- crates/vm/levm/src/vm.rs | 87 +++++++++++++++++++++++------------- 2 files changed, 57 insertions(+), 34 deletions(-) diff --git a/crates/vm/levm/src/errors.rs b/crates/vm/levm/src/errors.rs index c56a3f428..0f76834e1 100644 --- a/crates/vm/levm/src/errors.rs +++ b/crates/vm/levm/src/errors.rs @@ -114,8 +114,6 @@ pub enum TxValidationError { Type3TxBlobCountExceeded, #[error("Type3TxContractCreation")] Type3TxContractCreation, - #[error("Undefined state")] - UndefinedState(i32), // This error is temporarily for things that cause an undefined state. #[error("Gas limit price product overflow")] GasLimitPriceProductOverflow, } @@ -176,6 +174,8 @@ pub enum InternalError { UtilsError, #[error("PC out of bounds")] PCOutOfBounds, + #[error("Undefined state")] + UndefinedState(i32), // This error is temporarily for things that cause an undefined state. } #[derive(Debug, Clone)] diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index cff7da7be..21b2d8406 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -420,7 +420,7 @@ impl VM { .tx_max_fee_per_blob_gas .unwrap_or_default() .checked_mul(blob_gas_used) - .ok_or(TxValidationError::UndefinedState(1))?; + .ok_or(InternalError::UndefinedState(1))?; Ok(blob_gas_cost) } @@ -437,7 +437,8 @@ impl VM { /// This method performs validations and returns an error if any of the validations fail. /// It also makes pre-execution changes: /// - It increases sender nonce - /// - It substracts up-front-cost from sender balance. (Not doing this for now) + /// - It substracts up-front-cost from sender balance. + /// - It adds value to receiver balance. /// - It calculates and adds intrinsic gas to the 'gas used' of callframe and environment. /// See 'docs' for more information about validations. fn prepare_execution(&mut self, initial_call_frame: &mut CallFrame) -> Result<(), VMError> { @@ -469,19 +470,18 @@ impl VM { let up_front_cost = gaslimit_price_product .checked_add(value) - .ok_or(TxValidationError::UndefinedState(1))? + .ok_or(InternalError::UndefinedState(1))? .checked_add(blob_gas_cost) - .ok_or(TxValidationError::UndefinedState(1))?; + .ok_or(InternalError::UndefinedState(1))?; // There is no error specified for overflow in up_front_cost in ef_tests. Maybe we can go with GasLimitPriceProductOverflow or InsufficientAccountFunds. // (2) INSUFFICIENT_ACCOUNT_FUNDS - // NOT CHANGING SENDER BALANCE HERE FOR NOW - // This will be increment_account_balance - sender_account - .info - .balance - .checked_sub(up_front_cost) - .ok_or(TxValidationError::InsufficientAccountFunds)?; + self.decrease_account_balance(sender_address, up_front_cost) + .map_err(|_| TxValidationError::InsufficientAccountFunds)?; + + // Transfer value to receiver + let receiver_address = initial_call_frame.to; + self.increase_account_balance(receiver_address, initial_call_frame.msg_value)?; // (3) INSUFFICIENT_MAX_FEE_PER_GAS if self.max_fee_per_gas_or_gasprice() < self.env.base_fee_per_gas { @@ -504,7 +504,8 @@ impl VM { self.add_intrinsic_gas(initial_call_frame)?; // (6) NONCE_IS_MAX - self.increment_account_nonce(sender_address)?; + self.increment_account_nonce(sender_address) + .map_err(|_| VMError::TxValidation(TxValidationError::NonceIsMax))?; // (7) PRIORITY_GREATER_THAN_MAX_FEE_PER_GAS if let (Some(tx_max_priority_fee), Some(tx_max_fee_per_gas)) = ( @@ -589,17 +590,14 @@ impl VM { let cache_before_execution = self.cache.clone(); self.prepare_execution(&mut initial_call_frame)?; - // Maybe can be done in validate_transaction - let sender = initial_call_frame.msg_sender; - let receiver_address = initial_call_frame.to; - self.decrease_account_balance(sender, initial_call_frame.msg_value)?; - self.increase_account_balance(receiver_address, initial_call_frame.msg_value)?; - let mut report = self.execute(&mut initial_call_frame)?; + let sender_address = initial_call_frame.msg_sender; + let receiver_address = initial_call_frame.to; + if let TxResult::Revert(_) = report.result { self.decrease_account_balance(receiver_address, initial_call_frame.msg_value)?; - self.increase_account_balance(sender, initial_call_frame.msg_value)?; + self.increase_account_balance(sender_address, initial_call_frame.msg_value)?; } if self.is_create() { @@ -610,7 +608,9 @@ impl VM { return Err(error); } else { report.result = TxResult::Revert(error); - report.gas_used = self.env.gas_limit.low_u64(); + if report.result != TxResult::Revert(VMError::RevertOpcode) { + report.gas_used = self.env.gas_limit.low_u64(); // Consume all gas unless the error cause is revert opcode + } self.cache = cache_before_execution; remove_account(&mut self.cache, &initial_call_frame.to); } @@ -618,27 +618,50 @@ impl VM { }; } - let coinbase_address = self.env.coinbase; + // Give back to the user the unused gas + refunds + let max_gas = self.env.gas_limit.low_u64(); + let consumed_gas = report.gas_used; + let refunded_gas = report.gas_refunded.min( + consumed_gas + .checked_div(5) + .ok_or(VMError::Internal(InternalError::UndefinedState(-1)))?, + ); + // "The max refundable proportion of gas was reduced from one half to one fifth by EIP-3529 by Buterin and Swende [2021] in the London release" - self.decrease_account_balance( - sender, - U256::from(report.gas_used) - .checked_mul(self.env.gas_price) - .ok_or(VMError::GasLimitPriceProductOverflow)?, - )?; + let gas_to_return = max_gas + .checked_sub(consumed_gas) + .and_then(|gas| gas.checked_add(refunded_gas)) + .ok_or(VMError::Internal(InternalError::UndefinedState(0)))?; + + let gas_return_amount = self + .env + .gas_price + .low_u64() + .checked_mul(gas_to_return) + .ok_or(VMError::Internal(InternalError::UndefinedState(1)))?; + + // Increase gas not used instead + self.increase_account_balance(sender_address, U256::from(gas_return_amount))?; // Send coinbase fee + let coinbase_address = self.env.coinbase; + + let gas_to_pay_coinbase = consumed_gas + .checked_sub(refunded_gas) + .ok_or(VMError::Internal(InternalError::UndefinedState(2)))?; + let priority_fee_per_gas = self .env .gas_price - .checked_sub(self.env.base_fee_per_gas) + .low_u64() + .checked_sub(self.env.base_fee_per_gas.low_u64()) .ok_or(VMError::GasPriceIsLowerThanBaseFee)?; - let coinbase_fee = (U256::from(report.gas_used)) + let coinbase_fee = gas_to_pay_coinbase .checked_mul(priority_fee_per_gas) .ok_or(VMError::BalanceOverflow)?; - if !coinbase_fee.is_zero() { - self.increase_account_balance(coinbase_address, coinbase_fee)?; + if coinbase_fee != 0 { + self.increase_account_balance(coinbase_address, U256::from(coinbase_fee))?; } report.new_state.clone_from(&self.cache); @@ -1071,7 +1094,7 @@ impl VM { .info .nonce .checked_add(1) - .ok_or(VMError::TxValidation(TxValidationError::NonceIsMax))?; + .ok_or(VMError::NonceOverflow)?; Ok(account.info.nonce) } From efdc619f3531f167fef0dce19522020926c04d14 Mon Sep 17 00:00:00 2001 From: JereSalo Date: Thu, 5 Dec 2024 15:27:12 -0300 Subject: [PATCH 3/4] add function post execution changes, to improve readability --- crates/vm/levm/src/vm.rs | 76 +++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index 21b2d8406..0f79f5d6b 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -581,44 +581,22 @@ impl VM { Ok(()) } - pub fn transact(&mut self) -> Result { - let mut initial_call_frame = self - .call_frames - .pop() - .ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?; - - let cache_before_execution = self.cache.clone(); - self.prepare_execution(&mut initial_call_frame)?; - - let mut report = self.execute(&mut initial_call_frame)?; - + fn post_execution_changes( + &mut self, + initial_call_frame: &CallFrame, + report: &TransactionReport, + ) -> Result<(), VMError> { + // POST-EXECUTION Changes let sender_address = initial_call_frame.msg_sender; let receiver_address = initial_call_frame.to; + // 1. Undo value transfer if the transaction was reverted if let TxResult::Revert(_) = report.result { self.decrease_account_balance(receiver_address, initial_call_frame.msg_value)?; self.increase_account_balance(sender_address, initial_call_frame.msg_value)?; } - if self.is_create() { - match self.create_post_execution(&mut initial_call_frame, &mut report) { - Ok(_) => {} - Err(error) => { - if error.is_internal() { - return Err(error); - } else { - report.result = TxResult::Revert(error); - if report.result != TxResult::Revert(VMError::RevertOpcode) { - report.gas_used = self.env.gas_limit.low_u64(); // Consume all gas unless the error cause is revert opcode - } - self.cache = cache_before_execution; - remove_account(&mut self.cache, &initial_call_frame.to); - } - } - }; - } - - // Give back to the user the unused gas + refunds + // 2. Return unused gas + gas refunds to the sender. let max_gas = self.env.gas_limit.low_u64(); let consumed_gas = report.gas_used; let refunded_gas = report.gas_refunded.min( @@ -640,10 +618,9 @@ impl VM { .checked_mul(gas_to_return) .ok_or(VMError::Internal(InternalError::UndefinedState(1)))?; - // Increase gas not used instead self.increase_account_balance(sender_address, U256::from(gas_return_amount))?; - // Send coinbase fee + // 3. Pay coinbase fee let coinbase_address = self.env.coinbase; let gas_to_pay_coinbase = consumed_gas @@ -662,8 +639,43 @@ impl VM { if coinbase_fee != 0 { self.increase_account_balance(coinbase_address, U256::from(coinbase_fee))?; + }; + + Ok(()) + } + + pub fn transact(&mut self) -> Result { + let mut initial_call_frame = self + .call_frames + .pop() + .ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?; + + let cache_before_execution = self.cache.clone(); + self.prepare_execution(&mut initial_call_frame)?; + + let mut report = self.execute(&mut initial_call_frame)?; + + if self.is_create() { + match self.create_post_execution(&mut initial_call_frame, &mut report) { + Ok(_) => {} + Err(error) => { + if error.is_internal() { + return Err(error); + } else { + report.result = TxResult::Revert(error); + if report.result != TxResult::Revert(VMError::RevertOpcode) { + report.gas_used = self.env.gas_limit.low_u64(); // Consume all gas unless the error cause is revert opcode + } + self.cache = cache_before_execution; + remove_account(&mut self.cache, &initial_call_frame.to); + } + } + }; } + self.post_execution_changes(&initial_call_frame, &report)?; + // There shouldn't be any errors here but I don't know what the desired behavior is if something goes wrong. + report.new_state.clone_from(&self.cache); Ok(report) From a6872d62208c3294db390d1b30a95a67e8324e33 Mon Sep 17 00:00:00 2001 From: JereSalo Date: Thu, 5 Dec 2024 15:37:12 -0300 Subject: [PATCH 4/4] add comment to function --- crates/vm/levm/src/vm.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index 0f79f5d6b..aa8fd6b9c 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -581,6 +581,10 @@ impl VM { Ok(()) } + /// ## Changes post execution + /// 1. Undo value transfer if the transaction was reverted + /// 2. Return unused gas + gas refunds to the sender. + /// 3. Pay coinbase fee fn post_execution_changes( &mut self, initial_call_frame: &CallFrame,