add SmtForest::insert_path() #650
Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of small comments inline.
Also, seems like there are some merge conflicts which need to be resolved.
| let mut smt = Smt::new(); | ||
| let key = Word::new([ZERO, ZERO, ZERO, ONE]); | ||
| let value = Word::new([ONE; WORD_SIZE]); | ||
| smt.insert(key, value)?; | ||
| let proof = smt.open(&key); |
There was a problem hiding this comment.
Could we make the smt a contain a few entries (in different leaves)? IIUC, right now, smt contains a single entry, and thus, the corresponding proof fully describes it. Would be good to test the situation where smt is a superset of the data in the forest (i.e., forest does not have all leaves from the smt).
There was a problem hiding this comment.
I’ve updated the test to use an SMT with multiple entries and verified that the forest correctly handles the partial view (returning UntrackedKey or NodeIndexNotFoundInStore for missing data).
One question: currently insert_proof returns early if the root already exists (as per its docstring), which means we can’t incrementally merge multiple proofs for the same root to build up a larger partial view.
I’ve started looking into this, but it seems that supporting merging correctly would require more complex node reference counting.
Another potential solution would be to introduce batch_insert_proofs, but then we’d need to have all proofs we want to insert up front.
There was a problem hiding this comment.
One question: currently
insert_proofreturns early if the root already exists (as per its docstring), which means we can’t incrementally merge multiple proofs for the same root to build up a larger partial view.
I’ve started looking into this, but it seems that supporting merging correctly would require more complex node reference counting.
Ah - interesting! Yeah, I didn't think of this before and it does seems like if we are supporting partial views, we should be able to support them more comprehensively (i.e., not just single-leaf partial views).
Maybe in light of the approach we landed on in #670 it would make sense to put this work on hold because making this work in the overly-centric design should be a bit easier.
This also does mean that we should design the storage of the persistent SMT forest in such a way that we support both full and partial SMTs - though, the support for partial SMTs could come a bit later.
# Conflicts: # CHANGELOG.md # miden-crypto/src/merkle/smt/forest/store.rs
# Conflicts: # CHANGELOG.md
3 participants
Describe your changes
Closes #591
Checklist before requesting a review
nextaccording to naming convention.