chore: remove merge transition code - #145
Conversation
Summary of ChangesHello @guha-rahul, 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 of the codebase by removing all remaining vestiges of the 'pre-merge' transition logic. This includes eliminating the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively removes the merge transition code, simplifying several functions. The changes are generally clean and follow the PR's motivation. However, I've found a critical issue in isExecutionEnabled where a necessary check was removed, which could lead to a crash. I've provided a detailed comment and a suggested fix for this. There's also a logical issue in the same function that you may want to look at.
| pub fn isExecutionEnabled(state: *const BeaconStateAllForks, block: Block) bool { | ||
| if (!state.isPostBellatrix()) return false; | ||
| if (isMergeTransitionComplete(state)) return true; | ||
|
|
||
| // TODO(bing): in lodestar prod, state root comparison should be enough but spec tests were failing. This switch block is a failsafe for that. | ||
| // | ||
| // Ref: https://github.com/ChainSafe/lodestar/blob/7f2271a1e2506bf30378da98a0f548290441bdc5/packages/state-transition/src/util/execution.ts#L37-L42 | ||
| switch (block) { | ||
| .blinded => |b| { | ||
| const body = b.beaconBlockBody(); | ||
|
|
||
| return switch (body) { | ||
| .capella => |bd| !types.capella.ExecutionPayloadHeader.equals(&bd.execution_payload_header, &types.capella.ExecutionPayloadHeader.default_value), | ||
| .deneb => |bd| !types.deneb.ExecutionPayloadHeader.equals(&bd.execution_payload_header, &types.deneb.ExecutionPayloadHeader.default_value), | ||
| .electra => |bd| !types.electra.ExecutionPayloadHeader.equals(&bd.execution_payload_header, &types.electra.ExecutionPayloadHeader.default_value), | ||
| }; | ||
| }, | ||
| .regular => |b| { | ||
| const body = b.beaconBlockBody(); | ||
|
|
||
| return switch (body) { | ||
| .phase0, .altair => @panic("Unsupported"), | ||
| .bellatrix => |bd| !types.bellatrix.ExecutionPayload.equals(&bd.execution_payload, &types.bellatrix.ExecutionPayload.default_value), | ||
| .capella => |bd| !types.capella.ExecutionPayload.equals(&bd.execution_payload, &types.capella.ExecutionPayload.default_value), | ||
| .deneb => |bd| !types.deneb.ExecutionPayload.equals(&bd.execution_payload, &types.deneb.ExecutionPayload.default_value), | ||
| .electra, .fulu => |bd| !types.electra.ExecutionPayload.equals(&bd.execution_payload, &types.electra.ExecutionPayload.default_value), | ||
| }; | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| pub fn isMergeTransitionBlock(state: *const BeaconStateAllForks, body: *const BeaconBlockBody) bool { | ||
| if (!state.isBellatrix()) { | ||
| return false; | ||
| if (isMergeTransitionComplete(state)) { | ||
| return true; | ||
| } | ||
|
|
||
| return (!isMergeTransitionComplete(state) and | ||
| !types.bellatrix.ExecutionPayload.equals(body.getExecutionPayload().bellatrix, types.bellatrix.ExecutionPayload.default_value)); | ||
| return switch (block) { | ||
| .blinded => !types.bellatrix.ExecutionPayloadHeader.equals( | ||
| &state.bellatrix.latest_execution_payload_header, | ||
| &types.bellatrix.ExecutionPayloadHeader.default_value, | ||
| ), | ||
| .regular => |b| switch (b.beaconBlockBody()) { | ||
| .bellatrix => |bd| !types.bellatrix.ExecutionPayload.equals( | ||
| &bd.execution_payload, | ||
| &types.bellatrix.ExecutionPayload.default_value, | ||
| ), | ||
| else => false, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This function has a couple of issues after the refactoring:
-
Potential Crash: The check for pre-Bellatrix states (
if (!state.isPostBellatrix()) return false;) was removed. Without it, calling this function with a pre-Bellatrix state (e.g., Altair) will cause a crash.isMergeTransitionCompletewill correctly returnfalse, and the code will then try to accessstate.bellatrix, which is invalid for an Altair state. This violates the style guide rule to 'Assert all function arguments and return values, pre/postconditions and invariants' (line 51), as the function proceeds without ensuring the state is at least Bellatrix. -
Incorrect Logic for Blinded Blocks: The logic for
.blindedblocks now checks the state'slatest_execution_payload_header. However, the purpose here is to check if the block is a transition block. IfisMergeTransitionCompletewas false, it's because the state's header is empty, so this check will always befalse. The logic should inspect theexecution_payload_headerfrom theblockitself.
I'm providing a suggestion to fix the critical crash. The logical issue with blinded blocks might require more extensive changes to your type definitions, so I've left that part as is for you to address.
pub fn isExecutionEnabled(state: *const BeaconStateAllForks, block: Block) bool {
if (!state.isPostBellatrix()) return false;
if (isMergeTransitionComplete(state)) {
return true;
}
return switch (block) {
.blinded => !types.bellatrix.ExecutionPayloadHeader.equals(
&state.bellatrix.latest_execution_payload_header,
&types.bellatrix.ExecutionPayloadHeader.default_value,
),
.regular => |b| switch (b.beaconBlockBody()) {
.bellatrix => |bd| !types.bellatrix.ExecutionPayload.equals(
&bd.execution_payload,
&types.bellatrix.ExecutionPayload.default_value,
),
else => false,
},
};
}
References
- The function proceeds to access fork-specific state fields without first asserting that the state is of the correct fork or a later one. This violates the rule to assert all preconditions. (link)
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Review: chore: remove merge transition code
Thanks for the cleanup effort @guha-rahul — the intent here is sound. The merge transition (Bellatrix → PoS activation) happened on mainnet in Sept 2022 and all live networks have long passed it. Removing pre_merge, ExecutionPayloadStatusPreMerge, and isMergeTransitionBlock reduces dead code paths.
Problem: PR is stale against current main
Since this was opened (Dec 2025), main has gone through a major refactor — PR #190 (feat: comptime fork) rewrote execution.zig to use comptime fork parameters and ForkTypes(fork) dispatch instead of runtime BeaconStateAllForks switches. The current file looks nothing like what this PR patches against:
Current main (execution.zig):
- Uses
comptime fork: ForkSeqparameters isExecutionEnabledtakesBeaconState(fork)+BeaconBlock(block_type, fork)isMergeTransitionCompletecheckslatestExecutionPayloadHeaderBlockHash()againstZERO_HASHisMergeTransitionBlockusesForkTypes(fork).ExecutionPayload.equals()
This PR's diff targets the old runtime-dispatched version with BeaconStateAllForks and inline fork switches — which no longer exists.
The PR correctly shows CONFLICTING merge status.
What needs to happen to land this
- Rebase onto current
main— the diff needs to be rewritten against the comptime-fork versions of these functions - Remove
pre_mergefromExecutionPayloadStatus(still exists atsrc/state_transition/state_transition.zig:35) - Remove
pre_mergechecks inprocess_blob_kzg_commitments.zigandprocess_execution_payload.zig(still reference.pre_merge) - Remove
isMergeTransitionBlock— currently has zero callers (confirmed via grep) - Keep
isMergeTransitionComplete— still called fromisExecutionEnabledandprocess_execution_payload.zig
The actual changes on current main would be quite small (~15 lines removed).
Devil's advocate: should we keep merge transition code?
For a production client that needs to sync from genesis, yes. But lodestar-z doesn't support pre-Bellatrix sync yet, and when it does, it'll likely use checkpoint sync (post-merge). The spec tests also don't test the merge transition path for post-Bellatrix forks. So removing this dead code is the right call.
However, isMergeTransitionComplete must stay — isExecutionEnabled depends on it for the edge case where a post-Bellatrix state hasn't yet seen an execution payload (unlikely in practice but spec-correct).
Verdict
The cleanup is welcome, but the PR needs a full rebase. @guha-rahul — are you still interested in updating this? If not, we can pick up the remaining cleanup. The work is small on current main.
## chore: remove merge transition code Closes #130 (supersedes stale PR #145) ### Context The merge transition (Bellatrix → PoS activation) happened on mainnet in September 2022. All live networks have long passed it, and lodestar-z doesn't support pre-Bellatrix sync. The related code paths are dead. PR #145 targeted the old runtime-dispatched version of these functions, which was replaced by the comptime fork refactor in #190. This PR applies the removal against current `main`. ### Changes - Removed `pre_merge` variant from `ExecutionPayloadStatus` enum - Removed `.pre_merge` check in `process_blob_kzg_commitments.zig` - Removed `.pre_merge` check in `process_execution_payload.zig` - Removed `isMergeTransitionBlock` function from `execution.zig` (zero callers) `isMergeTransitionComplete` is intentionally kept — it's still used by `isExecutionEnabled` for the edge case where a post-Bellatrix state hasn't yet seen an execution payload. *AI disclosure: Claude was consulted for reviewing the diff and drafting this description. All code changes were authored manually.* --------- Co-authored-by: bing <spiralladder@fastmail.com> Co-authored-by: Chen Kai <281165273grape@gmail.com> Co-authored-by: Cayman <caymannava@gmail.com> Co-authored-by: Nazar Hussain <nazarhussain@gmail.com> Co-authored-by: NC <17676176+ensi321@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
3 participants
Motivation
Closes #130