Skip to content

feat(deneb): proposer rpc to handle builder flow#12554

Merged
terencechain merged 6 commits intodeneb-integrationfrom
builder-rpc
Jun 26, 2023
Merged

feat(deneb): proposer rpc to handle builder flow#12554
terencechain merged 6 commits intodeneb-integrationfrom
builder-rpc

Conversation

@terencechain
Copy link
Collaborator

This PR updates the proposer RPC to handle builder flow for Deneb.

GetBeaconBlock highlights:

  • getBuilderPayload returns blindBlobsBundle to align symmetric with getLocalPayload
  • setKzgCommitments takes in blindBlobsBundle alongside blobsBundle and choose accordingly
  • new helper blindBlobsBundleToSidecars to convert blind bundles to blind sidecars
  • GenericBeaconBlock_BlindedDeneb gets returned to the validator client is the block is blind

ProposeBeaconBlock highlights:

  • newUnblinder takes in blinded blobs to call SubmitBlindBlock
  • new helper unblindBlobsSidecars to convert blind sidecars and full bundle to unblinded sidecars
  • unblindBuilderBlock returns unblinded sidecars

}
if len(b.Deneb.Blobs) > fieldparams.MaxBlobsPerBlock {
return nil, status.Errorf(codes.InvalidArgument, "Too many blobs in block: %d", len(b.Deneb.Blobs))
if wasBlinded {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there is a code smell around this, a lot of different arrays used, can't put my finger on how to clean it up just yet but some parts can be broken into its own function or cleaned up some way just for readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

slightly better: 2c8530e

@terencechain terencechain marked this pull request as ready for review June 23, 2023 19:01
@terencechain terencechain requested a review from a team as a code owner June 23, 2023 19:01
@terencechain terencechain requested review from kasey, prestonvanloon and rkapka and removed request for a team June 23, 2023 19:01
for i, blob := range b.Deneb.Blobs {
if err := vs.P2P.BroadcastBlob(ctx, blob.Message.Index, blob); err != nil {
log.WithError(err).Errorf("Could not broadcast blob index %d / %d", i, len(b.Deneb.Blobs))
sidecar := make([]*ethpb.BlobSidecar, len(scs))
Copy link
Contributor

@james-prysm james-prysm Jun 23, 2023

Choose a reason for hiding this comment

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

sidecar -> sidecars?

or maybe savableSidecars?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

blk, err = unblinder.unblindBuilderBlock(ctx)
blind := unblinder.b.IsBlinded() //

blk, sidecars, err := unblinder.unblindBuilderBlock(ctx)
Copy link
Contributor

@james-prysm james-prysm Jun 23, 2023

Choose a reason for hiding this comment

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

sidecars -> signedSidecars?
or hmm unblindedSignedSidecars?

a bit verbose but I think it can get rid of some of the comments and the blind check below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

f5f9478, went with unblindedSidecars. Didn't think signed convey anything extra here

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.

I think the logic makes sense, there's probably more improvements on readability and the creation of all those slices to do similar things has some kind of code smell to me, but I don't have great suggestions on improvement right now, I do think that maybe blinded variable can be removed in favor of checking if unblindedsidecars is not nil but that's not too much of an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants