From ab7d6bc67685fbb67eb52de042d40e12e0dfaa0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jerem=C3=ADas=20Salom=C3=B3n?= <48994069+JereSalo@users.noreply.github.com> Date: Thu, 5 Dec 2024 16:35:54 -0300 Subject: [PATCH] fix(levm): change gas consumption order and some logic (#1414) **Motivation** - Gas consumption order is wrong, we are substracting balance from the user at the end of the transaction and that's not how it should be. - There are some things to fix and organize related to gas usage so this PR can address that. **Description** - Contemplates gas refunds for coinbase fee - Substracts up-front cost from user pre-execution and returns unused gas contemplating gas refunds post-execution. - Changes behavior when reverting create post execution -> If revert reason is the RevertOpcode don't consume remaining gas. - `validate_transaction` is now named `prepare_execution` because it both validates and makes pre-execution changes (that are very related with validations). Is has added functionalities like adding the value to the receiver's balance. - It creates method `post_execution_changes()` for organizing `transact()` method Tests Passing: 1584 total --- crates/vm/levm/src/errors.rs | 4 +- crates/vm/levm/src/vm.rs | 157 +++++++++++++++++++++-------------- 2 files changed, 97 insertions(+), 64 deletions(-) diff --git a/crates/vm/levm/src/errors.rs b/crates/vm/levm/src/errors.rs index 99ff007ed..daba52547 100644 --- a/crates/vm/levm/src/errors.rs +++ b/crates/vm/levm/src/errors.rs @@ -116,8 +116,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, } @@ -178,6 +176,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 752db87da..47253e952 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -414,7 +414,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) } @@ -429,12 +429,13 @@ 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 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 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 @@ -463,19 +464,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 { @@ -498,7 +498,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)) = ( @@ -571,6 +572,79 @@ impl VM { } } + if self.is_create() { + // Assign bytecode to context and empty calldata + initial_call_frame.bytecode = initial_call_frame.calldata.clone(); + initial_call_frame.calldata = Bytes::new(); + } + + 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, + 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)?; + } + + // 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( + 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" + + 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)))?; + + self.increase_account_balance(sender_address, U256::from(gas_return_amount))?; + + // 3. Pay 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 + .low_u64() + .checked_sub(self.env.base_fee_per_gas.low_u64()) + .ok_or(VMError::GasPriceIsLowerThanBaseFee)?; + let coinbase_fee = gas_to_pay_coinbase + .checked_mul(priority_fee_per_gas) + .ok_or(VMError::BalanceOverflow)?; + + if coinbase_fee != 0 { + self.increase_account_balance(coinbase_address, U256::from(coinbase_fee))?; + }; + Ok(()) } @@ -581,27 +655,10 @@ impl VM { .ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?; let cache_before_execution = self.cache.clone(); - self.validate_transaction(&mut initial_call_frame)?; - - if self.is_create() { - // Assign bytecode to context and empty calldata - initial_call_frame.bytecode = initial_call_frame.calldata.clone(); - initial_call_frame.calldata = Bytes::new(); - } - - // 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)?; + self.prepare_execution(&mut initial_call_frame)?; let mut report = self.execute(&mut initial_call_frame)?; - 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)?; - } - if self.is_create() { match self.create_post_execution(&mut initial_call_frame, &mut report) { Ok(_) => {} @@ -610,7 +667,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,34 +677,8 @@ impl VM { }; } - let coinbase_address = self.env.coinbase; - - self.decrease_account_balance( - sender, - U256::from(report.gas_used) - .checked_mul(self.env.gas_price) - .ok_or(VMError::GasLimitPriceProductOverflow)?, - )?; - self.increase_account_balance( - sender, - U256::from(report.gas_refunded) - .checked_mul(self.env.gas_price) - .ok_or(VMError::GasLimitPriceProductOverflow)?, - )?; - - // Send coinbase fee - let priority_fee_per_gas = self - .env - .gas_price - .checked_sub(self.env.base_fee_per_gas) - .ok_or(VMError::GasPriceIsLowerThanBaseFee)?; - let coinbase_fee = (U256::from(report.gas_used)) - .checked_mul(priority_fee_per_gas) - .ok_or(VMError::BalanceOverflow)?; - - if !coinbase_fee.is_zero() { - self.increase_account_balance(coinbase_address, coinbase_fee)?; - } + 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); @@ -1081,7 +1114,7 @@ impl VM { .info .nonce .checked_add(1) - .ok_or(VMError::TxValidation(TxValidationError::NonceIsMax))?; + .ok_or(VMError::NonceOverflow)?; Ok(account.info.nonce) }