Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
milestone 1 spec #20
milestone 1 spec #20
Changes from all commits
7daaf94
a18f093
b95dc8c
4c9cd3e
977d487
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 JSON-RPC serialized, while
SignedBlindedBeaconBlock
is only specified here as an SSZ type. Aspects of SSZ type serialization aren't otherwise specified by the engine API spec on which this builds, such asSignedBlindedBeaconBlock.signature
andSignedMEVPayloadHeader.signature
(bothBLSSignature
s), leaving room for noninteroperability across implementations.https://github.com/ethereum/execution-apis/blob/v1.0.0-alpha.6/src/engine/specification.md#executionpayloadv1 provides an explicit mapping between a JSON-RPC-serialization of
engine.ExecutionPayloadV1
and SSZ representation ofbellatrix.ExecutionPayload
, where both are supposed to be otherwise isomorphic.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 agree this should be in JSON-RPC terms.
We could fully specify blocks here, similar to how
ExecutionPayloadV1
is defined by the execution API, but that might not be worth doing and maintaining because we already have blocks specified here, and I don't think we'd want to leave room for discrepancy between JSON serialization of the same type between the two APIs.I've raised a PR here to add Bellatrix types to the beacon API as well as endpoints and types necessary for blind transaction signing.
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.
Thanks @tersec and @realbigsean. I need help here to understand the proposal of using JSON-RPC for encoding these new methods. As I understand, SSZ is the new encoding for the consensus layer. I wonder why do we want to use the old encoding for the new things. Probably this has been discussed long ago, so please be patient with me :)
The way I imagine it, these APIs will be implemented by new software in the future, so these seems like a good way to start moving the future to be more consistent and easier to read. Am I being naive or missing some part of the story?
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.
SSZ is used as as the intra-consensus layer/inter-beacon-node serialization over the libp2p gossip network, yes.
JSON-RPC serializations appear in the REST API and related areas, e.g., validator clients mostly use JSON-RPC-type encodings to interact with beacon nodes. Both remain in use.
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.
Moved to #69 for tracking.
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.
Will MEV boost forward
relay_getPayloadHeaderV1
/relay_proposeBlindedBlockV1
requests it receives to the relay? The format of the spec here makes it look like these are available MEV boost endpoints, but I think they might be endpoints only the relay is required to serve.If these are only definitions for what the relay serves, I think it would make sense to have a clearer separation (in new section at the bottom here or maybe even splitting the relay API and mev-boost API into separate docs?)
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.
No, they shouldn't be forwarded. I agree, let's make a separate section to specity how the relayer is going to work.
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.
Along the lines of the comments about
SignedBlindedBeaconBlock
, but here, the JSON-RPC and SSZ types are freely intermixed, with the sort-of-SSZ-type-lookingSignedMEVPayloadHeader
containing aMEVPayloadHeader
the fields of which are aData
andQuantity
(JSON-RPC serialization types), which itself contains aExecutionPayloadHeaderV1
(a consensus layer SSZ type, defined using the SSZ-serialization-definedHash32
,ExecutionAddress
,Bytes32
,uint64
, etc types). It's not internally consistent.Implementing this might involve drawing out, at each layer, the dual JSON-RPC form of
SignedMEVPayloadHeader
, the dual SSZ form ofMEVPayloadHeader
(because the sort-of-SSZ-definedSignedMEVPayloadHeader
either has to be read as a purely JSON-RPC object, or if not, for all its component parts to have consensus layer-like definitions in addition to the purely JSON-RPC definitions here), and likewise but inverted for the nominally-natively-SSZExecutionPayloadHeaderV1
.For example, one might imagine the CL/SSZ
MEVPayloadHeader
:https://github.com/realbigsean/lighthouse/blob/mev-lighthouse/beacon_node/execution_layer/src/engine_api/json_structures.rs fills this out with some gaps, but none of it is directly specified and there are various points of ambiguity.
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.
oh what a mess. As I said before, I would like everything to be SSZ.
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.
Moved to #76 for traceability and better discussion.
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.
These references to the now-Bellatrix hardfork of the CL specs don't account for, e.g., supporting multiple CL MEV-compatible CL hardforks. While I'm not sure what the role of MEV in phase 0 or Altair might be, lacking transactions and all, the next hard fork, called the sharding fork pending whatever it's renamed to, will also support transactions.
The algorithm to derive the blinded types is the same, granted -- replace the execution payload with the execution payload header in the body, then propagate that back up the dependency chain -- but this creates ambiguous naming within a single global namespace as specified. In the context of MEV boosting, is a
BlindedBeaconBlock
(signed, body, etc) the Bellatrix or Sharding version?Of course, that won't be for a while, but it's an eventuality that will happen and it'll be smoother to anticipate it now than later.
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.
AFAIK fork awareness hasn't been spec'd out in the engine API, and since this should be an extension of that repo I don't think it should be defined here. We should just roll with the precedent set upstream, which I imagine will be similar to what is used by the beacon API.