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

Simplify spec, update based on recent discussions #82

Merged
merged 11 commits into from Apr 8, 2022

Conversation

lightclient
Copy link
Collaborator

@lightclient lightclient commented Apr 4, 2022

I took a stab at updating the spec based on our recent discussions about the spec. I need to update sequence diagram and the process overview, but I think it's relatively straightforward.

A few notes:

  • I removed the concept of builder vs. relay methods and combined them. The CL can ignore things like feeRecipientDiff and signatures, or it can use that info to compare against it's local payload. I think this really simplifies things a lot.
  • I decided to go with SSZ encoding the block because it simplifies the spec a lot and is already the form that the builder will need it in to validate the signature.

docs/specification.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #82 (53140d3) into main (e166756) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #82   +/-   ##
=======================================
  Coverage   59.48%   59.48%           
=======================================
  Files           8        8           
  Lines         701      701           
=======================================
  Hits          417      417           
  Misses        221      221           
  Partials       63       63           
Flag Coverage Δ
unittests 59.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e166756...53140d3. Read the comment docs.

@metachris
Copy link
Collaborator

Thanks for putting this together @lightclient! Good stuff in there. I'd love to have setFeeRecipient in the launch version, and +1 on SSZ.

I think there's going to be a bit more discussion about removing builder/relay separation, because having a relay prefix allows for a bit more flexibility on iteration.

Will follow up tomorrow.

docs/specification.md Outdated Show resolved Hide resolved
@come-maiz
Copy link
Contributor

I think there's going to be a bit more discussion about removing builder/relay separation, because having a relay prefix allows for a bit more flexibility on iteration.

I think the relay concept is outdated, we can get rid of it. mev-boost now requests full blocks to a set of builders, and chooses the most profitable.
Or mev-boost could talk to only one builder aggregator that sends a single proposal. This aggregator can conform to the same API of a single builder.

We can move forward this way, and if the relay concept becomes relevant we reintroduce it. I agree with @lightclient that this clarifies concepts and simplifies the specification. @metachris I wonder what kind of iteration do you have in mind here.

@metachris
Copy link
Collaborator

I like this proposal and am on board with it.

Side-question: I wonder if mev-boost should just poll the relays always for headers, so it can return a payload very quickly when the slot has arrived (the relay could deliver a header for every fork choice even a few blocks deep).

@realbigsean
Copy link
Contributor

realbigsean commented Apr 6, 2022

