Skip to content

starknet_patricia_storage: always cache writes in cached storage #12443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nimrod-starkware wants to merge 1 commit into
base: nimrod/parallel-reads/flush-siblings-to-storage
Choose a base branch
from
+2 −31
Open

starknet_patricia_storage: always cache writes in cached storage #12443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 0 additions & 1 deletion crates/apollo_deployments/resources/app_configs/committer_config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"committer_config.db_path": "/data/committer",
"committer_config.reader_config.warn_on_trivial_modifications": false,
"committer_config.reader_config.build_storage_tries_concurrently": false,
"committer_config.storage_config.cache_on_write": true,
"committer_config.storage_config.cache_size": 1000000,
"committer_config.storage_config.include_inner_stats": true,
"committer_config.storage_config.inner_storage_config.bloom_filter_bits": 10,
Expand Down
1 change: 0 additions & 1 deletion crates/apollo_deployments/resources/app_configs/replacer_committer_config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"committer_config.db_path": "/data/committer",
"committer_config.reader_config.warn_on_trivial_modifications": false,
"committer_config.reader_config.build_storage_tries_concurrently": false,
"committer_config.storage_config.cache_on_write": true,
"committer_config.storage_config.cache_size": "$$$_COMMITTER_CONFIG-STORAGE_CONFIG-CACHE_SIZE_$$$",
"committer_config.storage_config.include_inner_stats": true,
"committer_config.storage_config.inner_storage_config.bloom_filter_bits": 10,
Expand Down
5 changes: 0 additions & 5 deletions crates/apollo_node/resources/config_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,11 +519,6 @@
"privacy": "Public",
"value": false
},
"committer_config.storage_config.cache_on_write": {
"description": "If true, the cache is updated on write operations",
"privacy": "Public",
"value": true
},
"committer_config.storage_config.cache_size": {
"description": "Max number of entries in the cache",
"privacy": "Public",
Expand Down
1 change: 0 additions & 1 deletion crates/starknet_committer_cli/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,6 @@ impl<InnerStorageArgs: StorageFromArgs + Sync> StorageFromArgs
fn storage_config(&self) -> <Self::Storage as Storage>::Config {
CachedStorageConfig {
cache_size: NonZeroUsize::new(self.cache_size).unwrap(),
cache_on_write: true,
include_inner_stats: self.include_inner_stats,
inner_storage_config: self.storage_args.storage_config(),
}
Expand Down
24 changes: 2 additions & 22 deletions crates/starknet_patricia_storage/src/map_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ impl Storage for MapStorage {
pub struct CachedStorage<S: Storage> {
pub storage: S,
pub cache: LruCache<DbKey, Option<DbValue>>,
pub cache_on_write: bool,
reads: AtomicU64,
cached_reads: AtomicU64,
writes: u128,
Expand All @@ -88,9 +87,6 @@ pub struct CachedStorageConfig<InnerStorageConfig: StorageConfigTrait> {
// Max number of entries in the cache.
pub cache_size: NonZeroUsize,

// If true, the cache is updated on write operations even if the value is not in the cache.
pub cache_on_write: bool,

// If true, the inner stats are included when collecting statistics.
pub include_inner_stats: bool,

Expand All @@ -102,7 +98,6 @@ impl<InnerStorageConfig: StorageConfigTrait> Default for CachedStorageConfig<Inn
fn default() -> Self {
Self {
cache_size: NonZeroUsize::new(DEFAULT_CACHE_SIZE).unwrap(),
cache_on_write: true,
include_inner_stats: true,
inner_storage_config: InnerStorageConfig::default(),
}
Expand All @@ -126,12 +121,6 @@ impl<InnerStorageConfig: StorageConfigTrait> SerializeConfig
"Max number of entries in the cache",
ParamPrivacyInput::Public,
),
ser_param(
"cache_on_write",
&self.cache_on_write,
"If true, the cache is updated on write operations",
ParamPrivacyInput::Public,
),
ser_param(
"include_inner_stats",
&self.include_inner_stats,
Expand Down Expand Up @@ -204,20 +193,13 @@ impl<S: Storage> CachedStorage<S> {
Self {
storage,
cache: LruCache::new(config.cache_size),
cache_on_write: config.cache_on_write,
reads: AtomicU64::new(0),
cached_reads: AtomicU64::new(0),
writes: 0,
include_inner_stats: config.include_inner_stats,
}
}

fn update_cached_value(&mut self, key: &DbKey, value: &DbValue) {
if self.cache_on_write || self.cache.contains(key) {
self.cache.put(key.clone(), Some(value.clone()));
}
}

pub fn total_writes(&self) -> u128 {
self.writes
}
Expand All @@ -241,7 +223,7 @@ impl<S: Storage> Storage for CachedStorage<S> {
async fn set(&mut self, key: DbKey, value: DbValue) -> PatriciaStorageResult<()> {
self.writes += 1;
self.storage.set(key.clone(), value.clone()).await?;
self.update_cached_value(&key, &value);
self.cache.put(key, Some(value));
Ok(())
}

Expand Down Expand Up @@ -276,9 +258,7 @@ impl<S: Storage> Storage for CachedStorage<S> {
async fn mset(&mut self, key_to_value: DbHashMap) -> PatriciaStorageResult<()> {
self.writes += u128::try_from(key_to_value.len()).expect("usize should fit in u128");
self.storage.mset(key_to_value.clone()).await?;
key_to_value.iter().for_each(|(key, value)| {
self.update_cached_value(key, value);
});
self.flush_to_cache(key_to_value);
Copy link

@cursor cursor bot Feb 19, 2026

Choose a reason for hiding this comment

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

Ignored Result from flush_to_cache in mset

Medium Severity

The call to self.flush_to_cache(key_to_value) in mset discards the PatriciaStorageResult<()> return value without propagating it with ?. The other call site in forest_trait.rs correctly propagates errors via storage.flush_to_cache(all_siblings)?. If the flush_to_cache implementation ever returns an error, mset would silently swallow it and report success to the caller despite the cache being in an inconsistent state.

Fix in Cursor Fix in Web

Ok(())
}

Expand Down
1 change: 0 additions & 1 deletion crates/starknet_patricia_storage/src/map_storage_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use crate::storage_trait::{DbKey, DbValue, EmptyStorageConfig, Storage};
#[case::cached_storage(
CachedStorage::new(MapStorage::default(), CachedStorageConfig {
cache_size: NonZeroUsize::new(2).unwrap(),
cache_on_write: true,
include_inner_stats: false,
inner_storage_config: EmptyStorageConfig::default(),
})
Expand Down
Loading