Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

Copy link
Collaborator Author

dorimedini-starkware commented Oct 29, 2025

@reviewable-StarkWare
Copy link

This change is Reviewable

@dorimedini-starkware dorimedini-starkware self-assigned this Oct 29, 2025
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review October 29, 2025 14:36
@dorimedini-starkware dorimedini-starkware force-pushed the 10-29-starknet_committer_cli_add_option_for_key_size branch 3 times, most recently from e445ff3 to 7ccd12c Compare October 29, 2025 14:58
@dorimedini-starkware dorimedini-starkware force-pushed the 10-29-starknet_committer_cli_use_box_dyn_storage_for_storage_init branch from db97ed3 to d9a8059 Compare October 29, 2025 19:42
@dorimedini-starkware dorimedini-starkware force-pushed the 10-29-starknet_committer_cli_add_option_for_key_size branch from 7ccd12c to 58e0cf2 Compare October 29, 2025 19:42
Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

@nimrod-starkware reviewed 2 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rotem-starkware)


crates/starknet_patricia_storage/src/short_key_storage.rs line 48 at r2 (raw file):

                size => panic!("Invalid key size {size}."),
            }
        }

Please mention that the given sizes need to be in [16, 32]

Code quote:

        /// Wrap an existing storage implementation in a boxed short key storage implementation.
        /// If no size is given, boxes and returns the original storage.
        /// Panics if the size is not within the allowed range.
        pub fn wrap_storage_or_panic<S: Storage + 'static>(
            size: Option<u8>,
            storage: S,
        ) -> Box<dyn Storage> {
            let Some(size) = size else {
                return Box::new(storage);
            };
            match size {
                $( $sizes => Box::new($names::new(storage)), )+
                size => panic!("Invalid key size {size}."),
            }
        }

@dorimedini-starkware dorimedini-starkware force-pushed the 10-29-starknet_committer_cli_add_option_for_key_size branch from 58e0cf2 to 514bd18 Compare October 30, 2025 10:31
@dorimedini-starkware dorimedini-starkware changed the base branch from 10-29-starknet_committer_cli_use_box_dyn_storage_for_storage_init to 10-30-starknet_committer_cli_reduce_boilerplate_without_using_box_dyn_storage_ October 30, 2025 10:31
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @rotem-starkware)


crates/starknet_patricia_storage/src/short_key_storage.rs line 48 at r2 (raw file):

Previously, nimrod-starkware wrote…

Please mention that the given sizes need to be in [16, 32]

I prefer to keep a single point of truth... if I change the 16..=32 range in the short_key module I will not remember to change the panic message (and if someone else adds new byte ranges, even worse)

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @rotem-starkware)


crates/starknet_committer_cli/src/bin/benchmark_run/Dockerfile line 35 at r4 (raw file):

    "--n-diffs", "4000", \
    "--storage-type", "cached-mdbx", \
    "--key-size", "16", \

should we use this default? or keep it None?

Code quote:

"--key-size", "16", \

@dorimedini-starkware dorimedini-starkware force-pushed the 10-30-starknet_committer_cli_reduce_boilerplate_without_using_box_dyn_storage_ branch from 3b43ea2 to 964dec5 Compare October 30, 2025 13:48
@dorimedini-starkware dorimedini-starkware force-pushed the 10-29-starknet_committer_cli_add_option_for_key_size branch from 514bd18 to 677fbb0 Compare October 30, 2025 13:48
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @rotem-starkware)


crates/starknet_patricia_storage/src/short_key_storage.rs line 48 at r2 (raw file):

Previously, dorimedini-starkware wrote…

I prefer to keep a single point of truth... if I change the 16..=32 range in the short_key module I will not remember to change the panic message (and if someone else adds new byte ranges, even worse)

the next PR should address your concerns

Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

@nimrod-starkware reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rotem-starkware)


crates/starknet_committer_cli/src/bin/benchmark_run/Dockerfile line 35 at r4 (raw file):

Previously, dorimedini-starkware wrote…

should we use this default? or keep it None?

I think the default should be None, @rotem-starkware wdyt?

@dorimedini-starkware dorimedini-starkware force-pushed the 10-29-starknet_committer_cli_add_option_for_key_size branch from 677fbb0 to 25e7965 Compare October 30, 2025 15:18
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @rotem-starkware)


crates/starknet_committer_cli/src/bin/benchmark_run/Dockerfile line 35 at r4 (raw file):

Previously, nimrod-starkware wrote…

I think the default should be None, @rotem-starkware wdyt?

Done.

Copy link
Contributor

@rotem-starkware rotem-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@rotem-starkware reviewed 1 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

@dorimedini-starkware dorimedini-starkware force-pushed the 10-29-starknet_committer_cli_add_option_for_key_size branch from 25e7965 to 3a7cf7e Compare November 2, 2025 14:47
@dorimedini-starkware dorimedini-starkware changed the base branch from 10-30-starknet_committer_cli_reduce_boilerplate_without_using_box_dyn_storage_ to main-v0.14.1-committer November 2, 2025 14:47
Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@nimrod-starkware reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Nov 3, 2025
Merged via the queue into main-v0.14.1-committer with commit 302de54 Nov 3, 2025
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants