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

Pruning bugfix #4160

Merged
merged 8 commits into from
Jun 15, 2022
Merged

Pruning bugfix #4160

merged 8 commits into from
Jun 15, 2022

Conversation

BeniaminDrasovean
Copy link
Contributor

After a node restarts, delay the trie nodes removal from db in order to give the evictionWaitingList enough time to build the pruning history.

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #4160 (11636b4) into rc/2022-june (c5a4774) will increase coverage by 0.03%.
The diff coverage is 93.33%.

@@               Coverage Diff                @@
##           rc/2022-june    #4160      +/-   ##
================================================
+ Coverage         75.27%   75.31%   +0.03%     
================================================
  Files               617      618       +1     
  Lines             82418    82451      +33     
================================================
+ Hits              62043    62099      +56     
+ Misses            15685    15659      -26     
- Partials           4690     4693       +3     
Impacted Files Coverage Δ
...ate/storagePruningManager/storagePruningManager.go 73.91% <66.66%> (-1.98%) ⬇️
process/block/baseProcess.go 72.85% <100.00%> (+0.22%) ⬆️
process/block/metablock.go 60.57% <100.00%> (+0.20%) ⬆️
process/block/shardblock.go 65.97% <100.00%> (+0.19%) ⬆️
process/txsimulator/wrappedAccountsDB.go 73.91% <100.00%> (ø)
state/accountsDB.go 77.61% <100.00%> (ø)
state/accountsDBApi.go 100.00% <100.00%> (ø)
state/pruningHandler.go 100.00% <100.00%> (ø)
storage/txcache/txListBySenderMap.go 97.50% <0.00%> (-2.50%) ⬇️
p2p/libp2p/netMessenger.go 79.65% <0.00%> (+3.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5a4774...11636b4. Read the comment docs.

@iulianpascalau iulianpascalau self-requested a review June 3, 2022 14:35
Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

👍
GJ implementing this

}

// NewPruningHandler returns a new instance of pruningHandler with the given parameters
func NewPruningHandler(isPruningEnabled bool) *pruningHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not creating 2 constructors instead of one that requires a bool passing?
Something like:

NewEnabledPruningHandler
NewDisabledPruningHandler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking that if in the future we add more options, the number of constructors will grow exponentially 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, valid point.
Then, can we define a type for that constructor (in the same state/pruningHandler.go) file

type PruningHandlerOperation bool

const EnableDataRemoval PruningHandlerOperation = true
const DisableDataRemoval PruningHandlerOperation = false

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.

iulianpascalau
iulianpascalau previously approved these changes Jun 9, 2022
@bogdan-rosianu bogdan-rosianu self-requested a review June 10, 2022 13:33
process/block/metablock.go Outdated Show resolved Hide resolved
process/block/shardblock.go Outdated Show resolved Hide resolved
process/block/metablock.go Outdated Show resolved Hide resolved
state/pruningHandler.go Outdated Show resolved Hide resolved
process/block/baseProcess.go Outdated Show resolved Hide resolved
bogdan-rosianu
bogdan-rosianu previously approved these changes Jun 14, 2022
pruningQueueSize := arguments.Config.StateTriesConfig.PeerStatePruningQueueSize
pruningDelay := uint32(pruningQueueSize * pruningDelayMultiplier)
if pruningDelay < defaultPruningDelay {
log.Warn("using default pruning delay", "peer state pruning queue size", pruningQueueSize)
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 this will be used also on the metachain's user account. Suggestion to change the message accordingly.

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

System test passed.
@@ Log scanner @@

pruning-bugfix

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

  • Known Warnings 9
  • New Warnings 0
  • Known Errors 0
  • New Errors 0
  • Panics 0
    ================================================================================

@iulianpascalau iulianpascalau merged commit fe4df8e into rc/2022-june Jun 15, 2022
@iulianpascalau iulianpascalau deleted the pruning-bugfix branch June 15, 2022 10:27
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

5 participants