adding builder boost factor to proposer settings #13422
Conversation
james-prysm
requested review from
kasey,
nalepae,
rauljordan,
saolyn and
rkapka
I think it needs to be this way to follow ethereum/beacon-APIs#386 |
james-prysm
added
Builder
cmd/beacon-chain/flags/base.go
Outdated
| @@ -28,7 +28,7 @@ var ( | |||
| LocalBlockValueBoost = &cli.Uint64Flag{ | |||
| Name: "local-block-value-boost", | |||
| Usage: "A percentage boost for local block construction as a Uint64. This is used to prioritize local block construction over relay/builder block construction" + | |||
| "Boost is an additional percentage to multiple local block value. Use builder block if: builder_bid_value * 100 > local_block_value * (local-block-value-boost + 100)", | |||
| "Boost is an additional percentage to multiple local block value. Use builder block if: builderValueGwei*builderBoostFactor(default is 100) > local_block_value * (local-block-value-boost + 100)", | |||
There was a problem hiding this comment.
--local-block-value-boost value A percentage boost for local block construction as a Uint64. This is used to prioritize local block construction over relay/builder block constructionBoost is an additional percentage to multiple local block value. Use builder block if: builderValueGwei*builderBoostFactor(default is 100) > local_block_value * (local-block-value-boost + 100) (default: 10)
Missing period and space between construction and Boost.
... builder block constructionBoost is an additional ...
In the help message, mix between xxx-yyy, xxx_yyy, xxxYyy.
Sometimes there is spaces between operands, sometimes not.
If displaying the default value of builderBoostFactor, I thinks it's better to use the real default value, and not to hardcode it in the help message in case we change it. But I guess displaying the default value here does not really help.
There was a problem hiding this comment.
tried to be more consistent and take out the defaults in the description
config/proposer/loader/loader.go
Outdated
| @@ -117,8 +129,11 @@ func (psl *settingsLoader) Load(cliCtx *cli.Context) (*proposer.Settings, error) | |||
| loadConfig := &validatorpb.ProposerSettingsPayload{} | |||
|
|
|||
| // override settings based on other options | |||
| if psl.options.builderConfig != nil && psl.options.gasLimit != nil { | |||
| psl.options.builderConfig.GasLimit = *psl.options.gasLimit | |||
| if psl.options.builderConfig != nil { | |||
There was a problem hiding this comment.
Will panic if psl.options == nil.
There was a problem hiding this comment.
i don't think options can be nil but i'll add a check anyways
| } | ||
| func processBuilderConfig(current *validatorpb.BuilderConfig, options *flagOptions) *validatorpb.BuilderConfig { | ||
| // If there are no options, return what was passed in | ||
| if options == nil { | ||
| return current |
There was a problem hiding this comment.
in the current setup it's not possible I guess, I can either remove this check but was just trying to be safe
| current.GasLimit = reviewGasLimit(current.GasLimit) | ||
|
|
||
| if options.builderBoostFactor != nil { | ||
| current.BuilderBoostFactor = options.builderBoostFactor |
| @@ -77,7 +77,7 @@ func (c *beaconApiValidatorClient) GetAttestationData(ctx context.Context, in *e | |||
|
|
|||
| func (c *beaconApiValidatorClient) GetBeaconBlock(ctx context.Context, in *ethpb.BlockRequest) (*ethpb.GenericBeaconBlock, error) { | |||
| return wrapInMetrics[*ethpb.GenericBeaconBlock]("GetBeaconBlock", func() (*ethpb.GenericBeaconBlock, error) { | |||
| return c.getBeaconBlock(ctx, in.Slot, in.RandaoReveal, in.Graffiti) | |||
| return c.getBeaconBlock(ctx, in.Slot, in.RandaoReveal, in.Graffiti, in.BuilderBoostFactor) | |||
There was a problem hiding this comment.
getBeaconBlock should be tested with the builder boost factor under get_beacon_block_test.go
Co-authored-by: Manu NALEPA <[email protected]>
Co-authored-by: Manu NALEPA <[email protected]>
Co-authored-by: Manu NALEPA <[email protected]>
| } | ||
| // If there's nothing to process further, return the override or current based on what's available | ||
| if current == nil { | ||
| return override |
There was a problem hiding this comment.
This will return nil if both options.builderConfig and current are nil. Is this OK?
There was a problem hiding this comment.
yeah it means there's nothing to override
| @@ -225,6 +227,31 @@ func (v *validator) ProposeBlock(ctx context.Context, slot primitives.Slot, pubK | |||
| } | |||
| } | |||
|
|
|||
| func findBuilderBoost(pubKey [fieldparams.BLSPubkeyLength]byte, proposerSettings *proposer.Settings) *wrapperspb.UInt64Value { | |||
There was a problem hiding this comment.
I like the name of this function :-)
|
Maybe local boost could be removed and only builder boost factor can be used. If builder boost factor is less then 100 it adds 10% to the local block. This is what lighthouse integrated and to have some consistency across multiple clients it would be great if prysm could also integrate it the same way. |
|
lets close this... this isn't the right time to add. |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR adds configurations to the proposer settings for builder boost factor which is a new request option when getting a block. this would enable the validator to prioritize or deprioritize the builder's block as long as there are no errors. A previous PR #13409 enabled this option but did not allow the user to do self-configuration.
this PR also adds a new flag
--builder-boost-factorand can be used providing a uint64(--builder-boost-factor=110)note: builder boost factor will be ignored if local boost is active
related PRs ethereum/beacon-APIs#386 ethereum/beacon-APIs#398