Block v3 builder boost factor

[eserilev]

Issue Addressed

ethereum/beacon-APIs#386

Proposed Changes

Add a builder_boost_factor query param to the block v3 endpoint as described in the beacon api spec

Additional Info

warp will reject any builder_boost_factor query param larger than u64::MAX. Saturating math is used to make sure we dont overflow when calculating the boosted_relay_value.

This PR should be merged before #4813

[realbigsean]

Looks good to me!

[eserilev]

I pushed up a commit that deprecates the following flags from the BN

• always-prefer-builder-payload
• builder-profit-threshold
• ignore-builder-override-suggestion-threshold

As Jimmy mentioned, keeping these flags around is non spec compliant and the new builder_boost_factor serves as a replacement for the functionality these flags provided.

I removed mentions of these deprecated flags in the documentation. Once new flags are added to the VC to support builder_boost_factor we'll need to make sure to add relevant documentation.

If we wanted to keep these flags around for the goerli fork I can quickly revert. Also, removing these flags affects the blinded block v2 endpoint.

[jimmygchen]

Thanks for the updates @eserilev! ☺️ Let's wait for michael and @realbigsean's inputs early next week.
btw we usually keep the flags around (as no-ops) for a few releases, so people have some time to update their setups.

Example help text from previously deprecated flags:

        .arg(
            Arg::with_name("count-unrealized")
                .long("count-unrealized")
                .hidden(true)
                .help("This flag is deprecated and has no effect.")
                .takes_value(true)
                .default_value("true")
        )

[realbigsean]

So we definitely need to support something equivalent to --always-prefer-builder-payload somewhere, this is some background on the flag: #4040

I like the idea of deprecating it in the BN, and shifting it to the VC, per-validator. But we will need to include that before the release if we deprecate it here.

In considering should_override_builder related to the above, we can either:

1. keep --ignore-builder-override-suggestion-threshold and continue to respect should_override_builder by default
2. remove --ignore-builder-override-suggestion-threshold and ignore should_override_builder always

I'm kind of leaning toward 2, and revisiting this later. I'm not sure EL's are even populating that with useful info at the moment. The config for whether or not to override the builder suggestion should probably also be made per-validator client, which would mean it'd have to be in the GET request for a block as a query param or something (so requires an API change). Also an EL could mimic should_override_builder in a way that's more cohesive with the rest of the API by just lying about the payload value (set it to max when there's censorship happening).

[michaelsproul]

I think we might want to keep respecting should_override_builder as that's new in Deneb and will hopefully be adopted by ELs as an anti-censorship mechanism. I think it's cleaner than requiring the EL to fake the payload value, which could cause downstream issues (e.g. if anyone is checking claimed payload values against actual payload values, spoofing would trigger an error).

As for ignore-builder-override-suggestion-threshold, on one hand I'm in favour of keeping it if we have should_override_builder (what I wrote here: #5035 (comment)), but on the other hand I would like to delete it as:

1. Users haven't started using it yet, so we don't even need a deprecation
2. It adds significant complexity, particularly when interacting with the builder boost factor

I also think for the 4.6.0-rc.0 release (Deneb on Goerli) it would be OK to deprecate always-prefer-builder-payloads without providing a replacement immediately, as people will mostly be running this release on testnets. IMO builder-profit-threshold doesn't need a replacement at all because it's the same as mev-boost's min-bid.

We can add the builder-boost-factor into the VC for 4.6.0-rc.1 or v4.6.0 proper, once we start recommending them for mainnet (with the networking fix)?

[realbigsean]

Ok cool, all good with me. Yea I think this PR is good as is for the goerli release modulo the small change Michael noted (and I think there's a compile error in a test)

[michaelsproul]

LGTM. I just tweaked the CLI flags slightly so that:

• ignore-builder-override-suggestion-threshold is removed completely (it was never in stable, so we don't need backwards compatibility for it)
• The deprecated flags still show in --help and the Lighthouse book. This might help some people who go looking for the flags in the book.
• A warning is logged if either of the deprecated flags are provided.