From 511d48133f4e0b4368fdbba5e092792871199d33 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 | 8 ++- .../src/blockifier/transaction_executor.rs | 15 +++-- .../blockifier/transaction_executor_test.rs | 25 ++++---- .../src/concurrency/worker_logic.rs | 23 ++++---- .../src/concurrency/worker_logic_test.rs | 58 +++++++++++-------- .../src/test_utils/transfers_generator.rs | 3 +- .../src/stateful_transaction_validator.rs | 2 +- .../src/py_block_executor.rs | 13 +++-- crates/native_blockifier/src/py_validator.rs | 2 +- 10 files changed, 93 insertions(+), 63 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..9ea08fe50f 100644 --- a/crates/blockifier/src/blockifier/stateful_validator_test.rs +++ b/crates/blockifier/src/blockifier/stateful_validator_test.rs @@ -69,7 +69,9 @@ 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 charge_fee = true; + let skip_validation = false; + let result = stateful_validator.perform_validations(tx, skip_validation, charge_fee); assert!(result.is_ok(), "Validation failed: {:?}", result.unwrap_err()); } @@ -91,7 +93,9 @@ fn test_transaction_validator_skip_validate() { }); let mut stateful_validator = StatefulValidator::create(state, block_context); + let charge_fee = true; + let skip_validation = true; // 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, skip_validation, charge_fee); assert_matches!(result, Ok(())); } diff --git a/crates/blockifier/src/blockifier/transaction_executor.rs b/crates/blockifier/src/blockifier/transaction_executor.rs index 14c13ed932..9fb717125f 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)), @@ -134,6 +136,7 @@ impl TransactionExecutor { pub fn execute_chunk( &mut self, _chunk: &[Transaction], + _charge_fee: bool, ) -> Vec> { unimplemented!() } @@ -179,10 +182,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 +205,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 +223,7 @@ impl TransactionExecutor { pub fn execute_chunk( &mut self, chunk: &[Transaction], + charge_fee: bool, ) -> Vec> { use crate::concurrency::utils::AbortIfPanic; @@ -246,7 +251,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..62364d401f 100644 --- a/crates/blockifier/src/blockifier/transaction_executor_test.rs +++ b/crates/blockifier/src/blockifier/transaction_executor_test.rs @@ -52,7 +52,8 @@ 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 charge_fee = true; + let _tx_execution_info = tx_executor.execute(&tx, charge_fee).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!( @@ -264,12 +265,15 @@ 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), - ))) + .execute( + &Transaction::AccountTransaction(emit_n_events_tx( + n_events, + account_address, + contract_address, + nonce_manager.next(account_address), + )), + true, // charge_fee + ) .map_err(|error| panic!("{error:?}: {error}")) .unwrap(); } @@ -305,7 +309,8 @@ fn test_execute_txs_bouncing() { .collect(); // Run. - let results = tx_executor.execute_txs(&txs); + let charge_fee = true; + let results = tx_executor.execute_txs(&txs, charge_fee); // Check execution results. let expected_offset = 3; @@ -333,12 +338,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, charge_fee); 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, charge_fee); 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..dfab65b05a 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,17 @@ 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 }; + let execution_flags = ExecutionFlags { charge_fee, validate: true, concurrency_mode: true }; let execution_result = tx.execute_raw(&mut transactional_state, self.block_context, execution_flags); @@ -191,7 +190,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 +208,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..986e40a563 100644 --- a/crates/blockifier/src/concurrency/worker_logic_test.rs +++ b/crates/blockifier/src/concurrency/worker_logic_test.rs @@ -113,6 +113,8 @@ pub fn test_commit_tx() { let executor = WorkerExecutor::new(versioned_state, &txs, &block_context, Mutex::new(&mut bouncer)); + let charge_fee = true; + // Execute transactions. // Simulate a concurrent run by executing tx1 before tx0. // tx1 should fail execution since its nonce equals 1, and it is being executed before tx0, @@ -125,7 +127,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, charge_fee); 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 +148,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, charge_fee); 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 +222,8 @@ 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); + let charge_fee = true; + executor.execute_tx(tx_index, charge_fee); 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 +240,8 @@ 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); + let charge_fee = true; + executor.commit_tx(tx_index, charge_fee); 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 +335,8 @@ fn test_worker_execute(max_resource_bounds: ResourceBoundsMapping) { // Successful execution. let tx_index = 0; - worker_executor.execute(tx_index); + let charge_fee = true; + worker_executor.execute(tx_index, charge_fee); // Read a write made by the transaction. assert_eq!( safe_versioned_state @@ -392,7 +397,8 @@ fn test_worker_execute(max_resource_bounds: ResourceBoundsMapping) { // Failed execution. let tx_index = 1; - worker_executor.execute(tx_index); + let charge_fee = true; + worker_executor.execute(tx_index, charge_fee); // No write was made by the transaction. assert_eq!( safe_versioned_state.pin_version(tx_index).get_nonce_at(account_address).unwrap(), @@ -411,7 +417,8 @@ fn test_worker_execute(max_resource_bounds: ResourceBoundsMapping) { // Reverted execution. let tx_index = 2; - worker_executor.execute(tx_index); + let charge_fee = true; + worker_executor.execute(tx_index, charge_fee); // Read a write made by the transaction. assert_eq!( safe_versioned_state.pin_version(tx_index).get_nonce_at(account_address).unwrap(), @@ -491,8 +498,9 @@ 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); + let charge_fee = true; + worker_executor.execute(1, charge_fee); + worker_executor.execute(0, charge_fee); // Creates 2 active tasks. worker_executor.scheduler.next_task(); @@ -600,8 +608,9 @@ 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); + let charge_fee = true; + worker_executor.execute(1, charge_fee); + worker_executor.execute(0, charge_fee); 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 +626,8 @@ fn test_deploy_before_declare( assert_eq!(worker_executor.validate(1), Task::ExecutionTask(1)); // Execute transaction 1 again. - worker_executor.execute(1); + let charge_fee = true; + worker_executor.execute(1, charge_fee); let execution_output = worker_executor.execution_outputs[1].lock().unwrap(); assert!(!execution_output.as_ref().unwrap().result.as_ref().unwrap().is_reverted()); @@ -668,7 +678,8 @@ 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(); + let charge_fee = true; + worker_executor.commit_while_possible(charge_fee); // Verify no transaction was committed. assert_eq!(worker_executor.scheduler.get_n_committed_txs(), 0); @@ -681,11 +692,12 @@ 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); + let charge_fee = true; + worker_executor.execute(0, charge_fee); + worker_executor.execute(1, charge_fee); // Commit the first two transactions (only). - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(charge_fee); // Verify the commit index is now 2. assert_eq!(worker_executor.scheduler.get_n_committed_txs(), 2); @@ -696,10 +708,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, charge_fee); // Commit the third (and last) transaction. - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(charge_fee); // Verify the number of committed transactions is 3, the status of the last transaction is // `Committed`, and the next task is `Done`. @@ -708,7 +720,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(charge_fee); assert_eq!(worker_executor.scheduler.get_n_committed_txs(), 3); // Make sure all transactions were executed successfully. @@ -763,13 +775,13 @@ fn test_worker_commit_phase_with_halt() { // `ReadyToExecute` and not `Executing` as expected). assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(0)); assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(1)); - + let charge_fee = true; // Execute both transactions. - worker_executor.execute(0); - worker_executor.execute(1); + worker_executor.execute(0, charge_fee); + worker_executor.execute(1, charge_fee); // Commit both transactions. - worker_executor.commit_while_possible(); + worker_executor.commit_while_possible(charge_fee); // 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..88bbca7ebf 100644 --- a/crates/blockifier/src/test_utils/transfers_generator.rs +++ b/crates/blockifier/src/test_utils/transfers_generator.rs @@ -155,7 +155,8 @@ 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 charge_fee = true; + let results = self.executor.execute_txs(&txs, charge_fee); 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..12a56193fa 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 @@ -194,8 +196,9 @@ impl PyBlockExecutor { .collect(); // Run. - let results = - Python::with_gil(|py| py.allow_threads(|| self.tx_executor().execute_txs(&txs))); + let results = 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(()) }