Builder: Electra - #14344
Conversation
| // 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 |
There was a problem hiding this comment.
temp uses lightclient's fork to use the updated libraries in e2e
| if !ok { | ||
| return nil, errors.New("builder returned non-electra bid") | ||
| } | ||
| for _, c := range kzgCommitments { |
There was a problem hiding this comment.
removing this validation as we already validate when we convert the bid from json to proto in func (bb *BuilderBidElectra) ToProto() (*eth.BuilderBidElectra, error) {
| return consensus_types.ErrNotSupported("SetExecutionRequests", b.version) | ||
| } | ||
| if req == nil { | ||
| return errors.New("nil execution request") |
There was a problem hiding this comment.
should I throw an error or just set it to an empty request object?
There was a problem hiding this comment.
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.
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| p.currVersion = version.Electra |
There was a problem hiding this comment.
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
| _, 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) { |
There was a problem hiding this comment.
should i add any more kinds of tests?
| 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() |
There was a problem hiding this comment.
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)
| if isBlinded { | ||
| requestsErr = "failed to set builder execution requests" | ||
| } | ||
| if err := blk.SetExecutionRequests(requests); err != nil { |
There was a problem hiding this comment.
If a builder returns us an invalid execution requests, shouldn't we default to local block?
There was a problem hiding this comment.
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)
| } | ||
|
|
||
| // ExecHeaderResponseElectra is the header response for builder API /eth/v1/builder/header/{slot}/{parent_hash}/{pubkey}. | ||
| type ExecHeaderResponseElectra struct { |
There was a problem hiding this comment.
Why cant the testing middleware uses this struct here?
https://github.com/prysmaticlabs/prysm/blob/5bbbad65bc2287c50a8a17e4dd8410ad5ea75427/testing/middleware/builder/builder.go#L109
There was a problem hiding this comment.
good question i'm not sure i should fix the old ones too
| if bb.ExecutionRequests == nil { | ||
| return nil, errors.New("execution requests is empty") | ||
| } | ||
| ExecutionRequests, err := bb.ExecutionRequests.ToProto() |
There was a problem hiding this comment.
Why is ExecutionRequests cap?
| } | ||
|
|
||
| // MaxBlobsPerBlockByVersion returns the maximum number of blobs per block for the given fork version | ||
| func (b *BeaconChainConfig) MaxBlobsPerBlockByVersion(v int) int { |
There was a problem hiding this comment.
Please add a test for this helper function
| return consensus_types.ErrNotSupported("SetExecutionRequests", b.version) | ||
| } | ||
| if req == nil { | ||
| return errors.New("nil execution request") |
There was a problem hiding this comment.
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 <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
3 participants
What type of PR is this?
Feature
What does this PR do? Why is it needed?
implements ethereum/builder-specs#101
does not include
Which issues(s) does this PR fix?
Fixes #
Other notes for review