starknet_committer: add index layout leaves by ArielElp · Pull Request #10681 · starkware-libs/sequencer

ArielElp · 2025-12-10T08:19:49Z

No description provided.

reviewable-StarkWare · 2025-12-10T08:19:59Z

This change is

ArielElp · 2025-12-10T08:20:01Z

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

nimrod-starkware

@nimrod-starkware reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ArielElp and @yoavGrs)

crates/starknet_committer/src/db/index_db/leaves.rs line 50 at r1 (raw file):

Previously, ArielElp wrote…

Yes, derive_more::AsRef does this, I can also remove all the From implementations with derive_more::From

nice!
please use it below too (added a comment)

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_impl.rs line 27 at r1 (raw file):

Previously, ArielElp wrote…

See above

same here

crates/starknet_committer/src/db/index_db/leaves.rs line 50 at r4 (raw file):

}

// TODO(Ariel): Remove this macro  when HasStaticPrefix is a local trait. Replace with a blanket

WDYM by local trait?
What is the plan here?

Code quote:

Remove this macro  when HasStaticPrefix is a local trait

crates/starknet_committer/src/db/index_db/leaves.rs line 127 at r4 (raw file):

        serialize_felts(vec![self.0.0])
    }
    fn deserialize(

Suggestion:

    }
    
    fn deserialize(

crates/starknet_committer/src/db/index_db/leaves.rs line 143 at r4 (raw file):

        serialize_felts(vec![self.0.0])
    }
    fn deserialize(

Suggestion:

    }
    
    fn deserialize(

crates/starknet_committer/src/db/index_db/leaves.rs line 156 at r4 (raw file):

fn deserialize_felt(cursor: &mut &[u8], value: &DbValue) -> Result<Felt, DeserializationError> {
    Felt::deserialize(cursor).ok_or(DeserializationError::FeltDeserializationError(value.clone()))
}

You should take only one arg here, not sure if my suggestion is the right way to do it.

Suggestion:

fn deserialize_felt(value: &mut DbValue) -> Result<Felt, DeserializationError> {
    Felt::deserialize(&value.0[..]).ok_or(DeserializationError::FeltDeserializationError(value.clone()))
}

crates/starknet_committer/src/db/index_db/leaves.rs line 218 at r4 (raw file):

                .unwrap();
        assert_eq!(index_layout_starknet_storage_value, deserialized);
    }

move the test module into a separate file leaves_test.rs - this is our convention

Code quote:

#[cfg(test)]
mod leaves_serde_tests {
    use starknet_api::core::{ClassHash, Nonce};
    use starknet_api::hash::HashOutput;
    use starknet_patricia_storage::db_object::{DBObject, EmptyDeserializationContext};
    use starknet_types_core::felt::Felt;

    use crate::block_committer::input::StarknetStorageValue;
    use crate::db::index_db::leaves::{
        IndexLayoutCompiledClassHash,
        IndexLayoutContractState,
        IndexLayoutStarknetStorageValue,
    };
    use crate::patricia_merkle_tree::leaf::leaf_impl::ContractState;
    use crate::patricia_merkle_tree::types::CompiledClassHash;

    #[test]
    fn test_index_layout_contract_state_serde() {
        let contract_state = ContractState {
            class_hash: ClassHash(Felt::from(1)),
            storage_root_hash: HashOutput(Felt::from(2)),
            nonce: Nonce(Felt::from(3)),
        };
        let index_layout_contract_state = IndexLayoutContractState(contract_state);
        let serialized = index_layout_contract_state.serialize().unwrap();
        let deserialized =
            IndexLayoutContractState::deserialize(&serialized, &EmptyDeserializationContext)
                .unwrap();
        assert_eq!(index_layout_contract_state, deserialized);
    }

    #[test]
    fn test_index_layout_compiled_class_hash_serde() {
        let compiled_class_hash = CompiledClassHash(Felt::from(1));
        let index_layout_compiled_class_hash = IndexLayoutCompiledClassHash(compiled_class_hash);
        let serialized = index_layout_compiled_class_hash.serialize().unwrap();
        let deserialized =
            IndexLayoutCompiledClassHash::deserialize(&serialized, &EmptyDeserializationContext)
                .unwrap();
        assert_eq!(index_layout_compiled_class_hash, deserialized);
    }

