SSE for payload attributes #305
Conversation
|
Looks good, didn't mark approved, will let others take a look first so any discussions can happen... |
There was a problem hiding this comment.
Is the node expected to emit this event for the blocks its connected validators want to propose or for every slot?
What should suggested_fee_recipient be set too? If the previous question is true then the response should be agnostic of who is connected to the bn so spec it to set to 0 address always
|
@dapplion In my PoC implementation I've added a flag to cause payload attributes to be sent with every fcU. Without that flag Lighthouse would just send payload attributes for the blocks it is proposing. I don't think it matters what the fee recipient is set to as the builders should be ignoring that anyway. IMO the BNs driving block builders should probably be distinct from the ones used by validators but if someone did want to do this they could configure their real fee recipient in their BN config and use that for everything. In other words I think it would be overkill and less flexible to mandate setting it to 0. |
There was a problem hiding this comment.
@dapplion it should be pretty easy to add this event in the proposed (ssz json format) for on prepareNextSlot which we run in last 1/3 part of the slot to usually send a block building fcU (for next slot)
Adding a flag as suggested by @michaelsproul sounds reasonable. 👍
michaelsproul
force-pushed
the
payload-attributes-sse
branch
from
64815e2
to
57f46b4
Compare
| - `proposal_slot`: the slot at which a block using these payload attributes may be | ||
| built. | ||
| - `parent_block_root`: the beacon block root of the parent block to be built upon. | ||
| - `parent_block_number`: the execution block number of the parent block. | ||
| - `parent_block_hash`: the execution block hash of the parent block. | ||
| - `proposer_index`: the validator index of the proposer at `proposal_slot` on | ||
| the chain identified by `parent_block_root`. |
There was a problem hiding this comment.
is there a reason we have all of this data, instead of just the minimum required data?
following the lead of the builder APIs, we can uniquely identify a proposal with the tuple (proposal_slot, proposer_index, parent_block_hash)
which suggests we can drop parent_block_root and parent_block_number
There was a problem hiding this comment.
@ralexstokes I figured the block number is useful for stateless dummy builders that just return empty payloads. We use such a builder to run blockprint. It wouldn't be hard to track the block number but it would require separately looking up data from a CL/EL.
The parent block root is potentially interesting to differentiate the case where an identical payload ends up in two competing beacon blocks. Although arguably this case shouldn't be differentiated because the EL state is identical. Another reason to keep it is that it grounds the data in something CL-related, so if the receiver wants to look up more info from the beacon API they have a concrete reference point (a slot is ambiguous, and a payload hash isn't natively understood).
There was a problem hiding this comment.
makes sense, if everyone else thinks they pull their own weight then lets leave them in
Sounds reasonable to me. Please could you mention that in the spec? |
Closes #244.
Define a new server sent event (SSE) for payload attributes.
The difficult part of specifying this event was deciding on an encoding for the payload attributes. Payload attributes presently only exist in the
execution-specsrepo where they are always encoded withcamelCaseidents and big-endian hex integers. We have two options for encoding them here:execution-apisencoding and break consistency with the rest ofbeacon-apis.There are pros and cons in favour of each approach. Option (1) potentially minimises the number of encodings that clients need to maintain for the payload attributes data structure, which could be good for implementation complexity. On the other hand, the break in consistency might make it hard for implementations to use a different encoding for just these fields. After discussions with @rolfyone we came to the conclusion that the consistency of option (2) is preferable, and not difficult to implement in either Lighthouse or Teku. We would like feedback from other CL devs about their preference as well.
The way the events are currently specified is quite loose, so I've added some extra descriptive text to convey the choice of encoding as clearly as possible. Perhaps in future it would be good to clean up the events API so that the returned events are given full schemas in their own right.