Map Verification: Fix for proofs of leaves in empty branches. #780
Conversation
gdbelvin
changed the title
|
Can you rebase? Looks like just the tests conflict. |
gdbelvin
force-pushed
the
fix/emptyproof
branch
from
17916f4 to
9bd0f0f
Compare
|
Rebased |
merkle/map_verifier.go
Outdated
|
|
||
| runningHash := make([]byte, len(leafHash)) | ||
| copy(runningHash, leafHash) | ||
| // Since HashChildren(e0, e0) is not always equal to the empty |
There was a problem hiding this comment.
This is talking about HashChildren(e0, e0) at level N where N > 0? If so might be worth adding to the text. And in the comment below is "value at level 1" correct i.e. not level 0? Maybe a whiteboard would help in going through this?
There was a problem hiding this comment.
How's this:
// Since empty values are tied to a location and a
// HashEmpty(leve1) != HashChildren(E0,
// Therefore we need to maintain an empty marker along the
// proof path until the first non-empty element so we can call
// HashEmpty once at the top of the empty branch.
There was a problem hiding this comment.
Basically OK. Looks like there might be a couple of typos "leve1" and the HashChildren line
seems to be truncated.
| return nil, status.Errorf(codes.InvalidArgument, | ||
| "len(%x): %v, want %v", l.Index, got, want) | ||
| } | ||
| if l.LeafValue == nil { |
There was a problem hiding this comment.
I was slightly surprised that this isn't an error but I'm not certain of the map semantics here.
There was a problem hiding this comment.
Storing empty leaves in maps is valid, we just don't want to put anything on disk for them.
| desc: "maphasher multi", | ||
| HashStrategy: trillian.HashStrategy_TEST_MAP_HASHER, | ||
| desc: "multi", | ||
| HashStrategy: []trillian.HashStrategy{trillian.HashStrategy_TEST_MAP_HASHER, trillian.HashStrategy_CONIKS_SHA512_256}, |
There was a problem hiding this comment.
Do you think these tests are enough to show verification works correctly at multiple levels and leaves? It's hard for me to be sure.
There was a problem hiding this comment.
added some comments for clarity.
|
Thank you very much for all your reviews, Martin! |
3 participants
The CONIKS hashing algorithm commits to empty branches uniquely for every position in the tree #691. This breaks the previous assumption that
E2 = H(E1, E1). The verification function needs to be aware of when it is verifying a node within an empty branch and use the commitment to the whole empty branch. Thus,VerifyMapInclusionProofis modified to acceptLeafValuerather thanLeafHashso that it can perform this empty branch detection.This PR also adds a check to prevent clients from storing empty leaf nodes in the leaf table.
The testing of empty branches has been incorporated into
TestLeafHistoryin order to test empty branch inclusion proofs at a variety of points - and remove duplicate code.