builder: deneb and handling blobs #12477

james-prysm · 2023-05-30T20:47:22Z

What type of PR is this?
Feature

What does this PR do? Why is it needed?

Introduces deneb block and blobs to builder

implements changes from
ethereum/builder-specs#80
ethereum/builder-specs#79
ethereum/builder-specs#61

ethereum/consensus-specs#3391
ethereum/consensus-specs#3392

part of #12248

james-prysm · 2023-06-01T21:18:24Z

beacon-chain/builder/service.go

-		return nil, ErrNoBuilder
+		return nil, nil, ErrNoBuilder
+	}
+	if blobs != nil && uint64(len(blobs)) > fieldparams.MaxBlobsPerBlock {


Do i need this check here?

I dont think you need blobs != nil here

james-prysm · 2023-06-01T21:21:29Z

api/client/builder/client.go

+			return nil, nil, errors.Wrapf(err, "could not get protobuf block")
+		}
+
+		b := &ethpb.SignedBlindedBeaconBlockAndBlobsDeneb{Block: psb, Blobs: blobs}


any additional checks needed for blobs here?

I think we are good here

terencechain · 2023-06-05T19:38:52Z

api/client/builder/bid.go

+// Signature --
+func (b signedBuilderBidDeneb) Signature() []byte {
+	return b.p.Signature
+}
+
+// Version --
+func (b signedBuilderBidDeneb) Version() int {
+	return version.Deneb
+}
+
+// IsNil --
+func (b signedBuilderBidDeneb) IsNil() bool {
+	return b.p == nil
+}


let's move these before or after builderBidDeneb methods

terencechain · 2023-06-05T19:46:22Z

api/client/builder/client.go

+			return nil, nil, errors.Wrapf(err, "could not get protobuf block")
+		}
+
+		b := &ethpb.SignedBlindedBeaconBlockAndBlobsDeneb{Block: psb, Blobs: blobs}


I think we are good here

terencechain · 2023-06-05T19:50:22Z

api/client/builder/types.go

+	blobs := make([][]byte, len(b.Blobs))
+	for i := range b.Blobs {
+		blobs[i] = b.Blobs[i]
+	}


I'd probably check the length here similar to the above fieldparams.BlobLength

terencechain · 2023-06-05T19:50:57Z

api/client/builder/types.go

