builder: deneb and handling blobs by james-prysm · Pull Request #12477 · OffchainLabs/prysm

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

 	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

 		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 potuz, prestonvanloon, saolyn 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

		// 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.

Conversation

james-prysm commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkapka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terencechain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

james-prysm commented May 30, 2023 •

edited

Loading

terencechain left a comment •

edited

Loading