Skip to content
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

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

Merged
merged 6 commits into from
Dec 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
74a51d8
change validate transaction name
JereSalo Dec 5, 2024
761c58c
refactor gas consumption order and add some things
JereSalo Dec 5, 2024
949a263
merge main
JereSalo Dec 5, 2024
efdc619
add function post execution changes, to improve readability
JereSalo Dec 5, 2024
a6872d6
add comment to function
JereSalo Dec 5, 2024
5b3eef9
merge main, fix conflicts
JereSalo Dec 5, 2024
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
4 changes: 2 additions & 2 deletions crates/vm/levm/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down Expand Up @@ -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)]
Expand Down
157 changes: 95 additions & 62 deletions crates/vm/levm/src/vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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)) = (
Expand Down Expand Up @@ -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(())
}

Expand All @@ -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(_) => {}
Expand All @@ -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);

Expand Down Expand Up @@ -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)
}

Expand Down
Loading