From a08ea8a879632081f65950aad9b6fb96fc3a3c6b Mon Sep 17 00:00:00 2001 From: BeniaminDrasovean Date: Mon, 16 Dec 2024 15:03:02 +0200 Subject: [PATCH 1/7] add baseNode mutex --- trie/baseNode.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/trie/baseNode.go b/trie/baseNode.go index 20db3ea597..97df070d6d 100644 --- a/trie/baseNode.go +++ b/trie/baseNode.go @@ -16,8 +16,6 @@ type baseNode struct { } func (bn *baseNode) getHash() []byte { - //TODO add mutex protection for all methods - bn.mutex.RLock() defer bn.mutex.RUnlock() @@ -25,6 +23,9 @@ func (bn *baseNode) getHash() []byte { } func (bn *baseNode) setGivenHash(hash []byte) { + bn.mutex.Lock() + defer bn.mutex.Unlock() + bn.hash = hash } @@ -36,6 +37,9 @@ func (bn *baseNode) isDirty() bool { } func (bn *baseNode) setDirty(dirty bool) { + bn.mutex.Lock() + defer bn.mutex.Unlock() + bn.dirty = dirty } From 5a65927562932e8a3e25d4ba149fc83eb8135426 Mon Sep 17 00:00:00 2001 From: BeniaminDrasovean Date: Mon, 16 Dec 2024 16:40:51 +0200 Subject: [PATCH 2/7] create disabledGoroutinesManager --- trie/disabledGoroutinesManager.go | 50 ++++++++++++++++++++++++++ trie/disabledGoroutinesManager_test.go | 44 +++++++++++++++++++++++ trie/sync.go | 16 ++------- 3 files changed, 97 insertions(+), 13 deletions(-) create mode 100644 trie/disabledGoroutinesManager.go create mode 100644 trie/disabledGoroutinesManager_test.go diff --git a/trie/disabledGoroutinesManager.go b/trie/disabledGoroutinesManager.go new file mode 100644 index 0000000000..9cfce44906 --- /dev/null +++ b/trie/disabledGoroutinesManager.go @@ -0,0 +1,50 @@ +package trie + +import "github.com/multiversx/mx-chain-go/common" + +type disabledGoroutinesManager struct { + err error +} + +// NewDisabledGoroutinesManager creates a new instance of disabledGoroutinesManager +func NewDisabledGoroutinesManager() *disabledGoroutinesManager { + return &disabledGoroutinesManager{} +} + +// ShouldContinueProcessing returns true if there is no error +func (d *disabledGoroutinesManager) ShouldContinueProcessing() bool { + if d.err != nil { + return false + } + + return true +} + +// CanStartGoRoutine returns false +func (d *disabledGoroutinesManager) CanStartGoRoutine() bool { + return false +} + +// EndGoRoutineProcessing does nothing +func (d *disabledGoroutinesManager) EndGoRoutineProcessing() { +} + +// SetNewErrorChannel does nothing +func (d *disabledGoroutinesManager) SetNewErrorChannel(_ common.BufferedErrChan) error { + return nil +} + +// SetError sets the given error +func (d *disabledGoroutinesManager) SetError(err error) { + d.err = err +} + +// GetError returns the error +func (d *disabledGoroutinesManager) GetError() error { + return d.err +} + +// IsInterfaceNil returns true if there is no value under the interface +func (d *disabledGoroutinesManager) IsInterfaceNil() bool { + return d == nil +} diff --git a/trie/disabledGoroutinesManager_test.go b/trie/disabledGoroutinesManager_test.go new file mode 100644 index 0000000000..c82c66ebca --- /dev/null +++ b/trie/disabledGoroutinesManager_test.go @@ -0,0 +1,44 @@ +package trie + +import ( + "errors" + "testing" + + "github.com/multiversx/mx-chain-core-go/core/check" + "github.com/stretchr/testify/assert" +) + +func TestNewDisabledGoroutinesManager(t *testing.T) { + t.Parallel() + + d := NewDisabledGoroutinesManager() + assert.False(t, check.IfNil(d)) +} + +func TestDisabledGoroutinesManager_ShouldContinueProcessing(t *testing.T) { + t.Parallel() + + d := NewDisabledGoroutinesManager() + assert.True(t, d.ShouldContinueProcessing()) + + d.SetError(errors.New("error")) + assert.False(t, d.ShouldContinueProcessing()) +} + +func TestDisabledGoroutinesManager_CanStartGoRoutine(t *testing.T) { + t.Parallel() + + d := NewDisabledGoroutinesManager() + assert.False(t, d.CanStartGoRoutine()) +} + +func TestDisabledGoroutinesManager_SetAndGetError(t *testing.T) { + t.Parallel() + + d := NewDisabledGoroutinesManager() + assert.Nil(t, d.GetError()) + + err := errors.New("error") + d.SetError(err) + assert.Equal(t, err, d.GetError()) +} diff --git a/trie/sync.go b/trie/sync.go index 6f498e58c0..0e362c87c4 100644 --- a/trie/sync.go +++ b/trie/sync.go @@ -4,8 +4,6 @@ import ( "bytes" "context" "fmt" - "github.com/multiversx/mx-chain-core-go/core/throttler" - "github.com/multiversx/mx-chain-go/common/errChan" "sync" "time" @@ -359,17 +357,9 @@ func trieNode( return nil, err } - th, err := throttler.NewNumGoRoutinesThrottler(1) - if err != nil { - return nil, err - } - goRoutinesManager, err := NewGoroutinesManager(th, errChan.NewErrChanWrapper(), make(chan struct{})) - if err != nil { - return nil, err - } - - decodedNode.setHash(goRoutinesManager) - err = goRoutinesManager.GetError() + manager := NewDisabledGoroutinesManager() + decodedNode.setHash(manager) + err = manager.GetError() if err != nil { return nil, err } From 0e4014e7f38811fbe4ecd52801e2d0b31cdd1b4e Mon Sep 17 00:00:00 2001 From: BeniaminDrasovean Date: Wed, 18 Dec 2024 16:32:54 +0200 Subject: [PATCH 3/7] fix linter issues --- trie/disabledGoroutinesManager.go | 6 +----- trie/node.go | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/trie/disabledGoroutinesManager.go b/trie/disabledGoroutinesManager.go index 9cfce44906..4133c5fb8e 100644 --- a/trie/disabledGoroutinesManager.go +++ b/trie/disabledGoroutinesManager.go @@ -13,11 +13,7 @@ func NewDisabledGoroutinesManager() *disabledGoroutinesManager { // ShouldContinueProcessing returns true if there is no error func (d *disabledGoroutinesManager) ShouldContinueProcessing() bool { - if d.err != nil { - return false - } - - return true + return d.err != nil } // CanStartGoRoutine returns false diff --git a/trie/node.go b/trie/node.go index a8d9b3b64e..448bb42237 100644 --- a/trie/node.go +++ b/trie/node.go @@ -125,11 +125,7 @@ func concat(s1 []byte, s2 ...byte) []byte { func hasValidHash(n node) bool { childHash := n.getHash() - if childHash == nil { - return false - } - - return true + return len(childHash) != 0 } func decodeNode(encNode []byte, marshalizer marshal.Marshalizer, hasher hashing.Hasher) (node, error) { From 381ecec5ae67a5b1a1c612c313e910697f49fbd7 Mon Sep 17 00:00:00 2001 From: BeniaminDrasovean Date: Wed, 18 Dec 2024 16:52:14 +0200 Subject: [PATCH 4/7] small fix --- trie/disabledGoroutinesManager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trie/disabledGoroutinesManager.go b/trie/disabledGoroutinesManager.go index 4133c5fb8e..e5be63e7f9 100644 --- a/trie/disabledGoroutinesManager.go +++ b/trie/disabledGoroutinesManager.go @@ -13,7 +13,7 @@ func NewDisabledGoroutinesManager() *disabledGoroutinesManager { // ShouldContinueProcessing returns true if there is no error func (d *disabledGoroutinesManager) ShouldContinueProcessing() bool { - return d.err != nil + return d.err == nil } // CanStartGoRoutine returns false From 0ca88fbad64c671fa019d2ef48c5f4b8a2a769e9 Mon Sep 17 00:00:00 2001 From: BeniaminDrasovean Date: Thu, 19 Dec 2024 15:35:26 +0200 Subject: [PATCH 5/7] add setHash concurrency unit tests --- trie/patriciaMerkleTrie_test.go | 122 ++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/trie/patriciaMerkleTrie_test.go b/trie/patriciaMerkleTrie_test.go index 31c9d05957..bba864c20d 100644 --- a/trie/patriciaMerkleTrie_test.go +++ b/trie/patriciaMerkleTrie_test.go @@ -1598,6 +1598,9 @@ func TestPatriciaMerkleTrie_AddBatchedDataToTrie(t *testing.T) { time.Sleep(time.Millisecond * 100) } }, + SetErrorCalled: func(err error) { + assert.Fail(t, "should not have called this function") + }, } trie.SetGoRoutinesManager(tr, grm) @@ -1663,6 +1666,9 @@ func TestPatriciaMerkleTrie_AddBatchedDataToTrie(t *testing.T) { time.Sleep(time.Millisecond * 100) } }, + SetErrorCalled: func(err error) { + assert.Fail(t, "should not have called this function") + }, } trie.SetGoRoutinesManager(tr, grm) @@ -1828,6 +1834,9 @@ func TestPatriciaMerkleTrie_Get(t *testing.T) { time.Sleep(time.Millisecond * 100) } }, + SetErrorCalled: func(err error) { + assert.Fail(t, "should not have called this function") + }, } trie.SetGoRoutinesManager(tr, grm) @@ -1860,6 +1869,119 @@ func TestPatriciaMerkleTrie_Get(t *testing.T) { }) } +func TestPatriciaMerkleTrie_RootHash(t *testing.T) { + t.Parallel() + + t.Run("set root hash with batched data commits batch", func(t *testing.T) { + t.Parallel() + + tr := emptyTrie() + numOperations := 1000 + for i := 0; i < numOperations; i++ { + _ = tr.Update([]byte("dog"+strconv.Itoa(i)), []byte("reindeer")) + } + + rootHash, err := tr.RootHash() + assert.Nil(t, err) + assert.NotEqual(t, emptyTrieHash, rootHash) + }) + t.Run("set root hash and update trie concurrently should serialize operations", func(t *testing.T) { + t.Parallel() + + // create trie with some data + tr := emptyTrie() + numOperations := 1000 + for i := 0; i < numOperations; i++ { + _ = tr.Update([]byte("dog"+strconv.Itoa(i)), []byte("reindeer")) + } + trie.ExecuteUpdatesFromBatch(tr) + + // compute rootHash + waitForSignal := atomic.Bool{} + waitForSignal.Store(true) + startedComputingRootHash := atomic.Bool{} + grm := &mock.GoroutinesManagerStub{ + CanStartGoRoutineCalled: func() bool { + startedComputingRootHash.Store(true) + return true + }, + EndGoRoutineProcessingCalled: func() { + for waitForSignal.Load() { + time.Sleep(time.Millisecond * 100) + } + }, + SetErrorCalled: func(err error) { + assert.Fail(t, "should not have called this function") + }, + } + trie.SetGoRoutinesManager(tr, grm) + + go func() { + rootHash1, err := tr.RootHash() + assert.Nil(t, err) + assert.NotEqual(t, emptyTrieHash, rootHash1) + }() + + // wait for start of the computation of the root hash + for !startedComputingRootHash.Load() { + time.Sleep(time.Millisecond * 100) + } + + for i := numOperations; i < numOperations*2; i++ { + _ = tr.Update([]byte("dog"+strconv.Itoa(i)), []byte("reindeer")) + } + setNewErrChanCalled := atomic.Bool{} + wg := sync.WaitGroup{} + wg.Add(1) + go func() { + grm.SetNewErrorChannelCalled = func(common.BufferedErrChan) error { + setNewErrChanCalled.Store(true) + return nil + } + trie.ExecuteUpdatesFromBatch(tr) + wg.Done() + }() + + // commit batch to trie does not start until root hash is fully computed + time.Sleep(time.Millisecond * 500) + assert.False(t, setNewErrChanCalled.Load()) + + waitForSignal.Store(false) + wg.Wait() + assert.True(t, setNewErrChanCalled.Load()) + }) + t.Run("set root hash and get from trie concurrently", func(t *testing.T) { + t.Parallel() + + tr := emptyTrie() + numOperations := 100000 + for i := 0; i < numOperations; i++ { + _ = tr.Update([]byte("dog"+strconv.Itoa(i)), []byte("reindeer")) + } + trie.ExecuteUpdatesFromBatch(tr) + + wg := sync.WaitGroup{} + wg.Add(1) + setRootHashFinished := atomic.Bool{} + go func() { + for !setRootHashFinished.Load() { + index := rand.Intn(numOperations) + val, _, err := tr.Get([]byte("dog" + strconv.Itoa(index))) + assert.Nil(t, err) + assert.Equal(t, []byte("reindeer"), val) + } + wg.Done() + }() + + rootHash, err := tr.RootHash() + assert.Nil(t, err) + assert.NotEqual(t, emptyTrieHash, rootHash) + + setRootHashFinished.Store(true) + wg.Wait() + }) +} + func TestPatricianMerkleTrie_ConcurrentOperations(t *testing.T) { t.Parallel() From 4ac8de43e8f3a56bd4f2a9fbf0cf41ce8de5ce77 Mon Sep 17 00:00:00 2001 From: BeniaminDrasovean Date: Wed, 8 Jan 2025 11:55:04 +0200 Subject: [PATCH 6/7] fix after review --- trie/patriciaMerkleTrie.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/trie/patriciaMerkleTrie.go b/trie/patriciaMerkleTrie.go index d2e6f80d7e..59b2c33ba2 100644 --- a/trie/patriciaMerkleTrie.go +++ b/trie/patriciaMerkleTrie.go @@ -683,19 +683,15 @@ func logMapWithTrace(message string, paramName string, hashes common.ModifiedHas // GetProof computes a Merkle proof for the node that is present at the given key func (tr *patriciaMerkleTrie) GetProof(key []byte, rootHash []byte) ([][]byte, []byte, error) { - trie, err := tr.Recreate(rootHash) - if err != nil { - return nil, nil, err - } - - pmt, ok := trie.(*patriciaMerkleTrie) - if !ok { - return nil, nil, ErrWrongTypeAssertion + //TODO refactor this function to avoid encoding the node after it is retrieved from the DB. + // The encoded node is actually the value from db, thus we can use the retrieved value directly + if len(key) == 0 || bytes.Equal(rootHash, common.EmptyTrieHash) { + return nil, nil, ErrNilNode } - rootNode := pmt.GetRootNode() - if check.IfNil(rootNode) { - return nil, nil, ErrNilNode + rootNode, err := getNodeFromDBAndDecode(rootHash, tr.trieStorage, tr.marshalizer, tr.hasher) + if err != nil { + return nil, nil, fmt.Errorf("trie get proof error: %w", err) } var proof [][]byte From 7620cd9482fd67b46b7c0bd4412bc24d74b794bc Mon Sep 17 00:00:00 2001 From: BeniaminDrasovean Date: Mon, 13 Jan 2025 14:34:16 +0200 Subject: [PATCH 7/7] run goimports for some files --- trie/extensionNode.go | 5 +++-- trie/leafNode.go | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/trie/extensionNode.go b/trie/extensionNode.go index 8dc4472441..dcd3743c4e 100644 --- a/trie/extensionNode.go +++ b/trie/extensionNode.go @@ -5,14 +5,15 @@ import ( "context" "encoding/hex" "fmt" + "io" + "math" + "github.com/multiversx/mx-chain-core-go/core" "github.com/multiversx/mx-chain-core-go/core/check" "github.com/multiversx/mx-chain-core-go/hashing" "github.com/multiversx/mx-chain-core-go/marshal" "github.com/multiversx/mx-chain-go/common" vmcommon "github.com/multiversx/mx-chain-vm-common-go" - "io" - "math" ) var _ = node(&extensionNode{}) diff --git a/trie/leafNode.go b/trie/leafNode.go index f7d0243dda..13f04b9f16 100644 --- a/trie/leafNode.go +++ b/trie/leafNode.go @@ -5,6 +5,9 @@ import ( "context" "encoding/hex" "fmt" + "io" + "math" + "github.com/multiversx/mx-chain-core-go/core" "github.com/multiversx/mx-chain-core-go/core/check" "github.com/multiversx/mx-chain-core-go/core/keyValStorage" @@ -12,8 +15,6 @@ import ( "github.com/multiversx/mx-chain-core-go/marshal" "github.com/multiversx/mx-chain-go/common" vmcommon "github.com/multiversx/mx-chain-vm-common-go" - "io" - "math" ) var _ = node(&leafNode{})