From b443c9b874ea783c1ccd4170467975c124781988 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 6 Dec 2023 13:47:56 -0500 Subject: [PATCH] disambiguate ErrInsufficientHistory from ErrNoEndRoot --- x/merkledb/db.go | 2 ++ x/merkledb/history.go | 9 +++++++-- x/merkledb/history_test.go | 21 ++++++++++----------- x/merkledb/proof_test.go | 1 + x/sync/g_db/db_client.go | 3 +++ x/sync/network_server.go | 7 +++++++ x/sync/network_server_test.go | 2 +- 7 files changed, 31 insertions(+), 14 deletions(-) diff --git a/x/merkledb/db.go b/x/merkledb/db.go index c813a9478b1f..4e70c02fbd8f 100644 --- a/x/merkledb/db.go +++ b/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, diff --git a/x/merkledb/history.go b/x/merkledb/history.go index c52385445cd2..dd4ff4f93c8c 100644 --- a/x/merkledb/history.go +++ b/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 diff --git a/x/merkledb/history_test.go b/x/merkledb/history_test.go index d2945c9c5018..3c8e8700d567 100644 --- a/x/merkledb/history_test.go +++ b/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) } diff --git a/x/merkledb/proof_test.go b/x/merkledb/proof_test.go index b22b80ffd09d..fbee117d4e68 100644 --- a/x/merkledb/proof_test.go +++ b/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) } diff --git a/x/sync/g_db/db_client.go b/x/sync/g_db/db_client.go index 64e63bb76652..af1ce1c9080b 100644 --- a/x/sync/g_db/db_client.go +++ b/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 } diff --git a/x/sync/network_server.go b/x/sync/network_server.go index c213bee6a739..8027a05042f1 100644 --- a/x/sync/network_server.go +++ b/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. diff --git a/x/sync/network_server_test.go b/x/sync/network_server_test.go index d79b27a14c44..7b83afc3e7d5 100644 --- a/x/sync/network_server_test.go +++ b/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,