apollo_propeller: added model merkle implementation

[sirandreww-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[sirandreww-starkware]

• apollo_propeller: added merkle implementation tests #11206 : 2 dependent PRs (#11047 , #11572 )
• apollo_propeller: added merkle tree builder #11205
• apollo_propeller: added model merkle implementation #10985 👈 (View in Graphite)
• apollo_propeller: added model merkle proof implementation #11204
• apollo_protobuf: added propeller proto file #10654
• apollo_propeller: new crate for the propeller p2p protocol #11192
• main-v0.14.1-committer

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Artifacts upload workflows:

• Started at 2025-12-23T11:08:48Z
• Started at 2025-12-28T09:14:28Z
• Started at 2025-12-28T09:41:13Z
• Started at 2025-12-28T10:12:47Z
• Started at 2025-12-28T10:41:38Z

[sirandreww-starkware]

This PR provides a simple canonical merkle tree implementation that can easily be imported to other languages. This is the reason why we implemented our own merkle tree implementation instead of using another crate. If you have thoughts on this please share them.

@sirandreww-starkware made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).

[noamsp-starkware]

@noamsp-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @guy-starkware, @ShahakShama, and @sirandreww-starkware).

---

Cargo.lock line 2251 at r1 (raw file):

version = "0.0.0"
dependencies = [
 "sha2 0.10.9",

Why is this an explicit version and not just sha2 like all the other use cases in this file?

[noamsp-starkware]

@noamsp-starkware made 3 comments.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @guy-starkware, @ShahakShama, and @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 16 at r1 (raw file):

/// A Merkle tree for verifying data integrity.
#[derive(Debug, Clone)]
pub struct MerkleTree {

Wouldn't it be simpler for this struct to set nodes as Vec<Vec>?
I don't see any advantage in flattening the tree.
Seems like all it did was add extra steps to the build phase and extra variable maintenance to the proof building (level_start, for example).

This would also remove the need to clone 'leaves', since you'll know that 'leaves' is in the last element in nodes.

---

crates/apollo_propeller/src/merkle.rs line 99 at r1 (raw file):

        while level_size > 1 {
            // Find sibling index
            let sibling_index = if index.is_multiple_of(2) { index + 1 } else { index - 1 };

Use XOR instead.

Code snippet:

let sibling_index = index ^ 1;

---

crates/apollo_propeller/src/merkle.rs line 103 at r1 (raw file):

            // Add sibling hash
            // If sibling doesn't exist (odd node at end), use current node (it was duplicated)
            let sibling_hash = if sibling_index < level_size {

Move this check before calculating sibling_index / avoid calculating it if it won't be used.

[noamsp-starkware]

crates/apollo_propeller/src/merkle.rs line 16 at r1 (raw file):

Previously, noamsp-starkware wrote…

Wouldn't it be simpler for this struct to set nodes as Vec<Vec>?
I don't see any advantage in flattening the tree.
Seems like all it did was add extra steps to the build phase and extra variable maintenance to the proof building (level_start, for example).

This would also remove the need to clone 'leaves', since you'll know that 'leaves' is in the last element in nodes.

I meant Vec<Vec>

[noamsp-starkware]

@noamsp-starkware made 1 comment.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @guy-starkware, @ShahakShama, and @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 16 at r1 (raw file):

Previously, noamsp-starkware wrote…

I meant Vec<Vec>

...Reviewable is messing it up for me.
Vec<Vec<MerkleHash...

[ShahakShama]

@ShahakShama reviewed 1 file and all commit messages, made 5 comments, and resolved 6 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 35 at r5 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

see previous PR, if we resolved that conversation update me if I missed updating this one

Copy the TODO to here too

---

crates/apollo_propeller/src/merkle.rs line 41 at r5 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

Let's resolve the other discussion about disallowing empty trees first?

Sure. I think we should not disallow

---

crates/apollo_propeller/src/merkle.rs line 73 at r5 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

Are you suggesting that we skip the insert when the sibling is out of range and update the validation code to match?
I can add a todo to do this when we do optimizations later on?

Yes
My main concern is doing what's the common thing for merkle to do, and specifically what we do on starknet in other places, so the way I see this, it's not an optimization question, it's a protocol question

---

crates/apollo_propeller/src/merkle.rs line 92 at r5 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

It's just a helper function because proof does not have the root in it. Would you like me to remove it?

Nah let's keep it

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, noamsp-starkware wrote…

I agree with @sirandreww-starkware.
@ShahakShama In what case would an empty merkle tree actually be useful?

In general it's very useful. A merkle tree is a proof that I'm holding a collection. That collection can be empty
I think even in propeller we can decide to transfer an empty vector of bytes (though maybe in that case we still pass its size)
What will you gain be enforcing this?

[sirandreww-starkware]

@sirandreww-starkware made 3 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).

---

crates/apollo_propeller/src/merkle.rs line 35 at r5 (raw file):

Previously, ShahakShama wrote…

Copy the TODO to here too

Done.

---

crates/apollo_propeller/src/merkle.rs line 73 at r5 (raw file):

Previously, ShahakShama wrote…

Yes
My main concern is doing what's the common thing for merkle to do, and specifically what we do on starknet in other places, so the way I see this, it's not an optimization question, it's a protocol question

I see your point now. I think I understand what you mean, you want a merkle trie (where leaves can be in different depths)
I opened a PR #11572 to do exactly this. It has tests to show that that is what we are doing too. I'll add a TODO here too

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, ShahakShama wrote…

In general it's very useful. A merkle tree is a proof that I'm holding a collection. That collection can be empty
I think even in propeller we can decide to transfer an empty vector of bytes (though maybe in that case we still pass its size)
What will you gain be enforcing this?

If I get the freedom to enforce this it means that I cannot broadcast on an empty set of peers (that even excludes the broadcaster).
I think I'm alright with that since it's a no-op anyway. What I gain from this is code simplification, I don't need to consider the annoying case of 0 leaves in the merkle tree. That means I don't have to think about what a proof of 0 leaves means and so on...

[ShahakShama]

@ShahakShama made 2 comments and resolved 2 discussions.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 41 at r5 (raw file):

Previously, ShahakShama wrote…

Sure. I think we should not disallow

Could you do my original suggestion then (and_then and get instead of map and [0])

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

If I get the freedom to enforce this it means that I cannot broadcast on an empty set of peers (that even excludes the broadcaster).
I think I'm alright with that since it's a no-op anyway. What I gain from this is code simplification, I don't need to consider the annoying case of 0 leaves in the merkle tree. That means I don't have to think about what a proof of 0 leaves means and so on...

I don't see a reason to enforce it. Just because we enforce it doesn't mean you can leave panics throughout the code whenever there's an empty tree

[ShahakShama]

@ShahakShama made 1 comment.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 41 at r5 (raw file):

Previously, ShahakShama wrote…

Could you do my original suggestion then (and_then and get instead of map and [0])

first instead of get

[sirandreww-starkware]

@sirandreww-starkware made 1 comment.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, ShahakShama wrote…

I don't see a reason to enforce it. Just because we enforce it doesn't mean you can leave panics throughout the code whenever there's an empty tree

I will never instantiate a merkle tree with no leaves though...

[ShahakShama]

@ShahakShama reviewed all commit messages and made 1 comment.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

I will never instantiate a merkle tree with no leaves though...

Where do you plan to use this assumption? I think it's much simpler to just check it's not empty there and that's it

[sirandreww-starkware]

@sirandreww-starkware made 2 comments.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).

---

crates/apollo_propeller/src/merkle.rs line 41 at r5 (raw file):

Previously, ShahakShama wrote…

first instead of get

Done

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, ShahakShama wrote…

Where do you plan to use this assumption? I think it's much simpler to just check it's not empty there and that's it

Let me make sure we're on the same page:

1. what do you want the construction of an empty tree to return? a valid tree? None? or panic?
2. What do you want empty_tree.root() to return? a hard-coded value? None? panic?
3. what do you want the proof of any index (because no index is in the tree) to be? an empty vector? None? panic?

[ShahakShama]

@ShahakShama reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

Let me make sure we're on the same page:

1. what do you want the construction of an empty tree to return? a valid tree? None? or panic?
2. What do you want empty_tree.root() to return? a hard-coded value? None? panic?
3. what do you want the proof of any index (because no index is in the tree) to be? an empty vector? None? panic?

1. A valid tree
2. EMPTY_TREE_ROOT
3. empty vec

[ShahakShama]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @sirandreww-starkware).

[sirandreww-starkware]

@sirandreww-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ShahakShama).

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, ShahakShama wrote…

1. A valid tree
2. EMPTY_TREE_ROOT
3. empty vec

I don't agree with that, EMPTY_TREE_ROOT is effectively a null magic value that is not checked at compile time. This is a rust anti-pattern and should be avoided

[ShahakShama]

@ShahakShama made 1 comment and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sirandreww-starkware).

---

crates/apollo_propeller/src/merkle.rs line 67 at r6 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

I don't agree with that, EMPTY_TREE_ROOT is effectively a null magic value that is not checked at compile time. This is a rust anti-pattern and should be avoided

then None is good too

[graphite-app]

Merge activity

• Jan 12, 9:01 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

[sirandreww-starkware]

@sirandreww-starkware reviewed 5 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sirandreww-starkware).