Add Maybe to the end bound of proofs (Part 1) #1793
Conversation
dboehm-avalabs
requested review from
danlaine,
darioush,
abi87,
gyuho,
hexfusion,
StephenButtolph and
joshua-kim
as code owners
| rangeProof, err := db.GetRangeProofAtRoot(context.Background(), root, step.key, step.value, 100) | ||
| require.NoError(err) | ||
| end := Nothing[[]byte]() | ||
| if len(step.value) > 0 { |
There was a problem hiding this comment.
nit: should we update the comments on the randStep struct to mention value/key are used differently for getting range/change proofs?
|
|
||
| if err := db.VerifyChangeProof(ctx, &changeProof, req.StartKey, req.EndKey, endRoot); err != nil { | ||
| endKey := merkledb.Nothing[[]byte]() | ||
| if req.EndKey != nil && !req.EndKey.IsNothing { |
There was a problem hiding this comment.
I don't understand if we have a valid representation where req.EndKey can be nil, why do we need an explicit IsNothing?
There was a problem hiding this comment.
quirk of proto, ideally that shouldnt be possible, but I need to figure out if it is possible to enforce. If it were just a []byte, then it wouldn't be clear when it was nothing vs nil
There was a problem hiding this comment.
We can keep the existing semantics for now and possibly avoid the IsNothing bool field in a separate PR
| // TODO there's probably a better way to do this. | ||
| var errString string | ||
| if err := s.db.VerifyChangeProof(ctx, &proof, req.StartKey, req.EndKey, rootID); err != nil { | ||
| if err := s.db.VerifyChangeProof(ctx, &proof, req.StartKey, merkledb.Some(req.EndKey), rootID); err != nil { |
There was a problem hiding this comment.
why do we always pass Some here? (shouldn't nil value map to Nothing?)
There was a problem hiding this comment.
This just hasn't been updated yet to support Maybes yet.
| m.config.Log.Info("completed range", | ||
| zap.Binary("start", work.start), | ||
| zap.Binary("end", largestHandledKey), | ||
| zap.Binary("end", largestHandledKey.Value()), |
| proof.StartProof, | ||
| insertChildrenLessThan, | ||
| insertChildrenGreaterThan, | ||
| Some(smallestPath), |
There was a problem hiding this comment.
Why do we now always pass in Some instead of passing in Nothing when len(smallestPath) == 0?
There was a problem hiding this comment.
functionally makes no difference, since nil is the smallest possible key, it is equivalent to no bounds.
| proof.EndProof, | ||
| insertChildrenLessThan, | ||
| insertChildrenGreaterThan, | ||
| Some(smallestPath), |
There was a problem hiding this comment.
Why do we now always pass in Some instead of passing in Nothing when len(smallestPath) == 0?
There was a problem hiding this comment.
functionally makes no difference, since nil is the smallest possible key, it is equivalent to no bounds.
| proof *ChangeProof, | ||
| start []byte, | ||
| end []byte, | ||
| end Maybe[[]byte], |
There was a problem hiding this comment.
Should we also use end Maybe[[]byte] in GetChangeProof above and GetRangeProofAtRoot below for consistency?
There have been several bugs related to the nil key being confused for no end bound. By converting the end bounds to Maybe[T], it will be more explicit in the code which bound is meant (nil key vs no bound)