Compute Neighbor during Map Inclusion Verification#772
Compute Neighbor during Map Inclusion Verification#772gdbelvin merged 3 commits intogoogle:masterfrom
Conversation
merkle/common.go
Outdated
| return r | ||
| } | ||
|
|
||
| // Neighbor returns index with only the left i bits set and the i'th bit flipped. |
There was a problem hiding this comment.
Can you give a bit more details on this? In particular we have a Sibling() function for logs, which could lead to confusion.
There was a problem hiding this comment.
I'll move this functionality to Sibling
integration/maptest/map_test.go
Outdated
| proof := incl.GetInclusion() | ||
|
|
||
| if got, want := leafHash, hasher.HashLeaf(treeID, index, hasher.BitLen(), leaf); !bytes.Equal(got, want) { | ||
| if got, want := leafHash, hasher.HashLeaf(treeID, index, 0, leaf); !bytes.Equal(got, want) { |
There was a problem hiding this comment.
Do we need tests at non zero depth?
There was a problem hiding this comment.
The map doesn't support leaves at non-zero depths, so I think not.
There was a problem hiding this comment.
Do we need that parameter then?
There was a problem hiding this comment.
Right now, strictly speaking, we don't.
If we were to support leaves at non-zero levels in the future, we would.
There was a problem hiding this comment.
Supporting that would be a fairly big change. I could argue for removing this parameter until it's used.
integration/maptest/map_test.go
Outdated
| proof := incl.GetInclusion() | ||
|
|
||
| if got, want := leafHash, hasher.HashLeaf(treeID, index, hasher.BitLen(), leaf); !bytes.Equal(got, want) { | ||
| if got, want := leafHash, hasher.HashLeaf(treeID, index, 0, leaf); !bytes.Equal(got, want) { |
There was a problem hiding this comment.
Supporting that would be a fairly big change. I could argue for removing this parameter until it's used.
storage/types.go
Outdated
| return n | ||
| } | ||
|
|
||
| // Neighbor sets this node to be it's own neighbor. |
There was a problem hiding this comment.
Still not sure if this means the same as Sibling(), or is it equivalent to Sibling() for maps. Can you expand the documentation?
There was a problem hiding this comment.
I've updated this so it does exactly the same thing as the previous version of Sibling. The steps have just been broken up for clarity.
Neighbor is doing the following snippet from the previous:
bi = n.PathLenBits() - n.PrefixLenBits + i ==
height = n.PathLenBits() - (n.PrefixLenBits - i)
r[i].SetBit(bi, n.Bit(bi)^1)
Mask Left is doing this snippet
r[i].PrefixLenBits = n.PrefixLenBits - i
The only additional piece is Mask which sets the unused bits to 0
There was a problem hiding this comment.
OK. it looks like this is only used by maps / SMT. In the C++ version we had Sibling() which was used by logs but that wasn't carried over to the new code. I wanted to make sure that there wasn't going to be API confusion.
There was a problem hiding this comment.
Sounds good. In either case, the original functionality has been maintained.
gdbelvin
force-pushed
the
verifier
branch
2 times, most recently
from
4bf878f to
4a65a2f
Compare
|
Is this all merged now via other PRs? |
|
This is not merged yet. Several other PRs have been rebased on it though. |
3 participants
Have the Map verification algorithm compute the proper neighbor nodes. This is a requirement of verification correctness.
This updates and simplifies the
Sibling()logic instorage.NodeID.The only functional change is masking off the unused bits.