From 30144ebd0cf8728561e70f53a7d86bc8da9cf977 Mon Sep 17 00:00:00 2001 From: Aviv Greenburg Date: Sun, 15 Sep 2024 16:11:44 +0300 Subject: [PATCH] chore(blockifier): have charge_fee flag represent enforce_fee return value --- .../src/blockifier/stateful_validator.rs | 2 +- .../src/blockifier/transaction_executor.rs | 6 ++- .../src/concurrency/versioned_state_test.rs | 9 ++-- .../src/concurrency/worker_logic.rs | 9 +++- .../src/execution/stack_trace_test.rs | 4 +- crates/blockifier/src/fee/fee_checks.rs | 2 +- .../src/transaction/account_transaction.rs | 6 ++- .../transaction/account_transactions_test.rs | 54 +------------------ .../blockifier/src/transaction/test_utils.rs | 5 +- .../src/transaction/transactions_test.rs | 32 ++++++----- 10 files changed, 50 insertions(+), 79 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index 9d7a66929d..b72e5f158d 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -95,7 +95,7 @@ impl StatefulValidator { ) -> StatefulValidatorResult<()> { let strict_nonce_check = false; // Run pre-validation in charge fee mode to perform fee and balance related checks. - let charge_fee = true; + let charge_fee = tx.enforce_fee(); tx.perform_pre_validation_stage( self.tx_executor.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR), tx_context, diff --git a/crates/blockifier/src/blockifier/transaction_executor.rs b/crates/blockifier/src/blockifier/transaction_executor.rs index 78de3a7cfc..8616610489 100644 --- a/crates/blockifier/src/blockifier/transaction_executor.rs +++ b/crates/blockifier/src/blockifier/transaction_executor.rs @@ -16,7 +16,7 @@ use crate::state::cached_state::{CachedState, CommitmentStateDiff, Transactional use crate::state::errors::StateError; use crate::state::state_api::{StateReader, StateResult}; use crate::transaction::errors::TransactionExecutionError; -use crate::transaction::objects::TransactionExecutionInfo; +use crate::transaction::objects::{TransactionExecutionInfo, TransactionInfoCreator}; use crate::transaction::transaction_execution::Transaction; use crate::transaction::transactions::{ExecutableTransaction, ExecutionFlags}; @@ -98,9 +98,11 @@ impl TransactionExecutor { let mut transactional_state = TransactionalState::create_transactional( self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR), ); + let tx_charge_fee = tx.create_tx_info().enforce_fee(); + // Executing a single transaction cannot be done in a concurrent mode. let execution_flags = - ExecutionFlags { charge_fee: true, validate: true, concurrency_mode: false }; + ExecutionFlags { charge_fee: tx_charge_fee, validate: true, concurrency_mode: false }; let tx_execution_result = tx.execute_raw(&mut transactional_state, &self.block_context, execution_flags); match tx_execution_result { diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index 2d09ea036d..2599b638b6 100644 --- a/crates/blockifier/src/concurrency/versioned_state_test.rs +++ b/crates/blockifier/src/concurrency/versioned_state_test.rs @@ -52,7 +52,7 @@ use crate::test_utils::dict_state_reader::DictStateReader; use crate::test_utils::initial_test_state::test_state; use crate::test_utils::{CairoVersion, BALANCE, DEFAULT_STRK_L1_GAS_PRICE}; use crate::transaction::account_transaction::AccountTransaction; -use crate::transaction::objects::{HasRelatedFeeType, TransactionInfoCreator}; +use crate::transaction::objects::HasRelatedFeeType; use crate::transaction::test_utils::{l1_resource_bounds, max_l1_resource_bounds}; use crate::transaction::transactions::ExecutableTransaction; @@ -246,7 +246,7 @@ fn test_run_parallel_txs(max_l1_resource_bounds: ValidResourceBounds) { &mut NonceManager::default(), ); let account_tx_1 = AccountTransaction::DeployAccount(deploy_account_tx_1); - let enforce_fee = account_tx_1.create_tx_info().enforce_fee(); + let enforce_fee = account_tx_1.enforce_fee(); let class_hash = grindy_account.get_class_hash(); let ctor_storage_arg = felt!(1_u8); @@ -272,11 +272,12 @@ fn test_run_parallel_txs(max_l1_resource_bounds: ValidResourceBounds) { let block_context_1 = block_context.clone(); let block_context_2 = block_context.clone(); + // Execute transactions thread::scope(|s| { s.spawn(move || { - let result = account_tx_1.execute(&mut state_1, &block_context_1, true, true); - assert_eq!(result.is_err(), enforce_fee); + let result = account_tx_1.execute(&mut state_1, &block_context_1, enforce_fee, true); + assert_eq!(result.is_err(), enforce_fee); // Transaction fails iff we enforced the fee charge (as the acount is not funded). }); s.spawn(move || { account_tx_2.execute(&mut state_2, &block_context_2, true, true).unwrap(); diff --git a/crates/blockifier/src/concurrency/worker_logic.rs b/crates/blockifier/src/concurrency/worker_logic.rs index 425b810985..eb26f2a26e 100644 --- a/crates/blockifier/src/concurrency/worker_logic.rs +++ b/crates/blockifier/src/concurrency/worker_logic.rs @@ -22,7 +22,11 @@ use crate::state::cached_state::{ TransactionalState, }; use crate::state::state_api::{StateReader, UpdatableState}; -use crate::transaction::objects::{TransactionExecutionInfo, TransactionExecutionResult}; +use crate::transaction::objects::{ + TransactionExecutionInfo, + TransactionExecutionResult, + TransactionInfoCreator, +}; use crate::transaction::transaction_execution::Transaction; use crate::transaction::transactions::{ExecutableTransaction, ExecutionFlags}; @@ -128,10 +132,11 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { fn execute_tx(&self, tx_index: TxIndex) { let mut tx_versioned_state = self.state.pin_version(tx_index); let tx = &self.chunk[tx_index]; + let tx_charge_fee = tx.create_tx_info().enforce_fee(); let mut transactional_state = TransactionalState::create_transactional(&mut tx_versioned_state); let execution_flags = - ExecutionFlags { charge_fee: true, validate: true, concurrency_mode: true }; + ExecutionFlags { charge_fee: tx_charge_fee, validate: true, concurrency_mode: true }; let execution_result = tx.execute_raw(&mut transactional_state, self.block_context, execution_flags); diff --git a/crates/blockifier/src/execution/stack_trace_test.rs b/crates/blockifier/src/execution/stack_trace_test.rs index 0b9cca54c2..d49c69e72e 100644 --- a/crates/blockifier/src/execution/stack_trace_test.rs +++ b/crates/blockifier/src/execution/stack_trace_test.rs @@ -591,7 +591,8 @@ An ASSERT_EQ instruction failed: 1 != 0. // Clean pc locations from the trace. let re = Regex::new(r"pc=0:[0-9]+").unwrap(); let cleaned_expected_error = &re.replace_all(&expected_error, "pc=0:*"); - let actual_error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let charge_fee = account_tx.enforce_fee(); + let actual_error = account_tx.execute(state, block_context, charge_fee, true).unwrap_err(); let actual_error_str = actual_error.to_string(); let cleaned_actual_error = &re.replace_all(&actual_error_str, "pc=0:*"); // Compare actual trace to the expected trace (sans pc locations). @@ -617,6 +618,7 @@ fn test_account_ctor_frame_stack_trace( scenario: INVALID, class_hash, max_fee: BALANCE, + resource_bounds: max_l1_resource_bounds(), validate_constructor: true, ..Default::default() }); diff --git a/crates/blockifier/src/fee/fee_checks.rs b/crates/blockifier/src/fee/fee_checks.rs index 38f8ceec23..50a3820b1b 100644 --- a/crates/blockifier/src/fee/fee_checks.rs +++ b/crates/blockifier/src/fee/fee_checks.rs @@ -268,7 +268,7 @@ impl PostExecutionReport { let TransactionReceipt { fee, gas, .. } = tx_receipt; // If fee is not enforced, no need to check post-execution. - if !charge_fee || !tx_context.tx_info.enforce_fee() { + if !charge_fee { return Ok(Self(FeeCheckReport::success_report(*fee))); } diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 9221160775..ac2478413d 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -255,6 +255,10 @@ impl AccountTransaction { } } + pub fn enforce_fee(&self) -> bool { + self.create_tx_info().enforce_fee() + } + fn verify_tx_version(&self, version: TransactionVersion) -> TransactionExecutionResult<()> { let allowed_versions: Vec = match self { // Support `Declare` of version 0 in order to allow bootstrapping of a new system. @@ -292,7 +296,7 @@ impl AccountTransaction { let tx_info = &tx_context.tx_info; Self::handle_nonce(state, tx_info, strict_nonce_check)?; - if charge_fee && tx_info.enforce_fee() { + if charge_fee { self.check_fee_bounds(tx_context)?; verify_can_pay_committed_bounds(state, tx_context)?; diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 333ac89810..b3f95938a9 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -56,7 +56,6 @@ use crate::state::cached_state::{StateChangesCount, TransactionalState}; use crate::state::state_api::{State, StateReader}; use crate::test_utils::contracts::FeatureContract; use crate::test_utils::declare::declare_tx; -use crate::test_utils::deploy_account::deploy_account_tx; use crate::test_utils::initial_test_state::{fund_account, test_state}; use crate::test_utils::{ create_calldata, @@ -65,7 +64,6 @@ use crate::test_utils::{ get_tx_resources, CairoVersion, BALANCE, - DEFAULT_STRK_L1_GAS_PRICE, DEFAULT_STRK_L2_GAS_PRICE, MAX_FEE, }; @@ -164,33 +162,6 @@ fn test_rc96_holes(block_context: BlockContext, max_l1_resource_bounds: ValidRes ); } -#[rstest] -fn test_fee_enforcement( - block_context: BlockContext, - #[values(TransactionVersion::ONE, TransactionVersion::THREE)] version: TransactionVersion, - #[values(true, false)] zero_bounds: bool, -) { - let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo0); - let state = &mut test_state(&block_context.chain_info, BALANCE, &[(account, 1)]); - let deploy_account_tx = deploy_account_tx( - deploy_account_tx_args! { - class_hash: account.get_class_hash(), - max_fee: Fee(u128::from(!zero_bounds)), - resource_bounds: l1_resource_bounds( - u8::from(!zero_bounds).into(), - DEFAULT_STRK_L1_GAS_PRICE.into() - ), - version, - }, - &mut NonceManager::default(), - ); - - let account_tx = AccountTransaction::DeployAccount(deploy_account_tx); - let enforce_fee = account_tx.create_tx_info().enforce_fee(); - let result = account_tx.execute(state, &block_context, true, true); - assert_eq!(result.is_err(), enforce_fee); -} - #[rstest] #[case::positive_case_deprecated_tx(true, true)] #[case::positive_case_new_tx(true, false)] @@ -240,30 +211,6 @@ fn test_assert_actual_fee_in_bounds( } } -#[rstest] -#[case(TransactionVersion::ZERO)] -#[case(TransactionVersion::ONE)] -#[case(TransactionVersion::THREE)] -fn test_enforce_fee_false_works(block_context: BlockContext, #[case] version: TransactionVersion) { - let TestInitData { mut state, account_address, contract_address, mut nonce_manager } = - create_test_init_data(&block_context.chain_info, CairoVersion::Cairo0); - let tx_execution_info = run_invoke_tx( - &mut state, - &block_context, - invoke_tx_args! { - max_fee: Fee(0), - resource_bounds: l1_resource_bounds(0_u8.into(), DEFAULT_STRK_L1_GAS_PRICE.into()), - sender_address: account_address, - calldata: create_trivial_calldata(contract_address), - version, - nonce: nonce_manager.next(account_address), - }, - ) - .unwrap(); - assert!(!tx_execution_info.is_reverted()); - assert_eq!(tx_execution_info.receipt.fee, Fee(0)); -} - // TODO(Dori, 15/9/2023): Convert version variance to attribute macro. #[rstest] fn test_account_flow_test( @@ -688,6 +635,7 @@ fn test_fail_deploy_account( scenario: INVALID, class_hash: faulty_account_feature_contract.get_class_hash(), max_fee: BALANCE, + resource_bounds: max_l1_resource_bounds(), ..Default::default() }); let fee_token_address = chain_info.fee_token_address(&deploy_account_tx.fee_type()); diff --git a/crates/blockifier/src/transaction/test_utils.rs b/crates/blockifier/src/transaction/test_utils.rs index 276217f006..85a8fed075 100644 --- a/crates/blockifier/src/transaction/test_utils.rs +++ b/crates/blockifier/src/transaction/test_utils.rs @@ -323,7 +323,10 @@ pub fn run_invoke_tx( block_context: &BlockContext, invoke_args: InvokeTxArgs, ) -> TransactionExecutionResult { - account_invoke_tx(invoke_args).execute(state, block_context, true, true) + let tx = account_invoke_tx(invoke_args); + let charge_fee = tx.enforce_fee(); + + tx.execute(state, block_context, charge_fee, true) } /// Creates a `ResourceBoundsMapping` with the given `max_amount` and `max_price` for L1 gas limits. diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index e6ab48cc2f..4b76464780 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -120,6 +120,7 @@ use crate::transaction::objects::{ HasRelatedFeeType, TransactionExecutionInfo, TransactionInfo, + TransactionInfoCreator, }; use crate::transaction::test_utils::{ account_invoke_tx, @@ -803,6 +804,7 @@ fn test_state_get_fee_token_balance( create_calldata(fee_token_address, "permissionedMint", &[recipient, mint_low, mint_high]); let account_tx = account_invoke_tx(invoke_tx_args! { max_fee: MAX_FEE, + resource_bounds: max_l1_resource_bounds(), sender_address: account_address, calldata: execute_calldata, version: tx_version, @@ -823,7 +825,9 @@ fn assert_failure_if_resource_bounds_exceed_balance( block_context: &BlockContext, invalid_tx: AccountTransaction, ) { - match block_context.to_tx_context(&invalid_tx).tx_info { + let tx_info = invalid_tx.create_tx_info(); + + match tx_info { TransactionInfo::Deprecated(context) => { assert_matches!( invalid_tx.execute(state, block_context, true, true).unwrap_err(), @@ -1755,7 +1759,10 @@ fn test_validate_accounts_tx( additional_data: None, ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + // We test `__validate__`, and don't care about the cahrge fee flow. + let charge_fee = false; + + let error = account_tx.execute(state, block_context, charge_fee, true).unwrap_err(); check_tx_execution_error_for_invalid_scenario!(cairo_version, error, validate_constructor,); // Try to call another contract (forbidden). @@ -1768,7 +1775,7 @@ fn test_validate_accounts_tx( resource_bounds: ValidResourceBounds::create_for_testing(), ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let error = account_tx.execute(state, block_context, charge_fee, true).unwrap_err(); check_tx_execution_error_for_custom_hint!( &error, "Unauthorized syscall call_contract in execution mode Validate.", @@ -1784,7 +1791,7 @@ fn test_validate_accounts_tx( resource_bounds: ValidResourceBounds::create_for_testing(), ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let error = account_tx.execute(state, block_context, charge_fee, true).unwrap_err(); check_tx_execution_error_for_custom_hint!( &error, "Unauthorized syscall get_block_hash in execution mode Validate.", @@ -1799,7 +1806,7 @@ fn test_validate_accounts_tx( resource_bounds: ValidResourceBounds::create_for_testing(), ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let error = account_tx.execute(state, block_context, charge_fee, true).unwrap_err(); check_tx_execution_error_for_custom_hint!( &error, "Unauthorized syscall get_sequencer_address in execution mode Validate.", @@ -1823,7 +1830,7 @@ fn test_validate_accounts_tx( ..default_args }, ); - let result = account_tx.execute(state, block_context, true, true); + let result = account_tx.execute(state, block_context, charge_fee, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); if tx_type != TransactionType::DeployAccount { @@ -1840,7 +1847,7 @@ fn test_validate_accounts_tx( ..default_args }, ); - let result = account_tx.execute(state, block_context, true, true); + let result = account_tx.execute(state, block_context, charge_fee, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); } @@ -1860,7 +1867,7 @@ fn test_validate_accounts_tx( ..default_args }, ); - let result = account_tx.execute(state, block_context, true, true); + let result = account_tx.execute(state, block_context, charge_fee, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); // Call the syscall get_block_timestamp and assert the returned timestamp was modified @@ -1876,7 +1883,7 @@ fn test_validate_accounts_tx( ..default_args }, ); - let result = account_tx.execute(state, block_context, true, true); + let result = account_tx.execute(state, block_context, charge_fee, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); } @@ -1898,7 +1905,7 @@ fn test_validate_accounts_tx( ..default_args }, ); - let result = account_tx.execute(state, block_context, true, true); + let result = account_tx.execute(state, block_context, charge_fee, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); } } @@ -2042,8 +2049,7 @@ fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) { let key = calldata.0[1]; let value = calldata.0[2]; let payload_size = tx.payload_size(); - - let actual_execution_info = tx.execute(state, block_context, true, true).unwrap(); + let actual_execution_info = tx.execute(state, block_context, false, true).unwrap(); // Do not charge fee as L1Handler's resource bounds (/max fee) is 0. // Build the expected call info. let accessed_storage_key = StorageKey::try_from(key).unwrap(); @@ -2173,7 +2179,7 @@ fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) { // always uptade the storage instad. state.set_storage_at(contract_address, StorageKey::try_from(key).unwrap(), Felt::ZERO).unwrap(); let tx_no_fee = L1HandlerTransaction::create_for_testing(Fee(0), contract_address); - let error = tx_no_fee.execute(state, block_context, true, true).unwrap_err(); + let error = tx_no_fee.execute(state, block_context, false, true).unwrap_err(); // Do not charge fee as L1Handler's resource bounds (/max fee) is 0. // Today, we check that the paid_fee is positive, no matter what was the actual fee. let expected_actual_fee = get_fee_by_gas_vector(&block_context.block_info, total_gas, &FeeType::Eth);