Skip to content

Prysm V4: SSE api adding payload_attributes#12102

Merged
terencechain merged 38 commits intodevelopfrom
payload-attribute-events-api
Mar 15, 2023
Merged

Prysm V4: SSE api adding payload_attributes#12102
terencechain merged 38 commits intodevelopfrom
payload-attribute-events-api

Conversation

@james-prysm
Copy link
Copy Markdown
Contributor

@james-prysm james-prysm commented Mar 8, 2023

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR adds the new payload_attributes topic to the EventStream, There are 2 versions of the payload at this time based on hardfork ( "bellatrix" and "capella"). The endpoint is intended to be used by builders and relays especially to retrieve withdrawal information. The event is triggered by new head events which should happen on every slot that has a canonical block. The results are not limited to validators you own but will show the information of every block proposer added as a new head.

Which issues(s) does this PR fix?

related #11841
Implements ethereum/beacon-APIs#305
Fixes ethereum/beacon-APIs#244

@james-prysm james-prysm changed the title Payload attribute events api Prysm V4: SSE api adding payload_attributes Mar 8, 2023
@james-prysm james-prysm marked this pull request as ready for review March 14, 2023 01:41
@james-prysm james-prysm requested a review from a team as a code owner March 14, 2023 01:41
@james-prysm james-prysm requested a review from rkapka March 14, 2023 16:10
Version: version.String(headState.Version()),
Data: &ethpb.EventPayloadAttributeV1_BasePayloadAttribute{
ProposerIndex: headBlock.Block().ProposerIndex(),
ProposalSlot: headState.Slot(),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've seen this as s.CurrentSlot +1 on others but since the headState is saved in this case i think it's headState.Slot()

Copy link
Copy Markdown
Collaborator

@terencechain terencechain Mar 14, 2023

Choose a reason for hiding this comment

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

headState.Slot() feels a little useless to me. I think it should be +1 but best to check with others

EDIT: this is wrong

Copy link
Copy Markdown
Collaborator

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

So close!


nextSlot := s.CurrentSlot() + 1 // Cache payload ID for next slot proposer.
hasAttr, attr, proposerId := s.getPayloadAttribute(ctx, arg.headState, nextSlot)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why remove these lines?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will add it back in, was reverting some changes removed extra lines

}
}

// notifyPayloadAttributesStream on successful FCU notify the event stream that a payload was sent
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably want to update this comment

return streamData(stream, PayloadAttributesTopic, &ethpb.EventPayloadAttributeV1{
Version: version.String(headState.Version()),
Data: &ethpb.EventPayloadAttributeV1_BasePayloadAttribute{
ProposerIndex: headBlock.Block().ProposerIndex(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this should be the proposer index of headState.slot + 1. We want to advance the state by 1 and use BeaconProposerIndex

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I checked the API again. This is what it says

 - `proposal_slot`: the slot at which a block using these payload attributes may be built.

Based on that, I'm taking it as headState.slot. Your current implementation is right

Version: version.String(headState.Version()),
Data: &ethpb.EventPayloadAttributeV1_BasePayloadAttribute{
ProposerIndex: headBlock.Block().ProposerIndex(),
ProposalSlot: headState.Slot(),
Copy link
Copy Markdown
Collaborator

@terencechain terencechain Mar 14, 2023

Choose a reason for hiding this comment

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

headState.Slot() feels a little useless to me. I think it should be +1 but best to check with others

EDIT: this is wrong

Copy link
Copy Markdown
Collaborator

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Lgtm! Nice work @james-prysm !

@terencechain terencechain merged commit b180a7d into develop Mar 15, 2023
@delete-merged-branch delete-merged-branch bot deleted the payload-attribute-events-api branch March 15, 2023 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSE subscription for newPayload event to trigger block building

2 participants