-
Notifications
You must be signed in to change notification settings - Fork 129
refactor: Exposing more of nullifier tree state #589
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
Changes from all commits
278755a
e6621b7
bd32be3
b4e823b
2e0efee
814134f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,8 +106,8 @@ template <typename Store> fr_hash_path MerkleTree<Store>::get_hash_path(index_t | |
| current = hash_pair_native(path[j].first, path[j].second); | ||
| } | ||
| } else { | ||
| // Requesting path to a different, indepenent element. | ||
| // We know that this element exits in an empty subtree, of height determined by the common bits in the | ||
| // Requesting path to a different, independent element. | ||
| // We know that this element exists in an empty subtree, of height determined by the common bits in the | ||
| // stumps index and the requested index. | ||
| size_t common_bits = numeric::count_leading_zeros(diff); | ||
| size_t common_height = sizeof(index_t) * 8 - common_bits - 1; | ||
|
|
@@ -133,6 +133,65 @@ template <typename Store> fr_hash_path MerkleTree<Store>::get_hash_path(index_t | |
| return path; | ||
| } | ||
|
|
||
| template <typename Store> fr_sibling_path MerkleTree<Store>::get_sibling_path(index_t index) | ||
| { | ||
| fr_sibling_path path(depth_); | ||
|
|
||
| std::vector<uint8_t> data; | ||
| bool status = store_.get(root().to_buffer(), data); | ||
|
|
||
| for (size_t i = depth_ - 1; i < depth_; --i) { | ||
| if (!status) { | ||
| // This is an empty subtree. Fill in zero value. | ||
| path[i] = zero_hashes_[i]; | ||
| continue; | ||
| } | ||
|
|
||
| if (data.size() == 64) { | ||
| // This is a regular node with left and right trees. Descend according to index path. | ||
| bool is_right = bit_set(index, i); | ||
| path[i] = from_buffer<fr>(data, is_right ? 0 : 32); | ||
|
|
||
| auto it = data.data() + (is_right ? 32 : 0); | ||
| status = store_.get(std::vector<uint8_t>(it, it + 32), data); | ||
| } else { | ||
| // This is a stump. The sibling path can be fully restored from this node. | ||
| // In case of a stump, we store: [key : (value, local_index, true)], i.e. 65-byte data. | ||
| ASSERT(data.size() == 65); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a magic number, can we make this a constant? ditto for the number 32 and 64 |
||
| fr current = from_buffer<fr>(data, 0); | ||
| index_t element_index = from_buffer<index_t>(data, 32); | ||
| index_t subtree_index = numeric::keep_n_lsb(index, i + 1); | ||
| index_t diff = element_index ^ subtree_index; | ||
|
|
||
| // Populate the sibling path with zero hashes. | ||
| for (size_t j = 0; j <= i; ++j) { | ||
| path[j] = zero_hashes_[j]; | ||
| } | ||
|
|
||
| // If diff == 0, we are requesting the sibling path of the only non-zero element in the stump which is | ||
| // populated only with zero hashes. | ||
| if (diff == 1) { | ||
| // Requesting path of the sibling of the non-zero leaf in the stump. | ||
| // Set the bottom element of the path to the non-zero leaf (the rest is already populated correctly | ||
| // with zero hashes). | ||
| path[0] = current; | ||
| } else if (diff > 1) { | ||
| // Requesting path to a different, independent element. | ||
| // We know that this element exists in an empty subtree, of height determined by the common bits in the | ||
| // stumps index and the requested index. | ||
| size_t common_bits = numeric::count_leading_zeros(diff); | ||
| size_t common_height = sizeof(index_t) * 8 - common_bits - 1; | ||
|
|
||
| // Insert the only non-zero sibling at the common height. | ||
| path[common_height] = compute_zero_path_hash(common_height, element_index, current); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return path; | ||
| } | ||
|
|
||
| template <typename Store> fr MerkleTree<Store>::update_element(index_t index, fr const& value) | ||
| { | ||
| auto leaf = value; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,12 +25,19 @@ template <typename Store> class NullifierTree : public MerkleTree<Store> { | |
|
|
||
| fr update_element(fr const& value); | ||
|
|
||
| private: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we changing this from private to protected, only for testing purposes?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, because through the testing harness in aztec3 package we are exposing some of the internal values for testing purposes. But the original implementation of the testing harness used NullifierMemoryTree which was always only for testing so I can imagine that when it was originally implemented nobody cared about exposing more stuff. Do you think we should rethink how we test it or is having some of the value as protected ok with you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without looking deeper, it's okay with me. Just wanted to flag it up as usually changing the viability of an item for tests is a code smell. Moreover, we discussed this being a problem in other parts of the code in Valencia, so this can be addressed when we look into cleaning up the API as a whole. I'll open up an issue to track this when I get back to my laptop |
||
| protected: | ||
| using MerkleTree<Store>::update_element; | ||
| using MerkleTree<Store>::get_element; | ||
| using MerkleTree<Store>::compute_zero_path_hash; | ||
|
|
||
| private: | ||
| // const std::vector<barretenberg::fr>& get_hashes() { return hashes_; } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. commented out code, also two lines below |
||
| const WrappedNullifierLeaf get_leaf(size_t index) | ||
| { | ||
| // return (index < leaves_.size()) ? leaves_[index] : WrappedNullifierLeaf::zero(); | ||
| return (index < leaves.size()) ? leaves[index] : WrappedNullifierLeaf::zero(); | ||
| } | ||
| const std::vector<WrappedNullifierLeaf>& get_leaves() { return leaves; } | ||
|
|
||
| using MerkleTree<Store>::store_; | ||
| using MerkleTree<Store>::zero_hashes_; | ||
| using MerkleTree<Store>::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.
whats special about 64?
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.
Hello, I intended this PR to be reviewed and merged after #584 is merged. This PR is unfinished and messy.
But your comment applies to #584 as well. The get_sibling_path method is based on get_hash_path and in both cases 64 is a size of a regular node (65 is "stump"). I think introducing a constant is a good idea and I will do that in #584.