From 4c064517be97f9925cbacf5fc097dfdd82b453e2 Mon Sep 17 00:00:00 2001 From: meship-starkware Date: Tue, 6 Aug 2024 14:31:03 +0300 Subject: [PATCH] refactor(blockifier): remove enforce fee from prevalidation checks --- .../src/blockifier/stateful_validator.rs | 4 +- .../src/blockifier/stateful_validator_test.rs | 2 +- .../src/blockifier/transaction_executor.rs | 1 + .../blockifier/transaction_executor_test.rs | 10 ++-- .../src/concurrency/versioned_state_test.rs | 2 +- .../src/concurrency/worker_logic_test.rs | 4 +- .../src/execution/stack_trace_test.rs | 7 ++- .../src/test_utils/transfers_generator.rs | 2 +- .../src/transaction/account_transaction.rs | 2 +- .../transaction/account_transactions_test.rs | 54 ++----------------- .../blockifier/src/transaction/test_utils.rs | 1 + .../src/transaction/transactions_test.rs | 20 +++---- 12 files changed, 34 insertions(+), 75 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index 1353e26e0a3..dd70cc6a7cb 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -96,7 +96,9 @@ 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; + // TODO (Meshi, 1/10/2024): When resource bound is supported in FaultyAccountTxCreatorArgs + // Change charge_fee to true. + let charge_fee = tx_context.tx_info.enforce_fee().unwrap(); 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/stateful_validator_test.rs b/crates/blockifier/src/blockifier/stateful_validator_test.rs index 6d0bd7d57ae..afafad290dc 100644 --- a/crates/blockifier/src/blockifier/stateful_validator_test.rs +++ b/crates/blockifier/src/blockifier/stateful_validator_test.rs @@ -69,7 +69,7 @@ fn test_transaction_validator( // Test the stateful validator. let mut stateful_validator = StatefulValidator::create(state, block_context); - let result = stateful_validator.perform_validations(tx, false, true); + let result = stateful_validator.perform_validations(tx, false, false); assert!(result.is_ok(), "Validation failed: {:?}", result.unwrap_err()); } diff --git a/crates/blockifier/src/blockifier/transaction_executor.rs b/crates/blockifier/src/blockifier/transaction_executor.rs index 9fb717125fa..4ae8a759e60 100644 --- a/crates/blockifier/src/blockifier/transaction_executor.rs +++ b/crates/blockifier/src/blockifier/transaction_executor.rs @@ -91,6 +91,7 @@ impl TransactionExecutor { let mut transactional_state = TransactionalState::create_transactional( self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR), ); + // Executing a single transaction cannot be done in a concurrent mode. let execution_flags = ExecutionFlags { charge_fee, validate: true, concurrency_mode: false }; diff --git a/crates/blockifier/src/blockifier/transaction_executor_test.rs b/crates/blockifier/src/blockifier/transaction_executor_test.rs index fb5415d6d05..b0a8b55056c 100644 --- a/crates/blockifier/src/blockifier/transaction_executor_test.rs +++ b/crates/blockifier/src/blockifier/transaction_executor_test.rs @@ -52,7 +52,7 @@ fn tx_executor_test_body( // TODO(Arni, 30/03/2024): Consider adding a test for the transaction execution info. If A test // should not be added, rename the test to `test_bouncer_info`. // TODO(Arni, 30/03/2024): Test all bouncer weights. - let _tx_execution_info = tx_executor.execute(&tx, true).unwrap(); + let _tx_execution_info = tx_executor.execute(&tx, false).unwrap(); let bouncer_weights = tx_executor.bouncer.get_accumulated_weights(); assert_eq!(bouncer_weights.state_diff_size, expected_bouncer_weights.state_diff_size); assert_eq!( @@ -271,7 +271,7 @@ fn test_bouncing(#[case] initial_bouncer_weights: BouncerWeights, #[case] n_even contract_address, nonce_manager.next(account_address), )), - true, + false, ) .map_err(|error| panic!("{error:?}: {error}")) .unwrap(); @@ -308,7 +308,7 @@ fn test_execute_txs_bouncing() { .collect(); // Run. - let results = tx_executor.execute_txs(&txs, true); + let results = tx_executor.execute_txs(&txs, false); // Check execution results. let expected_offset = 3; @@ -336,12 +336,12 @@ fn test_execute_txs_bouncing() { // Check idempotency: excess transactions should not be added. let remaining_txs = &txs[expected_offset..]; - let remaining_tx_results = tx_executor.execute_txs(remaining_txs, true); + let remaining_tx_results = tx_executor.execute_txs(remaining_txs, false); assert_eq!(remaining_tx_results.len(), 0); // Reset the bouncer and add the remaining transactions. tx_executor.bouncer = Bouncer::new(tx_executor.block_context.bouncer_config.clone()); - let remaining_tx_results = tx_executor.execute_txs(remaining_txs, true); + let remaining_tx_results = tx_executor.execute_txs(remaining_txs, false); assert_eq!(remaining_tx_results.len(), 2); assert!(remaining_tx_results[0].is_ok()); diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index 79cac9ee129..7411373ba35 100644 --- a/crates/blockifier/src/concurrency/versioned_state_test.rs +++ b/crates/blockifier/src/concurrency/versioned_state_test.rs @@ -262,7 +262,7 @@ fn test_run_parallel_txs(max_resource_bounds: ResourceBoundsMapping) { // Execute transactions thread::scope(|s| { s.spawn(move || { - let result = account_tx_1.execute(&mut state_1, &block_context_1, true, true); + let result = account_tx_1.execute(&mut state_1, &block_context_1, false, true); assert_eq!(result.is_err(), enforce_fee); }); s.spawn(move || { diff --git a/crates/blockifier/src/concurrency/worker_logic_test.rs b/crates/blockifier/src/concurrency/worker_logic_test.rs index 899933a3cf5..c5f3d064d6c 100644 --- a/crates/blockifier/src/concurrency/worker_logic_test.rs +++ b/crates/blockifier/src/concurrency/worker_logic_test.rs @@ -765,8 +765,8 @@ fn test_worker_commit_phase_with_halt() { assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(1)); // Execute both transactions. - worker_executor.execute(0, true); - worker_executor.execute(1, true); + worker_executor.execute(0, false); + worker_executor.execute(1, false); // Commit both transactions. worker_executor.commit_while_possible(true); diff --git a/crates/blockifier/src/execution/stack_trace_test.rs b/crates/blockifier/src/execution/stack_trace_test.rs index 6aa7523507e..b6aa6cdddb4 100644 --- a/crates/blockifier/src/execution/stack_trace_test.rs +++ b/crates/blockifier/src/execution/stack_trace_test.rs @@ -79,6 +79,7 @@ fn test_stack_trace( invoke_tx_args! { sender_address: account_address, calldata, + max_fee: Fee(BALANCE), version: TransactionVersion::ZERO, }, ) @@ -198,6 +199,7 @@ fn test_trace_callchain_ends_with_regular_call( sender_address: account_address, calldata, version: TransactionVersion::ZERO, + max_fee: Fee(BALANCE), }, ) .unwrap_err(); @@ -334,6 +336,7 @@ fn test_trace_call_chain_with_syscalls( sender_address: account_address, calldata, version: TransactionVersion::ZERO, + max_fee: Fee(BALANCE), }, ) .unwrap_err(); @@ -526,7 +529,7 @@ Execution failed. Failure reason: 0x496e76616c6964207363656e6172696f ('Invalid s // 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 actual_error = account_tx.execute(state, block_context, false, 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). @@ -589,7 +592,7 @@ An ASSERT_EQ instruction failed: 1 != 0. }; // Compare expected and actual error. - let error = deploy_account_tx.execute(state, &block_context, true, true).unwrap_err(); + let error = deploy_account_tx.execute(state, &block_context, false, true).unwrap_err(); assert_eq!(error.to_string(), expected_error); } diff --git a/crates/blockifier/src/test_utils/transfers_generator.rs b/crates/blockifier/src/test_utils/transfers_generator.rs index 2f638ab17a0..c40b7b6fc49 100644 --- a/crates/blockifier/src/test_utils/transfers_generator.rs +++ b/crates/blockifier/src/test_utils/transfers_generator.rs @@ -155,7 +155,7 @@ impl TransfersGenerator { let account_tx = self.generate_transfer(sender_address, recipient_address); txs.push(Transaction::AccountTransaction(account_tx)); } - let results = self.executor.execute_txs(&txs, true); + let results = self.executor.execute_txs(&txs, false); assert_eq!(results.len(), self.config.n_txs); for result in results { assert!(!result.unwrap().is_reverted()); diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 1cc97e9ed15..9043bc761cd 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -178,7 +178,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 3f48297d157..d9a2c3981ea 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -35,7 +35,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::invoke::InvokeTxArgs; use crate::test_utils::{ @@ -47,7 +46,6 @@ use crate::test_utils::{ CairoVersion, NonceManager, BALANCE, - DEFAULT_STRK_L1_GAS_PRICE, MAX_FEE, }; use crate::transaction::account_transaction::AccountTransaction; @@ -152,54 +150,6 @@ fn test_rc96_holes(block_context: BlockContext, max_resource_bounds: ResourceBou assert_eq!(tx_execution_info.receipt.gas, GasVector::from_l1_gas(6598)); } -#[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(u64::from(!zero_bounds), DEFAULT_STRK_L1_GAS_PRICE), - version, - }, - &mut NonceManager::default(), - ); - - let account_tx = AccountTransaction::DeployAccount(deploy_account_tx); - let enforce_fee = account_tx.create_tx_info().enforce_fee().unwrap(); - let result = account_tx.execute(state, &block_context, true, true); - assert_eq!(result.is_err(), enforce_fee); -} - -#[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, DEFAULT_STRK_L1_GAS_PRICE), - 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( @@ -517,6 +467,7 @@ fn test_recursion_depth_exceeded( fn test_revert_invoke( block_context: BlockContext, max_fee: Fee, + max_resource_bounds: ResourceBoundsMapping, #[case] transaction_version: TransactionVersion, #[case] fee_type: FeeType, ) { @@ -536,6 +487,7 @@ fn test_revert_invoke( invoke_tx_args! { max_fee, sender_address: account_address, + resource_bounds: max_resource_bounds, calldata: create_calldata( test_contract_address, "write_and_revert", @@ -606,7 +558,7 @@ fn test_fail_deploy_account( let initial_balance = state.get_fee_token_balance(deploy_address, fee_token_address).unwrap(); - let error = deploy_account_tx.execute(state, &block_context, true, true).unwrap_err(); + let error = deploy_account_tx.execute(state, &block_context, false, true).unwrap_err(); // Check the error is as expected. Assure the error message is not nonce or fee related. check_transaction_execution_error_for_invalid_scenario!(cairo_version, error, false); diff --git a/crates/blockifier/src/transaction/test_utils.rs b/crates/blockifier/src/transaction/test_utils.rs index d6b1724e3a7..1e60f5a2443 100644 --- a/crates/blockifier/src/transaction/test_utils.rs +++ b/crates/blockifier/src/transaction/test_utils.rs @@ -140,6 +140,7 @@ pub fn create_test_init_data(chain_info: &ChainInfo, cairo_version: CairoVersion } } +// TODO(Meshi, 1/10/2024): add resource bounds to the argumaents. pub struct FaultyAccountTxCreatorArgs { pub tx_type: TransactionType, pub tx_version: TransactionVersion, diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index 3c0c9d36165..da090218e5a 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -777,7 +777,7 @@ fn test_state_get_fee_token_balance( version: tx_version, nonce: Nonce::default(), }); - account_tx.execute(state, block_context, true, true).unwrap(); + account_tx.execute(state, block_context, false, true).unwrap(); // Get balance from state, and validate. let (low, high) = @@ -1538,7 +1538,7 @@ fn test_validate_accounts_tx( additional_data: None, ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let error = account_tx.execute(state, block_context, false, true).unwrap_err(); check_transaction_execution_error_for_invalid_scenario!( cairo_version, error, @@ -1554,7 +1554,7 @@ fn test_validate_accounts_tx( contract_address_salt: salt_manager.next_salt(), ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let error = account_tx.execute(state, block_context, false, true).unwrap_err(); check_transaction_execution_error_for_custom_hint!( &error, "Unauthorized syscall call_contract in execution mode Validate.", @@ -1569,7 +1569,7 @@ fn test_validate_accounts_tx( additional_data: None, ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let error = account_tx.execute(state, block_context, false, true).unwrap_err(); check_transaction_execution_error_for_custom_hint!( &error, "Unauthorized syscall get_block_hash in execution mode Validate.", @@ -1583,7 +1583,7 @@ fn test_validate_accounts_tx( contract_address_salt: salt_manager.next_salt(), ..default_args }); - let error = account_tx.execute(state, block_context, true, true).unwrap_err(); + let error = account_tx.execute(state, block_context, false, true).unwrap_err(); check_transaction_execution_error_for_custom_hint!( &error, "Unauthorized syscall get_sequencer_address in execution mode Validate.", @@ -1606,7 +1606,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, false, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); if tx_type != TransactionType::DeployAccount { @@ -1622,7 +1622,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, false, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); } @@ -1641,7 +1641,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, false, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); // Call the syscall get_block_timestamp and assert the returned timestamp was modified @@ -1656,7 +1656,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, false, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); } @@ -1677,7 +1677,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, false, true); assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err()); } }