Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
9 changes: 9 additions & 0 deletions crates/vm/backends/levm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,15 @@ pub fn generic_system_contract_levm(
..Default::default()
};

if !(contract_address == HISTORY_STORAGE_ADDRESS.address
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be better to have a vec of system contracts somewhere? Otherwise it will be easy to miss this when a new system contract is added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a vec of system contracts. However this case is more specific; it's not any given system contract but those that have code (the function in which this check is in is already only reached when a system contract is called). We could build a vec specifically for the system contracts that do have code though (or add a has_code attribute to the existing ones)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally add a comment that we have this code in order to pass the tests and that it's impossible in real scenarios. I am not in favor of adding the check but if we want it I'd just add that comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done here. Let me know if you think the comment is adequate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great, thanks

|| contract_address == BEACON_ROOTS_ADDRESS.address)
&& db.get_account_code(contract_address)?.is_empty()
{
return Err(EvmError::SystemContractCallFailed(format!(
"System contract: {contract_address} has no code after deployment"
)));
};

let tx = &Transaction::EIP1559Transaction(EIP1559Transaction {
to: TxKind::Call(contract_address),
value: U256::zero(),
Expand Down
4 changes: 3 additions & 1 deletion crates/vm/levm/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,10 @@ pub enum TxValidationError {
priority_fee: U256,
max_fee_per_gas: U256,
},
#[error("Intrinsic gas too low")]
#[error("Transaction gas limit lower than the minimum gas cost to execute the transaction")]
IntrinsicGasTooLow,
#[error("Transaction gas limit lower than the gas cost floor for calldata tokens")]
IntrinsicGasBelowFloorGasCost,
#[error(
"Gas allowance exceeded. Block gas limit: {block_gas_limit}, transaction gas limit: {tx_gas_limit}"
)]
Expand Down
11 changes: 6 additions & 5 deletions crates/vm/levm/src/hooks/default_hook.rs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that the changes in this file are somehow unrelated to the PR title? Like the PR is for when the code is empty in system contract calls, this is for other scenario, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also did you add this because you saw a test that made it necessary or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those changes were from a separate PR I made before this one. That one already got merged though, so I'm not sure why those are showing up in the diff for this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are from this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok there, merging the latest version of main into the branch fixed it, the files changed should show up properly now

Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ use crate::{
use bytes::Bytes;
use ethrex_common::{Address, U256, types::Fork};

use std::cmp::max;

pub const MAX_REFUND_QUOTIENT: u64 = 5;

pub struct DefaultHook;
Expand Down Expand Up @@ -248,6 +246,10 @@ pub fn validate_min_gas_limit(vm: &mut VM<'_>) -> Result<(), VMError> {
let calldata = vm.current_call_frame.calldata.clone();
let intrinsic_gas: u64 = vm.get_intrinsic_gas()?;

if vm.current_call_frame.gas_limit < intrinsic_gas {
return Err(TxValidationError::IntrinsicGasTooLow.into());
}

// calldata_cost = tokens_in_calldata * 4
let calldata_cost: u64 = gas_cost::tx_calldata(&calldata)?;

Expand All @@ -261,9 +263,8 @@ pub fn validate_min_gas_limit(vm: &mut VM<'_>) -> Result<(), VMError> {
.checked_add(TX_BASE_COST)
.ok_or(InternalError::Overflow)?;

let min_gas_limit = max(intrinsic_gas, floor_cost_by_tokens);
if vm.current_call_frame.gas_limit < min_gas_limit {
return Err(TxValidationError::IntrinsicGasTooLow.into());
if vm.current_call_frame.gas_limit < floor_cost_by_tokens {
return Err(TxValidationError::IntrinsicGasBelowFloorGasCost.into());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion tooling/ef_tests/blockchain/deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ where
)
}
"TransactionException.INTRINSIC_GAS_TOO_LOW" => {
BlockChainExpectedException::TxtException("Intrinsic gas too low".to_string())
BlockChainExpectedException::TxtException("Transaction gas limit lower than the minimum gas cost to execute the transaction".to_string())
}
"TransactionException.INSUFFICIENT_ACCOUNT_FUNDS" => {
BlockChainExpectedException::TxtException(
Expand Down
3 changes: 3 additions & 0 deletions tooling/ef_tests/state/deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ where
"TransactionException.INTRINSIC_GAS_TOO_LOW" => {
TransactionExpectedException::IntrinsicGasTooLow
}
"TransactionException.INTRINSIC_BELOW_FLOOR_GAS_COST" => {
TransactionExpectedException::IntrinsicGasBelowFloorGasCost
}
"TransactionException.INSUFFICIENT_ACCOUNT_FUNDS" => {
TransactionExpectedException::InsufficientAccountFunds
}
Expand Down
3 changes: 3 additions & 0 deletions tooling/ef_tests/state/runner/levm_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,9 @@ fn exception_is_expected(
(
TransactionExpectedException::IntrinsicGasTooLow,
VMError::TxValidation(TxValidationError::IntrinsicGasTooLow)
) | (
TransactionExpectedException::IntrinsicGasBelowFloorGasCost,
VMError::TxValidation(TxValidationError::IntrinsicGasBelowFloorGasCost)
) | (
TransactionExpectedException::InsufficientAccountFunds,
VMError::TxValidation(TxValidationError::InsufficientAccountFunds)
Expand Down
1 change: 1 addition & 0 deletions tooling/ef_tests/state/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ pub enum TransactionExpectedException {
Type3TxInvalidBlobVersionedHash,
Type4TxContractCreation,
IntrinsicGasTooLow,
IntrinsicGasBelowFloorGasCost,
InsufficientAccountFunds,
SenderNotEoa,
PriorityGreaterThanMaxFeePerGas,
Expand Down
3 changes: 3 additions & 0 deletions tooling/ef_tests/state_v2/src/modules/deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ where
"TransactionException.INTRINSIC_GAS_TOO_LOW" => {
TransactionExpectedException::IntrinsicGasTooLow
}
"TransactionException.INTRINSIC_GAS_BELOW_FLOOR_GAS_COST" => {
TransactionExpectedException::IntrinsicGasBelowFloorGasCost
}
"TransactionException.INSUFFICIENT_ACCOUNT_FUNDS" => {
TransactionExpectedException::InsufficientAccountFunds
}
Expand Down
3 changes: 3 additions & 0 deletions tooling/ef_tests/state_v2/src/modules/result_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ fn exception_matches_expected(
(
TransactionExpectedException::IntrinsicGasTooLow,
VMError::TxValidation(TxValidationError::IntrinsicGasTooLow)
) | (
TransactionExpectedException::IntrinsicGasBelowFloorGasCost,
VMError::TxValidation(TxValidationError::IntrinsicGasBelowFloorGasCost)
) | (
TransactionExpectedException::InsufficientAccountFunds,
VMError::TxValidation(TxValidationError::InsufficientAccountFunds)
Expand Down
1 change: 1 addition & 0 deletions tooling/ef_tests/state_v2/src/modules/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,7 @@ pub enum TransactionExpectedException {
Type3TxInvalidBlobVersionedHash,
Type4TxContractCreation,
IntrinsicGasTooLow,
IntrinsicGasBelowFloorGasCost,
InsufficientAccountFunds,
SenderNotEoa,
PriorityGreaterThanMaxFeePerGas,
Expand Down