Skip to content

refactor(levm): removed call frame as parameter from certain functions #2645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

refactor(levm): removed call frame as parameter from certain functions #2645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3791861
Started refactor from scratch to see why tests are failing
SDartayet Apr 30, 2025
5e71108
Ran cargo fmt
SDartayet Apr 30, 2025
d02ca55
Applied the refactor to handle_opcode_result
SDartayet Apr 30, 2025
6419eb5
Moved call frame pop to return handlers
SDartayet Apr 30, 2025
c6d65dd
Removed unused import and renamed variables
SDartayet Apr 30, 2025
739598f
Removed unnecessary imports
SDartayet Apr 30, 2025
13f251b
Small bug fix in instruction order
SDartayet Apr 30, 2025
0d172c9
Revert "Removed unnecessary imports"
SDartayet Apr 30, 2025
ba6aaad
Revert "Revert "Removed unnecessary imports""
SDartayet Apr 30, 2025
6ff11af
Undoing previous changes
SDartayet Apr 30, 2025
7062c3c
Fixed bug with refund handling
SDartayet May 5, 2025
2e6b940
Ran cargo fmt
SDartayet May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 79 additions & 75 deletions crates/vm/levm/src/execution_handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::{
call_frame::CallFrame,
constants::*,
errors::{ExecutionReport, InternalError, OpcodeResult, OutOfGasError, TxResult, VMError},
gas_cost::CODE_DEPOSIT_COST,
Expand All @@ -8,35 +7,31 @@ use crate::{
vm::{StateBackup, VM},
};

use ethrex_common::types::Fork;

use bytes::Bytes;

impl<'a> VM<'a> {
pub fn handle_precompile_result(
&mut self,
precompile_result: Result<Bytes, VMError>,
backup: StateBackup,
current_call_frame: &mut CallFrame,
) -> Result<ExecutionReport, VMError> {
match precompile_result {
Ok(output) => Ok(ExecutionReport {
result: TxResult::Success,
gas_used: current_call_frame.gas_used,
gas_used: self.current_call_frame()?.gas_used,
gas_refunded: self.env.refunded_gas,
output,
logs: std::mem::take(&mut current_call_frame.logs),
logs: std::mem::take(&mut self.current_call_frame_mut()?.logs),
}),
Err(error) => {
if error.is_internal() {
return Err(error);
}

self.restore_state(backup, current_call_frame.cache_backup.clone())?;

self.restore_state(backup, self.current_call_frame()?.cache_backup.clone())?;
Ok(ExecutionReport {
result: TxResult::Revert(error),
gas_used: current_call_frame.gas_limit,
gas_used: self.current_call_frame()?.gas_limit,
gas_refunded: self.env.refunded_gas,
output: Bytes::new(),
logs: vec![],
Expand Down Expand Up @@ -150,38 +145,39 @@ impl<'a> VM<'a> {
}
}

pub fn handle_opcode_result(
&mut self,
current_call_frame: &mut CallFrame,
) -> Result<ExecutionReport, VMError> {
pub fn handle_opcode_result(&mut self) -> Result<ExecutionReport, VMError> {
let backup = self
.backups
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
// On successful create check output validity
if (self.is_create() && current_call_frame.depth == 0)
|| current_call_frame.create_op_called
let mut transaction_result = TxResult::Success;
{
let contract_code = std::mem::take(&mut current_call_frame.output);
let code_length = contract_code.len();
// Check for the case where the transaction is a contract creation one
let transaction_is_create = self.is_create();
let current_call_frame = self.current_call_frame_mut()?;
// On successful create check output validity
if (transaction_is_create && current_call_frame.depth == 0)
|| current_call_frame.create_op_called
{
let contract_code = std::mem::take(&mut current_call_frame.output);
let code_length = contract_code.len();

let code_length_u64: u64 = code_length
.try_into()
.map_err(|_| VMError::Internal(InternalError::ConversionError))?;
let code_length_u64: u64 = code_length
.try_into()
.map_err(|_| VMError::Internal(InternalError::ConversionError))?;

let code_deposit_cost: u64 =
code_length_u64
.checked_mul(CODE_DEPOSIT_COST)
.ok_or(VMError::Internal(
InternalError::ArithmeticOperationOverflow,
))?;
let code_deposit_cost: u64 =
code_length_u64
.checked_mul(CODE_DEPOSIT_COST)
.ok_or(VMError::Internal(
InternalError::ArithmeticOperationOverflow,
))?;

// Revert
// If the first byte of code is 0xef
// If the code_length > MAX_CODE_SIZE
// If current_consumed_gas + code_deposit_cost > gas_limit
let validate_create =
if code_length > MAX_CODE_SIZE && self.env.config.fork >= Fork::SpuriousDragon {
// Revert
// If the first byte of code is 0xef
// If the code_length > MAX_CODE_SIZE
// If current_consumed_gas + code_deposit_cost > gas_limit
let validate_create = if code_length > MAX_CODE_SIZE {
Err(VMError::ContractOutputTooBig)
} else if contract_code.first().unwrap_or(&0) == &INVALID_CONTRACT_PREFIX {
Err(VMError::InvalidContractPrefix)
Expand All @@ -194,41 +190,44 @@ impl<'a> VM<'a> {
Ok(current_call_frame.to)
};

match validate_create {
Ok(new_address) => {
// Set bytecode to new account if success
self.update_account_bytecode(new_address, contract_code)?;
match validate_create {
Ok(new_address) => {
// Set bytecode to new account if success
self.update_account_bytecode(new_address, contract_code)?;
}
Err(error) => {
// Revert if error
current_call_frame.gas_used = current_call_frame.gas_limit;
transaction_result = TxResult::Revert(error);
}
}
Err(error) => {
// Revert if error
current_call_frame.gas_used = current_call_frame.gas_limit;
self.restore_state(backup, current_call_frame.cache_backup.clone())?;
}
}

return Ok(ExecutionReport {
result: TxResult::Revert(error),
gas_used: current_call_frame.gas_used,
gas_refunded: self.env.refunded_gas,
output: std::mem::take(&mut current_call_frame.output),
logs: vec![],
});
}
let logs;
match transaction_result {
TxResult::Success => {
logs = std::mem::take(&mut self.current_call_frame_mut()?.logs);
}
TxResult::Revert(_) => {
logs = vec![];
// Restore state if error
self.restore_state(backup, self.current_call_frame()?.cache_backup.clone())?;
}
}
let refunded_gas = self.env.refunded_gas;
let current_call_frame = self.current_call_frame_mut()?;

Ok(ExecutionReport {
result: TxResult::Success,
result: transaction_result,
gas_used: current_call_frame.gas_used,
gas_refunded: self.env.refunded_gas,
gas_refunded: refunded_gas,
output: std::mem::take(&mut current_call_frame.output),
logs: std::mem::take(&mut current_call_frame.logs),
logs,
})
}

pub fn handle_opcode_error(
&mut self,
error: VMError,
current_call_frame: &mut CallFrame,
) -> Result<ExecutionReport, VMError> {
pub fn handle_opcode_error(&mut self, error: VMError) -> Result<ExecutionReport, VMError> {
let backup = self
.backups
.pop()
Expand All @@ -237,26 +236,31 @@ impl<'a> VM<'a> {
return Err(error);
}

// Unless error is from Revert opcode, all gas is consumed
if error != VMError::RevertOpcode {
let left_gas = current_call_frame
.gas_limit
.saturating_sub(current_call_frame.gas_used);
current_call_frame.gas_used = current_call_frame.gas_used.saturating_add(left_gas);
}
let execution_report;

let refunded = backup.refunded_gas;
let output = std::mem::take(&mut current_call_frame.output); // Bytes::new() if error is not RevertOpcode
let gas_used = current_call_frame.gas_used;
{
let current_call_frame = self.current_call_frame_mut()?;
// Unless error is from Revert opcode, all gas is consumed
if error != VMError::RevertOpcode {
let left_gas = current_call_frame
.gas_limit
.saturating_sub(current_call_frame.gas_used);
current_call_frame.gas_used = current_call_frame.gas_used.saturating_add(left_gas);
}
let refunded = backup.refunded_gas;
let output = std::mem::take(&mut current_call_frame.output); // Bytes::new() if error is not RevertOpcode
let gas_used = current_call_frame.gas_used;
execution_report = Ok(ExecutionReport {
result: TxResult::Revert(error),
gas_used,
gas_refunded: refunded,
output,
logs: vec![],
});
}

self.restore_state(backup, current_call_frame.cache_backup.clone())?;
self.restore_state(backup, self.current_call_frame()?.cache_backup.clone())?;

Ok(ExecutionReport {
result: TxResult::Revert(error),
gas_used,
gas_refunded: refunded,
output,
logs: vec![],
})
execution_report
}
}
37 changes: 19 additions & 18 deletions crates/vm/levm/src/opcode_handlers/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,32 +908,30 @@ impl<'a> VM<'a> {
Ok(OpcodeResult::Continue { pc_increment: 0 })
}

pub fn handle_return(
&mut self,
call_frame: &CallFrame,
tx_report: &ExecutionReport,
) -> Result<bool, VMError> {
if call_frame.depth == 0 {
self.call_frames.push(call_frame.clone());
pub fn handle_return(&mut self, tx_report: &ExecutionReport) -> Result<bool, VMError> {
if self.current_call_frame()?.depth == 0 {
return Ok(false);
}
let retdata = self
.return_data
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
if retdata.is_create {
self.handle_return_create(call_frame, tx_report, retdata)?;
self.handle_return_create(tx_report, retdata)?;
} else {
self.handle_return_call(call_frame, tx_report, retdata)?;
self.handle_return_call(tx_report, retdata)?;
}
Ok(true)
}
pub fn handle_return_call(
&mut self,
call_frame: &CallFrame,
tx_report: &ExecutionReport,
retdata: RetData,
) -> Result<(), VMError> {
let call_frame = self
.call_frames
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
// Return gas left from subcontext
let gas_left_from_new_call_frame = call_frame
.gas_limit
Expand Down Expand Up @@ -984,32 +982,35 @@ impl<'a> VM<'a> {
}
pub fn handle_return_create(
&mut self,
call_frame: &CallFrame,
tx_report: &ExecutionReport,
retdata: RetData,
) -> Result<(), VMError> {
let previous_call_frame = self
.call_frames
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
let unused_gas = retdata
.max_message_call_gas
.checked_sub(tx_report.gas_used)
.ok_or(InternalError::GasOverflow)?;

{
let current_call_frame = self.current_call_frame_mut()?;
let call_frame_to_execute = self.current_call_frame_mut()?;
// Return reserved gas
current_call_frame.gas_used = current_call_frame
call_frame_to_execute.gas_used = call_frame_to_execute
.gas_used
.checked_sub(unused_gas)
.ok_or(InternalError::GasOverflow)?;

current_call_frame.logs.extend(tx_report.logs.clone());
call_frame_to_execute.logs.extend(tx_report.logs.clone());
}

match tx_report.result.clone() {
TxResult::Success => {
self.current_call_frame_mut()?
.stack
.push(address_to_word(retdata.to))?;
for (address, account_opt) in call_frame.cache_backup.clone() {
for (address, account_opt) in previous_call_frame.cache_backup.clone() {
self.current_call_frame_mut()?
.cache_backup
.entry(address)
Expand All @@ -1024,12 +1025,12 @@ impl<'a> VM<'a> {
cache::remove_account(&mut self.db.cache, &retdata.to);
self.accrued_substate.created_accounts.remove(&retdata.to);

let current_call_frame = self.current_call_frame_mut()?;
let call_frame_to_execute = self.current_call_frame_mut()?;
// If revert we have to copy the return_data
if err == VMError::RevertOpcode {
current_call_frame.sub_return_data = tx_report.output.clone();
call_frame_to_execute.sub_return_data = tx_report.output.clone();
}
current_call_frame.stack.push(CREATE_DEPLOYMENT_FAIL)?;
call_frame_to_execute.stack.push(CREATE_DEPLOYMENT_FAIL)?;
}
}
Ok(())
Expand Down
27 changes: 7 additions & 20 deletions crates/vm/levm/src/vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,18 +299,13 @@ impl<'a> VM<'a> {
let fork = self.env.config.fork;

if is_precompile(&self.current_call_frame()?.code_address, fork) {
let mut current_call_frame = self
.call_frames
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
let precompile_result = execute_precompile(&mut current_call_frame, fork);
let precompile_result = execute_precompile(self.current_call_frame_mut()?, fork);
let backup = self
.backups
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
let report =
self.handle_precompile_result(precompile_result, backup, &mut current_call_frame)?;
self.handle_return(&current_call_frame, &report)?;
let report = self.handle_precompile_result(precompile_result, backup)?;
self.handle_return(&report)?;
self.current_call_frame_mut()?.increment_pc_by(1)?;
return Ok(report);
}
Expand All @@ -325,24 +320,16 @@ impl<'a> VM<'a> {
.current_call_frame_mut()?
.increment_pc_by(pc_increment)?,
Ok(OpcodeResult::Halt) => {
let mut current_call_frame = self
.call_frames
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
let report = self.handle_opcode_result(&mut current_call_frame)?;
if self.handle_return(&current_call_frame, &report)? {
let report = self.handle_opcode_result()?;
if self.handle_return(&report)? {
self.current_call_frame_mut()?.increment_pc_by(1)?;
} else {
return Ok(report);
}
}
Err(error) => {
let mut current_call_frame = self
.call_frames
.pop()
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?;
let report = self.handle_opcode_error(error, &mut current_call_frame)?;
if self.handle_return(&current_call_frame, &report)? {
let report = self.handle_opcode_error(error)?;
if self.handle_return(&report)? {
self.current_call_frame_mut()?.increment_pc_by(1)?;
} else {
return Ok(report);
Expand Down
Loading