feat: prune BlsToExecutionChange opPool with head state #6252
Conversation
This file contains 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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6252 +/- ##
=========================================
Coverage 80.31% 80.31%
=========================================
Files 202 202
Lines 19543 19543
Branches 1169 1169
=========================================
Hits 15695 15695
Misses 3820 3820
Partials 28 28 |
Performance Report✔️ no performance regression detected Full benchmark results
|
jeluard
reviewed
Jan 5, 2024
dapplion
reviewed
Jan 8, 2024
|
|
||
| const recentBlsToExecutionChangeIndexes = new Set( | ||
| recentBlsToExecutionChanges.map((blsToExecutionChange) => blsToExecutionChange.message.validatorIndex) | ||
| ); |
There was a problem hiding this comment.
This check looks insufficient. The prune handler is only called once per finalization event ~1 time / epoch. Then only the execution changes included in that block will be pruned, with all remaining changes staying there.
A more complete solution, using our state caches:
Attach new cache to the state:
- appliedBlsToExecutionChangesCurrentEpoch
- appliedBlsToExecutionChangesPreviousEpoch
During block processing you register the validator index of applied execution changes. During epoch transition you rotate the caches. Then to prune you use the appliedBlsToExecutionChangesPreviousEpoch list to prune those. This will give you the complete list of items to prune if there are no re-orgs deeper than 1 epoch, which is an okay compromise.
There was a problem hiding this comment.
Then only the execution changes included in that block will be pruned, with all remaining changes staying there.
just to clarify, execution changes in the head block is not pruned, others will be pruned. So we store BLSToExecutionChange messages in max 33 slots
A more complete solution, using our state caches
the solution works, however I think it's too much for this feature since we have to apply ImmutableJS to maintain these 2 arrays like in EIP-6110 implementation #6042 Also it's tricky in the edge case
I think there are pros and cons for these 2 solutions:
| # | BLSToExecutionChange messages in block slot in opPool | pros | cons |
|---|---|---|---|
| current | 3 epochs | straightforward | have to query the finalized state |
| no state cache (this PR) | 1 slot to 33 slots | no state cache is needed, lighthouse runs it since capella | cover 1-slot reorg scenario only |
| state cache (Lion's suggestion) | 32 slots to 64 slots | cover up to 1-epoch reorg scenario | need to have 2 ImmutableJS arrays maintained, also need to think about edge case when finalized checkpoint happens >4s, after clock epoch |
I prefer the no state cache approach as it's worked for lighthouse since capella and in the worse case if the reorg happen, it'll not affect block reward (if it's the bn's turn to propose block) and messages will be added back once gossip seenTTL passes anyway, I'd like to know others' opinions cc @wemeetagain @g11tech
There was a problem hiding this comment.
I think what @tuyennhv proposed here is sufficient and his reasoning is sound.
There was a problem hiding this comment.
Sounds good, thanks for the table! please monitor metrics after deploying to check the pool is not growing
There was a problem hiding this comment.
the heuristic is fine i guess, since no one is anyway producing these messages and deep reorgs are rare on mainnet
wemeetagain
approved these changes
Jan 8, 2024
twoeths
deleted the
tuyen/prune_bls_to_execution_change_with_head_state
branch
|
🎉 This PR is included in v1.14.0 🎉 |
ensi321
pushed a commit
to ensi321/lodestar
that referenced
this pull request
Jan 22, 2024
* feat: prune BlsToExecutionChange opPool with head state * fix: address PR comment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
TODOlistDescription
this is a prerequisite for #6250