add custom error for trie get node #4724

ssd04 · 2022-11-21T20:43:28Z

Reasoning behind the pull request

When syncing the trie is synced based on root hash, it would be better to sync based only on the missing nodes

Proposed changes

Handle get nodes from db error in order to pass also the missing node hash

Testing procedure

System test with scenario:
- with #missing-nodes-sed
- AccountsStatePruningEnabled and PeerStatePruningEnabled on false

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

codecov-commenter · 2022-11-21T21:35:20Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/sync-missing-trie-nodes@a729637). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 3666928 differs from pull request most recent head 2a46bcc. Consider uploading reports for the commit 2a46bcc to get more accurate results

Additional details and impacted files

@@                       Coverage Diff                       @@
##             feat/sync-missing-trie-nodes    #4724   +/-   ##
===============================================================
  Coverage                                ?   70.86%           
===============================================================
  Files                                   ?      674           
  Lines                                   ?    87714           
  Branches                                ?        0           
===============================================================
  Hits                                    ?    62161           
  Misses                                  ?    20875           
  Partials                                ?     4678

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

process/sync/shardblock.go

errors/missingTrieNodeError.go

storage/pruning/triePruningStorer.go

trie/interface.go

trie/node.go

errors/missingTrieNodeError.go

iulianpascalau · 2022-12-13T06:09:16Z

process/sync/metablock.go