    #[test]
    fn test_index_layout_starknet_storage_value_serde() {
        let starknet_storage_value = StarknetStorageValue(Felt::from(1));
        let index_layout_starknet_storage_value =
            IndexLayoutStarknetStorageValue(starknet_storage_value);
        let serialized = index_layout_starknet_storage_value.serialize().unwrap();
        let deserialized =
            IndexLayoutStarknetStorageValue::deserialize(&serialized, &EmptyDeserializationContext)
                .unwrap();
        assert_eq!(index_layout_starknet_storage_value, deserialized);
    }

crates/starknet_committer/src/db/index_db/leaves.rs line 218 at r4 (raw file):

                .unwrap();
        assert_eq!(index_layout_starknet_storage_value, deserialized);
    }

Can you make these generic in L to reduce code duplication?

Suggestion:

    #[rstest]
    #[case(IndexLayoutContractState(ContractState { 
            class_hash: ClassHash(Felt::from(1)),
            storage_root_hash: HashOutput(Felt::from(2)),
            nonce: Nonce(Felt::from(3)),}
    ))
    #[case(IndexLayoutCompiledClassHash(CompiledClassHash(Felt::from(1)))
    .....
    fn test_layout_leaf_serde<L: Leaf>(#[case] leaf: L) {
        let serialized = leaf.serialize().unwrap();
        let deserialized =
            IndexLayoutContractState::deserialize(&serialized, &EmptyDeserializationContext)
                .unwrap();
        assert_eq!(index_layout_contract_state, deserialized);
    }

crates/starknet_committer/src/patricia_merkle_tree/types.rs line 30 at r4 (raw file):

        self
    }
}

use derive more please

Code quote:

impl AsRef<CompiledClassHash> for CompiledClassHash {
    fn as_ref(&self) -> &CompiledClassHash {
        self
    }
}

ArielElp

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)

crates/starknet_committer/src/db/index_db/leaves.rs line 50 at r4 (raw file):

Previously, nimrod-starkware wrote…

WDYM by local trait?
What is the plan here?

Ideally I would do this:

trait IndexLayoutStaticPrefix;
impl<T: IndexLayoutStaticPrefix> HasStaticPrefix for T {
    ...
}
impl IndexLayoutStaticPrefix for IndexLayoutContractState {}
impl IndexLayoutStaticPrefix for IndexLayoutCompiledClassHash {}
impl IndexLayoutStaticPrefix for IndexLayoutStarknetStorageValue {}

nicer than a new macro IMO, but I can't since HasStaticPrefix is foreign (defined in starknet_patricia_storage), so I can't implement it for a generic T, only for types defined in the committer crate.

Maybe it's an overkill and we'll never move it, at the time it made more sense to me to have the db_object.rs traits in the committer, but maybe this is a bad idea and I should just remove this comment.

crates/starknet_committer/src/patricia_merkle_tree/types.rs line 30 at r4 (raw file):

Previously, nimrod-starkware wrote…

use derive more please

I don't think that I can here, I only do it to return myself, not for the wrapper type.

The motivation is related to leaf hashes in the next PR (avoid duplicating the hash logic for different layouts as much as possible).

Maybe I should just delete here and potentially re-introduce in the next PR?

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_impl.rs line 27 at r1 (raw file):

Previously, nimrod-starkware wrote…

same here

See above

crates/starknet_patricia_storage/src/storage_trait.rs line 175 at r3 (raw file):

Previously, ArielElp wrote…

I disagree here, the static is because HasStaticPrefix doesn't require self. There are two notions of "static" that don't necessarily collide: lifetime of prefix, and needing the node to determine the prefix.

Facts layout: all prefixes have static lifetime (it's always constants), but FilledNode implements HasDynamicPrefix since we need to check if the node is a leaf or not before choosing the correct prefix.

Index layout: some prefixes have static lifetime (contract state and compiled classes), others don't (storage, due to contract address prefix), but we never need the node to determine the prefix. Thus, for index layout, both leaves and nodes have static prefixes (don't depend on self) but the lifetime is sometimes static and sometimes not.

I have a stash for DbKeyPrefix that allows both references and owned data on initialization. This involves a lifetime var on the get_prefix method in HasXXXPrefix traits (trait's generics themselves don't have to change). Overall it's quite a bit of changes, let's discuss tomorrow and decide whether we want them in a separate PR to save those copies or just go with Vec.

Done via Cow in the previous PR

ArielElp

Reviewable status: 6 of 9 files reviewed, 13 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)

