Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add custom error for trie get node #4724

Merged
merged 37 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
088cc58
add custom error for trie get node
ssd04 Nov 21, 2022
e0dbd57
move get node from db custom error to common errors
ssd04 Nov 21, 2022
004264c
fix unit test on context closing
ssd04 Nov 21, 2022
0d6e8d6
fix unit tests in process sync
ssd04 Nov 21, 2022
0141588
add custom error cast check in error check function
ssd04 Nov 23, 2022
8b2d6e4
use new custom error in process unit tests
ssd04 Nov 23, 2022
8d6ea22
fix linter issue
ssd04 Nov 23, 2022
3bcb4a3
revert get node from db error additional check
ssd04 Nov 23, 2022
4462ced
use identifier to determine storage type
ssd04 Nov 23, 2022
6c5d073
add custom interface for get identifier
ssd04 Nov 23, 2022
d7e1b6b
fix comments and log messages
ssd04 Nov 23, 2022
ac87c63
fixes after review: comments fixes + renamings
ssd04 Nov 24, 2022
113c833
error from db renaming
ssd04 Dec 14, 2022
482aecb
renamed error with key in trie node
ssd04 Dec 15, 2022
737c93e
add interface for db error with key
ssd04 Dec 19, 2022
5f852a3
added todo for db identifier constants refactoring
ssd04 Dec 19, 2022
349d22a
refactor to use storer ids for dataRetriever
ssd04 Dec 22, 2022
3548ad0
fix and update unit tests for db identifier set
ssd04 Dec 22, 2022
bec60f1
fix factory processing unit tests
ssd04 Dec 23, 2022
ff5674d
refactor the set of accounts db identifier
ssd04 Dec 23, 2022
094f227
Merge pull request #4830 from ElrondNetwork/handle-duplicate-constant…
ssd04 Dec 23, 2022
0a17029
changed log warn to trace in get node from db
ssd04 Jan 9, 2023
73275b1
return err on CreateAndProcessMiniBlocks if getNodeFromDB err
BeniaminDrasovean Jan 12, 2023
a2deeff
small refactor
BeniaminDrasovean Jan 12, 2023
65e517f
return error if key not found
BeniaminDrasovean Feb 3, 2023
7dceb49
Merge branch 'feat/sync-missing-trie-nodes' into sync-missing-keys-only
BeniaminDrasovean Feb 3, 2023
a683564
fix after merge
BeniaminDrasovean Feb 3, 2023
36ddeb4
remove error wrapping from trie Get()
BeniaminDrasovean Feb 13, 2023
7123970
small logs refactor
BeniaminDrasovean Feb 14, 2023
a99d8d7
remove error wrapping when loading data trie
BeniaminDrasovean Mar 6, 2023
141ad52
Merge branch 'feat/sync-missing-trie-nodes' into sync-missing-keys-only
BeniaminDrasovean Mar 6, 2023
aea1014
add GetIdentifier to StorageManager interface
BeniaminDrasovean Mar 16, 2023
6d65587
add stack trace print if getNodeFromDb error
BeniaminDrasovean Mar 16, 2023
25dc1d4
refactor log prints
BeniaminDrasovean Mar 16, 2023
3e522d6
Merge branch 'feat/sync-missing-trie-nodes' into sync-missing-keys-only
BeniaminDrasovean Apr 3, 2023
90c6e21
move IsGetNodeFromDbErr() to core
BeniaminDrasovean Apr 3, 2023
2a46bcc
fix after review
BeniaminDrasovean Apr 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions common/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -834,3 +834,9 @@ const MetricTrieSyncNumReceivedBytes = "erd_trie_sync_num_bytes_received"

// MetricTrieSyncNumProcessedNodes is the metric that outputs the number of trie nodes processed for accounts during trie sync
const MetricTrieSyncNumProcessedNodes = "erd_trie_sync_num_nodes_processed"

// AccountsTrieIdentifier defines the identifier for accounts trie storer
const AccountsTrieIdentifier = "AccountsTrie"

// PeerAccountsTrieIdentifier defines the identifier for peer accounts storer
const PeerAccountsTrieIdentifier = "PeerAccountsTrie"
38 changes: 38 additions & 0 deletions errors/missingTrieNodeError.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package errors

import (
"encoding/hex"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go imports?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"fmt"
"strings"

"github.com/ElrondNetwork/elrond-go/common"
Expand All @@ -22,3 +24,39 @@ func IsGetNodeFromDBError(err error) bool {

return false
}

// GetNodeFromDBErr defines a custom error for trie get node
type GetNodeFromDBErr struct {
getErr error
key []byte
dbIdentifier string
}

// NewGetNodeFromDBErr will create a new instance of GetNodeFromDBErr
func NewGetNodeFromDBErr(key []byte, err error, id string) *GetNodeFromDBErr {
return &GetNodeFromDBErr{
getErr: err,
key: key,
dbIdentifier: id,
}
}

// Error returns the error as string
func (e *GetNodeFromDBErr) Error() string {
return fmt.Sprintf(
"%s: %s for key %v",
common.GetNodeFromDBErrorString,
e.getErr.Error(),
hex.EncodeToString(e.key),
)
}

// GetKey will return the key that generated the error
func (e *GetNodeFromDBErr) GetKey() []byte {
return e.key
}

// GetIdentifier will return the db corresponding to the db
BeniaminDrasovean marked this conversation as resolved.
Show resolved Hide resolved
func (e *GetNodeFromDBErr) GetIdentifier() string {
return e.dbIdentifier
}
9 changes: 2 additions & 7 deletions process/sync/baseSync.go
Original file line number Diff line number Diff line change
Expand Up @@ -693,14 +693,9 @@ func (boot *baseBootstrap) handleTrieSyncError(err error, ctx context.Context) {
}
}

func (boot *baseBootstrap) syncUserAccountsState() error {
rootHash, err := boot.accounts.RootHash()
if err != nil {
return err
}

func (boot *baseBootstrap) syncUserAccountsState(key []byte) error {
log.Warn("base sync: started syncUserAccountsState")
return boot.accountsDBSyncer.SyncAccounts(rootHash)
return boot.accountsDBSyncer.SyncAccounts(key)
}

func (boot *baseBootstrap) cleanNoncesSyncedWithErrorsBehindFinal() {
Expand Down
5 changes: 5 additions & 0 deletions process/sync/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,11 @@ func (boot *MetaBootstrap) GetNotarizedInfo(
}
}

// SyncAccountsDBs -
func (boot *MetaBootstrap) SyncAccountsDBs(key []byte, id string) error {
return boot.syncAccountsDBs(key, id)
}

// ProcessReceivedHeader -
func (boot *baseBootstrap) ProcessReceivedHeader(headerHandler data.HeaderHandler, headerHash []byte) {
boot.processReceivedHeader(headerHandler, headerHash)
Expand Down
39 changes: 18 additions & 21 deletions process/sync/metablock.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ package sync

import (
"context"
"fmt"

"github.com/ElrondNetwork/elrond-go-core/core"
"github.com/ElrondNetwork/elrond-go-core/core/check"
"github.com/ElrondNetwork/elrond-go-core/data"
"github.com/ElrondNetwork/elrond-go-core/data/block"
"github.com/ElrondNetwork/elrond-go/common"
"github.com/ElrondNetwork/elrond-go/dataRetriever"
"github.com/ElrondNetwork/elrond-go/errors"
"github.com/ElrondNetwork/elrond-go/process"
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added interface here

if !ok {
return err
}

errSync := boot.syncAccountsDBs(getNodeErr.GetKey(), getNodeErr.GetIdentifier())
boot.handleTrieSyncError(errSync, ctx)
}

return err
}

func (boot *MetaBootstrap) syncAccountsDBs() error {
var err error

err = boot.syncValidatorAccountsState()
if err != nil {
return err
func (boot *MetaBootstrap) syncAccountsDBs(key []byte, id string) error {
switch id {
iulianpascalau marked this conversation as resolved.
Show resolved Hide resolved
case common.AccountsTrieIdentifier:
return boot.syncUserAccountsState(key)
case common.PeerAccountsTrieIdentifier:
return boot.syncValidatorAccountsState(key)
default:
return fmt.Errorf("invalid trie identifier, id: %s", id)
}

err = boot.syncUserAccountsState()
if err != nil {
return err
}

return nil
}

func (boot *MetaBootstrap) syncValidatorAccountsState() error {
rootHash, err := boot.validatorAccountsDB.RootHash()
if err != nil {
return err
}

func (boot *MetaBootstrap) syncValidatorAccountsState(key []byte) error {
log.Warn("base sync: started syncValidatorAccountsState")
return boot.validatorStatisticsDBSyncer.SyncAccounts(rootHash)
return boot.validatorStatisticsDBSyncer.SyncAccounts(key)
}

// Close closes the synchronization loop
Expand Down
55 changes: 46 additions & 9 deletions process/sync/metablock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/ElrondNetwork/elrond-go/consensus/round"
"github.com/ElrondNetwork/elrond-go/dataRetriever"
"github.com/ElrondNetwork/elrond-go/dataRetriever/blockchain"
commonErrors "github.com/ElrondNetwork/elrond-go/errors"
"github.com/ElrondNetwork/elrond-go/process"
"github.com/ElrondNetwork/elrond-go/process/mock"
"github.com/ElrondNetwork/elrond-go/process/sync"
Expand Down Expand Up @@ -1621,7 +1622,7 @@ func TestMetaBootstrap_SyncBlockErrGetNodeDBShouldSyncAccounts(t *testing.T) {
}
args.ChainHandler = blkc

errGetNodeFromDB := errors.New(common.GetNodeFromDBErrorString)
errGetNodeFromDB := commonErrors.NewGetNodeFromDBErr([]byte("key"), errors.New("get error"), common.AccountsTrieIdentifier)
blockProcessor := createMetaBlockProcessor(args.ChainHandler)
blockProcessor.ProcessBlockCalled = func(header data.HeaderHandler, body data.BodyHandler, haveTime func() time.Duration) error {
return errGetNodeFromDB
Expand Down Expand Up @@ -1680,13 +1681,8 @@ func TestMetaBootstrap_SyncBlockErrGetNodeDBShouldSyncAccounts(t *testing.T) {
SyncAccountsCalled: func(rootHash []byte) error {
accountsSyncCalled = true
return nil
}}
validatorSyncCalled := false
args.ValidatorStatisticsDBSyncer = &mock.AccountsDBSyncerStub{
SyncAccountsCalled: func(rootHash []byte) error {
validatorSyncCalled = true
return nil
}}
},
}
args.Accounts = &stateMock.AccountsStub{RootHashCalled: func() ([]byte, error) {
return []byte("roothash"), nil
}}
Expand All @@ -1699,5 +1695,46 @@ func TestMetaBootstrap_SyncBlockErrGetNodeDBShouldSyncAccounts(t *testing.T) {

assert.Equal(t, errGetNodeFromDB, err)
assert.True(t, accountsSyncCalled)
assert.True(t, validatorSyncCalled)
}

func TestMetaBootstrap_SyncAccountsDBs(t *testing.T) {
t.Parallel()

t.Run("sync user accounts state", func(t *testing.T) {
t.Parallel()

args := CreateMetaBootstrapMockArguments()
accountsSyncCalled := false
args.AccountsDBSyncer = &mock.AccountsDBSyncerStub{
SyncAccountsCalled: func(rootHash []byte) error {
accountsSyncCalled = true
return nil
},
}

bs, _ := sync.NewMetaBootstrap(args)

err := bs.SyncAccountsDBs([]byte("key"), common.AccountsTrieIdentifier)
require.Nil(t, err)
require.True(t, accountsSyncCalled)
})

t.Run("sync validator accounts state", func(t *testing.T) {
t.Parallel()

args := CreateMetaBootstrapMockArguments()
accountsSyncCalled := false
args.ValidatorStatisticsDBSyncer = &mock.AccountsDBSyncerStub{
SyncAccountsCalled: func(rootHash []byte) error {
accountsSyncCalled = true
return nil
},
}

bs, _ := sync.NewMetaBootstrap(args)

err := bs.SyncAccountsDBs([]byte("key"), common.PeerAccountsTrieIdentifier)
require.Nil(t, err)
require.True(t, accountsSyncCalled)
})
}
7 changes: 6 additions & 1 deletion process/sync/shardblock.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,12 @@ func (boot *ShardBootstrap) StartSyncingBlocks() {
func (boot *ShardBootstrap) SyncBlock(ctx context.Context) error {
err := boot.syncBlock()
if errors.IsGetNodeFromDBError(err) {
BeniaminDrasovean marked this conversation as resolved.
Show resolved Hide resolved
errSync := boot.syncUserAccountsState()
getNodeErr, ok := err.(*errors.GetNodeFromDBErr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here, misleading names, please refactor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not done

if !ok {
return err
}

errSync := boot.syncUserAccountsState(getNodeErr.GetKey())
boot.handleTrieSyncError(errSync, ctx)
}

Expand Down
3 changes: 2 additions & 1 deletion process/sync/shardblock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/ElrondNetwork/elrond-go/consensus/round"
"github.com/ElrondNetwork/elrond-go/dataRetriever"
"github.com/ElrondNetwork/elrond-go/dataRetriever/blockchain"
commonErrors "github.com/ElrondNetwork/elrond-go/errors"
"github.com/ElrondNetwork/elrond-go/process"
"github.com/ElrondNetwork/elrond-go/process/mock"
"github.com/ElrondNetwork/elrond-go/process/sync"
Expand Down Expand Up @@ -2061,7 +2062,7 @@ func TestShardBootstrap_SyncBlockGetNodeDBErrorShouldSync(t *testing.T) {
}
args.ChainHandler = blkc

errGetNodeFromDB := errors.New(common.GetNodeFromDBErrorString)
errGetNodeFromDB := commonErrors.NewGetNodeFromDBErr([]byte("key"), errors.New("get error"), "")
blockProcessor := createBlockProcessor(args.ChainHandler)
blockProcessor.ProcessBlockCalled = func(header data.HeaderHandler, body data.BodyHandler, haveTime func() time.Duration) error {
return errGetNodeFromDB
Expand Down
5 changes: 5 additions & 0 deletions storage/pruning/pruningStorer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added todo and separate task for this

}

// IsInterfaceNil returns true if there is no value under the interface
func (ps *PruningStorer) IsInterfaceNil() bool {
return ps == nil
Expand Down
4 changes: 4 additions & 0 deletions trie/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,7 @@ type storageManagerExtension interface {
type StorageMarker interface {
MarkStorerAsSyncedAndActive(storer common.StorageManager)
}

type dbWriteCacherWithIdentifier interface {
GetIdentifier() string
}
10 changes: 7 additions & 3 deletions trie/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package trie

import (
"context"
"encoding/hex"
"fmt"
"time"

"github.com/ElrondNetwork/elrond-go-core/hashing"
Expand Down Expand Up @@ -119,8 +117,14 @@ func computeAndSetNodeHash(n node) ([]byte, error) {
func getNodeFromDBAndDecode(n []byte, db common.DBWriteCacher, marshalizer marshal.Marshalizer, hasher hashing.Hasher) (node, error) {
encChild, err := db.Get(n)
if err != nil {
dbWithID, ok := db.(dbWriteCacherWithIdentifier)
if !ok {
log.Warn("wrong type assertion on", common.GetNodeFromDBErrorString, "error", err, "key", n)
return nil, errors.NewGetNodeFromDBErr(n, err, "")
}

log.Trace(common.GetNodeFromDBErrorString, "error", err, "key", n)
return nil, fmt.Errorf(common.GetNodeFromDBErrorString+" %w for key %v", err, hex.EncodeToString(n))
return nil, errors.NewGetNodeFromDBErr(n, err, dbWithID.GetIdentifier())
}

return decodeNode(encChild, marshalizer, hasher)
Expand Down