Snapshot in latest storer fix #5381

BeniaminDrasovean · 2023-06-28T08:47:06Z

Reasoning behind the pull request

In some edgecases, the state snapshot would start before the storers could change the epoch.

Proposed changes

When starting a snapshot, wait for the storer to change to the new epoch, and only then continue with the snapshot process.

Testing procedure

Normal testing procedure

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 · 2023-06-28T09:02:01Z

Codecov Report

Patch coverage: 94.17% and no project coverage change.

Comparison is base (cf4fe2e) 80.09% compared to head (02d6610) 80.10%.
Report is 1 commits behind head on rc/v1.6.0.

❗ Current head 02d6610 differs from pull request most recent head 13e319a. Consider uploading reports for the commit 13e319a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           rc/v1.6.0    #5381   +/-   ##
==========================================
  Coverage      80.09%   80.10%           
==========================================
  Files            706      708    +2     
  Lines          93355    93408   +53     
==========================================
+ Hits           74770    74821   +51     
- Misses         13269    13275    +6     
+ Partials        5316     5312    -4

Files Changed	Coverage Δ
process/block/metablock.go	`61.99% <0.00%> (ø)`
process/block/shardblock.go	`68.47% <0.00%> (-0.05%)`	⬇️
state/errors.go	`25.00% <ø> (ø)`
state/stateMetrics/stateMetrics.go	`85.18% <85.18%> (ø)`
state/peerAccountsDB.go	`96.00% <90.90%> (-4.00%)`	⬇️
state/snapshotsManager.go	`96.55% <96.55%> (ø)`
...ocess/transactionEvaluator/simulationAccountsDB.go	`61.62% <100.00%> (ø)`
state/accountsDB.go	`77.62% <100.00%> (-3.43%)`	⬇️
state/accountsDBApi.go	`91.45% <100.00%> (ø)`
state/accountsDBApiWithHistory.go	`89.21% <100.00%> (ø)`
... and 1 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

iulianpascalau · 2023-06-28T11:04:07Z

state/accountsDB.go

+		}
+
+		select {
+		case <-time.After(args.WaitTimeForSnapshotEpochCheck):


we should use time.Timer

iulianpascalau · 2023-06-28T11:08:06Z

state/accountsDB.go

+			return nil
+		}
+
+		if storageManager.IsClosed() {


I think this will be better moved on L1167, immediately after getting the storageManager pointer

iulianpascalau · 2023-06-28T11:10:04Z

process/block/metablock.go

@@ -1442,8 +1442,8 @@ func (mp *metaProcessor) updateState(lastMetaBlock data.MetaHeaderHandler, lastM
 			"rootHash", lastMetaBlock.GetRootHash(),
 			"prevRootHash", prevMetaBlock.GetRootHash(),
 			"validatorStatsRootHash", lastMetaBlock.GetValidatorStatsRootHash())
-		mp.accountsDB[state.UserAccountsState].SnapshotState(lastMetaBlock.GetRootHash())
-		mp.accountsDB[state.PeerAccountsState].SnapshotState(lastMetaBlock.GetValidatorStatsRootHash())
+		go mp.accountsDB[state.UserAccountsState].SnapshotState(lastMetaBlock.GetRootHash(), lastMetaBlock.GetEpoch())


this won't work in import-db mode. Please remove the 2 go calls + go call on shardblock.go L1261.
Let's refactor the solution.

Right, refactored.

…st-storer-fix # Conflicts: # process/transactionEvaluator/simulationAccountsDB_test.go # process/txsimulator/wrappedAccountsDB.go # state/accountsDB.go # state/export_test.go # state/interface.go

ssd04 · 2023-08-07T10:39:02Z

state/snapshotsManager.go

+	mutex                sync.RWMutex
+}
+
+// 	NewSnapshotsManager creates a new snapshots manager


Suggested change

// NewSnapshotsManager creates a new snapshots manager

// NewSnapshotsManager creates a new snapshots manager

ssd04 · 2023-08-07T10:45:02Z

state/accountsDB.go