@@ -180,37 +182,32 @@ func (boot *MetaBootstrap) setLastEpochStartRound() {
 func (boot *MetaBootstrap) SyncBlock(ctx context.Context) error {
 	err := boot.syncBlock()
 	if errors.IsGetNodeFromDBError(err) {
-		errSync := boot.syncAccountsDBs()
+		getNodeErr, ok := err.(*errors.GetNodeFromDBErr)


Misleading names. We have the if errors.IsGetNodeFromDBError(err) { instruction and then we can not cast it to a *errors.GetNodeFromDBErr ? Let's refactor this.

not done. We have the same function and the same cast. Please refactor.

added interface here

process/sync/metablock.go

iulianpascalau · 2022-12-13T06:16:34Z

process/sync/shardblock.go

@@ -144,7 +144,12 @@ func (boot *ShardBootstrap) StartSyncingBlocks() {
 func (boot *ShardBootstrap) SyncBlock(ctx context.Context) error {
 	err := boot.syncBlock()
 	if errors.IsGetNodeFromDBError(err) {
-		errSync := boot.syncUserAccountsState()
+		getNodeErr, ok := err.(*errors.GetNodeFromDBErr)


also here, misleading names, please refactor

iulianpascalau · 2022-12-13T06:20:40Z

storage/pruning/pruningStorer.go

@@ -1033,6 +1033,11 @@ func (ps *PruningStorer) RangeKeys(_ func(key []byte, val []byte) bool) {
 	debug.PrintStack()
 }

+// GetIdentifier returns the identifier for storer
+func (ps *PruningStorer) GetIdentifier() string {
+	return ps.identifier


I think that if we somehow, change the paths and names for the accounts storers, the whole code will break. We need to find a proper way to use storers identifiers in this case. Perhaps using the trie containers implementation as seen in factory/state/stateComponents.go L151-L178?
This happens because we have 2 sets of constants declared: one in trie/trieFactory/trieFactoryArgs.go and the other one in dataRetriever/unitType.go

from what i've tested, it still seems to work, will think more on the refactoring here

Works for now because the strings from both constant sets are the same. Will need to refactor this somehow. You can refactor now or add a TODO + task.

added todo and separate task for this

…s-for-storage-mappers refactor to use storer ids for dataRetriever

# Conflicts: # dataRetriever/factory/resolverscontainer/metaResolversContainerFactory.go # dataRetriever/factory/resolverscontainer/metaResolversContainerFactory_test.go # dataRetriever/factory/resolverscontainer/shardResolversContainerFactory.go # dataRetriever/factory/resolverscontainer/shardResolversContainerFactory_test.go # factory/consensus/consensusComponents.go # factory/processing/blockProcessorCreator_test.go # genesis/process/genesisBlockCreator.go # genesis/process/genesisBlockCreator_test.go # integrationTests/state/stateTrie/stateTrie_test.go # integrationTests/state/stateTrieSync/stateTrieSync_test.go # integrationTests/testProcessorNode.go # node/nodeRunner.go # process/block/shardblock.go # process/coordinator/process.go # process/sync/baseSync.go # process/sync/metablock_test.go # process/sync/shardblock_test.go # testscommon/components/components.go # testscommon/components/default.go # update/genesis/import.go # update/genesis/import_test.go

# Conflicts: # testscommon/components/default.go

iulianpascalau · 2023-04-10T07:20:22Z

go.mod

@@ -13,13 +13,13 @@ require (
 	github.com/google/gops v0.3.18
 	github.com/gorilla/websocket v1.5.0
 	github.com/mitchellh/mapstructure v1.5.0
-	github.com/multiversx/mx-chain-core-go v1.2.0
+	github.com/multiversx/mx-chain-core-go v1.2.1-0.20230403113932-916b16d18978


proper releases

This will be merged in a feature branch. Will create a release tag when the feature branch will be merged.

iulianpascalau · 2023-04-10T07:28:43Z

process/coordinator/process.go

@@ -719,6 +719,11 @@ func (tc *transactionCoordinator) CreateMbsAndProcessCrossShardTransactionsDstMe
 				"total gas penalized", tc.gasHandler.TotalGasPenalized(),
 				"error", errProc,
 			)
+
+			if core.IsGetNodeFromDBError(errProc) {
+				return nil, 0, false, err


Added. Also, return errProc instead of err

Actually, removed this check. As discussed, it is not needed here.

iulianpascalau · 2023-04-10T07:28:53Z

process/block/shardblock.go

@@ -2026,6 +2026,10 @@ func (sp *shardProcessor) createMiniBlocks(haveTime func() bool, randomness []by
 	log.Debug("elapsed time to create mbs to me", "time", elapsedTime)
 	if err != nil {
 		log.Debug("createAndProcessCrossMiniBlocksDstMe", "error", err.Error())
+
+		if core.IsGetNodeFromDBError(err) {
+			return nil, nil, err


Removed the check, not needed here.

iulianpascalau · 2023-04-10T07:30:03Z

state/accountsDB.go

@@ -524,7 +523,8 @@ func (adb *AccountsDB) loadDataTrie(accountHandler baseAccountHandler, mainTrie

 	dataTrie, err := mainTrie.Recreate(accountHandler.GetRootHash())
 	if err != nil {
-		return fmt.Errorf("trie was not found for hash, rootHash = %s, err = %w", hex.EncodeToString(accountHandler.GetRootHash()), err)
+		log.Error("trie was not found for hash", "rootHash", accountHandler.GetRootHash(), "err", err)


log error & return error at the same time? Maybe an error wrap will help?

Reverted the changes, the err is now wrapped before returning. Removed the log print.

iulianpascalau · 2023-04-10T07:31:49Z

trie/node.go

 		dbWithID, ok := db.(dbWriteCacherWithIdentifier)
 		if !ok {
-			log.Trace(common.GetNodeFromDBErrorString, "error", err, "key", n, "db type", fmt.Sprintf("%T", db))
+			log.Warn("db does not have an identifier", "db type", fmt.Sprintf("%T", db))


log warn while returning the error. Why not wrap?

Maybe use log trace if you already return this error in the caller side as well.

Wrapped the err.

iulianpascalau · 2023-04-10T07:32:26Z

trie/patriciaMerkleTrie.go

@@ -88,7 +88,7 @@ func (tr *patriciaMerkleTrie) Get(key []byte) ([]byte, uint32, error) {

 	val, depth, err := tr.root.tryGet(hexKey, rootDepthLevel, tr.trieStorage)
 	if err != nil {
-		err = fmt.Errorf("trie get error: %w, for key %v", err, hex.EncodeToString(key))
+		log.Error("trie get error", "error", err.Error(), "key", key)


same here, log error while returning. Why not wrap?

The same here

Reverted the changes, use wrapped err.

iulianpascalau · 2023-04-10T07:32:48Z

trie/trieStorageManager.go

@@ -687,6 +687,17 @@ func (tsm *trieStorageManager) GetBaseTrieStorageManager() common.StorageManager
 	return tsm
 }

+// GetIdentifier returns the identifier of the main storer
+func (tsm *trieStorageManager) GetIdentifier() string {
+	dbWithIdentifier, ok := tsm.mainStorer.(dbWriteCacherWithIdentifier)


errors/missingTrieNodeError.go

SebastianMarian · 2023-04-10T13:38:07Z

errors/missingTrieNodeError.go

@@ -1,24 +1,43 @@
 package errors

 import (
-	"strings"
-
+	"encoding/hex"


go imports?

SebastianMarian · 2023-04-10T13:39:17Z

errors/missingTrieNodeError.go


-	if IsClosingError(err) {
-		return false
+// NewGetNodeFromDBErrWithKey will create a new instance of GetNodeFromDBErr


GetNodeFromDBErrWithKey?

SebastianMarian · 2023-04-10T13:56:09Z

process/block/preprocess/transactions.go

@@ -1090,6 +1089,11 @@ func (txs *transactions) CreateAndProcessMiniBlocks(haveTime func() bool, random

 	if err != nil {
 		log.Debug("createAndProcessMiniBlocksFromMe", "error", err.Error())
+
+		if core.IsGetNodeFromDBError(err) {


Will have this change the desired effect? In the caller side, func (tc *transactionCoordinator) CreateMbsAndProcessTransactionsFromMe, there is only a print when this method returns error. I think that the behavior will remain the same.

Right, removed.

SebastianMarian · 2023-04-10T14:41:38Z

process/coordinator/process.go

@@ -719,6 +719,11 @@ func (tc *transactionCoordinator) CreateMbsAndProcessCrossShardTransactionsDstMe
 				"total gas penalized", tc.gasHandler.TotalGasPenalized(),
 				"error", errProc,
 			)
+
+			if core.IsGetNodeFromDBError(errProc) {
+				return nil, 0, false, err


If we want that the execution of miniblocks dest me to continue with other mini blocks (from other shards) we need only to break here. The return will be in format return miniblocks, numTxsAdded, false, nil. Otherwise all the execution dest me will be skipped. Which is the real intention here?

Removed, as discussed.

SebastianMarian · 2023-04-10T15:37:00Z

trie/node.go

 		dbWithID, ok := db.(dbWriteCacherWithIdentifier)
 		if !ok {
-			log.Trace(common.GetNodeFromDBErrorString, "error", err, "key", n, "db type", fmt.Sprintf("%T", db))
+			log.Warn("db does not have an identifier", "db type", fmt.Sprintf("%T", db))


Maybe use log trace if you already return this error in the caller side as well.

SebastianMarian · 2023-04-10T15:38:00Z

trie/patriciaMerkleTrie.go

@@ -88,7 +88,7 @@ func (tr *patriciaMerkleTrie) Get(key []byte) ([]byte, uint32, error) {

 	val, depth, err := tr.root.tryGet(hexKey, rootDepthLevel, tr.trieStorage)
 	if err != nil {
-		err = fmt.Errorf("trie get error: %w, for key %v", err, hex.EncodeToString(key))
+		log.Error("trie get error", "error", err.Error(), "key", key)


The same here

SebastianMarian · 2023-04-10T15:40:45Z

update/genesis/import.go

@@ -5,6 +5,7 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
+	"github.com/multiversx/mx-chain-go/dataRetriever"


SebastianMarian · 2023-04-10T15:41:19Z

update/genesis/import.go

@@ -292,9 +292,9 @@ func (si *stateImport) getTrie(shardID uint32, accType Type) (common.Trie, error
 		return trieForShard, nil
 	}

-	trieStorageManager := si.trieStorageManagers[triesFactory.UserAccountTrie]
+	trieStorageManager := si.trieStorageManagers[dataRetriever.PeerAccountsUnit.String()]


UserAccountsUnit?

Yes. Changed.

iulianpascalau · 2023-04-19T09:25:21Z

process/sync/shardblock_test.go

@@ -2156,3 +2156,23 @@ func TestShardBootstrap_NilInnerBootstrapperClose(t *testing.T) {
 	bootstrapper := &sync.ShardBootstrap{}
 	assert.Nil(t, bootstrapper.Close())
 }
+
+func TestUnwrapGetNodeFromDBErr(t *testing.T) {


add custom error for trie get node

088cc58

ssd04 self-assigned this Nov 21, 2022

ssd04 added 3 commits November 21, 2022 22:58

move get node from db custom error to common errors

e0dbd57

fix unit test on context closing

004264c

fix unit tests in process sync

0d6e8d6

BeniaminDrasovean approved these changes Nov 22, 2022

View reviewed changes

BeniaminDrasovean requested changes Nov 23, 2022

View reviewed changes

process/sync/shardblock.go Outdated Show resolved Hide resolved

ssd04 added 7 commits November 23, 2022 10:00

add custom error cast check in error check function

0141588

use new custom error in process unit tests

8b2d6e4

fix linter issue

8d6ea22

revert get node from db error additional check

3bcb4a3

use identifier to determine storage type

4462ced

add custom interface for get identifier

6c5d073

fix comments and log messages

d7e1b6b

BeniaminDrasovean reviewed Nov 23, 2022

View reviewed changes

fixes after review: comments fixes + renamings

ac87c63

BeniaminDrasovean reviewed Dec 6, 2022

View reviewed changes

errors/missingTrieNodeError.go Outdated Show resolved Hide resolved

ssd04 marked this pull request as ready for review December 7, 2022 09:19

iulianpascalau self-requested a review December 7, 2022 09:56

iulianpascalau requested changes Dec 13, 2022

View reviewed changes

ssd04 added 4 commits December 14, 2022 12:21

error from db renaming

113c833

renamed error with key in trie node

482aecb

add interface for db error with key

737c93e

added todo for db identifier constants refactoring

5f852a3

BeniaminDrasovean previously approved these changes Dec 20, 2022

View reviewed changes

ssd04 added 4 commits December 22, 2022 14:05

refactor to use storer ids for dataRetriever

349d22a

fix and update unit tests for db identifier set

3548ad0

fix factory processing unit tests

bec60f1

refactor the set of accounts db identifier

ff5674d

Merge pull request #4830 from ElrondNetwork/handle-duplicate-constant…

094f227

…s-for-storage-mappers refactor to use storer ids for dataRetriever

ssd04 dismissed BeniaminDrasovean’s stale review via 094f227 December 23, 2022 15:42

iulianpascalau previously approved these changes Dec 28, 2022

View reviewed changes

changed log warn to trace in get node from db

0a17029

ssd04 dismissed iulianpascalau’s stale review via 0a17029 January 9, 2023 09:00

BeniaminDrasovean added 2 commits January 12, 2023 10:27

return err on CreateAndProcessMiniBlocks if getNodeFromDB err

73275b1

small refactor

a2deeff

iulianpascalau previously approved these changes Jan 20, 2023

View reviewed changes

return error if key not found

65e517f

BeniaminDrasovean dismissed iulianpascalau’s stale review via 65e517f February 3, 2023 10:14

BeniaminDrasovean added 11 commits February 3, 2023 13:54

fix after merge

a683564

remove error wrapping from trie Get()

36ddeb4

small logs refactor

7123970

remove error wrapping when loading data trie

a99d8d7

Merge branch 'feat/sync-missing-trie-nodes' into sync-missing-keys-only

141ad52

# Conflicts: # testscommon/components/default.go

add GetIdentifier to StorageManager interface

aea1014

add stack trace print if getNodeFromDb error

6d65587

refactor log prints

25dc1d4

Merge branch 'feat/sync-missing-trie-nodes' into sync-missing-keys-only

3e522d6

move IsGetNodeFromDbErr() to core

90c6e21

iulianpascalau reviewed Apr 10, 2023

View reviewed changes

SebastianMarian reviewed Apr 10, 2023

View reviewed changes

fix after review

2a46bcc

iulianpascalau approved these changes Apr 19, 2023

View reviewed changes

SebastianMarian approved these changes Apr 19, 2023

View reviewed changes

BeniaminDrasovean merged commit da1e15e into feat/sync-missing-trie-nodes Apr 21, 2023

BeniaminDrasovean deleted the sync-missing-keys-only branch April 21, 2023 08:06

add custom error for trie get node #4724

add custom error for trie get node #4724

Conversation

ssd04 commented Nov 21, 2022 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov-commenter commented Nov 21, 2022 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeniaminDrasovean Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssd04 commented Nov 21, 2022 •

edited

Loading

codecov-commenter commented Nov 21, 2022 •

edited by codecov bot

Loading

BeniaminDrasovean Apr 18, 2023 •

edited

Loading