Specify builder_comparison_factor#386
Conversation
|
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. |
|
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 |
| * `builder_comparison_factor=2**64 - 1`: prefer the builder payload unless an error or | ||
| beacon node health check makes it unviable. |
There was a problem hiding this comment.
| * `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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 builderFor non-negative comparison factor, |
|
Is everyone happy with this now? |
roughly agreed on discord Co-authored-by: g11tech <develop@g11tech.io>
6f68742
| unviable. | ||
| * `builder_comparison_factor=100`: default profit maximization mode; choose whichever | ||
| payload pays more. | ||
| * `builder_comparison_factor=2**64 - 1`: prefer the builder payload unless an error or |
There was a problem hiding this comment.
What does this mean? if the beacon health check makes it unviable, should we return the local payload or error out?
There was a problem hiding this comment.
(as discussed on Discord) local payload
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: