Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 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
14 changes: 13 additions & 1 deletion crates/vm/backends/levm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ mod tracing;
use super::BlockExecutionResult;
use crate::system_contracts::{
BEACON_ROOTS_ADDRESS, CONSOLIDATION_REQUEST_PREDEPLOY_ADDRESS, HISTORY_STORAGE_ADDRESS,
SYSTEM_ADDRESS, WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS,
SYSTEM_ADDRESS, SYSTEM_CONTRACTS_WITH_CODE, WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS,
};
use crate::{EvmError, ExecutionResult};
use bytes::Bytes;
Expand Down Expand Up @@ -381,6 +381,18 @@ pub fn generic_system_contract_levm(
..Default::default()
};

// This check is not necessary in practice, since contract deployment has succesfully happened in all relevant testnets and mainnet
// However, it's necessary to pass some of the Hive tests related to system contract deployment, which is why we have it
if SYSTEM_CONTRACTS_WITH_CODE
.iter()
.any(|contract| contract.address == contract_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
6 changes: 6 additions & 0 deletions crates/vm/system_contracts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,9 @@ pub fn system_contracts_for_fork(fork: Fork) -> impl Iterator<Item = SystemContr
.into_iter()
.filter(move |system_contract| system_contract.active_since_fork <= fork)
}

pub const SYSTEM_CONTRACTS_WITH_CODE: [SystemContract; 3] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is an accurate name. The contracts that should be here are just 2, the EIP 7002 and EIP 7251 contracts. Idk if we can call them PRAGUE_SYSTEM_CONTRACTS or any name representative of that

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, the beacon root contract is a system contract with code but doesn't belong to the check we are trying to make. Correct me if I'm wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, I didn't know the details of the beacon roots contract. As for the rest, I'll fix it in a sec. I thought the deposits contract should be included too but I guess thinking about it now it needn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DEPOSIT_CONTRACT_ADDRESS,
WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS,
CONSOLIDATION_REQUEST_PREDEPLOY_ADDRESS,
];
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
Loading