ethereum / keymanager-APIs

Collection of RESTful APIs provided by Ethereum consensus keymanagers
https://ethereum.github.io/keymanager-APIs/
Creative Commons Zero v1.0 Universal
38 stars 19 forks source link

GET - `/eth/v1/validator/{pubkey}/feerecipient` - Specify expected behaviour when the fee recipient is not registered for `{pubkey}` #55

Open nalepae opened 1 year ago

nalepae commented 1 year ago

Issue description:

When calling /eth/v1/validator/{pubkey}/feerecipient for a {pubkey} with no fee recipient registered, the response differs depending of validator client implementation:

Lighthouse: Lighthouse response is

{
    "code": 500,
    "message": "INTERNAL_SERVER_ERROR: no fee recipient set",
    "stacktraces": []
}

(Note: https://github.com/sigp/lighthouse/issues/3507 is open about error code)

Teku: Teku does not allow to start a validator client without setting a fee recipient. At start, Teku validator client logs are:

2023-01-30 12:53:28.336 FATAL - Invalid configuration. --validators-proposer-default-fee-recipient or --validators-proposer-config must be specified when Bellatrix milestone is active

then Teku validator client quits.

==> A user cannot query /eth/v1/validator/{pubkey}/feerecipient on a Teku validator client if no (at least default) fee recipient is registered for {pubkey}.

Prysm: Currently, Prysm validator client uses a custom gRPC API to communicate with the beacon node.

This gRPC API has special GetFeeRecipientByPubKey method which is the GET equivalent of Beacon API prepareBeaconProposer: The request body contains a validator pubkey. The beacon node responds the corresponding fee recipient. If no specific fee recipient for this pubkey is known by the beacon node, then the beacon node answers the default fee recipient specified in beacon node configuration. If no default fee recipient is specified in beacon node configuration, then the beacon node answers the burn address 0x0000....

Proposals:

Proposal 1:

Define in Key Manager API specification that any call to /eth/v1/validator/{pubkey}/feerecipient validator client API with {pubkey} with no registered fee recipient should return HTTP 404.

Proposal 2:

Define in Beacon API specification a new GET fee_recipient method. This method could take in query parameters a list of validators pubkey/index and could return corresponding fee recipient addresses. In this case, the call to /eth/v1/validator/{pubkey}/feerecipient validator client API will be able to answer the default fee recipient specified in beacon node configuration if no specific fee recipient is registered for {pubkey} in the validator client.

However, if {pubkey} has a registerd fee recipient in the validator client configuration, the validator client will have 2 possibilities:

  1. Returning directly fee recipient from validator client configuration corresponding to {pubkey}
  2. Querying the newly defined Beacon API GET fee_recipient method and returning the result.

1. and 2. may differ.

rolfyone commented 1 year ago

So basically we're saying that we should allow validators to start in an invalid state? Teku took the approach that if configuration is invalid, that we should not allow the user to run, because they can fix it and get into a valid configuration, if they know the problem. My preference would be that people aren't allowed to start up with a configuration that won't work, so from a teku perspective I think that will remain for now at least. If I had decided to allow it, i think i'd be slightly against 404. As a general rule i do dislike 404, as it's indistinguishable from a route that is not implemented in a way... 500 is probably on the harsh side depending on how you look at it, but I can definitely see the argument for using it in preference to 404.

If you decide that its a valid state for a validator to be, and decide to startup, then you accept this complexity. I think that the result is an error whichever way you cut it, and it's probably not hugely important the code, though I'd probably steer towards a 400 or 500 in preference to a 404. Regardless of which way, I think we definitely want a body, which is ever so slightly not the normal with a 404, as generally its handled by the router, and you don't get an option (admittedly you would in this circumstance).

james-prysm commented 1 year ago

I do think this goes against the original intention of having these apis throw as few errors as possible. that's why at prysm ours just goes with whatever the default was even if it's not set because after you import the key it's going to use it anyways. to do this it's already using option 2. with an unofficial gRPC call to get the fee recipient from the beacon node