chore: move tx-too-big check to execute_raw (#1959)

crates/blockifier/src/blockifier/transaction_executor_test.rs

@@ -250,6 +250,7 @@ fn test_bouncing(#[case] initial_bouncer_weights: BouncerWeights, #[case] n_even
     let TestInitData { state, account_address, contract_address, mut nonce_manager } =
         create_test_init_data(&block_context.chain_info, CairoVersion::Cairo1);
 
+    // TODO(Yoni, 15/6/2024): turn on concurrency mode.
     let mut tx_executor =
         TransactionExecutor::new(state, block_context, TransactionExecutorConfig::default());
 

crates/blockifier/src/bouncer.rs

@@ -14,7 +14,9 @@ use crate::fee::gas_usage::get_onchain_data_segment_length;
 use crate::state::cached_state::{StateChangesKeys, StorageEntry};
 use crate::state::state_api::StateReader;
 use crate::transaction::errors::TransactionExecutionError;
-use crate::transaction::objects::{ExecutionResourcesTraits, TransactionResources};
+use crate::transaction::objects::{
+    ExecutionResourcesTraits, TransactionExecutionResult, TransactionResources,
+};
 
 #[cfg(test)]
 #[path = "bouncer_test.rs"]
@@ -227,11 +229,6 @@ impl Bouncer {
             &marginal_state_changes_keys,
         )?;
 
-        // Check if the transaction is too large to fit any block.
-        if !self.bouncer_config.has_room(tx_weights) {
-            Err(TransactionExecutionError::TransactionTooLarge)?
-        }
-
         // Check if the transaction can fit the current block available capacity.
         if !self.bouncer_config.has_room(self.accumulated_weights + tx_weights) {
             log::debug!(
@@ -273,7 +270,7 @@ pub fn get_tx_weights<S: StateReader>(
     n_visited_storage_entries: usize,
     tx_resources: &TransactionResources,
     state_changes_keys: &StateChangesKeys,
-) -> TransactionExecutorResult<BouncerWeights> {
+) -> TransactionExecutionResult<BouncerWeights> {
     let (message_segment_length, gas_usage) =
         tx_resources.starknet_resources.calculate_message_l1_resources();
 
@@ -298,7 +295,7 @@ pub fn get_tx_weights<S: StateReader>(
 pub fn get_casm_hash_calculation_resources<S: StateReader>(
     state_reader: &S,
     executed_class_hashes: &HashSet<ClassHash>,
-) -> TransactionExecutorResult<ExecutionResources> {
+) -> TransactionExecutionResult<ExecutionResources> {
     let mut casm_hash_computation_resources = ExecutionResources::default();
 
     for class_hash in executed_class_hashes {
@@ -326,3 +323,25 @@ pub fn get_particia_update_resources(n_visited_storage_entries: usize) -> Execut
         n_memory_holes: 0,
     }
 }
+
+pub fn verify_tx_weights_in_bounds<S: StateReader>(
+    state_reader: &S,
+    tx_execution_summary: &ExecutionSummary,
+    tx_resources: &TransactionResources,
+    tx_state_changes_keys: &StateChangesKeys,
+    bouncer_config: &BouncerConfig,
+) -> TransactionExecutionResult<()> {
+    let tx_weights = get_tx_weights(
+        state_reader,
+        &tx_execution_summary.executed_class_hashes,
+        tx_execution_summary.visited_storage_entries.len(),
+        tx_resources,
+        tx_state_changes_keys,
+    )?;
+
+    if !bouncer_config.has_room(tx_weights) {
+        return Err(TransactionExecutionError::TransactionTooLarge);
+    }
+
+    Ok(())
+}

crates/blockifier/src/bouncer_test.rs

@@ -10,7 +10,7 @@ use super::BouncerConfig;
 use crate::blockifier::transaction_executor::{
     TransactionExecutorError, TransactionExecutorResult,
 };
-use crate::bouncer::{Bouncer, BouncerWeights, BuiltinCount};
+use crate::bouncer::{verify_tx_weights_in_bounds, Bouncer, BouncerWeights, BuiltinCount};
 use crate::context::BlockContext;
 use crate::execution::call_info::ExecutionSummary;
 use crate::state::cached_state::{StateChangesKeys, TransactionalState};
@@ -258,13 +258,27 @@ fn test_bouncer_try_update(
     };
     let tx_state_changes_keys = transactional_state.get_actual_state_changes().unwrap().into_keys();
 
-    // Try to update the bouncer.
-    let result = bouncer.try_update(
+    // TODO(Yoni, 1/10/2024): simplify this test and move tx-too-large cases out.
+
+    // Check that the transaction is not too large.
+    let mut result = verify_tx_weights_in_bounds(
         &transactional_state,
-        &tx_state_changes_keys,
         &execution_summary,
         &tx_resources,
-    );
+        &tx_state_changes_keys,
+        &bouncer.bouncer_config,
+    )
+    .map_err(TransactionExecutorError::TransactionExecutionError);
+
+    if result.is_ok() {
+        // Try to update the bouncer.
+        result = bouncer.try_update(
+            &transactional_state,
+            &tx_state_changes_keys,
+            &execution_summary,
+            &tx_resources,
+        );
+    }
 
     // TODO(yael 27/3/24): compare the results without using string comparison.
     assert_eq!(format!("{:?}", result), format!("{:?}", expected_result));

crates/blockifier/src/concurrency/worker_logic.rs

@@ -23,7 +23,6 @@ use crate::state::cached_state::{
     ContractClassMapping, StateChanges, StateMaps, TransactionalState,
 };
 use crate::state::state_api::{StateReader, UpdatableState};
-use crate::transaction::errors::TransactionExecutionError;
 use crate::transaction::objects::{TransactionExecutionInfo, TransactionExecutionResult};
 use crate::transaction::transaction_execution::Transaction;
 use crate::transaction::transactions::ExecutableTransaction;
@@ -217,33 +216,6 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> {
             if let Err(error) = bouncer_result {
                 match error {
                     TransactionExecutorError::BlockFull => return false,
-                    TransactionExecutorError::TransactionExecutionError(
-                        TransactionExecutionError::TransactionTooLarge,
-                    ) => {
-                        // TransactionTooLarge error - revise the execution result, delete writes
-                        // and commit.
-                        // TODO(Avi, 20/6/2024): Move TransactionTooLarge inside execute_raw.
-                        let old_execution_output =
-                            execution_output.take().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR);
-                        tx_versioned_state.delete_writes(
-                            &old_execution_output.writes,
-                            &old_execution_output.contract_classes,
-                        );
-
-                        *execution_output = Some(ExecutionTaskOutput {
-                            reads: old_execution_output.reads,
-                            writes: StateMaps::default(),
-                            contract_classes: HashMap::default(),
-                            visited_pcs: HashMap::default(),
-                            result: Err(TransactionExecutionError::TransactionTooLarge),
-                        });
-
-                        // Signal to the scheduler that the execution output has been revised, so
-                        // higher transactions should be re-validated.
-                        self.scheduler.finish_execution_during_commit(tx_index);
-
-                        return true;
-                    }
                     _ => {
                         // TODO(Avi, 01/07/2024): Consider propagating the error.
                         panic!("Bouncer update failed. {error:?}: {error}");

crates/blockifier/src/transaction/transaction_execution.rs

@@ -4,6 +4,7 @@ use cairo_vm::vm::runners::cairo_runner::ExecutionResources;
 use starknet_api::core::{calculate_contract_address, ContractAddress};
 use starknet_api::transaction::{Fee, Transaction as StarknetApiTransaction, TransactionHash};
 
+use crate::bouncer::verify_tx_weights_in_bounds;
 use crate::context::BlockContext;
 use crate::execution::contract_class::ClassInfo;
 use crate::execution::entry_point::EntryPointExecutionContext;
@@ -157,13 +158,30 @@ impl<U: UpdatableState> ExecutableTransaction<U> for Transaction {
         charge_fee: bool,
         validate: bool,
     ) -> TransactionExecutionResult<TransactionExecutionInfo> {
-        match self {
+        // TODO(Yoni, 1/8/2024): consider unimplementing the ExecutableTransaction trait for inner
+        // types, since now running Transaction::execute_raw is not identical to
+        // AccountTransaction::execute_raw.
+        let tx_execution_info = match self {
             Self::AccountTransaction(account_tx) => {
-                account_tx.execute_raw(state, block_context, charge_fee, validate)
+                account_tx.execute_raw(state, block_context, charge_fee, validate)?
             }
             Self::L1HandlerTransaction(tx) => {
-                tx.execute_raw(state, block_context, charge_fee, validate)
+                tx.execute_raw(state, block_context, charge_fee, validate)?
             }
-        }
+        };
+
+        // Check if the transaction is too large to fit any block.
+        // TODO(Yoni, 1/8/2024): consider caching these two.
+        let tx_execution_summary = tx_execution_info.summarize();
+        let tx_state_changes_keys = state.get_actual_state_changes()?.into_keys();
+        verify_tx_weights_in_bounds(
+            state,
+            &tx_execution_summary,
+            &tx_execution_info.actual_resources,
+            &tx_state_changes_keys,
+            &block_context.bouncer_config,
+        )?;
+
+        Ok(tx_execution_info)
     }
 }