Incremental Merkle Tree Implementation #72
Conversation
* IMT implementation * IMT tests
| let hash_of_empty_node: P::InnerDigest = P::InnerDigest::default(); | ||
| let hash_of_empty_leaf: P::LeafDigest = P::LeafDigest::default(); |
There was a problem hiding this comment.
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.
I cannot think of any immediate harm here.
There was a problem hiding this comment.
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.
@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, both P::InnerDigest::default() and P::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.
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.
|
|
|
Since @daira's concern about the inner node empty value is not specific to this PR. Please let me know whether I should address her concern in this PR or not. @tsunrise @weikengchen @Pratyush |
Description
This PR implements an incremental merkle tree data structure. This is an append only merkle tree that only maintains
Log Ninternal nodes. An invariant this implementation tries to maintain is that all operations isLog N.closes: #69
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.mdFiles changedin the Github PR explorer