fix(levm): change gas consumption order and some logic (#1414)

**Motivation**

<!-- Why does this pull request exist? What are its goals? -->
- 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

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->

Tests Passing: 1584 total

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

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,42 +667,18 @@ 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);
                     }
                 }
             };
         }
 
-        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)
     }