chore: building original skeleton

[nimrod-starkware]

implementing rust equivalent to 'fetch_nodes` in the python repository.
wrote a few tests as well

---

This change is 

[dorimedini-starkware]

Reviewed 12 of 14 files at r1, all commit messages.
Reviewable status: 12 of 14 files reviewed, 36 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

---

Cargo.toml line 21 at r1 (raw file):

rstest = "0.17.0"
starknet-types-core = { version = "0.0.11", features = ["hash"] }
bisection = "0.1.0"

alphabetize dependencies

Suggestion:

[workspace.dependencies]
bisection = "0.1.0"
clap = { version = "4.5.4", features = ["cargo", "derive"] }
derive_more = "0.99.17"
pretty_assertions = "1.2.1"
rstest = "0.17.0"
starknet-types-core = { version = "0.0.11", features = ["hash"] }

---

crates/committer/Cargo.toml line 19 at r1 (raw file):

derive_more.workspace = true
starknet-types-core.workspace = true
bisection.workspace = true

Suggestion:

[dependencies]
bisection.workspace = true
derive_more.workspace = true
starknet-types-core.workspace = true

---

crates/committer/src/types.rs line 15 at r1 (raw file):

    Ord,
    derive_more::Sub,
)]

Suggestion:

#[derive(
    Eq,
    PartialEq,
    Clone,
    Copy,
    Debug,
    Default,
    Hash,
    derive_more::Add,
    derive_more::Sub,
    PartialOrd,
    Ord,
)]

---

crates/committer/src/types.rs line 65 at r1 (raw file):

    }

    pub fn shift_left(&self, steps: u8) -> Self {

depending on use case, this can be misleading.
what should (PRIME-1).shift_left(1) return? i.e. what about overflow?

Code quote:

pub fn shift_left(&self, steps: u8) -> Self {

---

crates/committer/src/types.rs line 66 at r1 (raw file):

    pub fn shift_left(&self, steps: u8) -> Self {
        *self * Felt::TWO.pow(steps)

is there no built-in method of StarknetTypesFelt that does this?

Code quote:

*self * Felt::TWO.pow(steps)

---

crates/committer/src/types.rs line 70 at r1 (raw file):

    pub fn from_bytes_be_slice(bytes: &[u8]) -> Self {
        Self(StarknetTypesFelt::from_bytes_be_slice(bytes))
    }

while you're here, these should be pub(crate) unless we explicitly decide to include these functions as part of public API

Suggestion:

    pub(crate) const ZERO: Felt = Felt(StarknetTypesFelt::ZERO);
    pub(crate) const ONE: Felt = Felt(StarknetTypesFelt::ONE);
    pub(crate) const TWO: Felt = Felt(StarknetTypesFelt::TWO);

    /// Raises `self` to the power of `exponent`.
    pub(crate) fn pow(&self, exponent: impl Into<u128>) -> Self {
        Self(self.0.pow(exponent.into()))
    }
    pub(crate) fn bits(&self) -> u8 {
        self.0
            .bits()
            .try_into()
            .expect("Should not fail as it takes less than 252 bits to represent a felt.")
    }

    pub(crate) fn shift_left(&self, steps: u8) -> Self {
        *self * Felt::TWO.pow(steps)
    }
    pub(crate) fn from_bytes_be_slice(bytes: &[u8]) -> Self {
        Self(StarknetTypesFelt::from_bytes_be_slice(bytes))
    }

---

crates/committer/src/types.rs line 73 at r1 (raw file):

    pub(crate) fn to_bytes_be(self) -> [u8; 32] {
        self.0.to_bytes_be()
    }

newlines between functions

Suggestion:

    }

    pub fn bits(&self) -> u8 {
        self.0
            .bits()
            .try_into()
            .expect("Should not fail as it takes less than 252 bits to represent a felt.")
    }

    pub fn shift_left(&self, steps: u8) -> Self {
        *self * Felt::TWO.pow(steps)
    }

    pub fn from_bytes_be_slice(bytes: &[u8]) -> Self {
        Self(StarknetTypesFelt::from_bytes_be_slice(bytes))
    }

    pub(crate) fn to_bytes_be(self) -> [u8; 32] {
        self.0.to_bytes_be()
    }

---

crates/committer/src/types.rs line 78 at r1 (raw file):

    pub fn from_hex(hex_string: &str) -> Result<Self, FromStrError> {
        Ok(StarknetTypesFelt::from_hex(hex_string)?.into())
    }

while you're here...

Suggestion:

    pub(crate) fn from_hex(hex_string: &str) -> Result<Self, FromStrError> {
        Ok(StarknetTypesFelt::from_hex(hex_string)?.into())
    }

---

crates/committer/src/hash/types.rs line 9 at r1 (raw file):

#[allow(dead_code)]
#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash)]

we will try to keep these ordered alphabetically, unless context requires otherwise (Eq next to PartialEq)

Suggestion:

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]

---

crates/committer/src/patricia_merkle_tree/errors.rs line 5 at r1 (raw file):

#[allow(dead_code)]
pub(crate) enum OriginalSkeletonTreeError {
    Deserializtion(String),

Suggestion:

Deserialization

---

crates/committer/src/patricia_merkle_tree/errors.rs line 6 at r1 (raw file):

pub(crate) enum OriginalSkeletonTreeError {
    Deserializtion(String),
    StorageRead,

we have a StorageError type, please use it

Suggestion:

StorageRead(#[from] StorageError)

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 9 at r1 (raw file):

#[allow(dead_code)]
#[derive(Debug, PartialEq, Eq)]
pub(crate) struct Nonce(pub Felt);

I wonder why rustfmt doesn't handle this...

Suggestion:

use crate::patricia_merkle_tree::types::{EdgeData, LeafDataTrait};
use crate::{hash::types::HashOutput, types::Felt};

// TODO(Nimrod, 1/6/2024): Swap to starknet-types-core types once implemented.
#[allow(dead_code)]
#[derive(Debug, PartialEq, Eq)]
pub(crate) struct ClassHash(pub Felt);

#[allow(dead_code)]
#[derive(Debug, PartialEq, Eq)]
pub(crate) struct Nonce(pub Felt);

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 14 at r1 (raw file):

use std::collections::HashMap;

use super::errors::OriginalSkeletonTreeError;

do not use super::, use crate::, unless in a test module X_test.rs and you are importing from X.rs

Code quote:

use super::errors::OriginalSkeletonTreeError;

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 17 at r1 (raw file):

#[allow(dead_code)]
pub(crate) struct SkeletonTree<'a> {

1. consistency (we have other Impl structs IIRC)
2. Without the word "original", there is ambiguity (this struct represents the skeleton tree before or after update?)

Suggestion:

pub(crate) struct OriginalSkeletonTreeImpl<'a> {

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

    leaf_modifications: &'a HashMap<NodeIndex, LeafData>,
    tree_height: TreeHeight,
}

question (I don't know the answer): why should SkeletonTree not own the leaf modifications?

Code quote:

#[allow(dead_code)]
pub(crate) struct SkeletonTree<'a> {
    nodes: HashMap<NodeIndex, OriginalSkeletonNode<LeafData>>,
    leaf_modifications: &'a HashMap<NodeIndex, LeafData>,
    tree_height: TreeHeight,
}

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 22 at r1 (raw file):

    tree_height: TreeHeight,
}
#[allow(dead_code)]

Suggestion:

}

#[allow(dead_code)]

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 24 at r1 (raw file):

#[allow(dead_code)]
impl SkeletonTree<'_> {
    fn fetch_nodes(

add a docstring

Code quote:

fn fetch_nodes(

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 26 at r1 (raw file):

    fn fetch_nodes(
        &mut self,
        subtrees: Vec<SubTree<'_>>,

the final result of this function is to populate this vector, correct?
is the conversion of this vector to a struct implementing OriginalSkeletonTree a WIP at the moment?
I.e. you are planning to keep fetch_nodes a private method, and the public entry point in this struct will return an instance?

Code quote:

subtrees: Vec<SubTree<'_>>,

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 42 at r1 (raw file):

                    .ok_or(OriginalSkeletonTreeError::StorageRead)
            })
            .collect::<OriginalSkeletonTreeResult<Vec<_>>>())?;

I don't think you need the extra parens

Suggestion:

        let root_vals: Vec<&StorageValue> = subtrees
            .iter()
            .map(|subtree| {
                storage
                    .get(&subtree.root_hash.with_patricia_prefix())
                    .ok_or(OriginalSkeletonTreeError::StorageRead)
            })
            .collect::<OriginalSkeletonTreeResult<Vec<_>>>()?;

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 44 at r1 (raw file):

            .collect::<OriginalSkeletonTreeResult<Vec<_>>>())?;
        for (root_val, subtree) in root_vals.iter().zip(subtrees.iter()) {
            if root_val.is_binary_node() {

replace the if root_val.is_X() if-else with:

root_node = root_val.deserialize()?;  // See below comment about skeleton node deserialization
match root_node {
    ...
}

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 69 at r1 (raw file):

                    root_index: left_root_index + NodeIndex(Felt::ONE),
                    root_hash: right_key,
                };

once you have a typed skeleton node (binary) to work with, I prefer you move this index computation logic to it:

impl SkeletonNode {
    fn binary_children_indices(parent_index: NodeIndex) -> (NodeIndex, NodeIndex) { ... }
}

Code quote:

                let left_root_index = subtree.root_index.shift_left(1);
                let left_subtree = SubTree {
                    sorted_leaf_indices: left_leaves,
                    root_index: left_root_index,
                    root_hash: left_key,
                };
                let right_subtree = SubTree {
                    sorted_leaf_indices: right_leaves,
                    root_index: left_root_index + NodeIndex(Felt::ONE),
                    root_hash: right_key,
                };

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 89 at r1 (raw file):

                    }
                    continue;
                }

