Dedup post merge readiness notifiers #6797
Open
+116
−690
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
dapplion
commented
Jan 13, 2025
beacon_node/client/src/notifier.rs
Outdated
| @@ -63,7 +72,7 @@ pub fn spawn_notifier<T: BeaconChainTypes>( | |||
| ); | |||
| eth1_logging(&beacon_chain, &log); | |||
| bellatrix_readiness_logging(Slot::new(0), &beacon_chain, &log).await; | |||
| capella_readiness_logging(Slot::new(0), &beacon_chain, &log).await; | |||
| post_capella_readiness_logging(Slot::new(0), &beacon_chain, &log).await; | |||
There was a problem hiding this comment.
All forks after capella forgot to add the notifier here, resulting is the lack of readiness checks pre-genesis. Since it only affects devnets It was not noticed probably
dapplion
commented
Jan 13, 2025
| ); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
No need to do this check here, we have the EL status upcheck service
macladson
requested changes
Jan 28, 2025
beacon_node/client/src/notifier.rs
Outdated
| @@ -417,231 +410,139 @@ async fn bellatrix_readiness_logging<T: BeaconChainTypes>( | |||
| } | |||
|
|
|||
| /// Provides some helpful logging to users to indicate if their node is ready for Capella | |||
| async fn capella_readiness_logging<T: BeaconChainTypes>( | |||
| async fn post_capella_readiness_logging<T: BeaconChainTypes>( | |||
There was a problem hiding this comment.
For clarity I think this should be:
Suggested change
| async fn post_capella_readiness_logging<T: BeaconChainTypes>( | |
| async fn post_bellatrix_readiness_logging<T: BeaconChainTypes>( |
There was a problem hiding this comment.
Fixed with af49dc8
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.
Issue Addressed
Lots of repeated code that just checks if the engine API has the required methods for the next fork. All readiness checks after the merge just check if a specific methods are in the list of capabilities.
Proposed Changes
Delete boilerplate so future forks don't need to copypasta again.
The logic now will:
If a future fork requires more complex readiness checks, it can add a dedicated _readiness file like for the merge.