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

Implement bls_to_execution_changes beacon endpoint #11845

Merged
merged 9 commits into from
Jan 6, 2023

Conversation

potuz
Copy link
Contributor

@potuz potuz commented Jan 4, 2023

This PR implements the main logic for the REST endpoint https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/submitPoolBLSToExecutionChange.

It takes an array of signed BLS_TO_EXECUTION_CHANGE messages and validates their signature. Valid messages are then added to the local pool.

If the headstate is prior to the Capella fork, then the message is validated against the capella fork version, this allows users to fill their pool before the Capella fork happens.

If in addition the headstate is of version Capella, the beacon will broadcast these messages.

@potuz potuz changed the title Initial implementation Implement bls_to_execution_changes beacon endpoint Jan 4, 2023
@potuz potuz marked this pull request as ready for review January 4, 2023 19:22
@potuz potuz requested a review from a team as a code owner January 4, 2023 19:22
@potuz potuz requested review from saolyn, rkapka and james-prysm January 4, 2023 19:22
@potuz potuz added Ready For Review API Api related tasks labels Jan 4, 2023
beacon-chain/core/blocks/withdrawals.go Outdated Show resolved Hide resolved
req *http.Request,
) (apimiddleware.RunDefault, apimiddleware.ErrorJson) {
if _, ok := endpoint.PostRequest.(*SubmitBLSToExecutionChangesRequest); !ok {
return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return true, nil if !ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every other wrapper does this so I'll follow this pattern so I didn't want to change it, pinging @rkapka that may have had a reason. This can never happen anyway in runtime.

beacon-chain/rpc/eth/beacon/pool.go Outdated Show resolved Hide resolved
}
bs.BLSChangesPool.InsertBLSToExecChange(alphaChange)
if st.Version() >= version.Capella {
if err := bs.Broadcaster.Broadcast(ctx, alphaChange); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This broadcast feels a bit silent. I wonder if we can log something at the end that we successfully inserted indices x, y, z into the pool and broadcast them, but failed indices a, b, c and neither inserted nor broadcast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For our users we will provide a verbose version in prysmctl that will log everything detailed. A curl to the endpoint will only log on errors. I feel it's a good compromise cause otherwise the UX with prysmctl will log everything twice

err,
)
}
return nil, status.Errorf(codes.InvalidArgument, "One or more BLSToExecutionChange failed validation")
Copy link
Contributor

Choose a reason for hiding this comment

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

To be most informative, why don't we tell the user that all other indices not in the failure list succeeded and were included in the pool + broadcast to the network? People might think a failure could mean none of them succeeded. Can we included the indices in the error?

Copy link
Contributor

Choose a reason for hiding this comment

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

The nicest UX would be to tell the user the validator indices for which the failures correspond to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dito above, James' PR is the user facing end. This implements the backend only

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@rauljordan rauljordan added the Priority: High High priority item label Jan 5, 2023
@prylabs-bulldozer prylabs-bulldozer bot merged commit c1f0092 into develop Jan 6, 2023
@delete-merged-branch delete-merged-branch bot deleted the submit-bls-changes branch January 6, 2023 05:17
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.

3 participants