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

State snapshot improvement #4459

Merged
merged 5 commits into from
Sep 14, 2022
Merged

Conversation

BeniaminDrasovean
Copy link
Contributor

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

  • There are some edgecases in which a certain trie node is not saved in the proper storer. The problem is that we will have storers that do not have the complete state.

Proposed Changes

  • During the snapshot process, if a trie node is not present in the previous storer (which should have had the entire state), it will also be saved there.

Testing procedure

  • Normal test, but with "*:DEBUG,trie:TRACE" log level, and we will see in the logs that the missing nodes are also saved in the previous storer.

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Merging #4459 (55a1dfc) into rc/v1.4.0 (ff936b2) will increase coverage by 0.00%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           rc/v1.4.0    #4459   +/-   ##
==========================================
  Coverage      73.84%   73.84%           
==========================================
  Files            689      689           
  Lines          88127    88146   +19     
==========================================
+ Hits           65075    65092   +17     
- Misses         18140    18141    +1     
- Partials        4912     4913    +1     
Impacted Files Coverage Δ
trie/snapshotTrieStorageManager.go 69.23% <70.58%> (+5.34%) ⬆️
storage/pruning/triePruningStorer.go 69.13% <87.50%> (+1.18%) ⬆️
trie/trieStorageManager.go 63.34% <100.00%> (+0.26%) ⬆️
storage/txcache/txListBySenderMap.go 97.50% <0.00%> (-2.50%) ⬇️
common/pidQueue.go 96.66% <0.00%> (+3.33%) ⬆️

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

@iulianpascalau iulianpascalau self-requested a review September 13, 2022 06:17
if errors.IsClosingError(err) {
return nil, err
}
if len(val) != 0 {
stsm.putInPreviousStorerIfAbsent(key, val, epoch)
Copy link
Contributor

Choose a reason for hiding this comment

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

putIfAbsent :D
brings back so many memories

return
}

if epoch.Value >= stsm.epoch-1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

so basically, if we are in current epoch 500, epoch will be 499 and we test for epoch 498?
Is this >= correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that if the value was not retrieved from the current storer or previous storer, it needs to be saved to the previous storer.

iulianpascalau
iulianpascalau previously approved these changes Sep 13, 2022
sstanculeanu
sstanculeanu previously approved these changes Sep 13, 2022
Copy link
Contributor

@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.

System test passed.

@BeniaminDrasovean BeniaminDrasovean merged commit 90bd65a into rc/v1.4.0 Sep 14, 2022
@BeniaminDrasovean BeniaminDrasovean deleted the snapshot-improvement branch September 14, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants