Specify builder_comparison_factor

[michaelsproul]

Generalisation and replacement for #377 based on the rough consensus achieved in the discussion there.

One new parameter is added for block building which can be used to encode several types of validator preferences. There is a trade-off between simplicity and expressiveness, and IMO this single parameter achieves a good middle ground.

I haven't done any renames of blinded -> builder in this PR as that seems like a separate issue, and the current spec is actually already unambiguous on this matter when it states:

The beacon node must return an unblinded block if it obtains the execution payload from its paired execution node. It must only return a blinded block if it obtains the execution payload header from an MEV relay.

[g11tech]

lgtm

[potuz]

Just a nit, but I prefer comparisons to be in addition to instead of relative. That is, instead of 120, I rather see the comparison be 20, as in 20% more than the local payload instead of 1.2 times the local payload

[mcdee]

Just a nit, but I prefer comparisons to be in addition to instead of relative. That is, instead of 120, I rather see the comparison be 20, as in 20% more than the local payload instead of 1.2 times the local payload

I don't see how you could request a locally-built block with the additive style configuration, given that the parameter is unsigned. Relative seems more flexible.

[potuz]

Locally built would be max uint. What's impossible is to request a builders block unconditionally which is part of the reason I like this better

[tbenr]

Suggested change

	* `builder_comparison_factor=2**64 - 1`: prefer the builder payload unless an error or
	beacon node health check makes it unviable.
	* `builder_comparison_factor=2**64 - 1`: prefer the builder payload unless an error
	makes it unviable or beacon node health check makes it unviable.

[tersec]

I'm not really opposed to this, but how is this this an improvement?

Also, if nothing else, the grammar's wonky now: "beacon node health check" lacks an article or other similarly-functioning word. Before, it shared an with error, but this removes that linkage.

[dapplion]

Why not just

 * `builder_comparison_factor=2**64 - 1`: prefer the builder payload unless
    an error makes it unviable.

@michaelsproul why is a "beacon node health check" relevant here?

[michaelsproul]

I wanted to flag that a health check would be the most likely reason a local block would be returned when the user greatly prefers a builder block.

[michaelsproul]

Locally built would be max uint. What's impossible is to request a builders block unconditionally which is part of the reason I like this better

@potuz The point of the multiplicative factor is to make a large variety of preferences expressible using a single param. Removing the ability to define the "prefer builder" case would be antithetical to that. A "builder" block also doesn't necessarily imply censorship. In a DVT setup you could e.g. sit a piece of software that uses the builder API in front of a local EL and share the blocks it produces with the entire cluster so that they achieve consensus on it.

I'm assuming your system is:

if exec_node_payload_value + c * (exec_node_payload_value // 100) >= builder_payload_value:
  # use local
else:
  # use builder

For non-negative comparison factor, c. If c were allowed to be negative then this would be equally expressive (albeit more complex).

[rolfyone]

Is everyone happy with this now?

[rolfyone]

LGTM

[rolfyone]

LGTM

[potuz]

What does this mean? if the beacon health check makes it unviable, should we return the local payload or error out?

[michaelsproul]

(as discussed on Discord) local payload