Builder: Electra #14344

james-prysm · 2024-08-14T20:20:48Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

implements ethereum/builder-specs#101

does not include

Add SSZ support to builder api ethereum/builder-specs#104
E2E enabled testing for Electra

Which issues(s) does this PR fix?

Fixes #

Other notes for review

s branch and updating values

james-prysm · 2024-08-15T20:25:07Z

go.mod

 // See https://github.com/prysmaticlabs/grpc-gateway/issues/2
 replace github.com/grpc-ecosystem/grpc-gateway/v2 => github.com/prysmaticlabs/grpc-gateway/v2 v2.3.1-0.20230315201114-09284ba20446
+
+replace github.com/ethereum/go-ethereum => github.com/lightclient/go-ethereum v1.10.10-0.20240726203109-4a0622f95d30


temp uses lightclient's fork to use the updated libraries in e2e

james-prysm · 2024-10-17T21:47:08Z

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

+		if !ok {
+			return nil, errors.New("builder returned non-electra bid")
 		}
-		for _, c := range kzgCommitments {


removing this validation as we already validate when we convert the bid from json to proto in func (bb *BuilderBidElectra) ToProto() (*eth.BuilderBidElectra, error) {

james-prysm · 2025-01-22T20:16:55Z

consensus-types/blocks/setters.go

 		return consensus_types.ErrNotSupported("SetExecutionRequests", b.version)
 	}
+	if req == nil {
+		return errors.New("nil execution request")


should I throw an error or just set it to an empty request object?

In my opinion it's bad practice to have different ways of handling the same thing within one package. We don't check this in other setters, so we should just skip it here too.

james-prysm · 2025-01-22T20:17:59Z

testing/middleware/builder/builder.go

+		http.Error(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+	p.currVersion = version.Electra


i'm not sure if this is the best way to do it, it's because the payload is still deneb so I can't do it by payload version

james-prysm · 2025-01-22T20:58:26Z

api/client/builder/client_test.go

 		_, err := c.GetHeader(ctx, slot, bytesutil.ToBytes32(parentHash), bytesutil.ToBytes48(pubkey))
 		require.ErrorContains(t, "could not extract proto message from header: too many blob commitments: 7", err)
 	})
+	t.Run("electra", func(t *testing.T) {


should i add any more kinds of tests?

terencechain · 2025-01-22T23:13:58Z

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

+		bidDeneb, ok := bid.(builder.BidDeneb)
+		if !ok {
+			log.Warnf("bid type %T does not implement builder.BidDeneb ", bid)
+		} else {
+			builderKzgCommitments = bidDeneb.BlobKzgCommitments()
+		}
+	}
+
+	var executionRequests *enginev1.ExecutionRequests
+	if bid.Version() >= version.Electra {
+		bidElectra, ok := bid.(builder.BidElectra)
+		if !ok {
+			log.Warnf("bid type %T does not implement builder.BidElectra ", bid)
+		} else {
+			executionRequests = bidElectra.ExecutionRequests()


we dont need to do this until after: higherValueBuilder && withdrawalsMatched
for the default bellatrix case, we can just set them as nil: setBuilderExecution(blk, builderPayload, nil, nil)

terencechain · 2025-01-22T23:20:58Z

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

+	if isBlinded {
+		requestsErr = "failed to set builder execution requests"
+	}
+	if err := blk.SetExecutionRequests(requests); err != nil {


If a builder returns us an invalid execution requests, shouldn't we default to local block?

isn't that what we do? it returns an error and then it falls back to

if err := setBuilderExecution(blk, builderPayload, builderKzgCommitments, executionRequests); err != nil { log.WithError(err).Warn("Proposer: failed to set builder payload") return local.Bid, local.BlobsBundle, setLocalExecution(blk, local)

terencechain · 2025-01-22T23:26:28Z

api/client/builder/types.go

 }

+// ExecHeaderResponseElectra is the header response for builder API /eth/v1/builder/header/{slot}/{parent_hash}/{pubkey}.
+type ExecHeaderResponseElectra struct {


Why cant the testing middleware uses this struct here?
https://github.com/prysmaticlabs/prysm/blob/5bbbad65bc2287c50a8a17e4dd8410ad5ea75427/testing/middleware/builder/builder.go#L109

good question i'm not sure i should fix the old ones too

terencechain · 2025-01-22T23:27:58Z

api/client/builder/types.go

+	if bb.ExecutionRequests == nil {
+		return nil, errors.New("execution requests is empty")
+	}
+	ExecutionRequests, err := bb.ExecutionRequests.ToProto()


Why is ExecutionRequests cap?

terencechain · 2025-01-22T23:32:44Z

config/params/config.go

 }

+// MaxBlobsPerBlockByVersion returns the maximum number of blobs per block for the given fork version
+func (b *BeaconChainConfig) MaxBlobsPerBlockByVersion(v int) int {


Please add a test for this helper function

api/client/builder/types.go

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

rkapka · 2025-01-23T13:17:24Z

consensus-types/blocks/setters.go

 		return consensus_types.ErrNotSupported("SetExecutionRequests", b.version)
 	}
+	if req == nil {
+		return errors.New("nil execution request")


In my opinion it's bad practice to have different ways of handling the same thing within one package. We don't check this in other setters, so we should just skip it here too.

Co-authored-by: Radosław Kapka <[email protected]>

james-prysm and others added 3 commits August 14, 2024 10:41

removing skip from test

d0522ac

Merge branch 'develop' into electra-test-remove-skip

9269e0e

builder wip

1ed404c

james-prysm added builder Electra labels Aug 14, 2024

james-prysm and others added 5 commits August 14, 2024 15:20

Merge branch 'develop' into builder-electra

592a45a

removing todo, it's probably ok

512e388

adding more TODOs

bfae0ca

adding fromProtoElectra

d8b98d7

using lightclient

dbc55c9

s branch and updating values

james-prysm added the blocked label Aug 15, 2024

james-prysm commented Aug 15, 2024

View reviewed changes

james-prysm and others added 5 commits August 15, 2024 15:33

minor fixes

2521755

Merge branch 'develop' into builder-electra

9c8d26e

Merge branch 'develop' into builder-electra

734ef94

Merge branch 'develop' into builder-electra

066ce6a

rolling back dependency changes

67235fb

james-prysm changed the title ~~Builder electra~~ Builder: Electra Sep 24, 2024

james-prysm and others added 10 commits September 24, 2024 15:15

go mod tidy

6d580ec

adding space back in

c67df35

Merge branch 'develop' into builder-electra

6d1a9fa

updating builder changes based on execution request changes

d5f8e02

Merge branch 'develop' into builder-electra

2c907b2

update ssz

289bb52

changelog

1c4be98

Merge branch 'develop' into builder-electra

45c1d0b

updating based on execution request changes

c1a93d8

fixing validation

802a68b

james-prysm commented Oct 17, 2024

View reviewed changes

adding builder test for electra

0aae98d

james-prysm commented Jan 22, 2025

View reviewed changes

james-prysm and others added 3 commits January 22, 2025 14:40

improving tests and updating more code based on review feedback

fbf7aae

gofmt

e0c5034

Merge branch 'develop' into builder-electra

6ec4a0f

james-prysm commented Jan 22, 2025

View reviewed changes

fixing unit tests

6595008

terencechain reviewed Jan 22, 2025

View reviewed changes

james-prysm added 2 commits January 22, 2025 18:04

more feedback from terence

b8aa910

gofmt

2a29e0f

rkapka reviewed Jan 23, 2025

View reviewed changes

james-prysm and others added 8 commits January 23, 2025 08:10

Update api/client/builder/types.go

08def54

Co-authored-by: Radosław Kapka <[email protected]>

Update beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go

03f5754

Co-authored-by: Radosław Kapka <[email protected]>

Update beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go

7ab30bd

Co-authored-by: Radosław Kapka <[email protected]>

Update beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go

40c180e

Co-authored-by: Radosław Kapka <[email protected]>

Update beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go

f9483ef

Co-authored-by: Radosław Kapka <[email protected]>

Update api/client/builder/types.go

2ac5089

Co-authored-by: Radosław Kapka <[email protected]>

addressing nitpicks

ac64d37

gofmt

73d2b5b

james-prysm requested review from rkapka and terencechain January 23, 2025 16:19

james-prysm and others added 3 commits January 23, 2025 10:55

radek feedback

ab16642

Merge branch 'develop' into builder-electra

4c40f18

improves error

15199e5

rkapka approved these changes Jan 23, 2025

View reviewed changes

james-prysm enabled auto-merge January 23, 2025 17:37

james-prysm added this pull request to the merge queue Jan 23, 2025

Merged via the queue into develop with commit 2c78e50 Jan 23, 2025
15 checks passed

james-prysm deleted the builder-electra branch January 23, 2025 17:55

Builder: Electra #14344

Builder: Electra #14344

Uh oh!

Conversation

james-prysm commented Aug 14, 2024 • 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!

james-prysm Oct 17, 2024 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

james-prysm commented Aug 14, 2024 •

edited

Loading

james-prysm Oct 17, 2024 •

edited

Loading