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_execution_enabled condition since Capella #3350

Merged
merged 4 commits into from
May 24, 2023

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 9, 2023

Related issue: #3232

Issue

I would argue that the Bellatrix is_execution_enabled condition check is technical debt that we can remove now. Since this condition is surely satisfied on the mainnet, the benefits of having the ability to support the "not-merged-beacon-chain" testnets are inessential.

How did I fix it

  • Remove the is_execution_enabled check since Capella.
  • Update the spec tests accordingly

This is a substantive beacon chain change but backward-compatible with the Ethereum mainnet.

@hwwhww hwwhww added the Capella label May 9, 2023
@mkalinin
Copy link
Collaborator

I am in favour of this change as neither Capella nor Deneb can't be taken into effect without execution being enabled, thus, they are dependent on Bellatrix. The only scenario when is_execution_enabled can still be relevant is a simultaneous activation of Bellatrix and Capella, and potentially Deneb. I doubt any testnet requires this type of upgrade, likewise, any mainnet accepting risks related to a bundled update of this kind.

Do we also want to remove validate_merge_block from Deneb FC spec?

@potuz
Copy link
Contributor

potuz commented May 10, 2023

+1 on removing merge block validation.

@terencechain
Copy link
Contributor

+1. This will enable cleaner client code

@hwwhww
Copy link
Contributor Author

hwwhww commented May 10, 2023

@mkalinin

Do we also want to remove validate_merge_block from Deneb FC spec?

Yes, this PR and #3232 should be considered together. 👍
And I'm in favor of updating them since Capella.

@djrtwo
Copy link
Contributor

djrtwo commented May 10, 2023

The main reason I've pushed back on this in the past is that alternative networks might want to do their own "Merge" to switch over to beacon chain consensus, and in such a case, wouldn't want to do so at some past construction of the beacon chain -- Bellatrix.

I understand that that claim might be weak against maintaining complex codepaths that are no longer used on mainnet and keeping the consideration in the spec moving forward.

I suppose if someone wants to do a merge in their consensus, they can use old client releases and then step forward to current beacon chain spec via a number of forks.

If this is the consensus, I won't block it

@mkalinin
Copy link
Collaborator

I think that this change should also get rid of is_merge_transition_complete check in the process_execution_payload

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants