-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce SparseMerkleTree
trait
#245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I reviewed most things excluding tests and left some comments inline. Overall, I think this will work very well. One of the main questions is whether we should expose SparseMerkleTree
publicly - my current thinking is that probably not.
In terms of naming, I think SparseMerkleTree
is fine for the trait. The NewSmt
, I'd probably rename into Smt
(and keep SimpleSmt
as is). Another option is FullSmt
, but I like just Smt
better.
We could also come up with a more creative name (Octopus
? :)) - but I'm not sure that's needed.
src/merkle/smt.rs
Outdated
fn set_root(&mut self, root: RpoDigest); | ||
|
||
/// Retrieves an inner node at the given index | ||
fn get_inner_node(&self, index: NodeIndex) -> InnerNode; | ||
|
||
/// Inserts an inner node at the given index | ||
fn insert_inner_node(&mut self, index: NodeIndex, inner_node: InnerNode); | ||
|
||
/// Inserts a leaf node, and returns the value at the key if already exists | ||
fn insert_value(&mut self, key: Self::Key, value: Self::Value) -> Option<Self::Value>; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think exposing mutator methods publicly is not ideal because it allows modifying the underlying data structure in invalid ways (e.g., set an invalid root). I think we have two options here:
- Make the trait private and expose provided methods via pass through methods on implementing structs.
- Split the trait into two traits and make the one with mutator methods internal. The public trait would then depend on the internal trait.
It seems to me that the first approach is better because:
- It makes the structures easier to use - i.e., we don't need to import extra traits to get access to a subset of methods on a given struct.
- It provides more flexibly for the future - i.e., if the implementations of
SimpleSmt
andNewSmt
need to diverge in the future, this would be transparent to the users of the crate.
src/merkle/new_smt/mod.rs
Outdated
Multiple(Vec<(NewSmtKey, Word)>), | ||
} | ||
|
||
impl NewSmtLeaf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd also need to implement something like to_elements()
here - e.g.:
pub fn to_elements(&self) -> Vec<Felt>
pub fn into_elements(self) -> Vec<Felt>
We will probably need these to inject the SMT into the advice provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Any reason why we shouldn't implement From
for that, instead of custom methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong reason. Sometimes it make the code more readable, but in this case probably doesn't make a difference.
src/merkle/new_smt/mod.rs
Outdated
fn insert_value(&mut self, key: Self::Key, value: Self::Value) -> Option<Self::Value> { | ||
let leaf_index: LeafIndex<NEW_SMT_DEPTH> = key.into(); | ||
match self.leaves.get_mut(&leaf_index.value()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a special case we need to handle here: value == EMPTY_VALUE
. In this case, we need to return the tree into the state it was as if the key wasn't inserted in the first place. This implies:
- For a single leaf - remove the leaf.
- For multiple leaf:
a. If there are more than 2 entries in the leaf - remove the entry associated with the key.
b. If there are exactly 2 entries, remove the relevant entry and convert the leaf into a single leaf.
I would probably create a couple of helper functions to handle these cases separately (would help with readability).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I handled the case, but was able to make perform_remove()
clean. For insert()
, I moved it to SmtLeaf
; but you can't do that for remove()
, since in one case, "removing the leaf" involves removing the element from Smt.leaves
.
I'll write the tests, and then find a better way to clean it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like this:
fn perform_remove(&mut self, key: SmtKey) -> Option<Word> {
let leaf_index: LeafIndex<SMT_DEPTH> = key.into();
if let Some(leaf) = self.leaves.get_mut(&leaf_index.value()) {
let (old_value, is_empty) leaf.remove(key);
if is_empty {
self.leaves.remove(&leaf_index.value());
}
old_value
} else {
// there's nothing stored at the leaf; nothing to update
None
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
src/merkle/mod.rs
Outdated
mod smt; | ||
pub use smt::{InnerNode, LeafIndex, SparseMerkleTree, SMT_MAX_DEPTH, SMT_MIN_DEPTH}; | ||
|
||
mod new_smt; | ||
pub use new_smt::{NewSmt, NewSmtKey, NewSmtLeaf, NEW_SMT_DEPTH}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep SparseMerkleTree
private in this create, we probably won't need to make InnerNode
and LeafIndex
public either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made SparseMerkleTree
private, but let LeafIndex
public, since LeafIndex
: is now part of SimpleSmt
's public API (e.g. get_leaf()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a few comments inline - all are pretty minor.
Some more general thoughts:
In terms of overall module layout. I wonder if we should do something like this:
merkle
├── smt
│ ├── full
│ │ ├── mod.rs [this is where Smt struct could go]
│ │ └── tests.rs
│ ├── mod.rs [this is where SparseMerkleTree trait would go]
│ └── simple
│ ├── mod.rs [this is where SimpleSmt struct could go]
│ └── tests.rs
The above would be similar to the organization of the Mmr
module.
Next, I think the Smt
struct is missing several methods (most of these could probably be added in a subsequent PR):
get_value(&self, key: RpoDigest) -> Word
.- Something similar to
Tsmt::prove()
. inner_nodes(&self) -> impl Iterator<Item = InnerNodeInfo>
.leaves(&self) -> impl Iterator<Item = (u64, &SmtLeaf)>
.entries(&self) -> impl Iterator<Item = &(RpoDigest, Word)>
.
Looking through various Merkle structs, I'm wondering if we should adapt a slightly different naming/method convention for SparseMerkleTree
. Specifically:
- We could rename
get_leaf_path() -> MerklePath
intoopen() -> (MerklePath, Self::Leaf)
. Or maybe have a separateOpening
associated type. This would be more consistent with the terminology used in crypto papers and similar to what we do in theMmr
module. - Similarly, maybe we should rename
update_leaf()
intoinsert()
. The justification is that the operation the user cares about is inserting a value into the underlying tree. Whether it updates a leaf or something else is an implementation detail.
Lastly, this PR is getting pretty big. I wonder if we should split it into two:
- PR introducing
SparseMerkleTree
and applying it toSimpleSmt
. This would be a breaking PR as it will break things in miden vm/base/node, and maybe client. - Once the above PR is merged and updates propagated to downstream repos, we would do another PR to introduce
Smt
struct.
@bobbinth ready for re-review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a few comments inline - most of the are small nits.
Also, as I mentioned in one of the comments, we should probably remove Smt
implementation from this PR and add it in a subsequent PR which also addresses #251.
Lastly, I wouldn't merge this PR just yet. Let's create a PR in Miden VM which incorporates this new SimpleSmt
struct - so that once we merge this PR we can also merge that PR and nothing in Miden VM would be broken. After that, we'd make corresponding updates to miden-base
and miden-node
, and then come back here to finish Smt
and remove TieredSmt
in the next PR.
TieredSmt
replacementSparseMerkleTree
trait
Removed |
This reverts commit d39aee9.
fa8b582
to
1c92eeb
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
/// The root of the tree is recomputed on each new leaf update. | ||
#[derive(Debug, Clone, PartialEq, Eq)] | ||
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
pub struct SimpleSmt<const DEPTH: u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke downstream, do we have PRs to fix it on the works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - all fixing PRs have been merged. If you rebase from main
and do cargo update
it should fix the node build.
Work towards: #243
Left to do:
SparseMerkleTree
trait andNewSmt
structSparseMerkleTree
, and double check which arguments should be passed by move vs referenceFelt
of aWord
is the lastFelt
of theWord
Smt::Opening
: Change to a struct close toTieredSmtProof
forSmt
SparseMerkleTree::open()
: take a reference tokey
(afterkey_to_index()
change)get_leaf() -> &Leaf
(instead ofLeaf
)