Implement feerecipient API for keymanager (#3213)

## Issue Addressed

* #3173 

## Proposed Changes

Moved all `fee_recipient_file` related logic inside the `ValidatorStore` as it makes more sense to have this all together there. I tested this with the validators I have on `mainnet-shadow-fork-5` and everything appeared to work well. Only technicality is that I can't get the method to return `401` when the authorization header is not specified (it returns `400` instead). Fixing this is probably quite difficult given that none of `warp`'s rejections have code `401`.. I don't really think this matters too much though as long as it fails.

book/src/suggested-fee-recipient.md

@@ -26,14 +26,9 @@ Lighthouse BN also provides a method for defining this value, should the VC not
 Assuming trustworthy nodes, the priority for the four methods is:
 
 1. `validator_definitions.yml`
-1. `--suggested-fee-recipient-file`
 1. `--suggested-fee-recipient` provided to the VC.
 1. `--suggested-fee-recipient` provided to the BN.
 
-Users may configure the fee recipient via `validator_definitions.yml` or via the
-`--suggested-fee-recipient-file` flag. The value in `validator_definitions.yml` will always take
-precedence.
-
 ### 1. Setting the fee recipient in the `validator_definitions.yml`
 
 Users can set the fee recipient in `validator_definitions.yml` with the `suggested_fee_recipient`
@@ -56,36 +51,111 @@ Below is an example of the validator_definitions.yml with `suggested_fee_recipie
   suggested_fee_recipient: "0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d"
 ```
 
-### 2. Using the "--suggested-fee-recipient-file" flag on the validator client
+### 2. Using the "--suggested-fee-recipient" flag on the validator client
 
-Users can specify a file with the `--suggested-fee-recipient-file` flag. This option is useful for dynamically
-changing fee recipients. This file is reloaded each time a validator is chosen to propose a block.
+The `--suggested-fee-recipient` can be provided to the VC to act as a default value for all
+validators where a `suggested_fee_recipient` is not loaded from another method.
 
-Usage:
-`lighthouse vc --suggested-fee-recipient-file fee_recipient.txt`
+### 3. Using the "--suggested-fee-recipient" flag on the beacon node
 
-The file should contain key value pairs corresponding to validator public keys and their associated
-fee recipient. The file can optionally contain a `default` key for the default case.
+The `--suggested-fee-recipient` can be provided to the BN to act as a default value when the
+validator client does not transmit a `suggested_fee_recipient` to the BN.
+
+## Setting the fee recipient dynamically using the keymanager API
+
+When the [validator client API](api-vc.md) is enabled, the
+[standard keymanager API](https://ethereum.github.io/keymanager-APIs/) includes an endpoint
+for setting the fee recipient dynamically for a given public key. When used, the fee recipient
+will be saved in `validator_definitions.yml` so that it persists across restarts of the validator
+client.
+
+| Property | Specification |
+| --- | --- |
+Path | `/eth/v1/validator/{pubkey}/feerecipient`
+Method | POST
+Required Headers | [`Authorization`](./api-vc-auth-header.md)
+Typical Responses | 202, 404
+
+#### Example Request Body
+```json
+{
+    "ethaddress": "0x1D4E51167DBDC4789a014357f4029ff76381b16c"
+}
+```
 
-The following example sets the default and the values for the validators with pubkeys `0x87a5` and
-`0xa556`:
+```bash
+DATADIR=$HOME/.lighthouse/mainnet
+PUBKEY=0xa9735061c84fc0003657e5bd38160762b7ef2d67d280e00347b1781570088c32c06f15418c144949f5d736b1d3a6c591
+FEE_RECIPIENT=0x1D4E51167DBDC4789a014357f4029ff76381b16c
 
+curl -X POST \
+    -H "Authorization: Bearer $(cat ${DATADIR}/validators/api-token.txt)" \
+    -H "Content-Type: application/json" \
+    -d "{ \"ethaddress\": \"${FEE_RECIPIENT}\" }" \
+    http://localhost:5062/eth/v1/validator/${PUBKEY}/feerecipient | jq
 ```
-default: 0x6cc8dcbca744a6e4ffedb98e1d0df903b10abd21
-0x87a580d31d7bc69069b55f5a01995a610dd391a26dc9e36e81057a17211983a79266800ab8531f21f1083d7d84085007: 0x6cc8dcbca744a6e4ffedb98e1d0df903b10abd21
-0xa5566f9ec3c6e1fdf362634ebec9ef7aceb0e460e5079714808388e5d48f4ae1e12897fed1bea951c17fa389d511e477: 0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d
+
+#### Successful Response (202)
+```json
+null
 ```
 
-Lighthouse will first search for the fee recipient corresponding to the public key of the proposing
-validator, if there are no matches for the public key, then it uses the address corresponding to the
-default key (if present).
+### Querying the fee recipient
 
-### 3. Using the "--suggested-fee-recipient" flag on the validator client
+The same path with a `GET` request can be used to query the fee recipient for a given public key at any time.
 
-The `--suggested-fee-recipient` can be provided to the VC to act as a default value for all
-validators where a `suggested_fee_recipient` is not loaded from another method.
+| Property | Specification |
+| --- | --- |
+Path | `/eth/v1/validator/{pubkey}/feerecipient`
+Method | GET
+Required Headers | [`Authorization`](./api-vc-auth-header.md)
+Typical Responses | 200, 404
+
+```bash
+DATADIR=$HOME/.lighthouse/mainnet
+PUBKEY=0xa9735061c84fc0003657e5bd38160762b7ef2d67d280e00347b1781570088c32c06f15418c144949f5d736b1d3a6c591
+
+curl -X GET \
+    -H "Authorization: Bearer $(cat ${DATADIR}/validators/api-token.txt)" \
+    -H "Content-Type: application/json" \
+    http://localhost:5062/eth/v1/validator/${PUBKEY}/feerecipient | jq
+```
+
+#### Successful Response (200)
+```json
+{
+  "data": {
+    "pubkey": "0xa9735061c84fc0003657e5bd38160762b7ef2d67d280e00347b1781570088c32c06f15418c144949f5d736b1d3a6c591",
+    "ethaddress": "0x1d4e51167dbdc4789a014357f4029ff76381b16c"
+  }
+}
+```
+
+### Removing the fee recipient
+
+The same path with a `DELETE` request can be used to remove the fee recipient for a given public key at any time.
+This is useful if you want the fee recipient to fall back to the validator client (or beacon node) default.
+
+| Property | Specification |
+| --- | --- |
+Path | `/eth/v1/validator/{pubkey}/feerecipient`
+Method | DELETE
+Required Headers | [`Authorization`](./api-vc-auth-header.md)
+Typical Responses | 204, 404
+
+```bash
+DATADIR=$HOME/.lighthouse/mainnet
+PUBKEY=0xa9735061c84fc0003657e5bd38160762b7ef2d67d280e00347b1781570088c32c06f15418c144949f5d736b1d3a6c591
+
+curl -X DELETE \
+    -H "Authorization: Bearer $(cat ${DATADIR}/validators/api-token.txt)" \
+    -H "Content-Type: application/json" \
+    http://localhost:5062/eth/v1/validator/${PUBKEY}/feerecipient | jq
+```
+
+#### Successful Response (204)
+```json
+null
+```
 
-### 4. Using the "--suggested-fee-recipient" flag on the beacon node
 
-The `--suggested-fee-recipient` can be provided to the BN to act as a default value when the
-validator client does not transmit a `suggested_fee_recipient` to the BN.

common/eth2/src/lighthouse_vc/http_client.rs

@@ -303,11 +303,11 @@ impl ValidatorClientHttpClient {
     }
 
     /// Perform a HTTP DELETE request.
-    async fn delete_with_unsigned_response<T: Serialize, U: IntoUrl, V: DeserializeOwned>(
+    async fn delete_with_raw_response<T: Serialize, U: IntoUrl>(
         &self,
         url: U,
         body: &T,
-    ) -> Result<V, Error> {
+    ) -> Result<Response, Error> {
         let response = self
             .client
             .delete(url)
@@ -316,7 +316,16 @@ impl ValidatorClientHttpClient {
             .send()
             .await
             .map_err(Error::Reqwest)?;
-        let response = ok_or_error(response).await?;
+        ok_or_error(response).await
+    }
+
+    /// Perform a HTTP DELETE request.
+    async fn delete_with_unsigned_response<T: Serialize, U: IntoUrl, V: DeserializeOwned>(
+        &self,
+        url: U,
+        body: &T,
+    ) -> Result<V, Error> {
+        let response = self.delete_with_raw_response(url, body).await?;
         Ok(response.json().await?)
     }
 
@@ -486,6 +495,18 @@ impl ValidatorClientHttpClient {
         Ok(url)
     }
 
+    fn make_fee_recipient_url(&self, pubkey: &PublicKeyBytes) -> Result<Url, Error> {
+        let mut url = self.server.full.clone();
+        url.path_segments_mut()
+            .map_err(|()| Error::InvalidUrl(self.server.clone()))?
+            .push("eth")
+            .push("v1")
+            .push("validator")
+            .push(&pubkey.to_string())
+            .push("feerecipient");
+        Ok(url)
+    }
+
     /// `GET lighthouse/auth`
     pub async fn get_auth(&self) -> Result<AuthResponse, Error> {
         let mut url = self.server.full.clone();
@@ -543,14 +564,44 @@ impl ValidatorClientHttpClient {
         let url = self.make_remotekeys_url()?;
         self.delete_with_unsigned_response(url, req).await
     }
+
+    /// `GET /eth/v1/validator/{pubkey}/feerecipient`
+    pub async fn get_fee_recipient(
+        &self,
+        pubkey: &PublicKeyBytes,
+    ) -> Result<GetFeeRecipientResponse, Error> {
+        let url = self.make_fee_recipient_url(pubkey)?;
+        self.get(url)
+            .await
+            .map(|generic: GenericResponse<GetFeeRecipientResponse>| generic.data)
+    }
+
+    /// `POST /eth/v1/validator/{pubkey}/feerecipient`
+    pub async fn post_fee_recipient(
+        &self,
+        pubkey: &PublicKeyBytes,
+        req: &UpdateFeeRecipientRequest,
+    ) -> Result<Response, Error> {
+        let url = self.make_fee_recipient_url(pubkey)?;
+        self.post_with_raw_response(url, req).await
+    }
+
+    /// `POST /eth/v1/validator/{pubkey}/feerecipient`
+    pub async fn delete_fee_recipient(&self, pubkey: &PublicKeyBytes) -> Result<Response, Error> {
+        let url = self.make_fee_recipient_url(pubkey)?;
+        self.delete_with_raw_response(url, &()).await
+    }
 }
 
-/// Returns `Ok(response)` if the response is a `200 OK` response. Otherwise, creates an
-/// appropriate error message.
+/// Returns `Ok(response)` if the response is a `200 OK` response or a
+/// `202 Accepted` response. Otherwise, creates an appropriate error message.
 async fn ok_or_error(response: Response) -> Result<Response, Error> {
     let status = response.status();
 
-    if status == StatusCode::OK {
+    if status == StatusCode::OK
+        || status == StatusCode::ACCEPTED
+        || status == StatusCode::NO_CONTENT
+    {
         Ok(response)
     } else if let Ok(message) = response.json().await {
         Err(Error::ServerMessage(message))

common/eth2/src/lighthouse_vc/std_types.rs

@@ -2,7 +2,13 @@ use account_utils::ZeroizeString;
 use eth2_keystore::Keystore;
 use serde::{Deserialize, Serialize};
 use slashing_protection::interchange::Interchange;
-use types::PublicKeyBytes;
+use types::{Address, PublicKeyBytes};
+
+#[derive(Debug, Deserialize, Serialize, PartialEq)]
+pub struct GetFeeRecipientResponse {
+    pub pubkey: PublicKeyBytes,
+    pub ethaddress: Address,
+}
 
 #[derive(Debug, Deserialize, Serialize, PartialEq)]
 pub struct AuthResponse {

common/eth2/src/lighthouse_vc/types.rs

@@ -97,3 +97,8 @@ pub struct Web3SignerValidatorRequest {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub client_identity_password: Option<String>,
 }
+
+#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
+pub struct UpdateFeeRecipientRequest {
+    pub ethaddress: Address,
+}

common/warp_utils/src/reject.rs

@@ -205,8 +205,13 @@ pub async fn handle_rejection(err: warp::Rejection) -> Result<impl warp::Reply,
         code = StatusCode::FORBIDDEN;
         message = format!("FORBIDDEN: Invalid auth token: {}", e.0);
     } else if let Some(e) = err.find::<warp::reject::MissingHeader>() {
-        code = StatusCode::BAD_REQUEST;
-        message = format!("BAD_REQUEST: missing {} header", e.name());
+        if e.name().eq("Authorization") {
+            code = StatusCode::UNAUTHORIZED;
+            message = "UNAUTHORIZED: missing Authorization header".to_string();
+        } else {
+            code = StatusCode::BAD_REQUEST;
+            message = format!("BAD_REQUEST: missing {} header", e.name());
+        }
     } else if let Some(e) = err.find::<warp::reject::InvalidHeader>() {
         code = StatusCode::BAD_REQUEST;
         message = format!("BAD_REQUEST: invalid {} header", e.name());

lighthouse/tests/validator_client.rs

@@ -249,66 +249,6 @@ fn fee_recipient_flag() {
             )
         });
 }
-#[test]
-fn fee_recipient_file_flag() {
-    let dir = TempDir::new().expect("Unable to create temporary directory");
-    let mut file =
-        File::create(dir.path().join("fee_recipient.txt")).expect("Unable to create file");
-    let new_key = Keypair::random();
-    let pubkeybytes = PublicKeyBytes::from(new_key.pk);
-    let contents = "default:0x00000000219ab540356cbb839cbe05303d7705fa";
-    file.write_all(contents.as_bytes())
-        .expect("Unable to write to file");
-    CommandLineTest::new()
-        .flag(
-            "suggested-fee-recipient-file",
-            dir.path().join("fee_recipient.txt").as_os_str().to_str(),
-        )
-        .run()
-        .with_config(|config| {
-            // Public key not present so load default.
-            assert_eq!(
-                config
-                    .fee_recipient_file
-                    .clone()
-                    .unwrap()
-                    .load_fee_recipient(&pubkeybytes)
-                    .unwrap(),
-                Some(Address::from_str("0x00000000219ab540356cbb839cbe05303d7705fa").unwrap())
-            )
-        });
-}
-#[test]
-fn fee_recipient_file_with_pk_flag() {
-    let dir = TempDir::new().expect("Unable to create temporary directory");
-    let mut file =
-        File::create(dir.path().join("fee_recipient.txt")).expect("Unable to create file");
-    let new_key = Keypair::random();
-    let pubkeybytes = PublicKeyBytes::from(new_key.pk);
-    let contents = format!(
-        "{}:0x00000000219ab540356cbb839cbe05303d7705fa",
-        pubkeybytes.to_string()
-    );
-    file.write_all(contents.as_bytes())
-        .expect("Unable to write to file");
-    CommandLineTest::new()
-        .flag(
-            "suggested-fee-recipient-file",
-            dir.path().join("fee_recipient.txt").as_os_str().to_str(),
-        )
-        .run()
-        .with_config(|config| {
-            assert_eq!(
-                config
-                    .fee_recipient_file
-                    .clone()
-                    .unwrap()
-                    .load_fee_recipient(&pubkeybytes)
-                    .unwrap(),
-                Some(Address::from_str("0x00000000219ab540356cbb839cbe05303d7705fa").unwrap())
-            )
-        });
-}
 
 // Tests for HTTP flags.
 #[test]

testing/web3signer_tests/src/lib.rs

@@ -310,6 +310,7 @@ mod tests {
                 spec,
                 None,
                 slot_clock,
+                None,
                 executor,
                 log.clone(),
             );

validator_client/src/cli.rs

@@ -136,14 +136,6 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .value_name("FEE-RECIPIENT")
                 .takes_value(true)
         )
-        .arg(
-            Arg::with_name("suggested-fee-recipient-file")
-                .long("suggested-fee-recipient-file")
-                .help("The fallback address provided to the BN if nothing suitable is found \
-                           in the validator definitions.")
-                .value_name("FEE-RECIPIENT-FILE")
-                .takes_value(true)
-        )
         /* REST API related arguments */
         .arg(
             Arg::with_name("http")