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.

Sign up for GitHub

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

Specify builder_comparison_factor #386

Merged
merged 7 commits into from
Dec 21, 2023
Merged

Specify builder_comparison_factor #386

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c76953
Specify `builder_comparison_factor`
michaelsproul Dec 11, 2023
bfb254c
Spellcheck
michaelsproul Dec 11, 2023
b0cbcc9
Clarify default & error behaviour
michaelsproul Dec 11, 2023
e5b1045
Clarify local terminology & improve rounding behaviour
michaelsproul Dec 12, 2023
e596deb
Merge branch 'master' into comparison-factor
rolfyone Dec 14, 2023
af4cd82
Merge branch 'master' into comparison-factor
rolfyone Dec 18, 2023
6f68742
Update apis/validator/block.v3.yaml
rolfyone Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions apis/validator/block.v3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,39 @@ get:
is a flag and does not take a value.
schema: {}
allowEmptyValue: true
- name: builder_comparison_factor
rolfyone marked this conversation as resolved.
Show resolved Hide resolved
in: query
required: false
description: |
Percentage multiplier to apply to the builder's payload value when choosing between a
builder payload header and a local payload. This parameter is only relevant if the beacon
node is connected to a builder, deems it safe to produce a builder payload, and receives
valid responses from both the builder endpoint _and_ the local execution node. When these
preconditions are met, the server MUST act as follows:

* if `local_payload_value >= builder_comparison_factor * builder_payload_value // 100`, then
michaelsproul marked this conversation as resolved.
Show resolved Hide resolved
return a full (unblinded) block containing the local payload.
michaelsproul marked this conversation as resolved.
Show resolved Hide resolved
* otherwise, return a blinded block containing the builder payload header.

Servers must support the following values of the comparison factor which encode common
preferences:

* `builder_comparison_factor=0`: prefer the local payload unless an error makes it
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
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(as discussed on Discord) local payload

beacon node health check makes it unviable.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@dapplion dapplion Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@michaelsproul michaelsproul Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Servers should use saturating arithmetic or another technique to ensure that large values of
the `builder_comparison_factor` do not trigger overflows or errors. If this parameter is
provided and the beacon node is not configured with a builder then the beacon node MUST
respond with a full local block, which the caller can choose to reject if it wishes. If this
parameter is **not** provided then it should be treated as having the default value of 100.
If the value is provided but out of range for a 64-bit unsigned integer, then an error
response with status code 400 MUST be returned.
schema:
$ref: "../../beacon-node-oapi.yaml#/components/schemas/Uint64"
responses:
"200":
description: Success response
Expand Down
Loading