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

SignedMEVPayloadHeader mixes JSON-RPC and SSZ types #76

Closed
come-maiz opened this issue Mar 31, 2022 · 2 comments
Closed

SignedMEVPayloadHeader mixes JSON-RPC and SSZ types #76

come-maiz opened this issue Mar 31, 2022 · 2 comments

Comments

@come-maiz
Copy link
Contributor

tersec on Feb 2

Along the lines of the comments about SignedBlindedBeaconBlock (#69), but here, the JSON-RPC and SSZ types are freely intermixed, with the sort-of-SSZ-type-looking SignedMEVPayloadHeader containing a MEVPayloadHeader the fields of which are a Data and Quantity (JSON-RPC serialization types), which itself contains a ExecutionPayloadHeaderV1(a consensus layer SSZ type, defined using the SSZ-serialization-defined Hash32, 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 of MEVPayloadHeader (because the sort-of-SSZ-defined SignedMEVPayloadHeader 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-SSZ ExecutionPayloadHeaderV1.

For example, one might imagine the CL/SSZ MEVPayloadHeader:

class MEVPayloadHeader(Container):
payloadHeader: ExecutionPayloadHeaderV1
feeRecipient: ExecutionAddress
feeRecipientBalance: uint256

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.

@metachris
Copy link
Collaborator

metachris commented Apr 8, 2022

I think we should use SSZ for encoding all the block/header fields in builder_getHeaderV1 and builder_getPayloadV1, but not for the other regular params in the JSON-RPC. This is what SSZ is made for and what it's really good at.

For instance, setFeeRecipient should send it's fields just as regular JSON-RPC params, and not SSZ encoded.

ping @Ruteri @tersec @terencechain @lightclient

PS: Is there a reason why the header/block in the response for builder_getHeaderV1 and builder_getPayloadV1 is not explciitly SSZ encoded?

@metachris
Copy link
Collaborator

Closing this issue now - it's in the spec with the reasoning as follows (cc/ @lightclient):

  • We use SSZ encoding for the block data in the builder_getPayloadV1 request
  • The Engine API encodes everything as a JSON object, and to be consistent with that we use the same JSON objects in the responses as the EL client.
  • We use regular JSON-RPC params for regular requests, like builder_setFeeRecipientV1

If there's additional comments or questions, we can reopen :)

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

No branches or pull requests

2 participants