Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/apollo_batcher/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ lru.workspace = true
reqwest = { workspace = true, features = ["json"] }
serde.workspace = true
starknet_api.workspace = true
starknet_committer.workspace = true
strum = { workspace = true, features = ["derive"] }
thiserror.workspace = true
tokio.workspace = true
Expand Down
62 changes: 8 additions & 54 deletions crates/apollo_batcher/src/batcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,6 @@ use apollo_storage::partial_block_hash::{
PartialBlockHashComponentsStorageWriter,
};
use apollo_storage::state::{StateStorageReader, StateStorageWriter};
use apollo_storage::state_commitment_infos::{
CompressedStateCommitmentInfos,
StateCommitmentInfosStorageReader,
StateCommitmentInfosStorageWriter,
};
use apollo_storage::storage_reader_server::{
DynamicConfigError,
DynamicConfigProvider,
Expand Down Expand Up @@ -127,6 +122,7 @@ use starknet_api::core::{ContractAddress, GlobalRoot, Nonce, StateDiffCommitment
use starknet_api::state::{StateNumber, ThinStateDiff};
use starknet_api::transaction::fields::Calldata;
use starknet_api::transaction::TransactionHash;
use starknet_committer::patricia_merkle_tree::types::CompressedStateCommitmentInfos;
use tokio::sync::{Mutex, Semaphore};
use tokio::task::AbortHandle;
use tracing::{debug, error, info, instrument, trace, warn, Instrument};
Expand Down Expand Up @@ -1627,33 +1623,20 @@ impl Batcher {
Ok(block_hash)
}

/// Serves the state commitment infos of recently committed heights from memory. `None` means
/// the height is not cached: not committed yet, committed without accessed keys, or older than
/// the cache window.
pub fn get_state_commitment_infos(
&mut self,
block_number: BlockNumber,
) -> BatcherResult<Option<CompressedStateCommitmentInfos>> {
self.get_commitment_results_and_write_to_storage()?;
if let Some(state_commitment_infos) =
self.commitment_manager.recent_state_commitment_infos_cache.get(&block_number)
{
return Ok(Some(state_commitment_infos.clone()));
}

self.storage_reader.get_state_commitment_infos(block_number).map_err(|err| {
error!("Failed to get state commitment infos from storage: {err}");
BatcherError::InternalError
})
Ok(self.commitment_manager.recent_state_commitment_infos_cache.get(&block_number).cloned())
}

pub fn has_state_commitment_infos(&mut self, block_number: BlockNumber) -> BatcherResult<bool> {
self.get_commitment_results_and_write_to_storage()?;
if self.commitment_manager.recent_state_commitment_infos_cache.contains(&block_number) {
return Ok(true);
}

self.storage_reader.has_state_commitment_infos(block_number).map_err(|err| {
error!("Failed to check state commitment infos existence in storage: {err}");
BatcherError::InternalError
})
Ok(self.commitment_manager.recent_state_commitment_infos_cache.contains(&block_number))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache miss stalls commitment infos backfill

Medium Severity

get_state_commitment_infos and has_state_commitment_infos now return a cache miss as None/false for any height outside the in-memory LRU, including heights the committer still has. Consensus walks from the cende recorder offset and stops at the first missing height, so a recorder that lags past the cache window receives no infos at all, including heights that are still cached. Blob backfill then cannot catch up.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4d9c466. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would organize the top 10 elements in the cache in some random order.
Saying block H is in place H-10. We can add up to 10 more elements to the cache before reading H.
So the cache size of 20 solves it.

}

/// Warms the state commitment infos cache from the committer with the heights in
Expand Down Expand Up @@ -1902,14 +1885,6 @@ pub trait BatcherStorageReader: Send + Sync {

fn get_block_hash(&self, height: BlockNumber) -> StorageResult<Option<BlockHash>>;

fn get_state_commitment_infos(
&self,
height: BlockNumber,
) -> StorageResult<Option<CompressedStateCommitmentInfos>>;

/// Returns whether the state commitment infos for the given height are stored.
fn has_state_commitment_infos(&self, height: BlockNumber) -> StorageResult<bool>;

fn get_parent_hash_and_partial_block_hash_components(
&self,
height: BlockNumber,
Expand Down Expand Up @@ -2006,17 +1981,6 @@ impl BatcherStorageReader for StorageReader {
self.begin_ro_txn()?.get_block_hash(&height)
}

fn get_state_commitment_infos(
&self,
height: BlockNumber,
) -> StorageResult<Option<CompressedStateCommitmentInfos>> {
self.begin_ro_txn()?.get_state_commitment_infos(height)
}

fn has_state_commitment_infos(&self, height: BlockNumber) -> StorageResult<bool> {
self.begin_ro_txn()?.has_state_commitment_infos(height)
}

fn get_parent_hash_and_partial_block_hash_components(
&self,
height: BlockNumber,
Expand Down Expand Up @@ -2057,17 +2021,14 @@ pub trait BatcherStorageWriter: Send + Sync {

fn revert_block(&mut self, height: BlockNumber);

/// Sets the global root and block hash (unless it's None) for the given height, and persists
/// the commitment infos (when present) in the same transaction.
/// Sets the global root and block hash (unless it's None) for the given height.
/// Increments the block hash marker by 1.
/// Block hash is optional because for old blocks, the block hash was set separately.
/// Commitment infos are optional for blocks that doesn't come from decision_reached flow.
fn set_global_root_and_block_hash(
&mut self,
height: BlockNumber,
global_root: GlobalRoot,
block_hash: Option<BlockHash>,
state_commitment_infos: Option<CompressedStateCommitmentInfos>,
) -> StorageResult<()>;

fn set_block_hash(&mut self, height: BlockNumber, block_hash: BlockHash) -> StorageResult<()>;
Expand Down Expand Up @@ -2112,14 +2073,10 @@ impl BatcherStorageWriter for StorageWriter {
height: BlockNumber,
global_root: GlobalRoot,
block_hash: Option<BlockHash>,
state_commitment_infos: Option<CompressedStateCommitmentInfos>,
) -> StorageResult<()> {
info!(
"Setting global root and block hash for height {height}. Root: {global_root:?}, Block \
hash: {block_hash:?}, compressed commitment infos byte length: {:?}.",
state_commitment_infos
.as_ref()
.map(|state_commitment_infos| state_commitment_infos.payload.0.len())
hash: {block_hash:?}."
);
let mut txn = self
.begin_rw_txn()?
Expand All @@ -2128,9 +2085,6 @@ impl BatcherStorageWriter for StorageWriter {
if let Some(block_hash) = block_hash {
txn = txn.set_block_hash(&height, block_hash)?;
}
if let Some(state_commitment_infos) = state_commitment_infos {
txn = txn.append_state_commitment_infos(height, &state_commitment_infos)?;
}
txn.commit()
}

Expand Down
8 changes: 4 additions & 4 deletions crates/apollo_batcher/src/batcher_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1649,7 +1649,7 @@ async fn revert_block() {

// The commit of the reverted height is still pending when the revert is requested, so its
// result is written, and cached, before the storage is reverted.
storage_writer.expect_set_global_root_and_block_hash().times(1).returning(|_, _, _, _| Ok(()));
storage_writer.expect_set_global_root_and_block_hash().times(1).returning(|_, _, _| Ok(()));
let mut storage_reader = mock_storage_reader_for_revert();
storage_reader
.expect_get_parent_hash_and_partial_block_hash_components()
Expand Down Expand Up @@ -2045,8 +2045,8 @@ async fn get_block_hash_after_reading_commitment_results() {
mock_dependencies.storage_writer.expect_set_global_root_and_block_hash();
set_global_root_expectation.times(1);
set_global_root_expectation
.with(eq(INITIAL_HEIGHT), eq(global_root), always(), always())
.returning(|_, _, _, _| Ok(()));
.with(eq(INITIAL_HEIGHT), eq(global_root), always())
.returning(|_, _, _| Ok(()));

let mut batcher = create_batcher(mock_dependencies).await;

Expand Down Expand Up @@ -2087,7 +2087,7 @@ async fn get_state_commitment_infos_after_reading_commitment_results() {
.storage_writer
.expect_set_global_root_and_block_hash()
.times(1)
.returning(|_, _, _, _| Ok(()));
.returning(|_, _, _| Ok(()));

let mut batcher = create_batcher(mock_dependencies).await;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use apollo_committer_types::committer_types::{
};
use apollo_committer_types::communication::{CommitterRequestLabelValue, SharedCommitterClient};
use apollo_storage::accessed_keys::AccessedKeys as StorageAccessedKeys;
use apollo_storage::state_commitment_infos::CompressedStateCommitmentInfos;
use lru::LruCache;
use starknet_api::block::{BlockHash, BlockNumber};
use starknet_api::block_hash::block_hash_calculator::{
Expand All @@ -23,6 +22,7 @@ use starknet_api::block_hash::block_hash_calculator::{
};
use starknet_api::core::StateDiffCommitment;
use starknet_api::state::ThinStateDiff;
use starknet_committer::patricia_merkle_tree::types::CompressedStateCommitmentInfos;
use tokio::sync::mpsc::error::{TryRecvError, TrySendError};
use tokio::sync::mpsc::{channel, Receiver, Sender};
use tokio::time::{sleep, Duration};
Expand Down Expand Up @@ -310,17 +310,12 @@ impl<S: StateCommitterTrait> CommitmentManager<S> {
}

// Add commitment infos to cache.
if let Some(state_commitment_infos) = state_commitment_infos.clone() {
if let Some(state_commitment_infos) = state_commitment_infos {
self.recent_state_commitment_infos_cache.put(height, state_commitment_infos);
}

// Write the block hash and global root to storage.
storage_writer.set_global_root_and_block_hash(
height,
global_root,
block_hash,
state_commitment_infos,
)?;
storage_writer.set_global_root_and_block_hash(height, global_root, block_hash)?;
GLOBAL_ROOT_HEIGHT.increment(1);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,9 +393,7 @@ async fn test_add_task_wait_for_full_channel(mut mock_dependencies: MockDependen
let set_global_root_expectation =
mock_dependencies.storage_writer.expect_set_global_root_and_block_hash();
set_global_root_expectation.times(expected_n_calls);
set_global_root_expectation
.withf(move |h, _, _, _| *h == height)
.returning(|_, _, _, _| Ok(()));
set_global_root_expectation.withf(move |h, _, _| *h == height).returning(|_, _, _| Ok(()));
}

let (mut commitment_manager, storage_reader, mut storage_writer) =
Expand Down
2 changes: 1 addition & 1 deletion crates/apollo_batcher/src/commitment_manager/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ use apollo_committer_types::committer_types::{
RevertBlockResponse,
};
use apollo_committer_types::communication::CommitterRequestLabelValue;
use apollo_storage::state_commitment_infos::CompressedStateCommitmentInfos;
use starknet_api::block::{BlockHash, BlockNumber};
use starknet_api::core::GlobalRoot;
use starknet_committer::patricia_merkle_tree::types::CompressedStateCommitmentInfos;
use tracing::warn;

/// Input for commitment tasks.
Expand Down
10 changes: 5 additions & 5 deletions crates/apollo_batcher/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ use apollo_l1_events_types::MockL1EventsProviderClient;
use apollo_mempool_types::communication::MockMempoolClient;
use apollo_mempool_types::mempool_types::CommitBlockArgs;
use apollo_storage::accessed_keys::AccessedKeys;
use apollo_storage::state_commitment_infos::{
CompressedPayload,
CompressedStateCommitmentInfos,
STATE_COMMITMENT_INFOS_VERSION,
};
use async_trait::async_trait;
use blockifier::blockifier::transaction_executor::BlockExecutionSummary;
use blockifier::bouncer::{BouncerWeights, CasmHashComputationData};
Expand All @@ -43,6 +38,11 @@ use starknet_api::test_utils::l1_handler::{executable_l1_handler_tx, L1HandlerTx
use starknet_api::transaction::fields::{Fee, TransactionSignature};
use starknet_api::transaction::TransactionHash;
use starknet_api::{class_hash, contract_address, nonce, tx_hash};
use starknet_committer::patricia_merkle_tree::types::{
CompressedPayload,
CompressedStateCommitmentInfos,
STATE_COMMITMENT_INFOS_VERSION,
};
use starknet_types_core::felt::Felt;
use tokio::sync::mpsc::{Receiver, Sender, UnboundedSender};
use tokio::time::sleep;
Expand Down
7 changes: 1 addition & 6 deletions crates/apollo_reverts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use apollo_storage::global_root::GlobalRootStorageWriter;
use apollo_storage::header::HeaderStorageWriter;
use apollo_storage::partial_block_hash::PartialBlockHashComponentsStorageWriter;
use apollo_storage::state::StateStorageWriter;
use apollo_storage::state_commitment_infos::StateCommitmentInfosStorageWriter;
use apollo_storage::StorageWriter;
use futures::future::pending;
use futures::never::Never;
Expand Down Expand Up @@ -146,11 +145,7 @@ pub fn revert_block(storage_writer: &mut StorageWriter, target_block_marker: Blo
.revert_global_root(&target_block_marker)
.unwrap();

let txn = txn
.revert_accessed_keys(target_block_marker)
.unwrap()
.revert_state_commitment_infos(target_block_marker)
.unwrap();
let txn = txn.revert_accessed_keys(target_block_marker).unwrap();

txn.commit().unwrap();
}
1 change: 0 additions & 1 deletion crates/apollo_storage/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true, features = ["arbitrary_precision"] }
starknet-types-core = { workspace = true, features = ["papyrus-serialization"] }
starknet_api.workspace = true
starknet_committer.workspace = true
tempfile = { workspace = true, optional = true }
thiserror.workspace = true
tokio = { workspace = true, features = ["rt-multi-thread"] }
Expand Down
Loading
Loading