why do you need this special case?
if you move to matching on skeleton node types, one of the branches should be of leaf type, and you won't need this if here, right?

Code quote:

                if subtree.get_height(&self.tree_height).is_of_height_one() {
                    // Children are leaves.
                    if left_subtree.is_sibling() {
                        self.nodes.insert(
                            left_subtree.root_index,
                            OriginalSkeletonNode::LeafOrBinarySibling(HashOutput(
                                Felt::from_bytes_be_slice(&left_subtree.root_hash.0),
                            )),
                        );
                    }
                    if right_subtree.is_sibling() {
                        self.nodes.insert(
                            right_subtree.root_index,
                            OriginalSkeletonNode::LeafOrBinarySibling(HashOutput(
                                Felt::from_bytes_be_slice(&right_subtree.root_hash.0),
                            )),
                        );
                    }
                    continue;
                }

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 149 at r1 (raw file):

    pub root_index: NodeIndex,
    pub root_hash: StorageKey,
}

move this definition new to the original skeleton tree implementation struct

Code quote:

#[allow(dead_code)]
struct SubTree<'a> {
    pub sorted_leaf_indices: &'a [NodeIndex],
    pub root_index: NodeIndex,
    pub root_hash: StorageKey,
}

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 151 at r1 (raw file):

}
#[allow(dead_code)]
impl<'a> SubTree<'a> {

will this struct be useful for Tzahi as well? Please sync

Code quote:

impl<'a> SubTree<'a> {

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 173 at r1 (raw file):

        self.sorted_leaf_indices.is_empty()
    }
}

since the tree logic depends on the subtree logic, I think it's more readable if the implementation of SubTree is above the implementation of the skeleton

Code quote:

#[allow(dead_code)]
impl<'a> SubTree<'a> {
    pub(crate) fn get_height(&self, total_tree_height: &TreeHeight) -> TreeHeight {
        TreeHeight(total_tree_height.0 - self.root_index.0.bits() + 1)
    }

    pub(crate) fn split_leaves(
        &self,
        total_tree_height: &TreeHeight,
    ) -> (&'a [NodeIndex], &'a [NodeIndex]) {
        let height = self.get_height(total_tree_height);
        let leftmost_index_in_right_subtree =
            ((self.root_index.shift_left(1)) + NodeIndex(Felt::ONE)).shift_left(height.0 - 1);
        let mid = bisect_left(self.sorted_leaf_indices, &leftmost_index_in_right_subtree);
        (
            &self.sorted_leaf_indices[..mid],
            &self.sorted_leaf_indices[mid..],
        )
    }

    pub(crate) fn is_sibling(&self) -> bool {
        self.sorted_leaf_indices.is_empty()
    }
}

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 177 at r1 (raw file):

#[cfg(test)]
#[path = "original_skeleton_calc_test.rs"]
pub mod original_skeleton_calc_test;

move to top of file, right under imports (with a newline)

Code quote:

#[cfg(test)]
#[path = "original_skeleton_calc_test.rs"]
pub mod original_skeleton_calc_test;

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree.rs line 20 at r1 (raw file):

    fn compute_original_skeleton_tree(
        storage: impl Storage,
        leaf_indices: &HashMap<NodeIndex, LeafData>,

this needs to be renamed if it's no longer a collection of indices

Code quote:

leaf_indices

---

crates/committer/src/patricia_merkle_tree/types.rs line 61 at r1 (raw file):

    PartialOrd,
    Ord,
    derive_more::Sub,

Suggestion:

    Clone,
    Copy,
    Debug,
    PartialEq,
    Eq,
    Hash,
    derive_more::Add,
    derive_more::Mul,
    derive_more::Sub,
    PartialOrd,
    Ord,

---

crates/committer/src/patricia_merkle_tree/types.rs line 78 at r1 (raw file):

        NodeIndex(index.0 * Felt::TWO.pow(length.0) + path.0)
    }
    pub(crate) fn shift_left(&self, steps: u8) -> Self {

Suggestion:

    }
    
    pub(crate) fn shift_left(&self, steps: u8) -> Self {

---

crates/committer/src/patricia_merkle_tree/types.rs line 88 at r1 (raw file):

    }
}
#[allow(dead_code)]

Suggestion:

}

#[allow(dead_code)]

---

crates/committer/src/patricia_merkle_tree/types.rs line 111 at r1 (raw file):

pub(crate) struct EdgeData {
    pub bottom_hash: HashOutput,
    pub path_to_bottom: PathToBottom,

Suggestion:

    pub(crate) bottom_hash: HashOutput,
    pub(crate) path_to_bottom: PathToBottom,

---

crates/committer/src/patricia_merkle_tree/types.rs line 126 at r1 (raw file):

    pub(crate) fn is_of_height_one(&self) -> bool {
        self.0 == 1
    }

why is this method useful?

Code quote:

    pub(crate) fn is_of_height_one(&self) -> bool {
        self.0 == 1
    }

---

crates/committer/src/patricia_merkle_tree/types.rs line 133 at r1 (raw file):

        root_index.shift_left(self.length.0) + NodeIndex(self.path.0)
    }
}

please sync with Aner, he may have something similar

Code quote:

