chore: tight range checks in NodeIndex

[TzahiTaub]

This change is 

[codecov-commenter]

Codecov Report

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

Project coverage is 75.94%. Comparing base (d138ace) to head (5f7259d).

Files	Patch %	Lines
crates/committer/src/patricia_merkle_tree/types.rs	92.00%	1 Missing and 1 partial ⚠️
...patricia_merkle_tree/updated_skeleton_tree/tree.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   75.87%   75.94%   +0.07%     
==========================================
  Files          25       25              
  Lines         858      869      +11     
  Branches      858      869      +11     
==========================================
+ Hits          651      660       +9     
- Misses        174      175       +1     
- Partials       33       34       +1     

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

[Yoni-Starkware]

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

---

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

pub const MAX_HEIGHT: u8 = 251;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << (MAX_HEIGHT + 1)) - U256::ONE);

Does this work? if so, better to keep it simple

Suggestion:

use ethnum::U256;

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

const MAX_HEIGHT: u8 = 251;
const MAX_INDEX: U256 = (U256::ONE << MAX_HEIGHT + 1) - U256::ONE;

---

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

    Ord,
)]
pub(crate) struct NodeIndex(pub U256);

This will make the constructor private and enforce using the new function

Suggestion:

pub(crate) struct NodeIndex(U256);

---

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

// Wraps a U256. Maximal possible value is the largest index in a tree of height 251 (2 ^ 252 - 1).
impl NodeIndex {
    pub const BITS: u8 = MAX_HEIGHT + 1;

BIT_LENGTH/BIT_LEN/N_BITS

Code quote:

BITS

---

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

        }
        Self(index)
    }

Maybe implement the trait From from U256

Code quote:

    pub(crate) fn new(index: U256) -> Self {
        if index > *MAX_INDEX {
            panic!("Index is too large.");
        }
        Self(index)
    }

---

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

    pub(crate) fn highest_index() -> NodeIndex {
        NodeIndex::new(*MAX_INDEX)
    }

If it works, better to do it in compile time

Suggestion:

    pub(crate) const ROOT: Self = { Self::from(U256::ONE) };

    pub(crate) const MAX: Self = { Self::from(*MAX_INDEX) };

---

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

        Self(U256::from(1_u8) << tree_height.0) + Self::from(address.0)
    }
}

Why not here?

Code quote:

    pub(crate) fn from_starknet_storage_key(
        key: &StarknetStorageKey,
        tree_height: &TreeHeight,
    ) -> Self {
        Self(U256::from(1_u8) << tree_height.0) + Self::from(key.0)
    }

    pub(crate) fn from_contract_address(
        address: &ContractAddress,
        tree_height: &TreeHeight,
    ) -> Self {
        Self(U256::from(1_u8) << tree_height.0) + Self::from(address.0)
    }
}

[Yoni-Starkware]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

BIT_LENGTH/BIT_LEN/N_BITS

NVM, I see it's consistent with U256

---

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

    /// Returns the number of leading zeroes when represented with Self::BITS bits (252).
    pub(crate) fn leading_zeros(&self) -> u8 {

Suggestion:

    /// Returns the number of leading zeroes when represented with Self::BITS bits.
    pub(crate) fn leading_zeros(&self) -> u8 {

[Yoni-Starkware]

Reviewable status: 0 of 7 files reviewed, 7 unresolved discussions (waiting on @TzahiTaub)

---

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

    /// Returns the number of leading zeroes when represented with Self::BITS bits (252).
    pub(crate) fn leading_zeros(&self) -> u8 {
        (self.0.leading_zeros() - (U256::BITS - u32::from(Self::BITS)))

Why not keep Self::BITS as u32?

Code quote:

u32::from(Self::BITS)))

[TzahiTaub]

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @TzahiTaub and @Yoni-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Does this work? if so, better to keep it simple

No, shift (<<) is not a const function, this is why I use Lazy.

---

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

Previously, Yoni-Starkware (Yoni) wrote…

This will make the constructor private and enforce using the new function

👍 good pointer
The problem is I do access the field to use its functions. e.g., (first\_leaf.0 & child\_direction\_mask) where the mask is U256. I can use derive_more and turn the mask to NodeIndex, but I'm not sure it's a good idea (as it makes NodeIndex represent numbers and not really just indices in a tree). WDYT?

---

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

Previously, Yoni-Starkware (Yoni) wrote…

NVM, I see it's consistent with U256

Yes, I don't like the names either.

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Maybe implement the trait From from U256

Instead of new? I can make TryFrom (as not every U256 can be converted) but I prefer to use the ctor with panic than to get an error and handle it everywhere if it makes sense.

---

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

Previously, Yoni-Starkware (Yoni) wrote…

If it works, better to do it in compile time

root is fine, max isn't possible inside the NodeIndex impl.

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Why not keep Self::BITS as u32?

We can , it's just fits in a u8 and is like the TreeHeight (I don't know why U256 uses u32).

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Why not here?

Why WHAT is not here?

[Yoni-Starkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

No, shift (<<) is not a const function, this is why I use Lazy.

Suggestion: something like

const MAX_INDEX: U256 = U256::from_words(!0 >> (255 - MAX_HEIGHT), !0);

Use can move them to NodeIndex and replace (255 - MAX_HEIGHT) with (U256::BITS - Self::BITS)

---

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

Previously, TzahiTaub (Tzahi) wrote…

👍 good pointer
The problem is I do access the field to use its functions. e.g., (first\_leaf.0 & child\_direction\_mask) where the mask is U256. I can use derive_more and turn the mask to NodeIndex, but I'm not sure it's a good idea (as it makes NodeIndex represent numbers and not really just indices in a tree). WDYT?

You can add a getter:
first_leaf.0 -> first_leaf.index()/first_leaf.num() or something like that

---

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

Previously, TzahiTaub (Tzahi) wrote…

We can , it's just fits in a u8 and is like the TreeHeight (I don't know why U256 uses u32).

Because 256 does not fit in 8 bits. Okay, your choice

---

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

    pub(crate) fn from_contract_address(
        address: &ContractAddress,
        tree_height: &TreeHeight,

The addition is not safe (not going through your new method)
Can we disable the Add impl? are we using it elsewhere?

Suggestion:

Self(U256::ONE << tree_height.0) + Self::from(address.0)

---

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

Previously, TzahiTaub (Tzahi) wrote…

Why WHAT is not here?

Using Self::new

[dorimedini-starkware]

Reviewed 3 of 7 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @amosStarkware, @TzahiTaub, and @Yoni-Starkware)

---

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

Previously, TzahiTaub (Tzahi) wrote…

root is fine, max isn't possible inside the NodeIndex impl.

max is impossible also because it's not const (the shift-left operator is not const)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

The addition is not safe (not going through your new method)
Can we disable the Add impl? are we using it elsewhere?

@Yoni-Starkware contract addresses, class hashes and storage keys are all less than 2^251 (which is much less than PRIME-1), right?
(otherwise a height of 251 is not enough)
if so, why is the addition unsafe?

---

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r2 (raw file):

pub const MAX_HEIGHT: u8 = 251;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << (MAX_HEIGHT + 1)) - U256::ONE);

Suggestion:

pub const MAX_HEIGHT: u8 = 251;
const NODE_INDEX_BITS: u8 = MAX_HEIGHT + 1;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << NODE_INDEX_BITS) - U256::ONE);

---

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

// Wraps a U256. Maximal possible value is the largest index in a tree of height 251 (2 ^ 252 - 1).
impl NodeIndex {
    pub const BITS: u8 = MAX_HEIGHT + 1;

delete (see above, use NODE_INDEX_BITS)

---

crates/committer/src/patricia_merkle_tree/types.rs line 40 at r2 (raw file):

    pub(crate) fn new(index: U256) -> Self {
        if index > *MAX_INDEX {
            panic!("Index is too large.");

Suggestion:

"Index {index} is too large."

---

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

    pub(crate) fn highest_index() -> NodeIndex {
        NodeIndex(*MAX_INDEX)
    }

Suggestion:

    pub(crate) fn highest_index() -> Self {
        Self(*MAX_INDEX)
    }

---

crates/committer/src/patricia_merkle_tree/types.rs line 100 at r2 (raw file):

        NodeIndex::new(self.0 >> rhs)
    }
}

while you're here

Suggestion:

impl std::ops::Shl<u8> for NodeIndex {
    type Output = Self;

    /// Returns the index of the left descendant (child for rhs=1) of the node.
    fn shl(self, rhs: u8) -> Self::Output {
        Self::new(self.0 << rhs)
    }
}

impl std::ops::Shr<u8> for NodeIndex {
    type Output = Self;

    /// Returns the index of the ancestor (parent for rhs=1) of the node.
    fn shr(self, rhs: u8) -> Self::Output {
        Self::new(self.0 >> rhs)
    }
}

[TzahiTaub]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @Yoni-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Suggestion: something like

const MAX_INDEX: U256 = U256::from_words(!0 >> (255 - MAX_HEIGHT), !0);

Use can move them to NodeIndex and replace (255 - MAX_HEIGHT) with (U256::BITS - Self::BITS)

I don't understand what is written in your proposal. Is it supposed to be more readable? 😕
And I CAN'T USE << (a non const function) in NodeIndex as we can't define a static there (and we can't use if for const in general)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

You can add a getter:
first_leaf.0 -> first_leaf.index()/first_leaf.num() or something like that

Done.

---

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

Previously, dorimedini-starkware wrote…

max is impossible also because it's not const (the shift-left operator is not const)

Yep. I meant it also isn't possible to put a static inside the impl.

---

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

Previously, dorimedini-starkware wrote…

@Yoni-Starkware contract addresses, class hashes and storage keys are all less than 2^251 (which is much less than PRIME-1), right?
(otherwise a height of 251 is not enough)
if so, why is the addition unsafe?

It's not safe in general (as two NodeIndices can overflow the 252 bit limitations).
As a side note I think we should enforce the hight limitation in TreeHeight as done in NodeIndex (and in other strcuts if there are any).
Not sure what to do here ATM.

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Using Self::new

Didn't find it in my search&replace because of the Self usage...

---

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r2 (raw file):

pub const MAX_HEIGHT: u8 = 251;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << (MAX_HEIGHT + 1)) - U256::ONE);

Why? I think it's better for the consts/statics related to a struct to be in it if possible. How about this?

---

crates/committer/src/patricia_merkle_tree/types.rs line 40 at r2 (raw file):

    pub(crate) fn new(index: U256) -> Self {
        if index > *MAX_INDEX {
            panic!("Index is too large.");

Done.

---

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

    pub(crate) fn highest_index() -> NodeIndex {
        NodeIndex(*MAX_INDEX)
    }

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 100 at r2 (raw file):

Previously, dorimedini-starkware wrote…

while you're here

Done.

[Yoni-Starkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

I don't understand what is written in your proposal. Is it supposed to be more readable? 😕
And I CAN'T USE << (a non const function) in NodeIndex as we can't define a static there (and we can't use if for const in general)

This line works, I checked. The >> operates on u128
It is similar to the way U256 defined MAX (pub const MAX: Self = Self([!0; 2]);).
The goal is do avoid Lazy and to define our MAXas const as well.

[Yoni-Starkware]

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

[Yoni-Starkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

It's not safe in general (as two NodeIndices can overflow the 252 bit limitations).
As a side note I think we should enforce the hight limitation in TreeHeight as done in NodeIndex (and in other strcuts if there are any).
Not sure what to do here ATM.

@TzahiTaub do we use this addition anywhere outside this file?

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

    pub(crate) fn index(&self) -> U256 {
        self.0
    }

why not impl From<NodeIndex> for U256? it's safe (the other way around is unsafe)

Code quote:

    /// Returns the index as U256.
    pub(crate) fn index(&self) -> U256 {
        self.0
    }

[TzahiTaub]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

This line works, I checked. The >> operates on u128
It is similar to the way U256 defined MAX (pub const MAX: Self = Self([!0; 2]);).
The goal is do avoid Lazy and to define our MAXas const as well.

Uhhh, OK then. Changed to u128::MAX, seems less confusing to me (ATM).
You've just searched for const fn from in U256?

---

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

Previously, Yoni-Starkware (Yoni) wrote…

@TzahiTaub do we use this addition anywhere outside this file?

Yes. Not sure how to find all usages, but there are some, e.g., in updated_skeleton_tree/tree.rs

---

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

Previously, dorimedini-starkware wrote…

why not impl From<NodeIndex> for U256? it's safe (the other way around is unsafe)

I don't know, it's not really a conversion but a getter. Does this options seem more "rust appropriate"?

[TzahiTaub]

crates/committer/src/patricia_merkle_tree/types.rs line 90 at r5 (raw file):

    fn from_leaf_felt(felt: &Felt, tree_height: &TreeHeight) -> Self {
        Self(U256::ONE << tree_height.0) + (*felt).into()

Had some conflicts with a recent merge to main - fixed accordingly.

Code quote:

 (*felt).into()

[Yoni-Starkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

Uhhh, OK then. Changed to u128::MAX, seems less confusing to me (ATM).
You've just searched for const fn from in U256?

I wrote U256::MAX and went to the impl

---

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

    #[allow(clippy::as_conversions)]
    pub const MAX_INDEX: Self = Self(U256::from_words(
        u128::MAX >> (U256::BITS - Self::BITS as u32),

We don't use as at all (bad practice).
I think it's time to store BITS in u32

Code quote:

Self::BITS as u32

---

crates/committer/src/patricia_merkle_tree/types.rs line 36 at r4 (raw file):

        u128::MAX >> (U256::BITS - Self::BITS as u32),
        u128::MAX,
    ));

A nice convention from SN API repo.

Suggestion:

impl NodeIndex {
    pub const BITS: u8 = MAX_HEIGHT + 1;
    
    /// [NodeIndex] constant that represents the root index.
    pub const ROOT: Self = Self(U256::ONE);
    
    /// [NodeIndex] constant that represents the largest index in a tree.
    #[allow(clippy::as_conversions)]
    pub const MAX_INDEX: Self = Self(U256::from_words(
        u128::MAX >> (U256::BITS - Self::BITS as u32),
        u128::MAX,
    ));

[Yoni-Starkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

Yes. Not sure how to find all usages, but there are some, e.g., in updated_skeleton_tree/tree.rs

So I think you should impl these by yourself

    derive_more::Add,
    derive_more::BitAnd,
    derive_more::Mul,
    derive_more::Sub,

(not a big deal)

[dorimedini-starkware]

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @TzahiTaub, and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I don't know, it's not really a conversion but a getter. Does this options seem more "rust appropriate"?

I think so.
NodeIndex -> U256 is a "natural conversion"

---

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

We don't use as at all (bad practice).
I think it's time to store BITS in u32

@Yoni-Starkware u32::from is not const unfortunately; uint conversions in const expressions use as (see blockifier for examples)

[TzahiTaub]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @Yoni-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

So I think you should impl these by yourself

    derive_more::Add,
    derive_more::BitAnd,
    derive_more::Mul,
    derive_more::Sub,

(not a big deal)

I think Add and Mul are sufficient (the rest can only decrease numbers)

---

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

Previously, dorimedini-starkware wrote…

I think so.
NodeIndex -> U256 is a "natural conversion"

Alrighty then.

---

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

We don't use as at all (bad practice).
I think it's time to store BITS in u32

This causes a chain reaction to that everything needs to be u32 or converted from u8 (TREE_HEIGHT, bit_length, leading_zeros, maybe more). Why not allow as usage in special casses when you don't lose accuracy (I do need to suppress clippy explicitly so it seems like a reasonable "do if you're sure its OK" thing).

[dorimedini-starkware]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @TzahiTaub, and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/types.rs line 96 at r6 (raw file):

        Self::new(self.0 * rhs.0)
    }
}

does U256 panic on overflow?
I think we'll want to panic.

1. If U256 already panics then @Yoni-Starkware I think using the derive was just as good
2. If not, please panic explicitly

Code quote:

impl std::ops::Add for NodeIndex {
    type Output = Self;

    fn add(self, rhs: Self) -> Self {
        Self::new(self.0 + rhs.0)
    }
}

impl std::ops::Mul for NodeIndex {
    type Output = Self;

    fn mul(self, rhs: Self) -> Self {
        Self::new(self.0 * rhs.0)
    }
}

---

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

        let child_direction_mask = U256::ONE << (root_height.0 - 1);
        (U256::from(first_leaf) & child_direction_mask)
            != (U256::from(*last_leaf) & child_direction_mask)

or not, your call, non-blocking

Suggestion:

        let child_direction_mask = U256::ONE << (root_height.0 - 1);
        (child_direction_mask & first_leaf.into())
            != (child_direction_mask & last_leaf.into())

[TzahiTaub]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/types.rs line 96 at r6 (raw file):

Previously, dorimedini-starkware wrote…

does U256 panic on overflow?
I think we'll want to panic.

1. If U256 already panics then @Yoni-Starkware I think using the derive was just as good
2. If not, please panic explicitly

It does, thats why Sub is fine, but we want to avoid an overflow of NodeIndex (252 bits) and not U256.

[TzahiTaub]

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

---

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

Previously, dorimedini-starkware wrote…

or not, your call, non-blocking

Tried that before, didn't work (it needs an explicit type in any case)

[Yoni-Starkware]

Reviewed 3 of 7 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This causes a chain reaction to that everything needs to be u32 or converted from u8 (TREE_HEIGHT, bit_length, leading_zeros, maybe more). Why not allow as usage in special casses when you don't lose accuracy (I do need to suppress clippy explicitly so it seems like a reasonable "do if you're sure its OK" thing).

Okay, that sounds reasonable.

---

crates/committer/src/patricia_merkle_tree/types.rs line 96 at r6 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

It does, thats why Sub is fine, but we want to avoid an overflow of NodeIndex (252 bits) and not U256.

Yeah, what @TzahiTaub said

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: 6 of 10 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

[Yoni-Starkware]

Reviewed 2 of 6 files at r2, 1 of 2 files at r4, 3 of 4 files at r6, 3 of 4 files at r7.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/types.rs line 8 at r7 (raw file):

use ethnum::U256;

use super::node_data::inner_node::EdgePath;

don't use super outside of tests

Code quote:

use super::node_data::inner_node::EdgePath;

---

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r7 (raw file):

    ) -> NodeIndex {
        let PathToBottom { path, length } = path_to_bottom;
        (index << length.0) + (*path).into()

make this explicit

Code quote:

(*path).into()

---

crates/committer/src/patricia_merkle_tree/types.rs line 143 at r7 (raw file):

        Self::new(U256::from_be_bytes(value.0.to_bytes_be()))
    }
}

remove (see above)

Code quote:

impl From<EdgePath> for NodeIndex {
    fn from(value: EdgePath) -> Self {
        Self::new(U256::from_be_bytes(value.0.to_bytes_be()))
    }
}

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/types.rs line 8 at r7 (raw file):

Previously, dorimedini-starkware wrote…

don't use super outside of tests

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r7 (raw file):

Previously, dorimedini-starkware wrote…

make this explicit

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 143 at r7 (raw file):

Previously, dorimedini-starkware wrote…

remove (see above)

Done.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/types.rs line 50 at r8 (raw file):

    ) -> NodeIndex {
        let PathToBottom { path, length } = path_to_bottom;
        (index << length.0) + Self::new(U256::from_be_bytes(path.0.to_bytes_be()))

Suggestion:

Self::from_felt_value(path.0)

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/types.rs line 50 at r8 (raw file):

    ) -> NodeIndex {
        let PathToBottom { path, length } = path_to_bottom;
        (index << length.0) + Self::new(U256::from_be_bytes(path.0.to_bytes_be()))

Done.

[dorimedini-starkware]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)