starknet_patricia_storage: move is_gatherable to ReadOnlyStorage

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

• starknet_committer: fetch patricia paths concurrently #14283
• starknet_committer: define fetch_contract_storage_paths_concurrently #14282
• starknet_committer: define StoragePathsReadTask #14281
• starknet_patricia_storage: move is_gatherable to ReadOnlyStorage #14280 👈 (View in Graphite)
• apollo_committer,starknet_committer: use blockifier's AccessedKeys #14252
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[cursor]

PR Summary

Low Risk
Trait-only relocation with unchanged gather semantics; limited to Patricia storage trait wiring and impl blocks.

Overview
Moves as_gatherable_storage from Storage to ReadOnlyStorage, so callers that only hold a mutable read-only view (e.g. overlay wrappers, concurrent trie reads) can probe GatherableStorage without requiring write-capable storage.

Each backend and wrapper now implements the hook on ReadOnlyStorage: real backends still return Some(self); ReadsCollectorStorage, TwoLayerStorage, and short-key wrappers return None with documented rationale. CachedStorage’s ReadOnlyStorage impl picks up the same 'static bound already used for GatherableStorage.

^{Reviewed by Cursor Bugbot for commit 22fbbee. Bugbot is set up for automated code reviews on this repo. Configure here.}

[yoavGrs]

@yoavGrs reviewed 8 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArielElp).

---

crates/starknet_patricia_storage/src/map_storage.rs line 305 at r1 (raw file):

}

// TODO(Nimrod): Find a way to share the implementation with `ImmutableReadOnlyStorage`.

Mark as done?

Code quote:

// TODO(Nimrod): Find a way to share the implementation with `ImmutableReadOnlyStorage`.

[ArielElp]

@ArielElp made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on yoavGrs).

---

crates/starknet_patricia_storage/src/map_storage.rs line 305 at r1 (raw file):

Previously, yoavGrs wrote…

Mark as done?

I don't think it's related to the change. This one is about attempting to use the logic of ImmutableReadOnlyStorage's get in get_mut so that it looks something like:

1. immutable get
2. update cache

[yoavGrs]

@yoavGrs resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ArielElp).

[dorimedini-starkware]

@dorimedini-starkware reviewed 8 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).

---

crates/starknet_patricia_storage/src/two_layer_storage.rs line 75 at r1 (raw file):

    fn as_gatherable_storage(&mut self) -> Option<&mut impl GatherableStorage> {
        None::<&mut NullStorage>
    }

add a docstring remarking why this type should not return Some please

Code quote:

    fn as_gatherable_storage(&mut self) -> Option<&mut impl GatherableStorage> {
        None::<&mut NullStorage>
    }

---

crates/starknet_patricia_storage/src/reads_collector_storage.rs line 51 at r1 (raw file):

    fn as_gatherable_storage(&mut self) -> Option<&mut impl GatherableStorage> {
        None::<&mut NullStorage>
    }

am i right?

Suggestion:

    /// This type is intended to be the gather *target*; do not spawn gatherable instances.
    fn as_gatherable_storage(&mut self) -> Option<&mut impl GatherableStorage> {
        None::<&mut NullStorage>
    }

[ArielElp]

@ArielElp made 2 comments.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on dorimedini-starkware and yoavGrs).

---

crates/starknet_patricia_storage/src/reads_collector_storage.rs line 51 at r1 (raw file):

Previously, dorimedini-starkware wrote…

am i right?

I'm not sure wdym by "the gather target". gather has a default implementation that is running tasks and collecting reads via the ReadsCollectorStorage wrapper, but there is nothing inherent in this behavior. Any storage (well, except TwoLayerStorage) can be gatherable; we only did it for types we intend to use in that way.

---

crates/starknet_patricia_storage/src/two_layer_storage.rs line 75 at r1 (raw file):

Previously, dorimedini-starkware wrote…

add a docstring remarking why this type should not return Some please

Done.

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArielElp).

---

crates/starknet_patricia_storage/src/reads_collector_storage.rs line 51 at r1 (raw file):

Previously, ArielElp wrote…

I'm not sure wdym by "the gather target". gather has a default implementation that is running tasks and collecting reads via the ReadsCollectorStorage wrapper, but there is nothing inherent in this behavior. Any storage (well, except TwoLayerStorage) can be gatherable; we only did it for types we intend to use in that way.

we use "gatherable" storage to spawn separate storage readers which separately cache reads, so they can be collected via ReadsCollectorStorage; this is why making ReadCollectorStorage gatherable is redundant, no?

[ArielElp]

@ArielElp made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on dorimedini-starkware).

---

crates/starknet_patricia_storage/src/reads_collector_storage.rs line 51 at r1 (raw file):

Previously, dorimedini-starkware wrote…

we use "gatherable" storage to spawn separate storage readers which separately cache reads, so they can be collected via ReadsCollectorStorage; this is why making ReadCollectorStorage gatherable is redundant, no?

This is what we use it for in practice, but the notion of gatherable is "run a bunch of tasks concurrently". We don't need to do it with ReadsCollectorStorage, as it's just an implementation detail of the default gather logic, but we could do it in the future for some reason.

[dorimedini-starkware]

@dorimedini-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArielElp).

---

crates/starknet_patricia_storage/src/reads_collector_storage.rs line 51 at r1 (raw file):

Previously, ArielElp wrote…

This is what we use it for in practice, but the notion of gatherable is "run a bunch of tasks concurrently". We don't need to do it with ReadsCollectorStorage, as it's just an implementation detail of the default gather logic, but we could do it in the future for some reason.

it would be best to add a remark to explain why it's Null. it can be

/// It's possible to implement [GatherableStorage] on [Self] but it currently has no use case.

[ArielElp]

@ArielElp made 1 comment.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on dorimedini-starkware and yoavGrs).

---

crates/starknet_patricia_storage/src/reads_collector_storage.rs line 51 at r1 (raw file):

Previously, dorimedini-starkware wrote…

it would be best to add a remark to explain why it's Null. it can be

/// It's possible to implement [GatherableStorage] on [Self] but it currently has no use case.

Added, WDYT?

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ArielElp).