impl PathToBottom {
    pub(crate) fn bottom_index(&self, root_index: NodeIndex) -> NodeIndex {
        root_index.shift_left(self.length.0) + NodeIndex(self.path.0)
    }
}

---

crates/committer/src/storage/storage_trait.rs line 33 at r1 (raw file):

    fn delete(&mut self, key: &StorageKey) -> Result<(), StorageError>;
}

The implementations here should not be implemented on storage objects.
we store objects that are not tree-related in storage, and these functions are undefined on such objects.

You are deserializing nodes from storage, right? and they are "filled nodes", in the sense that you are not reading a skeleton from storage, you are actually reading real nodes with hashes and data?

If so, sync with Aviv - the serialization part (of your deserialization) is in his court, you will probably be sharing API

Please move these functions; I suggest:

1. a SerializableSkeletonNode trait with a single deserialize(value: StorageValue) -> SkeletonNode function (not sure about the return type... may need a generic).
2. The function may also need the StorageKey as input, to check the node type... not sure
3. implement the trait for OriginalSkeletonNode

This function should if/else to find the node type and deserialize; do not define separate functions (deserialize_binary_node and deserialize_edge_node) that can panic; using a single entry point we can only ever panic if the input does not match any known node type.

---

crates/committer/src/storage/storage_trait.rs line 35 at r1 (raw file):

impl StorageValue {
    pub(crate) fn deserialize_binary_node(

You should have no magic numbers here. i.e, no writing 32, 64, 65.
Please sync with Aviv, he has such constants defined IIRC

---

crates/committer/src/storage/storage_trait.rs line 78 at r1 (raw file):

impl StorageKey {
    pub(crate) fn with_patricia_prefix(&self) -> Self {
        let mut prefix = "patricia_node:".as_bytes().to_vec();

Please give this string a name.
I suggest an enum of node prefixes, with a to_bytes method:

enum StoragePrefix {
    PatriciaNode,
    ...
}

impl StoragePrefix {
    pub(crate) fn to_bytes(&self) -> &[u8] {
        match self {
            Self::PatriciaNode => "patricia_node:".as_bytes(),
            ...
        }
    }
}

And then to use it, write StoragePrefix::PatriciaNode.to_bytes().

Code quote:

"patricia_node:"

[dorimedini-starkware]

Reviewed 1 of 14 files at r1.
Reviewable status: 13 of 14 files reviewed, 46 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 39 at r1 (raw file):

                       17  13     \
                       / \   \     \
                          9  11    15

this is more precise, right?

1. Binary non-sibling nodes have no data
2. Edge nodes have data (path data) but it is described in the ASCII art
3. Leaves that don't change are siblings (have a "hash" value used to update the skeleton), 9 in this example
4. Updated leaves are either zero or nonzero

Suggestion:

                             B
                           /   \
                          B     E
                         / \     \
                        B   E     \
                       / \   \     \
                      NZ  9   NZ    NZ

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 41 at r1 (raw file):

                          9  11    15

*/

triple slash it (use alt+shift+mouse click in VSCode to edit multiple lines at once :) )

Suggestion:

/// This test assumes for simplicity that hash is addition (i.e hash(a,b) = a + b).
///
///                   Old tree structure:
///
///                             50
///                           /   \
///                         30     20
///                        /  \     \
///                       17  13     \
///                      /  \   \     \
///                     8    9  11     15
///
///                   Modified leaves indices: [8, 10, 13]
///
///                   Expected skeleton:
///
///                             50
///                           /   \
///                         30     20
///                        /  \     \
///                       17  13     \
///                       / \   \     \
///                          9  11    15

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 55 at r1 (raw file):

    StorageValue((create_32_bytes_entry(8).into_iter().chain(create_32_bytes_entry(9).into_iter())).collect())),
    (StorageKey(create_32_bytes_entry(11 + 1 + 1)).with_patricia_prefix(),
    StorageValue(create_32_bytes_entry(11).into_iter().chain(create_32_bytes_entry(1).into_iter()).chain([1].into_iter()).collect())),

To reduce boilerplate and make this easier to read: in addition to create_32_bytes_entry define a

create_patricia_entry(key: u8, value: [u8]) -> (StorageKey, StorageValue)

... and use it everywhere

Suggestion:

    create_patricia_entry(11 + 1 + 1, [11, 1, 1]),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 68 at r1 (raw file):

        (NodeIndex::from(13), LeafData::StorageValue(Felt::from(2_u128))),
    ]
    ),

implement node_data_map(data: [(u8, u8)]) -> HashMap<NodeIndex, LeafData> and use it every case

Suggestion:

    node_data_map([(8, 4), (10, 3), (13, 2)]),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 83 at r1 (raw file):

        (NodeIndex::from(15), OriginalSkeletonNode::LeafOrBinarySibling(HashOutput(Felt::from(15_u128)))),
        (NodeIndex::from(11), OriginalSkeletonNode::LeafOrBinarySibling(HashOutput(Felt::from(11_u128)))),
    ]),

I can't think of a way to reduce this boilerplate :(
also, auto-formatters don't really work in macros - and #[cases] is a macro... here is a suggestion

