From d93d8b5b6df41382b6a052b88a2e5630833855b6 Mon Sep 17 00:00:00 2001 From: meship-starkware Date: Wed, 7 Aug 2024 09:54:18 +0300 Subject: [PATCH] chore(blockifier): Add charge fee flage to execute --- .../src/blockifier/stateful_validator.rs | 7 +-- .../src/blockifier/stateful_validator_test.rs | 4 +- .../src/blockifier/transaction_executor.rs | 14 +++--- .../blockifier/transaction_executor_test.rs | 10 ++--- .../src/concurrency/worker_logic.rs | 22 +++++----- .../src/concurrency/worker_logic_test.rs | 44 +++++++++---------- .../src/test_utils/transfers_generator.rs | 2 +- .../src/stateful_transaction_validator.rs | 2 +- .../src/py_block_executor.rs | 10 +++-- crates/native_blockifier/src/py_validator.rs | 2 +- .../src/simulation_network_receiver.rs | 2 + 11 files changed, 64 insertions(+), 55 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index 3c6a8606dd..1353e26e0a 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -56,12 +56,13 @@ impl StatefulValidator { &mut self, tx: AccountTransaction, skip_validate: bool, + charge_fee: bool, ) -> StatefulValidatorResult<()> { // Deploy account transactions should be fully executed, since the constructor must run // before `__validate_deploy__`. The execution already includes all necessary validations, // so they are skipped here. if let AccountTransaction::DeployAccount(_) = tx { - self.execute(tx)?; + self.execute(tx, charge_fee)?; return Ok(()); } @@ -83,8 +84,8 @@ impl StatefulValidator { Ok(()) } - fn execute(&mut self, tx: AccountTransaction) -> StatefulValidatorResult<()> { - self.tx_executor.execute(&Transaction::AccountTransaction(tx))?; + fn execute(&mut self, tx: AccountTransaction, charge_fee: bool) -> StatefulValidatorResult<()> { + self.tx_executor.execute(&Transaction::AccountTransaction(tx), charge_fee)?; Ok(()) } diff --git a/crates/blockifier/src/blockifier/stateful_validator_test.rs b/crates/blockifier/src/blockifier/stateful_validator_test.rs index dc1fd08cc3..6d0bd7d57a 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); + let result = stateful_validator.perform_validations(tx, false, true); assert!(result.is_ok(), "Validation failed: {:?}", result.unwrap_err()); } @@ -92,6 +92,6 @@ fn test_transaction_validator_skip_validate() { let mut stateful_validator = StatefulValidator::create(state, block_context); // The transaction validations should be skipped and the function should return Ok. - let result = stateful_validator.perform_validations(tx, true); + let result = stateful_validator.perform_validations(tx, true, true); assert_matches!(result, Ok(())); } diff --git a/crates/blockifier/src/blockifier/transaction_executor.rs b/crates/blockifier/src/blockifier/transaction_executor.rs index 14c13ed932..eb2eab331f 100644 --- a/crates/blockifier/src/blockifier/transaction_executor.rs +++ b/crates/blockifier/src/blockifier/transaction_executor.rs @@ -86,13 +86,14 @@ 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), ); // 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, validate: true, concurrency_mode: false }; let tx_execution_result = tx.execute_raw(&mut transactional_state, &self.block_context, execution_flags); match tx_execution_result { @@ -118,10 +119,11 @@ 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) { + match self.execute(tx, charge_fee) { Ok(tx_execution_info) => results.push(Ok(tx_execution_info)), Err(TransactionExecutorError::BlockFull) => break, Err(error) => results.push(Err(error)), @@ -179,10 +181,11 @@ 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) + self.execute_txs_sequentially(txs, charge_fee) } else { log::debug!("Executing transactions concurrently."); let chunk_size = self.config.concurrency_config.chunk_size; @@ -201,7 +204,7 @@ impl TransactionExecutor { ); txs.chunks(chunk_size) .fold_while(Vec::new(), |mut results, chunk| { - let chunk_results = self.execute_chunk(chunk); + let chunk_results = self.execute_chunk(chunk, charge_fee); if chunk_results.len() < chunk.len() { // Block is full. results.extend(chunk_results); @@ -219,6 +222,7 @@ impl TransactionExecutor { pub fn execute_chunk( &mut self, chunk: &[Transaction], + charge_fee: bool, ) -> Vec> { use crate::concurrency::utils::AbortIfPanic; @@ -246,7 +250,7 @@ impl TransactionExecutor { // If a panic is not handled or the handling logic itself panics, then we abort // the program. if let Err(err) = catch_unwind(AssertUnwindSafe(|| { - worker_executor.run(); + worker_executor.run(charge_fee); })) { // If the program panics here, the abort guard will exit the program. // In this case, no panic message will be logged. Add the cargo flag diff --git a/crates/blockifier/src/blockifier/transaction_executor_test.rs b/crates/blockifier/src/blockifier/transaction_executor_test.rs index b3f649d5c4..e87855278e 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).unwrap(); + let _tx_execution_info = tx_executor.execute(&tx, true).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!( @@ -269,7 +269,7 @@ fn test_bouncing(#[case] initial_bouncer_weights: BouncerWeights, #[case] n_even account_address, contract_address, nonce_manager.next(account_address), - ))) + )), true) .map_err(|error| panic!("{error:?}: {error}")) .unwrap(); } @@ -305,7 +305,7 @@ fn test_execute_txs_bouncing() { .collect(); // Run. - let results = tx_executor.execute_txs(&txs); + let results = tx_executor.execute_txs(&txs, true); // Check execution results. let expected_offset = 3; @@ -333,12 +333,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); + let remaining_tx_results = tx_executor.execute_txs(remaining_txs, true); 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); + let remaining_tx_results = tx_executor.execute_txs(remaining_txs, true); assert_eq!(remaining_tx_results.len(), 2); assert!(remaining_tx_results[0].is_ok()); diff --git a/crates/blockifier/src/concurrency/worker_logic.rs b/crates/blockifier/src/concurrency/worker_logic.rs index 3029236f6f..de44ba5b36 100644 --- a/crates/blockifier/src/concurrency/worker_logic.rs +++ b/crates/blockifier/src/concurrency/worker_logic.rs @@ -86,13 +86,13 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { } } - pub fn run(&self) { + pub fn run(&self, charge_fee: bool) { let mut task = Task::AskForTask; loop { - self.commit_while_possible(); + self.commit_while_possible(charge_fee); task = match task { Task::ExecutionTask(tx_index) => { - self.execute(tx_index); + self.execute(tx_index, charge_fee); Task::AskForTask } Task::ValidationTask(tx_index) => self.validate(tx_index), @@ -108,10 +108,10 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { } } - fn commit_while_possible(&self) { + fn commit_while_possible(&self, charge_fee: bool) { if let Some(mut transaction_committer) = self.scheduler.try_enter_commit_phase() { while let Some(tx_index) = transaction_committer.try_commit() { - let commit_succeeded = self.commit_tx(tx_index); + let commit_succeeded = self.commit_tx(tx_index, charge_fee); if !commit_succeeded { transaction_committer.halt_scheduler(); } @@ -119,18 +119,18 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { } } - fn execute(&self, tx_index: TxIndex) { - self.execute_tx(tx_index); + fn execute(&self, tx_index: TxIndex, charge_fee: bool) { + self.execute_tx(tx_index, charge_fee); self.scheduler.finish_execution(tx_index) } - fn execute_tx(&self, tx_index: TxIndex) { + fn execute_tx(&self, tx_index: TxIndex, charge_fee: bool) { let mut tx_versioned_state = self.state.pin_version(tx_index); let tx = &self.chunk[tx_index]; 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, validate: true, concurrency_mode: true }; let execution_result = tx.execute_raw(&mut transactional_state, self.block_context, execution_flags); @@ -191,7 +191,7 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { /// - Else (no room), do not commit. The block should be closed without the transaction. /// * Else (execution failed), commit the transaction without fixing the call info or /// updating the sequencer balance. - fn commit_tx(&self, tx_index: TxIndex) -> bool { + fn commit_tx(&self, tx_index: TxIndex, charge_fee: bool) -> bool { let execution_output = lock_mutex_in_array(&self.execution_outputs, tx_index); let execution_output_ref = execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR); let reads = &execution_output_ref.reads; @@ -209,7 +209,7 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { // Release the execution output lock as it is acquired in execution (avoid dead-lock). drop(execution_output); - self.execute_tx(tx_index); + self.execute_tx(tx_index, charge_fee); self.scheduler.finish_execution_during_commit(tx_index); let execution_output = lock_mutex_in_array(&self.execution_outputs, tx_index); diff --git a/crates/blockifier/src/concurrency/worker_logic_test.rs b/crates/blockifier/src/concurrency/worker_logic_test.rs index ec6b8a6575..899933a3cf 100644 --- a/crates/blockifier/src/concurrency/worker_logic_test.rs +++ b/crates/blockifier/src/concurrency/worker_logic_test.rs @@ -125,7 +125,7 @@ pub fn test_commit_tx() { for &(execute_idx, should_fail_execution) in [(1, true), (0, false), (2, false), (3, true)].iter() { - executor.execute_tx(execute_idx); + executor.execute_tx(execute_idx, true); let execution_task_outputs = lock_mutex_in_array(&executor.execution_outputs, execute_idx); let result = &execution_task_outputs.as_ref().unwrap().result; assert_eq!(result.is_err(), should_fail_execution); @@ -146,7 +146,7 @@ pub fn test_commit_tx() { for &(commit_idx, should_fail_execution) in [(0, false), (1, false), (2, true), (3, true)].iter() { - executor.commit_tx(commit_idx); + executor.commit_tx(commit_idx, true); let execution_task_outputs = lock_mutex_in_array(&executor.execution_outputs, commit_idx); let execution_result = &execution_task_outputs.as_ref().unwrap().result; let expected_sequencer_balance_high = 0_u128; @@ -220,7 +220,7 @@ fn test_commit_tx_when_sender_is_sequencer() { let tx_versioned_state = executor.state.pin_version(tx_index); // Execute and save the execution result. - executor.execute_tx(tx_index); + executor.execute_tx(tx_index, true); let execution_task_outputs = lock_mutex_in_array(&executor.execution_outputs, tx_index); let execution_result = &execution_task_outputs.as_ref().unwrap().result; let fee_transfer_call_info = @@ -237,7 +237,7 @@ fn test_commit_tx_when_sender_is_sequencer() { tx_versioned_state.get_storage_at(fee_token_address, sequencer_balance_key_low).unwrap(); // Commit tx and check that the commit made no changes in the execution result or the state. - executor.commit_tx(tx_index); + executor.commit_tx(tx_index, true); let execution_task_outputs = lock_mutex_in_array(&executor.execution_outputs, tx_index); let commit_result = &execution_task_outputs.as_ref().unwrap().result; let fee_transfer_call_info = @@ -331,7 +331,7 @@ fn test_worker_execute(max_resource_bounds: ResourceBoundsMapping) { // Successful execution. let tx_index = 0; - worker_executor.execute(tx_index); + worker_executor.execute(tx_index, true); // Read a write made by the transaction. assert_eq!( safe_versioned_state @@ -392,7 +392,7 @@ fn test_worker_execute(max_resource_bounds: ResourceBoundsMapping) { // Failed execution. let tx_index = 1; - worker_executor.execute(tx_index); + worker_executor.execute(tx_index, true); // No write was made by the transaction. assert_eq!( safe_versioned_state.pin_version(tx_index).get_nonce_at(account_address).unwrap(), @@ -411,7 +411,7 @@ fn test_worker_execute(max_resource_bounds: ResourceBoundsMapping) { // Reverted execution. let tx_index = 2; - worker_executor.execute(tx_index); + worker_executor.execute(tx_index, true); // Read a write made by the transaction. assert_eq!( safe_versioned_state.pin_version(tx_index).get_nonce_at(account_address).unwrap(), @@ -491,8 +491,8 @@ fn test_worker_validate(max_resource_bounds: ResourceBoundsMapping) { worker_executor.scheduler.next_task(); // Execute transactions in the wrong order, making the first execution invalid. - worker_executor.execute(1); - worker_executor.execute(0); + worker_executor.execute(1, true); + worker_executor.execute(0, true); // Creates 2 active tasks. worker_executor.scheduler.next_task(); @@ -600,8 +600,8 @@ fn test_deploy_before_declare( worker_executor.scheduler.next_task(); // Execute transactions in the wrong order, making the first execution invalid. - worker_executor.execute(1); - worker_executor.execute(0); + worker_executor.execute(1, true); + worker_executor.execute(0, true); let execution_output = worker_executor.execution_outputs[1].lock().unwrap(); let tx_execution_info = execution_output.as_ref().unwrap().result.as_ref().unwrap(); @@ -617,7 +617,7 @@ fn test_deploy_before_declare( assert_eq!(worker_executor.validate(1), Task::ExecutionTask(1)); // Execute transaction 1 again. - worker_executor.execute(1); + worker_executor.execute(1, true); let execution_output = worker_executor.execution_outputs[1].lock().unwrap(); assert!(!execution_output.as_ref().unwrap().result.as_ref().unwrap().is_reverted()); @@ -668,7 +668,7 @@ fn test_worker_commit_phase(max_resource_bounds: ResourceBoundsMapping) { WorkerExecutor::new(safe_versioned_state, &txs, &block_context, Mutex::new(&mut bouncer)); // Try to commit before any transaction is ready. - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(true); // Verify no transaction was committed. assert_eq!(worker_executor.scheduler.get_n_committed_txs(), 0); @@ -681,11 +681,11 @@ fn test_worker_commit_phase(max_resource_bounds: ResourceBoundsMapping) { assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(1)); // Execute the first two transactions. - worker_executor.execute(0); - worker_executor.execute(1); + worker_executor.execute(0, true); + worker_executor.execute(1, true); // Commit the first two transactions (only). - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(true); // Verify the commit index is now 2. assert_eq!(worker_executor.scheduler.get_n_committed_txs(), 2); @@ -696,10 +696,10 @@ fn test_worker_commit_phase(max_resource_bounds: ResourceBoundsMapping) { // Create the final execution task and execute it. assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(2)); - worker_executor.execute(2); + worker_executor.execute(2, true); // Commit the third (and last) transaction. - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(true); // Verify the number of committed transactions is 3, the status of the last transaction is // `Committed`, and the next task is `Done`. @@ -708,7 +708,7 @@ fn test_worker_commit_phase(max_resource_bounds: ResourceBoundsMapping) { assert_eq!(worker_executor.scheduler.next_task(), Task::Done); // Try to commit when all transactions are already committed. - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(true); assert_eq!(worker_executor.scheduler.get_n_committed_txs(), 3); // Make sure all transactions were executed successfully. @@ -765,11 +765,11 @@ fn test_worker_commit_phase_with_halt() { assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(1)); // Execute both transactions. - worker_executor.execute(0); - worker_executor.execute(1); + worker_executor.execute(0, true); + worker_executor.execute(1, true); // Commit both transactions. - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(true); // Verify the scheduler is halted. assert_eq!(worker_executor.scheduler.next_task(), Task::Done); diff --git a/crates/blockifier/src/test_utils/transfers_generator.rs b/crates/blockifier/src/test_utils/transfers_generator.rs index 3d3a6a911d..2f638ab17a 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); + let results = self.executor.execute_txs(&txs, true); assert_eq!(results.len(), self.config.n_txs); for result in results { assert!(!result.unwrap().is_reverted()); diff --git a/crates/gateway/src/stateful_transaction_validator.rs b/crates/gateway/src/stateful_transaction_validator.rs index 6e140f1cf5..4468aa9250 100644 --- a/crates/gateway/src/stateful_transaction_validator.rs +++ b/crates/gateway/src/stateful_transaction_validator.rs @@ -52,7 +52,7 @@ impl StatefulTransactionValidatorTrait for BlockifierStatefulValidator { account_tx: AccountTransaction, skip_validate: bool, ) -> BlockifierStatefulValidatorResult<()> { - self.perform_validations(account_tx, skip_validate) + self.perform_validations(account_tx, skip_validate, true) } fn get_nonce( diff --git a/crates/native_blockifier/src/py_block_executor.rs b/crates/native_blockifier/src/py_block_executor.rs index f1f953d8b9..5a29d72c09 100644 --- a/crates/native_blockifier/src/py_block_executor.rs +++ b/crates/native_blockifier/src/py_block_executor.rs @@ -159,14 +159,15 @@ impl PyBlockExecutor { self.tx_executor = None; } - #[pyo3(signature = (tx, optional_py_class_info))] + #[pyo3(signature = (tx, charge_fee, optional_py_class_info))] pub fn execute( &mut self, tx: &PyAny, + charge_fee: bool, optional_py_class_info: Option, ) -> NativeBlockifierResult> { let tx: Transaction = py_tx(tx, optional_py_class_info).expect(PY_TX_PARSING_ERR); - let tx_execution_info = self.tx_executor().execute(&tx)?; + let tx_execution_info = self.tx_executor().execute(&tx, charge_fee)?; let thin_tx_execution_info = ThinTransactionExecutionInfo::from_tx_execution_info( &self.tx_executor().block_context, tx_execution_info, @@ -180,10 +181,11 @@ impl PyBlockExecutor { /// Executes the given transactions on the Blockifier state. /// Stops if and when there is no more room in the block, and returns the executed transactions' /// results as a PyList of (success (bool), serialized result (bytes)) tuples. - #[pyo3(signature = (txs_with_class_infos))] + #[pyo3(signature = (txs_with_class_infos, charge_fee))] pub fn execute_txs( &mut self, txs_with_class_infos: Vec<(&PyAny, Option)>, + charge_fee: bool, ) -> Py { // Parse Py transactions. let txs: Vec = txs_with_class_infos @@ -195,7 +197,7 @@ impl PyBlockExecutor { // Run. let results = - Python::with_gil(|py| py.allow_threads(|| self.tx_executor().execute_txs(&txs))); + Python::with_gil(|py| py.allow_threads(|| self.tx_executor().execute_txs(&txs, charge_fee))); // Process results. // TODO(Yoni, 15/5/2024): serialize concurrently. diff --git a/crates/native_blockifier/src/py_validator.rs b/crates/native_blockifier/src/py_validator.rs index 8398e48c1a..4031186a10 100644 --- a/crates/native_blockifier/src/py_validator.rs +++ b/crates/native_blockifier/src/py_validator.rs @@ -73,7 +73,7 @@ impl PyValidator { // processed. let skip_validate = self .skip_validate_due_to_unprocessed_deploy_account(&account_tx, deploy_account_tx_hash)?; - self.stateful_validator.perform_validations(account_tx, skip_validate)?; + self.stateful_validator.perform_validations(account_tx, skip_validate, true)?; Ok(()) } diff --git a/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs b/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs index bf784103da..c8e62cf036 100644 --- a/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs +++ b/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs @@ -43,6 +43,8 @@ impl NetworkReceiver where ReceiverT: Stream, ReportSender)>, { + // TODO(Meshi): remove this before merging this pr. + #[allow(dead_code)] pub fn new( receiver: ReceiverT, cache_size: usize,