From dffc899a59583cfb8b4ad17cd1f738f3fabdc508 Mon Sep 17 00:00:00 2001 From: Asaf Merschon Date: Sun, 16 Aug 2026 10:37:17 +0300 Subject: [PATCH] apollo_batcher,apollo_dashboard: read class definitions through the class manager in view calls Batcher::call_contract built its state reader with ApolloReader::new, which leaves class_reader as None. On that path classes are read straight from the batcher's storage, but the batcher only records that a class was declared, via append_state_diff, and never writes the definition: on the node path only apollo_central_sync calls append_classes. For a Cairo 1 class is_declared is true while get_casm_and_sierra returns (None, None), couple_casm_and_sierra maps that to Ok(None), and the expect in read_casm_and_sierra panics inside the blocking task. The node installs set_exit_process_on_panic, a global hook that calls std::process::exit(1), so the panic killed the node. Cairo 0 was broken too: ThinStateDiff::deprecated_declared_classes carries class hashes only, and the definitions reach storage through that same append_classes path, so the read failed with UndeclaredClassHash. Every view call therefore failed. Nothing caught it because call_contract is not wired into any deployment yet, and the existing tests inject a DictStateReader, which never reaches ApolloReader. Build the reader with new_with_class_reader instead, passing the class manager the execution path already uses in block_builder.rs, and thread the runtime handle the class reader needs to block on it from the blocking task, the way create_block_builder already takes one. Wrap that reader in StateReaderAndContractManager the way the execution path does, so both share one ContractClassManager. Without it every view call re-fetches and re-compiles every class it touches, pulling the full Sierra just to read get_sierra_version. The native classes whitelist comes from the same dynamic config block production reads, so a view call executes on the backend a block would: the deployments set it to the empty list, which the implicit NativeClassesWhitelist::All of StateReaderAndContractManager::new would have ignored. That part holds by construction and not by test: in the default build TrivialClassManager::get_runnable discards the whitelist and BatcherDynamicConfig::default() is All, so reverting to the implicit constructor fails nothing in CI. Sharing the manager shares its costs too. A view call cache miss enqueues on the same single-threaded native compilation worker block production uses, and the classes a view call brings in occupy the same 2000 entry LRU. Both are latent while the deployed whitelist is empty and nothing compiles natively. View call cache hits and misses are counted under their own metrics, so the block production miss ratio keeps measuring block production alone. Both sets live under the batcher scope, so the dashboard panel names its source explicitly instead of deriving the name from the scope; CacheMetrics::get_scope becomes CacheMetrics::validate_scope, keeping the assertion that misses and hits agree on their scope. The class reads have no overall deadline across retries. The distributed deployment sets components.class_manager.remote_client_config.retries to 150 in apollo_deployments/resources/services/distributed/batcher.json, over the default 30 second request_timeout_ms, and the local client the other deployments use has neither timeout nor retries. Bound the call with batcher_config.dynamic_config.view_call_timeout_millis, five seconds. It holds the batcher's serialized request slot, which block production shares; timing out frees the slot but does not cancel the blocking task, which runs to completion on its own thread. The new tests read classes back through the factory at the height call_contract uses, one past the last written state diff, covering the Cairo 1 route, the deprecated route, and a class the class manager does not have. Four more drive call_contract itself: over the real factory against a deployed Cairo 1 contract and a deployed Cairo 0 one, over a second call that must serve the class from the cache, and against a reader that never answers, for the timeout. Co-Authored-By: Claude Opus 5 (1M context) --- crates/apollo_batcher/Cargo.toml | 1 + crates/apollo_batcher/src/batcher.rs | 110 +++-- crates/apollo_batcher/src/batcher_test.rs | 390 +++++++++++++++++- crates/apollo_batcher/src/metrics.rs | 8 + crates/apollo_batcher_config/src/config.rs | 15 + .../resources/dev_grafana.json | 13 +- .../apollo_dashboard/src/panels/blockifier.rs | 36 +- .../resources/app_configs/batcher_config.json | 1 + .../app_configs/replacer_batcher_config.json | 1 + .../apollo_node/resources/config_schema.json | 5 + crates/blockifier/src/metrics.rs | 6 +- 11 files changed, 540 insertions(+), 46 deletions(-) diff --git a/crates/apollo_batcher/Cargo.toml b/crates/apollo_batcher/Cargo.toml index 9c8e04118c1..8ddcb1dd2c9 100644 --- a/crates/apollo_batcher/Cargo.toml +++ b/crates/apollo_batcher/Cargo.toml @@ -78,3 +78,4 @@ rstest.workspace = true starknet-types-core.workspace = true starknet_api = { workspace = true, features = ["testing"] } tempfile.workspace = true +tokio = { workspace = true, features = ["test-util"] } diff --git a/crates/apollo_batcher/src/batcher.rs b/crates/apollo_batcher/src/batcher.rs index 24aa4938734..a42cccabfad 100644 --- a/crates/apollo_batcher/src/batcher.rs +++ b/crates/apollo_batcher/src/batcher.rs @@ -44,7 +44,7 @@ use apollo_mempool_types::communication::SharedMempoolClient; use apollo_mempool_types::mempool_types::CommitBlockArgs; use apollo_proof_manager_types::SharedProofManagerClient; use apollo_reverts::revert_block; -use apollo_state_reader::apollo_state::ApolloReader; +use apollo_state_reader::apollo_state::{ApolloReader, ClassReader}; use apollo_state_sync_types::state_sync_types::SyncBlock; #[cfg(feature = "os_input")] use apollo_storage::accessed_keys::{AccessedKeys, AccessedKeysStorageWriter}; @@ -81,6 +81,7 @@ use apollo_storage::{ StorageWriter, }; use async_trait::async_trait; +use blockifier::blockifier::config::NativeClassesWhitelist; use blockifier::blockifier_versioned_constants::VersionedConstants; use blockifier::bouncer::BouncerConfig; use blockifier::concurrency::worker_pool::WorkerPool; @@ -90,6 +91,7 @@ use blockifier::execution::entry_point::call_view_entry_point; use blockifier::state::cached_state::CommitmentStateDiff; use blockifier::state::contract_class_manager::ContractClassManager; use blockifier::state::state_api::StateReader; +use blockifier::state::state_reader_and_contract_manager::StateReaderAndContractManager; use blockifier::transaction::objects::TransactionExecutionInfo; use futures::FutureExt; use indexmap::{IndexMap, IndexSet}; @@ -140,6 +142,7 @@ use crate::metrics::{ ProposalMetricsHandle, BATCHED_TRANSACTIONS, BATCHER_L1_EVENTS_PROVIDER_ERRORS, + BATCHER_VIEW_CALL_CLASS_CACHE_METRICS, BUILDING_HEIGHT, GLOBAL_ROOT_HEIGHT, L2_GAS_IN_LAST_BLOCK, @@ -176,6 +179,13 @@ use crate::utils::{ type OutputStreamReceiver = tokio::sync::mpsc::UnboundedReceiver; type InputStreamSender = tokio::sync::mpsc::Sender; +/// Maximal number of felts a view entry point call may return, about 3.2 MB at 32 bytes per felt. +/// Far above the largest legitimate reader (the staking contract's staker list, a few felts per +/// staker), and small enough to stay cheap to serialize over the batcher's remote server boundary. +/// +/// Bounds the top-level return value only. +pub(crate) const MAX_VIEW_CALL_RETDATA_LENGTH: usize = 100_000; + #[cfg_attr(test, apollo_proc_macros::upgrade_fields_visibility(pub(crate)))] pub struct Batcher { pub config: BatcherConfig, @@ -745,7 +755,11 @@ impl Batcher { Some(last_committed_block) => self.get_block_info(last_committed_block)?, }; - let state_reader = self.view_state_reader_factory.create(height); + let state_reader = self.view_state_reader_factory.create( + height, + self.config.dynamic_config.native_classes_whitelist.clone(), + tokio::runtime::Handle::current(), + ); let block_context = BlockContext::new( block_info, self.config.static_config.block_builder_config.chain_info.clone(), @@ -753,7 +767,7 @@ impl Batcher { BouncerConfig::max(), ); - let retdata = tokio::task::spawn_blocking(move || { + let call_task = tokio::task::spawn_blocking(move || { call_view_entry_point( state_reader, Arc::new(block_context), @@ -762,13 +776,27 @@ impl Batcher { Calldata::from(input.calldata), ) .map(|call_info| call_info.execution.retdata.0) - }) - .await - .map_err(|err| { - error!("Failed to spawn blocking task for call_contract: {err}"); - BatcherError::InternalError - })? - .map_err(|err| BatcherError::ContractCallFailed { reason: err.to_string() })?; + }); + + // Timing out releases the batcher's request slot but does not cancel the blocking task, + // which runs to completion on its own thread. + let view_call_timeout = self.config.dynamic_config.view_call_timeout_millis; + let retdata = tokio::time::timeout(view_call_timeout, call_task) + .await + .map_err(|_| { + error!("View call timed out after {view_call_timeout:?}."); + BatcherError::ContractCallFailed { + reason: format!( + "Call timed out after {} seconds.", + view_call_timeout.as_secs() + ), + } + })? + .map_err(|err| { + error!("Failed to spawn blocking task for call_contract: {err}"); + BatcherError::InternalError + })? + .map_err(|err| BatcherError::ContractCallFailed { reason: err.to_string() })?; validate_retdata_length(retdata.len())?; @@ -1539,14 +1567,6 @@ impl Batcher { } } -/// Maximal number of felts a view entry point call may return, about 3.2 MB at 32 bytes per felt. -/// Far above the largest legitimate reader (the staking contract's staker list, a few felts per -/// staker), and small enough to stay cheap to serialize over the batcher's remote server boundary. -/// -/// Bounds the top-level return value only; the view call's resource bounds are what limit memory -/// during execution. -pub(crate) const MAX_VIEW_CALL_RETDATA_LENGTH: usize = 100_000; - /// Rejects a view call whose return value is too large to hand back to the caller. pub(crate) fn validate_retdata_length(retdata_length: usize) -> BatcherResult<()> { if retdata_length > MAX_VIEW_CALL_RETDATA_LENGTH { @@ -1660,18 +1680,22 @@ pub async fn create_batcher( config: config.static_config.pre_confirmed_block_writer_config, cende_client: pre_confirmed_cende_client, }); + // Block production and view calls share one class cache. + let contract_class_manager = + ContractClassManager::start(config.static_config.contract_class_manager_config.clone()); let block_builder_factory = Box::new(BlockBuilderFactory { block_builder_config: config.static_config.block_builder_config.clone(), storage_reader: storage_reader.clone(), - contract_class_manager: ContractClassManager::start( - config.static_config.contract_class_manager_config.clone(), - ), - class_manager_client, + contract_class_manager: contract_class_manager.clone(), + class_manager_client: class_manager_client.clone(), proof_manager_client, worker_pool, }); - let view_state_reader_factory = - Box::new(StorageViewStateReaderFactory { storage_reader: storage_reader.clone() }); + let view_state_reader_factory = Box::new(StorageViewStateReaderFactory { + storage_reader: storage_reader.clone(), + contract_class_manager, + class_manager_client, + }); let storage_reader = Arc::new(storage_reader); let storage_writer = Box::new(storage_writer); @@ -1933,16 +1957,48 @@ impl BatcherStorageWriter for StorageWriter { /// real storage infrastructure. #[cfg_attr(test, automock)] pub trait ViewStateReaderFactory: Send + Sync { - fn create(&self, block_number: BlockNumber) -> Box; + /// `native_classes_whitelist` gates which classes may execute under Cairo native, and must be + /// the one block production runs with, so a view call returns what a block would compute. + /// The reader blocks on the class manager through `runtime`, from the blocking task the view + /// call runs in. + fn create( + &self, + block_number: BlockNumber, + native_classes_whitelist: NativeClassesWhitelist, + runtime: tokio::runtime::Handle, + ) -> Box; } pub(crate) struct StorageViewStateReaderFactory { pub(crate) storage_reader: StorageReader, + pub(crate) contract_class_manager: ContractClassManager, + pub(crate) class_manager_client: SharedClassManagerClient, } impl ViewStateReaderFactory for StorageViewStateReaderFactory { - fn create(&self, block_number: BlockNumber) -> Box { - Box::new(ApolloReader::new(self.storage_reader.clone(), block_number)) + fn create( + &self, + block_number: BlockNumber, + native_classes_whitelist: NativeClassesWhitelist, + runtime: tokio::runtime::Handle, + ) -> Box { + // The batcher's storage records class declarations but never writes the definitions, so a + // class is only readable through the class manager. + let class_reader = Some(ClassReader { reader: self.class_manager_client.clone(), runtime }); + let apollo_reader = ApolloReader::new_with_class_reader( + self.storage_reader.clone(), + block_number, + class_reader, + ); + // The class cache is shared with block production, so a view call fetches and compiles only + // the classes neither has seen yet. Its hits and misses are counted apart from block + // production's, which view calls would otherwise dominate. + Box::new(StateReaderAndContractManager::new_with_native_classes_whitelist( + apollo_reader, + self.contract_class_manager.clone(), + native_classes_whitelist, + Some(BATCHER_VIEW_CALL_CLASS_CACHE_METRICS), + )) } } diff --git a/crates/apollo_batcher/src/batcher_test.rs b/crates/apollo_batcher/src/batcher_test.rs index feecc8648f0..7246d9e8097 100644 --- a/crates/apollo_batcher/src/batcher_test.rs +++ b/crates/apollo_batcher/src/batcher_test.rs @@ -1,6 +1,9 @@ use std::fmt::Debug; use std::hash::Hash; +use std::sync::mpsc::{channel, Receiver}; use std::sync::{Arc, Mutex}; +use std::task::Poll; +use std::time::Duration; use apollo_batcher_config::config::{BatcherConfig, BatcherDynamicConfig, BatcherStaticConfig}; use apollo_batcher_types::batcher_types::{ @@ -23,6 +26,7 @@ use apollo_batcher_types::batcher_types::{ ValidateBlockInput, }; use apollo_batcher_types::errors::BatcherError; +use apollo_class_manager_types::MockClassManagerClient; use apollo_committer_types::committer_types::CommitBlockRequest; use apollo_config_manager_types::communication::MockConfigManagerClient; use apollo_infra::component_client::ClientError; @@ -37,13 +41,21 @@ use apollo_mempool_types::communication::{ use apollo_mempool_types::mempool_types::CommitBlockArgs; use apollo_state_sync_types::state_sync_types::SyncBlock; use apollo_storage::db::DbError; +use apollo_storage::header::HeaderStorageWriter; +use apollo_storage::partial_block_hash::PartialBlockHashComponentsStorageWriter; +use apollo_storage::state::StateStorageWriter; use apollo_storage::test_utils::get_test_storage; use apollo_storage::{StorageError, StorageReader, StorageWriter}; use assert_matches::assert_matches; use blockifier::abi::constants; +use blockifier::blockifier::config::{ContractClassManagerConfig, NativeClassesWhitelist}; use blockifier::context::{BlockContext, ChainInfo}; +use blockifier::execution::contract_class::RunnableCompiledClass; use blockifier::state::cached_state::CachedState; -use blockifier::state::state_api::StateReader; +use blockifier::state::contract_class_manager::ContractClassManager; +use blockifier::state::errors::StateError; +use blockifier::state::state_api::{StateReader, StateResult}; +use blockifier::test_utils::contracts::FeatureContractTrait; use blockifier::test_utils::dict_state_reader::DictStateReader; use blockifier::test_utils::initial_test_state::test_state; use blockifier::test_utils::BALANCE; @@ -55,15 +67,19 @@ use blockifier::transaction::transactions::ExecutableTransaction; use blockifier_test_utils::cairo_versions::{CairoVersion, RunnableCairo1}; use blockifier_test_utils::calldata::create_calldata; use blockifier_test_utils::contracts::FeatureContract; +use futures::poll; use indexmap::{indexmap, IndexMap, IndexSet}; use metrics_exporter_prometheus::PrometheusBuilder; use mockall::predicate::{always, eq}; use rstest::rstest; use starknet_api::block::{ BlockHash, + BlockHeader, BlockHeaderWithoutHash, BlockInfo, BlockNumber, + GasPrice, + GasPricePerToken, StarknetVersion, }; use starknet_api::block_hash::block_hash_calculator::{ @@ -72,10 +88,11 @@ use starknet_api::block_hash::block_hash_calculator::{ PartialBlockHashComponents, }; use starknet_api::consensus_transaction::InternalConsensusTransaction; -use starknet_api::core::{ClassHash, CompiledClassHash, GlobalRoot, Nonce}; -use starknet_api::state::ThinStateDiff; +use starknet_api::contract_class::ContractClass; +use starknet_api::core::{ClassHash, CompiledClassHash, ContractAddress, GlobalRoot, Nonce}; +use starknet_api::state::{SierraContractClass, StorageKey, ThinStateDiff}; use starknet_api::transaction::TransactionHash; -use starknet_api::{invoke_tx_args, tx_hash}; +use starknet_api::{class_hash, invoke_tx_args, tx_hash}; use starknet_types_core::felt::Felt; use tempfile::TempDir; use validator::Validate; @@ -158,6 +175,10 @@ const RECURSION_DEPTH_WITHIN_RESOURCE_BOUNDS: u64 = 1_000; /// not the block limit. One level of `recurse` costs about 973 Sierra gas and about 4 Cairo steps. const SIERRA_GAS_RECURSION_DEPTH_EXCEEDING_RESOURCE_BOUNDS: u64 = 300_000; const CAIRO_STEPS_RECURSION_DEPTH_EXCEEDING_RESOURCE_BOUNDS: u64 = 400_000; +/// Height the view call tests write their state diff at. A view call reads at +/// `state_diff_height()`, one past the last written diff, and takes its block info from the block +/// before that. +const LAST_COMMITTED_HEIGHT: BlockNumber = BlockNumber(0); struct TestViewStateReaderFactory { state: Arc>>, @@ -165,7 +186,12 @@ struct TestViewStateReaderFactory { } impl ViewStateReaderFactory for TestViewStateReaderFactory { - fn create(&self, block_number: BlockNumber) -> Box { + fn create( + &self, + block_number: BlockNumber, + _native_classes_whitelist: NativeClassesWhitelist, + _runtime: tokio::runtime::Handle, + ) -> Box { assert_eq!(block_number, self.expected_block_number); Box::new(self.state.lock().unwrap().clone()) } @@ -254,6 +280,7 @@ struct MockDependenciesWithRealStorage { storage_reader: StorageReader, storage_writer: StorageWriter, clients: MockClients, + class_manager_client: MockClassManagerClient, batcher_config: BatcherConfig, _temp_dir: TempDir, // Keep the temp dir alive. } @@ -266,12 +293,17 @@ impl Default for MockDependenciesWithRealStorage { storage_reader, storage_writer, clients: MockClients::default(), + class_manager_client: MockClassManagerClient::new(), batcher_config: BatcherConfig { static_config: BatcherStaticConfig { outstream_content_buffer_size: STREAMING_CHUNK_SIZE, ..Default::default() }, - ..Default::default() + // Compiling a class in a debug build takes longer than the production timeout. + dynamic_config: BatcherDynamicConfig { + view_call_timeout_millis: Duration::from_secs(300), + ..Default::default() + }, }, _temp_dir: temp_dir, } @@ -294,6 +326,8 @@ async fn create_batcher_with_real_storage( ) -> Batcher { let view_state_reader_factory = Box::new(StorageViewStateReaderFactory { storage_reader: mock_dependencies.storage_reader.clone(), + contract_class_manager: ContractClassManager::start(ContractClassManagerConfig::default()), + class_manager_client: Arc::new(mock_dependencies.class_manager_client), }); create_batcher_impl( Arc::new(mock_dependencies.storage_reader), @@ -2184,3 +2218,347 @@ fn validate_retdata_length_rejects_length_above_the_limit(#[case] retdata_length && reason.contains(&MAX_VIEW_CALL_RETDATA_LENGTH.to_string()) ); } + +/// Writes `state_diff` to real batcher storage at `LAST_COMMITTED_HEIGHT` and reads `class_hash` +/// back through the view state reader the factory builds over it, at the height `call_contract` +/// would use. +async fn read_class_through_view_state_reader( + state_diff: ThinStateDiff, + class_manager_client: MockClassManagerClient, + class_hash: ClassHash, +) -> StateResult { + let ((storage_reader, mut storage_writer), _temp_dir) = get_test_storage(); + storage_writer + .begin_rw_txn() + .unwrap() + .append_state_diff(LAST_COMMITTED_HEIGHT, state_diff) + .unwrap() + .commit() + .unwrap(); + + let factory = StorageViewStateReaderFactory { + storage_reader, + contract_class_manager: ContractClassManager::start(ContractClassManagerConfig::default()), + class_manager_client: Arc::new(class_manager_client), + }; + let state_reader = factory.create( + LAST_COMMITTED_HEIGHT.unchecked_next(), + NativeClassesWhitelist::All, + tokio::runtime::Handle::current(), + ); + + tokio::task::spawn_blocking(move || state_reader.get_compiled_class(class_hash)) + .await + .expect("Reading a declared class panicked.") +} + +/// Cairo 1 declaration marker only, with no definition behind it: `append_state_diff` records +/// `class_hash_to_compiled_class_hash` in the declared classes table that `is_declared` reads. +fn cairo_1_declaration(class_hash: ClassHash) -> ThinStateDiff { + ThinStateDiff { + class_hash_to_compiled_class_hash: indexmap! { class_hash => CompiledClassHash::default() }, + ..Default::default() + } +} + +/// Cairo 0 declaration marker only, with no definition behind it: `append_state_diff` records +/// `deprecated_declared_classes` in the deprecated classes table, which `is_declared` does not +/// read. +fn cairo_0_declaration(class_hash: ClassHash) -> ThinStateDiff { + ThinStateDiff { deprecated_declared_classes: vec![class_hash], ..Default::default() } +} + +/// The declaration marker `contract`'s class hash reaches the view state reader through. +fn declaration(contract: FeatureContract) -> ThinStateDiff { + match contract.cairo_version() { + CairoVersion::Cairo0 => cairo_0_declaration(contract.get_class_hash()), + CairoVersion::Cairo1(_) => cairo_1_declaration(contract.get_class_hash()), + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn view_state_reader_reads_cairo_1_class_through_the_class_manager() { + let class_hash = class_hash!("0x1234"); + let mut class_manager_client = MockClassManagerClient::new(); + class_manager_client + .expect_get_executable() + .times(1) + .with(eq(class_hash)) + .return_once(|_| Ok(Some(ContractClass::test_casm_contract_class()))); + class_manager_client + .expect_get_sierra() + .times(1) + .with(eq(class_hash)) + .return_once(|_| Ok(Some(SierraContractClass::default()))); + + let compiled_class = read_class_through_view_state_reader( + cairo_1_declaration(class_hash), + class_manager_client, + class_hash, + ) + .await; + + assert_matches!(compiled_class, Ok(RunnableCompiledClass::V1(_))); +} + +/// Pins the route a declared Cairo 0 class takes. `is_declared` reads the Cairo 1 declared classes +/// table only, and `append_state_diff` writes `deprecated_declared_classes` to a different table, +/// so the class takes the deprecated route, which asks the class manager for the definition and +/// never reads storage. Were `is_declared` widened to the deprecated table, the class would take +/// the Cairo 1 route instead and panic in `ClassReader::read_casm`. +#[tokio::test(flavor = "multi_thread")] +async fn view_state_reader_reads_cairo_0_class_through_the_class_manager() { + let class_hash = class_hash!("0x1234"); + let mut class_manager_client = MockClassManagerClient::new(); + class_manager_client + .expect_get_executable() + .times(1) + .with(eq(class_hash)) + .return_once(|_| Ok(Some(ContractClass::test_deprecated_casm_contract_class()))); + + let compiled_class = read_class_through_view_state_reader( + cairo_0_declaration(class_hash), + class_manager_client, + class_hash, + ) + .await; + + assert_matches!(compiled_class, Ok(RunnableCompiledClass::V0(_))); +} + +#[tokio::test(flavor = "multi_thread")] +async fn view_state_reader_errors_when_the_class_manager_lacks_a_declared_class() { + let class_hash = class_hash!("0x1234"); + let mut class_manager_client = MockClassManagerClient::new(); + class_manager_client + .expect_get_executable() + .times(1) + .with(eq(class_hash)) + .return_once(|_| Ok(None)); + + let result = read_class_through_view_state_reader( + cairo_1_declaration(class_hash), + class_manager_client, + class_hash, + ) + .await; + + assert_matches!( + result, + Err(StateError::UndeclaredClassHash(undeclared_class_hash)) + if undeclared_class_hash == class_hash + ); +} + +/// A state reader whose every read parks until the sending half of `release_receiver` is dropped, +/// standing in for a class manager that never answers. +struct GatedStateReader { + release_receiver: Arc>>, +} + +impl GatedStateReader { + fn wait_for_release(&self) -> StateError { + let _ = self.release_receiver.lock().unwrap().recv(); + StateError::StateReadError("Released.".to_string()) + } +} + +impl StateReader for GatedStateReader { + fn get_storage_at(&self, _address: ContractAddress, _key: StorageKey) -> StateResult { + Err(self.wait_for_release()) + } + + fn get_nonce_at(&self, _address: ContractAddress) -> StateResult { + Err(self.wait_for_release()) + } + + fn get_class_hash_at(&self, _address: ContractAddress) -> StateResult { + Err(self.wait_for_release()) + } + + fn get_compiled_class(&self, _class_hash: ClassHash) -> StateResult { + Err(self.wait_for_release()) + } + + fn get_compiled_class_hash(&self, _class_hash: ClassHash) -> StateResult { + Err(self.wait_for_release()) + } + + fn get_compiled_class_hash_v2( + &self, + _class_hash: ClassHash, + _compiled_class: &RunnableCompiledClass, + ) -> StateResult { + Err(self.wait_for_release()) + } +} + +struct GatedViewStateReaderFactory { + release_receiver: Arc>>, +} + +impl ViewStateReaderFactory for GatedViewStateReaderFactory { + fn create( + &self, + _block_number: BlockNumber, + _native_classes_whitelist: NativeClassesWhitelist, + _runtime: tokio::runtime::Handle, + ) -> Box { + Box::new(GatedStateReader { release_receiver: self.release_receiver.clone() }) + } +} + +#[tokio::test(start_paused = true)] +async fn call_contract_times_out_when_the_state_reader_never_answers() { + let (release_sender, release_receiver) = channel(); + + let batcher = create_batcher(MockDependencies { + view_state_reader_factory: Box::new(GatedViewStateReaderFactory { + release_receiver: Arc::new(Mutex::new(release_receiver)), + }), + ..Default::default() + }) + .await; + let view_call_timeout = batcher.config.dynamic_config.view_call_timeout_millis; + + let mut call_contract_future = Box::pin(batcher.call_contract(CallContractInput { + contract_address: Default::default(), + entry_point: "get_stakers".to_string(), + calldata: vec![], + })); + // The first poll registers the timeout's timer; advancing the paused clock then expires it. + assert!(poll!(&mut call_contract_future).is_pending()); + tokio::time::advance(view_call_timeout).await; + let Poll::Ready(result) = poll!(&mut call_contract_future) else { + panic!("The call did not return once its timeout elapsed."); + }; + + assert_matches!( + result, + Err(BatcherError::ContractCallFailed { reason }) + if reason.contains(&view_call_timeout.as_secs().to_string()) + ); + + // Let the blocked read finish, so the runtime can shut down. + drop(release_sender); +} + +/// Deploys `contract` in real batcher storage at `LAST_COMMITTED_HEIGHT`, and returns the +/// dependencies of a batcher whose view calls run over the real `StorageViewStateReaderFactory`. +/// The caller states the class manager expectations, so that each test owns the number of reads it +/// asserts on. +fn deploy_contract_in_real_storage(contract: FeatureContract) -> MockDependenciesWithRealStorage { + let class_hash = contract.get_class_hash(); + let mut mock_dependencies = MockDependenciesWithRealStorage::default(); + + let gas_price = GasPricePerToken { price_in_wei: GasPrice(1), price_in_fri: GasPrice(1) }; + mock_dependencies + .storage_writer + .begin_rw_txn() + .unwrap() + .append_header( + LAST_COMMITTED_HEIGHT, + &BlockHeader { + block_header_without_hash: BlockHeaderWithoutHash { + block_number: LAST_COMMITTED_HEIGHT, + l1_gas_price: gas_price, + l1_data_gas_price: gas_price, + l2_gas_price: gas_price, + ..Default::default() + }, + ..Default::default() + }, + ) + .unwrap() + .append_state_diff( + LAST_COMMITTED_HEIGHT, + ThinStateDiff { + deployed_contracts: indexmap! { contract.get_instance_address(0) => class_hash }, + ..declaration(contract) + }, + ) + .unwrap() + // The commitment manager panics on a committed height with no hash commitment behind it. + .set_partial_block_hash_components( + &LAST_COMMITTED_HEIGHT, + &PartialBlockHashComponents::default(), + ) + .unwrap() + .commit() + .unwrap(); + + mock_dependencies +} + +/// Drives `call_contract` over the real `StorageViewStateReaderFactory`, the composition the node +/// runs: the runtime handle reaches the factory, the reader lands inside `spawn_blocking`, and the +/// class read blocks on the class manager from that thread. Running the call on the async runtime +/// instead panics in `ClassReader::block_on`. +#[rstest] +#[case::cairo_1(SIERRA_GAS_TRACKED_RECURSIVE_CONTRACT)] +#[case::cairo_0(CAIRO_STEPS_TRACKED_RECURSIVE_CONTRACT)] +#[tokio::test(flavor = "multi_thread")] +async fn call_contract_over_real_storage_executes_a_class_from_the_class_manager( + #[case] contract: FeatureContract, +) { + let class_hash = contract.get_class_hash(); + let mut mock_dependencies = deploy_contract_in_real_storage(contract); + mock_dependencies + .class_manager_client + .expect_get_executable() + .times(1) + .with(eq(class_hash)) + .returning(move |_| Ok(Some(contract.get_class()))); + // The Cairo 1 route reads the Sierra too, for its version. The Cairo 0 route reads the + // executable alone. + if matches!(contract.cairo_version(), CairoVersion::Cairo1(_)) { + mock_dependencies + .class_manager_client + .expect_get_sierra() + .times(1) + .with(eq(class_hash)) + .returning(move |_| Ok(Some(contract.get_sierra()))); + } + let batcher = create_batcher_with_real_storage(mock_dependencies).await; + + let result = batcher + .call_contract(recurse_call_contract_input( + contract, + RECURSION_DEPTH_WITHIN_RESOURCE_BOUNDS, + )) + .await; + + assert_eq!(result.unwrap().retdata, vec![]); +} + +/// Two view calls fetch the class once: the second is served from the batcher's class cache, which +/// the view path and block production build over one `ContractClassManager`. +#[tokio::test(flavor = "multi_thread")] +async fn repeated_view_calls_fetch_the_class_once() { + let contract = SIERRA_GAS_TRACKED_RECURSIVE_CONTRACT; + let class_hash = contract.get_class_hash(); + let mut mock_dependencies = deploy_contract_in_real_storage(contract); + mock_dependencies + .class_manager_client + .expect_get_executable() + .times(1) + .with(eq(class_hash)) + .returning(move |_| Ok(Some(contract.get_class()))); + mock_dependencies + .class_manager_client + .expect_get_sierra() + .times(1) + .with(eq(class_hash)) + .returning(move |_| Ok(Some(contract.get_sierra()))); + let batcher = create_batcher_with_real_storage(mock_dependencies).await; + + for _ in 0..2 { + batcher + .call_contract(recurse_call_contract_input( + contract, + RECURSION_DEPTH_WITHIN_RESOURCE_BOUNDS, + )) + .await + .unwrap(); + } +} diff --git a/crates/apollo_batcher/src/metrics.rs b/crates/apollo_batcher/src/metrics.rs index e2849574aa0..ffdecbbf84a 100644 --- a/crates/apollo_batcher/src/metrics.rs +++ b/crates/apollo_batcher/src/metrics.rs @@ -24,6 +24,10 @@ define_metrics!( // Global class cache MetricCounter { CLASS_CACHE_MISSES, "batcher_class_cache_misses", "Counter of the batcher's global class cache misses", init=0 }, MetricCounter { CLASS_CACHE_HITS, "batcher_class_cache_hits", "Counter of the batcher's global class cache hits", init=0 }, + // The same cache, counted separately for view calls, which are higher volume and mostly + // hits, so that they do not dilute the block production miss ratio. + MetricCounter { VIEW_CALL_CLASS_CACHE_MISSES, "batcher_view_call_class_cache_misses", "Counter of the batcher's global class cache misses in view calls", init=0 }, + MetricCounter { VIEW_CALL_CLASS_CACHE_HITS, "batcher_view_call_class_cache_hits", "Counter of the batcher's global class cache hits in view calls", init=0 }, // Heights MetricGauge { BUILDING_HEIGHT, "batcher_building_height", "The height of the block that should be built next. The height of the state diff marker as stored in the batcher's storage." }, MetricGauge { GLOBAL_ROOT_HEIGHT, "batcher_global_root_height", "The height of the first block without global root stored." }, @@ -136,6 +140,9 @@ pub(crate) fn record_preconfirmed_block_write_failure(reason: PreconfirmedBlockW pub const BATCHER_CLASS_CACHE_METRICS: CacheMetrics = CacheMetrics::new(CLASS_CACHE_MISSES, CLASS_CACHE_HITS); +pub const BATCHER_VIEW_CALL_CLASS_CACHE_METRICS: CacheMetrics = + CacheMetrics::new(VIEW_CALL_CLASS_CACHE_MISSES, VIEW_CALL_CLASS_CACHE_HITS); + pub fn register_metrics(storage_height: BlockNumber, global_root_height: BlockNumber) { BUILDING_HEIGHT.register(); BUILDING_HEIGHT.set_lossy(storage_height.0); @@ -185,6 +192,7 @@ pub fn register_metrics(storage_height: BlockNumber, global_root_height: BlockNu // Blockifier's metrics BATCHER_CLASS_CACHE_METRICS.register(); + BATCHER_VIEW_CALL_CLASS_CACHE_METRICS.register(); CALLS_RUNNING_NATIVE.register(); NATIVE_CLASS_RETURNED.register(); NATIVE_COMPILATION_ERROR.register(); diff --git a/crates/apollo_batcher_config/src/config.rs b/crates/apollo_batcher_config/src/config.rs index d03f8ad7061..5d5ca7836f5 100644 --- a/crates/apollo_batcher_config/src/config.rs +++ b/crates/apollo_batcher_config/src/config.rs @@ -344,6 +344,13 @@ pub struct BatcherDynamicConfig { serialize_with = "serialize_duration_as_milliseconds" )] pub proposer_idle_detection_delay_millis: Duration, + /// Maximal wall time (in milliseconds) a view entry point call may hold the batcher's request + /// slot, which block production shares. + #[serde( + deserialize_with = "deserialize_milliseconds_to_duration", + serialize_with = "serialize_duration_as_milliseconds" + )] + pub view_call_timeout_millis: Duration, } impl Default for BatcherDynamicConfig { @@ -356,6 +363,7 @@ impl Default for BatcherDynamicConfig { validate_tx_polling_interval_millis: 10, results_polling_interval_millis: 10, proposer_idle_detection_delay_millis: Duration::from_millis(1500), + view_call_timeout_millis: Duration::from_secs(5), } } } @@ -404,6 +412,13 @@ impl SerializeConfig for BatcherDynamicConfig { currently being executed, the proposer will finish building the current block.", ParamPrivacyInput::Public, ), + ser_param( + "view_call_timeout_millis", + &self.view_call_timeout_millis.as_millis(), + "Maximal wall time (in milliseconds) a view entry point call may hold the \ + batcher's request slot, which block production shares.", + ParamPrivacyInput::Public, + ), ]); dump.append(&mut prepend_sub_config_name( self.storage_reader_server_dynamic_config.dump(), diff --git a/crates/apollo_dashboard/resources/dev_grafana.json b/crates/apollo_dashboard/resources/dev_grafana.json index fc042a371cc..63f30cad5e2 100644 --- a/crates/apollo_dashboard/resources/dev_grafana.json +++ b/crates/apollo_dashboard/resources/dev_grafana.json @@ -1218,7 +1218,7 @@ "description": "The ratio of Native classes returned by the Blockifier in Batcher", "type": "timeseries", "exprs": [ - "(increase(native_class_returned{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) / clamp_min((increase(batcher_class_cache_misses{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) + increase(batcher_class_cache_hits{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m])), 1))" + "(increase(native_class_returned{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) / clamp_min((increase(batcher_class_cache_misses{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) + increase(batcher_class_cache_hits{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) + increase(batcher_view_call_class_cache_misses{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) + increase(batcher_view_call_class_cache_hits{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m])), 1))" ], "extra_params": { "unit": "percentunit" @@ -1235,6 +1235,17 @@ "unit": "percentunit" } }, + { + "title": "Class Cache Miss in Batcher View Calls", + "description": "The ratio of cache misses when requesting compiled classes from the Blockifier State Reader in Batcher View Calls", + "type": "timeseries", + "exprs": [ + "(increase(batcher_view_call_class_cache_misses{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) / clamp_min((increase(batcher_view_call_class_cache_misses{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m]) + increase(batcher_view_call_class_cache_hits{cluster=~\"$cluster\", namespace=~\"$namespace\", pod=~\"$pod\"}[5m])), 1))" + ], + "extra_params": { + "unit": "percentunit" + } + }, { "title": "Native compilation error count", "description": "Count of the number of times there was a native compilation error", diff --git a/crates/apollo_dashboard/src/panels/blockifier.rs b/crates/apollo_dashboard/src/panels/blockifier.rs index ed4754c63cb..2624db43519 100644 --- a/crates/apollo_dashboard/src/panels/blockifier.rs +++ b/crates/apollo_dashboard/src/panels/blockifier.rs @@ -1,5 +1,6 @@ use apollo_batcher::metrics::{ BATCHER_CLASS_CACHE_METRICS, + BATCHER_VIEW_CALL_CLASS_CACHE_METRICS, L2_GAS_IN_LAST_BLOCK, NUM_TRANSACTION_IN_BLOCK, PROVING_GAS_IN_LAST_BLOCK, @@ -24,14 +25,18 @@ use crate::query_builder::{sum_by_label, DisplayMethod, DEFAULT_DURATION}; const DENOMINATOR_DIVISOR_FOR_READABILITY: f64 = 1_000_000_000.0; +/// `source` names the reader the metrics count, since one scope may have several: the batcher +/// counts block production and view calls separately over the same cache. fn get_panel_blockifier_state_reader_class_cache_miss_ratio( class_cache_metrics: &CacheMetrics, + source: &str, ) -> Panel { - let scope = class_cache_metrics.get_scope(); - let name = format!("Class Cache Miss in {scope}"); + class_cache_metrics.validate_scope(); + + let name = format!("Class Cache Miss in {source}"); let description = format!( "The ratio of cache misses when requesting compiled classes from the Blockifier State \ - Reader in {scope}" + Reader in {source}" ); Panel::ratio_time_series( name.as_str(), @@ -43,15 +48,20 @@ fn get_panel_blockifier_state_reader_class_cache_miss_ratio( } fn get_panel_blockifier_state_reader_native_class_returned_ratio() -> Panel { - let class_cache_metrics = BATCHER_CLASS_CACHE_METRICS; - let name = "Native Class Returned Ratio in Batcher"; let description = "The ratio of Native classes returned by the Blockifier in Batcher"; + // `NATIVE_CLASS_RETURNED` counts every class the batcher's state readers return, so the + // denominator sums the cache traffic of all of them. Panel::ratio_time_series( name, description, &NATIVE_CLASS_RETURNED, - &[class_cache_metrics.misses(), class_cache_metrics.hits()], + &[ + BATCHER_CLASS_CACHE_METRICS.misses(), + BATCHER_CLASS_CACHE_METRICS.hits(), + BATCHER_VIEW_CALL_CLASS_CACHE_METRICS.misses(), + BATCHER_VIEW_CALL_CLASS_CACHE_METRICS.hits(), + ], BLOCKIFIER_METRIC_RATE_DURATION, ) } @@ -140,10 +150,20 @@ pub(crate) fn get_blockifier_row() -> Row { Row::new( "Blockifier", vec![ - get_panel_blockifier_state_reader_class_cache_miss_ratio(&BATCHER_CLASS_CACHE_METRICS), + get_panel_blockifier_state_reader_class_cache_miss_ratio( + &BATCHER_CLASS_CACHE_METRICS, + "Batcher", + ), // TODO(Arni): Add native class returned ratio for gateway get_panel_blockifier_state_reader_native_class_returned_ratio(), - get_panel_blockifier_state_reader_class_cache_miss_ratio(&GATEWAY_CLASS_CACHE_METRICS), + get_panel_blockifier_state_reader_class_cache_miss_ratio( + &GATEWAY_CLASS_CACHE_METRICS, + "Gateway", + ), + get_panel_blockifier_state_reader_class_cache_miss_ratio( + &BATCHER_VIEW_CALL_CLASS_CACHE_METRICS, + "Batcher View Calls", + ), get_panel_native_compilation_error(), get_panel_native_execution_ratio(), get_panel_blocks_full_by_resource(), diff --git a/crates/apollo_deployments/resources/app_configs/batcher_config.json b/crates/apollo_deployments/resources/app_configs/batcher_config.json index 39c0ec467b2..61645afd213 100644 --- a/crates/apollo_deployments/resources/app_configs/batcher_config.json +++ b/crates/apollo_deployments/resources/app_configs/batcher_config.json @@ -5,6 +5,7 @@ "batcher_config.dynamic_config.storage_reader_server_dynamic_config.enable": false, "batcher_config.dynamic_config.tx_polling_interval_millis": 200, "batcher_config.dynamic_config.validate_tx_polling_interval_millis": 10, + "batcher_config.dynamic_config.view_call_timeout_millis": 5000, "batcher_config.static_config.block_builder_config.bouncer_config.block_max_capacity.l1_gas": 4400000, "batcher_config.static_config.block_builder_config.bouncer_config.block_max_capacity.message_segment_length": 3700, "batcher_config.static_config.block_builder_config.bouncer_config.block_max_capacity.n_events": 5000, diff --git a/crates/apollo_deployments/resources/app_configs/replacer_batcher_config.json b/crates/apollo_deployments/resources/app_configs/replacer_batcher_config.json index 91ca9f75c67..e92eaaacce4 100644 --- a/crates/apollo_deployments/resources/app_configs/replacer_batcher_config.json +++ b/crates/apollo_deployments/resources/app_configs/replacer_batcher_config.json @@ -5,6 +5,7 @@ "batcher_config.dynamic_config.storage_reader_server_dynamic_config.enable": false, "batcher_config.dynamic_config.tx_polling_interval_millis": 200, "batcher_config.dynamic_config.validate_tx_polling_interval_millis": 10, + "batcher_config.dynamic_config.view_call_timeout_millis": 5000, "batcher_config.static_config.block_builder_config.bouncer_config.block_max_capacity.l1_gas": 4400000, "batcher_config.static_config.block_builder_config.bouncer_config.block_max_capacity.message_segment_length": 3700, "batcher_config.static_config.block_builder_config.bouncer_config.block_max_capacity.n_events": "$$$_BATCHER_CONFIG-STATIC_CONFIG-BLOCK_BUILDER_CONFIG-BOUNCER_CONFIG-BLOCK_MAX_CAPACITY-N_EVENTS_$$$", diff --git a/crates/apollo_node/resources/config_schema.json b/crates/apollo_node/resources/config_schema.json index aa81ded1839..75cf8c28b1e 100644 --- a/crates/apollo_node/resources/config_schema.json +++ b/crates/apollo_node/resources/config_schema.json @@ -79,6 +79,11 @@ "privacy": "Public", "value": 10 }, + "batcher_config.dynamic_config.view_call_timeout_millis": { + "description": "Maximal wall time (in milliseconds) a view entry point call may hold the batcher's request slot, which block production shares.", + "privacy": "Public", + "value": 5000 + }, "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/metrics.rs b/crates/blockifier/src/metrics.rs index 541f694166d..9c9887096a0 100644 --- a/crates/blockifier/src/metrics.rs +++ b/crates/blockifier/src/metrics.rs @@ -1,4 +1,4 @@ -use apollo_metrics::metrics::{MetricCounter, MetricDetails, MetricScope}; +use apollo_metrics::metrics::{MetricCounter, MetricDetails}; use apollo_metrics::{define_metrics, generate_permutation_labels}; use crate::bouncer::BouncerWeights; @@ -90,13 +90,11 @@ impl CacheMetrics { self.hits.increment(1); } - pub fn get_scope(&self) -> MetricScope { + pub fn validate_scope(&self) { assert_eq!( self.misses.get_scope(), self.hits.get_scope(), "Scope of misses and hits must be the same" ); - - self.misses.get_scope() } }