diff --git a/crates/blockifier/src/bouncer.rs b/crates/blockifier/src/bouncer.rs index 5800edac32..485b6b57d1 100644 --- a/crates/blockifier/src/bouncer.rs +++ b/crates/blockifier/src/bouncer.rs @@ -11,6 +11,7 @@ use crate::execution::call_info::ExecutionSummary; use crate::fee::gas_usage::get_onchain_data_segment_length; use crate::state::cached_state::{StateChangesKeys, StorageEntry, TransactionalState}; use crate::state::state_api::StateReader; +use crate::transaction::errors::TransactionExecutionError; use crate::transaction::objects::TransactionResources; #[cfg(test)] @@ -33,8 +34,22 @@ macro_rules! impl_checked_sub { pub type HashMapWrapper = HashMap; +#[derive(Clone, Debug, Default, PartialEq)] +pub struct BouncerConfig { + pub block_max_capacity: BouncerWeights, + pub block_max_capacity_with_keccak: BouncerWeights, +} + #[derive( - Clone, Copy, Debug, Default, derive_more::Add, derive_more::Sub, Deserialize, PartialEq, + Clone, + Copy, + Debug, + Default, + derive_more::Add, + derive_more::AddAssign, + derive_more::Sub, + Deserialize, + PartialEq, )] /// Represents the execution resources counted throughout block creation. pub struct BouncerWeights { @@ -55,10 +70,22 @@ impl BouncerWeights { n_steps, state_diff_size ); + + pub fn has_room(&self, other: Self) -> bool { + self.checked_sub(other).is_some() + } } #[derive( - Clone, Copy, Debug, Default, derive_more::Add, derive_more::Sub, Deserialize, PartialEq, + Clone, + Copy, + Debug, + Default, + derive_more::Add, + derive_more::AddAssign, + derive_more::Sub, + Deserialize, + PartialEq, )] pub struct BuiltinCount { bitwise: usize, @@ -76,16 +103,16 @@ impl BuiltinCount { impl From for BuiltinCount { fn from(mut data: HashMapWrapper) -> Self { + // TODO(yael 24/3/24): replace the unwrap_or_default with expect, once the + // ExecutionResources contains all the builtins. let builtin_count = Self { - bitwise: data.remove(BuiltinName::bitwise.name()).expect("bitwise must be present"), - ecdsa: data.remove(BuiltinName::ecdsa.name()).expect("ecdsa must be present"), - ec_op: data.remove(BuiltinName::ec_op.name()).expect("ec_op must be present"), - keccak: data.remove(BuiltinName::keccak.name()).expect("keccak must be present"), - pedersen: data.remove(BuiltinName::pedersen.name()).expect("pedersen must be present"), - poseidon: data.remove(BuiltinName::poseidon.name()).expect("poseidon must be present"), - range_check: data - .remove(BuiltinName::range_check.name()) - .expect("range_check must be present"), + bitwise: data.remove(BuiltinName::bitwise.name()).unwrap_or_default(), + ecdsa: data.remove(BuiltinName::ecdsa.name()).unwrap_or_default(), + ec_op: data.remove(BuiltinName::ec_op.name()).unwrap_or_default(), + keccak: data.remove(BuiltinName::keccak.name()).unwrap_or_default(), + pedersen: data.remove(BuiltinName::pedersen.name()).unwrap_or_default(), + poseidon: data.remove(BuiltinName::poseidon.name()).unwrap_or_default(), + range_check: data.remove(BuiltinName::range_check.name()).unwrap_or_default(), }; assert!( data.is_empty(), @@ -96,69 +123,86 @@ impl From for BuiltinCount { } } -#[derive(Clone)] +#[derive(Clone, Debug, Default, PartialEq)] pub struct Bouncer { + // Additional info; maintained and used to calculate the residual contribution of a transaction + // to the accumulated weights. pub executed_class_hashes: HashSet, pub visited_storage_entries: HashSet, pub state_changes_keys: StateChangesKeys, - // The capacity is calculated based of the values of the other Bouncer fields. - capacity: BouncerWeights, + + pub bouncer_config: BouncerConfig, + + accumulated_weights: BouncerWeights, } impl Bouncer { - pub fn new(capacity: BouncerWeights) -> Self { - Bouncer { - executed_class_hashes: HashSet::new(), - state_changes_keys: StateChangesKeys::default(), - visited_storage_entries: HashSet::new(), - capacity, - } + pub fn new(bouncer_config: BouncerConfig) -> Self { + Bouncer { bouncer_config, ..Default::default() } } - pub fn create_transactional(self) -> TransactionalBouncer { - TransactionalBouncer::new(self) + fn _update( + &mut self, + tx_weights: BouncerWeights, + tx_execution_summary: &ExecutionSummary, + state_changes_keys: &StateChangesKeys, + ) { + self.accumulated_weights += tx_weights; + self.visited_storage_entries.extend(&tx_execution_summary.visited_storage_entries); + self.executed_class_hashes.extend(&tx_execution_summary.executed_class_hashes); + self.state_changes_keys.extend(state_changes_keys); } - pub fn merge(&mut self, other: Bouncer) { - self.executed_class_hashes.extend(other.executed_class_hashes); - self.state_changes_keys.extend(&other.state_changes_keys); - self.visited_storage_entries.extend(other.visited_storage_entries); - self.capacity = other.capacity; - } -} + /// Updates the bouncer with a new transaction. + pub fn try_update( + &mut self, + state: &mut TransactionalState<'_, S>, + tx_execution_summary: &ExecutionSummary, + tx_resources: &TransactionResources, + ) -> TransactionExecutorResult<()> { + let state_changes_keys = self.get_state_changes_keys(state)?; + let tx_weights = + self.get_tx_weights(state, tx_execution_summary, tx_resources, &state_changes_keys)?; + + let mut max_capacity = self.bouncer_config.block_max_capacity; + if self.accumulated_weights.builtin_count.keccak > 0 || tx_weights.builtin_count.keccak > 0 + { + max_capacity = self.bouncer_config.block_max_capacity_with_keccak; + } -#[derive(Clone)] -pub struct TransactionalBouncer { - // The bouncer can be modified only through the merge method. - bouncer: Bouncer, - // The transactional bouncer can be modified only through the update method. - transactional: Bouncer, -} + // Check if the transaction is too large to fit any block. + if !max_capacity.has_room(tx_weights) { + Err(TransactionExecutionError::TransactionTooLarge)? + } -impl TransactionalBouncer { - pub fn new(parent: Bouncer) -> TransactionalBouncer { - let capacity = parent.capacity; - TransactionalBouncer { bouncer: parent, transactional: Bouncer::new(capacity) } - } + // Check if the transaction can fit the current block available capacity. + if !max_capacity.has_room(self.accumulated_weights + tx_weights) { + Err(TransactionExecutionError::BlockFull)? + } - // TODO update function (in the next PRs) + self._update(tx_weights, tx_execution_summary, &state_changes_keys); + + Ok(()) + } pub fn get_tx_weights( &mut self, state: &mut TransactionalState<'_, S>, + tx_execution_summary: &ExecutionSummary, tx_resources: &TransactionResources, + state_changes_keys: &StateChangesKeys, ) -> TransactionExecutorResult { let (message_segment_length, gas_usage) = tx_resources.starknet_resources.calculate_message_l1_resources(); let mut additional_os_resources = get_casm_hash_calculation_resources( state, - &self.bouncer.executed_class_hashes, - &self.transactional.executed_class_hashes, + &self.executed_class_hashes, + &tx_execution_summary.executed_class_hashes, )?; additional_os_resources += &get_particia_update_resources( - &self.bouncer.visited_storage_entries, - &self.transactional.visited_storage_entries, + &self.visited_storage_entries, + &tx_execution_summary.visited_storage_entries, )?; let vm_resources = &additional_os_resources + &tx_resources.vm_resources; @@ -169,35 +213,15 @@ impl TransactionalBouncer { n_events: tx_resources.starknet_resources.n_events, n_steps: vm_resources.n_steps + vm_resources.n_memory_holes, builtin_count: BuiltinCount::from(vm_resources.builtin_instance_counter.clone()), - state_diff_size: get_onchain_data_segment_length( - &self.transactional.state_changes_keys.count(), - ), + state_diff_size: get_onchain_data_segment_length(&state_changes_keys.count()), }) } - pub fn update_auxiliary_info( - &mut self, - tx_execution_summary: &ExecutionSummary, + pub fn get_state_changes_keys( + &self, state: &mut TransactionalState<'_, S>, - ) -> TransactionExecutorResult<()> { - self.transactional - .executed_class_hashes - .extend(&tx_execution_summary.executed_class_hashes); - self.transactional - .visited_storage_entries - .extend(&tx_execution_summary.visited_storage_entries); + ) -> TransactionExecutorResult { let tx_state_changes_keys = state.get_actual_state_changes()?.into_keys(); - self.transactional.state_changes_keys = - tx_state_changes_keys.difference(&self.bouncer.state_changes_keys); - Ok(()) - } - - pub fn commit(mut self) -> Bouncer { - self.bouncer.merge(self.transactional); - self.bouncer - } - - pub fn abort(self) -> Bouncer { - self.bouncer + Ok(tx_state_changes_keys.difference(&self.state_changes_keys)) } } diff --git a/crates/blockifier/src/bouncer_test.rs b/crates/blockifier/src/bouncer_test.rs index 6cfa40e626..65ef866af6 100644 --- a/crates/blockifier/src/bouncer_test.rs +++ b/crates/blockifier/src/bouncer_test.rs @@ -1,9 +1,24 @@ -use std::ops::Sub; +use std::collections::{HashMap, HashSet}; +use cairo_vm::serde::deserialize_program::BuiltinName; +use rstest::rstest; +use starknet_api::core::{ClassHash, ContractAddress}; +use starknet_api::hash::StarkFelt; +use starknet_api::state::StorageKey; + +use super::BouncerConfig; +use crate::blockifier::transaction_executor::{ + TransactionExecutorError, TransactionExecutorResult, +}; use crate::bouncer::{Bouncer, BouncerWeights, BuiltinCount}; +use crate::context::BlockContext; +use crate::execution::call_info::ExecutionSummary; +use crate::state::cached_state::{CachedState, StateChangesKeys}; +use crate::test_utils::initial_test_state::test_state; +use crate::transaction::errors::TransactionExecutionError; #[test] -fn test_block_weights_sub_checked() { +fn test_block_weights_has_room() { let max_bouncer_weights = BouncerWeights { builtin_count: BuiltinCount { bitwise: 10, @@ -38,9 +53,7 @@ fn test_block_weights_sub_checked() { state_diff_size: 7, }; - let result = max_bouncer_weights.checked_sub(bouncer_weights).unwrap(); - let difference_bouncer_weights = max_bouncer_weights.sub(bouncer_weights); - assert_eq!(result, difference_bouncer_weights); + assert!(max_bouncer_weights.has_room(bouncer_weights)); let bouncer_weights_exceeds_max = BouncerWeights { builtin_count: BuiltinCount { @@ -59,13 +72,22 @@ fn test_block_weights_sub_checked() { state_diff_size: 5, }; - let result = max_bouncer_weights.checked_sub(bouncer_weights_exceeds_max); - assert!(result.is_none()); + assert!(!max_bouncer_weights.has_room(bouncer_weights_exceeds_max)); } -#[test] -fn test_transactional_bouncer() { - let initial_bouncer_weights = BouncerWeights { +#[rstest] +#[case::empty_initial_bouncer(Bouncer::new(BouncerConfig::default()))] +#[case::non_empty_initial_bouncer(Bouncer { + executed_class_hashes: HashSet::from([ClassHash(StarkFelt::from(0_u128))]), + visited_storage_entries: HashSet::from([( + ContractAddress::from(0_u128), + StorageKey::from(0_u128), + )]), + state_changes_keys: StateChangesKeys::create_for_testing(HashSet::from([ + ContractAddress::from(0_u128), + ])), + bouncer_config: BouncerConfig::default(), + accumulated_weights: BouncerWeights { builtin_count: BuiltinCount { bitwise: 10, ecdsa: 10, @@ -80,9 +102,22 @@ fn test_transactional_bouncer() { n_steps: 10, n_events: 10, state_diff_size: 10, + }, +})] +fn test_bouncer_update(#[case] initial_bouncer: Bouncer) { + let execution_summary_to_update = ExecutionSummary { + executed_class_hashes: HashSet::from([ + ClassHash(StarkFelt::from(1_u128)), + ClassHash(StarkFelt::from(2_u128)), + ]), + visited_storage_entries: HashSet::from([ + (ContractAddress::from(1_u128), StorageKey::from(1_u128)), + (ContractAddress::from(2_u128), StorageKey::from(2_u128)), + ]), + ..Default::default() }; - let weights_to_commit = BouncerWeights { + let weights_to_update = BouncerWeights { builtin_count: BuiltinCount { bitwise: 1, ecdsa: 2, @@ -99,15 +134,155 @@ fn test_transactional_bouncer() { state_diff_size: 2, }; - let bouncer = Bouncer::new(initial_bouncer_weights); - let mut transactional_bouncer = bouncer.create_transactional(); - transactional_bouncer.transactional.capacity = weights_to_commit; + let state_changes_keys_to_update = + StateChangesKeys::create_for_testing(HashSet::from([ContractAddress::from(1_u128)])); + + let mut updated_bouncer = initial_bouncer.clone(); + updated_bouncer._update( + weights_to_update, + &execution_summary_to_update, + &state_changes_keys_to_update, + ); + + let mut expected_bouncer = initial_bouncer; + expected_bouncer + .executed_class_hashes + .extend(&execution_summary_to_update.executed_class_hashes); + expected_bouncer + .visited_storage_entries + .extend(&execution_summary_to_update.visited_storage_entries); + expected_bouncer.state_changes_keys.extend(&state_changes_keys_to_update); + expected_bouncer.accumulated_weights += weights_to_update; + + assert_eq!(updated_bouncer, expected_bouncer); +} + +#[rstest] +#[case::positive_flow(0, 1, 0, Ok(()))] +#[case::block_full( + 0, + 11, + 0, + Err(TransactionExecutorError::TransactionExecutionError( + TransactionExecutionError::BlockFull + )) +)] +#[case::transaction_too_large( + 0, + 21, + 0, + Err(TransactionExecutorError::TransactionExecutionError( + TransactionExecutionError::TransactionTooLarge + )) +)] +#[case::positive_flow_with_keccak(0, 0, 1, Ok(()))] +#[case::block_full_with_keccak( + 1, + 0, + 1, + Err(TransactionExecutorError::TransactionExecutionError( + TransactionExecutionError::BlockFull + )) +)] +#[case::transaction_too_large_with_keccak( + 0, + 0, + 2, + Err(TransactionExecutorError::TransactionExecutionError( + TransactionExecutionError::TransactionTooLarge + )) +)] +#[case::block_full_with_keccak_ecdsa_exceeds( + 0, + 11, + 1, + Err(TransactionExecutorError::TransactionExecutionError( + TransactionExecutionError::BlockFull + )) +)] +#[case::transaction_too_large_with_keccak_ecdsa_too_large( + 0, + 21, + 1, + Err(TransactionExecutorError::TransactionExecutionError( + TransactionExecutionError::TransactionTooLarge + )) +)] +fn test_bouncer_try_update( + #[case] initial_keccak: usize, + #[case] added_ecdsa: usize, + #[case] added_keccak: usize, + #[case] expected_result: TransactionExecutorResult<()>, +) { + use cairo_vm::vm::runners::cairo_runner::ExecutionResources; + + use crate::transaction::objects::TransactionResources; + + let state = &mut test_state(&BlockContext::create_for_account_testing().chain_info, 0, &[]); + let mut transactional_state = CachedState::create_transactional(state); + + // Setup the bouncer. + let block_max_capacity = BouncerWeights { + builtin_count: BuiltinCount { + bitwise: 20, + ecdsa: 20, + ec_op: 20, + keccak: 0, + pedersen: 20, + poseidon: 20, + range_check: 20, + }, + gas: 20, + message_segment_length: 20, + n_steps: 20, + n_events: 20, + state_diff_size: 20, + }; + let mut block_max_capacity_with_keccak = block_max_capacity; + block_max_capacity_with_keccak.builtin_count.keccak = 1; + let bouncer_config = BouncerConfig { block_max_capacity, block_max_capacity_with_keccak }; + + let accumulated_weights = BouncerWeights { + builtin_count: BuiltinCount { + bitwise: 10, + ecdsa: 10, + ec_op: 10, + keccak: initial_keccak, + pedersen: 10, + poseidon: 10, + range_check: 10, + }, + gas: 10, + message_segment_length: 10, + n_steps: 10, + n_events: 10, + state_diff_size: 10, + }; + + let mut bouncer = Bouncer { accumulated_weights, bouncer_config, ..Default::default() }; + + // Prepare the resources to be added to the bouncer. + let execution_summary = ExecutionSummary { ..Default::default() }; + let builtin_counter = HashMap::from([ + (BuiltinName::bitwise.name().to_string(), 1), + (BuiltinName::ecdsa.name().to_string(), added_ecdsa), + (BuiltinName::ec_op.name().to_string(), 1), + (BuiltinName::keccak.name().to_string(), added_keccak), + (BuiltinName::pedersen.name().to_string(), 1), + (BuiltinName::poseidon.name().to_string(), 1), + (BuiltinName::range_check.name().to_string(), 1), + ]); + let tx_resources = TransactionResources { + vm_resources: ExecutionResources { + builtin_instance_counter: builtin_counter, + ..Default::default() + }, + ..Default::default() + }; - // Test transactional bouncer abort. - let final_weights = transactional_bouncer.clone().abort(); - assert!(final_weights.capacity == initial_bouncer_weights); + // Try to update the bouncer. + let result = bouncer.try_update(&mut transactional_state, &execution_summary, &tx_resources); - // Test transactional bouncer commit. - let final_weights = transactional_bouncer.commit(); - assert!(final_weights.capacity == weights_to_commit); + // 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/state/cached_state.rs b/crates/blockifier/src/state/cached_state.rs index 85fc4a558b..1edd8ba068 100644 --- a/crates/blockifier/src/state/cached_state.rs +++ b/crates/blockifier/src/state/cached_state.rs @@ -712,6 +712,11 @@ impl StateChangesKeys { n_modified_contracts: self.modified_contracts.len(), } } + + #[cfg(any(feature = "testing", test))] + pub fn create_for_testing(nonce_keys: HashSet) -> Self { + Self { nonce_keys, ..Default::default() } + } } /// Holds the state changes. diff --git a/crates/blockifier/src/transaction/errors.rs b/crates/blockifier/src/transaction/errors.rs index f5a0949813..6014ea588c 100644 --- a/crates/blockifier/src/transaction/errors.rs +++ b/crates/blockifier/src/transaction/errors.rs @@ -51,6 +51,8 @@ pub enum TransactionFeeError { #[derive(Debug, Error)] pub enum TransactionExecutionError { + #[error("Transaction cannot be added to the current block, block capacity reached.")] + BlockFull, #[error( "Declare transaction version {declare_version:?} must have a contract class of Cairo \ version {cairo_version:?}." @@ -89,6 +91,8 @@ pub enum TransactionExecutionError { TransactionPreValidationError(#[from] TransactionPreValidationError), #[error(transparent)] TryFromIntError(#[from] std::num::TryFromIntError), + #[error("Transaction size exceeds the maximum block capacity.")] + TransactionTooLarge, #[error("Transaction validation has failed:\n{}", gen_transaction_execution_error_trace(self))] ValidateTransactionError { error: EntryPointExecutionError,