crates/starknet_committer/src/db/index_db/leaves.rs line 156 at r4 (raw file):

Previously, nimrod-starkware wrote…

You should take only one arg here, not sure if my suggestion is the right way to do it.

I don't think that's what I want: when I have more than one felt (ContractState), I work with one buffer (initialized to the DbValue), and each deserialize consumes a part of the buffer.

crates/starknet_committer/src/db/index_db/leaves.rs line 218 at r4 (raw file):

Previously, nimrod-starkware wrote…

move the test module into a separate file leaves_test.rs - this is our convention

Done.

crates/starknet_committer/src/db/index_db/leaves.rs line 218 at r4 (raw file):

Previously, nimrod-starkware wrote…

Can you make these generic in L to reduce code duplication?

Done.

crates/starknet_committer/src/db/index_db/leaves.rs line 127 at r4 (raw file):

        serialize_felts(vec![self.0.0])
    }
    fn deserialize(

Done.

crates/starknet_committer/src/db/index_db/leaves.rs line 143 at r4 (raw file):

        serialize_felts(vec![self.0.0])
    }
    fn deserialize(

Done.

yoavGrs

@yoavGrs reviewed 3 files and all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ArielElp and @nimrod-starkware).

crates/starknet_committer/src/db/index_db/leaves.rs line 114 at r5 (raw file):

        let storage_root_hash = deserialize_felt(&mut cursor, value)?;
        let nonce = deserialize_felt(&mut cursor, value)?;
        Ok(IndexLayoutContractState(ContractState {

3 times

Suggestion:

Ok(Self(ContractState {

nimrod-starkware

@nimrod-starkware reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp)

crates/starknet_committer/src/db/index_db/leaves.rs line 50 at r4 (raw file):

Previously, ArielElp wrote…

Ideally I would do this:
trait IndexLayoutStaticPrefix;
impl<T: IndexLayoutStaticPrefix> HasStaticPrefix for T {
    ...
}
impl IndexLayoutStaticPrefix for IndexLayoutContractState {}
impl IndexLayoutStaticPrefix for IndexLayoutCompiledClassHash {}
impl IndexLayoutStaticPrefix for IndexLayoutStarknetStorageValue {}
nicer than a new macro IMO, but I can't since HasStaticPrefix is foreign (defined in starknet_patricia_storage), so I can't implement it for a generic T, only for types defined in the committer crate.

Maybe it's an overkill and we'll never move it, at the time it made more sense to me to have the db_object.rs traits in the committer, but maybe this is a bad idea and I should just remove this comment.

I'm ok with this macro

crates/starknet_committer/src/db/index_db/leaves.rs line 156 at r4 (raw file):

Previously, ArielElp wrote…

I don't think that's what I want: when I have more than one felt (ContractState), I work with one buffer (initialized to the DbValue), and each deserialize consumes a part of the buffer.

In general, passing an arg that is used only to return an error is not a good practice.
Maybe for the contract state you can do something like:

 let (Some(nonce), Some(storage_root_hash), Some(class_hash) =
      (Felt::deserialize(&mut cursor), Felt::deserialize(&mut cursor), Felt::deserialize(&mut cursor)) else {
        return Err(DeserializationError::FeltDeserializationError(value.clone()))
 } Ok(Self {...})

crates/starknet_committer/src/patricia_merkle_tree/types.rs line 30 at r4 (raw file):

Previously, ArielElp wrote…

I don't think that I can here, I only do it to return myself, not for the wrapper type.

The motivation is related to leaf hashes in the next PR (avoid duplicating the hash logic for different layouts as much as possible).

Maybe I should just delete here and potentially re-introduce in the next PR?

Yes, please remove it if it's not needed for this PR

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_impl.rs line 27 at r1 (raw file):

Previously, ArielElp wrote…

See above

same, please remove if not needed now

nimrod-starkware

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp)

crates/starknet_committer/src/db/index_db/leaves.rs line 50 at r4 (raw file):

Previously, nimrod-starkware wrote…

I'm ok with this macro

If you decide to keep this todo, then replace "when HasStaticPrefix is a local trait" with "when HasStaticPrefix is defined in this crate" as it unclear for me.

ArielElp

@ArielElp made 4 comments.
Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

crates/starknet_committer/src/db/index_db/leaves.rs line 156 at r4 (raw file):

Previously, nimrod-starkware wrote…

In general, passing an arg that is used only to return an error is not a good practice.
Maybe for the contract state you can do something like:
 let (Some(nonce), Some(storage_root_hash), Some(class_hash) =
      (Felt::deserialize(&mut cursor), Felt::deserialize(&mut cursor), Felt::deserialize(&mut cursor)) else {
        return Err(DeserializationError::FeltDeserializationError(value.clone()))
 } Ok(Self {...})

Ah didn't realize your issue is with value, WDYT now?

crates/starknet_committer/src/db/index_db/leaves.rs line 114 at r5 (raw file):

Previously, yoavGrs wrote…

3 times

Done.

crates/starknet_committer/src/patricia_merkle_tree/types.rs line 30 at r4 (raw file):

Previously, nimrod-starkware wrote…

Yes, please remove it if it's not needed for this PR

Done.

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_impl.rs line 27 at r1 (raw file):

Previously, nimrod-starkware wrote…

same, please remove if not needed now

Done.

nimrod-starkware

@nimrod-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @ArielElp and @yoavGrs).

crates/starknet_committer/src/db/index_db/leaves.rs line 156 at r4 (raw file):

Previously, ArielElp wrote…

Ah didn't realize your issue is with value, WDYT now?

I think that currently the error will contain the entire DbValue who can contain 3 felts.
And we will get the same error and won't be able to tell which deserialization failed.
WDYT of this?

fn deserialize_felt(cursor: &mut &[u8]) -> Result<Felt, DeserializationError> {
    Felt::deserialize(cursor).ok_or_else(|| DeserializationError::FeltDeserializationError(DbValue(cursor.to_vec())))

note also the ok_or_else, ok_or is lazy and always clones (also when there is no error)

ArielElp

@ArielElp made 1 comment.
Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

crates/starknet_committer/src/db/index_db/leaves.rs line 156 at r4 (raw file):

Previously, nimrod-starkware wrote…

I think that currently the error will contain the entire DbValue who can contain 3 felts.
And we will get the same error and won't be able to tell which deserialization failed.
WDYT of this?
fn deserialize_felt(cursor: &mut &[u8]) -> Result<Felt, DeserializationError> {
    Felt::deserialize(cursor).ok_or_else(|| DeserializationError::FeltDeserializationError(DbValue(cursor.to_vec())))
note also the ok_or_else, ok_or is lazy and always clones (also when there is no error)

I think your suggestion will show the cursor after some bytes were removed due to deserialization

We're using it for 3 felts, I think it's fine to see the entire value on which deserialization failed so we get more context (otherwise we may not be able to tell this is supposed to be ContractState)

yoavGrs

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

nimrod-starkware

@nimrod-starkware made 1 comment and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

dorimedini-starkware

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

crates/starknet_committer/src/db/index_db/leaves.rs line 41 at r8 (raw file):

        match self {
            TrieType::ContractsTrie => DbKeyPrefix::new(b"CONTRACTS_TREE_PREFIX".into()),
            TrieType::ClassesTrie => DbKeyPrefix::new(b"CLASSES_TREE_PREFIX".into()),

didn't we want these to be MAX_CONTRACT_ADDRESS + X?
also for metadata prefixes?

Code quote:

            TrieType::ContractsTrie => DbKeyPrefix::new(b"CONTRACTS_TREE_PREFIX".into()),
            TrieType::ClassesTrie => DbKeyPrefix::new(b"CLASSES_TREE_PREFIX".into()),

crates/starknet_committer/src/db/index_db/leaves.rs line 42 at r8 (raw file):

            TrieType::ContractsTrie => DbKeyPrefix::new(b"CONTRACTS_TREE_PREFIX".into()),
            TrieType::ClassesTrie => DbKeyPrefix::new(b"CLASSES_TREE_PREFIX".into()),
            TrieType::StorageTrie(contract_address) => {

Suggestion:

            Self::ContractsTrie => DbKeyPrefix::new(b"CONTRACTS_TREE_PREFIX".into()),
            Self::ClassesTrie => DbKeyPrefix::new(b"CLASSES_TREE_PREFIX".into()),
            Self::StorageTrie(contract_address) => {

crates/starknet_committer/src/db/index_db/leaves.rs line 117 at r8 (raw file):

        let nonce = deserialize_felt(&mut cursor, || {
            DeserializationError::FeltDeserialization(value.clone())
        })?;

please avoid cloning the value - you can implement some deserialize_felts(value: &DbValue, n: usize) -> Vec<Felt> to reuse the value, right?

Code quote:

        let mut cursor: &[u8] = &value.0;
        let class_hash = deserialize_felt(&mut cursor, || {
            DeserializationError::FeltDeserialization(value.clone())
        })?;
        let storage_root_hash = deserialize_felt(&mut cursor, || {
            DeserializationError::FeltDeserialization(value.clone())
        })?;
        let nonce = deserialize_felt(&mut cursor, || {
            DeserializationError::FeltDeserialization(value.clone())
        })?;

crates/starknet_committer/src/db/index_db/leaves.rs line 167 at r8 (raw file):

}

fn serialize_felts(felts: Vec<Felt>) -> Result<DbValue, SerializationError> {

must this be by value? slice is not enough?

Suggestion:

felts: &[Felt]

crates/starknet_committer/src/db/index_db/leaves_test.rs line 22 at r8 (raw file):

        storage_root_hash: HashOutput(Felt::from(2)),
        nonce: Nonce(Felt::from(3)),}
))]

non-blocking

Suggestion:

#[case::index_layout_contract_state(IndexLayoutContractState(ContractState {
    class_hash: ClassHash(Felt::from(1)),
    storage_root_hash: HashOutput(Felt::from(2)),
    nonce: Nonce(Felt::from(3)),
}))]

crates/starknet_committer/src/db/index_db/leaves_test.rs line 28 at r8 (raw file):

#[case::index_layout_starknet_storage_value(IndexLayoutStarknetStorageValue(
    StarknetStorageValue(Felt::from(1))
))]

also non-blocking

Suggestion:

#[case::index_layout_contract_state(IndexLayoutContractState(ContractState {
        class_hash: ClassHash(Felt::ONE),
        storage_root_hash: HashOutput(Felt::TWO),
        nonce: Nonce(Felt::THREE),}
))]
#[case::index_layout_compiled_class_hash(IndexLayoutCompiledClassHash(CompiledClassHash(
    Felt::ONE
)))]
#[case::index_layout_starknet_storage_value(IndexLayoutStarknetStorageValue(
    StarknetStorageValue(Felt::ONE)
))]

crates/starknet_committer/src/db/index_db/leaves_test.rs line 33 at r8 (raw file):

    let deserialized = L::deserialize(&serialized, &EmptyDeserializationContext).unwrap();
    assert_eq!(leaf, deserialized);
}

please add some regression-tests for serialized values, to make sure they are stable + nothing unexpected happens

#[rstest]
#[case(..)]
..
fn test_leaf_serialization_regression(#[case] leaf) {
    expect!["bla"].assert_debug_eq(leaf.serialize());
}

Code quote:

#[rstest]
#[case::index_layout_contract_state(IndexLayoutContractState(ContractState {
        class_hash: ClassHash(Felt::from(1)),
        storage_root_hash: HashOutput(Felt::from(2)),
        nonce: Nonce(Felt::from(3)),}
))]
#[case::index_layout_compiled_class_hash(IndexLayoutCompiledClassHash(CompiledClassHash(
    Felt::from(1)
)))]
#[case::index_layout_starknet_storage_value(IndexLayoutStarknetStorageValue(
    StarknetStorageValue(Felt::from(1))
))]
fn test_index_layout_leaf_serde<L: Leaf>(#[case] leaf: L) {
    let serialized = leaf.serialize().unwrap();
    let deserialized = L::deserialize(&serialized, &EmptyDeserializationContext).unwrap();
    assert_eq!(leaf, deserialized);
}

crates/starknet_committer/Cargo.toml line 32 at r8 (raw file):

tokio = { workspace = true, features = ["rt"] }
tracing.workspace = true
derive_more.workspace = true

alphabetize deps

Code quote:

derive_more.workspace = true

dorimedini-starkware

@dorimedini-starkware partially reviewed 6 files.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArielElp).

ArielElp

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

crates/starknet_committer/Cargo.toml line 32 at r8 (raw file):

Previously, dorimedini-starkware wrote…

alphabetize deps

Done.

crates/starknet_committer/src/db/index_db/leaves.rs line 41 at r8 (raw file):

Previously, dorimedini-starkware wrote…

didn't we want these to be MAX_CONTRACT_ADDRESS + X?
also for metadata prefixes?

right, forgot, should be ok now

crates/starknet_committer/src/db/index_db/leaves.rs line 117 at r8 (raw file):

Previously, dorimedini-starkware wrote…

please avoid cloning the value - you can implement some deserialize_felts(value: &DbValue, n: usize) -> Vec<Felt> to reuse the value, right?

Note that cloning only happens in the error path, zero clones in the happy path. Since the error owns DbValue and I don't want to add a lifetime var there, I think we have to clone. I cleaned it a bit so the closure is defined once.

crates/starknet_committer/src/db/index_db/leaves.rs line 167 at r8 (raw file):

Previously, dorimedini-starkware wrote…

must this be by value? slice is not enough?

Yep, can be done with slices

crates/starknet_committer/src/db/index_db/leaves_test.rs line 33 at r8 (raw file):

Previously, dorimedini-starkware wrote…

please add some regression-tests for serialized values, to make sure they are stable + nothing unexpected happens
#[rstest]
#[case(..)]
..
fn test_leaf_serialization_regression(#[case] leaf) {
    expect!["bla"].assert_debug_eq(leaf.serialize());
}

Done. I used the same simple leaves, we could have fancier values (to see some compression and not just removal of leading zeros), but this would essentially be testing Felt serialization.

crates/starknet_committer/src/db/index_db/leaves.rs line 42 at r8 (raw file):

            TrieType::ContractsTrie => DbKeyPrefix::new(b"CONTRACTS_TREE_PREFIX".into()),
            TrieType::ClassesTrie => DbKeyPrefix::new(b"CLASSES_TREE_PREFIX".into()),
            TrieType::StorageTrie(contract_address) => {

Done.

dorimedini-starkware

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

crates/starknet_committer/src/db/index_db/leaves.rs line 117 at r8 (raw file):

Previously, ArielElp wrote…

Note that cloning only happens in the error path, zero clones in the happy path. Since the error owns DbValue and I don't want to add a lifetime var there, I think we have to clone. I cleaned it a bit so the closure is defined once.

ahhhh right, my bad

crates/starknet_committer/src/db/index_db/leaves_test.rs line 33 at r8 (raw file):

Previously, ArielElp wrote…

Done. I used the same simple leaves, we could have fancier values (to see some compression and not just removal of leading zeros), but this would essentially be testing Felt serialization.

please add some cases that indicate the byte order: i.e. larger values
how is it possible that the ContractState case serializes into just 3 bytes...? don't you need to measure the byte length of a value to know how to deserialize it? if the actual byte size of the three values are 10, 10 and 12, you will get 32 bytes, and try to deserialize as a binary node..?

crates/starknet_committer/src/db/index_db/leaves.rs line 39 at r9 (raw file):

/// Set to 2^251 + 2 to avoid collisions with contract addresses prefixes.
const CLASSES_TREE_PREFIX: &[u8] =
    b"0800000000000000000000000000000000000000000000000000000000000010";

reuse existing constant
if const doesn't work use LazyLock (see existing examples in our code)
not sure how you should be converting Felt to bytes, I just gave an example here; also you can consider just defining Felts in this context and converting to bytes only in the db_prefix method

Suggestion:

/// Set to 2^251 + 1 to avoid collisions with contract addresses prefixes.
const FIRST_AVAILABLE_PREFIX_FELT: Felt = Felt::from_hex_unchecked(PATRICIA_KEY_UPPER_BOUND) + Felt::ONE;

/// The db key prefix of nodes in the contracts trie.
const CONTRACTS_TREE_PREFIX: &[u8] = FIRST_AVAILABLE_PREFIX_FELT.to_bytes_be();

/// The db key prefix of nodes in the classes trie.
const CLASSES_TREE_PREFIX: &[u8] = (FIRST_AVAILABLE_PREFIX_FELT + Felt::ONE).to_bytes_be();

ArielElp

@ArielElp made 2 comments.
Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware).

crates/starknet_committer/src/db/index_db/leaves.rs line 39 at r9 (raw file):

Previously, dorimedini-starkware wrote…

reuse existing constant

if const doesn't work use LazyLock (see existing examples in our code)

not sure how you should be converting Felt to bytes, I just gave an example here; also you can consider just defining Felts in this context and converting to bytes only in the db_prefix method

Done.

3 won't work since I need a reference for the non-dynamic prefixes (the whole point of Cow and keeping the static lifetime), but I can take slices of a [u8;32] under a lazy lock.

crates/starknet_committer/src/db/index_db/leaves_test.rs line 33 at r8 (raw file):

Previously, dorimedini-starkware wrote…

please add some cases that indicate the byte order: i.e. larger values

how is it possible that the ContractState case serializes into just 3 bytes...? don't you need to measure the byte length of a value to know how to deserialize it? if the actual byte size of the three values are 10, 10 and 12, you will get 32 bytes, and try to deserialize as a binary node..?

Re 2, leaves are not mixed with inner nodes due to DeserializationContext that has is_leaf.

Re 1, added two non-trivial thresholds.

dorimedini-starkware

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

crates/starknet_committer/src/db/index_db/leaves_test.rs line 45 at r10 (raw file):

        128_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8,
        0_u8, 0_u8,
    ])))

works? if so, much clearer

Suggestion:

    IndexLayoutStarknetStorageValue(StarknetStorageValue(Felt::from_bytes_be(&[
            vec![0_u8; 15], vec![128_u8], vec![0_u8; 16]
        ].concat()
    ])))

crates/starknet_committer/src/db/index_db/leaves_test.rs line 65 at r10 (raw file):

// 27 nibbles, the chooser is the number of bytes. In this case, the first byte will be 11000000
// (chooser 12, i.e. we need 12 bytes) followed by the value.
#[case(starknet_storage_value_leaf_96_bits(), DbValue(vec![192, 128, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]))]

Suggestion:

DbValue([vec![192, 128], vec![0; 11]].concat())

crates/starknet_committer/src/db/index_db/leaves_test.rs line 74 at r10 (raw file):

    0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0
]))]

Suggestion:

#[case(starknet_storage_value_leaf_136_bits(), DbValue([
    vec![240], vec![0; 14], vec![128], vec![0; 16]
].concat()))]

nimrod-starkware

@nimrod-starkware partially reviewed 1 file and made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp and @yoavGrs).

crates/starknet_committer/src/db/index_db/leaves.rs line 186 at r10 (raw file):

    for felt in felts {
        felt.serialize(&mut buffer)?;
    }

@yoavGrs needed because of this

Code quote:

fn serialize_felts(felts: &[Felt]) -> Result<DbValue, SerializationError> {
    let mut buffer = Vec::new();
    for felt in felts {
        felt.serialize(&mut buffer)?;
    }

ArielElp

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

crates/starknet_committer/src/db/index_db/leaves_test.rs line 45 at r10 (raw file):

Previously, dorimedini-starkware wrote…

works? if so, much clearer

done differently but solves the issue

crates/starknet_committer/src/db/index_db/leaves_test.rs line 65 at r10 (raw file):

// 27 nibbles, the chooser is the number of bytes. In this case, the first byte will be 11000000
// (chooser 12, i.e. we need 12 bytes) followed by the value.
#[case(starknet_storage_value_leaf_96_bits(), DbValue(vec![192, 128, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]))]

Done.

crates/starknet_committer/src/db/index_db/leaves_test.rs line 74 at r10 (raw file):

    0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0
]))]

Done.

dorimedini-starkware

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

ArielElp

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

This was referenced Dec 10, 2025

starknet_committer: reorganize facts db files #10677

Merged

starknet_committer,starknet_patricia,starknet_patricia_storage: add context to prefix and serde #10678

Merged

ArielElp mentioned this pull request Dec 10, 2025

starknet_committer,starknet_os,starknet_patricia: make node data generic #10679

Merged

ArielElp force-pushed the ariel/add_index_layout_leaves branch from 01c2851 to ba78a66 Compare December 10, 2025 11:26

ArielElp force-pushed the ariel/abstract_fetch_nodes branch from 021132a to 466fffe Compare December 10, 2025 11:26

ArielElp mentioned this pull request Dec 10, 2025

starknet_committer,starknet_patricia: add subtree trait #10694

Merged

ArielElp force-pushed the ariel/abstract_fetch_nodes branch from 466fffe to b4ed9c6 Compare December 10, 2025 13:16

ArielElp force-pushed the ariel/add_index_layout_leaves branch 2 times, most recently from 7578dc0 to 59b51ce Compare December 10, 2025 14:20

ArielElp force-pushed the ariel/abstract_fetch_nodes branch from b4ed9c6 to c700f80 Compare December 10, 2025 14:20

ArielElp force-pushed the ariel/add_index_layout_leaves branch from 59b51ce to f733314 Compare December 10, 2025 15:06