In order for mev-boost to have a role in the reputation management of relays/builders (in my view they're essentially one in the same), won't we still need an equivalent to the relay namespace? Because there will be information required by mev-boost related to reputation management that the CL should never need from mev-boost

docs/specification.md Show resolved Hide resolved
docs/specification.md Outdated Show resolved Hide resolved
- feeRecipientDiff: Quantity, 256 Bits - the change in balance of the feeRecipient address
- method: `builder_getHeaderV1`
- params:
1. `hash`: `DATA`, 32 Bytes - Hash of block which the validator intends to use as the parent for its proposal.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the validator public key be part of the request params or else how does the builder return Unknown validator if the validator public key has not been mapped to a feeRecipient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation was that the builder would know what validator is planning to propose for the current slot and would return Unknown validator if it had not received a setFeeRecipient message from the validator. I will update the wording here, because "recovered" doesn't make much sense in this context.

This does make me wonder if the builder should also return the slot it is building for in case there is a mismatch?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could skip slots on top of hash

hash -> request/propose
hash -> skipped_slot -> request/propoase
hash -> skipped_slot -> skipped_slot -> request/propose

The builder may utilize the current wall block but if there's a tiny clock synchronization mismatch between the BN and builder then it may fall apart. It may be safer to include an additional slot or proposer_index or pub_key in the request as an additional anchor point

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@terencechain thanks for the brainstorming on this. the PR is now merged, please open an issue/PR to continue the thread, if applicable 🙏

docs/specification.md Outdated Show resolved Hide resolved
docs/specification.md Outdated Show resolved Hide resolved
Comment on lines +186 to +187
1. `block`: `DATA`, arbitray length - SSZ encoded [`BlindBeaconBlock`](#blindbeaconblock).
2. `signature`: `DATA`, 96 Bytes - BLS signature of the validator over `block`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to pass in SignedBlindBeaconBlock which is basically 1. + 2.

Have we considered adding publicKey as an input? We don't need it if the builder has the beacon state and the ability to look up the block. proposer_index's public key

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in this comment, I am making the assumption that builders have the ability to look up the block.proposer_index's public key. I think this is a reasonable and important assumption, because without it, the builder may receive may builder_getHeader requests with different feeRecipients, which will cause them to calculate blocks that have no possibility of being included.

I'm not necessarily opposed to passing SignedBlindBeaconBlock - the rationale for not doing so currently is it breaks from the current pattern of signature always being an RPC parameter / response and I'm not sure what the value would be in disrupting that pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. I think block and signature are great. Let's see if others have feedback on this

Author: lightclient <lightclient@protonmail.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>
@lightclient
Copy link
Collaborator Author

lightclient commented Apr 7, 2022

In order for mev-boost to have a role in the reputation management of relays/builders (in my view they're essentially one in the same), won't we still need an equivalent to the relay namespace? Because there will be information required by mev-boost related to reputation management that the CL should never need from mev-boost

@realbigsean: could you give an example? So far, I'm trying to specify each method in way that makes faults attributable (the thorny unattributable behavior being the builder withholding data). From there, my understanding is that mev-boost clients will need to gossip this information amongst themselves, so it wouldn't be a direct communication between CL<>mev-boost or builder<>mev-boost and therefore does not necessitate a method.

@realbigsean
Copy link
Contributor

realbigsean commented Apr 7, 2022

From there, my understanding is that mev-boost clients will need to gossip this information amongst themselves, so it wouldn't be a direct communication between CL<>mev-boost or builder<>mev-boost and therefore does not necessitate a method.

Yes I'm thinking about how exactly instances of mev-boost can communicate issues to each other. I think a medium term goal would be to include something like fraud proofs into consensus layer p2p ... but as far as how this works at the merge, are there plans for mev-boost to implement some type of p2p communication?

I was under the impression there would be something similar to what was described here #34
Where relays/builders are relaying information about fraud proofs as well. It would work by mev-boost broadcasting fraud proofs to all relays/builders, and relays/builder making fraud proofs available to any other connected mev-boost clients. This would be reliant on relays/builders policing each other, I doubt an individual relay/builder would make available fraud proofs for payloads it produced

@metachris metachris merged commit 18d89c4 into flashbots:main Apr 8, 2022
@metachris
Copy link
Collaborator

Merged this as new spec v0.2! Thanks for all the contributions ⭐

Let's move any remaining and further discussions to separate issues and PRs.

@sambacha
Copy link

sambacha commented Apr 9, 2022

From there, my understanding is that mev-boost clients will need to gossip this information amongst themselves, so it wouldn't be a direct communication between CL<>mev-boost or builder<>mev-boost and therefore does not necessitate a method.

Yes I'm thinking about how exactly instances of mev-boost can communicate issues to each other. I think a medium term goal would be to include something like fraud proofs into consensus layer p2p ... but as far as how this works at the merge, are there plans for mev-boost to implement some type of p2p communication?

I was under the impression there would be something similar to what was described here #34 Where relays/builders are relaying information about fraud proofs as well. It would work by mev-boost broadcasting fraud proofs to all relays/builders, and relays/builder making fraud proofs available to any other connected mev-boost clients. This would be reliant on relays/builders policing each other, I doubt an individual relay/builder would make available fraud proofs for payloads it produced

Couldn't the attnets attribute in the node's ENR be used for initially determining if there is an issue with the node? Since the value of the attnets attribute is a bitvector showing what subnet the node is subscribed to, faulty nodes would show up, etc etc?

screwyprof pushed a commit to screwyprof/mev-boost that referenced this pull request Feb 3, 2023
* simplify spec, update based on recent discussions

* should be BeaconBlockHeader not BeaconBlock

* add setFeeRecipient and spec out the signing operations

* fix spacing

* switch back to using BeaconBlock, tidying up

* add public key to methods to verify BLS signatures

* note that builder should respond to get payload requests immediately

* add timestamp to fee recipient announcement

* do some renaming, add a diagram, add in definiton for blind beacon block

* fix malformed link

* address review feedback

Author: lightclient <lightclient@protonmail.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>

Co-authored-by: terence tsao <terence@prysmaticlabs.com>
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.

None yet

9 participants