refactor(levm): moved retdata from vm to callframe (#2694)

**Motivation**

Make code more readable by coupling related behaviour.

**Description**

Some fields in retdata were repeated in the callframe, and the retdata
was always being popped alongside the current call frame. This PR
deletes the retdata struct, and moves the fields that weren't redundant
to the call frame, refactoring the code where appropriate.

It also removes `gas_used` from `CallFrame::new()` because we were
always setting it to zero.

Closes #2571, Closes #2720

---------

Co-authored-by: JereSalo <[email protected]>
Co-authored-by: Jeremías Salomón <[email protected]>

Author: SDartayet

crates/vm/levm/src/call_frame.rs

@@ -94,6 +94,12 @@ pub struct CallFrame {
     pub create_op_called: bool,
     /// Everytime we want to write an account during execution of a callframe we store the pre-write state so that we can restore if it reverts
     pub call_frame_backup: CallFrameBackup,
+    /// Return data offset
+    pub ret_offset: U256,
+    /// Return data size
+    pub ret_size: usize,
+    /// If true then transfer value from caller to callee
+    pub should_transfer_value: bool,
 }
 
 #[derive(Debug, Clone, Eq, PartialEq, Default)]
@@ -113,9 +119,11 @@ impl CallFrame {
         calldata: Bytes,
         is_static: bool,
         gas_limit: u64,
-        gas_used: u64,
         depth: usize,
+        should_transfer_value: bool,
         create_op_called: bool,
+        ret_offset: U256,
+        ret_size: usize,
     ) -> Self {
         let valid_jump_destinations = get_valid_jump_destinations(&bytecode).unwrap_or_default();
         Self {
@@ -128,9 +136,11 @@ impl CallFrame {
             calldata,
             is_static,
             depth,
-            gas_used,
             valid_jump_destinations,
+            should_transfer_value,
             create_op_called,
+            ret_offset,
+            ret_size,
             ..Default::default()
         }
     }

crates/vm/levm/src/opcode_handlers/system.rs

@@ -7,7 +7,7 @@ use crate::{
     memory::{self, calculate_memory_size},
     precompiles::is_precompile,
     utils::{address_to_word, word_to_address, *},
-    vm::{RetData, StateBackup, VM},
+    vm::{StateBackup, VM},
 };
 use bytes::Bytes;
 use ethrex_common::{
@@ -715,24 +715,16 @@ impl<'a> VM<'a> {
             Bytes::new(),
             false,
             max_message_call_gas,
-            0,
             new_depth,
             true,
+            true,
+            U256::zero(),
+            0,
         );
         self.call_frames.push(new_call_frame);
 
         self.accrued_substate.created_accounts.insert(new_address); // Mostly for SELFDESTRUCT during initcode.
 
-        self.return_data.push(RetData {
-            is_create: true,
-            ret_offset: U256::zero(),
-            ret_size: 0,
-            should_transfer_value: true,
-            to: new_address,
-            msg_sender: deployer_address,
-            value: value_in_wei_to_send,
-            max_message_call_gas,
-        });
         // Backup of Database, Substate, Gas Refunds and Transient Storage if sub-context is reverted
         let backup = StateBackup::new(
             self.accrued_substate.clone(),
@@ -823,16 +815,6 @@ impl<'a> VM<'a> {
             self.increase_account_balance(to, value)?;
         }
 
-        self.return_data.push(RetData {
-            is_create: false,
-            ret_offset,
-            ret_size,
-            should_transfer_value,
-            to,
-            msg_sender,
-            value,
-            max_message_call_gas: gas_limit,
-        });
         let new_call_frame = CallFrame::new(
             msg_sender,
             to,
@@ -842,9 +824,11 @@ impl<'a> VM<'a> {
             calldata.into(),
             is_static,
             gas_limit,
-            0,
             new_depth,
+            should_transfer_value,
             false,
+            ret_offset,
+            ret_size,
         );
         self.call_frames.push(new_call_frame);
         // Backup of Database, Substate, Gas Refunds and Transient Storage if sub-context is reverted
@@ -870,22 +854,17 @@ impl<'a> VM<'a> {
             self.call_frames.push(executed_call_frame.clone());
             return Ok(false);
         }
-        let retdata = self
-            .return_data
-            .pop()
-            .ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
-        if retdata.is_create {
-            self.handle_return_create(executed_call_frame, tx_report, retdata)?;
+        if executed_call_frame.create_op_called {
+            self.handle_return_create(executed_call_frame, tx_report)?;
         } else {
-            self.handle_return_call(executed_call_frame, tx_report, retdata)?;
+            self.handle_return_call(executed_call_frame, tx_report)?;
         }
         Ok(true)
     }
     pub fn handle_return_call(
         &mut self,
         executed_call_frame: &CallFrame,
         tx_report: &ExecutionReport,
-        retdata: RetData,
     ) -> Result<(), VMError> {
         // Return gas left from subcontext
         let gas_left_from_new_call_frame = executed_call_frame
@@ -902,8 +881,8 @@ impl<'a> VM<'a> {
             parent_call_frame.logs.extend(tx_report.logs.clone());
             memory::try_store_range(
                 &mut parent_call_frame.memory,
-                retdata.ret_offset,
-                retdata.ret_size,
+                executed_call_frame.ret_offset,
+                executed_call_frame.ret_size,
                 &tx_report.output,
             )?;
             parent_call_frame.sub_return_data = tx_report.output.clone();
@@ -919,10 +898,16 @@ impl<'a> VM<'a> {
             }
             TxResult::Revert(_) => {
                 // Revert value transfer
-                if retdata.should_transfer_value {
-                    self.decrease_account_balance(retdata.to, retdata.value)?;
-
-                    self.increase_account_balance(retdata.msg_sender, retdata.value)?;
+                if executed_call_frame.should_transfer_value {
+                    self.decrease_account_balance(
+                        executed_call_frame.to,
+                        executed_call_frame.msg_value,
+                    )?;
+
+                    self.increase_account_balance(
+                        executed_call_frame.msg_sender,
+                        executed_call_frame.msg_value,
+                    )?;
                 }
                 // Push 0 to stack
                 self.current_call_frame_mut()?.stack.push(REVERT_FOR_CALL)?;
@@ -934,10 +919,9 @@ impl<'a> VM<'a> {
         &mut self,
         executed_call_frame: &CallFrame,
         tx_report: &ExecutionReport,
-        retdata: RetData,
     ) -> Result<(), VMError> {
-        let unused_gas = retdata
-            .max_message_call_gas
+        let unused_gas = executed_call_frame
+            .gas_limit
             .checked_sub(tx_report.gas_used)
             .ok_or(InternalError::GasOverflow)?;
 
@@ -956,16 +940,21 @@ impl<'a> VM<'a> {
             TxResult::Success => {
                 self.current_call_frame_mut()?
                     .stack
-                    .push(address_to_word(retdata.to))?;
+                    .push(address_to_word(executed_call_frame.to))?;
                 self.merge_call_frame_backup_with_parent(&executed_call_frame.call_frame_backup)?;
             }
             TxResult::Revert(err) => {
                 // Return value to sender
-                self.increase_account_balance(retdata.msg_sender, retdata.value)?;
+                self.increase_account_balance(
+                    executed_call_frame.msg_sender,
+                    executed_call_frame.msg_value,
+                )?;
 
                 // Deployment failed so account shouldn't exist
-                cache::remove_account(&mut self.db.cache, &retdata.to);
-                self.accrued_substate.created_accounts.remove(&retdata.to);
+                cache::remove_account(&mut self.db.cache, &executed_call_frame.to);
+                self.accrued_substate
+                    .created_accounts
+                    .remove(&executed_call_frame.to);
 
                 let current_call_frame = self.current_call_frame_mut()?;
                 // If revert we have to copy the return_data

crates/vm/levm/src/vm.rs

@@ -159,23 +159,11 @@ pub struct VM<'a> {
     pub access_list: AccessList,
     pub authorization_list: Option<AuthorizationList>,
     pub hooks: Vec<Arc<dyn Hook>>,
-    pub return_data: Vec<RetData>,
     pub backups: Vec<StateBackup>,
     /// Original storage values before the transaction. Used for gas calculations in SSTORE.
     pub storage_original_values: HashMap<Address, HashMap<H256, U256>>,
 }
 
-pub struct RetData {
-    pub is_create: bool,
-    pub ret_offset: U256,
-    pub ret_size: usize,
-    pub should_transfer_value: bool,
-    pub to: Address,
-    pub msg_sender: Address,
-    pub value: U256,
-    pub max_message_call_gas: u64,
-}
-
 impl<'a> VM<'a> {
     pub fn new(
         env: Environment,
@@ -280,8 +268,10 @@ impl<'a> VM<'a> {
             false,
             env.gas_limit,
             0,
-            0,
+            true,
             false,
+            U256::zero(),
+            0,
         );
 
         Ok(Self {
@@ -293,7 +283,6 @@ impl<'a> VM<'a> {
             access_list: tx.access_list(),
             authorization_list: tx.authorization_list(),
             hooks,
-            return_data: vec![],
             backups: vec![],
             storage_original_values: HashMap::new(),
         })