Update proofs #97
Update proofs #97
Conversation
There was a problem hiding this comment.
This looks good overall, but I think there's some things we can do to improve readability. I'd also love to see some more test coverage (especially on the complicated case where we have to split a leaf). For that case in particular, it's probably worth writing tests to ensure that we're always computing the correct number of default siblings, plus some end-to-end tests making sure that the insertion runs correctly
| // Freezes the current cache to make all contents in the current cache immutable. | ||
| tree_cache.freeze()?; | ||
| } | ||
|
|
||
| Ok(tree_cache.into()) | ||
| } | ||
|
|
||
| /// Same as [`put_value_sets`], this method returns a Merkle proof for every update of the Merkle tree. | ||
| /// The proofs can be verified using the [`verify_update`] method, which requires the old `root_hash`, the `merkle_proof` and the new `root_hash` | ||
| pub fn put_value_sets_with_proof( |
There was a problem hiding this comment.
This function looks like it duplicates a lot of logic from put_value_sets. Can we extract the shared code into a helper function?
There was a problem hiding this comment.
I don't know if it would be clearer to be honest, specially because we do populate values of a proof vector on each iteration of the nested loops in the proof generating version
…the node type tests
| /// siblings, but more efficient). | ||
| pub fn get_child_without_siblings(&self, node_key: &NodeKey, n: Nibble) -> Option<NodeKey> { | ||
| pub fn get_only_child_without_siblings( |
There was a problem hiding this comment.
I'm curious why we renamed this function - does it break if there are multiple children?
There was a problem hiding this comment.
So this one is a bit tricky, but basically there is two different type of get_child functions:
- The get_child* functions which basically returns the element pointed by n. If there is no child at that exact nibble number it will return None
- The get_only_child* functions which does the same as the previous one, but will return the closest child to the spot pointed by n if this spot is empty (needed for non-existence proofs)
There was a problem hiding this comment.
LGTM, except for a few small nits. Let's see if we can get a review from @plaidfinch before merging though.
| let num_default_siblings = (4 - (num_siblings % 4)) % 4 /* the number of default leaves we need to add to the previous root */ | ||
| + ((4*(common_prefix_len+1) - num_siblings) / 4 ) * 4 /* the number of default leaves we need to add to the path */ | ||
| + (default_siblings_leaf_nibble) | ||
| - 4; |
There was a problem hiding this comment.
Tagging this for future reference, since this simplifies to:
let num_default_siblings = (-num_siblings % 4) + (4*common_prefix_len) - num_siblings + default_siblings_leaf_nibbleIf the unreduced expression is here to help readers grok the intuition behind it, then I would add at least a few lines of context about how you're getting there.
There was a problem hiding this comment.
Just added the detailed comments in the last commit!
|
Thanks for your contribution, this is really good work. We will cut a release later today |
Description
This PR allows the generation of merkle proofs in case of tree updates.
This PR is finally ready for review.
Purpose
This PR aims to solve the issue #246 of the Sovereign lab sdk repo
Methods added
The
verify_updatemethod have been added to the SparseMerkleProof, this method verifies a Merkle proof for a tree update.The
put_value_set(s)_with_proofmethods have been added to the JellyfishMerkleTree struct. These methods update the tree and produce a set of Merkle proofs associated with the tree.Changes from existing implementation
get_child_with_siblingsfunction now takes an object that implements the tree reader trait as an argument. This is due to deletion proofs that need to specify the sibling nodes' types (to know if we need to perform coalescing or not).TreeCachewrapper object now implements theTreeReadertrait. We need that to correctly build the sibling list in theput_value_set(s)_with_proofmethods.get_child_with_siblingstoget_only_child_with_siblings: this function did not have the same behavior as thechildmethod of an internal node: when the nibble parameter points to an empty spot,get_child_with_siblingsused to return the closest only child to that spot, while thechildmethod would return none. Hence, I created aget_child_with_siblingsreproducing the behavior ofchildand renamed the former method toget_only_child_with_siblings.Testing
A file
update_proofs.rshave been added to the test set. This file reproduces most the tests fromjellyfish_merkle.rsand checks that the updates have been well performed. This file contains proptests.Actions remaining
node_tests.rsin the testing folder has been broken because of the new interface forget_child_with_siblings. This isn't a straightforward fix as one needs to know the list of leaf nodes to be able to generate a merkle tree reader and populate it with the tree data. This test would need to be either rewritten or removed.Once this PR is merged, I will open new issues for the remaining actions