diff --git a/basic_bootloader/src/bootloader/block_flow/zk/post_tx_op/mod.rs b/basic_bootloader/src/bootloader/block_flow/zk/post_tx_op/mod.rs index 0c581d236..cb01fe37a 100644 --- a/basic_bootloader/src/bootloader/block_flow/zk/post_tx_op/mod.rs +++ b/basic_bootloader/src/bootloader/block_flow/zk/post_tx_op/mod.rs @@ -215,7 +215,7 @@ pub fn read_multichain_root< &MESSAGE_ROOT_ADDRESS, &root_slot, ) - .expect("must read MessageRoot shared tree height") + .expect("must read MessageRoot multichain root") } /// diff --git a/basic_bootloader/src/bootloader/block_flow/zk/tx_loop.rs b/basic_bootloader/src/bootloader/block_flow/zk/tx_loop.rs index 2cb490cf8..f5a28b025 100644 --- a/basic_bootloader/src/bootloader/block_flow/zk/tx_loop.rs +++ b/basic_bootloader/src/bootloader/block_flow/zk/tx_loop.rs @@ -184,7 +184,7 @@ where block_data .transaction_hashes_accumulator .add_tx_hash(&tx_processing_result.tx_hash); - if tx_processing_result.is_l1_tx { + if tx_processing_result.is_priority_tx { block_data .enforced_transaction_hashes_accumulator .add_tx_hash(&tx_processing_result.tx_hash); diff --git a/basic_bootloader/src/bootloader/transaction_flow/mod.rs b/basic_bootloader/src/bootloader/transaction_flow/mod.rs index d11343408..a2508e337 100644 --- a/basic_bootloader/src/bootloader/transaction_flow/mod.rs +++ b/basic_bootloader/src/bootloader/transaction_flow/mod.rs @@ -64,14 +64,14 @@ pub enum ExecutionResult<'a, IOTypes: SystemIOTypesConfig> { } impl<'a, IOTypes: SystemIOTypesConfig> ExecutionResult<'a, IOTypes> { - pub fn reverted(self) -> Self { + pub fn to_reverted(self) -> Self { match self { Self::Success { - output: ExecutionOutput::Call(r), + output: ExecutionOutput::Call(_), } | Self::Success { - output: ExecutionOutput::Create(r, _), - } => Self::Revert { output: r }, + output: ExecutionOutput::Create(_, _), + } => Self::Revert { output: &[] }, a => a, } } diff --git a/basic_bootloader/src/bootloader/transaction_flow/zk/mod.rs b/basic_bootloader/src/bootloader/transaction_flow/zk/mod.rs index e0d1449c8..34c5311a9 100644 --- a/basic_bootloader/src/bootloader/transaction_flow/zk/mod.rs +++ b/basic_bootloader/src/bootloader/transaction_flow/zk/mod.rs @@ -52,7 +52,7 @@ pub struct ZkTransactionFlowOnlyEOA { pub struct ZkTxResult<'a> { pub result: ExecutionResult<'a, EthereumIOTypesConfig>, pub tx_hash: Bytes32, - pub is_l1_tx: bool, + pub is_priority_tx: bool, pub is_upgrade_tx: bool, pub is_service_tx: bool, pub gas_refunded: u64, @@ -136,7 +136,7 @@ impl core::fmt::Debug for TxContextForPreAndPostProcessing .field("native_per_gas", &self.native_per_gas) .field("tx_gas_limit", &self.tx_gas_limit) .field("gas_used", &self.gas_used) - .field("gas_refunded", &self.gas_used) + .field("gas_refunded", &self.gas_refunded) .field("validation_pubdata", &self.validation_pubdata) .field("total_pubdata", &self.total_pubdata) .field("native_used", &self.native_used) @@ -478,10 +478,10 @@ where // go to the operator. Base fees are effectively "burned" (not transferred anywhere). let gas_price_for_operator = if cfg!(feature = "burn_base_fee") { let base_fee = system.get_eip1559_basefee(); - context - .gas_price - .checked_sub(base_fee) - .ok_or(internal_error!("Gas_price - base_fee underflow"))? + // We use saturating arithmetic to allow the caller of this method to + // allow gas_price < base_fee. This can be used, for example, for + // transaction simulation + context.gas_price.saturating_sub(base_fee) } else { context.gas_price }; @@ -564,7 +564,7 @@ where ZkTxResult { result, tx_hash: context.tx_hash, - is_l1_tx: false, + is_priority_tx: false, is_upgrade_tx: false, is_service_tx: transaction.is_service(), gas_used: context.gas_used, @@ -862,10 +862,10 @@ where Some(context.validation_pubdata), )?; if !has_enough { - execution_result = execution_result.reverted(); + execution_result = execution_result.to_reverted(); system_log!(system, "Not enough gas for pubdata after execution\n"); Ok(( - execution_result.reverted(), + execution_result.to_reverted(), CachedPubdataInfo { pubdata_used, to_charge_for_pubdata, diff --git a/basic_bootloader/src/bootloader/transaction_flow/zk/process_l1_transaction.rs b/basic_bootloader/src/bootloader/transaction_flow/zk/process_l1_transaction.rs index 9d08d6419..dae326219 100644 --- a/basic_bootloader/src/bootloader/transaction_flow/zk/process_l1_transaction.rs +++ b/basic_bootloader/src/bootloader/transaction_flow/zk/process_l1_transaction.rs @@ -1,7 +1,7 @@ use crate::bootloader::config::BasicBootloaderExecutionConfig; use crate::bootloader::constants::{ - FREE_L1_TX_NATIVE_PER_GAS, L1_TX_INTRINSIC_NATIVE_COST, L1_TX_INTRINSIC_PUBDATA, - L1_TX_NATIVE_PRICE, + FREE_L1_TX_NATIVE_PER_GAS, L1_TX_INTRINSIC_L2_GAS, L1_TX_INTRINSIC_NATIVE_COST, + L1_TX_INTRINSIC_PUBDATA, L1_TX_NATIVE_PRICE, }; use crate::bootloader::errors::BootloaderInterfaceError; use crate::bootloader::errors::TxError; @@ -90,7 +90,7 @@ where native_per_gas, native_per_pubdata, minimal_gas_used, - } = prepare_and_check_resources::( + } = prepare_and_check_resources::( system, transaction, is_priority_op, @@ -333,7 +333,7 @@ where Ok(ZkTxResult { result, tx_hash, - is_l1_tx: is_priority_op, + is_priority_tx: is_priority_op, is_upgrade_tx: !is_priority_op, is_service_tx: false, gas_used, @@ -365,7 +365,11 @@ struct ResourceAndFeeInfo { /// The approach is to use saturating arithmetic and emit a system /// log if this situation ever happens. /// -fn prepare_and_check_resources<'a, S: EthereumLikeTypes + 'a>( +fn prepare_and_check_resources< + 'a, + S: EthereumLikeTypes + 'a, + Config: BasicBootloaderExecutionConfig, +>( system: &mut System, transaction: &AbiEncodedTransaction, is_priority_op: bool, @@ -382,15 +386,24 @@ where let native_price = L1_TX_NATIVE_PRICE; let native_per_gas = if is_priority_op { if gas_price.is_zero() { - FREE_L1_TX_NATIVE_PER_GAS - } else { - u256_try_to_u64(&gas_price.div_ceil(native_price)) - .unwrap_or_else(|| { - system_log!( - system, - "Native per gas calculation for L1 tx overflows, using saturated arithmetic instead"); + if Config::SIMULATION { + u256_try_to_u64(&system.get_eip1559_basefee().div_ceil(native_price)) + .unwrap_or_else(|| { + system_log!( + system, + "Native per gas calculation for L1 tx overflows, using saturated arithmetic instead"); u64::MAX - }) + }) + } else { + FREE_L1_TX_NATIVE_PER_GAS + } + } else { + u256_try_to_u64(&gas_price.div_ceil(native_price)).unwrap_or_else(|| { + system_log!( + system, + "Native per gas calculation for L1 tx overflows, using saturated arithmetic instead"); + u64::MAX + }) } } else { // Upgrade txs are paid by the protocol, so we use a fixed native per gas @@ -406,9 +419,15 @@ where u64::MAX }); - let native_prepaid_from_gas = native_per_gas.saturating_mul(gas_limit); + let native_prepaid_from_gas = native_per_gas.checked_mul(gas_limit) + .unwrap_or_else(|| { + system_log!( + system, + "Native prepaid from gas calculation for L1 tx overflows, using saturated arithmetic instead"); + u64::MAX + }); - let (calldata_tokens, intrinsic_cost) = + let (calldata_tokens, minimal_gas_used) = compute_calldata_tokens(system, transaction.calldata(), true); // With L1ResourcesPolicy, this returns Result, BootloaderSubsystemError> @@ -422,14 +441,21 @@ where false, // is_deployment transaction.calldata().len() as u64, calldata_tokens, - intrinsic_cost, + L1_TX_INTRINSIC_L2_GAS, L1_TX_INTRINSIC_PUBDATA, L1_TX_INTRINSIC_NATIVE_COST, )?; - // L1 transactions might have a gas limit < intrinsic cost, - // so we pick the min as minimal_gas_used. - let minimal_gas_used = intrinsic_cost.min(gas_limit); + // L1 transactions might have a gas limit < minimal_gas_used. This should be + // prevented by L1 validation, but we log and saturate if it happens. + if gas_limit < minimal_gas_used { + system_log!( + system, + "L1 tx gas limit below intrinsic cost, using saturated arithmetic instead" + ); + } + // Pick the min to keep processing L1 txs even if the L1 validation is wrong. + let minimal_gas_used = minimal_gas_used.min(gas_limit); Ok(ResourceAndFeeInfo { resources, @@ -578,7 +604,7 @@ where check_enough_resources_for_pubdata(system, native_per_pubdata, resources, None)?; let execution_result = if !enough { system_log!(system, "Not enough gas for pubdata after execution\n"); - execution_result.reverted() + execution_result.to_reverted() } else { execution_result }; diff --git a/basic_bootloader/src/bootloader/transaction_flow/zk/validation_impl.rs b/basic_bootloader/src/bootloader/transaction_flow/zk/validation_impl.rs index 7da21beca..0bd09cc8c 100644 --- a/basic_bootloader/src/bootloader/transaction_flow/zk/validation_impl.rs +++ b/basic_bootloader/src/bootloader/transaction_flow/zk/validation_impl.rs @@ -322,18 +322,19 @@ where // Parse blobs, if any // No need to feature gate this part, as blobs() should return an empty list // for non-EIP4844 transactions. + let block_base_fee_per_blob_gas = system.get_blob_base_fee_per_gas(); + + #[cfg(not(feature = "eip-4844"))] + crate::require_internal!( + block_base_fee_per_blob_gas == U256::ONE, + "Blob base fee should be set to 1 if EIP 4844 is disabled", + system + )?; + let blobs = if let Some(blobs_list) = transaction.blobs() { let tx_max_fee_per_blob_gas = transaction.max_fee_per_blob_gas().ok_or(internal_error!( "Tx with blobs must define max_fee_per_blob_gas" ))?; - let block_base_fee_per_blob_gas = system.get_blob_base_fee_per_gas(); - - #[cfg(not(feature = "eip-4844"))] - crate::require_internal!( - block_base_fee_per_blob_gas == U256::ONE, - "Blob base fee should be set to 1 if EIP 4844 is disabled", - system - )?; if &block_base_fee_per_blob_gas > tx_max_fee_per_blob_gas && !Config::SIMULATION { return Err(TxError::Validation( diff --git a/basic_system/src/system_implementation/ethereum_storage_model/storage_model.rs b/basic_system/src/system_implementation/ethereum_storage_model/storage_model.rs index adf10a912..966b872df 100644 --- a/basic_system/src/system_implementation/ethereum_storage_model/storage_model.rs +++ b/basic_system/src/system_implementation/ethereum_storage_model/storage_model.rs @@ -10,7 +10,6 @@ use crate::system_implementation::ethereum_storage_model::caches::full_storage_c use crate::system_implementation::ethereum_storage_model::caches::preimage::BytecodeKeccakPreimagesStorage; use crate::system_implementation::ethereum_storage_model::persist_changes::EthereumStoragePersister; use core::alloc::Allocator; -use ruint::aliases::B160; use storage_models::common_structs::snapshottable_io::SnapshottableIo; use storage_models::common_structs::StorageCacheModel; use storage_models::common_structs::StorageModel; @@ -374,34 +373,6 @@ impl< ); } - type AccountAddress<'a> - = &'a B160 - where - Self: 'a; - type AccountDiff<'a> - = BasicAccountDiff - where - Self: 'a; - - fn get_account_diff<'a>( - &'a self, - _address: Self::AccountAddress<'a>, - ) -> Option> { - None - } - fn accounts_diffs_iterator<'a>( - &'a self, - ) -> impl ExactSizeIterator, Self::AccountDiff<'a>)> + Clone - { - self.account_cache.cache.iter().map(|v| { - let current = v.current().value(); - ( - v.key().as_ref(), - (current.nonce, current.balance, current.bytecode_hash), - ) - }) - } - type StorageKey<'a> = &'a WarmStorageKey where diff --git a/basic_system/src/system_implementation/flat_storage_model/mod.rs b/basic_system/src/system_implementation/flat_storage_model/mod.rs index b0e6a7541..4dd2eb6a2 100644 --- a/basic_system/src/system_implementation/flat_storage_model/mod.rs +++ b/basic_system/src/system_implementation/flat_storage_model/mod.rs @@ -25,7 +25,6 @@ use storage_models::common_structs::StorageCacheModel; use storage_models::common_structs::StorageModel; use zk_ee::system::errors::internal::InternalError; use zk_ee::system::BalanceSubsystemError; -use zk_ee::system::BasicAccountDiff; use zk_ee::system::DeconstructionSubsystemError; use zk_ee::system::NonceSubsystemError; use zk_ee::system::Resources; @@ -439,28 +438,6 @@ impl< .expect("must report preimages"); } - type AccountAddress<'a> - = &'a B160 - where - Self: 'a; - type AccountDiff<'a> - = BasicAccountDiff - where - Self: 'a; - - fn get_account_diff<'a>( - &'a self, - _address: Self::AccountAddress<'a>, - ) -> Option> { - None - } - fn accounts_diffs_iterator<'a>( - &'a self, - ) -> impl ExactSizeIterator, Self::AccountDiff<'a>)> + Clone - { - [].into_iter() - } - type StorageKey<'a> = &'a WarmStorageKey where diff --git a/basic_system/src/system_implementation/system/io_subsystem.rs b/basic_system/src/system_implementation/system/io_subsystem.rs index ddf73e4b0..994e70a01 100644 --- a/basic_system/src/system_implementation/system/io_subsystem.rs +++ b/basic_system/src/system_implementation/system/io_subsystem.rs @@ -769,28 +769,6 @@ impl< self.storage.report_new_preimages(result_keeper); } - type AccountAddress<'a> - = M::AccountAddress<'a> - where - Self: 'a; - type AccountDiff<'a> - = M::AccountDiff<'a> - where - Self: 'a; - - fn get_account_diff<'a>( - &'a self, - address: Self::AccountAddress<'a>, - ) -> Option> { - self.storage.get_account_diff(address) - } - fn accounts_diffs_iterator<'a>( - &'a self, - ) -> impl ExactSizeIterator, Self::AccountDiff<'a>)> + Clone - { - self.storage.accounts_diffs_iterator() - } - type StorageKey<'a> = M::StorageKey<'a> where diff --git a/docs/system_hooks.md b/docs/system_hooks.md index feda46aa2..abd3e0c42 100644 --- a/docs/system_hooks.md +++ b/docs/system_hooks.md @@ -39,7 +39,7 @@ The hook accepts the following ABI-encoded parameters: - `bytes32` - observable bytecode hash Key features: -- Enforces EIP-158 by rejecting bytecode longer than 24576 bytes +- Enforces EIP-170 by rejecting bytecode longer than 24576 bytes - Used exclusively for protocol upgrades approved by governance - Does not publish full bytecode in pubdata to fit within gas/calldata limits - Bytecodes are published separately via Ethereum calldata diff --git a/evm_interpreter/src/interpreter.rs b/evm_interpreter/src/interpreter.rs index bfe86654b..e3ef8f391 100644 --- a/evm_interpreter/src/interpreter.rs +++ b/evm_interpreter/src/interpreter.rs @@ -367,7 +367,7 @@ impl<'ee, S: EthereumLikeTypes> Interpreter<'ee, S> { let deployed_code = return_values.returndata; let mut error_after_constructor = None; if deployed_code.len() > MAX_CODE_SIZE { - // EIP-158: reject code of length > 24576. + // EIP-170: reject code of length > 24576. error_after_constructor = Some(EvmError::CreateContractSizeLimit) } else if !deployed_code.is_empty() && deployed_code[0] == 0xEF { // EIP-3541: reject code starting with 0xEF. diff --git a/forward_system/src/run/query_processors/tx_data.rs b/forward_system/src/run/query_processors/tx_data.rs index ae956c928..a85aaae7a 100644 --- a/forward_system/src/run/query_processors/tx_data.rs +++ b/forward_system/src/run/query_processors/tx_data.rs @@ -28,7 +28,7 @@ pub struct TxDataResponder { /// Note: we use different fields for next_tx and next_tx_format /// so that they don't have to be consumed at the same time. pub next_tx_format: Option, - /// Cached next transaction format, populated after size query + /// Cached next transaction from, populated after size query /// (if present) pub next_tx_from: Option, } diff --git a/forward_system/src/system/system_types/mod.rs b/forward_system/src/system/system_types/mod.rs index e2abf7d92..2c1a59939 100644 --- a/forward_system/src/system/system_types/mod.rs +++ b/forward_system/src/system/system_types/mod.rs @@ -52,7 +52,7 @@ impl SystemTypes for ForwardSystemTypes { Self::Resources, EthereumLikeStorageAccessCostModel, VecStackFactory, - 0, // Stack limit (0 = unlimited) + 0, // VecStackFactory ignores N (node size), so 0 is fine here O, // Oracle implementation FlatTreeWithAccountsUnderHashesStorageModel< Self::Allocator, diff --git a/storage_models/src/common_structs/generic_transient_storage.rs b/storage_models/src/common_structs/generic_transient_storage.rs index 8f74a2240..d5c165739 100644 --- a/storage_models/src/common_structs/generic_transient_storage.rs +++ b/storage_models/src/common_structs/generic_transient_storage.rs @@ -20,7 +20,6 @@ pub struct GenericTransientStorage< cache: HistoryMap, pub(crate) current_tx_number: u32, phantom: PhantomData, - alloc: A, } impl< @@ -36,7 +35,6 @@ impl< cache: HistoryMap::new(allocator.clone()), current_tx_number: 0, phantom: PhantomData, - alloc: allocator.clone(), } } @@ -44,7 +42,7 @@ impl< // Just discard old history // Note: it will reset snapshots counter, old snapshots handlers can't be used anymore // Note: We will reset it redundantly for first tx - self.cache = HistoryMap::new(self.alloc.clone()); + self.cache.clear(); self.current_tx_number += 1; } diff --git a/storage_models/src/common_structs/traits/storage_model.rs b/storage_models/src/common_structs/traits/storage_model.rs index b49f498ea..955c70354 100644 --- a/storage_models/src/common_structs/traits/storage_model.rs +++ b/storage_models/src/common_structs/traits/storage_model.rs @@ -244,24 +244,6 @@ pub trait StorageModel: Sized + SnapshottableIo { /// Reports any new preimages (e.g., bytecode) to the result keeper. fn report_new_preimages(&mut self, result_keeper: &mut impl IOResultKeeper); - type AccountAddress<'a>: 'a + Clone + Copy + PartialEq + Eq + core::fmt::Debug - where - Self: 'a; - type AccountDiff<'a>: 'a + Clone + Copy + PartialEq + Eq + core::fmt::Debug - where - Self: 'a; - - /// Returns the diff for a specific account address, if any changes were made. - fn get_account_diff<'a>( - &'a self, - address: Self::AccountAddress<'a>, - ) -> Option>; - - /// Returns an iterator over all account diffs (address, diff pairs). - fn accounts_diffs_iterator<'a>( - &'a self, - ) -> impl ExactSizeIterator, Self::AccountDiff<'a>)> + Clone; - type StorageKey<'a>: 'a + Clone + Copy + PartialEq + Eq + core::fmt::Debug where Self: 'a; diff --git a/system_hooks/src/call_hooks/contract_deployer.rs b/system_hooks/src/call_hooks/contract_deployer.rs index 6d783d082..61c279e2f 100644 --- a/system_hooks/src/call_hooks/contract_deployer.rs +++ b/system_hooks/src/call_hooks/contract_deployer.rs @@ -115,7 +115,7 @@ where evm_interpreter::charge_native_and_ergs::( resources, HOOK_BASE_NATIVE_COST, - HOOK_BASE_ERGS_COST, + Ergs::empty(), )?; if calldata.len() < 4 { diff --git a/system_hooks/src/call_hooks/mint_base_token.rs b/system_hooks/src/call_hooks/mint_base_token.rs index 07b666dbf..9fe3d7858 100644 --- a/system_hooks/src/call_hooks/mint_base_token.rs +++ b/system_hooks/src/call_hooks/mint_base_token.rs @@ -27,7 +27,7 @@ where ergs_to_pass: _, input: calldata, call_scratch_space: _, - nominal_token_value: _, + nominal_token_value, caller, callee, callers_caller: _, @@ -46,6 +46,8 @@ where } let mut error = false; + // This hook doesn't accept any native token value + error |= nominal_token_value != U256::ZERO; let mut is_static = false; match modifier { CallModifier::Constructor => { @@ -73,7 +75,7 @@ where evm_interpreter::charge_native_and_ergs::( &mut resources, HOOK_BASE_NATIVE_COST, - HOOK_BASE_ERGS_COST, + Ergs(0), // Do not charge EVM gas here, it is already charged in the system contract )?; // Calldata length shouldn't be able to overflow u32, due to gas // limitations. diff --git a/system_hooks/src/call_hooks/set_bytecode_on_address.rs b/system_hooks/src/call_hooks/set_bytecode_on_address.rs index 2b76605a3..47adbffa7 100644 --- a/system_hooks/src/call_hooks/set_bytecode_on_address.rs +++ b/system_hooks/src/call_hooks/set_bytecode_on_address.rs @@ -121,7 +121,7 @@ where evm_interpreter::charge_native_and_ergs::( resources, HOOK_BASE_NATIVE_COST, - HOOK_BASE_ERGS_COST, + Ergs(0), // Do not charge EVM gas here, it is already charged in the system contract )?; if is_static { @@ -163,7 +163,7 @@ where // Although this can be called as a part of protocol upgrade, // we are checking the next invariants, just in case - // EIP-158: reject code of length > 24576. + // EIP-170: reject code of length > 24576. if bytecode_length as usize > MAX_CODE_SIZE { return Ok(Err( "Set bytecode on address failure: called with invalid bytecode(length > 24576)", diff --git a/system_hooks/src/lib.rs b/system_hooks/src/lib.rs index 468ac9a3f..eb2be8a2f 100644 --- a/system_hooks/src/lib.rs +++ b/system_hooks/src/lib.rs @@ -343,8 +343,5 @@ fn make_return_state_from_returndata_region( /// Base cost for calling into a system hook const HOOK_BASE_NATIVE_COST: u64 = 1000; -/// Base ergs cost for calling a system hook (100 gas) -const HOOK_BASE_ERGS_COST: Ergs = Ergs(100 * ERGS_PER_GAS); - /// Ergs cost per byte of bytecode for force deployments. const SET_BYTECODE_DETAILS_EXTRA_ERGS_PER_BYTE: Ergs = Ergs(50 * ERGS_PER_GAS); diff --git a/tests/instances/system_hooks/src/lib.rs b/tests/instances/system_hooks/src/lib.rs index bfb0c525b..8ab934bea 100644 --- a/tests/instances/system_hooks/src/lib.rs +++ b/tests/instances/system_hooks/src/lib.rs @@ -782,8 +782,8 @@ fn test_contract_deployer_gas_charging() { vec![(code_hash, bytecode)], ); - // The hook should charge HOOK_BASE_ERGS_COST (100 gas) + extra for bytecode length - assert_eq!(gas_used, 2950); + // The hook should charge for bytecode length + assert_eq!(gas_used, 2850); } #[test] @@ -931,6 +931,52 @@ fn test_mint_base_token_hook() { ); } +#[test] +fn test_mint_base_token_hook_rejects_non_zero_value() { + let mut tester = TestingFramework::new().with_minted_tokens_to_treasury(); + + let l2_base_token_address = address!("000000000000000000000000000000000000800a"); + let mint_hook_address = address!("0000000000000000000000000000000000007100"); + let mint_amount = alloy::primitives::U256::from(3000000000000000000u64); + let call_value = alloy::primitives::U256::from(1u64); + + let initial_balance = tester + .get_account_properties(&l2_base_token_address) + .balance; + + let calldata = mint_amount.to_be_bytes::<32>().to_vec(); + + let tx = L1TxBuilder::new() + .from(l2_base_token_address) + .to(mint_hook_address) + .input(calldata) + .value(call_value) + .gas_price(0) + .gas_limit(200_000) + .build(); + + let output = tester.execute_block(vec![tx]); + let tx_result = output.tx_results[0] + .as_ref() + .expect("Mint hook call should be processed"); + assert!( + !tx_result.is_success(), + "Mint hook should fail when called with non-zero value" + ); + + let final_balance = tester + .get_account_properties(&l2_base_token_address) + .balance; + + let balance_delta = final_balance + .checked_sub(initial_balance) + .expect("Final balance should not be below initial balance"); + assert!( + balance_delta <= call_value, + "Mint amount should not be credited when value is non-zero" + ); +} + #[test] fn test_event_hooks_empty_topics() { for test_contract_address in [L2_INTEROP_ROOT_STORAGE_ADDRESS, SYSTEM_CONTEXT_ADDRESS] { diff --git a/tests/instances/transactions/src/lib.rs b/tests/instances/transactions/src/lib.rs index 3c4a5740b..1d597a7b5 100644 --- a/tests/instances/transactions/src/lib.rs +++ b/tests/instances/transactions/src/lib.rs @@ -692,6 +692,88 @@ fn test_regression_returndata_empty_3541() { assert!(result0.is_ok_and(|o| o.is_success())); } +#[test] +fn test_returndata_cleared_when_reverted_after_execution() { + let wallet = testing_signer(0); + let from = wallet.address(); + let to = address!("0000000000000000000000000000000000010002"); + let bytecode = hex::decode( + "602a600052600160005560016001556001600255600160035560016004556001600555600160065560016007556001600855600160095560206000f3", + ) + .unwrap(); + + let make_tx = |nonce: u64| { + let tx = TxEip1559 { + chain_id: 37u64, + nonce, + max_fee_per_gas: 1000, + max_priority_fee_per_gas: 1000, + gas_limit: 250_000, + to: TxKind::Call(to), + value: U256::ZERO, + input: Default::default(), + access_list: Default::default(), + }; + ZKsyncTxEnvelope::from_eth_tx(tx, wallet.clone()) + }; + + // First run with regular block context: call succeeds and returns non-empty data. + let mut tester = TestingFramework::new() + .with_evm_contract(to, &bytecode) + .with_balance(from, U256::from(1_000_000_000_000_000_u64)); + let success_output = tester.execute_block(vec![make_tx(0)]); + let success_tx = success_output.tx_results[0] + .as_ref() + .expect("Control transaction should be processed"); + assert!( + success_tx.is_success(), + "Control transaction should succeed, got: {:?}", + success_output.tx_results[0] + ); + match &success_tx.execution_result { + rig::zksync_os_interface::types::ExecutionResult::Success( + rig::zksync_os_interface::types::ExecutionOutput::Call(output), + ) => { + assert!( + !output.is_empty(), + "Control transaction should return non-empty returndata" + ); + } + other => panic!("Unexpected control execution result: {other:?}"), + } + + // Run the same tx with expensive pubdata so post-execution pubdata check forces a revert. + // Regression: such reverts must not keep the successful call returndata. + let expensive_pubdata_context = BlockContext { + eip1559_basefee: U256::from(1000), + native_price: U256::ONE, + pubdata_price: U256::from(700_000u64), + ..Default::default() + }; + let mut tester = TestingFramework::new() + .with_evm_contract(to, &bytecode) + .with_balance(from, U256::from(1_000_000_000_000_000_u64)) + .with_block_context(expensive_pubdata_context); + let reverted_output = tester.execute_block(vec![make_tx(0)]); + let reverted_tx = reverted_output.tx_results[0] + .as_ref() + .expect("Transaction should be processed even if reverted"); + assert!( + !reverted_tx.is_success(), + "Transaction should be reverted by pubdata check, got: {:?}", + reverted_output.tx_results[0] + ); + match &reverted_tx.execution_result { + rig::zksync_os_interface::types::ExecutionResult::Revert(output) => { + assert!( + output.is_empty(), + "Returndata must be cleared when converting success into revert" + ); + } + other => panic!("Expected revert result, got: {other:?}"), + } +} + /// Test that transactions with balance calculation overflow are properly rejected #[test] fn test_balance_overflow_protection() { diff --git a/tests/rig/src/chain.rs b/tests/rig/src/chain.rs index f85cde4d7..985972e81 100644 --- a/tests/rig/src/chain.rs +++ b/tests/rig/src/chain.rs @@ -170,7 +170,7 @@ impl Default for BlockContext { gas_limit: MAX_BLOCK_GAS_LIMIT, pubdata_limit: u64::MAX, mix_hash: U256::ONE, - blob_fee: U256::MAX, + blob_fee: U256::ONE, } } } diff --git a/zk_ee/src/common_structs/history_map/mod.rs b/zk_ee/src/common_structs/history_map/mod.rs index 5800b3891..3db75e3e3 100644 --- a/zk_ee/src/common_structs/history_map/mod.rs +++ b/zk_ee/src/common_structs/history_map/mod.rs @@ -76,6 +76,18 @@ where } } + /// Clears the map while reusing history record allocations. + pub fn clear(&mut self) { + for (_, element) in self.btree.iter_mut() { + self.records_memory_pool + .reuse_memory(element.head, element.initial); + } + self.btree.clear(); + self.state.next_snapshot_id = CacheSnapshotId(1); + self.state.frozen_snapshot_id = CacheSnapshotId(0); + self.state.pending_updated_elements = StackLinkedList::empty(self.state.alloc.clone()); + } + /// Get history of an element by key pub fn get<'s>(&'s self, key: &'s K) -> Option> { self.btree @@ -611,4 +623,74 @@ mod tests { }) .unwrap(); } + + #[test] + fn clear_removes_elements_and_pending_changes() { + let mut map = HistoryMap::::new(Global); + + map.snapshot(); + + // Create one modified entry. + let mut v = map.get_or_insert::<()>(&1, || Ok((1, ()))).unwrap(); + v.update::<_, ()>(|x| { + *x = 2; + Ok(()) + }) + .unwrap(); + + assert_eq!(map.iter().len(), 1); + assert_eq!(map.iter_altered_since_commit().count(), 1); + + // Drop all state. + map.clear(); + + assert!(map.get(&1).is_none()); + assert_eq!(map.iter().len(), 0); + assert_eq!(map.iter_altered_since_commit().count(), 0); + map.apply_to_all_updated_elements::<_, ()>(|_, _, _| { + panic!("Map is expected to be empty after clear") + }) + .unwrap(); + } + + #[test] + fn clear_resets_snapshots() { + let mut map = HistoryMap::::new(Global); + + // Keep a pre-clear snapshot handle. + let pre_clear_snapshot = map.snapshot(); + + let mut v = map.get_or_insert::<()>(&1, || Ok((1, ()))).unwrap(); + v.update::<_, ()>(|x| { + *x = 2; + Ok(()) + }) + .unwrap(); + + map.clear(); + + // Old snapshot ids are no longer valid. + assert!(map.rollback(pre_clear_snapshot).is_err()); + + // Materialize key after clear with initial value. + map.get_or_insert::<()>(&1, || Ok((3, ()))).unwrap(); + + // Take snapshot after clear. + let post_clear_snapshot = map.snapshot(); + assert_eq!(post_clear_snapshot, super::CacheSnapshotId(1)); + + let mut v = map.get_or_insert::<()>(&1, || Ok((5, ()))).unwrap(); + v.update::<_, ()>(|x| { + *x = 4; + Ok(()) + }) + .unwrap(); + + // Rollback restores the post-clear initial value for this key. + map.rollback(post_clear_snapshot).expect("Valid snapshot"); + let restored = map.get(&1).expect("Element must remain after rollback"); + + assert_eq!(*restored.initial(), 3); + assert_eq!(*restored.current(), 3); + } } diff --git a/zk_ee/src/common_structs/system_hooks.rs b/zk_ee/src/common_structs/system_hooks.rs index 72492610c..c5dc316ad 100644 --- a/zk_ee/src/common_structs/system_hooks.rs +++ b/zk_ee/src/common_structs/system_hooks.rs @@ -146,7 +146,7 @@ impl HooksStorage { Ok((Some(res), remaining_memory)) } - /// Intercepts events emitted from low addresses (< 2^16) and executes hooks + /// Intercepts events emitted from low addresses (< 2^32) and executes hooks /// stored under that address. If no hook is stored there, return `Ok(None)`. /// pub fn try_intercept_event( diff --git a/zk_ee/src/oracle/usize_serialization/mod.rs b/zk_ee/src/oracle/usize_serialization/mod.rs index 79ccd14b2..1529a81cf 100644 --- a/zk_ee/src/oracle/usize_serialization/mod.rs +++ b/zk_ee/src/oracle/usize_serialization/mod.rs @@ -330,16 +330,11 @@ impl UsizeDeserializable for B160 { const USIZE_LEN: usize = ::USIZE_LEN; fn from_iter(src: &mut impl ExactSizeIterator) -> Result { - if src.len() < ::USIZE_LEN { - return Err(internal_error!("b160 deserialization failed: too short")); - } - let mut limbs = [0u64; B160::LIMBS]; - for limb in &mut limbs { - *limb = unsafe { ::from_iter(src).unwrap_unchecked() }; + let mut new = MaybeUninit::uninit(); + unsafe { + Self::init_from_iter(&mut new, src)?; + Ok(new.assume_init()) } - let new = B160::from_limbs(limbs); - - Ok(new) } unsafe fn init_from_iter( diff --git a/zk_ee/src/system/io.rs b/zk_ee/src/system/io.rs index 53de36f39..7575b3874 100644 --- a/zk_ee/src/system/io.rs +++ b/zk_ee/src/system/io.rs @@ -569,25 +569,6 @@ pub trait IOTeardown: IOSubsystemExt, ); - type AccountAddress<'a>: 'a + Clone + Copy + PartialEq + Eq + core::fmt::Debug - where - Self: 'a; - - type AccountDiff<'a>: 'a + Clone + Copy + PartialEq + Eq + core::fmt::Debug - where - Self: 'a; - - /// Returns the account diff for a specific address, if any changes occurred. - fn get_account_diff<'a>( - &'a self, - address: Self::AccountAddress<'a>, - ) -> Option>; - - /// Returns an iterator over all account state changes (nonce, balance, code). - fn accounts_diffs_iterator<'a>( - &'a self, - ) -> impl ExactSizeIterator, Self::AccountDiff<'a>)> + Clone; - type StorageKey<'a>: 'a + Clone + Copy + PartialEq + Eq + core::fmt::Debug where Self: 'a; diff --git a/zk_ee/src/utils/aligned_vector.rs b/zk_ee/src/utils/aligned_vector.rs index 3a996cbd6..be7e2f736 100644 --- a/zk_ee/src/utils/aligned_vector.rs +++ b/zk_ee/src/utils/aligned_vector.rs @@ -120,6 +120,11 @@ impl UsizeAlignedByteBox { alloc::boxed::Box::new_uninit_slice_in(buffer_size, allocator); let written_words = init_fn(&mut inner); assert!(written_words <= buffer_size); // we do not want to truncate or realloc, but we will expose only written part below + // Safety: init_fn only guarantees that it initialized `written_words` elements. + // Initialize the remainder to avoid UB in assume_init(). + for dst in inner.iter_mut().skip(written_words) { + dst.write(0); + } let byte_capacity = written_words * USIZE_SIZE; // we only count initialized words for capacity purposes Self { diff --git a/zk_ee/src/utils/integer_utils.rs b/zk_ee/src/utils/integer_utils.rs index 28d6003bc..69f92d6d7 100644 --- a/zk_ee/src/utils/integer_utils.rs +++ b/zk_ee/src/utils/integer_utils.rs @@ -93,7 +93,16 @@ pub fn u256_try_to_usize(src: &U256) -> Option { #[inline(always)] pub fn u256_to_usize_saturated(src: &U256) -> usize { - u256_to_u64_saturated(src) as usize + let value = u256_to_u64_saturated(src); + if cfg!(target_pointer_width = "32") { + if value > u32::MAX as u64 { + u32::MAX as usize + } else { + value as usize + } + } else { + value as usize + } } #[inline(always)]