+func (adb *AccountsDB) waitForStorageEpochChange(args StorageEpochChangeWaitArgs) error {
+	if args.SnapshotWaitTimeout < args.WaitTimeForSnapshotEpochCheck {


this func does not seem to be used, i think it was actually moved to snapshotsManager

Indeed, it was moved. Removed this and moved the tests.

ssd04 · 2023-08-07T10:47:00Z

state/snapshotsManager.go

+// StorageEpochChangeWaitArgs are the args needed for calling the WaitForStorageEpochChange function
+type StorageEpochChangeWaitArgs struct {


if waitForStorageEpochChange will be removed from accountsDB as it seems that it's not being used, i think we can unexport this struct

ssd04 · 2023-08-07T12:07:24Z

state/snapshotsManager.go

+	rootHash, epoch, err := sm.getSnapshotRootHashAndEpoch(trieStorageManager)
+	if err != nil {
+		log.Debug("could not retrieve snapshot info", "error", err)
+		return nil


so we are not starting the snapshot if there is an error getting the values here?

should we also consider import db processing mode, when starting snapshot after restart?

Yes, we do not start the snapshot because we do not know which rootHash to snapshot and for which epoch. When the node will reach the next epoch, it will start the snapshot for that epoch.
Regarding import-db, the process will be the same. If the import-db is stoped when a snapshot is in progress, that snapshot will restart when the node restarts.

Changed the log to WARN.

ssd04 · 2023-08-07T12:17:00Z

state/snapshotsManager.go

+func (sm *snapshotsManager) getSnapshotRootHashAndEpoch(trieStorageManager common.StorageManager) ([]byte, uint32, error) {
+	sm.mutex.RLock()
+	defer sm.mutex.RUnlock()


do we need mutex lock here?

No, removed.

ssd04 · 2023-08-07T12:48:25Z

state/snapshotsManager.go

+}
+
+func (sm *snapshotsManager) setStateCheckpoint(rootHash []byte, trieStorageManager common.StorageManager) {
+	log.Trace("accountsDB.SetStateCheckpoint", "root hash", rootHash)


Suggested change

log.Trace("accountsDB.SetStateCheckpoint", "root hash", rootHash)

log.Trace("snapshotsManager.SetStateCheckpoint", "root hash", rootHash)

iulianpascalau · 2023-08-04T13:28:39Z

state/peerAccountsDB_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"github.com/multiversx/mx-chain-core-go/core/atomic"


state/accountsDB.go

iulianpascalau · 2023-08-08T08:30:54Z

state/snapshotsManager.go

+
+		select {
+		case <-timer.C:
+			continue


no need for this continue

state/snapshotsManager.go

iulianpascalau · 2023-08-08T08:39:22Z

state/snapshotsManager.go

+	sm.waitForCompletionIfAppropriate(stats)
+}
+
+func (sm *snapshotsManager) verifyAndStartSnapshot(rootHash []byte, epoch uint32, trieStorageManager common.StorageManager) (*snapshotStatistics, bool) {


is this a new function?

No, is a rename of prepareSnapshot() with some small changes. Reverted the name as prepareSnapshot is more accurate to what the func actually does

state/snapshotsManager_test.go

iulianpascalau · 2023-08-08T08:45:27Z

state/snapshotsManager_test.go

+		sm, _ := state.NewSnapshotsManager(getDefaultSnapshotManagerArgs())
+		assert.Equal(t, state.ErrNilTrieSyncer, sm.StartSnapshotAfterRestartIfNeeded(&storageManager.StorageManagerStub{}))
+	})
+


remove the line between sub-tests?

iulianpascalau · 2023-08-08T08:47:33Z

trie/trieStorageManager.go

-	return isActiveDB(stsm)
-}
-
-func isActiveDB(stsm *snapshotTrieStorageManager) bool {


where is this logic moved?

This was removed. The reason was that when the node starts, it opens as many dbs as necessary. At epoch change the node also manages the opening/closing of epoch storers. So these checks are redundant. As an added bonus, even if something happens and a certain snapshot can not be completed fully from the node's own storage, the data will be synced from other nodes (this will not happen if these checks were still in place because the snapshot would not start at all).

great! 👍

gabi-vuls

@@ Log scanner @@

snapshot-in-latest-storer-fix

================================================================================

Known Warnings 13
New Warnings 7
Known Errors 0
New Errors 1
Panics 0
================================================================================
block hash does not match 22167
wrong nonce in block 7366
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 1
================================================================================
No jailed nodes on the testnet
================================================================================

BeniaminDrasovean added 3 commits June 28, 2023 11:24

wait for the storer to change epoch, and only then start the snapshot

aba0af1

log epoch when starting snapshot

c906659

get epoch from header

cd46812

BeniaminDrasovean added type:bug Something isn't working area:trie impact:low interested:protocol size:medium complexity:medium labels Jun 28, 2023

iulianpascalau self-requested a review June 28, 2023 11:02

iulianpascalau requested changes Jun 28, 2023

View reviewed changes

BeniaminDrasovean added 5 commits July 6, 2023 15:42

create snapshotsManager component

92654ad

fix failing tests

9fbaba4

add unit tests

55029ef

Merge remote-tracking branch 'origin/rc/v1.6.0' into snapshot-in-late…

0556125

…st-storer-fix # Conflicts: # process/transactionEvaluator/simulationAccountsDB_test.go # process/txsimulator/wrappedAccountsDB.go # state/accountsDB.go # state/export_test.go # state/interface.go

fix after merge

7ad20d7

ssd04 reviewed Aug 7, 2023

View reviewed changes

iulianpascalau reviewed Aug 8, 2023

View reviewed changes

BeniaminDrasovean and others added 2 commits August 10, 2023 15:48

fix after review

9ebf399

Merge branch 'rc/v1.6.0' into snapshot-in-latest-storer-fix

9c91271

iulianpascalau approved these changes Aug 10, 2023

View reviewed changes

ssd04 approved these changes Aug 11, 2023

View reviewed changes

BeniaminDrasovean added 2 commits August 11, 2023 11:26

Merge branch 'rc/v1.6.0' into snapshot-in-latest-storer-fix

9feca48

Merge branch 'rc/v1.6.0' into snapshot-in-latest-storer-fix

13e319a

gabi-vuls approved these changes Aug 17, 2023

View reviewed changes

BeniaminDrasovean merged commit 44c0f9c into rc/v1.6.0 Aug 18, 2023
6 checks passed

BeniaminDrasovean deleted the snapshot-in-latest-storer-fix branch August 18, 2023 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshot in latest storer fix #5381

Snapshot in latest storer fix #5381

BeniaminDrasovean commented Jun 28, 2023

codecov bot commented Jun 28, 2023 •

edited

iulianpascalau Jun 28, 2023

BeniaminDrasovean Jul 3, 2023

iulianpascalau Jun 28, 2023

BeniaminDrasovean Jul 3, 2023

iulianpascalau Jun 28, 2023

BeniaminDrasovean Jul 3, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 10, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 10, 2023 •

edited

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 10, 2023

ssd04 Aug 7, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 10, 2023

BeniaminDrasovean Aug 10, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 10, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 10, 2023

iulianpascalau Aug 4, 2023

BeniaminDrasovean Aug 10, 2023

iulianpascalau Aug 8, 2023

BeniaminDrasovean Aug 10, 2023

iulianpascalau Aug 8, 2023

BeniaminDrasovean Aug 10, 2023

iulianpascalau Aug 8, 2023

BeniaminDrasovean Aug 10, 2023

iulianpascalau Aug 8, 2023

BeniaminDrasovean Aug 10, 2023

iulianpascalau Aug 10, 2023

gabi-vuls left a comment

	// NewSnapshotsManager creates a new snapshots manager
	// NewSnapshotsManager creates a new snapshots manager

		func (adb *AccountsDB) waitForStorageEpochChange(args StorageEpochChangeWaitArgs) error {
		if args.SnapshotWaitTimeout < args.WaitTimeForSnapshotEpochCheck {

		// StorageEpochChangeWaitArgs are the args needed for calling the WaitForStorageEpochChange function
		type StorageEpochChangeWaitArgs struct {

	log.Trace("accountsDB.SetStateCheckpoint", "root hash", rootHash)
	log.Trace("snapshotsManager.SetStateCheckpoint", "root hash", rootHash)

Snapshot in latest storer fix #5381

Snapshot in latest storer fix #5381

Conversation

BeniaminDrasovean commented Jun 28, 2023

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Jun 28, 2023 • edited

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

BeniaminDrasovean Aug 10, 2023 • edited

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

gabi-vuls left a comment

Choose a reason for hiding this comment

snapshot-in-latest-storer-fix

codecov bot commented Jun 28, 2023 •

edited

BeniaminDrasovean Aug 10, 2023 •

edited