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

beacon/engine: add shouldOverrideBuilder to payload envelope #28029

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

lightclient
Copy link
Member

The shouldOverrideBuilder field was added to response of NewPayload with the intention that EL clients could give the CL client some indication of the health of the execution network in terms of censorship.

This PR is adds support for the field, but sets the value statically to zero so the CL will never override the builder. Marius has a more complete solution in #26829.

I've been using this proposed PR for testing the devents. We should make a decision on which approach to move forward with.

@rjl493456442
Copy link
Member

Do we reach the agreement that either to return the indicator in GetPayload, or a separate engine API?

Ref: ethereum/execution-apis#425

@lightclient
Copy link
Member Author

I think it already agreed upon to return the value in the GetPayload response. It's more that we need to decide if we will return false always or do something smarter like Marius' PR.

@rjl493456442
Copy link
Member

I think the indicator returned in Marius's PR is not very useful. I don't think the censored transactions will occur in the reorg in the first place, but stuck in the txpool forever instead.

But yeah, It's not relevant with this PR though. If we agree to return this extra field in GetPayload, then it's fine for me.

@lightclient
Copy link
Member Author

Yep it is in the dencun spec to return this value. I think this should be merged in the meantime while we seek out a solution which actually does let the CL know if it should override its builder.

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.13.0 milestone Sep 4, 2023
@holiman holiman merged commit f260a9e into ethereum:master Sep 4, 2023
1 check passed
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…eum#28029)

beacon/engine: add shouldOverrideBuilder to payload envelope
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants