From 4505d16f7a62126df3ceb8363f4ec315e3b863b9 Mon Sep 17 00:00:00 2001 From: Meshi Peled <141231558+meship-starkware@users.noreply.github.com> Date: Mon, 10 Jun 2024 15:14:47 +0300 Subject: [PATCH] fix(transaction): remove charge fee flag from the transaction executor execute (#1970) --- .../src/blockifier/stateful_validator.rs | 2 +- .../src/blockifier/transaction_executor.rs | 13 ++----- .../blockifier/transaction_executor_test.rs | 37 ++++++++----------- .../src/test_utils/transfers_generator.rs | 3 +- .../src/py_block_executor.rs | 7 +--- 5 files changed, 23 insertions(+), 39 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index a8a5648131..5b7f6696ef 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -97,7 +97,7 @@ impl StatefulValidator { } fn execute(&mut self, tx: AccountTransaction) -> StatefulValidatorResult<()> { - self.tx_executor.execute(&Transaction::AccountTransaction(tx), true)?; + self.tx_executor.execute(&Transaction::AccountTransaction(tx))?; Ok(()) } diff --git a/crates/blockifier/src/blockifier/transaction_executor.rs b/crates/blockifier/src/blockifier/transaction_executor.rs index 5f6589c8aa..f3d493fd29 100644 --- a/crates/blockifier/src/blockifier/transaction_executor.rs +++ b/crates/blockifier/src/blockifier/transaction_executor.rs @@ -82,12 +82,12 @@ impl TransactionExecutor { pub fn execute( &mut self, tx: &Transaction, - charge_fee: bool, ) -> TransactionExecutorResult { let mut transactional_state = TransactionalState::create_transactional( self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR), ); let validate = true; + let charge_fee = true; let tx_execution_result = tx.execute_raw(&mut transactional_state, &self.block_context, charge_fee, validate); @@ -114,11 +114,10 @@ impl TransactionExecutor { pub fn execute_txs_sequentially( &mut self, txs: &[Transaction], - charge_fee: bool, ) -> Vec> { let mut results = Vec::new(); for tx in txs { - match self.execute(tx, charge_fee) { + match self.execute(tx) { Ok(tx_execution_info) => results.push(Ok(tx_execution_info)), Err(TransactionExecutorError::BlockFull) => break, Err(error) => results.push(Err(error)), @@ -131,7 +130,6 @@ impl TransactionExecutor { pub fn execute_chunk( &mut self, _chunk: &[Transaction], - _charge_fee: bool, ) -> Vec> { unimplemented!() } @@ -177,11 +175,10 @@ impl TransactionExecutor { pub fn execute_txs( &mut self, txs: &[Transaction], - charge_fee: bool, ) -> Vec> { if !self.config.concurrency_config.enabled { log::debug!("Executing transactions sequentially."); - self.execute_txs_sequentially(txs, charge_fee) + self.execute_txs_sequentially(txs) } else { log::debug!("Executing transactions concurrently."); let chunk_size = self.config.concurrency_config.chunk_size; @@ -200,7 +197,7 @@ impl TransactionExecutor { ); txs.chunks(chunk_size) .fold_while(Vec::new(), |mut results, chunk| { - let chunk_results = self.execute_chunk(chunk, charge_fee); + let chunk_results = self.execute_chunk(chunk); if chunk_results.len() < chunk.len() { // Block is full. results.extend(chunk_results); @@ -218,8 +215,6 @@ impl TransactionExecutor { pub fn execute_chunk( &mut self, chunk: &[Transaction], - // TODO(barak, 01/08/2024): Make `charge_fee` a parameter of `WorkerExecutor`. - _charge_fee: bool, ) -> Vec> { let block_state = self.block_state.take().expect("The block state should be `Some`."); diff --git a/crates/blockifier/src/blockifier/transaction_executor_test.rs b/crates/blockifier/src/blockifier/transaction_executor_test.rs index 35dc2f021b..34d0e5c5d0 100644 --- a/crates/blockifier/src/blockifier/transaction_executor_test.rs +++ b/crates/blockifier/src/blockifier/transaction_executor_test.rs @@ -34,7 +34,6 @@ fn tx_executor_test_body( state: CachedState, block_context: BlockContext, tx: Transaction, - charge_fee: bool, expected_bouncer_weights: BouncerWeights, ) { let mut tx_executor = @@ -42,7 +41,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, charge_fee).unwrap(); + let _tx_execution_info = tx_executor.execute(&tx).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!( @@ -95,7 +94,6 @@ fn tx_executor_test_body( )] fn test_declare( block_context: BlockContext, - #[values(true, false)] charge_fee: bool, #[values(CairoVersion::Cairo0, CairoVersion::Cairo1)] account_cairo_version: CairoVersion, #[case] transaction_version: TransactionVersion, #[case] cairo_version: CairoVersion, @@ -115,7 +113,7 @@ fn test_declare( }, calculate_class_info_for_testing(declared_contract.get_class()), )); - tx_executor_test_body(state, block_context, tx, charge_fee, expected_bouncer_weights); + tx_executor_test_body(state, block_context, tx, expected_bouncer_weights); } #[rstest] @@ -123,7 +121,6 @@ fn test_deploy_account( block_context: BlockContext, #[values(TransactionVersion::ONE, TransactionVersion::THREE)] version: TransactionVersion, #[values(CairoVersion::Cairo0, CairoVersion::Cairo1)] cairo_version: CairoVersion, - #[values(true, false)] charge_fee: bool, ) { let account_contract = FeatureContract::AccountWithoutValidations(cairo_version); let state = test_state(&block_context.chain_info, BALANCE, &[(account_contract, 0)]); @@ -142,7 +139,7 @@ fn test_deploy_account( n_events: 0, ..Default::default() }; - tx_executor_test_body(state, block_context, tx, charge_fee, expected_bouncer_weights); + tx_executor_test_body(state, block_context, tx, expected_bouncer_weights); } #[rstest] @@ -185,7 +182,6 @@ fn test_deploy_account( )] fn test_invoke( block_context: BlockContext, - #[values(true, false)] charge_fee: bool, #[values(TransactionVersion::ONE, TransactionVersion::THREE)] version: TransactionVersion, #[values(CairoVersion::Cairo0, CairoVersion::Cairo1)] cairo_version: CairoVersion, #[case] entry_point_name: &str, @@ -207,11 +203,11 @@ fn test_invoke( calldata, version, })); - tx_executor_test_body(state, block_context, tx, charge_fee, expected_bouncer_weights); + tx_executor_test_body(state, block_context, tx, expected_bouncer_weights); } #[rstest] -fn test_l1_handler(block_context: BlockContext, #[values(true, false)] charge_fee: bool) { +fn test_l1_handler(block_context: BlockContext) { let test_contract = FeatureContract::TestContract(CairoVersion::Cairo1); let state = test_state(&block_context.chain_info, BALANCE, &[(test_contract, 1)]); @@ -225,7 +221,7 @@ fn test_l1_handler(block_context: BlockContext, #[values(true, false)] charge_fe n_events: 0, ..Default::default() }; - tx_executor_test_body(state, block_context, tx, charge_fee, expected_bouncer_weights); + tx_executor_test_body(state, block_context, tx, expected_bouncer_weights); } #[rstest] @@ -257,15 +253,12 @@ fn test_bouncing(#[case] initial_bouncer_weights: BouncerWeights, #[case] n_even tx_executor.bouncer.set_accumulated_weights(initial_bouncer_weights); tx_executor - .execute( - &Transaction::AccountTransaction(emit_n_events_tx( - n_events, - account_address, - contract_address, - nonce_manager.next(account_address), - )), - true, - ) + .execute(&Transaction::AccountTransaction(emit_n_events_tx( + n_events, + account_address, + contract_address, + nonce_manager.next(account_address), + ))) .map_err(|error| panic!("{error:?}: {error}")) .unwrap(); } @@ -301,7 +294,7 @@ fn test_execute_txs_bouncing() { .collect(); // Run. - let results = tx_executor.execute_txs(&txs, true); + let results = tx_executor.execute_txs(&txs); // Check execution results. let expected_offset = 3; @@ -329,12 +322,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); 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); assert_eq!(remaining_tx_results.len(), 2); assert!(remaining_tx_results[0].is_ok()); diff --git a/crates/blockifier/src/test_utils/transfers_generator.rs b/crates/blockifier/src/test_utils/transfers_generator.rs index 232947a100..7c0d44b5ee 100644 --- a/crates/blockifier/src/test_utils/transfers_generator.rs +++ b/crates/blockifier/src/test_utils/transfers_generator.rs @@ -21,7 +21,6 @@ use crate::transaction::transaction_execution::Transaction; const N_ACCOUNTS: u16 = 10000; const CHUNK_SIZE: usize = 10; const RANDOMIZATION_SEED: u64 = 0; -const CHARGE_FEE: bool = false; const TRANSACTION_VERSION: TransactionVersion = TransactionVersion(StarkFelt::ONE); pub struct TransfersGenerator { @@ -70,7 +69,7 @@ impl TransfersGenerator { let account_tx = self.generate_transfer(sender_address, recipient_address); chunk.push(Transaction::AccountTransaction(account_tx)); } - let results = self.executor.execute_txs(&chunk, CHARGE_FEE); + let results = self.executor.execute_txs(&chunk); assert_eq!(results.len(), CHUNK_SIZE); for result in results { assert!(!result.unwrap().is_reverted()); diff --git a/crates/native_blockifier/src/py_block_executor.rs b/crates/native_blockifier/src/py_block_executor.rs index 9bb26a6064..0142e76130 100644 --- a/crates/native_blockifier/src/py_block_executor.rs +++ b/crates/native_blockifier/src/py_block_executor.rs @@ -194,10 +194,9 @@ impl PyBlockExecutor { tx: &PyAny, optional_py_class_info: Option, ) -> NativeBlockifierResult> { - let charge_fee = true; let tx_type: String = get_py_tx_type(tx).expect(PY_TX_PARSING_ERR).to_string(); let tx: Transaction = py_tx(tx, optional_py_class_info).expect(PY_TX_PARSING_ERR); - let tx_execution_info = self.tx_executor().execute(&tx, charge_fee)?; + let tx_execution_info = self.tx_executor().execute(&tx)?; let typed_tx_execution_info = TypedTransactionExecutionInfo::from_tx_execution_info( &self.tx_executor().block_context, tx_execution_info, @@ -217,8 +216,6 @@ impl PyBlockExecutor { &mut self, txs_with_class_infos: Vec<(&PyAny, Option)>, ) -> Py { - let charge_fee = true; - // Parse Py transactions. let (tx_types, txs): (Vec, Vec) = txs_with_class_infos .into_iter() @@ -231,7 +228,7 @@ impl PyBlockExecutor { .unzip(); // Run. - let results = self.tx_executor().execute_txs(&txs, charge_fee); + let results = self.tx_executor().execute_txs(&txs); // Process results. // TODO(Yoni, 15/5/2024): serialize concurrently.