diff --git a/crates/apollo_batcher/src/block_builder.rs b/crates/apollo_batcher/src/block_builder.rs index 2a253d6110a..b68319316d8 100644 --- a/crates/apollo_batcher/src/block_builder.rs +++ b/crates/apollo_batcher/src/block_builder.rs @@ -29,7 +29,7 @@ use blockifier::blockifier::transaction_executor::{ use blockifier::blockifier_versioned_constants::VersionedConstants; use blockifier::bouncer::{BouncerWeights, CasmHashComputationData}; use blockifier::concurrency::worker_pool::WorkerPool; -use blockifier::context::BlockContext; +use blockifier::context::{parse_blocked_storage_keys, BlockContext}; use blockifier::state::cached_state::{CachedState, CommitmentStateDiff}; use blockifier::state::contract_class_manager::ContractClassManager; use blockifier::state::errors::StateError; @@ -781,6 +781,11 @@ impl BlockBuilderFactory { block_builder_config.chain_info, versioned_constants, block_builder_config.bouncer_config, + ) + .with_blocked_storage_keys( + parse_blocked_storage_keys(&block_builder_config.blocked_storage_keys) + .expect("Blocked storage keys are validated when the config is loaded."), + block_builder_config.blocked_storage_keys_error_message, ); // Block production has no per-call deadline to bound this against; leave it unbounded, as diff --git a/crates/apollo_batcher_config/src/config.rs b/crates/apollo_batcher_config/src/config.rs index 5d5ca7836f5..a3c84073470 100644 --- a/crates/apollo_batcher_config/src/config.rs +++ b/crates/apollo_batcher_config/src/config.rs @@ -25,7 +25,7 @@ use blockifier::blockifier::config::{ }; use blockifier::blockifier_versioned_constants::VersionedConstantsOverrides; use blockifier::bouncer::BouncerConfig; -use blockifier::context::ChainInfo; +use blockifier::context::{parse_blocked_storage_keys, ChainInfo}; use serde::{Deserialize, Serialize}; use starknet_api::block::{BlockHash, BlockNumber}; use url::Url; @@ -35,12 +35,21 @@ pub const DEFAULT_TASKS_CHANNEL_SIZE: usize = 1000; pub const DEFAULT_RESULTS_CHANNEL_SIZE: usize = 1000; /// Configuration for the block builder component of the batcher. -#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq)] +#[derive(Clone, Debug, Default, Serialize, Deserialize, Validate, PartialEq)] pub struct BlockBuilderConfig { pub chain_info: ChainInfo, pub execute_config: WorkerPoolConfig, pub bouncer_config: BouncerConfig, pub versioned_constants_overrides: Option, + #[validate(custom(function = "validate_blocked_storage_keys"))] + pub blocked_storage_keys: String, + pub blocked_storage_keys_error_message: String, +} + +fn validate_blocked_storage_keys(blocked_storage_keys: &str) -> Result<(), ValidationError> { + parse_blocked_storage_keys(blocked_storage_keys).map(|_| ()).map_err(|error| { + ValidationError::new("Invalid blocked_storage_keys").with_message(error.into()) + }) } impl SerializeConfig for BlockBuilderConfig { @@ -52,6 +61,22 @@ impl SerializeConfig for BlockBuilderConfig { &self.versioned_constants_overrides, "versioned_constants_overrides", )); + dump.append(&mut BTreeMap::from([ + ser_param( + "blocked_storage_keys", + &self.blocked_storage_keys, + "Comma-separated list of hexadecimal storage keys, i.e., \"0x1,0x2\", that \ + transactions must not access; a transaction that reads or writes any of them, in \ + any contract, fails.", + ParamPrivacyInput::Public, + ), + ser_param( + "blocked_storage_keys_error_message", + &self.blocked_storage_keys_error_message, + "The error message of a transaction that accessed a blocked storage key.", + ParamPrivacyInput::Public, + ), + ])); dump } } @@ -198,6 +223,7 @@ pub struct BatcherStaticConfig { pub storage: StorageConfig, pub outstream_content_buffer_size: usize, pub input_stream_content_buffer_size: usize, + #[validate(nested)] pub block_builder_config: BlockBuilderConfig, pub pre_confirmed_block_writer_config: PreconfirmedBlockWriterConfig, #[validate(nested)] diff --git a/crates/apollo_batcher_config/src/config_test.rs b/crates/apollo_batcher_config/src/config_test.rs new file mode 100644 index 00000000000..3261bdde36f --- /dev/null +++ b/crates/apollo_batcher_config/src/config_test.rs @@ -0,0 +1,16 @@ +use validator::Validate; + +use crate::config::BatcherStaticConfig; + +fn static_config_with_blocked_storage_keys(blocked_storage_keys: &str) -> BatcherStaticConfig { + let mut static_config = BatcherStaticConfig::default(); + static_config.block_builder_config.blocked_storage_keys = blocked_storage_keys.to_string(); + static_config +} + +#[test] +fn blocked_storage_keys_are_validated() { + assert!(static_config_with_blocked_storage_keys("").validate().is_ok()); + assert!(static_config_with_blocked_storage_keys("0x1, 0x2,").validate().is_ok()); + assert!(static_config_with_blocked_storage_keys("0x1,not_a_key").validate().is_err()); +} diff --git a/crates/apollo_batcher_config/src/lib.rs b/crates/apollo_batcher_config/src/lib.rs index ef68c36943d..3d54ecda72d 100644 --- a/crates/apollo_batcher_config/src/lib.rs +++ b/crates/apollo_batcher_config/src/lib.rs @@ -1 +1,3 @@ pub mod config; +#[cfg(test)] +mod config_test; diff --git a/crates/apollo_node/resources/config_schema.json b/crates/apollo_node/resources/config_schema.json index 2064a691ee9..f63514b660d 100644 --- a/crates/apollo_node/resources/config_schema.json +++ b/crates/apollo_node/resources/config_schema.json @@ -84,6 +84,16 @@ "privacy": "Public", "value": 5000 }, + "batcher_config.static_config.block_builder_config.blocked_storage_keys": { + "description": "Comma-separated list of hexadecimal storage keys, i.e., \"0x1,0x2\", that transactions must not access; a transaction that reads or writes any of them, in any contract, fails.", + "privacy": "Public", + "value": "" + }, + "batcher_config.static_config.block_builder_config.blocked_storage_keys_error_message": { + "description": "The error message of a transaction that accessed a blocked storage key.", + "privacy": "Public", + "value": "" + }, "batcher_config.static_config.block_builder_config.bouncer_config.block_max_capacity.l1_gas": { "description": "An upper bound on the total l1_gas used in a block.", "privacy": "Public", diff --git a/crates/blockifier/src/context.rs b/crates/blockifier/src/context.rs index 154d76b6d17..6bf05062328 100644 --- a/crates/blockifier/src/context.rs +++ b/crates/blockifier/src/context.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use std::sync::{Arc, OnceLock}; use apollo_config::dumping::{prepend_sub_config_name, ser_param, SerializeConfig}; @@ -7,6 +7,7 @@ use serde::{Deserialize, Serialize}; use starknet_api::block::{BlockInfo, BlockNumber, BlockTimestamp, FeeType, GasPriceVector}; use starknet_api::core::{ChainId, ContractAddress, OsChainInfo}; use starknet_api::execution_resources::GasAmount; +use starknet_api::state::StorageKey; use starknet_api::transaction::fields::{ AllResourceBounds, Fee, @@ -128,11 +129,33 @@ pub struct BlockContext { pub(crate) chain_info: ChainInfo, pub(crate) versioned_constants: VersionedConstants, pub bouncer_config: BouncerConfig, + /// Storage keys that transactions must not access, in any contract. A transaction that reads + /// or writes any of them fails with `blocked_storage_keys_error_message`. + pub(crate) blocked_storage_keys: HashSet, + pub(crate) blocked_storage_keys_error_message: String, /// Cached on first access; derived from `chain_info`. Fixed for the lifetime of a block /// context. virtual_os_config_hash: OnceLock, } +/// Parses a comma-separated list of hexadecimal storage keys, e.g. "0x1,0x2". Whitespace around +/// entries is ignored, and empty entries (e.g. a trailing comma) are skipped. +pub fn parse_blocked_storage_keys( + blocked_storage_keys: &str, +) -> Result, String> { + blocked_storage_keys + .split(',') + .map(str::trim) + .filter(|entry| !entry.is_empty()) + .map(|entry| { + let felt = Felt::from_hex(entry) + .map_err(|error| format!("Invalid blocked storage key {entry:?}: {error}"))?; + StorageKey::try_from(felt) + .map_err(|error| format!("Invalid blocked storage key {entry:?}: {error}")) + }) + .collect() +} + impl BlockContext { pub fn new( block_info: BlockInfo, @@ -145,10 +168,22 @@ impl BlockContext { chain_info, versioned_constants, bouncer_config, + blocked_storage_keys: HashSet::new(), + blocked_storage_keys_error_message: String::new(), virtual_os_config_hash: OnceLock::new(), } } + pub fn with_blocked_storage_keys( + mut self, + blocked_storage_keys: HashSet, + blocked_storage_keys_error_message: String, + ) -> Self { + self.blocked_storage_keys = blocked_storage_keys; + self.blocked_storage_keys_error_message = blocked_storage_keys_error_message; + self + } + pub fn block_info(&self) -> &BlockInfo { &self.block_info } @@ -208,6 +243,10 @@ impl BlockContext { #[cfg(any(test, feature = "testing"))] pub fn with_chain_info(self, chain_info: ChainInfo) -> Self { Self::new(self.block_info, chain_info, self.versioned_constants, self.bouncer_config) + .with_blocked_storage_keys( + self.blocked_storage_keys, + self.blocked_storage_keys_error_message, + ) } /// Test util to allow overriding block gas limits. diff --git a/crates/blockifier/src/transaction.rs b/crates/blockifier/src/transaction.rs index 78c157331da..02b0a2dd799 100644 --- a/crates/blockifier/src/transaction.rs +++ b/crates/blockifier/src/transaction.rs @@ -7,4 +7,6 @@ pub mod objects; #[cfg(any(feature = "testing", test))] pub mod test_utils; pub mod transaction_execution; +#[cfg(test)] +pub mod transaction_execution_test; pub mod transactions; diff --git a/crates/blockifier/src/transaction/errors.rs b/crates/blockifier/src/transaction/errors.rs index c5c2e42acaf..b8f620c5d9a 100644 --- a/crates/blockifier/src/transaction/errors.rs +++ b/crates/blockifier/src/transaction/errors.rs @@ -81,6 +81,8 @@ pub enum TransactionExecutionError { ContractClassVersionMismatch { declare_version: TransactionVersion, cairo_version: u64 }, #[error("{}", gen_tx_execution_error_trace(self))] ContractConstructorExecutionFailed(#[from] ConstructorEntryPointExecutionError), + #[error("{message}")] + BlockedStorageKeyAccessed { message: String }, #[error("Class with hash {:#066x} is already declared.", **class_hash)] DeclareTransactionError { class_hash: ClassHash }, #[error("{}", gen_tx_execution_error_trace(self))] diff --git a/crates/blockifier/src/transaction/transaction_execution.rs b/crates/blockifier/src/transaction/transaction_execution.rs index e4fb317424f..d7ef9d775e3 100644 --- a/crates/blockifier/src/transaction/transaction_execution.rs +++ b/crates/blockifier/src/transaction/transaction_execution.rs @@ -17,12 +17,14 @@ use starknet_api::transaction::{ use crate::bouncer::verify_tx_weights_within_max_capacity; use crate::context::BlockContext; +use crate::execution::call_info::CallInfo; use crate::state::cached_state::TransactionalState; use crate::state::state_api::UpdatableState; use crate::transaction::account_transaction::{ AccountTransaction, ExecutionFlags as AccountExecutionFlags, }; +use crate::transaction::errors::TransactionExecutionError; use crate::transaction::objects::{ TransactionExecutionInfo, TransactionExecutionResult, @@ -154,6 +156,8 @@ impl ExecutableTransaction for Transaction { Self::L1Handler(tx) => tx.execute_raw(state, block_context, concurrency_mode)?, }; + verify_no_blocked_storage_key_accessed(&tx_execution_info, block_context)?; + // 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(&block_context.versioned_constants); @@ -178,3 +182,26 @@ impl ExecutableTransaction for Transaction { Ok(tx_execution_info) } } + +fn verify_no_blocked_storage_key_accessed( + tx_execution_info: &TransactionExecutionInfo, + block_context: &BlockContext, +) -> TransactionExecutionResult<()> { + if block_context.blocked_storage_keys.is_empty() { + return Ok(()); + } + // `CallInfo::iter` walks the whole call tree, so inner calls are covered. + for call_info in tx_execution_info.non_optional_call_infos().flat_map(CallInfo::iter) { + if call_info + .storage_access_tracker + .accessed_storage_keys + .iter() + .any(|storage_key| block_context.blocked_storage_keys.contains(storage_key)) + { + return Err(TransactionExecutionError::BlockedStorageKeyAccessed { + message: block_context.blocked_storage_keys_error_message.clone(), + }); + } + } + Ok(()) +} diff --git a/crates/blockifier/src/transaction/transaction_execution_test.rs b/crates/blockifier/src/transaction/transaction_execution_test.rs new file mode 100644 index 00000000000..320cd5f3c70 --- /dev/null +++ b/crates/blockifier/src/transaction/transaction_execution_test.rs @@ -0,0 +1,145 @@ +use std::collections::HashSet; + +use assert_matches::assert_matches; +use blockifier_test_utils::cairo_versions::{CairoVersion, RunnableCairo1}; +use blockifier_test_utils::calldata::create_calldata; +use rstest::rstest; +use starknet_api::abi::abi_utils::selector_from_name; +use starknet_api::test_utils::invoke::executable_invoke_tx; +use starknet_api::transaction::fields::Calldata; +use starknet_api::{felt, invoke_tx_args, storage_key}; +use starknet_types_core::felt::Felt; + +use crate::context::{parse_blocked_storage_keys, BlockContext}; +use crate::transaction::account_transaction::AccountTransaction; +use crate::transaction::errors::TransactionExecutionError; +use crate::transaction::objects::{TransactionExecutionInfo, TransactionExecutionResult}; +use crate::transaction::test_utils::{ + create_test_init_data, + default_all_resource_bounds, + TestInitData, +}; +use crate::transaction::transaction_execution::Transaction; +use crate::transaction::transactions::ExecutableTransaction; + +const BLOCKED_STORAGE_KEY_ERROR_MESSAGE: &str = "Transaction accessed a blocked storage key."; + +/// Executes an invoke tx that writes to (and then reads) `storage_key` in the test contract, with +/// `blocked_storage_keys` configured on the block context. With `nested`, the write happens in a +/// contract call made by the test contract to itself, one level deeper in the call tree. +fn execute_storage_write_tx( + blocked_storage_keys: &str, + storage_key: Felt, + nested: bool, +) -> TransactionExecutionResult { + let block_context = BlockContext::create_for_account_testing().with_blocked_storage_keys( + parse_blocked_storage_keys(blocked_storage_keys).unwrap(), + BLOCKED_STORAGE_KEY_ERROR_MESSAGE.to_string(), + ); + let TestInitData { mut state, account_address, contract_address, mut nonce_manager } = + create_test_init_data( + &block_context.chain_info, + CairoVersion::Cairo1(RunnableCairo1::Casm), + ); + let storage_write_args = [storage_key, felt!(7_u8)]; + let calldata: Calldata = if nested { + create_calldata( + contract_address, + "test_call_contract", + &[ + vec![ + *contract_address.0.key(), + selector_from_name("test_storage_read_write").0, + felt!(2_u8), + ], + storage_write_args.to_vec(), + ] + .concat(), + ) + } else { + create_calldata(contract_address, "test_storage_read_write", &storage_write_args) + }; + let tx = executable_invoke_tx(invoke_tx_args! { + sender_address: account_address, + calldata, + resource_bounds: default_all_resource_bounds(), + nonce: nonce_manager.next(account_address), + }); + Transaction::Account(AccountTransaction::new_for_sequencing(tx)) + .execute(&mut state, &block_context) +} + +fn assert_blocked(result: TransactionExecutionResult) { + let error = result.expect_err("Access to a blocked storage key should fail the tx."); + assert_matches!( + &error, + TransactionExecutionError::BlockedStorageKeyAccessed { message } + if message == BLOCKED_STORAGE_KEY_ERROR_MESSAGE + ); + assert_eq!(error.to_string(), BLOCKED_STORAGE_KEY_ERROR_MESSAGE); +} + +fn assert_executed(result: TransactionExecutionResult) { + let tx_execution_info = result.expect("Tx should execute successfully."); + assert!(!tx_execution_info.is_reverted(), "{:?}", tx_execution_info.revert_error); +} + +#[rstest] +#[case::empty_blocklist("", felt!(0x10_u8))] +#[case::unrelated_key_blocked("0x10", felt!(0x11_u8))] +#[case::unrelated_long_key_blocked( + "0x3f1abc55d5d1c9d3f6a8f0e2b7c4d9e1f2a3b4c5d6e7f8091a2b3c4d5e6f70", + felt!("0x3f1abc55d5d1c9d3f6a8f0e2b7c4d9e1f2a3b4c5d6e7f8091a2b3c4d5e6f71") +)] +// A trailing comma must not turn into a blocked key 0x0. +#[case::trailing_comma_does_not_block_key_zero("0x10,", felt!(0x0_u8))] +fn test_blocked_storage_keys_do_not_affect_other_keys( + #[case] blocked_storage_keys: &str, + #[case] accessed_storage_key: Felt, + #[values(false, true)] nested: bool, +) { + assert_executed(execute_storage_write_tx(blocked_storage_keys, accessed_storage_key, nested)); +} + +#[rstest] +#[case::single_digit_key("0x1", felt!(0x1_u8))] +#[case::key_zero("0x0", felt!(0x0_u8))] +#[case::two_digit_key("0x10", felt!(0x10_u8))] +#[case::key_in_list("0x5,0x10,0x7", felt!(0x10_u8))] +#[case::hex_letter_key("0xab", felt!(0xab_u8))] +#[case::long_key( + "0x3f1abc55d5d1c9d3f6a8f0e2b7c4d9e1f2a3b4c5d6e7f8091a2b3c4d5e6f70", + felt!("0x3f1abc55d5d1c9d3f6a8f0e2b7c4d9e1f2a3b4c5d6e7f8091a2b3c4d5e6f70") +)] +// Normalization: surrounding whitespace, uppercase digits and leading zeros. +#[case::whitespace_around_key(" 0x10 , 0x20 ", felt!(0x10_u8))] +#[case::uppercase_digits("0xAB", felt!(0xab_u8))] +#[case::leading_zeros( + "0x0000000000000000000000000000000000000000000000000000000000000010", + felt!(0x10_u8) +)] +fn test_blocked_storage_key_access_fails_tx( + #[case] blocked_storage_keys: &str, + #[case] accessed_storage_key: Felt, + #[values(false, true)] nested: bool, +) { + assert_blocked(execute_storage_write_tx(blocked_storage_keys, accessed_storage_key, nested)); +} + +#[test] +fn test_parse_blocked_storage_keys() { + assert_eq!(parse_blocked_storage_keys("").unwrap(), HashSet::new()); + assert_eq!( + parse_blocked_storage_keys(" 0x1, ,0x10,").unwrap(), + HashSet::from([storage_key!(0x1_u8), storage_key!(0x10_u8)]) + ); + // Not a hex number. + assert!(parse_blocked_storage_keys("0x10,0xzz").is_err()); + // Out of the storage key range (2^251). + assert!( + parse_blocked_storage_keys( + "0x800000000000000000000000000000000000000000000000000000000000000" + ) + .is_err() + ); +}