Add gas limit API

[michaelsproul]

Add a keymanager API for managing the gas limit.

Although several clients are standardising around a config file format shared by builders and validator clients (here: ethereum/builder-specs#41), I believe it's useful to have a separate push-based API for managing the gas limit, just as we have for fee recipients (further rationale here: flashbots/mev-boost#154 (comment)).

[dapplion]

LGTM!

[ralexstokes]

@michaelsproul haven't done an in-depth review of this PR's content but support the general idea

Have you considered reworking/extending the existing suggested fee recipient endpoint to a more general validator preferences endpoint, rather than add (and then maintain) a separate endpoint per preference we may come across?

I'd say if we only ever wanted to support the gas limit alongside the fee recipient then this is a moot point but you can see here ethereum/builder-specs#29 that there are other parameters consumers of the builder APIs may care about -- e.g. I could see adding support for extra_data sooner rather than later

[michaelsproul]

@ralexstokes I think you're right that it would be better to have a consolidated endpoint for all of the parameters, but I was worried about deprecating the existing API so soon after it was introduced, and it's hard to update DELETE. E.g. should DELETE /eth/v1/{pubkey}/feerecipient clear the feerecipient and the gas limit? It seems that passing bodies with DELETE requests is non-standard, and that would be a drastic departure from the current API.

The alternative would be a new API endpoint that deprecates the feerecipient one and looks something like this:

GET /eth/v1/{pubkey}/proposer_config
{
    "fee_recipient": "0x0",
    "gas_limit": "0x4000",
}

POST /eth/v1/{pubkey}/proposer_config
{
   "fee_recipient": "0x01",
   "gas_limit": "0x8000"
}

DELETE /eth/v1/{pubkey}/proposer_config/fee_recipient
DELETE /etc/v1/{pubkey}/proposer_config/gas_limit

All fields in POST would be optional.

In future we could add graffiti, builder_enabled and extra_data in a mostly-backwards compatible way (any clients not implementing the new fields would error on requests using the new fields, while continuing to work if only supported fields were provided).

Pros and Cons

Advantages of new gas_limit API

• No need to change/deprecate feerecipient
• Easier to communicate which clients support which flags. With the proposer_config endpoint it's slightly less clear which version and fields each client supports.

Advantages of consolidated proposer_config

• Easy to add graffiti and other fields in future (or right away?). No need for 3 more endpoints and implementations every time we add a field.
• Slightly better performance for multi-field operations: can get or set all fields in one HTTP request per validator if desired, rather than O(num_fields).

[james-prysm]

at the time of discussing having a proposer_config vs individual APIs paul and I thought the specs mutated too much, at the end of the day it was asking the question of what benefits the end user ( who's typically using some UI to update) do we think the proposer config API will continue to change ( i think it will). I'd argue for different endpoints or for some endpoint specific to MEV/builder config.

also question on the value of gas limit itself

GET /eth/v1/{pubkey}/proposer_config
{
    "fee_recipient": "0x0",
    "gas_limit": "0x4000",
}

I've always used the uint64 value and not the hex uint64 is there some reason for this? was something changed?

[ralexstokes]

I've always used the uint64 value and not the hex uint64 is there some reason for this? was something changed?

we generally write uint64 values as strings to avoid issues w/ e.g. native JS numbers which are only 53 bits

[james-prysm]

we generally write uint64 values as strings to avoid issues w/ e.g. native JS numbers which are only 53 bits

got it this makes sense now, from a user experience perspective a number seems fine even though it doesn't take advantage of the full 64 bit as the default is 30million, do you think it's required for the user to ever go above the 53bits?

[ralexstokes]

the gas limit as specified in the yellow paper is an arbitrary positive integer so in general it could go much higher than 30 million -- I would not take a snapshot of today as an "anchoring point" when this value could be much bigger in the future

implementations should not special-case or cut corners on this as it means less maintenance burden over time, and the same thing applies to any sort of standard API we enshrine via this PR

[james-prysm]

I'd at least argue for the sake of a better user experience that it's just a string number instead of a string hex. or at the very least take in both options with a casting error.

[ralexstokes]

I'd prefer we stick w/ the beacon-apis here so that things are uniform

there we encode this as a uint64: https://github.com/ethereum/beacon-APIs/blob/c39ba03485bbe53715b3e8c706a6fbeb6eb61a76/types/primitive.yaml#L27

@michaelsproul what do you think about updating this? or do you have another reason for electing u256?

[rolfyone]

We went with updating specifically one field when feerecipeint was added, because at the time everything else was up in the air and it was the path of least resistance...

Are we always likely to update both at once? If we're likely to want to update parts as 'optional' suggests, potentially keeping a separate API and just calling the one you want to update is a simpler flow. Then in the cases you do actually want to update both, you can call both apis... equally you can remove an attribute without affecting the other(s).

If we do go with a larger configuration object plus having optional fields, using PATCH may be more analogous to what's actually going on.

[rolfyone]

Side note while i remember - add to readme table :)

[michaelsproul]

How's this for a consensus decision?

• Add a separate gas_limit API (this PR). This is the most compatible with the existing API, and allows people to keep using the feerecipient API (particularly so close before the merge).
• Use a decimal uint64 for the gas limit (not a hex uint256). This matches the 64-bit gas_limit in ExecutionPayload: https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#executionpayload

Leave a 👍 react if you're happy with this, or comment with alternatives

[michaelsproul]

Awesome, I've updated this with the u64 gas limit, so I think it's ready for a final read-through and merge 🚀

[ralexstokes]

lgtm!

[james-prysm]

LGTM

[rolfyone]

LGTM!