From d5e2d98970f250bdeadc91daa0bc584d713c3f69 Mon Sep 17 00:00:00 2001 From: ethDreamer Date: Wed, 6 Jul 2022 03:51:08 +0000 Subject: [PATCH] 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 | 132 ++++++++++--- common/eth2/src/lighthouse_vc/http_client.rs | 63 +++++- common/eth2/src/lighthouse_vc/std_types.rs | 8 +- common/eth2/src/lighthouse_vc/types.rs | 5 + common/warp_utils/src/reject.rs | 9 +- lighthouse/tests/validator_client.rs | 60 ------ testing/web3signer_tests/src/lib.rs | 1 + validator_client/src/cli.rs | 8 - validator_client/src/config.rs | 17 -- validator_client/src/fee_recipient_file.rs | 184 ------------------ validator_client/src/http_api/mod.rs | 132 ++++++++++++- validator_client/src/http_api/tests.rs | 4 +- .../src/http_api/tests/keystores.rs | 182 +++++++++++++++++ .../src/initialized_validators.rs | 72 +++++++ validator_client/src/lib.rs | 4 +- validator_client/src/preparation_service.rs | 57 +----- validator_client/src/validator_store.rs | 19 +- 17 files changed, 583 insertions(+), 374 deletions(-) delete mode 100644 validator_client/src/fee_recipient_file.rs diff --git a/book/src/suggested-fee-recipient.md b/book/src/suggested-fee-recipient.md index 3ff71ec7d..5c77081c3 100644 --- a/book/src/suggested-fee-recipient.md +++ b/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 - -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. - -Usage: -`lighthouse vc --suggested-fee-recipient-file fee_recipient.txt` - -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 following example sets the default and the values for the validators with pubkeys `0x87a5` and -`0xa556`: - -``` -default: 0x6cc8dcbca744a6e4ffedb98e1d0df903b10abd21 -0x87a580d31d7bc69069b55f5a01995a610dd391a26dc9e36e81057a17211983a79266800ab8531f21f1083d7d84085007: 0x6cc8dcbca744a6e4ffedb98e1d0df903b10abd21 -0xa5566f9ec3c6e1fdf362634ebec9ef7aceb0e460e5079714808388e5d48f4ae1e12897fed1bea951c17fa389d511e477: 0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d -``` - -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). - -### 3. Using the "--suggested-fee-recipient" flag on the validator client +### 2. Using the "--suggested-fee-recipient" flag on the validator client 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. -### 4. Using the "--suggested-fee-recipient" flag on the beacon node +### 3. 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. + +## 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" +} +``` + +```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 +``` + +#### Successful Response (202) +```json +null +``` + +### Querying the fee recipient + +The same path with a `GET` request can be used to query the fee recipient for a given public key at any time. + +| 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 +``` + + diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index 5e02ec0bb..d678ca34b 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/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( + async fn delete_with_raw_response( &self, url: U, body: &T, - ) -> Result { + ) -> Result { 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( + &self, + url: U, + body: &T, + ) -> Result { + 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 { + 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 { 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 { + let url = self.make_fee_recipient_url(pubkey)?; + self.get(url) + .await + .map(|generic: GenericResponse| generic.data) + } + + /// `POST /eth/v1/validator/{pubkey}/feerecipient` + pub async fn post_fee_recipient( + &self, + pubkey: &PublicKeyBytes, + req: &UpdateFeeRecipientRequest, + ) -> Result { + 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 { + 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 { 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)) diff --git a/common/eth2/src/lighthouse_vc/std_types.rs b/common/eth2/src/lighthouse_vc/std_types.rs index d9fe96913..62987c136 100644 --- a/common/eth2/src/lighthouse_vc/std_types.rs +++ b/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 { diff --git a/common/eth2/src/lighthouse_vc/types.rs b/common/eth2/src/lighthouse_vc/types.rs index fe9b6a48c..3e1c13dcf 100644 --- a/common/eth2/src/lighthouse_vc/types.rs +++ b/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, } + +#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)] +pub struct UpdateFeeRecipientRequest { + pub ethaddress: Address, +} diff --git a/common/warp_utils/src/reject.rs b/common/warp_utils/src/reject.rs index f5ce1156e..cf3d11af8 100644 --- a/common/warp_utils/src/reject.rs +++ b/common/warp_utils/src/reject.rs @@ -205,8 +205,13 @@ pub async fn handle_rejection(err: warp::Rejection) -> Result() { - 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::() { code = StatusCode::BAD_REQUEST; message = format!("BAD_REQUEST: invalid {} header", e.name()); diff --git a/lighthouse/tests/validator_client.rs b/lighthouse/tests/validator_client.rs index 61c239f86..4ff543468 100644 --- a/lighthouse/tests/validator_client.rs +++ b/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] diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index e39e6515f..5803f360a 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -310,6 +310,7 @@ mod tests { spec, None, slot_clock, + None, executor, log.clone(), ); diff --git a/validator_client/src/cli.rs b/validator_client/src/cli.rs index d02e26ace..414be2d90 100644 --- a/validator_client/src/cli.rs +++ b/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") diff --git a/validator_client/src/config.rs b/validator_client/src/config.rs index e56e64f5a..ddbe7f363 100644 --- a/validator_client/src/config.rs +++ b/validator_client/src/config.rs @@ -1,4 +1,3 @@ -use crate::fee_recipient_file::FeeRecipientFile; use crate::graffiti_file::GraffitiFile; use crate::{http_api, http_metrics}; use clap::ArgMatches; @@ -44,8 +43,6 @@ pub struct Config { pub graffiti_file: Option, /// Fallback fallback address. pub fee_recipient: Option
, - /// Fee recipient file to load per validator suggested-fee-recipients. - pub fee_recipient_file: Option, /// Configuration for the HTTP REST API. pub http_api: http_api::Config, /// Configuration for the HTTP REST API. @@ -86,7 +83,6 @@ impl Default for Config { graffiti: None, graffiti_file: None, fee_recipient: None, - fee_recipient_file: None, http_api: <_>::default(), http_metrics: <_>::default(), monitoring_api: None, @@ -206,19 +202,6 @@ impl Config { } } - if let Some(fee_recipient_file_path) = cli_args.value_of("suggested-fee-recipient-file") { - let mut fee_recipient_file = FeeRecipientFile::new(fee_recipient_file_path.into()); - fee_recipient_file - .read_fee_recipient_file() - .map_err(|e| format!("Error reading suggested-fee-recipient file: {:?}", e))?; - config.fee_recipient_file = Some(fee_recipient_file); - info!( - log, - "Successfully loaded suggested-fee-recipient file"; - "path" => fee_recipient_file_path - ); - } - if let Some(input_fee_recipient) = parse_optional::
(cli_args, "suggested-fee-recipient")? { diff --git a/validator_client/src/fee_recipient_file.rs b/validator_client/src/fee_recipient_file.rs deleted file mode 100644 index 637ca6d3d..000000000 --- a/validator_client/src/fee_recipient_file.rs +++ /dev/null @@ -1,184 +0,0 @@ -use serde_derive::{Deserialize, Serialize}; -use std::collections::HashMap; -use std::fs::File; -use std::io::{prelude::*, BufReader}; -use std::path::PathBuf; -use std::str::FromStr; - -use bls::PublicKeyBytes; -use types::Address; - -#[derive(Debug)] -#[allow(clippy::enum_variant_names)] -pub enum Error { - InvalidFile(std::io::Error), - InvalidLine(String), - InvalidPublicKey(String), - InvalidFeeRecipient(String), -} - -/// Struct to load validator fee-recipients from file. -/// The fee-recipient file is expected to have the following structure -/// -/// default: 0x00000000219ab540356cbb839cbe05303d7705fa -/// public_key1: fee-recipient1 -/// public_key2: fee-recipient2 -/// ... -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct FeeRecipientFile { - fee_recipient_path: PathBuf, - fee_recipients: HashMap, - default: Option
, -} - -impl FeeRecipientFile { - pub fn new(fee_recipient_path: PathBuf) -> Self { - Self { - fee_recipient_path, - fee_recipients: HashMap::new(), - default: None, - } - } - - /// Returns the fee-recipient corresponding to the given public key if present, else returns the - /// default fee-recipient. - /// - /// Returns an error if loading from the fee-recipient file fails. - pub fn get_fee_recipient(&self, public_key: &PublicKeyBytes) -> Result, Error> { - Ok(self - .fee_recipients - .get(public_key) - .copied() - .or(self.default)) - } - - /// Loads the fee-recipient file and populates the default fee-recipient and `fee_recipients` hashmap. - /// Returns the fee-recipient corresponding to the given public key if present, else returns the - /// default fee-recipient. - /// - /// Returns an error if loading from the fee-recipient file fails. - pub fn load_fee_recipient( - &mut self, - public_key: &PublicKeyBytes, - ) -> Result, Error> { - self.read_fee_recipient_file()?; - Ok(self - .fee_recipients - .get(public_key) - .copied() - .or(self.default)) - } - - /// Reads from a fee-recipient file with the specified format and populates the default value - /// and the hashmap. - /// - /// Returns an error if the file does not exist, or if the format is invalid. - pub fn read_fee_recipient_file(&mut self) -> Result<(), Error> { - let file = File::open(self.fee_recipient_path.as_path()).map_err(Error::InvalidFile)?; - let reader = BufReader::new(file); - - let lines = reader.lines(); - - self.default = None; - self.fee_recipients.clear(); - - for line in lines { - let line = line.map_err(|e| Error::InvalidLine(e.to_string()))?; - let (pk_opt, fee_recipient) = read_line(&line)?; - match pk_opt { - Some(pk) => { - self.fee_recipients.insert(pk, fee_recipient); - } - None => self.default = Some(fee_recipient), - } - } - Ok(()) - } -} - -/// Parses a line from the fee-recipient file. -/// -/// `Ok((None, fee_recipient))` represents the fee-recipient for the default key. -/// `Ok((Some(pk), fee_recipient))` represents fee-recipient for the public key `pk`. -/// Returns an error if the line is in the wrong format or does not contain a valid public key or fee-recipient. -fn read_line(line: &str) -> Result<(Option, Address), Error> { - if let Some(i) = line.find(':') { - let (key, value) = line.split_at(i); - // Note: `value.len() >=1` so `value[1..]` is safe - let fee_recipient = Address::from_str(value[1..].trim()) - .map_err(|e| Error::InvalidFeeRecipient(e.to_string()))?; - if key == "default" { - Ok((None, fee_recipient)) - } else { - let pk = PublicKeyBytes::from_str(key).map_err(Error::InvalidPublicKey)?; - Ok((Some(pk), fee_recipient)) - } - } else { - Err(Error::InvalidLine(format!("Missing delimiter: {}", line))) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use bls::Keypair; - use std::io::LineWriter; - use tempfile::TempDir; - - const DEFAULT_FEE_RECIPIENT: &str = "0x00000000219ab540356cbb839cbe05303d7705fa"; - const CUSTOM_FEE_RECIPIENT1: &str = "0x4242424242424242424242424242424242424242"; - const CUSTOM_FEE_RECIPIENT2: &str = "0x0000000000000000000000000000000000000001"; - const PK1: &str = "0x800012708dc03f611751aad7a43a082142832b5c1aceed07ff9b543cf836381861352aa923c70eeb02018b638aa306aa"; - const PK2: &str = "0x80001866ce324de7d80ec73be15e2d064dcf121adf1b34a0d679f2b9ecbab40ce021e03bb877e1a2fe72eaaf475e6e21"; - - // Create a fee-recipient file in the required format and return a path to the file. - fn create_fee_recipient_file() -> PathBuf { - let temp = TempDir::new().unwrap(); - let pk1 = PublicKeyBytes::deserialize(&hex::decode(&PK1[2..]).unwrap()).unwrap(); - let pk2 = PublicKeyBytes::deserialize(&hex::decode(&PK2[2..]).unwrap()).unwrap(); - - let file_name = temp.into_path().join("fee_recipient.txt"); - - let file = File::create(&file_name).unwrap(); - let mut fee_recipient_file = LineWriter::new(file); - fee_recipient_file - .write_all(format!("default: {}\n", DEFAULT_FEE_RECIPIENT).as_bytes()) - .unwrap(); - fee_recipient_file - .write_all(format!("{}: {}\n", pk1.as_hex_string(), CUSTOM_FEE_RECIPIENT1).as_bytes()) - .unwrap(); - fee_recipient_file - .write_all(format!("{}: {}\n", pk2.as_hex_string(), CUSTOM_FEE_RECIPIENT2).as_bytes()) - .unwrap(); - fee_recipient_file.flush().unwrap(); - file_name - } - - #[test] - fn test_load_fee_recipient() { - let fee_recipient_file_path = create_fee_recipient_file(); - let mut gf = FeeRecipientFile::new(fee_recipient_file_path); - - let pk1 = PublicKeyBytes::deserialize(&hex::decode(&PK1[2..]).unwrap()).unwrap(); - let pk2 = PublicKeyBytes::deserialize(&hex::decode(&PK2[2..]).unwrap()).unwrap(); - - // Read once - gf.read_fee_recipient_file().unwrap(); - - assert_eq!( - gf.load_fee_recipient(&pk1).unwrap().unwrap(), - Address::from_str(CUSTOM_FEE_RECIPIENT1).unwrap() - ); - assert_eq!( - gf.load_fee_recipient(&pk2).unwrap().unwrap(), - Address::from_str(CUSTOM_FEE_RECIPIENT2).unwrap() - ); - - // Random pk should return the default fee-recipient - let random_pk = Keypair::random().pk.compress(); - assert_eq!( - gf.load_fee_recipient(&random_pk).unwrap().unwrap(), - Address::from_str(DEFAULT_FEE_RECIPIENT).unwrap() - ); - } -} diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index 9ee983a35..56218cd81 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -9,10 +9,11 @@ use account_utils::{ mnemonic_from_phrase, validator_definitions::{SigningDefinition, ValidatorDefinition}, }; +pub use api_secret::ApiSecret; use create_validator::{create_validators_mnemonic, create_validators_web3signer}; use eth2::lighthouse_vc::{ - std_types::AuthResponse, - types::{self as api_types, PublicKey, PublicKeyBytes}, + std_types::{AuthResponse, GetFeeRecipientResponse}, + types::{self as api_types, GenericResponse, PublicKey, PublicKeyBytes}, }; use lighthouse_version::version_with_platform; use serde::{Deserialize, Serialize}; @@ -35,8 +36,6 @@ use warp::{ Filter, }; -pub use api_secret::ApiSecret; - #[derive(Debug)] pub enum Error { Warp(warp::Error), @@ -562,6 +561,123 @@ pub fn serve( let std_keystores = eth_v1.and(warp::path("keystores")).and(warp::path::end()); let std_remotekeys = eth_v1.and(warp::path("remotekeys")).and(warp::path::end()); + // GET /eth/v1/validator/{pubkey}/feerecipient + let get_fee_recipient = eth_v1 + .and(warp::path("validator")) + .and(warp::path::param::()) + .and(warp::path("feerecipient")) + .and(warp::path::end()) + .and(validator_store_filter.clone()) + .and(signer.clone()) + .and_then( + |validator_pubkey: PublicKey, validator_store: Arc>, signer| { + blocking_signed_json_task(signer, move || { + if validator_store + .initialized_validators() + .read() + .is_enabled(&validator_pubkey) + .is_none() + { + return Err(warp_utils::reject::custom_not_found(format!( + "no validator found with pubkey {:?}", + validator_pubkey + ))); + } + validator_store + .get_fee_recipient(&PublicKeyBytes::from(&validator_pubkey)) + .map(|fee_recipient| { + GenericResponse::from(GetFeeRecipientResponse { + pubkey: PublicKeyBytes::from(validator_pubkey.clone()), + ethaddress: fee_recipient, + }) + }) + .ok_or_else(|| { + warp_utils::reject::custom_server_error( + "no fee recipient set".to_string(), + ) + }) + }) + }, + ); + + // POST /eth/v1/validator/{pubkey}/feerecipient + let post_fee_recipient = eth_v1 + .and(warp::path("validator")) + .and(warp::path::param::()) + .and(warp::body::json()) + .and(warp::path("feerecipient")) + .and(warp::path::end()) + .and(validator_store_filter.clone()) + .and(signer.clone()) + .and_then( + |validator_pubkey: PublicKey, + request: api_types::UpdateFeeRecipientRequest, + validator_store: Arc>, + signer| { + blocking_signed_json_task(signer, move || { + if validator_store + .initialized_validators() + .read() + .is_enabled(&validator_pubkey) + .is_none() + { + return Err(warp_utils::reject::custom_not_found(format!( + "no validator found with pubkey {:?}", + validator_pubkey + ))); + } + validator_store + .initialized_validators() + .write() + .set_validator_fee_recipient(&validator_pubkey, request.ethaddress) + .map_err(|e| { + warp_utils::reject::custom_server_error(format!( + "Error persisting fee recipient: {:?}", + e + )) + }) + }) + }, + ) + .map(|reply| warp::reply::with_status(reply, warp::http::StatusCode::ACCEPTED)); + + // DELETE /eth/v1/validator/{pubkey}/feerecipient + let delete_fee_recipient = eth_v1 + .and(warp::path("validator")) + .and(warp::path::param::()) + .and(warp::path("feerecipient")) + .and(warp::path::end()) + .and(validator_store_filter.clone()) + .and(signer.clone()) + .and_then( + |validator_pubkey: PublicKey, validator_store: Arc>, signer| { + blocking_signed_json_task(signer, move || { + if validator_store + .initialized_validators() + .read() + .is_enabled(&validator_pubkey) + .is_none() + { + return Err(warp_utils::reject::custom_not_found(format!( + "no validator found with pubkey {:?}", + validator_pubkey + ))); + } + validator_store + .initialized_validators() + .write() + .delete_validator_fee_recipient(&validator_pubkey) + .map_err(|e| { + warp_utils::reject::custom_server_error(format!( + "Error persisting fee recipient removal: {:?}", + e + )) + }) + }) + }, + ) + .map(|reply| warp::reply::with_status(reply, warp::http::StatusCode::NO_CONTENT)); + // GET /eth/v1/keystores let get_std_keystores = std_keystores .and(signer.clone()) @@ -647,6 +763,7 @@ pub fn serve( .or(get_lighthouse_spec) .or(get_lighthouse_validators) .or(get_lighthouse_validators_pubkey) + .or(get_fee_recipient) .or(get_std_keystores) .or(get_std_remotekeys), ) @@ -655,11 +772,16 @@ pub fn serve( .or(post_validators_keystore) .or(post_validators_mnemonic) .or(post_validators_web3signer) + .or(post_fee_recipient) .or(post_std_keystores) .or(post_std_remotekeys), )) .or(warp::patch().and(patch_validators)) - .or(warp::delete().and(delete_std_keystores.or(delete_std_remotekeys))), + .or(warp::delete().and( + delete_fee_recipient + .or(delete_std_keystores) + .or(delete_std_remotekeys), + )), ) // The auth route is the only route that is allowed to be accessed without the API token. .or(warp::get().and(get_auth)) diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index 210555d9c..7ee056341 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -36,6 +36,7 @@ use tokio::runtime::Runtime; use tokio::sync::oneshot; const PASSWORD_BYTES: &[u8] = &[42, 50, 37]; +pub const TEST_DEFAULT_FEE_RECIPIENT: Address = Address::repeat_byte(42); type E = MainnetEthSpec; @@ -102,6 +103,7 @@ impl ApiTester { spec, Some(Arc::new(DoppelgangerService::new(log.clone()))), slot_clock, + Some(TEST_DEFAULT_FEE_RECIPIENT), executor.clone(), log.clone(), )); @@ -185,7 +187,7 @@ impl ApiTester { missing_token_client.send_authorization_header(false); match func(missing_token_client).await { Err(ApiError::ServerMessage(ApiErrorMessage { - code: 400, message, .. + code: 401, message, .. })) if message.contains("missing Authorization header") => (), Err(other) => panic!("expected missing header error, got {:?}", other), Ok(_) => panic!("expected missing header error, got Ok"), diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index a381378ff..530993ee0 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -1,5 +1,7 @@ use super::*; use account_utils::random_password_string; +use bls::PublicKeyBytes; +use eth2::lighthouse_vc::types::UpdateFeeRecipientRequest; use eth2::lighthouse_vc::{ http_client::ValidatorClientHttpClient as HttpClient, std_types::{KeystoreJsonStr as Keystore, *}, @@ -9,6 +11,7 @@ use itertools::Itertools; use rand::{rngs::SmallRng, Rng, SeedableRng}; use slashing_protection::interchange::{Interchange, InterchangeMetadata}; use std::{collections::HashMap, path::Path}; +use types::Address; fn new_keystore(password: ZeroizeString) -> Keystore { let keypair = Keypair::random(); @@ -585,6 +588,185 @@ fn import_invalid_slashing_protection() { }) } +#[test] +fn check_get_set_fee_recipient() { + run_test(|tester: ApiTester| async move { + let _ = &tester; + let password = random_password_string(); + let keystores = (0..3) + .map(|_| new_keystore(password.clone())) + .collect::>(); + let all_pubkeys = keystores.iter().map(keystore_pubkey).collect::>(); + + let import_res = tester + .client + .post_keystores(&ImportKeystoresRequest { + keystores: keystores.clone(), + passwords: vec![password.clone(); keystores.len()], + slashing_protection: None, + }) + .await + .unwrap(); + + // All keystores should be imported. + check_keystore_import_response(&import_res, all_imported(keystores.len())); + + // Check that GET lists all the imported keystores. + let get_res = tester.client.get_keystores().await.unwrap(); + check_keystore_get_response(&get_res, &keystores); + + // Before setting anything, every fee recipient should be set to TEST_DEFAULT_FEE_RECIPIENT + for pubkey in &all_pubkeys { + let get_res = tester + .client + .get_fee_recipient(pubkey) + .await + .expect("should get fee recipient"); + assert_eq!( + get_res, + GetFeeRecipientResponse { + pubkey: pubkey.clone(), + ethaddress: TEST_DEFAULT_FEE_RECIPIENT, + } + ); + } + + use std::str::FromStr; + let fee_recipient_public_key_1 = + Address::from_str("0x25c4a76E7d118705e7Ea2e9b7d8C59930d8aCD3b").unwrap(); + let fee_recipient_public_key_2 = + Address::from_str("0x0000000000000000000000000000000000000001").unwrap(); + let fee_recipient_override = + Address::from_str("0x0123456789abcdef0123456789abcdef01234567").unwrap(); + + // set the fee recipient for pubkey[1] using the API + tester + .client + .post_fee_recipient( + &all_pubkeys[1], + &UpdateFeeRecipientRequest { + ethaddress: fee_recipient_public_key_1.clone(), + }, + ) + .await + .expect("should update fee recipient"); + // now everything but pubkey[1] should be TEST_DEFAULT_FEE_RECIPIENT + for (i, pubkey) in all_pubkeys.iter().enumerate() { + let get_res = tester + .client + .get_fee_recipient(pubkey) + .await + .expect("should get fee recipient"); + let expected = if i == 1 { + fee_recipient_public_key_1.clone() + } else { + TEST_DEFAULT_FEE_RECIPIENT + }; + assert_eq!( + get_res, + GetFeeRecipientResponse { + pubkey: pubkey.clone(), + ethaddress: expected, + } + ); + } + + // set the fee recipient for pubkey[2] using the API + tester + .client + .post_fee_recipient( + &all_pubkeys[2], + &UpdateFeeRecipientRequest { + ethaddress: fee_recipient_public_key_2.clone(), + }, + ) + .await + .expect("should update fee recipient"); + // now everything but pubkey[1] & pubkey[2] should be fee_recipient_file_default + for (i, pubkey) in all_pubkeys.iter().enumerate() { + let get_res = tester + .client + .get_fee_recipient(pubkey) + .await + .expect("should get fee recipient"); + let expected = if i == 1 { + fee_recipient_public_key_1.clone() + } else if i == 2 { + fee_recipient_public_key_2.clone() + } else { + TEST_DEFAULT_FEE_RECIPIENT + }; + assert_eq!( + get_res, + GetFeeRecipientResponse { + pubkey: pubkey.clone(), + ethaddress: expected, + } + ); + } + + // should be able to override previous fee_recipient + tester + .client + .post_fee_recipient( + &all_pubkeys[1], + &UpdateFeeRecipientRequest { + ethaddress: fee_recipient_override.clone(), + }, + ) + .await + .expect("should update fee recipient"); + for (i, pubkey) in all_pubkeys.iter().enumerate() { + let get_res = tester + .client + .get_fee_recipient(pubkey) + .await + .expect("should get fee recipient"); + let expected = if i == 1 { + fee_recipient_override.clone() + } else if i == 2 { + fee_recipient_public_key_2.clone() + } else { + TEST_DEFAULT_FEE_RECIPIENT + }; + assert_eq!( + get_res, + GetFeeRecipientResponse { + pubkey: pubkey.clone(), + ethaddress: expected, + } + ); + } + + // delete fee recipient for pubkey[1] using the API + tester + .client + .delete_fee_recipient(&all_pubkeys[1]) + .await + .expect("should delete fee recipient"); + // now everything but pubkey[2] should be TEST_DEFAULT_FEE_RECIPIENT + for (i, pubkey) in all_pubkeys.iter().enumerate() { + let get_res = tester + .client + .get_fee_recipient(pubkey) + .await + .expect("should get fee recipient"); + let expected = if i == 2 { + fee_recipient_public_key_2.clone() + } else { + TEST_DEFAULT_FEE_RECIPIENT + }; + assert_eq!( + get_res, + GetFeeRecipientResponse { + pubkey: pubkey.clone(), + ethaddress: expected, + } + ); + } + }) +} + fn all_indices(count: usize) -> Vec { (0..count).collect() } diff --git a/validator_client/src/initialized_validators.rs b/validator_client/src/initialized_validators.rs index 0d5d4ad76..a0fe6dfe2 100644 --- a/validator_client/src/initialized_validators.rs +++ b/validator_client/src/initialized_validators.rs @@ -617,6 +617,78 @@ impl InitializedValidators { Ok(()) } + /// Sets the `InitializedValidator` and `ValidatorDefinition` `suggested_fee_recipient` values. + /// + /// ## Notes + /// + /// Setting a validator `fee_recipient` will cause `self.definitions` to be updated and saved to + /// disk. + /// + /// Saves the `ValidatorDefinitions` to file, even if no definitions were changed. + pub fn set_validator_fee_recipient( + &mut self, + voting_public_key: &PublicKey, + fee_recipient: Address, + ) -> Result<(), Error> { + if let Some(def) = self + .definitions + .as_mut_slice() + .iter_mut() + .find(|def| def.voting_public_key == *voting_public_key) + { + def.suggested_fee_recipient = Some(fee_recipient); + } + + if let Some(val) = self + .validators + .get_mut(&PublicKeyBytes::from(voting_public_key)) + { + val.suggested_fee_recipient = Some(fee_recipient); + } + + self.definitions + .save(&self.validators_dir) + .map_err(Error::UnableToSaveDefinitions)?; + + Ok(()) + } + + /// Removes the `InitializedValidator` and `ValidatorDefinition` `suggested_fee_recipient` values. + /// + /// ## Notes + /// + /// Removing a validator `fee_recipient` will cause `self.definitions` to be updated and saved to + /// disk. The fee_recipient for the validator will then fall back to the process level default if + /// it is set. + /// + /// Saves the `ValidatorDefinitions` to file, even if no definitions were changed. + pub fn delete_validator_fee_recipient( + &mut self, + voting_public_key: &PublicKey, + ) -> Result<(), Error> { + if let Some(def) = self + .definitions + .as_mut_slice() + .iter_mut() + .find(|def| def.voting_public_key == *voting_public_key) + { + def.suggested_fee_recipient = None; + } + + if let Some(val) = self + .validators + .get_mut(&PublicKeyBytes::from(voting_public_key)) + { + val.suggested_fee_recipient = None; + } + + self.definitions + .save(&self.validators_dir) + .map_err(Error::UnableToSaveDefinitions)?; + + Ok(()) + } + /// Tries to decrypt the key cache. /// /// Returns the decrypted cache if decryption was successful, or an error if a required password diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 5e4584759..a69d6a9f5 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -5,7 +5,6 @@ mod check_synced; mod cli; mod config; mod duties_service; -mod fee_recipient_file; mod graffiti_file; mod http_metrics; mod key_cache; @@ -360,6 +359,7 @@ impl ProductionValidatorClient { context.eth2_config.spec.clone(), doppelganger_service.clone(), slot_clock.clone(), + config.fee_recipient, context.executor.clone(), log.clone(), )); @@ -426,8 +426,6 @@ impl ProductionValidatorClient { .validator_store(validator_store.clone()) .beacon_nodes(beacon_nodes.clone()) .runtime_context(context.service_context("preparation".into())) - .fee_recipient(config.fee_recipient) - .fee_recipient_file(config.fee_recipient_file.clone()) .build()?; let sync_committee_service = SyncCommitteeService::new( diff --git a/validator_client/src/preparation_service.rs b/validator_client/src/preparation_service.rs index 34201180c..01dfc0ca0 100644 --- a/validator_client/src/preparation_service.rs +++ b/validator_client/src/preparation_service.rs @@ -1,8 +1,5 @@ use crate::beacon_node_fallback::{BeaconNodeFallback, RequireSynced}; -use crate::{ - fee_recipient_file::FeeRecipientFile, - validator_store::{DoppelgangerStatus, ValidatorStore}, -}; +use crate::validator_store::{DoppelgangerStatus, ValidatorStore}; use bls::PublicKeyBytes; use environment::RuntimeContext; use parking_lot::RwLock; @@ -31,8 +28,6 @@ pub struct PreparationServiceBuilder { slot_clock: Option, beacon_nodes: Option>>, context: Option>, - fee_recipient: Option
, - fee_recipient_file: Option, } impl PreparationServiceBuilder { @@ -42,8 +37,6 @@ impl PreparationServiceBuilder { slot_clock: None, beacon_nodes: None, context: None, - fee_recipient: None, - fee_recipient_file: None, } } @@ -67,16 +60,6 @@ impl PreparationServiceBuilder { self } - pub fn fee_recipient(mut self, fee_recipient: Option
) -> Self { - self.fee_recipient = fee_recipient; - self - } - - pub fn fee_recipient_file(mut self, fee_recipient_file: Option) -> Self { - self.fee_recipient_file = fee_recipient_file; - self - } - pub fn build(self) -> Result, String> { Ok(PreparationService { inner: Arc::new(Inner { @@ -92,8 +75,6 @@ impl PreparationServiceBuilder { context: self .context .ok_or("Cannot build PreparationService without runtime_context")?, - fee_recipient: self.fee_recipient, - fee_recipient_file: self.fee_recipient_file, validator_registration_cache: RwLock::new(HashMap::new()), }), }) @@ -106,8 +87,6 @@ pub struct Inner { slot_clock: T, beacon_nodes: Arc>, context: RuntimeContext, - fee_recipient: Option
, - fee_recipient_file: Option, // Used to track unpublished validator registration changes. validator_registration_cache: RwLock>, @@ -301,23 +280,6 @@ impl PreparationService { { let log = self.context.log(); - let fee_recipient_file = self - .fee_recipient_file - .clone() - .map(|mut fee_recipient_file| { - fee_recipient_file - .read_fee_recipient_file() - .map_err(|e| { - error!( - log, - "Error loading fee-recipient file"; - "error" => ?e - ); - }) - .unwrap_or(()); - fee_recipient_file - }); - let all_pubkeys: Vec<_> = self .validator_store .voting_pubkeys(DoppelgangerStatus::ignored); @@ -327,22 +289,7 @@ impl PreparationService { .filter_map(|pubkey| { // Ignore fee recipients for keys without indices, they are inactive. let validator_index = self.validator_store.validator_index(&pubkey)?; - - // If there is a `suggested_fee_recipient` in the validator definitions yaml - // file, use that value. - let fee_recipient = self - .validator_store - .suggested_fee_recipient(&pubkey) - .or_else(|| { - // If there's nothing in the validator defs file, check the fee - // recipient file. - fee_recipient_file - .as_ref()? - .get_fee_recipient(&pubkey) - .ok()? - }) - // If there's nothing in the file, try the process-level default value. - .or(self.fee_recipient); + let fee_recipient = self.validator_store.get_fee_recipient(&pubkey); if let Some(fee_recipient) = fee_recipient { Some(map_fn(pubkey, validator_index, fee_recipient)) diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 36ec5e895..de39f9126 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -86,6 +86,7 @@ pub struct ValidatorStore { log: Logger, doppelganger_service: Option>, slot_clock: T, + fee_recipient_process: Option
, task_executor: TaskExecutor, _phantom: PhantomData, } @@ -101,6 +102,7 @@ impl ValidatorStore { spec: ChainSpec, doppelganger_service: Option>, slot_clock: T, + fee_recipient_process: Option
, task_executor: TaskExecutor, log: Logger, ) -> Self { @@ -113,6 +115,7 @@ impl ValidatorStore { log, doppelganger_service, slot_clock, + fee_recipient_process, task_executor, _phantom: PhantomData, } @@ -356,7 +359,21 @@ impl ValidatorStore { self.validators.read().graffiti(validator_pubkey) } - pub fn suggested_fee_recipient(&self, validator_pubkey: &PublicKeyBytes) -> Option
{ + /// Returns the fee recipient for the given public key. The priority order for fetching + /// the fee recipient is: + /// 1. validator_definitions.yml + /// 2. process level fee recipient + pub fn get_fee_recipient(&self, validator_pubkey: &PublicKeyBytes) -> Option
{ + // If there is a `suggested_fee_recipient` in the validator definitions yaml + // file, use that value. + self.suggested_fee_recipient(validator_pubkey) + // If there's nothing in the file, try the process-level default value. + .or(self.fee_recipient_process) + } + + /// Returns the suggested_fee_recipient from `validator_definitions.yml` if any. + /// This has been pulled into a private function so the read lock is dropped easily + fn suggested_fee_recipient(&self, validator_pubkey: &PublicKeyBytes) -> Option
{ self.validators .read() .suggested_fee_recipient(validator_pubkey)