Skip to content
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

Prysm V4: SSE api adding payload_attributes #12102

Merged
merged 38 commits into from
Mar 15, 2023

Conversation

james-prysm
Copy link
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 added Priority: High High priority item API Api related tasks labels 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
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
Member

@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
Member

@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!

@@ -70,7 +70,6 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *notifyForkcho

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

Copy link
Member

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
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

@@ -232,6 +248,83 @@ func handleStateEvents(
}
}

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

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
Member

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
Member

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

beacon-chain/rpc/eth/events/events.go Show resolved Hide resolved
Version: version.String(headState.Version()),
Data: &ethpb.EventPayloadAttributeV1_BasePayloadAttribute{
ProposerIndex: headBlock.Block().ProposerIndex(),
ProposalSlot: headState.Slot(),
Copy link
Member

@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
Member

@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
API Api related tasks Priority: High High priority item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSE subscription for newPayload event to trigger block building
2 participants