Add support for Capella #3763
Merged
Add support for Capella #3763
Conversation
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
paulhauner
requested changes
Feb 16, 2023
There was a problem hiding this comment.
I just finished reviewing the execution_layer. It looks great, I only have really minor suggestions.
I believe that all of the suggestions here will compile if they're accepted and committed via the Github UI. Anything that I couldn't get to work via suggestions I added here: #3983
Co-authored-by: Paul Hauner <[email protected]>
* Restrict Engine::request to FnOnce * Use `Into::into` * Impl IntoIterator for VariableList * Use Instant rather than SystemTime
Update `capella` to `unstable`
|
I just finished reviewing |
* Remove CapellaReadiness::NotSynced Some EEs have a habit of flipping between synced/not-synced, which causes some spurious "Not read for the merge" messages back before the merge. For the merge, if the EE wasn't synced the CE simple wouldn't go through the transition (due to optimistic sync stuff). However, we don't have that hard requirement for Capella; the CE will go through the fork and just wait for the EE to catch up. I think that removing `NotSynced` here will avoid false-positives on the "Not ready logs..". We'll be creating other WARN/ERRO logs if the EE isn't synced, anyway. * Change some Capella readiness logging There's two changes here: 1. Shorten the log messages, for readability. 2. Change the hints. Connecting a Capella-ready LH to a non-Capella-ready EE gives this log: ``` WARN Not ready for Capella info: The execution endpoint does not appear to support the required engine api methods for Capella: Required Methods Unsupported: engine_getPayloadV2 engine_forkchoiceUpdatedV2 engine_newPayloadV2, service: slot_notifier ``` This variant of error doesn't get a "try updating" style hint, when it's the one that needs it. This is because we detect the method-not-found reponse from the EE and return default capabilities, rather than indicating that the request fails. I think it's fair to say that an EE upgrade is required whenever it doesn't provide the required methods. I changed the `ExchangeCapabilitiesFailed` message since that can only happen when the EE fails to respond with anything other than success or not-found.
* Add extra encoding/decoding tests * Remove TODO The method LGTM * Remove `FreeAttestation` This is an ancient relic, I'm surprised it still existed! * Add paranoid check for eip4844 code This is not technically necessary, but I think it's nice to be explicit about EIP4844 consensus code for the time being. * Reduce big-O complexity of address change pruning I'm not sure this is *actually* useful, but it might come in handy if we see a ton of address changes at the fork boundary. I know the devops team have been testing with ~100k changes, so maybe this will help in that case. * Revert "Reduce big-O complexity of address change pruning" This reverts commit e7d93e6.
* Allow for withdrawals in max block size * Ensure payload size is counted
* Recognise execution in post-merge blocks * Remove `.body()` * Fix typo * Use `is_default_with_empty_roots`.
* Modify comment to only include 4844 Capella only modifies per epoch processing by adding `process_historical_summaries_update`, which does not change the realization of justification or finality. Whilst 4844 does not currently modify realization, the spec is not yet final enough to say that it never will. * Clarify address change verification comment The verification of the address change doesn't really have anything to do with the current epoch. I think this was just a copy-paste from a function like `verify_exit`.
paulhauner
approved these changes
Feb 21, 2023
There was a problem hiding this comment.
Approved! 🎉 🎉
Congrats to all involved, this looks great! As you can see in the history, I made some suggestions via a bunch of PRs. However I don't think any of them were really significant, we probably could have gone to production as it was.
Seeing as we're planning to merge this into unstable via a merge commit (rather than squash merge via bors), I think it makes sense to wait and see that CI finishes before merging.
|
Leaving the |
michaelsproul
added
ready-for-merge
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.
No description provided.