Merge branch 'rc/v1.7.0' into fix_MetricCountConsensus

multiversx · Jan 15, 2024 · eeeb6f8 · eeeb6f8
2 parents 60b0978 + 6e6bad9
commit eeeb6f8
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 24 deletions.
diff --git a/trie/branchNode.go b/trie/branchNode.go
@@ -5,7 +5,6 @@ import (
 	"encoding/hex"
 	"fmt"
 	"io"
-	"strings"
 	"sync"
 
 	"github.com/multiversx/mx-chain-core-go/core"
@@ -311,13 +310,13 @@ func (bn *branchNode) commitSnapshot(
 
 	for i := range bn.children {
 		err = resolveIfCollapsed(bn, byte(i), db)
+		childIsMissing, err := treatCommitSnapshotError(err, bn.EncodedChildren[i], missingNodesChan)
 		if err != nil {
-			if strings.Contains(err.Error(), core.GetNodeFromDBErrorString) {
-				treatCommitSnapshotError(err, bn.EncodedChildren[i], missingNodesChan)
-				continue
-			}
 			return err
 		}
+		if childIsMissing {
+			continue
+		}
 
 		if bn.children[i] == nil {
 			continue

diff --git a/trie/branchNode_test.go b/trie/branchNode_test.go
@@ -1348,10 +1348,25 @@ func TestBranchNode_commitSnapshotDbIsClosing(t *testing.T) {
 	_, collapsedBn := getBnAndCollapsedBn(getTestMarshalizerAndHasher())
 	missingNodesChan := make(chan []byte, 10)
 	err := collapsedBn.commitSnapshot(db, nil, missingNodesChan, context.Background(), statistics.NewTrieStatistics(), &testscommon.ProcessStatusHandlerStub{}, 0)
-	assert.Nil(t, err)
+	assert.True(t, core.IsClosingError(err))
 	assert.Equal(t, 0, len(missingNodesChan))
 }
 
+func TestBranchNode_commitSnapshotChildIsMissingErr(t *testing.T) {
+	t.Parallel()
+
+	db := testscommon.NewMemDbMock()
+	db.GetCalled = func(key []byte) ([]byte, error) {
+		return nil, core.NewGetNodeFromDBErrWithKey(key, ErrKeyNotFound, "test")
+	}
+
+	_, collapsedBn := getBnAndCollapsedBn(getTestMarshalizerAndHasher())
+	missingNodesChan := make(chan []byte, 10)
+	err := collapsedBn.commitSnapshot(db, nil, missingNodesChan, context.Background(), statistics.NewTrieStatistics(), &testscommon.ProcessStatusHandlerStub{}, 0)
+	assert.Nil(t, err)
+	assert.Equal(t, 3, len(missingNodesChan))
+}
+
 func TestBranchNode_getVersion(t *testing.T) {
 	t.Parallel()
 

diff --git a/trie/extensionNode.go b/trie/extensionNode.go
@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io"
 	"math"
-	"strings"
 	"sync"
 
 	"github.com/multiversx/mx-chain-core-go/core"
@@ -229,17 +228,12 @@ func (en *extensionNode) commitSnapshot(
 	}
 
 	err = resolveIfCollapsed(en, 0, db)
-	isMissingNodeErr := false
+	childIsMissing, err := treatCommitSnapshotError(err, en.EncodedChild, missingNodesChan)
 	if err != nil {
-		isMissingNodeErr = strings.Contains(err.Error(), core.GetNodeFromDBErrorString)
-		if !isMissingNodeErr {
-			return err
-		}
+		return err
 	}
 
-	if isMissingNodeErr {
-		treatCommitSnapshotError(err, en.EncodedChild, missingNodesChan)
-	} else {
+	if !childIsMissing {
 		err = en.child.commitSnapshot(db, leavesChan, missingNodesChan, ctx, stats, idleProvider, depthLevel+1)
 		if err != nil {
 			return err

diff --git a/trie/extensionNode_test.go b/trie/extensionNode_test.go
@@ -1039,7 +1039,7 @@ func TestExtensionNode_commitSnapshotDbIsClosing(t *testing.T) {
 	_, collapsedEn := getEnAndCollapsedEn()
 	missingNodesChan := make(chan []byte, 10)
 	err := collapsedEn.commitSnapshot(db, nil, missingNodesChan, context.Background(), statistics.NewTrieStatistics(), &testscommon.ProcessStatusHandlerStub{}, 0)
-	assert.Nil(t, err)
+	assert.True(t, core.IsClosingError(err))
 	assert.Equal(t, 0, len(missingNodesChan))
 }
 

diff --git a/trie/node.go b/trie/node.go
@@ -275,14 +275,18 @@ func shouldStopIfContextDoneBlockingIfBusy(ctx context.Context, idleProvider Idl
 	}
 }
 
-func treatCommitSnapshotError(err error, hash []byte, missingNodesChan chan []byte) {
-	if core.IsClosingError(err) {
-		log.Debug("context closing", "hash", hash)
-		return
+func treatCommitSnapshotError(err error, hash []byte, missingNodesChan chan []byte) (nodeIsMissing bool, error error) {
+	if err == nil {
+		return false, nil
+	}
+
+	if !core.IsGetNodeFromDBError(err) {
+		return false, err
 	}
 
 	log.Error("error during trie snapshot", "err", err.Error(), "hash", hash)
 	missingNodesChan <- hash
+	return true, nil
 }
 
 func shouldMigrateCurrentNode(

diff --git a/trie/node_test.go b/trie/node_test.go
@@ -3,6 +3,7 @@ package trie
 import (
 	"context"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -1165,6 +1166,45 @@ func TestNodesVersion_deleteFromBn(t *testing.T) {
 	})
 }
 
+func Test_treatCommitSnapshotErr(t *testing.T) {
+	t.Parallel()
+
+	t.Run("nil err", func(t *testing.T) {
+		t.Parallel()
+
+		childIsMissing, err := treatCommitSnapshotError(nil, []byte("hash"), nil)
+		assert.False(t, childIsMissing)
+		assert.Nil(t, err)
+	})
+	t.Run("err is not of type GetNodeFromDBError", func(t *testing.T) {
+		t.Parallel()
+
+		expectedErr := errors.New("some error")
+		childIsMissing, err := treatCommitSnapshotError(expectedErr, []byte("hash"), nil)
+		assert.False(t, childIsMissing)
+		assert.Equal(t, expectedErr, err)
+	})
+	t.Run("is closing err", func(t *testing.T) {
+		t.Parallel()
+
+		expectedErr := fmt.Errorf("%w: %s", core.ErrContextClosing, core.GetNodeFromDBErrorString)
+		childIsMissing, err := treatCommitSnapshotError(expectedErr, []byte("hash"), nil)
+		assert.False(t, childIsMissing)
+		assert.Equal(t, expectedErr, err)
+	})
+	t.Run("child is missing", func(t *testing.T) {
+		t.Parallel()
+
+		expectedErr := fmt.Errorf("%w: %s", ErrKeyNotFound, core.GetNodeFromDBErrorString)
+		missingNodesChan := make(chan []byte, 1)
+		childIsMissing, err := treatCommitSnapshotError(expectedErr, []byte("hash"), missingNodesChan)
+		assert.True(t, childIsMissing)
+		assert.Nil(t, err)
+		assert.Equal(t, 1, len(missingNodesChan))
+		assert.Equal(t, []byte("hash"), <-missingNodesChan)
+	})
+}
+
 func Benchmark_ShouldStopIfContextDoneBlockingIfBusy(b *testing.B) {
 	ctx := context.Background()
 	b.ResetTimer()

diff --git a/trie/trieStorageManager.go b/trie/trieStorageManager.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"strings"
 	"sync"
 	"time"
 
@@ -425,10 +424,8 @@ func newSnapshotNode(
 	missingNodesCh chan []byte,
 ) (snapshotNode, error) {
 	newRoot, err := getNodeFromDBAndDecode(rootHash, db, msh, hsh)
+	_, _ = treatCommitSnapshotError(err, rootHash, missingNodesCh)
 	if err != nil {
-		if strings.Contains(err.Error(), core.GetNodeFromDBErrorString) {
-			treatCommitSnapshotError(err, rootHash, missingNodesCh)
-		}
 		return nil, err
 	}
 

diff --git a/trie/trieStorageManager_test.go b/trie/trieStorageManager_test.go
@@ -405,6 +405,20 @@ func TestTrieStorageManager_ShouldTakeSnapshot(t *testing.T) {
 
 		assert.False(t, ts.ShouldTakeSnapshot())
 	})
+	t.Run("different syncVal marker should return true", func(t *testing.T) {
+		t.Parallel()
+
+		args := trie.GetDefaultTrieStorageManagerParameters()
+		args.MainStorer = &trieMock.SnapshotPruningStorerStub{
+			GetFromCurrentEpochCalled: func(key []byte) ([]byte, error) {
+				return []byte("invalid marker"), nil
+			},
+			MemDbMock: testscommon.NewMemDbMock(),
+		}
+		ts, _ := trie.NewTrieStorageManager(args)
+
+		assert.True(t, ts.ShouldTakeSnapshot())
+	})
 	t.Run("GetFromOldEpochsWithoutAddingToCacheCalled returns ActiveDBVal should return true", func(t *testing.T) {
 		t.Parallel()