From 605b4bd52fa1c3d68dd3dfa9a63f1e9d4b57dece Mon Sep 17 00:00:00 2001 From: Yoni <78365039+Yoni-Starkware@users.noreply.github.com> Date: Sun, 9 Jun 2024 18:55:00 +0300 Subject: [PATCH] chore: move tx-too-big check to execute_raw (#1959) --- .../blockifier/transaction_executor_test.rs | 1 + crates/blockifier/src/bouncer.rs | 35 ++++++++++++++----- crates/blockifier/src/bouncer_test.rs | 24 ++++++++++--- .../src/concurrency/worker_logic.rs | 28 --------------- .../src/transaction/transaction_execution.rs | 26 +++++++++++--- 5 files changed, 69 insertions(+), 45 deletions(-) diff --git a/crates/blockifier/src/blockifier/transaction_executor_test.rs b/crates/blockifier/src/blockifier/transaction_executor_test.rs index 5d3697095a..35dc2f021b 100644 --- a/crates/blockifier/src/blockifier/transaction_executor_test.rs +++ b/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()); diff --git a/crates/blockifier/src/bouncer.rs b/crates/blockifier/src/bouncer.rs index 801700fefe..47a4b7ae8e 100644 --- a/crates/blockifier/src/bouncer.rs +++ b/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( n_visited_storage_entries: usize, tx_resources: &TransactionResources, state_changes_keys: &StateChangesKeys, -) -> TransactionExecutorResult { +) -> TransactionExecutionResult { let (message_segment_length, gas_usage) = tx_resources.starknet_resources.calculate_message_l1_resources(); @@ -298,7 +295,7 @@ pub fn get_tx_weights( pub fn get_casm_hash_calculation_resources( state_reader: &S, executed_class_hashes: &HashSet, -) -> TransactionExecutorResult { +) -> TransactionExecutionResult { 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( + 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(()) +} diff --git a/crates/blockifier/src/bouncer_test.rs b/crates/blockifier/src/bouncer_test.rs index 01b8c59673..2b8188447b 100644 --- a/crates/blockifier/src/bouncer_test.rs +++ b/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)); diff --git a/crates/blockifier/src/concurrency/worker_logic.rs b/crates/blockifier/src/concurrency/worker_logic.rs index df1b9fbeb8..c09c3c8949 100644 --- a/crates/blockifier/src/concurrency/worker_logic.rs +++ b/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}"); diff --git a/crates/blockifier/src/transaction/transaction_execution.rs b/crates/blockifier/src/transaction/transaction_execution.rs index 1e4345adb8..605df72c5a 100644 --- a/crates/blockifier/src/transaction/transaction_execution.rs +++ b/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 ExecutableTransaction for Transaction { charge_fee: bool, validate: bool, ) -> TransactionExecutionResult { - 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) } }