Fix/request-auth-clarifications - #165
Conversation
The slot in a signed request auth is the slot the validator will propose in, not the slot at which the request is signed or sent. Anchor the bid channel check to the slot path parameter, define the preferences channel semantics (the proposal slot the preferences apply to), and reject preferences for past slots so a replayed request cannot roll preferences back to a stale value.
data identifies the builder, not an API resource or a transport detail, so its canonical form is just the scheme and host: lowercased, with no port, path, query, fragment, or userinfo. The validator applies the rules before signing and the builder applies them to its own URL before the byte comparison, so both sides derive the same bytes.
…V1 so cached auths are easily reusable
05e1563 to
3dadb3a
Compare
|
From the SSV side, the proposal-slot clarification and reuse of one auth for the AOT and JIT channels are compatible with our pre-signing design. I think one signing-contract issue remains.
More importantly for SSV, normalizing inside verification authenticates a reconstructed message rather than the exact Could we instead canonicalize the configured builder URL before constructing This maps onto the existing spec structure: steps 1 and 2 belong in
|
Eth-Builder-Url header
|
5078eab decouples the request authorization data from the builder's URL. The motivation is that request authorizations require data unique to the builder. A builder's URL is just one instance of this, so rather than lock into one format, the idea is that the authorization data is just opaque bytes. These can be pre-negotiated between the builder and proposer, or very well just be the builder's URL. This change just makes it more flexible. Related to the conversation here ethereum/beacon-APIs/pull/625 |
- Eth-Builder-Url to submitSignedBeaconBlock - Specify clearly max_execution_payment in Gwei - additional clarity on what auth data SHOULD default to
|
Thanks @JasonVranek ! Very supportive of changes that:
|
| - [Builder Preferences](#builder-preferences) | ||
| - [`max_execution_payment`](#max_execution_payment) | ||
| - [Per-request Validator Inputs](#per-request-validator-inputs) | ||
| - [Routing through a proxy](#routing-through-a-proxy) |
There was a problem hiding this comment.
I think all explicit mentions to a proxy should be completely removed from the spec. This is out of protocol and unnecessary to be mentioned.
We just need to have two fields, one is the url to send the request, and another is arbitrary data. The latter is part of the signed auth, the former is not.
These two fields are already enough to enable proxying if people want to use that.
There was a problem hiding this comment.
Agreed, will remove explicit mentions of proxy across the PRs. The proxy was included because of the change to make auth data opaque bytes and not explicitly carry the builder's url. For a proxy to exist under this format, they must set url to their own and require data to carry the builder's URL to route the request. An easily fixable issue is that the constraint that per-key, URLs must be unique should be relaxed to (url, data) pairs should be unique. This would allow for example, a relay to receive N requests to different builders that carry real auth data.
For example the config would need to support:
{
"0xa057816155ad77931185101128655c0191bd0214c201ca48ed887f6c4c6adf334070efcd75140eada5ac83a92506dd7a": {
"fee_recipient": "0x50155530FCE8a85ec7055A5F8b2bE214B3DaeFd3",
"target_gas_limit": "45000000",
"builder": {
"enabled": true,
"builders": [
{
"url": "https://proxy.example.com",
"data": "https://builder-a.example.com",
"pubkey": "0x93247f2209abcacf57b75a51dafae777f9dd38bc7053d1af526f220a7489a6d3a2753e5f3e8b1cfe39b56f43611df74a",
"max_execution_payment": "250000000",
"min_bid": "10000000",
"builder_boost_factor": "100"
}
],
"max_execution_payment": "500000000"
},
{
"url": "https://proxy.example.com", // this should be allowed (will update PRs)
"data": "https://builder-b.example.com",
...
}
}- the auth body on getExecutionPayloadBid is required and builders MUST verify it: signature failure is a 401, data/slot mismatch a 400, missing or malformed body a 400 -- the same auth story as submitBuilderPreferences; proposer duties are known an epoch ahead so auths pre-sign off the proposal hot path - delete the proxy section and the Eth-Builder-Url header from all endpoints; url and auth data are sufficient to support optional proxies - relax the no-preferences default: a served bid MUST honor the max_execution_payment cap from stored preferences, but without them the builder MAY serve any execution_payment; the proposer's locally configured per-builder limits are the backstop and discard bids that exceed them - data default convention: UTF-8 bytes of the builder's own advertised URL, hex pattern tightened to whole bytes - error surface aligned: 401 is signature failure only (InvalidAuth), 400 gains MissingBody, WrongBuilder renamed DataMismatch, 204 is any non-served bid; submitBuilderPreferences notes JSON/SSZ bodies need no Eth-Consensus-Version header (not fork-versioned) - unify the path parameter name to proposer_pubkey across both endpoints
| in: header | ||
| required: true | ||
| description: | | ||
| The active consensus version to which the `SignedRequestAuthV1` in the |
There was a problem hiding this comment.
so in the future if we wanna have a SignedRequestAuthV2 we bump the whole api version? I do agree this isn't fork versioned per se, but the mechanism could still be used as such
I am still fine with bumping the whole api though, just wanna be clear what versioning strategy we wanna use here
There was a problem hiding this comment.
Not opinionated on this I just carried over the conventions from Bharath's original version
There was a problem hiding this comment.
the conventions from Bharath's original version
so we bump the api from v1 to v2 if we wanna introduce a SignedRequestAuthV2?
| - name: Date-Milliseconds | ||
| in: header | ||
| required: false | ||
| required: true |
There was a problem hiding this comment.
why is this change in this PR? it seems already quite convoluted with different changes
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't understand the relation, making Date-Milliseconds header required seems to be completely separate to me, but seems like you referring to the auth param, for that I agree is should be required and probably default to builder url if not supplied
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| - Builder | ||
| parameters: | ||
| - name: validator_pubkey | ||
| - name: proposer_pubkey |
There was a problem hiding this comment.
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.
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.
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 have been some ambiguities when reading the builder specs around slot numbers, URL encodings, request auth signing/verifying, plus some broken links. This PR aims to clarify them.