Add head_v2 event and deprecate head event#590
Add head_v2 event and deprecate head event#590chong-he wants to merge 11 commits intoethereum:masterfrom
head_v2 event and deprecate head event#590Conversation
apis/eventstream/index.yaml
Outdated
| event: new_head | ||
| data: {"slot":"10", "block":"0x9a2fefd2fdb57f74993c7780ea5b9030d2897b615b89f808011ca5aebed54eaf", "state":"0x600e852a08c1200654ddf11025f1ceacb3c2e74bdd5c630cde0838b2591b69f9", "epoch_transition":false, "previous_epoch_dependent_root":"0x5e0043f107cb57913498fbf2f99ff55e730bf1e151f02f221e977c91a90a0e91", "current_epoch_dependent_root":"0x5e0043f107cb57913498fbf2f99ff55e730bf1e151f02f221e977c91a90a0e91", "next_epoch_dependent_root":"0x5e0043f107cb57913498fbf2f99ff55e730bf1e151f02f221e977c91a90a0e91", "execution_optimistic": false} |
There was a problem hiding this comment.
we should probably add version tag in this event @nflaig thoughts?
There was a problem hiding this comment.
I think Nico was pushing back on the version idea here:
I am not convinced to use {"version": "fork_name", "data": { ... }} everywhere, that is, if they don't emit data based on spec containers, or not fork specific, it feels a bit unnecessary. Although since we change the head event now, we never know. It's just not consistent with format of existing events. When we go with v2 at some point, then yes, maybe it makes sense to wrap all events in the version container.
Although I agree with you, we may as well version it for future proofing.
There was a problem hiding this comment.
not against adding a version here but in this case it's not trivial for a consumer to interpret it, while in the spec container case it's simple, you just pick the fork container based on version and use it to deserialize the data
while here, if we have to change the event in the future I'd expect we likely change the semantics of the event, not just the data format in which case a new event likely makes sense anyways
and clients should not break if new fields are added to the event, ie. #585 should be totally fine, if a client can't handle that, it needs to be fixed on that end imo
There was a problem hiding this comment.
the more I think about the version here, the more I dislike it, it's not really clear what it is "versioning" in that case because the data isn't really fork specific
There was a problem hiding this comment.
I am ok to remove the version. The response in data is not fork-dependent as the fields x_epoch_dependent_root is fixed since Fulu now. What do others think?
apis/eventstream/index.yaml
Outdated
| event: payload_attestation_message | ||
| data: {"version":"gloas", "data":{"validator_index": "123", "data": {"beacon_block_root": "0x9a2fefd2fdb57f74993c7780ea5b9030d2897b615b89f808011ca5aebed54eaf", "slot": "10", "payload_present": true, "blob_data_available": true}, "signature": "0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505"}} | ||
| new_head: | ||
| description: The node has finished processing, resulting in a new head. previous_epoch_dependent_root is `get_block_root_at_slot(state, compute_start_slot_at_epoch(epoch - 2) - 1)`, current_epoch_dependent_root is `get_block_root_at_slot(state, compute_start_slot_at_epoch(epoch - 1) - 1)` and next_epoch_dependent_root is `get_block_root_at_slot(state, compute_start_slot_at_epoch(epoch) - 1)`. All dependent roots use the genesis block root in the case of underflow. This `new_head` event is for post-Fulu slots. For pre-Fulu slots, use the `head` event. |
There was a problem hiding this comment.
do we wanna update the duty apis as part of this PR? currently their description still refers to the old head event
beacon-APIs/apis/validator/duties/attester.yaml
Lines 15 to 17 in b4abce4
There was a problem hiding this comment.
Thanks, I have made the changes similar to proposer.v2.yaml. But I am not sure if I am doing it right?
new_head eventhead_v2 event and deprecate head event
chong-he
left a comment
There was a problem hiding this comment.
Added some questions that I have
apis/validator/duties/attester.yaml
Outdated
|
|
||
| After Fulu: | ||
|
|
||
| - event.previous_epoch_dependent_root when `compute_epoch_at_slot(event.slot) == epoch` |
There was a problem hiding this comment.
I use the term previous_epoch_dependent_root here (instead of previous_duty_dependent_root) as I thought after Fulu it is "epoch", not "duty". Is this right? If so, maybe I can also update this part in proposer.v2.yaml:
Edit: Should it actually be current_epoch_dependent_root instead of previous_epoch_dependent_root for post-Fulu case?
There was a problem hiding this comment.
After discussed with @michaelsproul , we revise this part to make it clearer:
- Remove before Fulu / after Fulu as now the chain is at Fulu
- rename to use
current_epoch...instead ofcurrent_duty... - remove
event.block otherwisebullet point as it serves no purpose (and could be confusing) - changes made to:
attester.yaml,proposer.v2.yamlandptc.yaml
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
apis/eventstream/index.yaml
Outdated
| value: | | ||
| event: payload_attestation_message | ||
| data: {"version":"gloas", "data":{"validator_index": "123", "data": {"beacon_block_root": "0x9a2fefd2fdb57f74993c7780ea5b9030d2897b615b89f808011ca5aebed54eaf", "slot": "10", "payload_present": true, "blob_data_available": true}, "signature": "0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505"}} | ||
| data: {"version":"gloas", "data":{"validator_index": "123", "data": {"beacon_block_root": "0x9a2fefd2fdb57f74993c7780ea5b9030d2897b615b89f808011ca5aebed54eaf", "slot": "10", "payload_present": true, "blob_data_available": true}, "signature": "0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505"}} |
There was a problem hiding this comment.
Should we also take the opportunity to disambiguate the slot field? As far as I know, clients do different things with this field because it was never precisely specified whether it should be:
- The slot of the head block matching
beacon_block_root, or - The current wall-clock slot, or fork choice store slot
This makes it somewhat less useful for validator clients like Vouch/Vero, which might want to keep track of head block slots.
In head_v2 we could fix this with two fields:
head_block_slot: Slotwall_clock_slot: Slot
What do we think?
There was a problem hiding this comment.
To be clear, this is meant for the head_v2 event response: https://github.com/ethereum/beacon-APIs/pull/590/changes#diff-3cc6eaa800c1a4bd6adcd78f1722b466f7a24048a66e320c8f92872fbcd9eefbR63
There was a problem hiding this comment.
I was personally not aware of this ambiguity and assumed every CL client returned option 1 - "The slot of the head block matching beacon_block_root".
I don't see a reason to ever return a different slot there but I very well may be missing something. Quite the opposite, it seems like a bad idea to me to return dozens of head events with the same slot while syncing / catching up.
As far as I know, clients do different things
Can we have CL client teams confirm what they do? This may not even be an issue if every client already does option 1 right now, and we just need to clarify that in the spec.
There was a problem hiding this comment.
I would read that as head.slot so slot should definitely not be the wall clock slot, as for lodestar, we emit the slot of the head block
This PR adds a
new_headevent to the event stream as per discussion in #589