chore: remove merge transition code - #8680
Conversation
Summary of ChangesHello @nflaig, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request performs a significant cleanup by eliminating all code, configurations, and tests pertaining to the Ethereum Merge transition. Since the network has successfully transitioned to Proof-of-Stake, the complex logic required to manage the PoW-to-PoS switch is no longer necessary. This change simplifies the codebase, reduces maintenance overhead, and removes obsolete features, ensuring the client focuses solely on post-Merge operations. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a comprehensive cleanup of code related to the Ethereum merge transition. It successfully removes a significant amount of obsolete logic, including the Eth1MergeBlockTracker, pre-merge conditional paths in block verification and production, and merge-specific test infrastructure. The changes span across application logic, tests, metrics, and configuration, resulting in a much simpler and more maintainable codebase that is purely focused on the post-merge state of the chain. The refactoring is well-executed and consistent throughout the project. I have reviewed the changes and found no issues.
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
| export const paramsOptions: CliCommandOptions<IParamsArgs> = { | ||
| ...paramsOptionsByName, | ||
|
|
||
| "terminal-total-difficulty-override": { |
There was a problem hiding this comment.
I don't expect anyone is still setting these flags, but if we wanna be extra careful could keep them around
| if ( | ||
| fork >= ForkSeq.bellatrix && | ||
| fork < ForkSeq.gloas && | ||
| isExecutionEnabled(state as CachedBeaconStateBellatrix, block) |
There was a problem hiding this comment.
removing this check might cause issues syncing through bellatrix, but really not sure we wanna support that anymore as it's outside of weak subjectivity period and not safe anyways
There was a problem hiding this comment.
as suspected this is an issue when trying to sync from genesis
debug: Block error slot=4636672, code=BLOCK_ERROR_BEACON_CHAIN_ERROR, error=Invalid execution payload random 0x0000000000000000000000000000000000000000000000000000000000000000 expected=0xa43ac70cdc82b95f90d74a03f08eb2df5adf46c7f5ccd44a62a9a3c4e8d8cc48
Dec 16 16:42:49 feat4-mainnet-hzax41 beacon_run.sh[144288]: Error: Invalid execution payload random 0x0000000000000000000000000000000000000000000000000000000000000000 expected=0xa43ac70cdc82b95f90d74a03f08eb2df5adf46c7f5ccd44a62a9a3c4e8d8cc48
if we wanna be able to still do that we would need to restore isExecutionEnabled which would be doable, we can still remove most the code as done in this PR
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #8680 +/- ##
============================================
+ Coverage 52.04% 52.12% +0.07%
============================================
Files 848 848
Lines 65734 65335 -399
Branches 4807 4789 -18
============================================
- Hits 34214 34055 -159
+ Misses 31451 31211 -240
Partials 69 69 🚀 New features to boost your workflow:
|
**Motivation** As noted in #8680 (comment) we cannot sync through bellatrix anymore. While I don't think it's a big deal it's simple enough to keep that functionality as that code is pretty isolated and won't get in our way during refactors and with gloas won't be part of the block processing pipeline anymore due to block/payload separation. **Description** Restore code required to perform sync through bellatrix - re-added `isExecutionEnabled()` and `isMergeTransitionComplete()` checks during block processing - enabled some spec tests again that were previously skipped - mostly copied original code removed in [#8680](#8680) but cleaned up some comments and simplified a bit
|
🎉 This PR is included in v1.39.0 🎉 |
Motiviation
All networks have completed the merge transition and most execution clients no longer support pre-merge so it's not even possible anymore to run a network from a genesis before bellatrix, unless you keep it to phase0/altair only, which still works after this PR is merged.
This code is effectively tech debt, no longer exercised and just gets in the way when doing refactors.
Description
Removes all code related to performing the merge transition. Running the node pre-merge (CL only mode) is still possible and syncing still works. Also removed a few CLI flags we added for the merge specifically, those shouldn't be used anymore. Spec constants like
TERMINAL_TOTAL_DIFFICULTYare kept for spec compliance and ssz types (likePowBlock) as well. I had to disable a few spec tests related to handling the merge block since those code paths are removed.Closes #8661