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

milestone 1 spec #20

Closed
wants to merge 5 commits into from
Closed

milestone 1 spec #20

wants to merge 5 commits into from

Conversation

thegostep
Copy link
Contributor

No description provided.

docs/milestone-1.md Show resolved Hide resolved
docs/milestone-1.md Outdated Show resolved Hide resolved
docs/milestone-1.md Outdated Show resolved Hide resolved
@thegostep thegostep marked this pull request as ready for review January 15, 2022 13:34
docs/milestone-1.md Show resolved Hide resolved
docs/milestone-1.md Show resolved Hide resolved

- method: `builder_proposeBlindedBlockV1`
- params:
1. [`SignedBlindedBeaconBlock`](#signedblindedbeaconblock)
Copy link

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 as SignedBlindedBeaconBlock.signature and SignedMEVPayloadHeader.signature (both BLSSignatures), 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 of bellatrix.ExecutionPayload, where both are supposed to be otherwise isomorphic.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link

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.

Copy link
Contributor

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.


### Types

#### SignedMEVPayloadHeader
Copy link

@tersec tersec Feb 2, 2022

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-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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@tersec
Copy link

tersec commented Feb 2, 2022

Still points to CL specs 1.1.6 and engine API alpha.5. v1.1.9 and alpha.6 don't seem to impinge directly too much on this, as the SignedBeaconBlock aspect in particular isn't changed.


#### BlindedBeaconBlock

This is forked from [here](https://github.com/ethereum/consensus-specs/blob/v1.1.6/specs/phase0/beacon-chain.md#beaconblock) with `body` replaced with `BlindedBeaconBlockBody`
Copy link

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.

Copy link
Contributor

@realbigsean realbigsean Feb 25, 2022

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.


Technically, this call only needs to return the `transactions` field of [`ExecutionPayloadV1`](https://github.com/ethereum/consensus-specs/blob/v1.1.6/specs/merge/beacon-chain.md#executionpayload), but we return the full payload for simplicity.

### relay_getPayloadHeaderV1
Copy link
Contributor

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?)

Copy link
Contributor

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.

@come-maiz
Copy link
Contributor

@terencechain @realbigsean @tersec I'm sorry for the long time without a reply. We were reorganizing the team to make sure that we could maintain this for the merge.
Now @thegostep @metachris @jparyani and me will be working on this, first getting the milestone 1 tested on kiln.

Thanks a lot for your reviews and feedback. Some things are very straightforward and we are working on those. For some others I'll have more questions as I get to understand more details about the specification.

bors bot pushed a commit to sigp/lighthouse that referenced this pull request Mar 31, 2022
## Issue Addressed

MEV boost compatibility

## Proposed Changes

See #2987

## Additional Info

This is blocked on the stabilization of a couple specs, [here](ethereum/beacon-APIs#194) and [here](flashbots/mev-boost#20).

Additional TODO's and outstanding questions

- [ ] MEV boost JWT Auth
- [ ] Will `builder_proposeBlindedBlock` return the revealed payload for the BN to propogate
- [ ] Should we remove `private-tx-proposals` flag and communicate BN <> VC with blinded blocks by default once these endpoints enter the beacon-API's repo? This simplifies merge transition logic. 

Co-authored-by: realbigsean <[email protected]>
Co-authored-by: realbigsean <[email protected]>
@come-maiz
Copy link
Contributor

We have cleaned a milestone1 specification in https://github.com/flashbots/mev-boost/blob/main/docs/specification.md and all the pending discussions are now in the issues. Please check and let me know if I missed any concerns, to make sure we are not dropping anything important. Moving forward, we should have one issue and one PR per concern, so it's easier to focus.

Thanks for work and the nice discussion everybody. I'm closing this one.

@come-maiz come-maiz closed this Mar 31, 2022
@come-maiz come-maiz deleted the thegostep/docs branch March 31, 2022 18:02
paulhauner pushed a commit to paulhauner/lighthouse that referenced this pull request Apr 4, 2022
## Issue Addressed

MEV boost compatibility

## Proposed Changes

See sigp#2987

## Additional Info

This is blocked on the stabilization of a couple specs, [here](ethereum/beacon-APIs#194) and [here](flashbots/mev-boost#20).

Additional TODO's and outstanding questions

- [ ] MEV boost JWT Auth
- [ ] Will `builder_proposeBlindedBlock` return the revealed payload for the BN to propogate
- [ ] Should we remove `private-tx-proposals` flag and communicate BN <> VC with blinded blocks by default once these endpoints enter the beacon-API's repo? This simplifies merge transition logic. 

Co-authored-by: realbigsean <[email protected]>
Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit to paulhauner/lighthouse that referenced this pull request May 6, 2022
## Issue Addressed

MEV boost compatibility

## Proposed Changes

See sigp#2987

## Additional Info

This is blocked on the stabilization of a couple specs, [here](ethereum/beacon-APIs#194) and [here](flashbots/mev-boost#20).

Additional TODO's and outstanding questions

- [ ] MEV boost JWT Auth
- [ ] Will `builder_proposeBlindedBlock` return the revealed payload for the BN to propogate
- [ ] Should we remove `private-tx-proposals` flag and communicate BN <> VC with blinded blocks by default once these endpoints enter the beacon-API's repo? This simplifies merge transition logic. 

Co-authored-by: realbigsean <[email protected]>
Co-authored-by: realbigsean <[email protected]>
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

Successfully merging this pull request may close these issues.

6 participants