New prysm get block RPC #11834

terencechain · 2022-12-30T22:48:56Z

What type of PR is this?

Rewrite of GetBlock Prysm RPC

What does this PR do? Why is it needed?

This PR rewrites GetBlock Prysm RPC to be better, cleaner, and more extensive. In the end, we were able to delete much of the "spaghetti" code. This piece of the code is already live in capella branch and utilized in all the Capella and eip-4844 testnets. This is the original PR which documents the workflow: #11721

I'm still working on more unit tests so please hold off on the detailed reviews

…poser-rpc

terencechain · 2023-01-04T22:09:14Z

I am marking it as blocked. I want to test this with mev-boost

…o new-proposer-rpc

potuz · 2023-01-06T12:41:04Z

beacon-chain/rpc/eth/validator/validator.go

-	if err != nil {
-		return nil, status.Errorf(codes.Internal, "Could not prepare blind beacon block: %v", err)
-	}
-	if !ok {
 		return nil, status.Error(codes.Unavailable, "Builder is not available due to miss-config or circuit breaker")


Is this error right now?

This is a beacon API for retrieving blind blocks (only). By the beacon API definition, you must return a valid blind block or fail. Very different than our current implementation used by Prysm API

I mean are you sure that the only failures are miss-config or circuit breaker in this path?

The error msg is wrong. I'll update

beacon-chain/rpc/eth/validator/validator_test.go

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

potuz · 2023-01-06T12:54:16Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

 		if err != nil {
-			return nil, status.Errorf(codes.Internal, "Could not fetch Altair beacon block: %v", err)
+			log.WithError(err).Error("Could not pack deposits and attestations")


Same here, do we need to set the empty deposits and attestations here or the marshaller takes care of it automatically?

I think this one is fine since those are not vectors but I added anyway

potuz · 2023-01-06T13:01:45Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

-	return vs.getBellatrixBeaconBlock(ctx, req)
+	// Set bls to execution change. New in Capella.
+	if err := vs.setBlsToExecData(blk, head); err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not set bls to execution data: %v", err)


This is a non-blocking error, a block is valid with an empty bls-to-exec-changes slice.

Nice catch!

potuz · 2023-01-06T13:07:30Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_altair.go

-		return nil, fmt.Errorf("could not build block data: %v", err)
+func (vs *Server) setSyncAggregate(ctx context.Context, blk interfaces.BeaconBlock) {
+	slot := blk.Slot()
+	if slot == 0 || slots.ToEpoch(slot) < params.BeaconConfig().AltairForkEpoch {


I feel there's something wrong here. For networks that start with AltairForkEpoch == 0 this should not fail. The problem is the slot - 1 statement below. In any case, we should never reach this point as there's no proposing on slot 0.

Ok, maybe a cleaner check should be based on block version

potuz · 2023-01-06T13:12:26Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_altair.go

+			if err := blk.Body().SetSyncAggregate(&ethpb.SyncAggregate{
+				SyncCommitteeBits:      make([]byte, params.BeaconConfig().SyncCommitteeSize),
+				SyncCommitteeSignature: make([]byte, fieldparams.BLSSignatureLength),


I don't think this is a valid empty sync committee signature. Take a look a my helper functions on testing/util for an example on how to sign with the point at infinity in the BLS curve.

Nice catch!

potuz · 2023-01-06T13:26:10Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go

+)
+
+// Sets the bls to exec data for a block.
+func (vs *Server) setBlsToExecData(blk interfaces.BeaconBlock, headState state.BeaconState) error {


I think it's better to deal with the error case withing this function. That is this function does not return an error, and if the call to SetBLSToExecutionChanges errors out, we set the empty slice here.

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go

potuz · 2023-01-06T21:46:15Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+			blk.Body().SetDeposits([]*ethpb.Deposit{})
+			blk.Body().SetAttestations([]*ethpb.Attestation{})


These need to go outside of the if/else block and remove the else block

potuz · 2023-01-06T21:52:05Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go

 		log.WithError(err).Error("Could not get bls to execution changes")
 	} else {
 		if err := blk.Body().SetBLSToExecutionChanges(changes); err != nil {
+			if err := blk.Body().SetBLSToExecutionChanges([]*ethpb.SignedBLSToExecutionChange{}); err != nil {
+				log.WithError(err).Error("Could not set bls to execution data in block")


I think part of the requests in my feedback was to remove the error return in this function, so that we don't need to be doing this if/else in multiple places.

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go

…poser-rpc

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_altair.go

…o new-proposer-rpc

terencechain added 12 commits December 27, 2022 13:09

Revamp proposer rpc

5bfb952

Add canUseBuilder helper method

b557ee1

Merge branch 'can-use-builder' into new-proposer-rpc

f5aad1d

Can build. Need to fix minimal setting

ab8b0d3

Fix build.bazel for v1alpha1 rpcs

b72323f

Merge branch 'fix-proposer-rpc-build-file' into new-proposer-rpc

6d9df33

Rm unused code

a14b667

Rm old tests

a8986b9

Add back tests

2851145

Sync with develop

a48b75c

Fix more tests

9a0aa6e

Gazelle

e298d35

terencechain self-assigned this Dec 30, 2022

terencechain requested a review from a team as a code owner December 30, 2022 22:48

terencechain requested review from saolyn, potuz and nisdas December 30, 2022 22:48

terencechain added 2 commits January 4, 2023 13:29

Merge branch 'develop' of github.com:prysmaticlabs/prysm into new-pro…

aa4ac54

…poser-rpc

Add capella test

144b800

terencechain force-pushed the new-proposer-rpc branch from 61abc7c to 144b800 Compare January 4, 2023 22:06

terencechain added Ready For Review A pull request ready for code review Withdrawals Priority: High High priority item Blocked Blocked by research or external factors and removed Ready For Review A pull request ready for code review labels Jan 4, 2023

terencechain added 4 commits January 4, 2023 14:10

Use metric

a29bef8

Use timeout

a1e86bf

Go fmt

2e038fa

Disable transition cache

933e920

terencechain added 3 commits January 5, 2023 07:34

Fix return

f380656

Merge branch 'new-proposer-rpc' of github.com:prysmaticlabs/prysm int…

edc24be

…o new-proposer-rpc

Fix test

4891145

potuz requested changes Jan 6, 2023

View reviewed changes

Potuz feedback

4756bbe

potuz reviewed Jan 6, 2023

View reviewed changes

terencechain added 7 commits January 6, 2023 14:23

Fix bls to exec data

d608353

Move deposit and att outside of else block

3ad693e

Fix err msg

daa1104

Rm elses

d0d5cde

Better reconstruct error msg when EL client is syncing

a40c2f9

Merge branch 'better-reconstruct-msg' into new-proposer-rpc

13baabf

Tested and worked

dd883df

potuz requested changes Jan 9, 2023

View reviewed changes

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go Show resolved Hide resolved

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go Show resolved Hide resolved

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go Show resolved Hide resolved

terencechain added 5 commits January 11, 2023 07:42

Potuz feedback

dd591bb

Merge branch 'develop' of github.com:prysmaticlabs/prysm into new-pro…

9361785

…poser-rpc

Rm

e2c59cc

Fix test

8291ed8

Merge branch 'develop' into new-proposer-rpc

455197d

potuz requested changes Jan 13, 2023

View reviewed changes

beacon-chain/rpc/prysm/v1alpha1/validator/proposer_altair.go Show resolved Hide resolved

terencechain added 5 commits January 13, 2023 13:46

Sync aggregate empty case

44b6099

Merge branch 'develop' into new-proposer-rpc

a2f1137

Dont set it

ac95af3

Merge branch 'new-proposer-rpc' of github.com:prysmaticlabs/prysm int…

142cf8d

…o new-proposer-rpc

Refactor to set empty within the error

575e523

terencechain force-pushed the new-proposer-rpc branch from e05915f to 575e523 Compare January 13, 2023 22:36

potuz approved these changes Jan 13, 2023

View reviewed changes

terencechain added the OK to merge label Jan 13, 2023

terencechain merged commit 1b2d917 into develop Jan 13, 2023

delete-merged-branch bot deleted the new-proposer-rpc branch January 13, 2023 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New prysm get block RPC #11834

New prysm get block RPC #11834

terencechain commented Dec 30, 2022

terencechain commented Jan 4, 2023

potuz Jan 6, 2023

terencechain Jan 6, 2023

potuz Jan 6, 2023

terencechain Jan 6, 2023

potuz Jan 6, 2023

terencechain Jan 6, 2023 •

edited

Loading

potuz Jan 6, 2023

terencechain Jan 6, 2023

potuz Jan 6, 2023

terencechain Jan 6, 2023

potuz Jan 6, 2023

terencechain Jan 6, 2023

potuz Jan 6, 2023

terencechain Jan 6, 2023

potuz Jan 6, 2023

potuz Jan 6, 2023

		blk.Body().SetDeposits([]*ethpb.Deposit{})
		blk.Body().SetAttestations([]*ethpb.Attestation{})

New prysm get block RPC #11834

New prysm get block RPC #11834

Conversation

terencechain commented Dec 30, 2022

terencechain commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain Jan 6, 2023 •

edited

Loading