Add head_v2 event in beacon API and remove some fields in data_column_sidecar event - #9597
Conversation
chong-he
changed the title
heav_v2 event in beacon API and remove some fields in data_column_sidecar eventhead_v2 event in beacon API and remove some fields in data_column_sidecar event
|
This pull request has merge conflicts. Could you please resolve them @chong-he? 🙏 |
mergify
Bot
added
waiting-on-author
mergify
Bot
added
ready-for-review
pawanjay176
left a comment
pawanjay176
left a comment
There was a problem hiding this comment.
Sorry this one took a while. Looks mostly good, just one small issue and a suggestion
| pub slot: Slot, | ||
| pub block: Hash256, | ||
| pub state: Hash256, | ||
| pub payload_status: PayloadStatus, |
There was a problem hiding this comment.
The spec seems to require the payload_status to be in lowercase. Maybe we can just do a serde container attribute for making it lowercase so that the json shows
{payload_status: "empty"} instead of Empty ?
There was a problem hiding this comment.
Great catch, I added #[serde(rename_all = "lowercase")] in the enum: 3742b57
| match (current_epoch_dependent_root, next_epoch_dependent_root) { | ||
| (Ok(current_epoch_dependent_root), Ok(next_epoch_dependent_root)) => { | ||
| let head_v2 = SseHeadV2 { | ||
| slot: state.slot(), |
There was a problem hiding this comment.
Maybe its more correct to choose the slot from new_snapshot.beacon_block.slot()?
pawanjay176
left a comment
pawanjay176
left a comment
There was a problem hiding this comment.
I'm happy to merge this and iterate now.
Although, I'm a bit confused on if the head events are also supposed to handle reorg notifications. Seems like we didn't do it for Head v1 sse events, and now we are doing it only for Empty -> Full status changes for the same beacon block root.
The spec seems kinda ambiguous to me because there is also a chain-reorg event. Can someone with better knowledge of the beacon api clarify the intent for the head event? Is it supposed to handle reorgs too?
|
Queued — the merge queue status continues in this comment ↓. |
This is clarified recently in this PR: ethereum/beacon-APIs#628 Only the first change of |
Merge Queue Status
This pull request spent 29 minutes 52 seconds in the queue, including 27 minutes 51 seconds running CI. Required conditions to merge
|
2 participants
Issue Addressed
#9045
Proposed Changes
head_v2eventkzg_commitmentsandversioned_hashesindata_column_sidecarThe other new task for the events API that has yet to be implemented include:
add(update: implemented in Implement Fast Confirmation Rule #8951 )fast_confirmationevent, pending as it seems that Lighthouse still doesn't implement thisRemoved parent_block_number field in payload_attributes eventis to be done after Gloas, as highlighted below, because current (Fulu) PayloadAttributes still hasparent_block_numberlighthouse/common/eth2/src/types.rs
Lines 1157 to 1159 in 81d5769
AI-assisted, self-reviewed