+func (b BlobsBundle) ToProto() (*v1.BlobsBundle, error) {
+	commitments := make([][]byte, len(b.Commitments))
+	for i := range b.Commitments {
+		if len(b.Commitments[i]) != 48 {


Replace 48 with fieldparams.BLSPubkeyLength

terencechain · 2023-06-05T19:52:02Z

api/client/builder/types_test.go

+			name:     "ExecPayloadResponse.ExecutionPayload.BlockHash",
+		},
+		{
+			expected: "1",


DataGasUsed and ExcessDataGas should be using different values to avoid mix up

terencechain · 2023-06-05T19:52:26Z

api/client/builder/types_test.go

+		DataGasUsed:   1,
+		ExcessDataGas: 1,


Let's use different values here

terencechain · 2023-06-05T19:52:43Z

api/client/builder/types_test.go

+		DataGasUsed:      1,
+		ExcessDataGas:    1,


Let's use different values here

terencechain · 2023-06-05T19:55:45Z

beacon-chain/builder/service.go

-		return nil, ErrNoBuilder
+		return nil, nil, ErrNoBuilder
+	}
+	if blobs != nil && uint64(len(blobs)) > fieldparams.MaxBlobsPerBlock {


I dont think you need blobs != nil here

rkapka

We should make safe copies when converting to proto objects

rkapka · 2023-06-12T15:16:49Z

api/client/builder/bid.go

+
+// Header --
+func (b builderBidDeneb) Header() (interfaces.ExecutionData, error) {
+	if b.p == nil {


You should either verify b.p in all getters or skip it everywhere. Since we error out in the constructor when wrapping a nil object, I would just remove it.

rkapka · 2023-06-12T15:19:30Z

api/client/builder/bid.go

+		return nil, errors.New("builder bid is nil")
+	}
+	// We have to convert big endian to little endian because the value is coming from the execution layer.
+	v := big.NewInt(0).SetBytes(bytesutil.ReverseByteOrder(b.p.Value))


I would extract math.WeiToGwei(big.NewInt(0).SetBytes(bytesutil.ReverseByteOrder(b.p.Value))) to a function and reuse it for Capella/Deneb/etc.

api/client/builder/client.go

rkapka · 2023-06-12T15:31:53Z

api/client/builder/types.go

+	return payload, bundle, nil
+}
+
+func (p *ExecutionPayloadDeneb) ToProto() (*v1.ExecutionPayloadDeneb, error) {


We make unsafe copies everywhere

proto/engine/v1/execution_engine.proto

testing/util/blob.go

rkapka · 2023-06-12T15:37:18Z

testing/util/merge.go

@@ -43,6 +43,10 @@ func NewBlindedBeaconBlockDeneb() *ethpb.SignedBlindedBeaconBlockDeneb {
 	return HydrateSignedBlindedBeaconBlockDeneb(&ethpb.SignedBlindedBeaconBlockDeneb{})
 }

+func NewBlindedBlobSidecar() *ethpb.SignedBlindedBlobSidecar {


godoc missing

terencechain

It's not entirely clear to me on why we need to copy. To be clear, I'm not against it but I will like to under why. We have similar implementations that we don't copy so there seems to be a miss alignment. Ex: consensus-types/blocks/execution.go

sorry, wrong PR

rkapka · 2023-06-14T15:56:00Z

testing/middleware/builder/builder.go

@@ -369,9 +369,11 @@ func (p *Builder) handleHeadeRequestCapella(w http.ResponseWriter) {
 		return
 	}
 	v := big.NewInt(0).SetBytes(bytesutil.ReverseByteOrder(b.Value))
+	// used for testing bid values


This comment doesn't really explain what's special about multiplying by 2. I think adding Nishant's explanation will be better:

we set the payload value as twice its actual one so that it always chooses builder payloads vs local payloads

rkapka · 2023-06-14T15:59:50Z

math/math_helper.go

+// Wei is the smallest unit of Ether, and sent in the payload value of the execution engine API call.
+// represented as a pointer to a bigInt.


Suggested change

// Wei is the smallest unit of Ether, and sent in the payload value of the execution engine API call.

// represented as a pointer to a bigInt.

// Wei is the smallest unit of Ether, represented as a pointer to a bigInt.

I don't think a type definition should specify a particular use case, although it's a nitpick really.

james-prysm changed the title ~~adding builder changes to handle blobs~~ builder: deneb and handling blobs May 31, 2023

terencechain force-pushed the deneb-integration branch from f42b326 to 0c7f3cc Compare May 31, 2023 18:39

james-prysm force-pushed the builder-deneb branch 2 times, most recently from 7a66220 to c3925b7 Compare June 1, 2023 03:59

james-prysm commented Jun 1, 2023

View reviewed changes

james-prysm force-pushed the builder-deneb branch from 153380c to 4216dd8 Compare June 5, 2023 16:45

james-prysm marked this pull request as ready for review June 5, 2023 16:59

james-prysm requested a review from a team as a code owner June 5, 2023 16:59

james-prysm requested review from prestonvanloon, saolyn, potuz and terencechain and removed request for a team June 5, 2023 16:59

terencechain reviewed Jun 5, 2023

View reviewed changes

james-prysm force-pushed the builder-deneb branch 3 times, most recently from 25b25cc to ae02dcd Compare June 5, 2023 21:30

james-prysm requested a review from rkapka June 6, 2023 14:43

terencechain force-pushed the deneb-integration branch from ccf3258 to ab9623e Compare June 7, 2023 15:27

james-prysm force-pushed the builder-deneb branch from 4fd3bef to bd554c4 Compare June 7, 2023 21:23

rkapka requested changes Jun 12, 2023

View reviewed changes

terencechain force-pushed the deneb-integration branch 2 times, most recently from 69b60fc to ceb1ad3 Compare June 12, 2023 19:43

james-prysm force-pushed the builder-deneb branch from 7b92a6c to fbaafd9 Compare June 12, 2023 20:04

terencechain previously requested changes Jun 13, 2023

View reviewed changes

james-prysm requested a review from rkapka June 13, 2023 15:19

rkapka reviewed Jun 14, 2023

View reviewed changes

introducing deneb changes and blobs to builder

6b0a45f

james-prysm force-pushed the builder-deneb branch from f67e3bb to 6b0a45f Compare June 14, 2023 16:40

rkapka approved these changes Jun 14, 2023

View reviewed changes

james-prysm merged commit 4ba782a into deneb-integration Jun 14, 2023

james-prysm deleted the builder-deneb branch June 14, 2023 18:04

terencechain pushed a commit that referenced this pull request Jun 16, 2023

introducing deneb changes and blobs to builder (#12477)

4c2d08e

terencechain pushed a commit that referenced this pull request Jun 27, 2023

introducing deneb changes and blobs to builder (#12477)

fa138e1

terencechain pushed a commit that referenced this pull request Jul 7, 2023

introducing deneb changes and blobs to builder (#12477)

17b66ad

terencechain pushed a commit that referenced this pull request Jul 9, 2023

introducing deneb changes and blobs to builder (#12477)

3dbd01f

kasey pushed a commit that referenced this pull request Jul 20, 2023

introducing deneb changes and blobs to builder (#12477)

a12555f

james-prysm added a commit that referenced this pull request Aug 4, 2023

introducing deneb changes and blobs to builder (#12477)

1aa670f

terencechain pushed a commit that referenced this pull request Aug 16, 2023

introducing deneb changes and blobs to builder (#12477)

91d5715

kasey pushed a commit that referenced this pull request Aug 21, 2023

introducing deneb changes and blobs to builder (#12477)

63d97e2

kasey pushed a commit that referenced this pull request Aug 22, 2023

introducing deneb changes and blobs to builder (#12477)

d067685

kasey pushed a commit that referenced this pull request Aug 22, 2023

introducing deneb changes and blobs to builder (#12477)

c965168

kasey pushed a commit that referenced this pull request Aug 22, 2023

introducing deneb changes and blobs to builder (#12477)

fe64709

kasey pushed a commit that referenced this pull request Aug 23, 2023

introducing deneb changes and blobs to builder (#12477)

a906d6c

kasey pushed a commit that referenced this pull request Aug 23, 2023

introducing deneb changes and blobs to builder (#12477)

898e143

kasey pushed a commit that referenced this pull request Aug 23, 2023

introducing deneb changes and blobs to builder (#12477)

ec25b4e

kasey pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

b51d50b

kasey pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

d9ae767

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

48c664a

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

45af7c2

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

dd93e5e

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

17abd3c

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

0dfb820

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

introducing deneb changes and blobs to builder (#12477)

98000a5

prestonvanloon pushed a commit that referenced this pull request Aug 30, 2023

introducing deneb changes and blobs to builder (#12477)

6903178

prestonvanloon pushed a commit that referenced this pull request Aug 30, 2023

introducing deneb changes and blobs to builder (#12477)

94bb078

prestonvanloon pushed a commit that referenced this pull request Aug 31, 2023

introducing deneb changes and blobs to builder (#12477)

8a67f61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder: deneb and handling blobs #12477

builder: deneb and handling blobs #12477

james-prysm commented May 30, 2023 •

edited

Loading

james-prysm Jun 1, 2023

terencechain Jun 5, 2023

james-prysm Jun 1, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

terencechain Jun 5, 2023

rkapka left a comment

rkapka Jun 12, 2023

rkapka Jun 12, 2023

rkapka Jun 12, 2023

rkapka Jun 12, 2023

terencechain left a comment •

edited

Loading

rkapka Jun 14, 2023

rkapka Jun 14, 2023

		// Wei is the smallest unit of Ether, and sent in the payload value of the execution engine API call.
		// represented as a pointer to a bigInt.

	// Wei is the smallest unit of Ether, and sent in the payload value of the execution engine API call.
	// represented as a pointer to a bigInt.
	// Wei is the smallest unit of Ether, represented as a pointer to a bigInt.

builder: deneb and handling blobs #12477

builder: deneb and handling blobs #12477

Conversation

james-prysm commented May 30, 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

Choose a reason for hiding this comment

rkapka left a 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 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-prysm commented May 30, 2023 •

edited

Loading

terencechain left a comment •

edited

Loading