fix: tree hash function for compiled class hash tree

[aner-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.97%. Comparing base (1e19316) to head (d899049).

Files	Patch %	Lines
...merkle_tree/updated_skeleton_tree/hash_function.rs	85.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #230      +/-   ##
==========================================
+ Coverage   64.90%   64.97%   +0.07%     
==========================================
  Files          36       36              
  Lines        1704     1716      +12     
  Branches     1704     1716      +12     
==========================================
+ Hits         1106     1115       +9     
- Misses        550      553       +3     
  Partials       48       48              

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

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 72 at r1 (raw file):

            ),
            NodeData::Leaf(leaf_data) => Self::compute_leaf_hash(leaf_data),
        }

duplicated code.
add a fn compute_node_hash_with_inner_hash_function<H: HashFunction> function in the impl TreeHashFunctionImpl block, and use it, three times.

I think the compiler should be fine with using Self::compute_leaf_hash from the impl TreeHashFunctionImpl block because you implement the trait below; if not, extract the common logic to a standalone function

Code quote:

    fn compute_node_hash(node_data: &NodeData<ContractState>) -> HashOutput {
        match node_data {
            NodeData::Binary(BinaryData {
                left_hash,
                right_hash,
            }) => HashOutput(Pedersen::hash(&left_hash.0.into(), &right_hash.0.into()).into()),
            NodeData::Edge(EdgeData {
                bottom_hash: hash_output,
                path_to_bottom: PathToBottom { path, length, .. },
            }) => HashOutput(
                Felt::from(Pedersen::hash(
                    &hash_output.0.into(),
                    &Felt::from(path).into(),
                )) + Felt::from(*length),
            ),
            NodeData::Leaf(leaf_data) => Self::compute_leaf_hash(leaf_data),
        }

[aner-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 72 at r1 (raw file):

Previously, dorimedini-starkware wrote…

duplicated code.
add a fn compute_node_hash_with_inner_hash_function<H: HashFunction> function in the impl TreeHashFunctionImpl block, and use it, three times.

I think the compiler should be fine with using Self::compute_leaf_hash from the impl TreeHashFunctionImpl block because you implement the trait below; if not, extract the common logic to a standalone function

Done.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 36 at r2 (raw file):

        HashOutput(Felt(Poseidon::hash(&left.0, &right.0)))
    }
}

delete (see below, there's a similar trait in starknet_types_core)

Code quote:

/// Trait for hash functions.
pub(crate) trait HashFunction {
    /// Computes the hash of the given input.
    fn hash(left: &Felt, right: &Felt) -> HashOutput;
}

/// Implementation of HashFunction for Pedersen hash function.
pub struct PedersenHashFunction;
impl HashFunction for PedersenHashFunction {
    fn hash(left: &Felt, right: &Felt) -> HashOutput {
        HashOutput(Felt(Pedersen::hash(&left.0, &right.0)))
    }
}

/// Implementation of HashFunction for Poseidon hash function.
pub struct PoseidonHashFunction;
impl HashFunction for PoseidonHashFunction {
    fn hash(left: &Felt, right: &Felt) -> HashOutput {
        HashOutput(Felt(Poseidon::hash(&left.0, &right.0)))
    }
}

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 47 at r2 (raw file):

    fn compute_node_hash(node_data: &NodeData<L>) -> HashOutput;

    fn compute_node_hash_with_inner_hash_function<H: HashFunction>(

both Pedersen and Poseidon implement this trait

Suggestion:

use starknet_types_core::hash::traits::StarkHash;
fn compute_node_hash_with_inner_hash_function<H: StarkHash>

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 47 at r2 (raw file):

Previously, dorimedini-starkware wrote…

both Pedersen and Poseidon implement this trait

you can use the hash(Felt, Felt) -> Felt API of StarkHash to hash your input

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 47 at r2 (raw file):

    fn compute_node_hash(node_data: &NodeData<L>) -> HashOutput;

    fn compute_node_hash_with_inner_hash_function<H: HashFunction>(

docstring is now relevant for the compute_node_hash_with_inner_hash_function function

Suggestion:

    fn compute_node_hash(node_data: &NodeData<L>) -> HashOutput;

    /// The default implementation for internal nodes is based on the following reference:
    /// https://docs.starknet.io/documentation/architecture_and_concepts/Network_Architecture/starknet-state/#trie_construction
    fn compute_node_hash_with_inner_hash_function<H: HashFunction>(

[aner-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 36 at r2 (raw file):

Previously, dorimedini-starkware wrote…

delete (see below, there's a similar trait in starknet_types_core)

See below.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 47 at r2 (raw file):

Previously, dorimedini-starkware wrote…

docstring is now relevant for the compute_node_hash_with_inner_hash_function function

Done.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 47 at r2 (raw file):

Previously, dorimedini-starkware wrote…

you can use the hash(Felt, Felt) -> Felt API of StarkHash to hash your input

But this uses different structs than ours, which undermines the idea of wrapping their Felt with our Felt/HashOutput.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 47 at r2 (raw file):

Previously, aner-starkware wrote…

But this uses different structs than ours, which undermines the idea of wrapping their Felt with our Felt/HashOutput.

good point

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 44 at r3 (raw file):

    fn compute_node_hash(node_data: &NodeData<L>) -> HashOutput;

    /// Computes the hash for the given node data.

I only meant the two lines explaining the default implementation

Suggestion:

    /// Computes the hash for the given node data.
    fn compute_node_hash(node_data: &NodeData<L>) -> HashOutput;

[aner-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 44 at r3 (raw file):

Previously, dorimedini-starkware wrote…

I only meant the two lines explaining the default implementation

Done.

[dorimedini-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 36 at r2 (raw file):

Previously, aner-starkware wrote…

See below.

actually... I think we won't need this extra code when we split into crates...

1. the patricia crate won't need to define the compute_node_hash_with_inner_hash_function method at all
2. the commitment crate won't need to really expose a generic public API; using StarkHash will be good enough

non-blocking for now though. worst case we'll delete this later