Fix Concurrency Bug In CommitToParent

x/merkledb/db.go

@@ -752,7 +752,7 @@ func (db *Database) commitChanges(ctx context.Context, trieToCommit *trieView) e
 	// invalidate all child views except for the view being committed
 	db.invalidateChildrenExcept(trieToCommit)
 
-	// move any child views of the committed trie onto the db
+	// move any child views of the committed trie onto the db after changes have been made
 	db.moveChildViewsToDB(trieToCommit)
 
 	if len(changes.nodes) == 0 {

x/merkledb/trie_test.go

@@ -1181,6 +1181,87 @@ func TestTrieViewInvalidChildrenExcept(t *testing.T) {
 	require.Empty(view1.childViews)
 }
 
+func Test_Trie_CommitToParentView_Concurrent(t *testing.T) {
+	for i := 0; i < 5000; i++ {
+		dbTrie, err := getBasicDB()
+		require.NoError(t, err)
+		require.NotNil(t, dbTrie)
+
+		baseView, err := dbTrie.NewView()
+		require.NoError(t, err)
+
+		parentView, err := baseView.NewView()
+		require.NoError(t, err)
+		err = parentView.Insert(context.Background(), []byte{0}, []byte{0})
+		require.NoError(t, err)
+
+		childView, err := parentView.NewView()
+		require.NoError(t, err)
+		err = childView.Insert(context.Background(), []byte{1}, []byte{1})
+		require.NoError(t, err)
+
+		var wg sync.WaitGroup
+		wg.Add(2)
+		go func() {
+			defer wg.Done()
+			require.NoError(t, parentView.CommitToParent(context.Background()))
+		}()
+		go func() {
+			defer wg.Done()
+			go require.NoError(t, childView.CommitToParent(context.Background()))
+		}()
+
+		wg.Wait()
+
+		val0, err := baseView.GetValue(context.Background(), []byte{0})
+		require.NoError(t, err)
+		require.Equal(t, []byte{0}, val0)
+
+		val1, err := baseView.GetValue(context.Background(), []byte{1})
+		require.NoError(t, err)
+		require.Equal(t, []byte{1}, val1)
+	}
+}
+
+func Test_Trie_CommitToParentDB_Concurrent(t *testing.T) {
+	for i := 0; i < 5000; i++ {
+		dbTrie, err := getBasicDB()
+		require.NoError(t, err)
+		require.NotNil(t, dbTrie)
+
+		parentView, err := dbTrie.NewView()
+		require.NoError(t, err)
+		err = parentView.Insert(context.Background(), []byte{0}, []byte{0})
+		require.NoError(t, err)
+
+		childView, err := parentView.NewView()
+		require.NoError(t, err)
+		err = childView.Insert(context.Background(), []byte{1}, []byte{1})
+		require.NoError(t, err)
+
+		var wg sync.WaitGroup
+		wg.Add(2)
+		go func() {
+			defer wg.Done()
+			require.NoError(t, parentView.CommitToParent(context.Background()))
+		}()
+		go func() {
+			defer wg.Done()
+			go require.NoError(t, childView.CommitToParent(context.Background()))
+		}()
+
+		wg.Wait()
+
+		val0, err := dbTrie.GetValue(context.Background(), []byte{0})
+		require.NoError(t, err)
+		require.Equal(t, []byte{0}, val0)
+
+		val1, err := dbTrie.GetValue(context.Background(), []byte{1})
+		require.NoError(t, err)
+		require.Equal(t, []byte{1}, val1)
+	}
+}
+
 func Test_Trie_ConcurrentReadWrite(t *testing.T) {
 	require := require.New(t)
 

x/merkledb/trieview.go

@@ -27,7 +27,7 @@ const defaultPreallocationSize = 100
 
 var (
 	ErrCommitted          = errors.New("view has been committed")
-	ErrInvalid            = errors.New("the trie this view was based on has changed, rending this view invalid")
+	ErrInvalid            = errors.New("the trie this view was based on has changed, rendering this view invalid")
 	ErrOddLengthWithValue = errors.New(
 		"the underlying db only supports whole number of byte keys, so cannot record changes with odd nibble length",
 	)
@@ -222,10 +222,8 @@ func (t *trieView) calculateNodeIDs(ctx context.Context) error {
 
 	// ensure that the view under this one is up-to-date before potentially pulling in nodes from it
 	// getting the Merkle root forces any unupdated nodes to recalculate their ids
-	if t.parentTrie != nil {
-		if _, err := t.getParentTrie().GetMerkleRoot(ctx); err != nil {
-			return err
-		}
+	if _, err := t.getParentTrie().GetMerkleRoot(ctx); err != nil {
+		return err
 	}
 
 	if err := t.applyChangedValuesToTrie(ctx); err != nil {
@@ -515,12 +513,14 @@ func (t *trieView) commitChanges(ctx context.Context, trieToCommit *trieView) er
 	case trieToCommit == nil:
 		// no changes to apply
 		return nil
-	case trieToCommit.getParentTrie() != t:
-		// trieToCommit needs to be a child of t, otherwise the changes merge would not work
-		return ErrViewIsNotAChild
 	case trieToCommit.isInvalid():
 		// don't apply changes from an invalid view
 		return ErrInvalid
+	case t.committed:
+		return t.getParentTrie().commitChanges(ctx, trieToCommit)
+	case trieToCommit.getParentTrie() != t:
+		// trieToCommit needs to be a child of t, otherwise the changes merge would not work
+		return ErrViewIsNotAChild
 	}
 
 	// Invalidate all child views except the view being committed.
@@ -601,9 +601,6 @@ func (t *trieView) commitToParent(ctx context.Context) error {
 	if err := t.getParentTrie().commitChanges(ctx, t); err != nil {
 		return err
 	}
-	if t.isInvalid() {
-		return ErrInvalid
-	}
 
 	t.committed = true