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

lastSnapshot marker fix #5642

Merged
merged 20 commits into from Oct 27, 2023
Merged

lastSnapshot marker fix #5642

merged 20 commits into from Oct 27, 2023

Conversation

BeniaminDrasovean
Copy link
Contributor

@BeniaminDrasovean BeniaminDrasovean commented Oct 6, 2023

Reasoning behind the pull request

  • There was an edgecase in which the lastSnapshot marker was put in the new epoch before the storers could actually open the new epoch storer. This triggered an WARN print, and the marker was not saved to storage.

Proposed changes

  • Before calling PutInEpoch for the lastSnapshot marker, wait for the storers to change epochs.
  • Created a new component which handles the lastSnapshot marker operations

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
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (b209c7f) 80.34% compared to head (aa2def0) 80.31%.
Report is 4 commits behind head on rc/v1.7.0.

❗ Current head aa2def0 differs from pull request most recent head 5726d2d. Consider uploading reports for the commit 5726d2d to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.7.0    #5642      +/-   ##
=============================================
- Coverage      80.34%   80.31%   -0.04%     
=============================================
  Files            708      710       +2     
  Lines          94096    94132      +36     
=============================================
  Hits           75601    75601              
- Misses         13187    13220      +33     
- Partials        5308     5311       +3     
Files Coverage Δ
api/groups/transactionGroup.go 98.31% <100.00%> (ø)
state/accountsDB.go 77.66% <100.00%> (+0.03%) ⬆️
state/errors.go 25.00% <ø> (ø)
state/peerAccountsDB.go 96.07% <100.00%> (+0.07%) ⬆️
state/snapshotsManager.go 97.19% <100.00%> (-0.27%) ⬇️
state/trackableDataTrie/trackableDataTrie.go 84.34% <100.00%> (+0.07%) ⬆️
storage/storageEpochChange/storageEpochChange.go 100.00% <100.00%> (ø)
state/lastSnapshotMarker/lastSnapshotMarker.go 88.23% <88.23%> (ø)

... and 4 files with indirect coverage changes

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

@iulianpascalau iulianpascalau self-requested a review October 6, 2023 09:56
common/storage/storage.go Outdated Show resolved Hide resolved
common/storage/storage.go Outdated Show resolved Hide resolved
state/accountsDB_test.go Outdated Show resolved Hide resolved
state/export_test.go Outdated Show resolved Hide resolved
state/interface.go Outdated Show resolved Hide resolved
state/lastSnapshotMarker/lastSnapshotMarker.go Outdated Show resolved Hide resolved
state/lastSnapshotMarker/lastSnapshotMarker.go Outdated Show resolved Hide resolved
@ssd04 ssd04 self-requested a review October 6, 2023 10:18
err := trieStorageManager.PutInEpoch([]byte(lastSnapshot), rootHash, epoch)
handleLoggingWhenError("could not set lastSnapshot", err, "rootHash", rootHash)
}()
go sm.lastSnapshotMarker.AddMarker(trieStorageManager, epoch, rootHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

AddMarker will check internally for WaitForStorageEpochChange , should we avoid that in ImportDB processing mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, there is no need for this if we run in importDB mode.

Comment on lines +66 to +67
if check.IfNil(args.LastSnapshotMarker) {
return nil, ErrNilLastSnapshotMarker
Copy link
Contributor

Choose a reason for hiding this comment

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

unit test for nil check LastSnapshotMarker?

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.

state/accountsDB_test.go Outdated Show resolved Hide resolved
common/storage/storage.go Outdated Show resolved Hide resolved
iulianpascalau
iulianpascalau previously approved these changes Oct 26, 2023
@@ -10,7 +10,7 @@ import (
logger "github.com/multiversx/mx-chain-logger-go"
)

var log = logger.GetOrCreate("state")
var log = logger.GetOrCreate("storage")
Copy link
Contributor

Choose a reason for hiding this comment

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

storage/storageEpochChange to comply with the rest of the project

iulianpascalau
iulianpascalau previously approved these changes Oct 26, 2023
ssd04
ssd04 previously approved these changes Oct 26, 2023
gabi-vuls
gabi-vuls previously approved these changes Oct 27, 2023
Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

Normal allin test: v1.5.13-dev-config-8c685c8e00 -> test-last-snapshot-marker--53d20a600c

--- Specific errors ---

block hash does not match 8716
wrong nonce in block 2958
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 0

/------/

--- Statistics ---

Nr. of all ERRORS: 0
Nr. of all WARNS: 385
Nr. of new ERRORS: 0
Nr. of new WARNS: 20
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

--- WARNINGS ---
/------/

@BeniaminDrasovean BeniaminDrasovean changed the base branch from rc/v1.6.0 to rc/v1.7.0 October 27, 2023 08:25
@BeniaminDrasovean BeniaminDrasovean dismissed stale reviews from gabi-vuls, ssd04, and iulianpascalau October 27, 2023 08:25

The base branch was changed.

@BeniaminDrasovean BeniaminDrasovean merged commit 9d81e92 into rc/v1.7.0 Oct 27, 2023
6 checks passed
@BeniaminDrasovean BeniaminDrasovean deleted the last-snapshot-marker-fix branch October 27, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants