MerkleDB Cleanup (ava-labs#1465)

Signed-off-by: Dan Laine <[email protected]> Co-authored-by: Dan Laine <[email protected]> Co-authored-by: Darioush Jalali <[email protected]>
joshua-kim · May 16, 2023 · 824c3b2 · 824c3b2
1 parent 2e44364
commit 824c3b2
Show file tree

Hide file tree

Showing 8 changed files with 148 additions and 183 deletions.
diff --git a/x/merkledb/db.go b/x/merkledb/db.go
@@ -43,7 +43,7 @@ var (
 
 	Codec, Version = newCodec()
 
-	rootKey                 = []byte{}
+	rootKey                 []byte
 	nodePrefix              = []byte("node")
 	metadataPrefix          = []byte("metadata")
 	cleanShutdownKey        = []byte("cleanShutdown")
@@ -65,7 +65,7 @@ type Config struct {
 	Tracer trace.Tracer
 }
 
-// Can only be edited by committing changes from a trieView.
+// Database can only be edited by committing changes from a trieView.
 type Database struct {
 	// Must be held when reading/writing fields.
 	lock sync.RWMutex
@@ -225,7 +225,7 @@ func New(ctx context.Context, db database.Database, config Config) (*Database, e
 	return newDatabase(ctx, db, config, metrics)
 }
 
-// Commits the key/value pairs within the [proof] to the db.
+// CommitChangeProof commits the key/value pairs within the [proof] to the db.
 func (db *Database) CommitChangeProof(ctx context.Context, proof *ChangeProof) error {
 	db.commitLock.Lock()
 	defer db.commitLock.Unlock()
@@ -241,7 +241,7 @@ func (db *Database) CommitChangeProof(ctx context.Context, proof *ChangeProof) e
 	return view.commitToDB(ctx)
 }
 
-// Commits the key/value pairs within the [proof] to the db.
+// CommitRangeProof commits the key/value pairs within the [proof] to the db.
 // [start] is the smallest key in the range this [proof] covers.
 func (db *Database) CommitRangeProof(ctx context.Context, start []byte, proof *RangeProof) error {
 	db.commitLock.Lock()
@@ -354,7 +354,7 @@ func (db *Database) getValueCopy(key path, lock bool) ([]byte, error) {
 // getValue returns the value for the given [key].
 // Returns database.ErrNotFound if it doesn't exist.
 // If [lock], [db.lock]'s read lock is acquired.
-// Otherwise assumes [db.lock] is already held.
+// Otherwise, assumes [db.lock] is already held.
 func (db *Database) getValue(key path, lock bool) ([]byte, error) {
 	if lock {
 		db.lock.RLock()
@@ -375,7 +375,7 @@ func (db *Database) getValue(key path, lock bool) ([]byte, error) {
 	return n.value.value, nil
 }
 
-// Returns the ID of the root node of the merkle trie.
+// GetMerkleRoot returns the ID of the root node of the merkle trie.
 func (db *Database) GetMerkleRoot(ctx context.Context) (ids.ID, error) {
 	_, span := db.tracer.Start(ctx, "MerkleDB.GetMerkleRoot")
 	defer span.End()
@@ -396,7 +396,7 @@ func (db *Database) getMerkleRoot() ids.ID {
 	return db.root.id
 }
 
-// Returns a proof of the existence/non-existence of [key] in this trie.
+// GetProof returns a proof of the existence/non-existence of [key] in this trie.
 func (db *Database) GetProof(ctx context.Context, key []byte) (*Proof, error) {
 	db.commitLock.RLock()
 	defer db.commitLock.RUnlock()
@@ -419,7 +419,7 @@ func (db *Database) getProof(ctx context.Context, key []byte) (*Proof, error) {
 	return view.getProof(ctx, key)
 }
 
-// Returns a proof for the key/value pairs in this trie within the range
+// GetRangeProof returns a proof for the key/value pairs in this trie within the range
 // [start, end].
 func (db *Database) GetRangeProof(
 	ctx context.Context,
@@ -433,7 +433,7 @@ func (db *Database) GetRangeProof(
 	return db.getRangeProofAtRoot(ctx, db.getMerkleRoot(), start, end, maxLength)
 }
 
-// Returns a proof for the key/value pairs in this trie within the range
+// GetRangeProofAtRoot returns a proof for the key/value pairs in this trie within the range
 // [start, end] when the root of the trie was [rootID].
 func (db *Database) GetRangeProofAtRoot(
 	ctx context.Context,
@@ -470,7 +470,7 @@ func (db *Database) getRangeProofAtRoot(
 	return historicalView.GetRangeProof(ctx, start, end, maxLength)
 }
 
-// Returns a proof for a subset of the key/value changes in key range
+// GetChangeProof returns a proof for a subset of the key/value changes in key range
 // [start, end] that occurred between [startRootID] and [endRootID].
 // Returns at most [maxLength] key/value pairs.
 func (db *Database) GetChangeProof(
@@ -577,7 +577,7 @@ func (db *Database) GetChangeProof(
 	return result, nil
 }
 
-// Returns a new view on top of this trie.
+// NewView returns a new view on top of this trie.
 // Changes made to the view will only be reflected in the original trie if Commit is called.
 // Assumes [db.lock] isn't held.
 func (db *Database) NewView() (TrieView, error) {
@@ -591,7 +591,7 @@ func (db *Database) newUntrackedView(estimatedSize int) (*trieView, error) {
 	return newTrieView(db, db, db.root.clone(), estimatedSize)
 }
 
-// Returns a new view preallocated to hold at least [estimatedSize] value changes at a time.
+// NewPreallocatedView returns a new view with memory allocated to hold at least [estimatedSize] value changes at a time.
 // If more changes are made, additional memory will be allocated.
 // The returned view is added to [db.childViews].
 // Assumes [db.lock] isn't held.
@@ -721,7 +721,7 @@ func (db *Database) onEviction(node *node) error {
 	return nil
 }
 
-// Inserts the key/value pair into the db.
+// Put upserts the key/value pair into the db.
 func (db *Database) Put(k, v []byte) error {
 	return db.Insert(context.Background(), k, v)
 }

diff --git a/x/merkledb/db_test.go b/x/merkledb/db_test.go
@@ -710,10 +710,8 @@ func applyOperations(t *Database, ops []*testOperation) (Trie, error) {
 			if err := view.Remove(context.Background(), op.key); err != nil {
 				return nil, err
 			}
-		} else {
-			if err := view.Insert(context.Background(), op.key, op.value); err != nil {
-				return nil, err
-			}
+		} else if err := view.Insert(context.Background(), op.key, op.value); err != nil {
+			return nil, err
 		}
 	}
 	return view, nil

diff --git a/x/merkledb/history.go b/x/merkledb/history.go
@@ -158,31 +158,33 @@ func (th *trieHistory) getValueChanges(startRoot, endRoot ids.ID, start, end []b
 
 			// Add the changes from this commit to [combinedChanges].
 			for key, valueChange := range item.values {
-				if (len(startPath) == 0 || key.Compare(startPath) >= 0) &&
-					(len(endPath) == 0 || key.Compare(endPath) <= 0) {
-					// The key is in the range [start, end].
-					if existing, ok := combinedChanges.values[key]; ok {
-						// A change to this key already exists in [combinedChanges].
-						existing.after = valueChange.after
-						if existing.before.hasValue == existing.after.hasValue &&
-							bytes.Equal(existing.before.value, existing.after.value) {
-							// The change to this key is a no-op, so remove it from [combinedChanges].
-							delete(combinedChanges.values, key)
-							sortedKeys.Delete(key)
-						}
-					} else {
-						combinedChanges.values[key] = &change[Maybe[[]byte]]{
-							before: valueChange.before,
-							after:  valueChange.after,
-						}
-						sortedKeys.ReplaceOrInsert(key)
+				// The key is outside the range [start, end].
+				if (len(startPath) > 0 && key.Compare(startPath) < 0) ||
+					(len(endPath) > 0 && key.Compare(endPath) > 0) {
+					continue
+				}
+
+				// A change to this key already exists in [combinedChanges]
+				// so update its before value with the earlier before value
+				if existing, ok := combinedChanges.values[key]; ok {
+					existing.after = valueChange.after
+					if existing.before.hasValue == existing.after.hasValue &&
+						bytes.Equal(existing.before.value, existing.after.value) {
+						// The change to this key is a no-op, so remove it from [combinedChanges].
+						delete(combinedChanges.values, key)
+						sortedKeys.Delete(key)
+					}
+				} else {
+					combinedChanges.values[key] = &change[Maybe[[]byte]]{
+						before: valueChange.before,
+						after:  valueChange.after,
 					}
+					sortedKeys.ReplaceOrInsert(key)
 				}
 			}
-
+			// continue to next change list
 			return true
-		},
-	)
+		})
 
 	// Keep only the smallest [maxLength] items in [combinedChanges.values].
 	for sortedKeys.Len() > maxLength {

diff --git a/x/merkledb/trieview.go b/x/merkledb/trieview.go
@@ -997,10 +997,8 @@ func (t *trieView) applyChangedValuesToTrie(ctx context.Context) error {
 			if err := t.removeFromTrie(key); err != nil {
 				return err
 			}
-		} else {
-			if _, err := t.insertIntoTrie(key, change); err != nil {
-				return err
-			}
+		} else if _, err := t.insertIntoTrie(key, change); err != nil {
+			return err
 		}
 	}
 	return nil
@@ -1228,7 +1226,7 @@ func (t *trieView) insertIntoTrie(
 
 	existingChildKey := key[:closestNodeKeyLength+1] + existingChildEntry.compressedPath
 
-	// the existing child's key is of length: len(closestNodekey) + 1 for the child index + len(existing child's compressed key)
+	// the existing child's key is of length: len(closestNodeKey) + 1 for the child index + len(existing child's compressed key)
 	// if that length is less than or equal to the branch node's key that implies that the existing child's key matched the key to be inserted
 	// since it matched the key to be inserted, it should have been returned by GetPathTo
 	if len(existingChildKey) <= len(branchNode.key) {

diff --git a/x/sync/client_test.go b/x/sync/client_test.go
@@ -119,7 +119,7 @@ func TestGetRangeProof(t *testing.T) {
 	smallTrieRoot, err := smallTrieDB.GetMerkleRoot(context.Background())
 	require.NoError(t, err)
 
-	largeTrieKeyCount := 10_000
+	largeTrieKeyCount := 3 * defaultRequestKeyLimit
 	largeTrieDB, largeTrieKeys, err := generateTrieWithMinKeyLen(t, r, largeTrieKeyCount, 1)
 	require.NoError(t, err)
 	largeTrieRoot, err := largeTrieDB.GetMerkleRoot(context.Background())
@@ -232,7 +232,7 @@ func TestGetRangeProof(t *testing.T) {
 			modifyResponse: func(response *merkledb.RangeProof) {
 				response.KeyValues = response.KeyValues[:len(response.KeyValues)-2]
 			},
-			expectedErr: merkledb.ErrInvalidProof,
+			expectedErr: merkledb.ErrProofNodeNotForKey,
 		},
 		"removed key from middle of response": {
 			db: largeTrieDB,