-
Notifications
You must be signed in to change notification settings - Fork 169
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
Add block v3 endpoint. #339
Conversation
@@ -421,3 +424,13 @@ components: | |||
required: true | |||
schema: | |||
$ref: '#/components/schemas/ConsensusVersion' | |||
Eth-Blinded-Payload: | |||
description: Required in response so client can deserialize returned json or ssz data to the correct object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: Required in response so client can deserialize returned json or ssz data to the correct object. | |
description: Required in response so client can deserialize returned JSON or SSZ data to the correct object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing Eth-Consensus-Version
uses lower-case so I stuck with it for consistency.
apis/validator/block.v3.yaml
Outdated
Eth-Consensus-Version: | ||
$ref: '../../beacon-node-oapi.yaml#/components/headers/Eth-Consensus-Version' | ||
Eth-Blinded-Payload: | ||
$ref: '../../beacon-node-oapi.yaml#/components/headers/Eth-Blinded-Payload' | ||
Eth-Payload-Value: | ||
$ref: '../../beacon-node-oapi.yaml#/components/headers/Eth-Payload-Value' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Headers are required for SSZ returns. Should the added two headers be also present as JSON values to the application/json
response type? Otherwise, why include version as property but not the payload value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, wasn't sure about that. In the past we added the version to the JSON to allow for decoding, but it has pretty much been replaced with the header values now (as you don't need to do any mucking around with parsing streams to obtain the values from the headers). I'm not averse to putting these new values in the JSON metadata, but not sure if they add significant value any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we do something controversial and just return the json object not wrapped in a 'data' and have it just the json object or the ssz object determined by header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we do something controversial and just return the json object not wrapped in a 'data' and have it just the json object or the ssz object determined by header?
It would definitely break the standard from other routes and prevent allowing to add more fields in the future. I would mirror the header values in the JSON type for overall consistency. It's easier to parse a JSON reply than headers for client side tooling that deals with JSON only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dapplion yeah I'm thinking the same. If the metadata fields are in the JSON then it will allow parsing of the JSON separate from the HTTP response (e.g. if the JSON is saved and parsed later). I'll update accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. One question that rises from this is the naming of the fields: do we want to explicitly state which values are related to the execution payload, i.e.
blinded-execution-payload
rather thanblinded-payload
execution-payload-value
rather thanpayload-value
(and equivalent change for the header fields). Obviously this makes them a little more verbose, but is safer if we ever end up with other payloads in the beacon block. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we went that way, i'd probably suggest something like
execution-payload-value
execution-payload-blinded
i don't mind either way on whether we expand it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Minor nit on mirroring header values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM will just make sure others are happy...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't findproduceBlindedBlockV3
in PR, is this a mistake or intentional
ahh realized this is replacement for both, so then can be add another query param like blockSource ("builder", "execution" or "any")- can be also add
shouldOverrideBuilder
flag in response so that validator driven architectures of making the choice (like lodestar validator or may be vouch) can consume this info - can we also add the blockSource ("builder" or "execution") to the response apart from the query to reflect the selection here (especially in case of any), also gives a way to figure out that if the publishBlock call is binded to a particular beacon in case of blinded response
Not sure to what this refers. There is a
Beacon nodes will by default return the highest value block from either the relay(s) or the local payload. However, if the beacon node's circuit breaker is triggered or the execution client specifies that the local payload should be used then the local payload will be returned regardless of value. Does knowing why the full payload was returned (higher value / consensus circuit breaker / execution shouldOverrideBuilder) have any value here?
This can be inferred from the returned payload:
|
imo, this only needs to pass certain more flags around which are very small in data size and doesn't require any extra implementation work. |
|
shouldn't these be left to implementation and support the implementation types. again lodestar can transition to this paradigm but should be nice to have and support implementation diversity. |
Which flags are you after here? The one that was under discussion was to tell the beacon node to not override use of a relay payload with a local payload (when it had been informed to either from its own circuit breaker or from the execution API |
lodestar current impl needs all the flags i mentioned before, but may be we will change our impl to push the block selection to beacon node. but then atleast blockSource should be there in the response so that if the beacon node chooses blinded response even for executon block, (configured for e.g. via a startup flag for beacon node) then validator knows that the publish call is bound to the same beacon node. For lodestar this matters as we have sometimes seen latency for block communication between beacon and validator and have even lead to some miss scenaros |
required: true | ||
schema: | ||
type: boolean | ||
Eth-Execution-Payload-Value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the consensus value, there's no (reasonable) way for a VC to compare blocks for profit - should we add this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Nimbus already compute this value during block proposal? Lighthouse sort of does, but approximates slightly (if there are slashable attestations we don't keep track of the rewards from both).
It would make VC block choosing more fair, as the heuristics Vouch uses currently seem to favour Prysm despite Prysm's blocks being less profitable than Lighthouse and Nimbus on average (according to my blockprint nodes, public dash coming soon).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be a new PR? this one is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Nimbus already compute this value during block proposal?
We do it for offline analysis for historical blocks (to pick out more detail than is needed for state transitions) but making it "online" would probably not be too hard - indeed, the VC currently has to do a lot of guesswork and it will likely always be wrong / skewed - further exacerbating the problem is that in that guesswork, it's not working with eth-denominated data usually but rather "fresh attestation count" and similar proxies which prevents the value from being compared with the execution profit (ie low cl+high el vs low cl+high el score)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be a new PR? this one is closed.
As per #309 this adds a version 3
block
endpoint that consolidates the current blinded and unblinded endpoints. Features of this endpoint: