-
Notifications
You must be signed in to change notification settings - Fork 220
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #82 +/- ##
=======================================
Coverage 59.48% 59.48%
=======================================
Files 8 8
Lines 701 701
=======================================
Hits 417 417
Misses 221 221
Partials 63 63
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Thanks for putting this together @lightclient! Good stuff in there. I'd love to have 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. |
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. 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. |
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). |
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 |
- 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. |
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.
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
?
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.
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?
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 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
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.
@terencechain thanks for the brainstorming on this. the PR is now merged, please open an issue/PR to continue the thread, if applicable 🙏
1. `block`: `DATA`, arbitray length - SSZ encoded [`BlindBeaconBlock`](#blindbeaconblock). | ||
2. `signature`: `DATA`, 96 Bytes - BLS signature of the validator over `block`. |
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.
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
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.
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 feeRecipient
s, 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.
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 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>
@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 |
Yes I'm thinking about how exactly instances of I was under the impression there would be something similar to what was described here #34 |
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. |
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? |
* 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>
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:
builder
vs.relay
methods and combined them. The CL can ignore things likefeeRecipientDiff
and signatures, or it can use that info to compare against it's local payload. I think this really simplifies things a lot.