Suggestion:

    HashMap::from([
        (NodeIndex::from(1), OriginalSkeletonNode::Binary),
        (NodeIndex::from(2), OriginalSkeletonNode::Binary),
        (NodeIndex::from(3), OriginalSkeletonNode::Edge {
            path_to_bottom: PathToBottom { path: EdgePath(Felt::from(3_u128)), length: EdgePathLength(2) }
        }),
        (NodeIndex::from(4), OriginalSkeletonNode::Binary),
        (NodeIndex::from(5), OriginalSkeletonNode::Edge {
            path_to_bottom: PathToBottom { path: EdgePath(Felt::from(1_u128)), length: EdgePathLength(1) }
        }),
        (NodeIndex::from(9), OriginalSkeletonNode::LeafOrBinarySibling(HashOutput(Felt::from(9_u128)))),
        (NodeIndex::from(15), OriginalSkeletonNode::LeafOrBinarySibling(HashOutput(Felt::from(15_u128)))),
        (NodeIndex::from(11), OriginalSkeletonNode::LeafOrBinarySibling(HashOutput(Felt::from(11_u128)))),
    ]),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 86 at r1 (raw file):

    TreeHeight(3)
)]
/*                 Old tree structure:

triple-slash it

Code quote:

/* 

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 106 at r1 (raw file):

                       12      5     11
                      /  \      \
                          2

Suggestion:

                             B
                           /    \
                         E       B
                        /      /    \
                       B       E     E
                      /  \      \     \
                     NZ   2      NZ    NZ

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 175 at r1 (raw file):

                     24         \   6   59
                                 \  /     \
                                 20 5     40

fix to skeleton format

Code quote:

                             116
                           /     \
                        26        90
                        /      /     \
                       /      25      65
                      /        \     /  \
                     24         \   6   59
                                 \  /     \
                                 20 5     40

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 233 at r1 (raw file):

    TreeHeight(4)
)]
fn test_fetch_nodes(

do you have a case with a sibling? a node that appears as a sibling in the skeleton?

Code quote:

test_fetch_nodes

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 236 at r1 (raw file):

    #[case] storage: HashMap<StorageKey, StorageValue>,
    #[case] leaf_modifications: HashMap<NodeIndex, LeafData>,
    #[case] root_hash: StorageKey,

wrong type for a root_hash - storage key is very general (can be any number of bytes)

Code quote:

root_hash: StorageKey

[TzahiTaub]

Reviewed 4 of 14 files at r1.
Reviewable status: 13 of 14 files reviewed, 48 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

---

crates/committer/src/types.rs line 62 at r1 (raw file):

            .bits()
            .try_into()
            .expect("Should not fail as it takes less than 252 bits to represent a felt.")

This looks like a comment msg. An error message should be clear without context - "Unexpected failure extracting bits from Felt" or something.

Code quote:

"Should not fail as it takes less than 252 bits to represent a felt.

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 33 at r1 (raw file):

#[allow(dead_code)]
#[derive(Debug, PartialEq, Eq)]

Suggestion:

Debug, Eq, PartialEq

[amosStarkware]

Reviewable status: 13 of 14 files reviewed, 51 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree.rs line 4 at r1 (raw file):

pub mod filled_node;
pub mod filled_tree;
pub mod original_skeleton_calc;

why not do this in original_skeleton_tree.rs? @dorimedini-starkware WYT?

Code quote:

pub mod original_skeleton_calc;

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, dorimedini-starkware wrote…

question (I don't know the answer): why should SkeletonTree not own the leaf modifications?

IMO it shouldn't have this field - the modifications are passed as a param to compute_updated_skeleton_tree()

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 24 at r1 (raw file):

#[allow(dead_code)]
impl SkeletonTree<'_> {
    fn fetch_nodes(

this is the same as fetch_nodes() in Python, right?
did that implementation look good to you?
(maybe it's possible to come up with a better one)

Code quote:

fn fetch_nodes(

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree.rs line 20 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this needs to be renamed if it's no longer a collection of indices

these aren't the modified leaves - these are the indices of the leaves in the original tree. @nimrod-starkware can you explain why this change is needed?

---

crates/committer/src/storage/storage_trait.rs line 42 at r1 (raw file):

                "Could not deserailize a binary node when building the original skeleton tree."
                    .to_string(),
            ));

This is probably not relevant anymore, but IMO it would be better to have two separate error variants, instead of one with a configurable error message.

Code quote:

            return Err(OriginalSkeletonTreeError::Deserializtion(
                "Could not deserailize a binary node when building the original skeleton tree."
                    .to_string(),
            ));

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.11765% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 73.56%. Comparing base (aa9bb60) to head (aab94a3).

Files	Patch %	Lines
...src/patricia_merkle_tree/original_skeleton_calc.rs	91.91%	8 Missing and 3 partials ⚠️
...rates/committer/src/patricia_merkle_tree/errors.rs	0.00%	1 Missing ⚠️
crates/committer/src/types.rs	93.33%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #39       +/-   ##
===========================================
+ Coverage   60.74%   73.56%   +12.82%     
===========================================
  Files          15       17        +2     
  Lines         377      594      +217     
  Branches      377      594      +217     
===========================================
+ Hits          229      437      +208     
- Misses        120      127        +7     
- Partials       28       30        +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nimrod-starkware]

Reviewable status: 3 of 14 files reviewed, 51 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @TzahiTaub)

---

Cargo.toml line 21 at r1 (raw file):

Previously, dorimedini-starkware wrote…

alphabetize dependencies

Done.

---

crates/committer/Cargo.toml line 19 at r1 (raw file):

derive_more.workspace = true
starknet-types-core.workspace = true
bisection.workspace = true

Done.

---

crates/committer/src/types.rs line 15 at r1 (raw file):

    Ord,
    derive_more::Sub,
)]

Done.

---

crates/committer/src/types.rs line 65 at r1 (raw file):

Previously, dorimedini-starkware wrote…

depending on use case, this can be misleading.
what should (PRIME-1).shift_left(1) return? i.e. what about overflow?

I agree it may be misleading. maybe a better name is times_two_to_the_power(&self, power: u8).
I am pretty sure that in case of overflow, it returns the result % PRIME (like we expect). However, I'm wondering how efficient it is.

---

crates/committer/src/types.rs line 66 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is there no built-in method of StarknetTypesFelt that does this?

couldn't find one..

---

crates/committer/src/types.rs line 70 at r1 (raw file):

Previously, dorimedini-starkware wrote…

while you're here, these should be pub(crate) unless we explicitly decide to include these functions as part of public API

Done.

---

crates/committer/src/types.rs line 73 at r1 (raw file):

Previously, dorimedini-starkware wrote…

newlines between functions

Done.

---

crates/committer/src/types.rs line 78 at r1 (raw file):

Previously, dorimedini-starkware wrote…

while you're here...

Done.

---

crates/committer/src/patricia_merkle_tree/errors.rs line 5 at r1 (raw file):

#[allow(dead_code)]
pub(crate) enum OriginalSkeletonTreeError {
    Deserializtion(String),

Done.

---

crates/committer/src/patricia_merkle_tree/errors.rs line 6 at r1 (raw file):

Previously, dorimedini-starkware wrote…

we have a StorageError type, please use it

will do

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 9 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I wonder why rustfmt doesn't handle this...

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 14 at r1 (raw file):

Previously, dorimedini-starkware wrote…

do not use super::, use crate::, unless in a test module X_test.rs and you are importing from X.rs

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 17 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. consistency (we have other Impl structs IIRC)
2. Without the word "original", there is ambiguity (this struct represents the skeleton tree before or after update?)

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, dorimedini-starkware wrote…

question (I don't know the answer): why should SkeletonTree not own the leaf modifications?

I think it actually can. changed it to have ownership of it.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 22 at r1 (raw file):

    tree_height: TreeHeight,
}
#[allow(dead_code)]

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 24 at r1 (raw file):

Previously, dorimedini-starkware wrote…

add a docstring

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 26 at r1 (raw file):

Previously, dorimedini-starkware wrote…

the final result of this function is to populate this vector, correct?
is the conversion of this vector to a struct implementing OriginalSkeletonTree a WIP at the moment?
I.e. you are planning to keep fetch_nodes a private method, and the public entry point in this struct will return an instance?

No, the final result is to populate OriginalSkeletonTreeImpl.nodes.
I think that the function should be private and there will be some kind of static method
OriginalSkeletonTreeImpl::build(tree_height: TreeHeight, leaf_modifications: HashMap<NodeIndex, LeafData>) -> Self that creates an instance and calls fetch_nodes on that instance and returns it.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 42 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I don't think you need the extra parens

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 44 at r1 (raw file):

Previously, dorimedini-starkware wrote…

replace the if root_val.is_X() if-else with:

root_node = root_val.deserialize()?;  // See below comment about skeleton node deserialization
match root_node {
    ...
}

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 89 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why do you need this special case?
if you move to matching on skeleton node types, one of the branches should be of leaf type, and you won't need this if here, right?

not exactly, we don't want to call fetch nodes with a subtree representing a leaf (it requires extra context when reading from storage)..

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 149 at r1 (raw file):

Previously, dorimedini-starkware wrote…

move this definition new to the original skeleton tree implementation struct

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 173 at r1 (raw file):

Previously, dorimedini-starkware wrote…

since the tree logic depends on the subtree logic, I think it's more readable if the implementation of SubTree is above the implementation of the skeleton

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 177 at r1 (raw file):

Previously, dorimedini-starkware wrote…

move to top of file, right under imports (with a newline)

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 39 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this is more precise, right?

1. Binary non-sibling nodes have no data
2. Edge nodes have data (path data) but it is described in the ASCII art
3. Leaves that don't change are siblings (have a "hash" value used to update the skeleton), 9 in this example
4. Updated leaves are either zero or nonzero

I think that in this case, it's better to describe it like this

                          B
                        /   \
                      B     E
                     / \      \
                    B   E     \
                    / \   \      \
                NZ  9  11   15

11 and 9 are siblings, we need their values in order to hash (not enough to know that it's not empty)
15 is needed if leaf at index 14 was modified

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 41 at r1 (raw file):

Previously, dorimedini-starkware wrote…

triple slash it (use alt+shift+mouse click in VSCode to edit multiple lines at once :) )

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 83 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I can't think of a way to reduce this boilerplate :(
also, auto-formatters don't really work in macros - and #[cases] is a macro... here is a suggestion

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 86 at r1 (raw file):

Previously, dorimedini-starkware wrote…

triple-slash it

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 106 at r1 (raw file):

                       12      5     11
                      /  \      \
                          2

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 175 at r1 (raw file):

Previously, dorimedini-starkware wrote…

fix to skeleton format

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 233 at r1 (raw file):

Previously, dorimedini-starkware wrote…

do you have a case with a sibling? a node that appears as a sibling in the skeleton?

yes, at the last example the node 24 is a binary sibling

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 236 at r1 (raw file):

Previously, dorimedini-starkware wrote…

wrong type for a root_hash - storage key is very general (can be any number of bytes)

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree.rs line 20 at r1 (raw file):

Previously, amosStarkware wrote…

these aren't the modified leaves - these are the indices of the leaves in the original tree. @nimrod-starkware can you explain why this change is needed?

After talking with @amosStarkware we agreed to keep it like that (leaf_indices). reverted

---

crates/committer/src/patricia_merkle_tree/types.rs line 61 at r1 (raw file):

    PartialOrd,
    Ord,
    derive_more::Sub,

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 78 at r1 (raw file):

        NodeIndex(index.0 * Felt::TWO.pow(length.0) + path.0)
    }
    pub(crate) fn shift_left(&self, steps: u8) -> Self {

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 88 at r1 (raw file):

    }
}
#[allow(dead_code)]

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 111 at r1 (raw file):

pub(crate) struct EdgeData {
    pub bottom_hash: HashOutput,
    pub path_to_bottom: PathToBottom,

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 126 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why is this method useful?

I use it in fetch_nodes. we don't want to call it on a subtree of height zero (leaves). so we check before if were on the last layer before the leaves (i.e height == 1)..
it can be replaced with if tree_height.0 == 1 {.... but I think it better to keep it that way (with maybe finding a better name)

---

crates/committer/src/patricia_merkle_tree/types.rs line 133 at r1 (raw file):

Previously, dorimedini-starkware wrote…

please sync with Aner, he may have something similar

@aner-starkware did you write something similar?

---

crates/committer/src/storage/storage_trait.rs line 33 at r1 (raw file):

Previously, dorimedini-starkware wrote…

The implementations here should not be implemented on storage objects.
we store objects that are not tree-related in storage, and these functions are undefined on such objects.

You are deserializing nodes from storage, right? and they are "filled nodes", in the sense that you are not reading a skeleton from storage, you are actually reading real nodes with hashes and data?

If so, sync with Aviv - the serialization part (of your deserialization) is in his court, you will probably be sharing API

Please move these functions; I suggest:

1. a SerializableSkeletonNode trait with a single deserialize(value: StorageValue) -> SkeletonNode function (not sure about the return type... may need a generic).
2. The function may also need the StorageKey as input, to check the node type... not sure
3. implement the trait for OriginalSkeletonNode

This function should if/else to find the node type and deserialize; do not define separate functions (deserialize_binary_node and deserialize_edge_node) that can panic; using a single entry point we can only ever panic if the input does not match any known node type.

Done.

---

crates/committer/src/storage/storage_trait.rs line 35 at r1 (raw file):

Previously, dorimedini-starkware wrote…

You should have no magic numbers here. i.e, no writing 32, 64, 65.
Please sync with Aviv, he has such constants defined IIRC

will do. i'm waiting for his PR to merge

---

crates/committer/src/storage/storage_trait.rs line 78 at r1 (raw file):

Previously, dorimedini-starkware wrote…

Please give this string a name.
I suggest an enum of node prefixes, with a to_bytes method:

enum StoragePrefix {
    PatriciaNode,
    ...
}

impl StoragePrefix {
    pub(crate) fn to_bytes(&self) -> &[u8] {
        match self {
            Self::PatriciaNode => "patricia_node:".as_bytes(),
            ...
        }
    }
}

And then to use it, write StoragePrefix::PatriciaNode.to_bytes().

Done.

---

crates/committer/src/hash/types.rs line 9 at r1 (raw file):

Previously, dorimedini-starkware wrote…

we will try to keep these ordered alphabetically, unless context requires otherwise (Eq next to PartialEq)

Done.

[amosStarkware]

Reviewable status: 3 of 14 files reviewed, 50 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, nimrod-starkware wrote…

I think it actually can. changed it to have ownership of it.

Why is it needed?

[TzahiTaub]

Reviewed 6 of 14 files at r1, 1 of 11 files at r2.
Reviewable status: 4 of 14 files reviewed, 53 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 22 at r1 (raw file):

Previously, nimrod-starkware wrote…

Done.

Nope

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 33 at r2 (raw file):

    /// Function's purpose is to fetch from storage the patricia witnesses in order to build the
    /// original skeleton tree. Given a list of subtrees, traverses towards their leaves and
    /// fetches all non-empty and sibiling nodes. Assumes no subtrees of height 0 (leaves).

Suggestion:

    /// Fetches the Patricia witnesses, required to build the original skeleton tree from storage. 
    /// Given a list of subtrees, traverses towards their leaves and
    /// fetches all non-empty and sibling nodes. Assumes no subtrees of height 0 (leaves).

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 41 at r1 (raw file):

Previously, nimrod-starkware wrote…

Done.

I'm not sure I follow. Should this also follow the H(node)=h(value, path) + length for edge nodes? Shouldn't the 20 (right son of the root) here be h(15, 0b111) + 3=15+7+3=25? Be happy for the numbers calculation explanation as I don't think it's very straightforward (like the value of leaves is their index, for binary nodes h(left, right) is the sum of their descendants, and for edge nodes h(value, path) + length = value+path+length

---

crates/committer/src/patricia_merkle_tree/original_skeleton_node.rs line 5 at r1 (raw file):

#[allow(dead_code)]
#[derive(PartialEq, Eq, Debug)]

Order

---

crates/committer/src/patricia_merkle_tree/types.rs line 89 at r1 (raw file):

}
#[allow(dead_code)]
#[derive(Debug, PartialEq, Eq)]

Order all

Code quote:

Debug, PartialEq, Eq

[amosStarkware]

Reviewable status: 4 of 14 files reviewed, 54 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 24 at r1 (raw file):

#[allow(dead_code)]
impl SkeletonTree<'_> {
    fn fetch_nodes(

IMO this method should be divided into sub methods - it's hard to read

Code quote:

fn fetch_nodes(

[dorimedini-starkware]

Reviewed 11 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 37 unresolved discussions (waiting on @amosStarkware, @aner-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree.rs line 4 at r1 (raw file):

Previously, amosStarkware wrote…

why not do this in original_skeleton_tree.rs? @dorimedini-starkware WYT?

let's think about it later; moving code in this PR will be painful

---

crates/committer/src/types.rs line 62 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This looks like a comment msg. An error message should be clear without context - "Unexpected failure extracting bits from Felt" or something.

why?

---

crates/committer/src/types.rs line 65 at r1 (raw file):

Previously, nimrod-starkware wrote…

I agree it may be misleading. maybe a better name is times_two_to_the_power(&self, power: u8).
I am pretty sure that in case of overflow, it returns the result % PRIME (like we expect). However, I'm wondering how efficient it is.

ok, then please rename so it's clear what is done.
as for efficiency - I wouldn't worry about it, actual usage won't overflow

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 75 at r2 (raw file):

        value: &StorageValue,
    ) -> OriginalSkeletonTreeResult<Self> {
        // TODO(Nimrod, 30/4/2024): Compare to constant values once Aviv's PR is merged.

even better, provide a link :)

Suggestion:

PR #18

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 85 at r2 (raw file):

            })
        }
        // TODO(Nimrod, 30/4/2024): Compare to constant values once Aviv's PR is merged.

Suggestion:

PR #18

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 101 at r2 (raw file):

            Err(OriginalSkeletonTreeError::Deserialization(
                "Could not deserialize given key and value.".to_string(),
            ))

Suggestion:

            todo!("Deserialize leaves.")

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, amosStarkware wrote…

Why is it needed?

ah, right... modifications aren't part of the "original skeleton".
@nimrod-starkware you should be able to remove it from the struct, you don't actually use this in fetch_nodes (right?), only @TzahiTaub will need it.
you only need a collection of leaf indices in the initial SubTree

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 89 at r1 (raw file):

Previously, nimrod-starkware wrote…

not exactly, we don't want to call fetch nodes with a subtree representing a leaf (it requires extra context when reading from storage)..

can you explain? it makes this code more difficult to follow

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 149 at r1 (raw file):

Previously, nimrod-starkware wrote…

Done.

sorry... I meant, move this struct definition above the impl OriginalSkeletonTreeImpl block

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 173 at r1 (raw file):

Previously, nimrod-starkware wrote…

Done.

where?

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 33 at r2 (raw file):

    /// Function's purpose is to fetch from storage the patricia witnesses in order to build the
    /// original skeleton tree. Given a list of subtrees, traverses towards their leaves and
    /// fetches all non-empty and sibiling nodes. Assumes no subtrees of height 0 (leaves).

typo

Suggestion:

sibling

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 39 at r2 (raw file):

        storage: HashMap<StorageKey, StorageValue>,
        // TODO(Nimrod, 25/4/2024): Change input type to Storage once Amos has implemented the
        // Storage trait for HashMap.

this is merged, no? PR #40

Code quote:

        // TODO(Nimrod, 25/4/2024): Change input type to Storage once Amos has implemented the
        // Storage trait for HashMap.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 121 at r2 (raw file):

                    let rightmost_in_subtree = leftmost_in_subtree
                        + (NodeIndex(Felt::ONE).shift_left(bottom_height.0))
                        - NodeIndex(Felt::ONE);

this chunk of logic should be implemented on some SkeletonNode (see above);
can you open a separate PR for this, and rebase the current PR over it?

Code quote:

                    let bottom_height =
                        subtree.get_height(&self.tree_height) - TreeHeight(path_to_bottom.length.0);
                    let leftmost_in_subtree = bottom_index.shift_left(bottom_height.0);
                    let rightmost_in_subtree = leftmost_in_subtree
                        + (NodeIndex(Felt::ONE).shift_left(bottom_height.0))
                        - NodeIndex(Felt::ONE);

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 152 at r2 (raw file):

                        "Fetch nodes called on a leaf subtree. Should not happen.".to_string(),
                    ))
                }

I think it is worth adding extra context to storage (as you mention above) to avoid this scenario... would like to understand what the issue is

Code quote:

                NodeData::Leaf(_) => {
                    return Err(OriginalSkeletonTreeError::Deserialization(
                        "Fetch nodes called on a leaf subtree. Should not happen.".to_string(),
                    ))
                }

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 39 at r1 (raw file):

Previously, nimrod-starkware wrote…

I think that in this case, it's better to describe it like this

                          B
                        /   \
                      B     E
                     / \      \
                    B   E     \
                    / \   \      \
                NZ  9  11   15

11 and 9 are siblings, we need their values in order to hash (not enough to know that it's not empty)
15 is needed if leaf at index 14 was modified

ah... maybe I'm misunderstanding how the original skeleton represents leaves.
the new leaves are not part of the skeleton?
that's why leaf number 13 does not appear in the example output?
so really no need to write NZ... sorry

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 48 at r2 (raw file):

    (create_patricia_key(9), StorageValue(create_32_bytes_entry(9))),
    (create_patricia_key(11), StorageValue(create_32_bytes_entry(11))),
    (create_patricia_key(15), StorageValue(create_32_bytes_entry(15))),

reduce boilerplate

Suggestion:

    single_storage_entry(8, 8),
    single_storage_entry(9, 9),
    single_storage_entry(11,11),
    single_storage_entry(15, 15),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 55 at r2 (raw file):

    (create_patricia_key(15 + 3 + 2), create_edge_val(15, 3, 2)),
    (create_patricia_key(30 + 20), create_binary_val(30, 20)),
    ]),

less duplication

Suggestion:

    create_binary_entry(8, 9),
    create_edge_entry(11, 1, 1),
    create_binary_entry(17, 13),
    create_edge_entry(15, 3, 2),
    create_binary_entry(30, 20),
),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 63 at r2 (raw file):

        (NodeIndex::from(3), OriginalSkeletonNode::Edge { path_to_bottom: PathToBottom {
            path: EdgePath(Felt::from(3_u128)), length: EdgePathLength(2)
        } }),

this is what I'm trying to avoid; also

X { y: Y {
   bla
}}

is not as good as

X {
    y: Y { bla }
}

Code quote:

 } }),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 103 at r2 (raw file):

///

#[case::another_simple_tree_of_height_3(

fix boilerplate (see above)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 137 at r2 (raw file):

            HashOutput(Felt::from(2_u128))
        ))
    ]),

fix macro formatting (see above)

Code quote:

        (NodeIndex::from(1), OriginalSkeletonNode::Binary),
        (NodeIndex::from(2), OriginalSkeletonNode::Edge {
            path_to_bottom: PathToBottom { path: EdgePath(Felt::ZERO), length: EdgePathLength(1) }
        }),
        (NodeIndex::from(3), OriginalSkeletonNode::Binary),
        (NodeIndex::from(4), OriginalSkeletonNode::Binary),
        (NodeIndex::from(6), OriginalSkeletonNode::Edge {
            path_to_bottom: PathToBottom {
                path: EdgePath(Felt::from(1_u128)), length: EdgePathLength(1)
            }
        }),
        (NodeIndex::from(7), OriginalSkeletonNode::LeafOrBinarySibling(
            HashOutput(Felt::from(11_u128))
        )),
        (NodeIndex::from(9), OriginalSkeletonNode::LeafOrBinarySibling(
            HashOutput(Felt::from(2_u128))
        ))
    ]),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 158 at r2 (raw file):

///                              B
///                           /     \
///                         E        B

maybe remane so it's clear it's different than a regular edge?

Suggestion:

  ES

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 168 at r2 (raw file):

#[case::tree_of_height_4_with_long_edge(
    HashMap::from([
    (create_patricia_key(11), StorageValue(create_32_bytes_entry(11))),

fix boilerplate (see above)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 211 at r2 (raw file):

        (NodeIndex::from(31), OriginalSkeletonNode::LeafOrBinarySibling(
            HashOutput(Felt::from(40_u128))
        )),

fix macro formatting (see above)

Code quote:

        (NodeIndex::from(1), OriginalSkeletonNode::Binary),
        (NodeIndex::from(2), OriginalSkeletonNode::Edge {
            path_to_bottom: PathToBottom { path: EdgePath(Felt::ZERO), length: EdgePathLength(2) }
        }),
        (NodeIndex::from(3), OriginalSkeletonNode::Binary),
        (NodeIndex::from(6), OriginalSkeletonNode::Edge {
            path_to_bottom: PathToBottom { path: EdgePath(Felt::from(3_u128)),
                                           length: EdgePathLength(2) }
        }),
        (NodeIndex::from(7), OriginalSkeletonNode::Binary),
        (NodeIndex::from(8), OriginalSkeletonNode::LeafOrBinarySibling(
            HashOutput(Felt::from(24_u128))
        )),
        (NodeIndex::from(14), OriginalSkeletonNode::Edge {
            path_to_bottom: PathToBottom {path: EdgePath(Felt::ZERO), length: EdgePathLength(1)}
        }),
        (NodeIndex::from(15), OriginalSkeletonNode::Binary),
        (NodeIndex::from(27), OriginalSkeletonNode::LeafOrBinarySibling(
            HashOutput(Felt::from(20_u128))
        )),
        (NodeIndex::from(28), OriginalSkeletonNode::LeafOrBinarySibling(
            HashOutput(Felt::from(5_u128))
        )),
        (NodeIndex::from(31), OriginalSkeletonNode::LeafOrBinarySibling(
            HashOutput(Felt::from(40_u128))
        )),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 234 at r2 (raw file):

        sorted_leaf_indices: &sorted_leaf_indices,
        root_index: NodeIndex(Felt::ONE),
        root_hash: StorageKey::from(root_hash.0),

why is the type of root_hash in SubTree a StorageKey and not a HashOutput?

Code quote:

root_hash: StorageKey::from(root_hash.0)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 258 at r2 (raw file):

    )
}
#[allow(dead_code)]

newlines between function defs

Suggestion:

}

#[allow(dead_code)]
fn create_binary_val(left: u8, right: u8) -> StorageValue {
    StorageValue(
        (create_32_bytes_entry(left)
            .into_iter()
            .chain(create_32_bytes_entry(right)))
        .collect(),
    )
}

#[allow(dead_code)]

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 258 at r2 (raw file):

    )
}
#[allow(dead_code)]

why do you need allow_dead_code? you're using it in the test

Code quote:

#[allow(dead_code)]
fn create_binary_val(left: u8, right: u8) -> StorageValue {
    StorageValue(
        (create_32_bytes_entry(left)
            .into_iter()
            .chain(create_32_bytes_entry(right)))
        .collect(),
    )
}
#[allow(dead_code)]

---

crates/committer/src/patricia_merkle_tree/types.rs line 126 at r1 (raw file):

Previously, nimrod-starkware wrote…

I use it in fetch_nodes. we don't want to call it on a subtree of height zero (leaves). so we check before if were on the last layer before the leaves (i.e height == 1)..
it can be replaced with if tree_height.0 == 1 {.... but I think it better to keep it that way (with maybe finding a better name)

see above, I want us to try to call the fetch_nodes function on leaves as well for cleaner logic

[amosStarkware]

Reviewable status: all files reviewed, 38 unresolved discussions (waiting on @aner-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 71 at r2 (raw file):

#[allow(dead_code)]
impl FilledNode<LeafData> {
    pub(crate) fn deserialize(

I liked @dorimedini-starkware 's suggestion of creating a trait for serialization (and deserialization) - @nimrod-starkware you don't like it?

Code quote:

pub(crate) fn deserialize(

[amosStarkware]

Reviewable status: all files reviewed, 39 unresolved discussions (waiting on @aner-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 74 at r2 (raw file):

        key: &StorageKey,
        value: &StorageValue,
    ) -> OriginalSkeletonTreeResult<Self> {

@AvivYossef-starkware created types for serializing nodes, please look here
https://reviewable.io/reviews/starkware-libs/committer/18

Code quote:

) -> OriginalSkeletonTreeResult<Self> {

[dorimedini-starkware]

Reviewable status: all files reviewed, 39 unresolved discussions (waiting on @amosStarkware, @aner-starkware, @AvivYossef-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 74 at r2 (raw file):

Previously, amosStarkware wrote…

@AvivYossef-starkware created types for serializing nodes, please look here
https://reviewable.io/reviews/starkware-libs/committer/18

yes this is in reverse. we will combine to be an implementation of a single trait later, when both PRs are in, otherwise we will get annoying conflicts

[TzahiTaub]

Reviewed 1 of 11 files at r2.
Reviewable status: 13 of 14 files reviewed, 37 unresolved discussions (waiting on @amosStarkware, @aner-starkware, @AvivYossef-starkware, @dorimedini-starkware, and @nimrod-starkware)

---

crates/committer/src/hash/types.rs line 9 at r1 (raw file):

Previously, nimrod-starkware wrote…

Done.

Why does Eq come after PartialEq?

[TzahiTaub]

Dismissed @nimrod-starkware from 9 discussions.
Reviewable status: 13 of 14 files reviewed, 37 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @dorimedini-starkware, and @nimrod-starkware)

[TzahiTaub]

Reviewable status: 13 of 14 files reviewed, 39 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @dorimedini-starkware, and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 75 at r3 (raw file):

        }
        let mut next_subtrees = Vec::new();
        let mut filled_nodes = Vec::new();

If this is right, I think it's clearer.

Suggestion:

  let mut subtrees_roots

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 80 at r3 (raw file):

                .get(&subtree.root_hash.with_prefix(StoragePrefix::PatriciaNode))
                .ok_or(OriginalSkeletonTreeError::StorageRead)?;
            filled_nodes.push(FilledNode::deserialize(&subtree.root_hash, val)?)

A FilledNode is the original node prior to the update?

Code quote:

 filled_nodes.push(FilledNode

[dorimedini-starkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 37 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 34 at r3 (raw file):

    pub root_hash: StorageKey,
}
#[allow(dead_code)]

newlines

Suggestion:

}

#[allow(dead_code)]
struct SubTree<'a> {
    pub sorted_leaf_indices: &'a [NodeIndex],
    pub root_index: NodeIndex,
    pub root_hash: StorageKey,
}

#[allow(dead_code)]

[nimrod-starkware]

Reviewable status: 6 of 14 files reviewed, 36 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @dorimedini-starkware, @n, and @TzahiTaub)

---

crates/committer/src/types.rs line 65 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ok, then please rename so it's clear what is done.
as for efficiency - I wouldn't worry about it, actual usage won't overflow

Done.

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 71 at r2 (raw file):

Previously, amosStarkware wrote…

I liked @dorimedini-starkware 's suggestion of creating a trait for serialization (and deserialization) - @nimrod-starkware you don't like it?

asf

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 101 at r2 (raw file):

            Err(OriginalSkeletonTreeError::Deserialization(
                "Could not deserialize given key and value.".to_string(),
            ))

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 33 at r2 (raw file):

    /// Function's purpose is to fetch from storage the patricia witnesses in order to build the
    /// original skeleton tree. Given a list of subtrees, traverses towards their leaves and
    /// fetches all non-empty and sibiling nodes. Assumes no subtrees of height 0 (leaves).

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 39 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ah... maybe I'm misunderstanding how the original skeleton represents leaves.
the new leaves are not part of the skeleton?
that's why leaf number 13 does not appear in the example output?
so really no need to write NZ... sorry

yes, new leaves are not part of the old skeleton.
I think it actually not a bad idea, when we want to indicate there was a NZ leaf, but it's actual value doesn't matter.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 41 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I'm not sure I follow. Should this also follow the H(node)=h(value, path) + length for edge nodes? Shouldn't the 20 (right son of the root) here be h(15, 0b111) + 3=15+7+3=25? Be happy for the numbers calculation explanation as I don't think it's very straightforward (like the value of leaves is their index, for binary nodes h(left, right) is the sum of their descendants, and for edge nodes h(value, path) + length = value+path+length

the length is 2 and the path is 3 (0b11) :)
I agree that the draw may be confusing

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 48 at r2 (raw file):

Previously, dorimedini-starkware wrote…

reduce boilerplate

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 55 at r2 (raw file):

Previously, dorimedini-starkware wrote…

less duplication

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 63 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this is what I'm trying to avoid; also

X { y: Y {
   bla
}}

is not as good as

X {
    y: Y { bla }
}

changed. not sure it's the format though.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 103 at r2 (raw file):

Previously, dorimedini-starkware wrote…

fix boilerplate (see above)

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 168 at r2 (raw file):

Previously, dorimedini-starkware wrote…

fix boilerplate (see above)

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 211 at r2 (raw file):

Previously, dorimedini-starkware wrote…

fix macro formatting (see above)

Done. not sure it's the right format

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 234 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why is the type of root_hash in SubTree a StorageKey and not a HashOutput?

changed to HashOutput. my bad

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 258 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why do you need allow_dead_code? you're using it in the test

removed it

---

crates/committer/src/storage/storage_trait.rs line 42 at r1 (raw file):

Previously, amosStarkware wrote…

This is probably not relevant anymore, but IMO it would be better to have two separate error variants, instead of one with a configurable error message.

I agree. I added a TODO
// TODO(Nimrod, 5/5/2024): If possible, divide Deserialization to more specific types of errors.
I think it will be clearer in the future how to optimize it.

[nimrod-starkware]

Reviewable status: 6 of 14 files reviewed, 36 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @dorimedini-starkware, @n, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 71 at r2 (raw file):

Previously, nimrod-starkware wrote…

asf

oops, it's a mistake above.

I like the idea as well. but I think it's best to do that in a future PR.

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 75 at r2 (raw file):

Previously, dorimedini-starkware wrote…

even better, provide a link :)

Done.

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 85 at r2 (raw file):

            })
        }
        // TODO(Nimrod, 30/4/2024): Compare to constant values once Aviv's PR is merged.

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 75 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

If this is right, I think it's clearer.

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 80 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

A FilledNode is the original node prior to the update?

yes exactly.

[TzahiTaub]

Reviewable status: 10 of 17 files reviewed, 15 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @dorimedini-starkware, @n, and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 41 at r1 (raw file):

Previously, nimrod-starkware wrote…

done

Great. Please do this change for all tree drawings in the file

[nimrod-starkware]

Reviewable status: 10 of 17 files reviewed, 15 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @dorimedini-starkware, @n, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc_test.rs line 41 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Great. Please do this change for all tree drawings in the file

Done.

[dorimedini-starkware]

Reviewed 1 of 2 files at r9, 3 of 4 files at r10, 1 of 2 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, @n, @nimrod-starkware, and @TzahiTaub)

[dorimedini-starkware]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 72 at r13 (raw file):

#[allow(dead_code)]
impl FilledNode<LeafData> {
    pub(crate) fn deserialize(

Suggestion:

impl FilledNode<LeafData> {
    /// Deserializes non-leaf nodes; if a serialized leaf node is given, the hash
    /// is used but the data is ignored.
    pub(crate) fn deserialize(

[nimrod-starkware]

Reviewable status: 7 of 17 files reviewed, 10 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/errors.rs line 6 at r1 (raw file):

Previously, nimrod-starkware wrote…

will do

done

---

crates/committer/src/patricia_merkle_tree/filled_node.rs line 72 at r13 (raw file):

#[allow(dead_code)]
impl FilledNode<LeafData> {
    pub(crate) fn deserialize(

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 121 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this chunk of logic should be implemented on some SkeletonNode (see above);
can you open a separate PR for this, and rebase the current PR over it?

will do

---

crates/committer/src/storage/storage_trait.rs line 35 at r1 (raw file):

Previously, nimrod-starkware wrote…

will do. i'm waiting for his PR to merge

done

[dorimedini-starkware]

Reviewed 10 of 10 files at r14, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

[dorimedini-starkware]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

[dorimedini-starkware]

Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

[nimrod-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

---

crates/committer/src/types.rs line 62 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Because this is what the end user will see in case the error is indeed raised for some reason. Like getting an "Internal Error" instead of "this should always succeed so no need for a real error msg".

@dorimedini-starkware @TzahiTaub
What do you think about that?

    pub(crate) fn bits(&self) -> u8 {
        self.0
            .bits()
            .try_into()
            // Should not fail as it takes less than 252 bits to represent a felt.
            .expect("Unexpected error occurred when extracting bits of a Felt.")
    }

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ah, right... modifications aren't part of the "original skeleton".
@nimrod-starkware you should be able to remove it from the struct, you don't actually use this in fetch_nodes (right?), only @TzahiTaub will need it.
you only need a collection of leaf indices in the initial SubTree

will open another PR that solves that

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 24 at r1 (raw file):

Previously, amosStarkware wrote…

IMO this method should be divided into sub methods - it's hard to read

i have refactored it a little bit to be more readable, what do you think @amosStarkware ?

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 149 at r1 (raw file):

Previously, dorimedini-starkware wrote…

sorry... I meant, move this struct definition above the impl OriginalSkeletonTreeImpl block

all the SubTree logic is above the impl OriginalSkeletonTreeImpl block

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 151 at r1 (raw file):

Previously, dorimedini-starkware wrote…

will this struct be useful for Tzahi as well? Please sync

@TzahiTaub?
At the Python repository, it seems like it's not used to build the updated skeleton.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 121 at r2 (raw file):

Previously, nimrod-starkware wrote…

will do

done

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 152 at r2 (raw file):

Previously, dorimedini-starkware wrote…

I think it is worth adding extra context to storage (as you mention above) to avoid this scenario... would like to understand what the issue is

no longer relevant..

[dorimedini-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/types.rs line 62 at r1 (raw file):

Previously, nimrod-starkware wrote…

@dorimedini-starkware @TzahiTaub
What do you think about that?

    pub(crate) fn bits(&self) -> u8 {
        self.0
            .bits()
            .try_into()
            // Should not fail as it takes less than 252 bits to represent a felt.
            .expect("Unexpected error occurred when extracting bits of a Felt.")
    }

lgtm

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, nimrod-starkware wrote…

will open another PR that solves that

add a TODO here then, so we can merge this without worry

[dorimedini-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, dorimedini-starkware wrote…

add a TODO here then, so we can merge this without worry

nvm, PR is open

[nimrod-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 21 at r1 (raw file):

Previously, dorimedini-starkware wrote…

add a TODO here then, so we can merge this without worry

Done.

[TzahiTaub]

Reviewable status: 16 of 17 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_calc.rs line 151 at r1 (raw file):

Previously, nimrod-starkware wrote…

@TzahiTaub?
At the Python repository, it seems like it's not used to build the updated skeleton.

I don't think I need it. I do need the split_leaves though, but I'll take if out after you merge.

[dorimedini-starkware]

Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware)

[dorimedini-starkware]

Dismissed @amosStarkware from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

[dorimedini-starkware]

Dismissed @amosStarkware from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

reviewed