-
Notifications
You must be signed in to change notification settings - Fork 222
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
REST API produceBlockV3 implementation #5474
Conversation
ba293fe
to
0d4dcab
Compare
4018c4d
to
494d994
Compare
7ee3982
to
7d11f25
Compare
beacon_chain/spec/mev/deneb_mev.nim
Outdated
@@ -89,6 +89,11 @@ type | |||
signed_blinded_blob_sidecars*: | |||
List[SignedBlindedBlobSidecar, Limit MAX_BLOBS_PER_BLOCK] | |||
|
|||
BlindedBlockContents* = 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.
Would be good to either note as explicitly non-spec, or if it's derived from, e.g., a Beacon API requirement fairly directly (it's the type required by or returned by some REST endpoint), link to a stable, not master
/main
/etc URL for that.
writer.endRecord() | ||
writer.endRecord() | ||
|
||
proc readValue*(reader: var JsonReader[RestJson], |
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.
Why would the REST server ever have to read a BlindedBeaconBlock
it doesn't know the specific fork of already? It's for proposing, in the moment, and if it doesn't match the current slot/epoch it's not relevant to the node.
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.
This is not for REST server it is made for VC, because VC will use this readValue
procedure to decode produceBlockV3
response.
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 VC knows too though. It knows the fork schedule and if it's asking for a produceBlockV3
response during Capella, anything except a Capella response is simply an error.
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 REST library uses a single type for each API, so the Forked
type is needed here I think.
qslot, proposer, qrandao, qskip_randao_verification): | ||
return RestApiResponse.jsonError(Http400, InvalidRandaoRevealValue) | ||
|
||
(await node.makeBeaconBlockForHeadAndSlotV3(qrandao, qgraffiti, qhead, |
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.
It apparently needs ForkedAndBlindedBeaconBlock
here primarily because it's storing one as message
, but then it immediately does two things with this message
:
- calls
getHeaders
(defined above); and - calls
RestApiResponse.sszResponse
, not even on theForked
type per se, but on each specific case object field. See question there about whywriteValue
is defined onForkedAndBlindedBeaconBlock
, as a result.
But neither really requires storage of a forked/"any fork" version of a BlindedBeaconBlock
or BlindedBeaconBlockContents
, because what this produceBlockV3
endpoint returns is neither, but a bytestring of some sort (e.g., SSZ). So it's still unclear to me why have all this infrastructure for a ForkedAndBlindedBeaconBlock
which seems not to be necessary to begin with.
The result is hundreds of lines of
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.
I think you need some form of ForkedBlindedBeaconBlock
, because v3 may return either forked or non-forked depending on which one is better. And that is not known at call-time.
This, unless you want to do that logic inside the REST layer, which doesn't seem correct, because it makes it non-reusable, e.g., for the non-VC local block proposal flow.
Also having a Forked
type instead of forky MaybeBlindedBeaconBlock
makes it consistent with the existing v1/v2 makeBeaconBlockForHeadAndSlot
which also returns a ForkedBeaconBlock
.
consensusValue: Opt[UInt256] | ||
data: Opt[JsonString] | ||
|
||
for fieldName in readObjectFields(reader): |
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.
Is this entire loop, with its specifically handwritten multiple-field detection, etc, not basically equivalent to something like
type
RestForkedAndBlindedBeaconBlockSkeleton = object
version: Opt[string] # because it otherwise has to do a special conversion
blinded: Opt[bool]
executionValue: Opt[UInt256]
consensusValue: Opt[UInt256]
data: Opt[JsonString]
and then letting the JSON parser handle this, or would it not detect multiple fields with each name?
It seems like recreating logic that exists elsewhere.
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's also strictParse
for the other fields and so on.
7d11f25
to
75fedb9
Compare
More deser logic.
385507a
to
e4b0f08
Compare
Co-authored-by: Jacek Sieka <jacek@status.im>
No description provided.