merkledb -- rewrite and test range proof invariants; fix proof generation/veriifcation bugs
#1629
Conversation
danlaine
added
bug
| @@ -300,12 +305,12 @@ func (proof *RangeProof) Verify( | |||
| return err | |||
| } | |||
|
|
|||
| largestkey := end | |||
| largestKey := end | |||
| @@ -167,6 +167,7 @@ func (proof *Proof) Verify(ctx context.Context, expectedRootID ids.ID) error { | |||
| return ErrProofValueDoesntMatch | |||
| } | |||
|
|
|||
| // Don't bother locking [view] -- nobody else has a reference to it. | |||
There was a problem hiding this comment.
Moved from below since it makes more sense to be here
| // Empty if the request for this range proof gave no upper bound | ||
| // on the range to fetch, unless this proof contains no [KeyValues] | ||
| // and [StartProof] is empty. | ||
| // If no upper range bound was given, [KeyValues] is empty, |
There was a problem hiding this comment.
Rewritten to make the invariants more clear
| insertChildrenLessThan Maybe[path], | ||
| insertChildrenGreaterThan Maybe[path], |
There was a problem hiding this comment.
Now uses a Maybe to properly handle the case where we're verifying a proof that contains only the root. Namely, suppose that we had a range proof for range [nil, [0]] where the only key-value pair in the proof is for the root, but the database resulting in the proof has more key-value pairs. In RangeProof.Verify we set largestPath to the empty path and called addPathInfo with startPath and endPath both the empty root. This causes hasLowerBound and hasUpperBound in addPathInfo to be false, and we did not add children greater than the root (the empty path), resulting in proof verification failing.
In other words, we need to use a Maybe to disambiguate between the case where the largest key in the proof is at the root and the case where we shouldn't add any proof node children on the "right" side.
| var ( | ||
| endProof *Proof | ||
| err error | ||
| ) | ||
| if len(result.KeyValues) > 0 { | ||
| end = result.KeyValues[len(result.KeyValues)-1].Key | ||
| } | ||
|
|
||
| if len(end) > 0 { | ||
| endProof, err := t.getProof(ctx, end) | ||
| greatestKey := result.KeyValues[len(result.KeyValues)-1].Key | ||
| endProof, err = t.getProof(ctx, greatestKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else if len(end) > 0 { | ||
| endProof, err = t.getProof(ctx, end) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| if endProof != nil { |
There was a problem hiding this comment.
The old code could violate a RangeProof.EndProof invariant. Namely, if KeyValues contained only the root, we would not set result.EndProof.
danlaine
removed
the
DO NOT MERGE
danlaine
changed the title
merkledb -- rewrite and test range proof invariants; fix proof generation/veriifcation bugs
…-labs/avalanchego into merkledb-test-range-proof-invariants
x/merkledb/db.go
Outdated
| // For all the nodes along the edges of the proof, insert children | ||
| // < [insertChildrenLessThan] and > [insertChildrenGreaterThan] |
There was a problem hiding this comment.
During the hackathon I found this comment super confusing.... specifically the < and >.
| // For all the nodes along the edges of the proof, insert children | |
| // < [insertChildrenLessThan] and > [insertChildrenGreaterThan] | |
| // For all the nodes along the edges of the proof, insert the children whose | |
| // keys are less than [insertChildrenLessThan] or whose keys are greater | |
| // than [insertChildrenGreaterThan] |
| if len(smallestPath) > 0 { | ||
| insertChildrenLessThan = Some(smallestPath) | ||
| } |
There was a problem hiding this comment.
This seems pretty suspicious... Should start and end be Maybe types being passed into this function?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else if len(end) > 0 { |
There was a problem hiding this comment.
I feel like this could would be a lot easier to understand if we just passed start, end Maybe[[]byte] rather than doing this len([]byte) > 0 stuff
|
It passed. |
Why this should be merged
Yay code quality and testing
How this works
How this was tested
New UT