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

Use v1alpha1 server in block production #12336

Merged
merged 17 commits into from
May 4, 2023
Merged

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Apr 26, 2023

What type of PR is this?

Other

What does this PR do? Why is it needed?

Inspired by #12319, this PR uses v1alpha1 server for block production and uses a mock server in tests.

@rkapka rkapka added the API Api related tasks label Apr 26, 2023
@rkapka rkapka requested a review from a team as a code owner April 26, 2023 17:30
@rkapka rkapka requested review from kasey, terencechain and nisdas April 26, 2023 17:30
@rkapka rkapka added the Ready For Review A pull request ready for code review label Apr 26, 2023
}

return bs.submitBlock(ctx, root, wrappedCapellaBlk)
return nil
}

func (bs *Server) submitBlock(ctx context.Context, blockRoot [fieldparams.RootLength]byte, block interfaces.ReadOnlySignedBeaconBlock) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change I think SubmitBlockSSZ and SubmitBlindedBlockSSZ can be addressed and have submit block just removed and just use 1 such as propose beacon block. will be less logic all over the place.
that being said I wonder if the unblind block logic is fine. at first glance it seems so and that seems to be the biggest difference between the submit block and proposeGenericBeaconBlock aside from the fact that the broadcaster is part is referenced from a different object.

if err != nil {
return status.Errorf(codes.InvalidArgument, "Could not tree hash block: %v", err)
return status.Errorf(codes.Internal, "Could not propose block: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be more custom if we are allowing for invalid argument code... otherwise, it'll return a 500 error when it should have been status.Errorf(codes.InvalidArgument, "Could not prepare block").
this will apply for all the other error handling logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The v1alpha1 server returns a simple error, so it's hard to figure out when to return what kind of error.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we still use error contains or error is and have some of the errors handled returning the appropriate error code?

like this return status.Errorf(codes.InvalidArgument, "Could not prepare block")

if err != nil {
return &emptypb.Empty{}, status.Errorf(codes.Internal, "Could not get proto block: %v", err)
}
_, err = bs.V1Alpha1ValidatorServer.ProposeBeaconBlock(ctx, &eth.GenericSignedBeaconBlock{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When processing SSZ, I avoid calling the helper submitXXX functions because I already have a v1alpha1 block, and they require a v1/v2 block that they convert to v1alpha1. So that means two unnecessary conversions.

@rkapka rkapka force-pushed the v1alpha1-server-submit-block branch from bac45f9 to caad354 Compare May 4, 2023 14:47
Copy link
Contributor

@james-prysm james-prysm left a comment

Choose a reason for hiding this comment

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

LGTM thanks for adding the additional fixes

@rkapka rkapka merged commit bd833e1 into develop May 4, 2023
@rkapka rkapka deleted the v1alpha1-server-submit-block branch May 4, 2023 17:52
rkapka added a commit that referenced this pull request May 5, 2023
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>

# Conflicts:
#	beacon-chain/rpc/eth/beacon/BUILD.bazel
#	beacon-chain/rpc/eth/beacon/blinded_blocks_test.go
#	beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go
terencechain pushed a commit that referenced this pull request May 5, 2023
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
terencechain pushed a commit that referenced this pull request May 5, 2023
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
terencechain pushed a commit that referenced this pull request May 5, 2023
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants