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

Scope shouldOverrideBuilder flag for Cancun #425

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

mkalinin
Copy link
Collaborator

Successor of #395 suggesting to scope shouldOverrideBuilder flag for Cancun.

@mcdee
Copy link

mcdee commented Jul 1, 2023

Apparently there was a comment in ACD that this proposal allows greater control by validators, but I'm not sure that I see how that is the case. Please could someone expand a little on the rationale behind this flag, and how it would work as part of the builder flow?

@mkalinin
Copy link
Collaborator Author

mkalinin commented Jul 1, 2023

@mcdee here is the original thread #388

This is the way for EL to make a suggestion to use locally built payload in the case when according to some heuristics (which are not strictly specified as we favour client diversity here) censorship is detected. CL is free to ignore this flag even if it's set to true, also, CL can employ its own heuristics to detect censorship.

All the above is up to implementation. If EL doesn't implement any heuristics it should simply return false all the time.

@mcdee
Copy link

mcdee commented Jul 1, 2023

Sure, but there was a comment about greater control by validators. I don't see where the validator is even informed about what's going on here, let along able to increase its control, with this flag.

The block building process where a local execution payload is required by the validator is already pretty well-defined, in that the validator client asks for a block proposal. The block building process where a remote block is requested is also pretty well-defined, in that the validator client asks for a blinded block proposal. But validators don't have control, in that if the beacon node decides that it wants to use a locally-generated payload and put it in a blinded block proposal it is free to do so and the validator client is unaware that this has occurred.

This proposal appears to be a way of informing the CL of the EL's current view of potential censorship on the network. If this is the case, would it not make sense for this to be a separate call that can be made to the EL by either the beacon node or the validator client to find out the state of the network, so that the validator can act accordingly?

@mkalinin
Copy link
Collaborator Author

mkalinin commented Jul 1, 2023

If this is the case, would it not make sense for this to be a separate call that can be made to the EL by either the beacon node or the validator client to find out the state of the network, so that the validator can act accordingly?

Do we have any restrictions that the current proposal does have vs a separate method, or clear benefits of the latter? Separate method requires addition engineering complexity which needs justification.

@mcdee
Copy link

mcdee commented Jul 1, 2023

It pretty much boils down to control. At current a validator client can ask for and receive a block with a local execution payload by requesting a block proposal. However, if asking for a blinded block it has no control of (or visibility into) the provenance of the execution payload (header) returned. Ultimately this means that validator clients do not have control of the execution payload of the block they wish to publish, regardless of what they have told the beacon node. If this is the desired result then it would be good to make this explicit, as there was a comment on the last ACD to the effect of this being a change that increased control for validators.

As for the idea of a separate method: with the current proposal the CL cannot know if the EL has detected censorship without requesting the payload. I am assuming that censorship detection is an on-going process, and as such would be faster to return than building and returning the complete payload. With the current suggestion I believe the flow of the CL that wanted to use this flag when being asked for a blinded block would be roughly as follows:

  1. CL is asked for a blinded block
  2. CL fetches execution payload header from MEV relay / MEV boost
  3. CL fetches execution payload from locally attached EL
  4. EL receives flag from EL and returns either payload accordingly

The concern is that the CL will wait for item 3 to complete even if that is significantly after item 2 has completed, resulting in the return of the payload to the VC being possibly delayed. With the recent changes made to the blinded block path that has resulted in later blocks and more orphans, would this potentially further increase the delay for block publication?

This is where the idea of a separate API call comes from: it can be called out of the critical path (for example 1s before the end of each slot) so that the CL can be aware, and then if asked for a blinded block can either go to the MEV relay or the locally attached EL, but not be required to do both (at least, in the happy path).

@mkalinin
Copy link
Collaborator Author

mkalinin commented Jul 3, 2023

The concern is that the CL will wait for item 3 to complete even if that is significantly after item 2 has completed

I don't think this is true. In a normal case CL should request its local EL for a payload at the beginning of a slot after it has confirmed that the head hasn't been changed since the latest build request. I would assume that (2) and (3) happens roughly at the same time. The usage of a new flag should be quite similar to already existing blockValue in that regard. cc @potuz

At current a validator client can ask for and receive a block with a local execution payload by requesting a block proposal.

According to this proposal a VC won't have access to this flag and can only defer the decision making to BN or ignore existence of this flag entirely. Do we have a VC that would benefit from the direct access to this information? Maybe Vouch?

@mcdee
Copy link

mcdee commented Jul 3, 2023

I would assume that (2) and (3) happens roughly at the same time.

They would start at roughly the same time, but 2) could complete well ahead of 3), which would be the concern.

Do we have a VC that would benefit from the direct access to this information? Maybe Vouch?

Very much so. If Vouch knows that the EL believes that transactions are being censored it could avoid a chunk of time-consuming work. At current it has to go through a relatively painful loop of "fetch bids from relays, ask beacon nodes for blocks, find out that beacon nodes aren't using relay execution payloads" before going back to beacon nodes to ask for unblinded blocks because it's only in the third step (when Vouch received blinded blocks from beacon nodes with execution payload headers that don't match what it was expecting) that it (implicitly) finds out the status of this flag.

@mkalinin
Copy link
Collaborator Author

mkalinin commented Jul 3, 2023

They would start at roughly the same time, but 2) could complete well ahead of 3), which would be the concern.

In a normal case fcU triggering a build process is sent seconds in advance to a slot where validator is proposing a block and then when payload gets requested by the call to getPayload EL instantly responds with a built payload it has upon request.

@potuz
Copy link
Contributor

potuz commented Jul 3, 2023

They would start at roughly the same time, but 2) could complete well ahead of 3), which would be the concern.

do you have data on this? our testing shows this never ever happens. Even less so now that relayers have an extra delay in returning the payload to the proposer while the local EL returns one immediately.

Regardless of this, at least Prysm (and presumably other CLs as well) will still wait for the local block to compare values for example.

@mcdee
Copy link

mcdee commented Jul 3, 2023

do you have data on this?

Nope.

...our testing shows this never ever happens. Even less so now that relayers have an extra delay in returning the payload to the proposer while the local EL returns one immediately.

This is a different bit of the process. This is the request to the relay to obtain the execution payload header, not to unblind the block, so before the signing process starts.

@potuz
Copy link
Contributor

potuz commented Jul 3, 2023

This is a different bit of the process. This is the request to the relay to obtain the execution payload header, not to unblind the block, so before the signing process starts.

Right, but still the delay in computing the state root is always orders of magnitude more than the EL providing the block.

@tbenr
Copy link

tbenr commented Jul 6, 2023

The concern is that the CL will wait for item 3 to complete even if that is significantly after item 2 has completed,

I think most clients are currently already waiting for local EL before accepting the Builder Bid. The reason is the withdrawals root validation.
Edit: and also for doing value comparison as already stated.

@terencechain
Copy link
Contributor

One thing that will be good to have is when EL returns true, also log something out could be a warning or debug verbosity on why the reason true was returned. CL won't really be able to explain anything except "we got an override signal" from EL and surely operators will ask for more reasoning

@mkalinin
Copy link
Collaborator Author

mkalinin commented Jul 7, 2023

One thing that will be good to have is when EL returns true, also log something out could be a warning or debug verbosity on why the reason true was returned. CL won't really be able to explain anything except "we got an override signal" from EL and surely operators will ask for more reasoning

Do you mean conveying e.g. the name of triggered heuristic to CL when true is returned?

@terencechain
Copy link
Contributor

One thing that will be good to have is when EL returns true, also log something out could be a warning or debug verbosity on why the reason true was returned. CL won't really be able to explain anything except "we got an override signal" from EL and surely operators will ask for more reasoning

Do you mean conveying e.g. the name of triggered heuristic to CL when true is returned?

I don't think it needs to send a string for reason, but certainly EL can log something for greater visibility. I don't think that's something to specified in the spec, but just making a mental note

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.

5 participants