starknet_committer: abstract original tree creation by ArielElp · Pull Request #10684 · starkware-libs/sequencer

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

No description provided.

reviewable-StarkWare · 2025-12-10T08:20:06Z

This change is

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

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

nimrod-starkware

@nimrod-starkware reviewed 10 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArielElp and @yoavGrs).

crates/starknet_committer/src/db/trie_traversal.rs line 366 at r3 (raw file):

    contracts_trie_sorted_indices: SortedLeafIndices<'a>,
) -> ForestResult<(OriginalSkeletonTreeImpl<'a>, HashMap<NodeIndex, ContractState>)> {
    Ok(create_original_skeleton_tree_and_get_previous_leaves(

Why not using it?

Code quote:

create_original_skeleton_tree_and_get_previous_leaves(

crates/starknet_committer/src/db/trie_traversal.rs line 305 at r3 (raw file):

        return Ok(handle_empty_subtree::<L>(sorted_leaf_indices).0);
    }
    let main_subtree = Layout::create_subtree(sorted_leaf_indices, NodeIndex::ROOT, root_hash);

see comment in prev PR

Suggestion:

let main_subtree = Layout::SubTree::create(sorted_leaf_indices, NodeIndex::ROOT, root_hash);

crates/starknet_committer/src/db/trie_traversal.rs line 353 at r3 (raw file):

    }
    Ok(storage_tries)
}

I think you should not be generic in L here:
Does this work?

Suggestion:

pub async fn create_storage_tries<
    'a,
    Layout: NodeLayout<'a, StarknetStorageValue>,
>(
    storage: &mut impl Storage,
    actual_storage_updates: &HashMap<ContractAddress, LeafModifications<StarknetStorageValue>>,
    original_contracts_trie_leaves: &HashMap<NodeIndex, ContractState>,
    config: &ReaderConfig,
    storage_tries_sorted_indices: &HashMap<ContractAddress, SortedLeafIndices<'a>>,
) -> ForestResult<HashMap<ContractAddress, OriginalSkeletonTreeImpl<'a>>> {
    let mut storage_tries = HashMap::new();
    for (address, updates) in actual_storage_updates {
        let sorted_leaf_indices = storage_tries_sorted_indices
            .get(address)
            .ok_or(ForestError::MissingSortedLeafIndices(*address))?;
        let contract_state = original_contracts_trie_leaves
            .get(&contract_address_into_node_index(address))
            .ok_or(ForestError::MissingContractCurrentState(*address))?;
        let config = OriginalSkeletonTrieConfig::new(config.warn_on_trivial_modifications());

        let original_skeleton = create_original_skeleton_tree::<StarknetStorageValue, Layout>(
            storage,
            contract_state.storage_root_hash,
            *sorted_leaf_indices,
            &config,
            &updates,
            &Layout::generate_key_context(TrieType::StorageTrie(*address)),
        )
        .await?;
        storage_tries.insert(*address, original_skeleton);
    }
    Ok(storage_tries)
}

crates/starknet_committer/src/db/trie_traversal.rs line 355 at r3 (raw file):

}

pub async fn create_contracts_trie<'a, L: Leaf + Into<ContractState>, Layout: NodeLayout<'a, L>>(

restore doc

Suggestion:

/// Creates the contracts trie original skeleton.
/// Also returns the previous contracts state of the modified contracts.
pub async fn create_contracts_trie<'a, L: Leaf + Into<ContractState>, Layout: NodeLayout<'a, L>>(

crates/starknet_committer/src/db/trie_traversal.rs line 358 at r3 (raw file):

    storage: &mut impl Storage,
    root_hash: HashOutput,
    sorted_leaf_indices: SortedLeafIndices<'a>,

restore arg names

Suggestion:

    contracts_trie_root_hash: HashOutput,
    contracts_trie_sorted_indices: SortedLeafIndices<'a>,

crates/starknet_committer/src/db/trie_traversal.rs line 385 at r3 (raw file):

    Ok((skeleton_tree, leaves))
}

same as above

Suggestion:

pub async fn create_contracts_trie<'a, Layout: NodeLayout<'a, ContractState>>(
    storage: &mut impl Storage,
    root_hash: HashOutput,
    sorted_leaf_indices: SortedLeafIndices<'a>,
) -> ForestResult<(OriginalSkeletonTreeImpl<'a>, HashMap<NodeIndex, ContractState>)> {
    if sorted_leaf_indices.is_empty() {
        let unmodified = OriginalSkeletonTreeImpl::create_unmodified(root_hash);
        return Ok((unmodified, HashMap::new()));
    }
    if root_hash == HashOutput::ROOT_OF_EMPTY_TREE {
        return Ok(handle_empty_subtree(sorted_leaf_indices));
    }
    let main_subtree = Layout::create_subtree(sorted_leaf_indices, NodeIndex::ROOT, root_hash);
    let mut skeleton_tree = OriginalSkeletonTreeImpl { nodes: HashMap::new(), sorted_leaf_indices };
    let mut leaves = HashMap::new();
    fetch_nodes::<L, Layout>(
        &mut skeleton_tree,
        vec![main_subtree],
        storage,
        &HashMap::new(),
        &OriginalSkeletonTrieDontCompareConfig,
        Some(&mut leaves),
        &Layout::generate_key_context(TrieType::ContractsTrie),
    )
    .await?;

    let leaves: HashMap<NodeIndex, ContractState> =
        leaves.into_iter().map(|(idx, leaf)| (idx, leaf.into())).collect();

    Ok((skeleton_tree, leaves))
}

crates/starknet_committer/src/db/trie_traversal.rs line 409 at r3 (raw file):

    )
    .await?)
}

same here - L should be concrete type

Code quote:

pub async fn create_classes_trie<
    'a,
    L: Leaf + From<CompiledClassHash>,
    Layout: NodeLayout<'a, L>,
>(
    storage: &mut impl Storage,
    actual_classes_updates: &LeafModifications<CompiledClassHash>,
    classes_trie_root_hash: HashOutput,
    config: &ReaderConfig,
    contracts_trie_sorted_indices: SortedLeafIndices<'a>,
) -> ForestResult<OriginalSkeletonTreeImpl<'a>> {
    let config = OriginalSkeletonTrieConfig::new(config.warn_on_trivial_modifications());

    Ok(create_original_skeleton_tree::<L, Layout>(
        storage,
        classes_trie_root_hash,
        contracts_trie_sorted_indices,
        &config,
        &actual_classes_updates.iter().map(|(idx, value)| (*idx, L::from(*value))).collect(),
        &Layout::generate_key_context(TrieType::ClassesTrie),
    )
    .await?)
}

nimrod-starkware

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

crates/starknet_committer/src/db/trie_traversal.rs line 353 at r3 (raw file):

Previously, nimrod-starkware wrote…

I think you should not be generic in L here:
Does this work?

I think I realise now what your'e trying here - youre trying to use the same code for StarknetStorageValue (facts layout) and IndexLayoutStarknetStorageValue (index layout), right?
If this is the case ignore the this and other comments about it, i'll leave a separate comment of how i think it should be

yoavGrs

@yoavGrs partially reviewed 3 files and made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArielElp).

crates/starknet_committer/src/db/trie_traversal.rs line 307 at r2 (raw file):

            leaf_modifications,
            config,
        )?;

Why is it deleted?

Code quote:

        log_warning_for_empty_leaves(
            sorted_leaf_indices.get_indices(),
            leaf_modifications,
            config,
        )?;

nimrod-starkware

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

crates/starknet_committer/src/db/trie_traversal.rs line 359 at r4 (raw file):

    root_hash: HashOutput,
    sorted_leaf_indices: SortedLeafIndices<'a>,
) -> ForestResult<(OriginalSkeletonTreeImpl<'a>, HashMap<NodeIndex, ContractState>)> {

doesn't it make sense?

Suggestion:

ForestResult<(OriginalSkeletonTreeImpl<'a>, HashMap<NodeIndex, L>)

nimrod-starkware

@nimrod-starkware resolved 3 discussions.
Reviewable status: 9 of 10 files reviewed, 6 unresolved discussions (waiting on @ArielElp).

ArielElp

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

crates/starknet_committer/src/db/trie_traversal.rs line 307 at r2 (raw file):

Previously, yoavGrs wrote…

Why is it deleted?

Mistake, restored

crates/starknet_committer/src/db/trie_traversal.rs line 305 at r3 (raw file):

Previously, nimrod-starkware wrote…

see comment in prev PR

See my comment there, it's actually a bit sensitive since Layout::SubTree::create expects NodeData in the third argument.

Maybe an alternative is to have a From constraint on NodeData, though it looks a bit weird (I'll need to implement From for the unit type ()).

crates/starknet_committer/src/db/trie_traversal.rs line 355 at r3 (raw file):

Previously, nimrod-starkware wrote…

restore doc

Done.

crates/starknet_committer/src/db/trie_traversal.rs line 358 at r3 (raw file):

Previously, nimrod-starkware wrote…

restore arg names

Done.

crates/starknet_committer/src/db/trie_traversal.rs line 366 at r3 (raw file):

Previously, nimrod-starkware wrote…

Why not using it?

I still didn't make get_leaves + create_original_skeleton_tree_and_get_previous_leaves generic yet. They're used from starknet_os_flow_tests, and given that we're not serving storage proofs yet I didn't refactor.

crates/starknet_committer/src/db/trie_traversal.rs line 359 at r4 (raw file):

Previously, nimrod-starkware wrote…

doesn't it make sense?

I want to make it easy on the user of this method. What you want back is ContractState, not the db representation (It's a bit unfortunate that we have duplicated logical leaf types for different db representations, but it allowed using DBObject down the line).

You later send the result to create_storage_tries via this argument:

original_contracts_trie_leaves: &HashMap<NodeIndex, ContractState>

So returning the db representation will require a conversion in the caller (we still have a single conversion in both cases, but IMO it's better in the body)

yoavGrs

@yoavGrs reviewed 5 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArielElp and @nimrod-starkware).

crates/starknet_committer/src/db/trie_traversal.rs line 366 at r3 (raw file):

Previously, ArielElp wrote…

I still didn't make get_leaves + create_original_skeleton_tree_and_get_previous_leaves generic yet. They're used from starknet_os_flow_tests, and given that we're not serving storage proofs yet I didn't refactor.

What is the difference between the functions?
Can we use a common util?

crates/starknet_committer/src/db/trie_traversal.rs line 262 at r5 (raw file):

            val,
            &subtree.get_root_context(),
        )?);

Is it possible? Impl into for NodeDbObject?

Suggestion:

        let filled_node = Layout::NodeDbObject::deserialize(
            val,
            &subtree.get_root_context(),
        )?.into();

ArielElp

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

crates/starknet_committer/src/db/trie_traversal.rs line 305 at r3 (raw file):

Previously, ArielElp wrote…

See my comment there, it's actually a bit sensitive since Layout::SubTree::create expects NodeData in the third argument.

Maybe an alternative is to have a From constraint on NodeData, though it looks a bit weird (I'll need to implement From for the unit type ()).

Done in the previous PR with a from constraint and an explicit EmptyNodeData type.

crates/starknet_committer/src/db/trie_traversal.rs line 366 at r3 (raw file):

Previously, yoavGrs wrote…

What is the difference between the functions?
Can we use a common util?

Done, TLDR of changes:

modified the standard create_original_skeleton_trie to return the previous leaves (just pass to fetch nodes)
removed the now unnecessary create_original_skeleton_tree_and_fetch_previous_leaves

crates/starknet_committer/src/db/trie_traversal.rs line 262 at r5 (raw file):

Previously, yoavGrs wrote…

Is it possible? Impl into for NodeDbObject?

Done.

nimrod-starkware

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

crates/starknet_committer/src/db/trie_traversal.rs line 377 at r6 (raw file):

        &HashMap::new(),
        Some(&mut leaves),
        &Layout::generate_key_context(TrieType::ClassesTrie),

no?

Suggestion:

&Layout::generate_key_context(TrieType::ContractsTrie)

crates/starknet_committer/src/db/trie_traversal.rs line 421 at r6 (raw file):

        sorted_leaf_indices.get_indices().iter().map(|idx| (*idx, L::default())).collect(),
    )
}

doc

Code quote:

pub(crate) fn handle_empty_subtree<'a, L: Leaf>(
    sorted_leaf_indices: SortedLeafIndices<'a>,
) -> (OriginalSkeletonTreeImpl<'a>, HashMap<NodeIndex, L>) {
    (
        OriginalSkeletonTreeImpl::create_empty(sorted_leaf_indices),
        sorted_leaf_indices.get_indices().iter().map(|idx| (*idx, L::default())).collect(),
    )
}

crates/starknet_committer/src/db/trie_traversal.rs line 421 at r6 (raw file):

        sorted_leaf_indices.get_indices().iter().map(|idx| (*idx, L::default())).collect(),
    )
}

The name is missleading, it should be the entire tree, not a subtree

Suggestion:

pub(crate) fn get_skeleton_and_previous_leaves_of_empty_tree<'a, L: Leaf>(
    sorted_leaf_indices: SortedLeafIndices<'a>,
) -> (OriginalSkeletonTreeImpl<'a>, HashMap<NodeIndex, L>) {
    (
        OriginalSkeletonTreeImpl::create_empty(sorted_leaf_indices),
        sorted_leaf_indices.get_indices().iter().map(|idx| (*idx, L::default())).collect(),
    )
}

yoavGrs

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

crates/starknet_committer/src/db/trie_traversal.rs line 304 at r6 (raw file):

            config,
        )?;
        return Ok(handle_empty_subtree::<L>(sorted_leaf_indices).0);

You're using this function only here, right?

Suggestion:

return Ok(OriginalSkeletonTreeImpl::create_empty(sorted_leaf_indices));

ArielElp

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

crates/starknet_committer/src/db/trie_traversal.rs line 304 at r6 (raw file):

Previously, yoavGrs wrote…

You're using this function only here, right?

Right... removed

I noticed that I didn't have previous_leaves handling in the empty tree case, so I added it.

crates/starknet_committer/src/db/trie_traversal.rs line 377 at r6 (raw file):

Previously, nimrod-starkware wrote…

no?

Ups, fixed

crates/starknet_committer/src/db/trie_traversal.rs line 421 at r6 (raw file):

Previously, nimrod-starkware wrote…

doc

Removed since it was used once and didn't really help

crates/starknet_committer/src/db/trie_traversal.rs line 421 at r6 (raw file):

Previously, nimrod-starkware wrote…

The name is missleading, it should be the entire tree, not a subtree

Same as above

nimrod-starkware

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

dorimedini-starkware

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

crates/starknet_committer/src/db/trie_traversal.rs line 294 at r8 (raw file):

    previous_leaves: Option<&mut HashMap<NodeIndex, L>>,
    key_context: &<L as HasStaticPrefix>::KeyContext,
) -> OriginalSkeletonTreeResult<OriginalSkeletonTreeImpl<'a>> {

please add a docstring detailing

what the parameters are
how they relate to the storage layout, if they do

Code quote:

pub async fn create_original_skeleton_tree<'a, L: Leaf, Layout: NodeLayout<'a, L>>(
    storage: &mut impl Storage,
    root_hash: HashOutput,
    sorted_leaf_indices: SortedLeafIndices<'a>,
    config: &impl OriginalSkeletonTreeConfig,
    leaf_modifications: &LeafModifications<L>,
    previous_leaves: Option<&mut HashMap<NodeIndex, L>>,
    key_context: &<L as HasStaticPrefix>::KeyContext,
) -> OriginalSkeletonTreeResult<OriginalSkeletonTreeImpl<'a>> {

crates/starknet_committer/src/db/trie_traversal.rs line 335 at r8 (raw file):

    L: Leaf + From<StarknetStorageValue>,
    Layout: NodeLayout<'a, L>,
>(

in this function the leaf type should be known; why do you need L?

Suggestion:

pub async fn create_storage_tries<
    'a,
    Layout: NodeLayout<'a, StarknetStorageValue>,
>(

crates/starknet_committer/src/db/trie_traversal.rs line 359 at r8 (raw file):

            *sorted_leaf_indices,
            &config,
            &updates.iter().map(|(idx, value)| (*idx, L::from(*value))).collect(),

this is not an O(1) copy, consider adding a TODO here to change LeafModifications to be an iterator over borrowed data, and then you may avoid this collect.
or, better yet, if StarknetStorageValue is the actual leaf type (why isn't it a leaf, BTW?), there will be no need for this conversion at all, right?

Code quote:

&updates.iter().map(|(idx, value)| (*idx, L::from(*value))).collect(),

crates/starknet_committer/src/db/trie_traversal.rs line 371 at r8 (raw file):

/// Creates the contracts trie original skeleton.
/// Also returns the previous contracts state of the modified contracts.
pub async fn create_contracts_trie<'a, L: Leaf + Into<ContractState>, Layout: NodeLayout<'a, L>>(

ditto (why L?)

Code quote:

create_contracts_trie<'a, L: Leaf + Into<ContractState>, Layout: NodeLayout<'a, L>>(

crates/starknet_committer/src/db/trie_traversal.rs line 399 at r8 (raw file):

    'a,
    L: Leaf + From<CompiledClassHash>,
    Layout: NodeLayout<'a, L>,

ditto (why L?)

Code quote:

pub async fn create_classes_trie<
    'a,
    L: Leaf + From<CompiledClassHash>,
    Layout: NodeLayout<'a, L>,

crates/starknet_committer/src/db/trie_traversal.rs line 416 at r8 (raw file):

        contracts_trie_sorted_indices,
        &config,
        &actual_classes_updates.iter().map(|(idx, value)| (*idx, L::from(*value))).collect(),

same TODO remark

Code quote:

&actual_classes_updates.iter().map(|(idx, value)| (*idx, L::from(*value))).collect(),

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

        &EmptyKeyContext,
    )
    .await?;

... unless you will need this in the next PR

Suggestion:

    create_original_skeleton_tree::<L, FactsNodeLayout>(
        storage,
        root_hash,
        sorted_leaf_indices,
        &config,
        &leaf_modifications,
        Some(&mut previous_leaves),
        &EmptyKeyContext,
    )
    .await?;

crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 61 at r8 (raw file):

    .await?;
    Ok((skeleton_tree, leaves))
}

does this deletion have to be part of this PR?
very difficult to see if the "new inlining" of this logic is equivalent

Code quote:

pub async fn create_original_skeleton_tree_and_get_previous_leaves<
    'a,
    L: LeafWithEmptyKeyContext,
>(
    storage: &mut impl Storage,
    root_hash: HashOutput,
    sorted_leaf_indices: SortedLeafIndices<'a>,
    leaf_modifications: &LeafModifications<L>,
    config: &impl OriginalSkeletonTreeConfig,
) -> OriginalSkeletonTreeResult<(OriginalSkeletonTreeImpl<'a>, HashMap<NodeIndex, L>)> {
    if sorted_leaf_indices.is_empty() {
        let unmodified = OriginalSkeletonTreeImpl::create_unmodified(root_hash);
        return Ok((unmodified, HashMap::new()));
    }
    if root_hash == HashOutput::ROOT_OF_EMPTY_TREE {
        return Ok((
            OriginalSkeletonTreeImpl::create_empty(sorted_leaf_indices),
            sorted_leaf_indices.get_indices().iter().map(|idx| (*idx, L::default())).collect(),
        ));
    }
    let main_subtree = FactsSubTree::create(sorted_leaf_indices, NodeIndex::ROOT, root_hash);
    let mut skeleton_tree = OriginalSkeletonTreeImpl { nodes: HashMap::new(), sorted_leaf_indices };
    let mut leaves = HashMap::new();
    fetch_nodes::<L, FactsNodeLayout>(
        &mut skeleton_tree,
        vec![main_subtree],
        storage,
        leaf_modifications,
        config,
        Some(&mut leaves),
        &EmptyKeyContext,
    )
    .await?;
    Ok((skeleton_tree, leaves))
}

yoavGrs

@yoavGrs reviewed all commit messages and resolved 1 discussion.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArielElp).

ArielElp

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

crates/starknet_committer/src/db/trie_traversal.rs line 294 at r8 (raw file):

Previously, dorimedini-starkware wrote…

please add a docstring detailing

what the parameters are

how they relate to the storage layout, if they do

Done.

crates/starknet_committer/src/db/trie_traversal.rs line 335 at r8 (raw file):

Previously, dorimedini-starkware wrote…

in this function the leaf type should be known; why do you need L?

These functions are still generic to support both layout leaf types, e.g. both IndexLayoutContractState and ContractState.

The upside is that consumers of these functions don't have to care about "db leaf representation", and just use the standard StarknetStorageValue/CompiledClassHash/ContractState. The cost is the conversion of leaf_modifications, where I need to scan them and apply the conversion (or for ContractState convert the return values).

crates/starknet_committer/src/db/trie_traversal.rs line 359 at r8 (raw file):

Previously, dorimedini-starkware wrote…

this is not an O(1) copy, consider adding a TODO here to change LeafModifications to be an iterator over borrowed data, and then you may avoid this collect.
or, better yet, if StarknetStorageValue is the actual leaf type (why isn't it a leaf, BTW?), there will be no need for this conversion at all, right?

See previous comment for the reason to this conversion. Added a TODO (hopefully what I wrote is doable and fixes this).

crates/starknet_committer/src/db/trie_traversal.rs line 371 at r8 (raw file):

Previously, dorimedini-starkware wrote…

ditto (why L?)

See above.

crates/starknet_committer/src/db/trie_traversal.rs line 399 at r8 (raw file):

Previously, dorimedini-starkware wrote…

ditto (why L?)

See above.

crates/starknet_committer/src/db/trie_traversal.rs line 416 at r8 (raw file):

Previously, dorimedini-starkware wrote…

same TODO remark

Added TODO

crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 61 at r8 (raw file):

Previously, dorimedini-starkware wrote…

does this deletion have to be part of this PR?
very difficult to see if the "new inlining" of this logic is equivalent

Nope, could be separate. But, you can compare this directly to the body of create_original_skeleton_trie, it's exactly the same modulo the empty tree case, with the following differences:

The deleted function returns the previous leaves in the empty tree case, create_original_skeleton_trie adds it to a given vec
create_original_skeleton_trie emits a warning when leaf modifications are not the leaf default, but this is irrelevant to get_leaves since leaf_modifications is empty

dorimedini-starkware

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

crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 61 at r8 (raw file):

Previously, ArielElp wrote…

Nope, could be separate. But, you can compare this directly to the body of create_original_skeleton_trie, it's exactly the same modulo the empty tree case, with the following differences:

The deleted function returns the previous leaves in the empty tree case, create_original_skeleton_trie adds it to a given vec

create_original_skeleton_trie emits a warning when leaf modifications are not the leaf default, but this is irrelevant to get_leaves since leaf_modifications is empty

easier to do this comparison as a separate PR... please split next time

crates/starknet_committer/src/db/trie_traversal.rs line 323 at r9 (raw file):

                    .get_indices()
                    .iter()
                    .map(|idx| (*idx, L::default()))

is there an L::empty()? seems more explicit

Code quote:

L::default()

ArielElp

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

crates/starknet_committer/src/db/trie_traversal.rs line 323 at r9 (raw file):

Previously, dorimedini-starkware wrote…

is there an L::empty()? seems more explicit

nope (there's is_empty that checks for Felt::ZERO everywhere)

ArielElp mentioned this pull request Dec 10, 2025

starknet_committer,starknet_patricia: remove generic config #10683

Merged

ArielElp mentioned this pull request Dec 10, 2025

starknet_committer: add index db #10685

Merged

ArielElp force-pushed the ariel/clean_skeleton_tree_configs branch from 89be22e to d6df426 Compare December 10, 2025 11:26

ArielElp force-pushed the ariel/layout_based_skeleton_creation branch from fd40107 to 787903d 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/layout_based_skeleton_creation branch from 787903d to d36e334 Compare December 10, 2025 13:16

ArielElp force-pushed the ariel/clean_skeleton_tree_configs branch 2 times, most recently from a6375be to 6a65d5a Compare December 10, 2025 14:20

ArielElp force-pushed the ariel/layout_based_skeleton_creation branch from d36e334 to 10ee434 Compare December 10, 2025 14:20

ArielElp force-pushed the ariel/clean_skeleton_tree_configs branch from 6a65d5a to bc73073 Compare December 10, 2025 15:06

ArielElp force-pushed the ariel/layout_based_skeleton_creation branch from 10ee434 to 97d3b36 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/layout_based_skeleton_creation branch from 97d3b36 to eb78b62 Compare December 10, 2025 15:35

ArielElp force-pushed the ariel/clean_skeleton_tree_configs branch from bc73073 to cc1ef83 Compare December 10, 2025 15:35

ArielElp force-pushed the ariel/layout_based_skeleton_creation branch 2 times, most recently from f176d41 to e0517f7 Compare December 10, 2025 15:53

ArielElp force-pushed the ariel/clean_skeleton_tree_configs branch from 214868d to 85a0005 Compare December 10, 2025 15:53

ArielElp force-pushed the ariel/layout_based_skeleton_creation branch from e0517f7 to 4ce8285 Compare December 10, 2025 19:58

ArielElp force-pushed the ariel/clean_skeleton_tree_configs branch from 85a0005 to 244c030 Compare December 10, 2025 19:58

ArielElp force-pushed the ariel/layout_based_skeleton_creation branch 3 times, most recently from e43b4d4 to 35d1594 Compare December 11, 2025 08:49

This was referenced Dec 25, 2025

starknet_committer: move trie creation logic to trie_traversal #11180

Merged

starknet_committer: enrich layout trait to abstract skeleton creation #11182

Merged

nimrod-starkware requested changes Dec 28, 2025

View reviewed changes

nimrod-starkware requested changes Dec 29, 2025

View reviewed changes

yoavGrs requested changes Dec 29, 2025

View reviewed changes

ArielElp mentioned this pull request Dec 29, 2025

starknet_committer,starknet_patricia: layout-based original tree creation tests #11236

Merged

nimrod-starkware requested changes Dec 29, 2025

View reviewed changes

nimrod-starkware reviewed Dec 29, 2025

View reviewed changes

ArielElp mentioned this pull request Dec 30, 2025

starknet_committer: index layout forest creation tests #11286

Merged

ArielElp commented Dec 30, 2025

View reviewed changes

yoavGrs requested changes Dec 31, 2025

View reviewed changes

ArielElp mentioned this pull request Dec 31, 2025

starknet_committer: add commit e2e tests #11331

Merged

ArielElp commented Jan 1, 2026

View reviewed changes

nimrod-starkware requested changes Jan 1, 2026

View reviewed changes

yoavGrs requested changes Jan 1, 2026

View reviewed changes

ArielElp commented Jan 1, 2026

View reviewed changes

nimrod-starkware approved these changes Jan 1, 2026

View reviewed changes

dorimedini-starkware requested changes Jan 4, 2026

View reviewed changes

yoavGrs approved these changes Jan 4, 2026

View reviewed changes

ArielElp commented Jan 8, 2026

View reviewed changes

dorimedini-starkware approved these changes Jan 8, 2026

View reviewed changes

ArielElp commented Jan 8, 2026

View reviewed changes

This was referenced Jan 9, 2026

starknet_committer,starknet_patricia: add layout for base trait #11566

Merged

starknet_committer,starknet_patricia: refactor key context #11592

Merged

starknet_committer: abstract original tree creation

1218f54

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!

nimrod-starkware 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!

yoavGrs 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!

yoavGrs 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!

dorimedini-starkware 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!

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