chore(skeleton): common split_leaves function

[TzahiTaub]

This change is 

[TzahiTaub]

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @TzahiTaub)

a discussion (no related file):
Waiting for @nimrod-starkware to fix a test, but can still be reviewed.

---

[dorimedini-starkware]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

---

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

    pub const ROOT: Self = Self(U256::ONE);

    #[allow(clippy::as_conversions)]

where is there an as here?

Code quote:

#[allow(clippy::as_conversions)]

---

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

    #[allow(clippy::as_conversions)]
    pub const _FIRST_LEAF: Self = Self(U256::from_words(1_u128 << (Self::BITS - 1 - 128), 0));

so... from_words is const on U256, but lsh is not?
bummer

Code quote:

pub const _FIRST_LEAF: Self = Self(U256::from_words(1_u128 << (Self::BITS - 1 - 128), 0));

---

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

    #[allow(clippy::as_conversions)]
    pub const _FIRST_LEAF: Self = Self(U256::from_words(1_u128 << (Self::BITS - 1 - 128), 0));

remove the underscore - if you want this to be private, remove the pub

Suggestion:

const FIRST_LEAF

---

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

        split_leaves(&self.tree_height, root_index, leaf_indices)
            .iter()
            .all(|x| !x.is_empty())

for clarity

Suggestion:

.all(|leaves_in_side| !leaves_in_side.is_empty())

---

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

pub mod utils_test;

pub(crate) fn get_node_height(tree_height: &TreeHeight, index: &NodeIndex) -> TreeHeight {

docstring (the name is confusing)

---

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

            .last()
            .expect("leaf_indices unexpectedly empty.");
        assert_descendant(last_leaf);

redundant variable (non-blocking)

Suggestion:

        assert_descendant(leaf_indices
            .last()
            .expect("leaf_indices unexpectedly empty.")
        );

---

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

    }

    let right_child_index = (*root_index << 1) + 1.into();

isn't this what was originally written?

Suggestion:

NodeIndex::ROOT

---

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

    if jump == U256::ZERO {
        jump = (U256::MAX - start) / size_u256;
    }

this "feature" is not used, right?

Suggestion:

/// Creates an array of increasing random U256 numbers, with jumps of up to 'jump' between two
/// consecutive numbers.
fn create_increasing_random_array(size: usize, start: U256, jump: U256) -> Vec<U256> {
    let size_u256: U256 = size.try_into().unwrap();

---

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

#[should_panic]
#[case::small_tree_not_descendant(
        3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[])]

no auto-format in macros (like #[case] attributes)...
please try to conform to "correct" formatting manually

Suggestion:

#[case::small_tree_one_side(
    3_u8, U256::ONE, &[uint!("8"), uint!("10"), uint!("11")],
    &[uint!("8"), uint!("10"), uint!("11")], &[]
)]
#[case::small_tree_two_sides(
    3_u8, U256::ONE, &[uint!("8"), uint!("10"), uint!("14")],
    &[uint!("8"), uint!("10")], &[uint!("14")]
)]
#[should_panic]
#[case::small_tree_wrong_height(
    3_u8, U256::ONE, &[uint!("8"), uint!("10"), uint!("16")], &[], &[]
)]
#[should_panic]
#[case::small_tree_not_descendant(
    3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[]
)]

---

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

        3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[])]

fn test_split_leaves(

remove newline

Suggestion:

        3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[])]
fn test_split_leaves(

---

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

#[rstest]

fn test_split_leaves_big_tree() {

remove newline

Suggestion:

#[rstest]
fn test_split_leaves_big_tree() {

[amosStarkware]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @TzahiTaub)

---

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

/// * The leaf indices array is sorted.
/// * All leaves are descendants of the root.
pub(crate) fn split_leaves<'a>(

this implementation is prettier and more concise - is it less efficient?
if not, please use it

Code quote:

pub(crate) fn split_leaves<'a>(

[TzahiTaub]

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

---

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

Previously, dorimedini-starkware wrote…

where is there an as here?

Oops, copy leftovers.

---

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

Previously, dorimedini-starkware wrote…

so... from_words is const on U256, but lsh is not?
bummer

Yes, that's what Yoni noticed.

---

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

Previously, dorimedini-starkware wrote…

remove the underscore - if you want this to be private, remove the pub

It's just for this PR as it's not used (test only). Will be used in the following. Prefer I bypass the warning?

---

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

Previously, dorimedini-starkware wrote…

for clarity

Done.

---

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

Previously, dorimedini-starkware wrote…

docstring (the name is confusing)

Is this OK?

---

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

Previously, amosStarkware wrote…

this implementation is prettier and more concise - is it less efficient?
if not, please use it

Do you mean the implementation that was in create_tree? It's still here (at the bottom). I just added assertions that the first and last leaves are actual descendants of the given root.

---

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

Previously, dorimedini-starkware wrote…

isn't this what was originally written?

I don't know, but IMO it makes sense to add 1 to the left son to get the right son and less sense to add ROOT to i.

---

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

Previously, dorimedini-starkware wrote…

this "feature" is not used, right?

Yep, not ATM. Removed.

---

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

Previously, dorimedini-starkware wrote…

no auto-format in macros (like #[case] attributes)...
please try to conform to "correct" formatting manually

Done.

---

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

Previously, dorimedini-starkware wrote…

remove newline

Done.

---

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

Previously, dorimedini-starkware wrote…

remove newline

Done.

[dorimedini-starkware]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

---

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

Previously, TzahiTaub (Tzahi) wrote…

It's just for this PR as it's not used (test only). Will be used in the following. Prefer I bypass the warning?

ah... ok I didn't realise the ocmpiler caught you. unblocking, either way is fine

[nimrod-starkware]

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

a discussion (no related file):

Previously, TzahiTaub (Tzahi) wrote…

Waiting for @nimrod-starkware to fix a test, but can still be reviewed.

PR that fixes the test

---

[dorimedini-starkware]

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

[nimrod-starkware]

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

a discussion (no related file):

Previously, nimrod-starkware wrote…

PR that fixes the test

merged

---

[amosStarkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

Do you mean the implementation that was in create_tree? It's still here (at the bottom). I just added assertions that the first and last leaves are actual descendants of the given root.

oh ok, removed block.
consider making assert_descendant a separate function

[dorimedini-starkware]

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

[codecov-commenter]

Codecov Report

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

Project coverage is 70.23%. Comparing base (5266ea6) to head (3c9a578).

Files	Patch %	Lines
...tricia_merkle_tree/original_skeleton_tree/utils.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   69.98%   70.23%   +0.25%     
==========================================
  Files          27       28       +1     
  Lines        1036     1045       +9     
  Branches     1036     1045       +9     
==========================================
+ Hits          725      734       +9     
  Misses        277      277              
  Partials       34       34              

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

[dorimedini-starkware]

Reviewed 8 of 8 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

[amosStarkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)