Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove is_merge_transition_block check since Capella #3232

Merged
merged 4 commits into from
May 24, 2023
Merged

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jan 27, 2023

This PR removed the is_merge_transition_block check from Capella on_block:

    if is_merge_transition_block(pre_state, block.body):
        validate_merge_block(block)

At least for mainnet, these checks are useless.

From @mkalinin:

I feel like we can safely remove if is_merge_transition_complete(state): condition from process_execution_payload in Capella unless Bellatrix and Capella are scheduled at the same epoch. In this case this condition must exist otherwise payload.parent_hash check will always fail. But I don't think anyone uses this chain specificaiton for any purposes, because it involves setting up PoW or PoA which ppl might not want to have a deal with.

I fee like the following code can be removed from the fork choice spec disregarding fork schedule as it just validates TTD and TBH which doesn't make sense for mainnet and likely for test/dev nets too.

@terencechain
Copy link
Contributor

terencechain commented Jan 27, 2023

Nice! +1 for client code clean up. But also worth checking if this breaks existing Hive or Antithesis tests

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mixed on the proposal. we don't yet remove it from the state transition function so weird to remove it only in one place

I also could imagine some use for doing a transition at Capella on an alternative network.

That said, I'm not that opposed to this if others want it. Def want to check with @marioevz to see if this will break some hive assumption

@mkalinin
Copy link
Collaborator

mkalinin commented Feb 3, 2023

Though, it would be still possible to do the merge transition at the same time as Deneb, I would consider doing a full clean up of transition logic at that time as introducing blob transactions on EL without execution enabled on CL is odd.

I also could imagine some use for doing a transition at Capella on an alternative network.

They would need to schedule Bellatrix first and then after the transition happens they could schedule Cancun. Also, those networks would need clients supporting transition logic.

@tersec
Copy link
Contributor

tersec commented Feb 14, 2023

This is consistent with simply not ever supporting doing a merge again. That, in itself, I have no objection to: either start unmerged and stay unmerged, or start merged.

However, if merging is supported at all (e.g., insofar as the Bellatrix fork retains such a function), this ensures that merging remains a strangely partly-supported functionality, if and only if one goes back to Bellatrix.

I am not strongly against this, but it conveys a rather muddy conceptual model.

@hwwhww hwwhww changed the title Delete is_merge_transition_block check from Capella Remove is_merge_transition_block check since Capella May 16, 2023
@djrtwo djrtwo mentioned this pull request May 23, 2023
8 tasks
@hwwhww hwwhww merged commit ee09c34 into dev May 24, 2023
24 checks passed
@hwwhww hwwhww deleted the capella-fc-cleanup branch May 24, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants