sync -- change proof request can return range proof
#1772
Conversation
x/merkledb/proof_test.go
Outdated
| name: "no roots in history and non-empty key-values", | ||
| proof: &ChangeProof{ | ||
| HadRootsInHistory: false, | ||
| KeyChanges: []KeyChange{{Key: []byte{1}, Value: Some([]byte{1})}}, | ||
| }, | ||
| start: []byte{0}, | ||
| end: nil, // Also tests start can be after end if end is nil | ||
| expectedErr: ErrDataInMissingRootProof, | ||
| }, | ||
| { | ||
| name: "no roots in history and non-empty deleted keys", | ||
| proof: &ChangeProof{ | ||
| HadRootsInHistory: false, | ||
| KeyChanges: []KeyChange{{Key: []byte{1}}}, | ||
| }, | ||
| start: nil, | ||
| end: nil, | ||
| expectedErr: ErrDataInMissingRootProof, | ||
| }, | ||
| { | ||
| name: "no roots in history and non-empty start proof", | ||
| proof: &ChangeProof{ | ||
| HadRootsInHistory: false, | ||
| StartProof: []ProofNode{{}}, | ||
| }, | ||
| start: nil, | ||
| end: nil, | ||
| expectedErr: ErrDataInMissingRootProof, | ||
| }, | ||
| { | ||
| name: "no roots in history and non-empty end proof", | ||
| proof: &ChangeProof{ | ||
| HadRootsInHistory: false, | ||
| EndProof: []ProofNode{{}}, | ||
| }, | ||
| start: nil, | ||
| end: nil, | ||
| expectedErr: ErrDataInMissingRootProof, | ||
| }, | ||
| { | ||
| name: "no roots in history; empty", | ||
| proof: &ChangeProof{ | ||
| HadRootsInHistory: false, | ||
| }, | ||
| start: nil, | ||
| end: nil, | ||
| expectedErr: nil, | ||
| }, | ||
| { | ||
| name: "root in history; empty", | ||
| proof: &ChangeProof{ | ||
| HadRootsInHistory: true, | ||
| }, |
There was a problem hiding this comment.
Removed tests where HadRootsInHistory is false because change proofs can no longer indicate that the server didn't have enough history to serve the change proof (a range proof is returned instead.)
| type mockClient struct { | ||
| db DB | ||
| } | ||
| func newCallthroughSyncClient(ctrl *gomock.Controller, db merkledb.MerkleDB) *MockClient { |
There was a problem hiding this comment.
Replaced mockClient with usage of the gomock mock since I don't think we need 2 mock implementations.
x/sync/network_server.go
Outdated
| startRoot, err := ids.ToID(req.StartRootHash) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| endRoot, err := ids.ToID(req.EndRootHash) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
We don't need to parse the start/end root in every loop iteration
| changeProof, err := s.db.GetChangeProof(ctx, startRoot, endRoot, req.StartKey, req.EndKey, int(keyLimit)) | ||
| if err != nil { | ||
| // handle expected errors so clients cannot cause servers to spam warning logs. | ||
| if errors.Is(err, merkledb.ErrRootIDNotPresent) || errors.Is(err, merkledb.ErrStartRootNotFound) { | ||
| s.log.Debug( |
There was a problem hiding this comment.
The old log was midleading. Just because we had insufficient history doesn't mean the request was invalid.
| return err | ||
| } | ||
|
|
||
| // TODO handle this fatal error |
There was a problem hiding this comment.
This was previously unhandled too
| if err != nil { | ||
| // handle expected errors so clients cannot cause servers to spam warning logs. | ||
| if errors.Is(err, merkledb.ErrRootIDNotPresent) { | ||
| s.log.Debug( |
There was a problem hiding this comment.
The old log was misleading -- insufficient history doesn't mean invalid request
| ) | ||
| return nil // dropping request | ||
| if errors.Is(err, merkledb.ErrInsufficientHistory) { | ||
| return nil, nil // drop request |
There was a problem hiding this comment.
I could add a log here but it seems kind of annoying. I'd also be open to checking for ErrInsufficientHistory in the caller.
danlaine
force-pushed
the
sync-handle-insufficient-history
branch
from
c9fba3b to
30ab6ea
Compare
x/merkledb/history.go
Outdated
| return nil, fmt.Errorf("%w: end root %s not found", ErrInsufficientHistory, endRoot) | ||
| } | ||
|
|
||
| // [startRootChanges] is the last appearance of [startRoot] | ||
| startRootChanges, ok := th.lastChanges[startRoot] | ||
| if !ok { | ||
| return nil, ErrStartRootNotFound | ||
| return nil, fmt.Errorf("%w: start root %s not found", ErrInsufficientHistory, startRoot) |
There was a problem hiding this comment.
It seems weird not to treat a missing endRoot differently to a missing startRoot... If we hit a missing endRoot I wouldn't expect the range proof to work either right?
There was a problem hiding this comment.
What do you mean? We also return ErrInsufficientHistory if the startRoot isn't found
There was a problem hiding this comment.
Why would we attempt to generate a range proof if we know we don't have the state to generate it?
(My original comment was missing a "not" oops)
There was a problem hiding this comment.
Still not sure I follow. This is the point where we figure out whether we have the state to generate the range proof.
There was a problem hiding this comment.
Put a TODO to return ErrNoEndRoot (or similar named error) once we require go 1.20 and then we can check for that error and not try to get a range proof if we know we have insufficient history
Why this should be merged
Right now, if a server has insufficient history to generate a change proof, it responds with a change proof that has a flag signifying this. It'd be better if instead, it sent a range proof for the trie at the end root. This change does that.
How this works
HadRootsInHistoryfield fromChangeProofErrRootIDNotPresentandErrStartRootNotFoundintoErrInsufficientHistoryChangeOrRangeProofclient.GetChangeProofto returnChangeOrRangeProofManager.getAndApplyChangeProofto handle range proofHow this was tested
New UT.