-
Notifications
You must be signed in to change notification settings - Fork 175
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
blocksv3: add ignore_builders flag #377
Conversation
When this endpoint was originally proposed there was significant pushback from some client teams on allowing the validator client to state the source of the payload (builder/local), as it was considered that it was the job of the beacon node to decide if to use a relay or not. Hence there was no such control in the endpoint. As such, I would want to see agreement on this change from the client teams (and specifically what happens when |
Any client that hasn't implemented this would basically look the same as if they are ignoring the setting, it'd be confusing as a consumer to figure out what's going on, so i could imagine this being the source of several downstream bugs... I'm not saying we shouldn't do this, just fair warning is my intent... |
The ship hasn't sailed on v3 – we're still in the process of testing it on devnets. I think it would be OK to get clients to commit to supporting @mcdee Lighthouse VC (and maybe others) have supported this flow of some validators using builders and others not using builders for a long time, so I worry we'll break some people's setups if we don't add this flag. The workaround of having to run separate "builder BNs" and "local BNs" is kind of annoying, especially if you're running keys on behalf of another party and need to switch them around. I feel like Consensys Staking are probably using the per-validator builder config, can you confirm @jmcruz1983? |
i would like to propose an alternative and more comprehensive flag in the query params: export enum BuilderSelection {
BuilderAlways = "builderalways",
MaxProfit = "maxprofit",
/** Only activate builder flow for DVT block proposal protocols */
BuilderOnly = "builderonly",
/** Only builds execution block*/
ExecutionOnly = "executiononly",
}
cc @nflaig |
Interesting! I can see how these flags could be potentially useful. There are similar options at the beacon node level in Lighthouse, and I've used them for testing too. Although it feels like having this level of granularity via the API could add complexity, as clients may already have different options (e.g. prefer builder, profit threshold, max profit etc) and different ways of controlling payload selection at the BN level, and this adds more combinations and could lead to unexpected behaviour when using different BN/VC combinations. Do we always priortise the HTTP param over the BN configuration, or the other way around? |
As the purpose of this PR is to give the validator client more control over the payload selection I'd say HTTP param should be prioritised. |
I believe additional controls on the VC side, in general, provide a better user experience. The question is how much control makes sense, especially within the context of some of the drawbacks that were mentioned above. In the case where a client doesn't support a subset (or all) |
Please, no. If it's going into the standard we should have agreement from all client teams that they are happy implementing it. |
The I'm not fully across the builder flows to know if 'maxprofit' will work, but I've circulated the PR in the team to see if there's feedback...
|
In teku we have a comparison factor and we already have an issue to allow this configuration to be specified per-validator instead of being a beacon node configuration applied to all connected keys. https://docs.teku.consensys.io/reference/cli#builder-bid-compare-factor So if we generalize the We already support the |
The 1st part of this comment is what I started writing before I had what I think is a good idea (see 2nd section) thought process I'm trying to work out how to proceed on this. I can't make a compelling case for why
And then there's what I think is a good idea I think the comparison factor (a la Teku) could encode a lot of cases! If we let the comparison factor be
There is no equivalent for Thanks @tbenr for lighting the way! PS: using |
@michaelsproul how would this play with the circuit breakers currently within lighthouse? (BTW would be good to call it |
Thanks @mcdee, fixed! For the circuit breakers, I think these would need to remain configurable per-BN. Arguably they shouldn't be meddled with very often, as they're a liveness protection mechanism. Similarly, if some clients wanted to support DVT/ |
The problem then is that you end up with parameters you're sending to the beacon node that may or may not be ignored, depending on the state of the chain and the configuration of the beacon node. I'd be more than happy to have some way of controlling block production from the validator client, but only if parameters sent in with the request are honored. If they are advisory only, and each beacon node has its own set of rules as to what it actually does based on criteria that are invisible to the validator client (not just the beacon node, but also the state of the MEV relays and network connectivity to them), it feels like we're letting ourselves in for potential confusion down the line e.g. "I said I wanted to only use a builder execution payload (perhaps for OFAC), and it gave me a local one instead (with a proscribed transaction in it)". |
For users that really care about this, there's still a way to achieve it without per-validator config for these BN-specific parameters. E.g. in Lighthouse you could run the BN with
I'm not sure what you meant by mentioning network connectivity here, but I think it's a salient example. Even if we try to cede as much control as possible to the VC, the BN is still going to return a local block if e.g. the MEV relay completely times out or fails to produce a block. I think given we can't give 100% control to the VC, then best-effort measures are OK.
For users that want this they can enforce it on the VC side as a last line of defence, i.e. reject all non-builder blocks returned from the v3 API. Although I personally would not support this feature in the Lighthouse VC, because it harms the network. |
To be clear, I agree with the situation at current where a beacon node will build a local block if it deems it the best option to do so (e.g. circuit breaker activated). I worry more about the fact that this is best-efforts isn't communicated clearly.
Agreed, which is why I'm wary of having a parameter that suggests that one thing will happen (e.g. only builder-built block) when in reality something different may happen. Perhaps the solution to my angsting is simply to rename the parameter to something that makes it clear that this is a preference or hint, as opposed to a constraint. |
Ah I see where you're coming from! My proposal is to just add the "comparison factor" to the existing API, which would avoid any certain language (like "always") in the BN spec. We could document that this parameter will be used by the BN if it is possible & deemed safe to propose either of the local or builder blocks. The VCs could then use less certain language as well like |
i think if a beacon doesn't support any of the options, they should error so that validator knows if the options have been respected or not, and the onus is on the one running the validator to make sure bns he/she is running with honor the options he has specified. this allows one to have desired behavior w.r.t. what validator wants and what a beacon client currently supports |
i like comparison factor to reduce the options space and could just be supported with addition of a new enum: so we can say some values each client have to support, and some values as optional ( e.g. |
Is there a use-case for |
That would be no fun for any large-scale operation that deals with multiple beacon nodes that can be individually upgraded.
That doesn't work when the beacon node has situational switches (e.g. circuit breakers). And having different beacon nodes work or not work with different parameters in the spec would be a significant pain. If this is going to be part of the standard then all beacon nodes should support it. |
If the client teams all agree to implement this, then sure. |
I'm in favour of having only one param (compare factor) and leave the other options on the BN side. @michaelsproul we apply the factor to he So encoding of If this clients haven't implemented a similar comparison factor, maybe we can go this way so we don't have to do any internal conversion (which may be confusing) |
i still insist of specifying some optional values, those who want to can implement it
yes obol does this regarding circuit breaker: again leave to the specific beacon implementations to throw or simply do a builder proposal on builder_only, but the beacon should be expected to follow what its been asked for.
correct but those maintaining multi BN infra are very capable of handling such complications so that really isn't an issue imo |
I'm strongly against this. Making implementation optional means it is very hard to trust the API servers to do one thing or another. Parameters should be well-defined, and agreed upon by all client teams, otherwise we don't have a standard spec. |
standardization is already failing hard on this particular flag imo, atleast allowing the interface could be standardized if not the implementations as clients will effectively need to deviate away from the standard. |
+1 to having well-defined param values and have all clients respect them, so we don't run into inconsistency and interop issues when running with different BN/VC combinations. I think this would be important for DVT as well, if they could be communicating with multiple different BNs? |
If this flag is |
I think we're getting closer to a consensus decision.
The remaining point of contention is @g11tech Is there any reason why DVT validators can't use The alternative that I see is having to define multiple query parameters, either |
There are a lot of configuration options put on the beacon node itself, I'd like to not see more options added to this API. we might have disagreements on the intended flows, as APIs may conflict with beacon node settings, it took a while to reach consensus on the states we get into for keymanager APIs and I feel like this is similar. |
the only diff is that the api should fail in case there is no builder block and musn't return with local. this is important for DVT because they all should get the same block to sign or not. This behavior isn't really hard to implement imo and should be supported.
I am for e.g. running solo + DVT from the same BNs (on holesky yet, but also looking to replicate on mainnet as well if things go well) and hence beacon level turning off doesnt work.
yes any of these would work, though i would prefer the later solution as it seems more elegant to me.
I agree and we should incorporate the new use cases like DVT because standardization paves way for client diversity
i think this is a good idea and one can fail/error So I am fine with local block always being produced for determining censorship but still the block production needs to follow the semantics of the passed |
I don't agree with this. One of the reasons for the metadata in the v3 response is that it provides enough information for the validator client to decide if it wants to work with the returned data or not. If the validator client requests a blinded block but the consensus node does not want to provide it (if, for example, as in potuz's example the execution node has detected censorship) then it makes sense for the beacon node to return a locally built payload. There is very little downside in doing so (time to encode and shift the data over the wire) and the benefit is that the payload is available if the validator client wishes to use it. The validator client will easily be able to understand from the metadata that regardless of the fact that it wanted a payload from the builder the consensus node was only willing to provide one from the local execution node, so if it truly doesn't want to build a block it can error out itself. I think that the principles we have here are:
These both seem reasonable to me, is there any disagreement here? If not then these should be able to guide what the API does. |
The problem with this is that you assume that a locally built block will produce a non-blind response. To me there is no such strong correlation, infact teku is currently returning blinded only due to current limitations (we will improve soon). In any case a BN could theoretically have an option to return blinded only for whatever reason. |
That was already part of the discussion for v3, and should be in the spec. Otherwise we're relying on the beacon node retaining state to allow the validator client to produce the block. |
I get the stateless-stateful argument, but if local MUST be returned as unblinded, than I think we should rename from "blinded" to "builder" or similar. But that's off topic. |
The only concern I have is that, if the builder flow is used for DVT flows, it doesn't mean that censorship check are not done down the line. |
From what I read in this thread it seems that the fundamental point of contention here is whether the VC or the BN should decide the contents of the block. Given my stance up there about not allowing the easier bypassing of the anti-censorship defaults, I am of course on the camp that the BN should have the final say here. |
I am fine with BN having a final say, but in the case that BN can't honor
yes thats correct |
Technically, doesn't the beacon node just provide a suggestion on what to sign and publish? The validator has the keys and hence the final say on that, it can get the payload from multiple BNs or even the builder directly. For example, the validator client can already prevent builder blocks by not calling registerValidator on the beacon node. As far as I am aware, all implementations only call this API if builder is explicitly enabled (via This implicitly addresses the problem statement of this PR
A validator client without builder enabled will only produce local blocks as builder payload requests will always fail. Considering this, I think it might make it even more important to have the HTTP param to indicate to the beacon node that it should not wait (and delay block proposal) for a builder response as it will fail anyways due to missing registration. Alternatively, the beacon node could keep track of registered pubkeys and skip builder blocks for validators that never registered. This might be problematic if a multiple BNs are used and introducing an additional cache is probably not desired. |
I am also against mandating that BN can't be configured to revert with a blinded version of a local block because most of the small/solo setups have validators just hooked to 1 BN and can potential leverage on latency if blinded versions are send for signing to the validators. This might be more meaningful post deneb where there are blobs (keep in mind that max num blobs per block might blow up in future HFs) So I would like that BNs should be free to provide implementations where they can respond with blinded even for the full block by supporting an explicit flag to do this (BN side, optional to implement) |
What is the rationale behind doing this and not returning a locally produced block, clearly stated as such? We're trying to build blocks here, after all. I don't see what the downside of giving a local block if the beacon node has built it. The validator client can still decide not to use it, if it really doesn't want to. |
DVTs' proposals seem run in two ways:
So for 2, they need a deterministic output (given a matching set of builders) |
We're not just building for DVT, but for DVT and option 2 above they can do the following:
Returning a locally built payload does not impact this workflow. |
The nimbus folks were pretty strongly in favour of making the process as stateless as possible. And any system that uses multiple beacon nodes would fail if beacon nodes didn't return the full info. |
only if explicitly configured by a flag on beacon blinded response would be returned. Again the one who is hooking the validator to BNs is supposed to know BN configurations, so by default it would be stateless only |
yes thats one hack, but again we also want to be able to configure the beacon to respond a local block in blinded version if configured that way. I don't understand the opposition to throwing error on a query value |
I don't see this as limiting flexibility, it seems to be adding an additional parameter (and complexity) for no gain. In your example:
In my example:
The only difference in the DVT validator client is that its error path kicks in when it doesn't receive a blinded block, rather than when it receives an error. And we lose the requirement for the additional |
in that case lets just the rename executionPayloadBlinded to executionPayloadBuilder in the response to mean what it really is so that we can extend the API if need be to support sending blinded versions of local block in future if we want it to be. |
I agree with this. As said previously, I don't like that we are associating a data format (blind or unblind) with the source of the data (locally or externally generated). We can enforce it to be the same thing in the spec but I think it is not a good design. adding a |
I've made a PR for the I haven't added |
yeah this PR is redundant if we agree on #386 |
closing this in favor of #386 |
The new blocksv3 endpoint doesn't provide validator clients with the ability to express wether to fetch a block from the builder relay or the local execution client. For example, the Lighthouse VC has a
builder-proposal
flag that, when set, will always attempt to fetch a block from the builder relay. Since the LH VC uses the blocksv2 endpoints, the flag just simply decides wether to attempt to fetch a block from the blinded payload endpoint or the full payload endpoint. With the consolidation of these two endpoints into the new blocksv3 endpoint, validators no longer have the ability to choose between fetching from the builder relay or the local execution client. If a user were to run multiple VCs and wanted some to use the builder relay and others to use the local execution client, they would be forced to run multiple BNs.This PR suggests a new query param to the blocksv3 endpoint that, when set to true, will always ignore block builders and attempt to fetch blocks from the local execution client.
Thanks @jimmygchen & Lighthouse team for raising this issue