Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Incremental Merkle Tree Implementation #72
base: main
Are you sure you want to change the base?
Incremental Merkle Tree Implementation #72
Changes from 11 commits
60ba9d8
0c42571
2849151
c807e41
33e2fab
dd6c521
9a07b1b
217cf11
5994e66
130cb93
d3bc571
39b65c5
511baf2
781c164
6f91e03
b3166a9
bc0ca83
fd9a0de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 use the same hash function, do we want to have different values for empty inner digest and empty leaf digest, or it's fine in this context?
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 cannot think of any immediate harm here.
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.
got it thanks! looks fine
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 the intention is to be compatible with Zcash note commitment trees, then there's a sentinel value for empty leaves but there is no special case for empty internal nodes; they have the same value as would be expected from hashing their children. This doesn't require actually computing hashes for empty internal nodes, since the hash of a completely empty subtree only depends on its height (and can be computed in logarithmic time, then cached).
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.
@daira Thanks for your comments! Honestly I was just following the current arkwork's merkle tree implementation and didn't give too much thought. Now digging into it deeper, here is what I find:
For example, on
JubJub
, bothP::InnerDigest::default()
andP::LeafDigest::default()
is{x:0, y: <one of 0's corresponding y on the curve> }
. In the current implementation, the empty inner node a special value that is not equal to the "true" value where you actually compute from subtree. Like you said, it is easy to compute these empty inner nodes once and then use the cached value.It is indeed not super "principled" but I am not too much concerned security wise due to the CRH (please let me know if i am wrong). If you think there is a concrete security issue here. We should fix both this PR and the arkworks merkle tree implementation.
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 technically a small security issue in that if you use a sentinel value for internal nodes, you must argue that it was chosen such that it would be infeasible to find a preimage. (I.e. it is technically possible for there to be a back door where the sentinel is chosen to be the hash of some nonempty subtree.) But the main issue is just interoperability.