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

Allow EL to suggest local execution #388

Closed
wants to merge 1 commit into from

Conversation

potuz
Copy link
Contributor

@potuz potuz commented Mar 9, 2023

This PR adds a boolean field shouldOverrideBuilder to engine_getPayload. This provides a way for the EL to indicate that the CL should consider using this payload and not rely in any externally provided one. Arbitrary heuristics are allowed, and the CL may or may not use this information.

Rationale: We have currently a circuit breaker to prevent using a relayer in case blocks are missing under certain scenarios. These scenarios are purely based on consensus data. I propose that we let the EL weigh in this decision based on its local view of execution data. Examples of this are

  • A particular transaction, paying much more than the usual priority fee, has not been included for several slots
  • A particular transaction has been included in many blocks and those have been reorged out systematically.
  • A particular transaction has been included in blocks that have been invalid systematically.

EL clients that do not want to implement any changes can simply return false in this field and behavior would be as before.

@terencechain
Copy link
Contributor

A possible extension to this is also to return the "reason" of the override

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

I like this change as it gives the CL more signal to determine how to orchestrate block production, including using mempool heuristics to decide if local execution should be preferred over an outsource.

I do think we should be careful about how much complexity/content we put into this interface as already we have effectively added two fields to this API to open a wider channel from the EL to CL when deciding how to think about block production.

I don't think this change in isolation makes the API unwieldy in this sense but is something we should all be thinking about with future changes.

@rolfyone
Copy link

We've just had our last testnet in theory, is there a critical issue this is solving? Otherwise can it wait till next fork?

@lucassaldanha
Copy link
Contributor

I agree with @ralexstokes and @rolfyone views.

I am not against this change, but I do think that we should be mindful when adding "extra" or "non-critical" information on the interface, especially when the core protocol does not require it.

And the timing isn't great as we are all ready and set for Capella. Adding new non-critical changes atm to me only adds unnecessary risk to the fork.

@ajsutton
Copy link

You could also achieve the same kind of signalling that a local block should be used by giving it an absurdly high value. Not as conceptually clean obviously but would be an effective way to show these kinds of censorship detection are viable before we change the protocol to support them.

@LukaszRozmej
Copy link

Could we make it an optional field? So missing or null value would be considered false? Then we could add it at any time without the need to bump the version.

@potuz
Copy link
Contributor Author

potuz commented Mar 16, 2023

having an optional field was my first reaction, but @mkalinin mentioned that even if the field is optional we should bump the version since the semantics is different. I am not an API person, but since implementing this is absolutely trivial, I would want to see this before Cancun, and of course not necessarily right now. If there's a path for clients to ship this without a hardfork I would advocate for this and it seems that bumping the version is the easiest way to do that.

@LukaszRozmej
Copy link

LukaszRozmej commented Mar 17, 2023

Not if you make it optional in the shangai version :)

@mkalinin
Copy link
Collaborator

Included into the Cancun spec by #425

@mkalinin mkalinin closed this Jan 13, 2024
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.

8 participants