fix: move from tokio join to spawn to fix parallelism

[aner-starkware]

This change is 

[codecov-commenter]

Codecov Report

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

Project coverage is 61.56%. Comparing base (524f1af) to head (ed6cce1).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...ter/src/patricia_merkle_tree/filled_tree/forest.rs	0.00%	9 Missing ⚠️
...itter/src/patricia_merkle_tree/filled_tree/tree.rs	86.36%	0 Missing and 3 partials ⚠️
crates/committer/src/block_committer/commit.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   60.29%   61.56%   +1.27%     
==========================================
  Files          34       35       +1     
  Lines        1511     1665     +154     
  Branches     1511     1665     +154     
==========================================
+ Hits          911     1025     +114     
- Misses        559      596      +37     
- Partials       41       44       +3     

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

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 35 at r2 (raw file):

        H: HashFunction + 'a + 'static,
        TH: TreeHashFunction<LeafDataImpl, H> + 'static,
    >(

this doesn't work (needed for the leaf_modifications)?
asking

Suggestion:

    async fn create<
        H: HashFunction + 'static,
        TH: TreeHashFunction<LeafDataImpl, H> + 'static,
    >(

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 37 at r2 (raw file):

    >(
        updated_skeleton: impl UpdatedSkeletonTree + 'static,
        leaf_modifications: &'a LeafModifications<LeafDataImpl>,

why does the lifespan here depend on the lifespan of H?

Code quote:

&'a

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 156 at r2 (raw file):

                    right_hash: right_hash
                        .await
                        .expect("right hash computation did not execute correctly")?,

do not expect here; the result could be a JoinError or something? please add a variant of FilledTreeError that can handle the Err variant of await

Suggestion:

                    left_hash: left_hash.await??,
                    right_hash: right_hash.await??,

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 209 at r2 (raw file):

        updated_skeleton: impl UpdatedSkeletonTree + 'static,
        leaf_modifications: &'a LeafModifications<LeafDataImpl>,
    ) -> Result<Self, FilledTreeError<LeafDataImpl>> {

same question as above about 'a: I don't understand why leaf_modifications is coupled with the H type

Code quote:

    async fn create<
        'a,
        H: HashFunction + 'a + 'static,
        TH: TreeHashFunction<LeafDataImpl, H> + 'static,
    >(
        updated_skeleton: impl UpdatedSkeletonTree + 'static,
        leaf_modifications: &'a LeafModifications<LeafDataImpl>,
    ) -> Result<Self, FilledTreeError<LeafDataImpl>> {

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 218 at r2 (raw file):

            Arc::new(updated_skeleton),
            NodeIndex::ROOT,
            Arc::new(leaf_modifications.clone()),

this clone is expensive and unecessary - you don't need the leaf modifications after this point.
consider changing the input from &'a LeafModifications<LeafDataImpl> (reference) to LeafModifications<LeafDataImpl> (transfer ownership), if it works it seems better IMO

Suggestion:

leaf_modifications

[aner-starkware]

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 35 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this doesn't work (needed for the leaf_modifications)?
asking

H no longer exists.

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 37 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why does the lifespan here depend on the lifespan of H?

I don't know, I don't really understand lifespans so well.

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 156 at r2 (raw file):

Previously, dorimedini-starkware wrote…

do not expect here; the result could be a JoinError or something? please add a variant of FilledTreeError that can handle the Err variant of await

Yes, the result could be a JoinError, but doesn't the expect just unwrap and then return the inner error? Isn't it the better way to handle this specific error in this case?

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 209 at r2 (raw file):

Previously, dorimedini-starkware wrote…

same question as above about 'a: I don't understand why leaf_modifications is coupled with the H type

Same as above

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 218 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this clone is expensive and unecessary - you don't need the leaf modifications after this point.
consider changing the input from &'a LeafModifications<LeafDataImpl> (reference) to LeafModifications<LeafDataImpl> (transfer ownership), if it works it seems better IMO

Apparently, there is some borrowing issue. I replaced the clone here with an Arc::clone above.

[dorimedini-starkware]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @yoavGrs)

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 156 at r2 (raw file):

Previously, aner-starkware wrote…

Yes, the result could be a JoinError, but doesn't the expect just unwrap and then return the inner error? Isn't it the better way to handle this specific error in this case?

expect panics on error; this may be what we want but I'd prefer to let the calling code decide to panic if need be. await?? is better IMO

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 218 at r2 (raw file):

Previously, aner-starkware wrote…

Apparently, there is some borrowing issue. I replaced the clone here with an Arc::clone above.

I still see a clone on the leaf modifications here

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 124 at r3 (raw file):

        TH: TreeHashFunction<LeafDataImpl> + 'static,
    {
        let binding = Arc::clone(&updated_skeleton);

this is an O(1) operation, right? no hashmap is being cloned here?

Code quote:

let binding = Arc::clone(&updated_skeleton);

[aner-starkware]

Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 218 at r2 (raw file):

Previously, dorimedini-starkware wrote…

I still see a clone on the leaf modifications here

Right, sorry, I was looking at a different .clone()

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 124 at r3 (raw file):

Previously, dorimedini-starkware wrote…

this is an O(1) operation, right? no hashmap is being cloned here?

IIUC, that is the point of Arc::clone.

[dorimedini-starkware]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)