Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding builder boost factor to get block v3 #13409

Merged
merged 15 commits into from
Jan 4, 2024
2 changes: 2 additions & 0 deletions beacon-chain/rpc/eth/validator/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ go_library(
"@io_opencensus_go//trace:go_default_library",
"@org_golang_google_grpc//codes:go_default_library",
"@org_golang_google_grpc//status:go_default_library",
"@org_golang_google_protobuf//types/known/wrapperspb:go_default_library",
],
)

Expand Down Expand Up @@ -94,5 +95,6 @@ go_test(
"@com_github_gorilla_mux//:go_default_library",
"@com_github_pkg_errors//:go_default_library",
"@com_github_sirupsen_logrus//hooks/test:go_default_library",
"@org_golang_google_protobuf//types/known/wrapperspb:go_default_library",
],
)
31 changes: 27 additions & 4 deletions beacon-chain/rpc/eth/validator/handlers_block.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"net/http"
"strconv"
"strings"

"github.com/pkg/errors"
Expand All @@ -19,6 +20,7 @@ import (
eth "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1"
"github.com/prysmaticlabs/prysm/v4/runtime/version"
"go.opencensus.io/trace"
"google.golang.org/protobuf/types/known/wrapperspb"
)

type blockType uint8
Expand Down Expand Up @@ -155,6 +157,12 @@ func (s *Server) ProduceBlockV3(w http.ResponseWriter, r *http.Request) {
rawGraffiti := r.URL.Query().Get("graffiti")
rawSkipRandaoVerification := r.URL.Query().Get("skip_randao_verification")

bbFactor, err := processBuilderBoostFactor(r.URL.Query().Get("builder_boost_factor"))
if err != nil {
httputil.HandleError(w, err.Error(), http.StatusBadRequest)
return
}

slot, valid := shared.ValidateUint(w, "slot", rawSlot)
if !valid {
return
Expand Down Expand Up @@ -182,13 +190,28 @@ func (s *Server) ProduceBlockV3(w http.ResponseWriter, r *http.Request) {
}

s.produceBlockV3(ctx, w, r, &eth.BlockRequest{
Slot: primitives.Slot(slot),
RandaoReveal: randaoReveal,
Graffiti: graffiti,
SkipMevBoost: false,
Slot: primitives.Slot(slot),
RandaoReveal: randaoReveal,
Graffiti: graffiti,
SkipMevBoost: false,
BuilderBoostFactor: bbFactor,
}, any)
}

func processBuilderBoostFactor(raw string) (*wrapperspb.UInt64Value, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec defines the boost factor as Uint64. There is no need for this function, you can just call shared.UintFromQuery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need it as nullable, i'll doublecheck sharedUintFromQuery though

trimmed := strings.ReplaceAll(raw, " ", "")
switch trimmed {
case "": // default to logic in setExecutionPayload
return nil, nil
default:
number, err := strconv.ParseUint(trimmed, 10, 64)
if err != nil {
return nil, errors.Wrap(err, "Unable to decode builder boost factor")
}
return &wrapperspb.UInt64Value{Value: number}, nil
}
}

func (s *Server) produceBlockV3(ctx context.Context, w http.ResponseWriter, r *http.Request, v1alpha1req *eth.BlockRequest, requiredType blockType) {
isSSZ := httputil.SszRequested(r)
v1alpha1resp, err := s.V1Alpha1Server.GetBeaconBlock(ctx, v1alpha1req)
Expand Down
70 changes: 70 additions & 0 deletions beacon-chain/rpc/eth/validator/handlers_block_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"math"
"net/http"
"net/http/httptest"
"strings"
Expand All @@ -23,6 +24,7 @@ import (
"github.com/prysmaticlabs/prysm/v4/testing/assert"
mock2 "github.com/prysmaticlabs/prysm/v4/testing/mock"
"github.com/prysmaticlabs/prysm/v4/testing/require"
"google.golang.org/protobuf/types/known/wrapperspb"
)

func TestProduceBlockV2(t *testing.T) {
Expand Down Expand Up @@ -1658,3 +1660,71 @@ func TestProduceBlockV3SSZ(t *testing.T) {
require.Equal(t, "10", writer.Header().Get(api.ConsensusBlockValueHeader))
})
}

func Test_processBuilderBoostFactor(t *testing.T) {
type args struct {
raw string
}
tests := []struct {
name string
args args
want *wrapperspb.UInt64Value
wantErr bool
}{
{
name: "builder boost factor of 0 returns 0",
args: args{raw: "0"},
want: &wrapperspb.UInt64Value{Value: 0},
},
{
name: "builder boost factor that is empty with spaces returns default",
args: args{raw: " "},
want: nil,
},
{
name: "builder boost factor of the default, 100 with spaces returns 100",
args: args{raw: "100 "},
want: &wrapperspb.UInt64Value{Value: 100},
},
{
name: "builder boost factor max uint64 returns max uint64",
args: args{raw: "18446744073709551615"},
want: &wrapperspb.UInt64Value{Value: math.MaxUint64},
},
{
name: "builder boost factor as a percentage returns error",
args: args{raw: "0.30"},
wantErr: true,
},
{
name: "builder boost factor negative int returns error",
args: args{raw: "-100"},
wantErr: true,
},
{
name: "builder boost factor max uint64 +1 returns error",
args: args{raw: "18446744073709551616"},
wantErr: true,
},
{
name: "builder boost factor of invalid string returns error",
args: args{raw: "asdf"},
wantErr: true,
},
{
name: "builder boost factor of number bigger than uint64 string returns error",
args: args{raw: "9871398721983721908372190837219837129803721983719283798217390821739081273918273918273918273981273982139812739821"},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := processBuilderBoostFactor(tt.args.raw)
if (err != nil) != tt.wantErr {
t.Errorf("processBuilderBoostFactor() error = %v, wantErr %v", err, tt.wantErr)
return
}
require.DeepEqual(t, got, tt.want)
})
}
}
14 changes: 10 additions & 4 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ import (
var eth1DataNotification bool

const (
eth1dataTimeout = 2 * time.Second
eth1dataTimeout = 2 * time.Second
defaultBuilderBoostFactor = uint64(100)
)

// GetBeaconBlock is called by a proposer during its assigned slot to request a block to sign
Expand Down Expand Up @@ -107,7 +108,12 @@ func (vs *Server) GetBeaconBlock(ctx context.Context, req *ethpb.BlockRequest) (
}
sBlk.SetProposerIndex(idx)

if err = vs.BuildBlockParallel(ctx, sBlk, head, req.SkipMevBoost); err != nil {
builderBoostFactor := defaultBuilderBoostFactor
if req.BuilderBoostFactor != nil {
builderBoostFactor = req.BuilderBoostFactor.Value
}

if err = vs.BuildBlockParallel(ctx, sBlk, head, req.SkipMevBoost, builderBoostFactor); err != nil {
return nil, errors.Wrap(err, "could not build block in parallel")
}

Expand All @@ -127,7 +133,7 @@ func (vs *Server) GetBeaconBlock(ctx context.Context, req *ethpb.BlockRequest) (
return vs.constructGenericBeaconBlock(sBlk, bundleCache.get(req.Slot))
}

func (vs *Server) BuildBlockParallel(ctx context.Context, sBlk interfaces.SignedBeaconBlock, head state.BeaconState, skipMevBoost bool) error {
func (vs *Server) BuildBlockParallel(ctx context.Context, sBlk interfaces.SignedBeaconBlock, head state.BeaconState, skipMevBoost bool, builderBoostFactor uint64) error {
// Build consensus fields in background
var wg sync.WaitGroup
wg.Add(1)
Expand Down Expand Up @@ -185,7 +191,7 @@ func (vs *Server) BuildBlockParallel(ctx context.Context, sBlk interfaces.Signed
}
}

if err := setExecutionData(ctx, sBlk, localPayload, builderPayload, builderKzgCommitments); err != nil {
if err := setExecutionData(ctx, sBlk, localPayload, builderPayload, builderKzgCommitments, builderBoostFactor); err != nil {
return status.Errorf(codes.Internal, "Could not set execution data: %v", err)
}

Expand Down
22 changes: 16 additions & 6 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ var emptyTransactionsRoot = [32]byte{127, 254, 36, 30, 166, 1, 135, 253, 176, 24
const blockBuilderTimeout = 1 * time.Second

// Sets the execution data for the block. Execution data can come from local EL client or remote builder depends on validator registration and circuit breaker conditions.
func setExecutionData(ctx context.Context, blk interfaces.SignedBeaconBlock, localPayload, builderPayload interfaces.ExecutionData, builderKzgCommitments [][]byte) error {
func setExecutionData(ctx context.Context, blk interfaces.SignedBeaconBlock, localPayload, builderPayload interfaces.ExecutionData, builderKzgCommitments [][]byte, builderBoostFactor uint64) error {
_, span := trace.StartSpan(ctx, "ProposerServer.setExecutionData")
defer span.End()

Expand Down Expand Up @@ -80,9 +80,17 @@ func setExecutionData(ctx context.Context, blk interfaces.SignedBeaconBlock, loc
}

// Use builder payload if the following in true:
// builder_bid_value * 100 > local_block_value * (local-block-value-boost + 100)
// builder_bid_value * builderBoostFactor(default 100) > local_block_value * (local-block-value-boost + 100)
Copy link
Contributor

@potuz potuz Jan 3, 2024

Choose a reason for hiding this comment

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

Hmmm I don't like this. The point of builderBoostFactor is that it replaces our current parameter local-block-value-boost not to use both... I'm not sure what the right approach is here but at the very least we should log if there's a request and the local boost is set to non-zero and the boost factor to non default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a log for now we'll see if others have an opinion

boost := params.BeaconConfig().LocalBlockValueBoost
higherValueBuilder := builderValueGwei*100 > localValueGwei*(100+boost)
higherValueBuilder := builderValueGwei*builderBoostFactor > localValueGwei*(100+boost)
if boost > 0 && builderBoostFactor > (100+boost) {
log.WithFields(logrus.Fields{
"localGweiValue": localValueGwei,
"localBoostPercentage": boost,
"builderGweiValue": builderValueGwei,
"builderBoostFactor": builderBoostFactor,
}).Warn("Proposer: using builder payload even though local block value boost is more than 0")
}

// If we can't get the builder value, just use local block.
if higherValueBuilder && withdrawalsMatched { // Builder value is higher and withdrawals match.
Expand All @@ -98,13 +106,15 @@ func setExecutionData(ctx context.Context, blk interfaces.SignedBeaconBlock, loc
"localGweiValue": localValueGwei,
"localBoostPercentage": boost,
"builderGweiValue": builderValueGwei,
"builderBoostFactor": builderBoostFactor,
}).Warn("Proposer: using local execution payload because higher value")
}
span.AddAttributes(
trace.BoolAttribute("higherValueBuilder", higherValueBuilder),
trace.Int64Attribute("localGweiValue", int64(localValueGwei)), // lint:ignore uintcast -- This is OK for tracing.
trace.Int64Attribute("localBoostPercentage", int64(boost)), // lint:ignore uintcast -- This is OK for tracing.
trace.Int64Attribute("builderGweiValue", int64(builderValueGwei)), // lint:ignore uintcast -- This is OK for tracing.
trace.Int64Attribute("localGweiValue", int64(localValueGwei)), // lint:ignore uintcast -- This is OK for tracing.
trace.Int64Attribute("localBoostPercentage", int64(boost)), // lint:ignore uintcast -- This is OK for tracing.
trace.Int64Attribute("builderGweiValue", int64(builderValueGwei)), // lint:ignore uintcast -- This is OK for tracing.
trace.Int64Attribute("builderBoostFactor", int64(builderBoostFactor)), // lint:ignore uintcast -- This is OK for tracing.
)
return setLocalExecution(blk, localPayload)
default: // Bellatrix case.
Expand Down
Loading
Loading