-
Notifications
You must be signed in to change notification settings - Fork 86
Fix/request-auth-clarifications #165
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
3566c77
d23c708
ea472f5
bd892d6
3dadb3a
5078eab
31712da
e6f34b2
4a60214
06ac935
51545cf
8eec2eb
07b94de
f99faf8
e27dfe3
bc91f6f
25fa22b
36cefe4
9a58e07
72165af
a5552d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,32 +5,35 @@ post: | |
| Requests a builder node to produce a valid execution payload bid, which | ||
| can be integrated into a beacon block and signed. | ||
|
|
||
| The proposer sends a POST request to the builder with the following information: | ||
| The proposer's beacon node sends a POST request to the builder with the following information: | ||
| - The slot for which the block should be proposed. | ||
| - The hash of the execution layer block the proposer will build on. | ||
| - The root of the beacon block the proposer will build on. | ||
| - The index of the proposer. | ||
| - Optionally, a `SignedRequestAuthV1` in the request body that | ||
| authenticates the request. The body MAY be encoded as JSON or SSZ. | ||
| - The public key of the proposer. | ||
| - A `SignedRequestAuth` in the request body that authenticates the | ||
| request. The body MAY be encoded as JSON or SSZ. | ||
| - A `Date-Milliseconds` header with the send time and an `X-Timeout-Ms` | ||
| header with the proposer's timeout for the request. | ||
|
|
||
| The proposer's `max_execution_payment` is communicated exclusively via the | ||
| `submitBuilderPreferences` endpoint. If no `BuilderPreferencesV1` have been | ||
| submitted for the proposer, the builder MUST treat `max_execution_payment` as `0` | ||
| and MUST NOT include an execution layer payment in the bid. | ||
| `submitBuilderPreferences` endpoint. Any bid the builder serves MUST honor | ||
| the `max_execution_payment` cap from stored preferences. Without stored | ||
| preferences the builder MAY serve a bid with any `execution_payment`; the | ||
| proposer's locally configured per-builder limits are the backstop: the | ||
| proposer discards any bid that exceeds them. | ||
|
|
||
| The `SignedRequestAuthV1` body is optional. If it is present but malformed | ||
| or fails signature verification, the builder MAY return a 401 response. | ||
| If it is absent, the builder MAY still serve a bid, but builders MAY | ||
| use the presence and validity of the `SignedRequestAuthV1` to apply | ||
| per-validator policy (e.g. rate-limiting, prioritization, or refusing | ||
| unauthenticated requests). | ||
| The `SignedRequestAuth` body is required. If the body is missing or | ||
| malformed, the request is invalid and the builder MUST return a 400 | ||
| response. If its `auth.message.data` or `auth.message.slot` does not match, | ||
| the builder MUST return a 400 response; if its signature fails to verify, | ||
| the builder MUST return a 401 response. Builders MAY use the authenticated | ||
| identity to apply per-validator policy (e.g. rate-limiting). | ||
|
|
||
| The builder responds with a 200 response containing an execution payload bid if it can provide one. | ||
|
|
||
| If the builder is unable to produce a valid execution payload bid, then | ||
| the builder MUST return a 204 response. If the request is invalid, then the | ||
| builder MUST return an error response (400) with a description of the | ||
| validation failure. | ||
| A builder that does not serve a bid MUST return a 204 response. If the | ||
| request is invalid, then the builder MUST return an error response (400) | ||
| with a description of the validation failure. | ||
|
|
||
| This API is applicable from Gloas fork onwards. | ||
| tags: | ||
|
|
@@ -62,48 +65,48 @@ post: | |
| $ref: "../../beacon-apis/types/primitive.yaml#/Pubkey" | ||
| - name: Date-Milliseconds | ||
| in: header | ||
| required: false | ||
| required: true | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this change in this PR? it seems already quite convoluted with different changes
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can put it back but this was a simplification born from having the keymanager api expect fully resolved fields including the request auth -> it's always going to be able to be supplied. The original protest (largely by me) of having required request auths was when it was difficult/unclear how a proxy could get/use them.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the relation, making
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops I misread. I was talking about request auths. for the timing headers I folded in based on the convo from discord https://discord.com/channels/595666850260713488/1528987631474446466/1529900444288155709
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep I know the discord discussion and agree with making them required in gloas, but a separate PR seems like the better way to go about just to get some approvals on those changes easier, also not quite sure how to merge anything in this repo, there is no maintainer since Bharath is gone |
||
| description: | | ||
| Optional header containing a Unix timestamp in milliseconds representing | ||
| the point-in-time the request was sent. This header can be used to measure | ||
| latency. | ||
| Unix timestamp in milliseconds at which the request was sent. The | ||
| builder compares it with its own receive time to estimate the transit | ||
| delay from proposer to builder. | ||
| schema: | ||
| type: integer | ||
| format: int64 | ||
| example: 1710338135000 | ||
| - name: X-Timeout-Ms | ||
| in: header | ||
| required: false | ||
| required: true | ||
| description: | | ||
| Optional header containing the proposer's timeout for the request in milliseconds. | ||
| The proposer's timeout for the request in milliseconds, measured from | ||
| `Date-Milliseconds`: the builder MUST respond by `Date-Milliseconds` | ||
| plus `X-Timeout-Ms`, and the proposer discards later responses. | ||
| schema: | ||
| type: integer | ||
| format: int64 | ||
| example: 10000 | ||
| - name: Eth-Consensus-Version | ||
| in: header | ||
| required: true | ||
| description: | | ||
| The active consensus version to which the `SignedRequestAuthV1` in the | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so in the future if we wanna have a I am still fine with bumping the whole api though, just wanna be clear what versioning strategy we wanna use here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not opinionated on this I just carried over the conventions from Bharath's original version
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
so we bump the api from v1 to v2 if we wanna introduce a |
||
| request body belongs. Required if the request body is SSZ encoded. | ||
| - in: header | ||
| schema: | ||
| $ref: "../../builder-oapi.yaml#/components/schemas/ConsensusVersion" | ||
| required: true | ||
| name: Eth-Consensus-Version | ||
| description: "The active consensus version to which the request body belongs." | ||
| requestBody: | ||
| description: | | ||
| Optional `SignedRequestAuthV1` authenticating the request. If provided, | ||
| the builder MAY verify the BLS signature against the validator pubkey | ||
| resolved from the `proposer_pubkey` path parameter, and check that | ||
| `data` matches its own URL and that `slot` matches the | ||
| requested slot. If absent, the builder MAY still serve a bid subject | ||
| to its own policy. | ||
| required: false | ||
| A `SignedRequestAuth` authenticating the request. The builder MUST | ||
| verify the BLS signature against the validator pubkey resolved from the | ||
| `proposer_pubkey` path parameter, and MUST check that `auth.message.data` | ||
| matches the value it agreed with the proposer and that | ||
| `auth.message.slot` matches the `slot` path parameter (the proposal | ||
| slot). | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "../../types/gloas/request_auth.yaml#/Gloas/SignedRequestAuthV1" | ||
| $ref: "../../types/gloas/request_auth.yaml#/Gloas/SignedRequestAuth" | ||
| application/octet-stream: | ||
| schema: | ||
| description: "SSZ serialized `SignedRequestAuthV1` bytes. Use Content-Type header to indicate that SSZ data is contained in the request body." | ||
| description: "SSZ serialized `SignedRequestAuth` bytes. Use Content-Type header to indicate that SSZ data is contained in the request body." | ||
| responses: | ||
| "200": | ||
| description: Success response. | ||
|
|
@@ -143,26 +146,30 @@ post: | |
| SlotMismatch: | ||
| value: | ||
| code: 400 | ||
| message: "Invalid SignedRequestAuthV1: auth.message.slot does not match the requested slot" | ||
| WrongBuilder: | ||
| message: "Invalid SignedRequestAuth: auth.message.slot does not match the proposal slot in the request path" | ||
| DataMismatch: | ||
| value: | ||
| code: 400 | ||
| message: "Invalid SignedRequestAuthV1: auth.message.data does not match this builder's URL" | ||
| message: "Invalid SignedRequestAuth: auth.message.data does not match the value agreed with this builder" | ||
| MissingBody: | ||
| value: | ||
| code: 400 | ||
| message: "Invalid request: SignedRequestAuth body is required" | ||
| MissingHeader: | ||
| value: | ||
| code: 400 | ||
| message: "Invalid request: Date-Milliseconds and X-Timeout-Ms headers are required" | ||
| "401": | ||
| description: Authentication required. | ||
| description: Authentication failed. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "../../builder-oapi.yaml#/components/schemas/ErrorMessage" | ||
| examples: | ||
| MissingAuth: | ||
| value: | ||
| code: 401 | ||
| message: "Missing SignedRequestAuthV1: this builder requires authenticated requests" | ||
| InvalidAuth: | ||
| value: | ||
| code: 401 | ||
| message: "Invalid SignedRequestAuthV1: signature verification failed" | ||
| message: "Invalid SignedRequestAuth: signature verification failed" | ||
| "406": | ||
| $ref: "../../builder-oapi.yaml#/components/responses/NotAcceptable" | ||
| "415": | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to ethereum/beacon-APIs#630 (comment), maybe there was some discussion around this but I don't really understand why we wanna sent a single request per pubkey, maybe someone can explain this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understood the question the first time. Is the assumption if a node operator has more than one proposer in the lookahead they can save on a call if batching was supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you only do a single call no matter how many validators are proposers in the lookahead, that matches how the proposer preferences api works
as far as I am aware all apis support batching, this one seems like an exception that decided to do a request per pubkey, but I don't see why it needs to be
to be clear, I don't feel strongly about this, on mainnet that will not make a large difference, but it would be kinda nice if the builder preferences work alongside the proposer preferences more closely unless there is a good reason to have a different api design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after thinking about this more and reading up on the current spec, although I haven't found that mentioned as an explicit argument during the initial spec design, I believe it makes sense due to signed auth requests since we allow pubkeys to sign over different auth, so there can be partial failures, however per the spec here this should return a 401 which is communicated back via http status code which doesn't really allow reporting partial failures. It seems favorable to send individual requests per-pubkey to me.