merkledb / sync -- Disambiguate no end root from no start root (#…

…2437)

x/merkledb/db.go

@@ -64,6 +64,8 @@ type ChangeProofer interface {
 	// Returns at most [maxLength] key/value pairs.
 	// Returns [ErrInsufficientHistory] if this node has insufficient history
 	// to generate the proof.
+	// Returns [ErrNoEndRoot], which wraps [ErrInsufficientHistory], if the
+	// history doesn't contain the [endRootID].
 	GetChangeProof(
 		ctx context.Context,
 		startRootID ids.ID,

x/merkledb/history.go

@@ -15,7 +15,10 @@ import (
 	"github.com/ava-labs/avalanchego/utils/set"
 )
 
-var ErrInsufficientHistory = errors.New("insufficient history to generate proof")
+var (
+	ErrInsufficientHistory = errors.New("insufficient history to generate proof")
+	ErrNoEndRoot           = fmt.Errorf("%w: end root not found", ErrInsufficientHistory)
+)
 
 // stores previous trie states
 type trieHistory struct {
@@ -77,6 +80,8 @@ func newTrieHistory(maxHistoryLookback int) *trieHistory {
 // If [end] is Nothing, there's no upper bound on the range.
 // Returns [ErrInsufficientHistory] if the history is insufficient
 // to generate the proof.
+// Returns [ErrNoEndRoot], which wraps [ErrInsufficientHistory], if
+// the [endRoot] isn't in the history.
 func (th *trieHistory) getValueChanges(
 	startRoot ids.ID,
 	endRoot ids.ID,
@@ -99,7 +104,7 @@ func (th *trieHistory) getValueChanges(
 	// lack the necessary history.
 	endRootChanges, ok := th.lastChanges[endRoot]
 	if !ok {
-		return nil, fmt.Errorf("%w: end root %s not found", ErrInsufficientHistory, endRoot)
+		return nil, fmt.Errorf("%w: %s", ErrNoEndRoot, endRoot)
 	}
 
 	// Confirm there's a change resulting in [startRoot] before

x/merkledb/history_test.go

@@ -165,13 +165,13 @@ func Test_History_Bad_GetValueChanges_Input(t *testing.T) {
 	require.NoError(batch.Put([]byte("key"), []byte("value")))
 	require.NoError(batch.Write())
 
-	toBeDeletedRoot := db.getMerkleRoot()
+	root1 := db.getMerkleRoot()
 
 	batch = db.NewBatch()
 	require.NoError(batch.Put([]byte("key"), []byte("value0")))
 	require.NoError(batch.Write())
 
-	startRoot := db.getMerkleRoot()
+	root2 := db.getMerkleRoot()
 
 	batch = db.NewBatch()
 	require.NoError(batch.Put([]byte("key1"), []byte("value0")))
@@ -185,31 +185,30 @@ func Test_History_Bad_GetValueChanges_Input(t *testing.T) {
 	require.NoError(batch.Put([]byte("key2"), []byte("value3")))
 	require.NoError(batch.Write())
 
-	endRoot := db.getMerkleRoot()
+	root3 := db.getMerkleRoot()
 
 	// ensure these start as valid calls
-	_, err = db.history.getValueChanges(toBeDeletedRoot, endRoot, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
+	_, err = db.history.getValueChanges(root1, root3, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
 	require.NoError(err)
-	_, err = db.history.getValueChanges(startRoot, endRoot, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
+	_, err = db.history.getValueChanges(root2, root3, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
 	require.NoError(err)
 
-	_, err = db.history.getValueChanges(startRoot, endRoot, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), -1)
+	_, err = db.history.getValueChanges(root2, root3, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), -1)
 	require.ErrorIs(err, ErrInvalidMaxLength)
 
-	_, err = db.history.getValueChanges(endRoot, startRoot, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
+	_, err = db.history.getValueChanges(root3, root2, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
 	require.ErrorIs(err, ErrInsufficientHistory)
 
-	// trigger the first root to be deleted by exiting the lookback window
+	// Cause root1 to be removed from the history
 	batch = db.NewBatch()
 	require.NoError(batch.Put([]byte("key2"), []byte("value4")))
 	require.NoError(batch.Write())
 
-	// now this root should no longer be present
-	_, err = db.history.getValueChanges(toBeDeletedRoot, endRoot, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
+	_, err = db.history.getValueChanges(root1, root3, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 1)
 	require.ErrorIs(err, ErrInsufficientHistory)
 
 	// same start/end roots should yield an empty changelist
-	changes, err := db.history.getValueChanges(endRoot, endRoot, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 10)
+	changes, err := db.history.getValueChanges(root3, root3, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 10)
 	require.NoError(err)
 	require.Empty(changes.values)
 }

x/merkledb/proof_test.go

@@ -654,6 +654,7 @@ func Test_ChangeProof_Missing_History_For_EndRoot(t *testing.T) {
 	require.NoError(err)
 
 	_, err = db.GetChangeProof(context.Background(), startRoot, ids.Empty, maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), 50)
+	require.ErrorIs(err, ErrNoEndRoot)
 	require.ErrorIs(err, ErrInsufficientHistory)
 }
 

x/sync/g_db/db_client.go

@@ -63,6 +63,9 @@ func (c *DBClient) GetChangeProof(
 	}
 
 	// TODO handle merkledb.ErrInvalidMaxLength
+	// TODO disambiguate between the root not being present due to
+	// the end root not being present and the start root not being
+	// present before the end root. i.e. ErrNoEndRoot vs ErrInsufficientHistory.
 	if resp.GetRootNotPresent() {
 		return nil, merkledb.ErrInsufficientHistory
 	}

x/sync/network_server.go

@@ -198,8 +198,15 @@ func (s *NetworkServer) HandleChangeProofRequest(
 		changeProof, err := s.db.GetChangeProof(ctx, startRoot, endRoot, start, end, int(keyLimit))
 		if err != nil {
 			if !errors.Is(err, merkledb.ErrInsufficientHistory) {
+				// We should only fail to get a change proof if we have insufficient history.
+				// Other errors are unexpected.
 				return err
 			}
+			if errors.Is(err, merkledb.ErrNoEndRoot) {
+				// [s.db] doesn't have [endRoot] in its history.
+				// We can't generate a change/range proof. Drop this request.
+				return nil
+			}
 
 			// [s.db] doesn't have sufficient history to generate change proof.
 			// Generate a range proof for the end root ID instead.

x/sync/network_server_test.go

@@ -263,7 +263,7 @@ func Test_Server_GetChangeProof(t *testing.T) {
 		"insufficient history for change proof or range proof": {
 			request: &pb.SyncGetChangeProofRequest{
 				// These roots don't exist so server has insufficient history
-				// to serve a change proof
+				// to serve a change proof or range proof
 				StartRootHash: ids.Empty[:],
 				EndRootHash:   fakeRootID[:],
 				KeyLimit:      defaultRequestKeyLimit,