feat: regen to consume state cache reload api#6456
Merged
Conversation
Contributor
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
ensi321
reviewed
Feb 26, 2024
|
|
||
| /** | ||
| * Get state after block `blockRoot` dialed forward to `slot` | ||
| * - shouldReload should be used with care, as it will cause the state to be reloaded from disk |
Contributor
There was a problem hiding this comment.
Do you think it is better to rename shouldReload to shouldReloadFromDisk to emphasize and warn about the consequence of setting this flag to true?
Contributor
Author
There was a problem hiding this comment.
I'm open to change, cc @wemeetagain for your preference
Member
There was a problem hiding this comment.
I think that is a good suggestion. shouldReloadFromDisk is more descriptive and expressive
Contributor
Author
There was a problem hiding this comment.
yes allowDiskReload really matches the usage of this flag 👍
g11tech
reviewed
Feb 29, 2024
| opts: StateCloneOpts, | ||
| rCaller: RegenCaller | ||
| rCaller: RegenCaller, | ||
| shouldReload = false |
Contributor
There was a problem hiding this comment.
should this flag move in StateCloneOpts?
Contributor
Author
There was a problem hiding this comment.
shouldReload is designed to use within this file only, I don't want to publish shouldReload flag to all consumers, it should only be true when we get state for block processing, or updateHeadState. It's easier to control it within this file
g11tech
reviewed
Feb 29, 2024
| opts: StateCloneOpts, | ||
| rCaller: RegenCaller | ||
| rCaller: RegenCaller, | ||
| shouldReload = false |
Contributor
There was a problem hiding this comment.
may be can move in StateCloneOpts here as well?
matthewkeil
reviewed
Feb 29, 2024
|
|
||
| /** | ||
| * Get state after block `blockRoot` dialed forward to `slot` | ||
| * - shouldReload should be used with care, as it will cause the state to be reloaded from disk |
Member
There was a problem hiding this comment.
I think that is a good suggestion. shouldReloadFromDisk is more descriptive and expressive
| const cp = getCheckpointFromState(checkpointState); | ||
| checkpointStateCache.add(cp, checkpointState); | ||
| emitter.emit(ChainEvent.checkpoint, cp, checkpointState); | ||
| emitter.emit(ChainEvent.checkpoint, cp, checkpointState.clone()); |
Member
There was a problem hiding this comment.
Why add the costly clone() here?
Contributor
Author
There was a problem hiding this comment.
- if downstream event handlers mutate the state mistakenly, it does not affect our cache
clone()is really cheap and that's awesome feature of our tree backed state, see https://github.com/ChainSafe/ssz/blob/6220d320b004ea80bd30925487ac0f3299295528/packages/persistent-merkle-tree/src/tree.ts#L80
in fact I will add more clone() when returning an item from cache to avoid this mistake, see #6178 (comment)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6456 +/- ##
============================================
+ Coverage 61.63% 61.69% +0.05%
============================================
Files 553 555 +2
Lines 57975 58253 +278
Branches 1832 1844 +12
============================================
+ Hits 35733 35937 +204
- Misses 22205 22277 +72
- Partials 37 39 +2 |
g11tech
approved these changes
Feb 29, 2024
Contributor
g11tech
left a comment
g11tech
left a comment
There was a problem hiding this comment.
seems to have addressed all concerns lgtm
Member
|
🎉 This PR is included in v1.17.0 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
getPreStateapi), we want to reload state if neededDescription
INVALID_STATE_ROOTerror during regen and add logsthis is cherry-picked from #6359, it does not affect production code because we haven't configured new state caches yet,
getOrReload*is just the same toget*part of #5968