Remove node/value lock from trieview

x/merkledb/README.md

@@ -221,26 +221,24 @@ A `Merkle Node` holds the IDs of its children, its value, as well as any path ex
 
 ### Validity
 
-A `trieView` is built atop another trie, and that trie could change at any point.  If it does, all descendants of the trie will be marked invalid before the edit of the trie occurs.  If an operation is performed on an invalid trie, an ErrInvalid error will be returned instead of the expected result.  When a view is committed, all of its sibling views (the views that share the same parent) are marked invalid and any child views of the view have their parent updated to exclude any committed views between them and the db.
+A `trieView` is built atop another trie, and there may be other `trieView`s built atop the same trie. We call these *siblings*. If one sibling is committed to database, we *invalidate* all other siblings and their descendants. Operations on an invalid trie return `ErrInvalid`. The children of the committed `trieView` are updated so that their new `parentTrie` is the database.
 
 ### Locking
 
-`Database` has a `RWMutex` named `lock`. Its read operations don't store data in a map, so a read lock suffices for read operations.
-`Database` has a `Mutex` named `commitLock`.  It enforces that only a single view/batch is attempting to commit to the database at one time.  `lock` is insufficient because there is a period of view preparation where read access should still be allowed, followed by a period where a full write lock is needed.  The `commitLock` ensures that only a single goroutine makes the transition from read => write.
+`merkleDB` has a `RWMutex` named `lock`. Its read operations don't store data in a map, so a read lock suffices for read operations.
+`merkleDB` has a `Mutex` named `commitLock`. It enforces that only a single view/batch is attempting to commit to the database at one time.  `lock` is insufficient because there is a period of view preparation where read access should still be allowed, followed by a period where a full write lock is needed. The `commitLock` ensures that only a single goroutine makes the transition from read => write.
 
-A `trieView` is built atop another trie, which may be the underlying `Database` or another `trieView`.
-It's important to guarantee atomicity/consistency of trie operations.
-That is, if a view method is executing, the views/database underneath the view shouldn't be changing.
-To prevent this, we need to use locking.
+A `trieView` is built atop another trie, which may be the underlying `merkleDB` or another `trieView`.
+We use locking to guarantee atomicity/consistency of trie operations.
 
-`trieView` has a `RWMutex` named `lock` that's held when methods that access the trie's structure are executing.  It is responsible for ensuring that writing/reading from a `trieView` or from any *ancestor* is safe.
-It also has a `RWMutex` named `validityTrackingLock` that is held during methods that change the view's validity, tracking of child views' validity, or of the `trieView` parent trie.  This lock ensures that writing/reading from `trieView` or any of its *descendants* is safe.
-The `Commit` function also grabs the `Database`'s `commitLock` lock. This is the only `trieView` method that modifies the underlying `Database`.  If an ancestor is modified during this time, the commit will error with ErrInvalid.
+`trieView` has a `RWMutex` named `commitLock` which ensures that we don't create a view atop the `trieView` while it's being committed.
+It also has a `RWMutex` named `validityTrackingLock` that is held during methods that change the view's validity, tracking of child views' validity, or of the `trieView` parent trie.  This lock ensures that writing/reading from `trieView` or any of its descendants is safe.
+The `CommitToDB` method grabs the `merkleDB`'s `commitLock`. This is the only `trieView` method that modifies the underlying `merkleDB`.
 
-In some of `Database`'s methods, we create a `trieView` and call unexported methods on it without locking it.
-We do so because the exported counterpart of the method read locks the `Database`, which is already locked.
-This pattern is safe because the `Database` is locked, so no data under the view is changing, and nobody else has a reference to the view, so there can't be any concurrent access.
+In some of `merkleDB`'s methods, we create a `trieView` and call unexported methods on it without locking it.
+We do so because the exported counterpart of the method read locks the `merkleDB`, which is already locked.
+This pattern is safe because the `merkleDB` is locked, so no data under the view is changing, and nobody else has a reference to the view, so there can't be any concurrent access.
 
-To prevent deadlocks, `trieView` and `Database` never acquire the `lock` of any descendant views that are built atop it.
-That is, locking is always done from a view down to the underlying `Database`, never the other way around.
-The `validityTrackingLock` goes the opposite way.  Views can validityTrackingLock their children, but not their ancestors. Because of this, any function that takes the `validityTrackingLock` should avoid taking the `lock` as this will likely trigger a deadlock.  Keeping `lock` solely in the ancestor direction and `validityTrackingLock` solely in the descendant direction prevents deadlocks from occurring.
+To prevent deadlocks, `trieView` and `merkleDB` never acquire the `commitLock` of descendant views.
+That is, locking is always done from a view toward to the underlying `merkleDB`, never the other way around.
+The `validityTrackingLock` goes the opposite way. A view can lock the `validityTrackingLock` of its children, but not its ancestors. Because of this, any function that takes the `validityTrackingLock` must not take the `commitLock` as this may cause a deadlock. Keeping `commitLock` solely in the ancestor direction and `validityTrackingLock` solely in the descendant direction prevents deadlocks from occurring.

x/merkledb/db.go

@@ -423,7 +423,7 @@ func (db *merkleDB) GetValues(ctx context.Context, keys [][]byte) ([][]byte, []e
 	values := make([][]byte, len(keys))
 	errors := make([]error, len(keys))
 	for i, key := range keys {
-		values[i], errors[i] = db.getValueCopy(newPath(key), false /*lock*/)
+		values[i], errors[i] = db.getValueCopy(newPath(key))
 	}
 	return values, errors
 }
@@ -434,13 +434,17 @@ func (db *merkleDB) GetValue(ctx context.Context, key []byte) ([]byte, error) {
 	_, span := db.tracer.Start(ctx, "MerkleDB.GetValue")
 	defer span.End()
 
-	return db.getValueCopy(newPath(key), true /*lock*/)
+	db.lock.RLock()
+	defer db.lock.RUnlock()
+
+	return db.getValueCopy(newPath(key))
 }
 
 // getValueCopy returns a copy of the value for the given [key].
 // Returns database.ErrNotFound if it doesn't exist.
-func (db *merkleDB) getValueCopy(key path, lock bool) ([]byte, error) {
-	val, err := db.getValue(key, lock)
+// Assumes [db.lock] is read locked.
+func (db *merkleDB) getValueCopy(key path) ([]byte, error) {
+	val, err := db.getValueWithoutLock(key)
 	if err != nil {
 		return nil, err
 	}
@@ -449,14 +453,18 @@ func (db *merkleDB) 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.
-func (db *merkleDB) getValue(key path, lock bool) ([]byte, error) {
-	if lock {
-		db.lock.RLock()
-		defer db.lock.RUnlock()
-	}
+// Assumes [db.lock] isn't held.
+func (db *merkleDB) getValue(key path) ([]byte, error) {
+	db.lock.RLock()
+	defer db.lock.RUnlock()
 
+	return db.getValueWithoutLock(key)
+}
+
+// getValueWithoutLock returns the value for the given [key].
+// Returns database.ErrNotFound if it doesn't exist.
+// Assumes [db.lock] is read locked.
+func (db *merkleDB) getValueWithoutLock(key path) ([]byte, error) {
 	if db.closed {
 		return nil, database.ErrClosed
 	}
@@ -657,7 +665,7 @@ func (db *merkleDB) GetChangeProof(
 // 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.commitLock] and [db.lock] aren't held.
-func (db *merkleDB) NewView(batchOps []database.BatchOp) (TrieView, error) {
+func (db *merkleDB) NewView(_ context.Context, batchOps []database.BatchOp) (TrieView, error) {
 	// ensure the db doesn't change while creating the new view
 	db.commitLock.RLock()
 	defer db.commitLock.RUnlock()
@@ -698,7 +706,7 @@ func (db *merkleDB) Has(k []byte) (bool, error) {
 		return false, database.ErrClosed
 	}
 
-	_, err := db.getValue(newPath(k), false /*lock*/)
+	_, err := db.getValueWithoutLock(newPath(k))
 	if err == database.ErrNotFound {
 		return false, nil
 	}
@@ -870,11 +878,6 @@ func (db *merkleDB) commitBatch(ops []database.BatchOp) error {
 	return view.commitToDB(context.Background())
 }
 
-// commitToDB is a no-op for the db because it is the db
-func (*merkleDB) commitToDB(context.Context) error {
-	return nil
-}
-
 // commitChanges commits the changes in [trieToCommit] to [db].
 // Assumes [trieToCommit]'s node IDs have been calculated.
 func (db *merkleDB) commitChanges(ctx context.Context, trieToCommit *trieView) error {
@@ -1108,7 +1111,7 @@ func (db *merkleDB) VerifyChangeProof(
 	}
 
 	// Make sure we get the expected root.
-	calculatedRoot, err := view.getMerkleRoot(ctx)
+	calculatedRoot, err := view.GetMerkleRoot(ctx)
 	if err != nil {
 		return err
 	}
@@ -1197,7 +1200,7 @@ func (db *merkleDB) getHistoricalViewForRange(
 	if err != nil {
 		return nil, err
 	}
-	return newTrieViewWithChanges(db, db, changeHistory)
+	return newHistoricalTrieView(db, changeHistory)
 }
 
 // Returns all keys in range [start, end] that aren't in [keySet].

x/merkledb/db_test.go

@@ -116,9 +116,9 @@ func Test_MerkleDB_DB_Load_Root_From_DB(t *testing.T) {
 		k := []byte(strconv.Itoa(i))
 		ops = append(ops, database.BatchOp{Key: k, Value: hashing.ComputeHash256(k)})
 	}
-	view, err := db.NewView(ops)
+	view, err := db.NewView(context.Background(), ops)
 	require.NoError(err)
-	require.NoError(view.commitToDB(context.Background()))
+	require.NoError(view.CommitToDB(context.Background()))
 
 	root, err := db.GetMerkleRoot(context.Background())
 	require.NoError(err)
@@ -162,7 +162,7 @@ func Test_MerkleDB_DB_Rebuild(t *testing.T) {
 		k := []byte(strconv.Itoa(i))
 		ops = append(ops, database.BatchOp{Key: k, Value: hashing.ComputeHash256(k)})
 	}
-	view, err := db.NewView(ops)
+	view, err := db.NewView(context.Background(), ops)
 	require.NoError(err)
 	require.NoError(view.CommitToDB(context.Background()))
 
@@ -237,12 +237,12 @@ func Test_MerkleDB_Invalidate_Siblings_On_Commit(t *testing.T) {
 	require.NoError(err)
 	require.NotNil(dbTrie)
 
-	viewToCommit, err := dbTrie.NewView([]database.BatchOp{{Key: []byte{0}, Value: []byte{0}}})
+	viewToCommit, err := dbTrie.NewView(context.Background(), []database.BatchOp{{Key: []byte{0}, Value: []byte{0}}})
 	require.NoError(err)
 
-	sibling1, err := dbTrie.NewView(nil)
+	sibling1, err := dbTrie.NewView(context.Background(), nil)
 	require.NoError(err)
-	sibling2, err := dbTrie.NewView(nil)
+	sibling2, err := dbTrie.NewView(context.Background(), nil)
 	require.NoError(err)
 
 	require.False(sibling1.(*trieView).isInvalid())
@@ -466,7 +466,7 @@ func TestDatabaseNewViewTracked(t *testing.T) {
 	require.NoError(err)
 
 	// Create a new tracked view.
-	view, err := db.NewView([]database.BatchOp{{Key: []byte{1}, Value: []byte{1}}})
+	view, err := db.NewView(context.Background(), []database.BatchOp{{Key: []byte{1}, Value: []byte{1}}})
 	require.NoError(err)
 	require.Len(db.childViews, 1)
 
@@ -486,39 +486,39 @@ func TestDatabaseCommitChanges(t *testing.T) {
 	dbRoot := db.getMerkleRoot()
 
 	// Committing a nil view should be a no-op.
-	require.NoError(db.commitToDB(context.Background()))
+	require.NoError(db.CommitToDB(context.Background()))
 	require.Equal(dbRoot, db.getMerkleRoot()) // Root didn't change
 
 	// Committing an invalid view should fail.
-	invalidView, err := db.NewView(nil)
+	invalidView, err := db.NewView(context.Background(), nil)
 	require.NoError(err)
 	invalidView.(*trieView).invalidate()
-	err = invalidView.commitToDB(context.Background())
+	err = invalidView.CommitToDB(context.Background())
 	require.ErrorIs(err, ErrInvalid)
 
 	// Add key-value pairs to the database
 	require.NoError(db.Put([]byte{1}, []byte{1}))
 	require.NoError(db.Put([]byte{2}, []byte{2}))
 
 	// Make a view and insert/delete a key-value pair.
-	view1Intf, err := db.NewView([]database.BatchOp{
+	view1Intf, err := db.NewView(context.Background(), []database.BatchOp{
 		{Key: []byte{3}, Value: []byte{3}},
 		{Key: []byte{1}, Delete: true},
 	})
 	require.NoError(err)
 	require.IsType(&trieView{}, view1Intf)
 	view1 := view1Intf.(*trieView)
-	view1Root, err := view1.getMerkleRoot(context.Background())
+	view1Root, err := view1.GetMerkleRoot(context.Background())
 	require.NoError(err)
 
 	// Make a second view
-	view2Intf, err := db.NewView(nil)
+	view2Intf, err := db.NewView(context.Background(), nil)
 	require.NoError(err)
 	require.IsType(&trieView{}, view2Intf)
 	view2 := view2Intf.(*trieView)
 
 	// Make a view atop a view
-	view3Intf, err := view1.NewView(nil)
+	view3Intf, err := view1.NewView(context.Background(), nil)
 	require.NoError(err)
 	require.IsType(&trieView{}, view3Intf)
 	view3 := view3Intf.(*trieView)
@@ -568,17 +568,17 @@ func TestDatabaseInvalidateChildrenExcept(t *testing.T) {
 	require.NoError(err)
 
 	// Create children
-	view1Intf, err := db.NewView(nil)
+	view1Intf, err := db.NewView(context.Background(), nil)
 	require.NoError(err)
 	require.IsType(&trieView{}, view1Intf)
 	view1 := view1Intf.(*trieView)
 
-	view2Intf, err := db.NewView(nil)
+	view2Intf, err := db.NewView(context.Background(), nil)
 	require.NoError(err)
 	require.IsType(&trieView{}, view2Intf)
 	view2 := view2Intf.(*trieView)
 
-	view3Intf, err := db.NewView(nil)
+	view3Intf, err := db.NewView(context.Background(), nil)
 	require.NoError(err)
 	require.IsType(&trieView{}, view3Intf)
 	view3 := view3Intf.(*trieView)
@@ -659,15 +659,15 @@ func Test_MerkleDB_Random_Insert_Ordering(t *testing.T) {
 		}
 		db, err := getBasicDB()
 		require.NoError(err)
-		result, err := db.NewView(ops)
+		result, err := db.NewView(context.Background(), ops)
 		require.NoError(err)
 		primaryRoot, err := result.GetMerkleRoot(context.Background())
 		require.NoError(err)
 		for shuffleIndex := 0; shuffleIndex < 3; shuffleIndex++ {
 			r.Shuffle(totalState, func(i, j int) {
 				ops[i], ops[j] = ops[j], ops[i]
 			})
-			result, err := db.NewView(ops)
+			result, err := db.NewView(context.Background(), ops)
 			require.NoError(err)
 			newRoot, err := result.GetMerkleRoot(context.Background())
 			require.NoError(err)
@@ -846,7 +846,7 @@ func runRandDBTest(require *require.Assertions, r *rand.Rand, rt randTest) {
 			for key, value := range values {
 				ops = append(ops, database.BatchOp{Key: key.Serialize().Value, Value: value})
 			}
-			newView, err := dbTrie.NewView(ops)
+			newView, err := dbTrie.NewView(context.Background(), ops)
 			require.NoError(err)
 
 			calculatedRoot, err := newView.GetMerkleRoot(context.Background())

x/merkledb/proof_test.go

@@ -255,7 +255,7 @@ func Test_RangeProof_MaxLength(t *testing.T) {
 	dbTrie, err := getBasicDB()
 	require.NoError(err)
 	require.NotNil(dbTrie)
-	trie, err := dbTrie.NewView(nil)
+	trie, err := dbTrie.NewView(context.Background(), nil)
 	require.NoError(err)
 
 	_, err = trie.GetRangeProof(context.Background(), maybe.Nothing[[]byte](), maybe.Nothing[[]byte](), -1)
@@ -271,7 +271,7 @@ func Test_Proof(t *testing.T) {
 	dbTrie, err := getBasicDB()
 	require.NoError(err)
 	require.NotNil(dbTrie)
-	trie, err := dbTrie.NewView([]database.BatchOp{
+	trie, err := dbTrie.NewView(context.Background(), []database.BatchOp{
 		{Key: []byte("key0"), Value: []byte("value0")},
 		{Key: []byte("key1"), Value: []byte("value1")},
 		{Key: []byte("key2"), Value: []byte("value2")},

x/merkledb/trie.go

@@ -39,7 +39,7 @@ type ReadOnlyTrie interface {
 
 	// get the value associated with the key in path form
 	// database.ErrNotFound if the key is not present
-	getValue(key path, lock bool) ([]byte, error)
+	getValue(key path) ([]byte, error)
 
 	// get an editable copy of the node with the given key path
 	getEditableNode(key path) (*node, error)
@@ -57,7 +57,7 @@ type Trie interface {
 	ReadOnlyTrie
 
 	// NewView returns a new view on top of this Trie with the specified changes
-	NewView(batchOps []database.BatchOp) (TrieView, error)
+	NewView(ctx context.Context, batchOps []database.BatchOp) (TrieView, error)
 }
 
 type TrieView interface {
@@ -66,7 +66,4 @@ type TrieView interface {
 	// CommitToDB writes the changes in this view to the database.
 	// Takes the DB commit lock.
 	CommitToDB(ctx context.Context) error
-
-	// Same as CommitToDB but doesn't take the DB commit lock.
-	commitToDB(ctx context.Context) error
 }