refactor: Exposing more of nullifier tree state #589
Conversation
kevaundray
changed the title
| continue; | ||
| } | ||
|
|
||
| if (data.size() == 64) { |
There was a problem hiding this comment.
whats special about 64?
There was a problem hiding this comment.
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.
| } 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.
Instead of a magic number, can we make this a constant? ditto for the number 32 and 64
| 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.
commented out code, also two lines below
| @@ -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.
Are we changing this from private to protected, only for testing purposes?
There was a problem hiding this comment.
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.
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
|
Closing this as it has been decided that we'll update the Nullifier tree stuff in aztec-packages at a later date making this PR not yet necessary. |
Description
Making more of nullifier tree state "protected" instead of "private" so that a testing harness can be written.
Note: Don't merge or review before #584 is merged.
Checklist:
@briefdescribing the intended functionality.includedirectives have been added.