starknet_patricia_storage: two layer storage by ArielElp · Pull Request #13990 · starkware-libs/sequencer

ArielElp · 2026-05-07T12:02:28Z

No description provided.

reviewable-StarkWare · 2026-05-07T12:02:38Z

This change is

ArielElp · 2026-05-07T12:02:38Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

cursor · 2026-05-07T14:32:17Z

PR Summary

Low Risk
Primarily additive code introducing a new ReadOnlyStorage wrapper with straightforward fall-through read semantics; limited risk beyond potential subtle ordering/consistency issues in mget_mut miss handling.

Overview
Adds TwoLayerStorage, a new ReadOnlyStorage implementation that composes an in-memory (or otherwise owned) overlay on top of a borrowed immutable base storage, returning overlay values first and falling through to the base on misses (including for batched mget_mut).

Exports the module from lib.rs and includes unit tests validating fall-through behavior, overlay shadowing, delete behavior in the overlay, and miss handling for multi-get.

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

cursor · 2026-05-07T14:34:00Z

+        base.0.insert(key.clone(), val.clone());
+
+        let mut layered = TwoLayerStorage::new(MapStorage::default(), &base);
+        assert_eq!(layered.get_mut(&key).await.unwrap(), Some(val));


Test for mget_mut actually calls get_mut instead

Medium Severity

The test mget_mut_uses_immutable_base_mget_on_miss is named to verify the mget_mut method but actually calls get_mut. This means the mget_mut two-layer fallthrough logic (collecting miss indices, batching base fetches, and merging results) is never exercised by any test, leaving potential bugs in that code path undetected.

^{Reviewed by Cursor Bugbot for commit 2de219b. Configure here.}

That's right.
Rename this test and add tests for mget.

Renamed the test, but this is deliberate, I can't call mut_xxx functions on base.

The functions in trie traversal require ReadOnlyStorage. I want to take an existing storage, wrap it with modifications, and then use it for traversal. To do this without consuming the storage, I need to be able to work with ref, hence I have no access to get_mut (base is ImmutableReadOnly). mget_xxx functions are a fake here. It's documented on the type.

I'm not following. Both get_mut and mget_mut are implemented, but you're only checking the first one here.
It's not related to trie_traversal.

yoavGrs

@yoavGrs reviewed 2 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on ArielElp).

a discussion (no related file):
Just a reminder, what do you need this for?

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

    pub overlay: Overlay,
    pub base: &'a Base,
}

Suggestion:

{
    overlay: Overlay,
    base: &'a Base,
}

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

}

#[cfg(test)]

Move tests to a testing file.

yoavGrs · 2026-05-10T13:13:43Z

+        base.0.insert(key.clone(), val.clone());
+
+        let mut layered = TwoLayerStorage::new(MapStorage::default(), &base);
+        assert_eq!(layered.get_mut(&key).await.unwrap(), Some(val));


That's right.
Rename this test and add tests for mget.

ArielElp

@ArielElp reviewed 1 file and made 3 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on yoavGrs).

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

Previously, yoavGrs wrote…

Move tests to a testing file.

Done.

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

    pub overlay: Overlay,
    pub base: &'a Base,
}

Done.

ArielElp · 2026-05-14T09:15:58Z

+        base.0.insert(key.clone(), val.clone());
+
+        let mut layered = TwoLayerStorage::new(MapStorage::default(), &base);
+        assert_eq!(layered.get_mut(&key).await.unwrap(), Some(val));


Renamed the test, but this is deliberate, I can't call mut_xxx functions on base.

The functions in trie traversal require ReadOnlyStorage. I want to take an existing storage, wrap it with modifications, and then use it for traversal. To do this without consuming the storage, I need to be able to work with ref, hence I have no access to get_mut (base is ImmutableReadOnly). mget_xxx functions are a fake here. It's documented on the type.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 6d22672. Configure here.}

cursor · 2026-05-14T11:58:22Z

+
+    let mut layered = TwoLayerStorage::new(MapStorage::default(), &base);
+    assert_eq!(layered.get_mut(&key).await.unwrap(), Some(val));
+}


Test named mget_mut actually calls get_mut instead

Medium Severity

The test mget_mut_uses_immutable_base_get_on_miss calls layered.get_mut(&key) instead of layered.mget_mut(...). This means the mget_mut code path in TwoLayerStorage — which has distinct logic for collecting miss indices, batch-fetching from base, and merging results — has no test coverage at all. The reviewer explicitly requested mget tests be added, but the current test only exercises get_mut.

^{Reviewed by Cursor Bugbot for commit 6d22672. Configure here.}

yoavGrs

@yoavGrs reviewed all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on ArielElp).

crates/starknet_patricia_storage/src/two_layer_storage.rs line 70 at r2 (raw file):

#[cfg(test)]
#[path = "two_layer_storage_test.rs"]

Move it to the top of the file.

yoavGrs · 2026-05-14T13:03:49Z

+        base.0.insert(key.clone(), val.clone());
+
+        let mut layered = TwoLayerStorage::new(MapStorage::default(), &base);
+        assert_eq!(layered.get_mut(&key).await.unwrap(), Some(val));


I'm not following. Both get_mut and mget_mut are implemented, but you're only checking the first one here.
It's not related to trie_traversal.

ArielElp mentioned this pull request May 7, 2026

starknet_committer: support both layouts in fetch_patricia_paths #13958

Open

ArielElp marked this pull request as ready for review May 7, 2026 14:32

cursor Bot reviewed May 7, 2026

View reviewed changes

yoavGrs requested changes May 10, 2026

View reviewed changes

ArielElp force-pushed the ariel/add_two_layer_storage branch from 2de219b to d746081 Compare May 14, 2026 08:43

ArielElp changed the base branch from ariel/move_tests_module to graphite-base/13990 May 14, 2026 09:03

ArielElp commented May 14, 2026

View reviewed changes

starknet_patricia_storage: two layer storage

6d22672

ArielElp force-pushed the ariel/add_two_layer_storage branch from d746081 to 6d22672 Compare May 14, 2026 11:56

ArielElp force-pushed the graphite-base/13990 branch from 6d471bd to 3f8b451 Compare May 14, 2026 11:56

ArielElp changed the base branch from graphite-base/13990 to ariel/move_tests_module May 14, 2026 11:56

cursor Bot reviewed May 14, 2026

View reviewed changes

yoavGrs requested changes May 14, 2026

View reviewed changes

Conversation

ArielElp commented May 7, 2026

Uh oh!

reviewable-StarkWare commented May 7, 2026

Uh oh!

ArielElp commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

cursor Bot May 7, 2026

Choose a reason for hiding this comment

Test for mget_mut actually calls get_mut instead

Uh oh!

yoavGrs May 10, 2026

Choose a reason for hiding this comment

Uh oh!

ArielElp May 14, 2026

Choose a reason for hiding this comment

Uh oh!

yoavGrs May 14, 2026

Choose a reason for hiding this comment

Uh oh!

yoavGrs left a comment

Choose a reason for hiding this comment

Uh oh!

yoavGrs May 10, 2026

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp May 14, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot May 14, 2026

Choose a reason for hiding this comment

Test named mget_mut actually calls get_mut instead

Uh oh!

yoavGrs left a comment

Choose a reason for hiding this comment

Uh oh!

yoavGrs May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ArielElp commented May 7, 2026 •

edited

Loading

cursor Bot commented May 7, 2026 •

edited

Loading

Test for `mget_mut` actually calls `get_mut` instead

Test named `mget_mut` actually calls `get_mut` instead