ArielElp force-pushed the ariel/abstract_fetch_nodes branch from c700f80 to 745b6d9 Compare December 10, 2025 15:06

ArielElp mentioned this pull request Dec 10, 2025

starknet_committer,starknet_patricia: add deserialization context #10704

Merged

ArielElp force-pushed the ariel/abstract_fetch_nodes branch from 745b6d9 to d1214e3 Compare December 10, 2025 15:35

ArielElp force-pushed the ariel/add_index_layout_leaves branch 2 times, most recently from 8ff59c0 to ac40644 Compare December 10, 2025 15:41

ArielElp force-pushed the ariel/abstract_fetch_nodes branch from d1214e3 to a4c64e9 Compare December 10, 2025 15:41

ArielElp force-pushed the ariel/add_index_layout_leaves branch from ac40644 to e30a635 Compare December 10, 2025 15:53

ArielElp force-pushed the ariel/abstract_fetch_nodes branch from a4c64e9 to ad38e50 Compare December 10, 2025 15:53

ArielElp force-pushed the ariel/add_index_layout_leaves branch from e30a635 to f25ec01 Compare December 10, 2025 19:58

ArielElp force-pushed the ariel/abstract_fetch_nodes branch 2 times, most recently from b24fb64 to 71c316b Compare December 11, 2025 06:51

ArielElp force-pushed the ariel/add_index_layout_leaves branch from f25ec01 to aad75aa Compare December 11, 2025 06:51

ArielElp mentioned this pull request Dec 17, 2025

starknet_committer,starknet_patricia,starknet_patricia_storage: support contract address prefix #10858

Merged

nimrod-starkware requested changes Dec 17, 2025

View reviewed changes

ArielElp commented Dec 17, 2025

View reviewed changes

yoavGrs requested changes Dec 18, 2025

View reviewed changes

nimrod-starkware requested changes Dec 18, 2025

View reviewed changes

nimrod-starkware reviewed Dec 18, 2025

View reviewed changes

ArielElp commented Dec 21, 2025

View reviewed changes

This was referenced Dec 21, 2025

starknet_committer: hash leaves of both layouts #10918

Merged

starknet_committer: add index layout subtree #10919

Merged

nimrod-starkware requested changes Dec 21, 2025

View reviewed changes

ArielElp commented Dec 21, 2025

View reviewed changes

yoavGrs approved these changes Dec 22, 2025

View reviewed changes

nimrod-starkware approved these changes Dec 22, 2025

View reviewed changes

This was referenced Dec 23, 2025

starknet_committer,starknet_patricia: move db_layout #10986

Merged

starknet_committer,starknet_patricia: add serialization utilities to node layout #10987

Merged

starknet_committer,starknet_patricia: layout-dependent FilledTree serialization #10988

Merged

dorimedini-starkware requested changes Dec 24, 2025

View reviewed changes

dorimedini-starkware reviewed Dec 24, 2025

View reviewed changes

ArielElp commented Dec 24, 2025

View reviewed changes

dorimedini-starkware requested changes Dec 25, 2025

View reviewed changes

ArielElp commented Dec 25, 2025

View reviewed changes

dorimedini-starkware requested changes Dec 25, 2025

View reviewed changes

nimrod-starkware reviewed Dec 25, 2025

View reviewed changes

ArielElp commented Dec 25, 2025

View reviewed changes

starknet_committer: add index layout leaves

8189934

dorimedini-starkware approved these changes Dec 25, 2025

View reviewed changes

ArielElp mentioned this pull request Dec 25, 2025

starknet_committer: move trie creation logic to trie_traversal #11180

Merged

ArielElp commented Dec 25, 2025

View reviewed changes

ArielElp mentioned this pull request Dec 25, 2025

starknet_committer: enrich layout trait to abstract skeleton creation #11182

Merged

Conversation

ArielElp commented Dec 10, 2025

Uh oh!

reviewable-StarkWare commented Dec 10, 2025

Uh oh!

ArielElp commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nimrod-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

yoavGrs left a comment

Choose a reason for hiding this comment

Uh oh!

nimrod-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

nimrod-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

nimrod-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

yoavGrs left a comment

Choose a reason for hiding this comment

Uh oh!

nimrod-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

dorimedini-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

dorimedini-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

dorimedini-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

dorimedini-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

nimrod-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

dorimedini-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

ArielElp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ArielElp commented Dec 10, 2025 •

edited

Loading