feat: add simple sparse merkle tree #27

vlopes11 · 2022-12-08T16:22:30Z

This commit moves the previous implementation of SparseMerkleTree from
miden-core to this crate.

It also include a couple of new tests, a bench suite, and a couple of
minor fixes. The original API was preserved to maintain compatibility
with AdviceTape.

closes #21

src/merkle/simple_smt/mod.rs

bobbinth

Looks great! Thank you! I left a few small comments inline and have a few general comments as well:

I am not sure we need a generic version of SimpleSmt. Under the merkle module we place structures which rely on Rpo256 as the hash function. Maybe in the future we'll also need other hash functions, but I would rather keep things simple for now and not add features which we might not need to use for a while.
I'd prefer to keep public interfaces as consistent with interfaces for MerkleTree and MerklePathSet as possible. These are all just different variants of advice sets and the more consistent they are, the better. So, ideally, the following methods should look the same across all 3:
a. root()
b. depth()
c. get_node()
d. get_path()
e. get_path()
f. update_leaf()
I like the idea of using MerklePath struct - but then we should use it across all 3 consistently.
It is difficult for me to understand performance implications of this structure vs. the one in the original implementation. So, I am thinking we should put together some benchmarks, both for new structure and for the old one and see how they perform. We probably want to measure tree construction times for various fill factors as well as get_merkle_path() and update_leaf() functions.

src/merkle/simple_smt/mod.rs

src/merkle/simple_smt/path.rs

src/merkle/simple_smt/storage.rs

src/merkle/simple_smt/mod.rs

Al-Kindi-0

Looks great!
In addition to the other comments, I have added just one, a nit in fact, to those.

src/merkle/simple_smt/storage.rs

vlopes11 · 2022-12-12T19:35:12Z

Understood! It is indeed not desirable to have a whole new implementation when we will lose compatibility with the AdviceTape interface.

I restored the original API and implementation from miden-core and reused the new added tests/bench to not waste the effort. It was actually beneficial because I picked a couple of small nits with the proptest.

I'm resolving the old conversations because the PR changed entirely.

bobbinth

Thank you! Looks good! I left a few small comments inline.

Also, one comment about benchmarks: it might be good to benchmark the get_leaf_path() and update_leaf() operations for a couple of different scenarios. Specifically:

For a tree which is 50% full.
For a tree which is 1% full.

Also, for tree depth - could we use 20 - or is that too slow?

src/merkle/simple_smt/mod.rs

src/lib.rs

benches/smt.rs

vlopes11 · 2022-12-13T00:51:12Z

@bobbinth as for the benches, 20 depth is indeed considerably slow. As for the tree contents, we shouldn't have difference of performance between empty, partially filled and full trees, except for the memory overhead of fetching the nodes/leaves from the internal BTreeMap. If we have relevant difference, then it is likely we have a implementation problem.

I'll add these as sanity check.

bobbinth · 2022-12-13T01:22:22Z

the benches, 20 depth is indeed considerably slow. As for the tree contents, we shouldn't have difference of performance between empty

Let's keep it at depth 16 then. Agreed that it shouldn't make a difference - but I think a sanity check in this case wouldn't hurt.

vlopes11 · 2022-12-13T01:31:31Z

The cost for inclusion was independent of the inserted count, as expected

But, there was a difference for path 🤔

Likely it's because of the empty precalculated nodes for |leaves| = 1

bobbinth

All looks good! Thank you! Just a couple more small nit comments inline.

src/lib.rs

bobbinth · 2022-12-13T06:28:32Z

src/merkle/simple_smt/mod.rs

+        if !self.store.check_leaf_node_exists(key) {
+            return Err(MerkleError::InvalidIndex(self.depth, key));
+        }


Since we already do this check in get_path(), we can probably get rid of it here, no?

True, should have been removed in the last commit. Fixed!

bobbinth

All looks good! Thank you!

vlopes11 · 2022-12-13T22:32:08Z

@bobbinth I pushed another commit to make the bench check for a range of depths and leaves, so I'm requesting your review again

bobbinth

All looks good! Thank you!

grjte

Looks great! I left one nit inline for clarity

src/merkle/simple_smt/mod.rs

Al-Kindi-0

Looks great, thank you! I left one small nit which can be discarded.

vlopes11 · 2022-12-14T13:25:13Z

Hey @Al-Kindi-0 , I don't see your comment. Is it for the previous storage file?

This commit moves the previous implementation of `SparseMerkleTree` from miden-core to this crate. It also include a couple of new tests, a bench suite, and a couple of minor fixes. The original API was preserved to maintain compatibility with `AdviceTape`. closes #21

Al-Kindi-0 · 2022-12-14T13:30:40Z

I think it was for some days ago. I see know that it was taken into account so thank you!

vlopes11 self-assigned this Dec 8, 2022

vlopes11 force-pushed the add-simple-smt branch 3 times, most recently from 8e68b7d to 463e90b Compare December 8, 2022 16:46

vlopes11 requested review from bobbinth and Al-Kindi-0 December 8, 2022 17:28

vlopes11 commented Dec 8, 2022

View reviewed changes

src/merkle/simple_smt/mod.rs Outdated Show resolved Hide resolved

bobbinth requested changes Dec 8, 2022

View reviewed changes

Al-Kindi-0 reviewed Dec 9, 2022

View reviewed changes

src/merkle/simple_smt/storage.rs Outdated Show resolved Hide resolved

vlopes11 force-pushed the add-simple-smt branch from 463e90b to 406eba5 Compare December 12, 2022 19:32

vlopes11 force-pushed the add-simple-smt branch 2 times, most recently from 5c2928c to 253bb23 Compare December 12, 2022 19:42

vlopes11 requested review from Al-Kindi-0 and bobbinth December 12, 2022 21:30

bobbinth requested changes Dec 12, 2022

View reviewed changes

vlopes11 force-pushed the add-simple-smt branch from 253bb23 to 340b554 Compare December 13, 2022 01:33

vlopes11 requested a review from bobbinth December 13, 2022 01:38

bobbinth requested changes Dec 13, 2022

View reviewed changes

vlopes11 force-pushed the add-simple-smt branch from 340b554 to af9fca7 Compare December 13, 2022 11:41

vlopes11 requested a review from bobbinth December 13, 2022 12:33

vlopes11 mentioned this pull request Dec 13, 2022

Define general tree storage trait #30

Closed

bobbinth approved these changes Dec 13, 2022

View reviewed changes

vlopes11 force-pushed the add-simple-smt branch from af9fca7 to 98155c2 Compare December 13, 2022 22:31

vlopes11 requested a review from bobbinth December 13, 2022 22:32

bobbinth approved these changes Dec 14, 2022

View reviewed changes

grjte approved these changes Dec 14, 2022

View reviewed changes

src/merkle/simple_smt/mod.rs Outdated Show resolved Hide resolved

Al-Kindi-0 approved these changes Dec 14, 2022

View reviewed changes

vlopes11 force-pushed the add-simple-smt branch from 98155c2 to 5fd0d69 Compare December 14, 2022 13:28

vlopes11 merged commit aa12215 into next Dec 14, 2022

vlopes11 deleted the add-simple-smt branch December 14, 2022 13:32

This was referenced Dec 14, 2022

Migrate simple Sparse Merkle tree #21

Closed

Tracking PR for next release #32

Merged

bobbinth mentioned this pull request Dec 14, 2022

Tracking PR for the next release #33

Closed

2 tasks

grjte mentioned this pull request Feb 6, 2023

Tracking PR for release v0.1.1 #50

Merged

2 tasks

hackaugusto mentioned this pull request Mar 9, 2023

Implement domain separation for compact sparse Merkle tree #35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add simple sparse merkle tree #27

feat: add simple sparse merkle tree #27

vlopes11 commented Dec 8, 2022 •

edited

Loading

bobbinth left a comment

Al-Kindi-0 left a comment

vlopes11 commented Dec 12, 2022

bobbinth left a comment

vlopes11 commented Dec 13, 2022

bobbinth commented Dec 13, 2022

vlopes11 commented Dec 13, 2022

bobbinth left a comment

bobbinth Dec 13, 2022

vlopes11 Dec 13, 2022

bobbinth left a comment

vlopes11 commented Dec 13, 2022

bobbinth left a comment

grjte left a comment

Al-Kindi-0 left a comment

vlopes11 commented Dec 14, 2022

Al-Kindi-0 commented Dec 14, 2022

feat: add simple sparse merkle tree #27

feat: add simple sparse merkle tree #27

Conversation

vlopes11 commented Dec 8, 2022 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

vlopes11 commented Dec 12, 2022

bobbinth left a comment

Choose a reason for hiding this comment

vlopes11 commented Dec 13, 2022

bobbinth commented Dec 13, 2022

vlopes11 commented Dec 13, 2022

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Dec 13, 2022

Choose a reason for hiding this comment

vlopes11 Dec 13, 2022

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

vlopes11 commented Dec 13, 2022

bobbinth left a comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

vlopes11 commented Dec 14, 2022

Al-Kindi-0 commented Dec 14, 2022

vlopes11 commented Dec 8, 2022 •

edited

Loading