From 33c942ff035268fb1ff1078707c1e5a4301a902b Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Thu, 22 Jun 2023 02:14:57 +0000 Subject: [PATCH] Add support for updating validator graffiti (#4417) ## Issue Addressed #4386 ## Proposed Changes The original proposal described in the issue adds a new endpoint to support updating validator graffiti, but I realized we already have an endpoint that we use for updating various validator fields in memory and in the validator definitions file, so I think that would be the best place to add this to. ### API endpoint `PATCH lighthouse/validators/{validator_pubkey}` This endpoint updates the graffiti in both the [ validator definition file](https://lighthouse-book.sigmaprime.io/graffiti.html#2-setting-the-graffiti-in-the-validator_definitionsyml) and the in memory `InitializedValidators`. In the next block proposal, the new graffiti will be used. Note that the [`--graffiti-file`](https://lighthouse-book.sigmaprime.io/graffiti.html#1-using-the---graffiti-file-flag-on-the-validator-client) flag has a priority over the validator definitions file, so if the caller attempts to update the graffiti while the `--graffiti-file` flag is present, the endpoint will return an error (Bad request 400). ## Tasks - [x] Add graffiti update support to `PATCH lighthouse/validators/{validator_pubkey}` - [x] Return error if user tries to update graffiti while the `--graffiti-flag` is set - [x] Update Lighthouse book --- book/src/api-vc-endpoints.md | 3 +- book/src/graffiti.md | 24 ++++++ common/eth2/src/lighthouse_vc/http_client.rs | 3 + common/eth2/src/lighthouse_vc/types.rs | 3 + validator_client/src/http_api/mod.rs | 17 +++- validator_client/src/http_api/tests.rs | 84 ++++++++++++++++++- .../src/http_api/tests/keystores.rs | 2 +- .../src/initialized_validators.rs | 18 +++- 8 files changed, 143 insertions(+), 11 deletions(-) diff --git a/book/src/api-vc-endpoints.md b/book/src/api-vc-endpoints.md index 406c5b1f0e..ee0cfd2001 100644 --- a/book/src/api-vc-endpoints.md +++ b/book/src/api-vc-endpoints.md @@ -426,7 +426,8 @@ Example Response Body ## `PATCH /lighthouse/validators/:voting_pubkey` -Update some values for the validator with `voting_pubkey`. The following example updates a validator from `enabled: true` to `enabled: false` +Update some values for the validator with `voting_pubkey`. Possible fields: `enabled`, `gas_limit`, `builder_proposals`, +and `graffiti`. The following example updates a validator from `enabled: true` to `enabled: false`. ### HTTP Specification diff --git a/book/src/graffiti.md b/book/src/graffiti.md index 75c2a86dd5..302f8f9679 100644 --- a/book/src/graffiti.md +++ b/book/src/graffiti.md @@ -29,6 +29,8 @@ Lighthouse will first search for the graffiti corresponding to the public key of ### 2. Setting the graffiti in the `validator_definitions.yml` Users can set validator specific graffitis in `validator_definitions.yml` with the `graffiti` key. This option is recommended for static setups where the graffitis won't change on every new block proposal. +You can also update the graffitis in the `validator_definitions.yml` file using the [Lighthouse API](api-vc-endpoints.html#patch-lighthousevalidatorsvoting_pubkey). See example in [Set Graffiti via HTTP](#set-graffiti-via-http). + Below is an example of the validator_definitions.yml with validator specific graffitis: ``` --- @@ -62,3 +64,25 @@ Usage: `lighthouse bn --graffiti fortytwo` > 3. If graffiti is not specified in `validator_definitions.yml`, load the graffiti passed in the `--graffiti` flag on the validator client. > 4. If the `--graffiti` flag on the validator client is not passed, load the graffiti passed in the `--graffiti` flag on the beacon node. > 4. If the `--graffiti` flag is not passed, load the default Lighthouse graffiti. + +### Set Graffiti via HTTP + +Use the [Lighthouse API](api-vc-endpoints.md) to set graffiti on a per-validator basis. This method updates the graffiti +both in memory and in the `validator_definitions.yml` file. The new graffiti will be used in the next block proposal +without requiring a validator client restart. + +Refer to [Lighthouse API](api-vc-endpoints.html#patch-lighthousevalidatorsvoting_pubkey) for API specification. + +#### Example Command + +```bash +DATADIR=/var/lib/lighthouse +curl -X PATCH "http://localhost:5062/lighthouse/validators/0xb0148e6348264131bf47bcd1829590e870c836dc893050fd0dadc7a28949f9d0a72f2805d027521b45441101f0cc1cde" \ +-H "Authorization: Bearer $(cat ${DATADIR}/validators/api-token.txt)" \ +-H "Content-Type: application/json" \ +-d '{ + "graffiti": "Mr F was here" +}' | jq +``` + +A `null` response indicates that the request is successful. \ No newline at end of file diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index e576cfcb36..720d8c7795 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -16,6 +16,7 @@ use std::path::Path; pub use reqwest; pub use reqwest::{Response, StatusCode, Url}; +use types::graffiti::GraffitiString; /// A wrapper around `reqwest::Client` which provides convenience methods for interfacing with a /// Lighthouse Validator Client HTTP server (`validator_client/src/http_api`). @@ -467,6 +468,7 @@ impl ValidatorClientHttpClient { enabled: Option, gas_limit: Option, builder_proposals: Option, + graffiti: Option, ) -> Result<(), Error> { let mut path = self.server.full.clone(); @@ -482,6 +484,7 @@ impl ValidatorClientHttpClient { enabled, gas_limit, builder_proposals, + graffiti, }, ) .await diff --git a/common/eth2/src/lighthouse_vc/types.rs b/common/eth2/src/lighthouse_vc/types.rs index dd2ed03221..7bbe041dbd 100644 --- a/common/eth2/src/lighthouse_vc/types.rs +++ b/common/eth2/src/lighthouse_vc/types.rs @@ -83,6 +83,9 @@ pub struct ValidatorPatchRequest { #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub builder_proposals: Option, + #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] + pub graffiti: Option, } #[derive(Clone, PartialEq, Serialize, Deserialize)] diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index fa6cde3ed5..f08c8da1bd 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -357,7 +357,7 @@ pub fn serve( .and(warp::path("graffiti")) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(graffiti_file_filter) + .and(graffiti_file_filter.clone()) .and(graffiti_flag_filter) .and(signer.clone()) .and(log_filter.clone()) @@ -617,18 +617,27 @@ pub fn serve( .and(warp::path::end()) .and(warp::body::json()) .and(validator_store_filter.clone()) + .and(graffiti_file_filter) .and(signer.clone()) .and(task_executor_filter.clone()) .and_then( |validator_pubkey: PublicKey, body: api_types::ValidatorPatchRequest, validator_store: Arc>, + graffiti_file: Option, signer, task_executor: TaskExecutor| { blocking_signed_json_task(signer, move || { + if body.graffiti.is_some() && graffiti_file.is_some() { + return Err(warp_utils::reject::custom_bad_request( + "Unable to update graffiti as the \"--graffiti-file\" flag is set" + .to_string(), + )); + } + + let maybe_graffiti = body.graffiti.clone().map(Into::into); let initialized_validators_rw_lock = validator_store.initialized_validators(); let mut initialized_validators = initialized_validators_rw_lock.write(); - match ( initialized_validators.is_enabled(&validator_pubkey), initialized_validators.validator(&validator_pubkey.compress()), @@ -641,7 +650,8 @@ pub fn serve( if Some(is_enabled) == body.enabled && initialized_validator.get_gas_limit() == body.gas_limit && initialized_validator.get_builder_proposals() - == body.builder_proposals => + == body.builder_proposals + && initialized_validator.get_graffiti() == maybe_graffiti => { Ok(()) } @@ -654,6 +664,7 @@ pub fn serve( body.enabled, body.gas_limit, body.builder_proposals, + body.graffiti, ), ) .map_err(|e| { diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index 84d2fe437d..dbb9d4d620 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -28,12 +28,14 @@ use slot_clock::{SlotClock, TestingSlotClock}; use std::future::Future; use std::marker::PhantomData; use std::net::{IpAddr, Ipv4Addr}; +use std::str::FromStr; use std::sync::Arc; use std::time::Duration; use task_executor::TaskExecutor; use tempfile::{tempdir, TempDir}; use tokio::runtime::Runtime; use tokio::sync::oneshot; +use types::graffiti::GraffitiString; const PASSWORD_BYTES: &[u8] = &[42, 50, 37]; pub const TEST_DEFAULT_FEE_RECIPIENT: Address = Address::repeat_byte(42); @@ -533,7 +535,7 @@ impl ApiTester { let validator = &self.client.get_lighthouse_validators().await.unwrap().data[index]; self.client - .patch_lighthouse_validators(&validator.voting_pubkey, Some(enabled), None, None) + .patch_lighthouse_validators(&validator.voting_pubkey, Some(enabled), None, None, None) .await .unwrap(); @@ -575,7 +577,13 @@ impl ApiTester { let validator = &self.client.get_lighthouse_validators().await.unwrap().data[index]; self.client - .patch_lighthouse_validators(&validator.voting_pubkey, None, Some(gas_limit), None) + .patch_lighthouse_validators( + &validator.voting_pubkey, + None, + Some(gas_limit), + None, + None, + ) .await .unwrap(); @@ -602,6 +610,7 @@ impl ApiTester { None, None, Some(builder_proposals), + None, ) .await .unwrap(); @@ -620,6 +629,34 @@ impl ApiTester { self } + + pub async fn set_graffiti(self, index: usize, graffiti: &str) -> Self { + let validator = &self.client.get_lighthouse_validators().await.unwrap().data[index]; + let graffiti_str = GraffitiString::from_str(graffiti).unwrap(); + self.client + .patch_lighthouse_validators( + &validator.voting_pubkey, + None, + None, + None, + Some(graffiti_str), + ) + .await + .unwrap(); + + self + } + + pub async fn assert_graffiti(self, index: usize, graffiti: &str) -> Self { + let validator = &self.client.get_lighthouse_validators().await.unwrap().data[index]; + let graffiti_str = GraffitiString::from_str(graffiti).unwrap(); + assert_eq!( + self.validator_store.graffiti(&validator.voting_pubkey), + Some(graffiti_str.into()) + ); + + self + } } struct HdValidatorScenario { @@ -723,7 +760,13 @@ fn routes_with_invalid_auth() { .await .test_with_invalid_auth(|client| async move { client - .patch_lighthouse_validators(&PublicKeyBytes::empty(), Some(false), None, None) + .patch_lighthouse_validators( + &PublicKeyBytes::empty(), + Some(false), + None, + None, + None, + ) .await }) .await @@ -931,6 +974,41 @@ fn validator_builder_proposals() { }); } +#[test] +fn validator_graffiti() { + let runtime = build_runtime(); + let weak_runtime = Arc::downgrade(&runtime); + runtime.block_on(async { + ApiTester::new(weak_runtime) + .await + .create_hd_validators(HdValidatorScenario { + count: 2, + specify_mnemonic: false, + key_derivation_path_offset: 0, + disabled: vec![], + }) + .await + .assert_enabled_validators_count(2) + .assert_validators_count(2) + .set_graffiti(0, "Mr F was here") + .await + .assert_graffiti(0, "Mr F was here") + .await + // Test setting graffiti while the validator is disabled + .set_validator_enabled(0, false) + .await + .assert_enabled_validators_count(1) + .assert_validators_count(2) + .set_graffiti(0, "Mr F was here again") + .await + .set_validator_enabled(0, true) + .await + .assert_enabled_validators_count(2) + .assert_graffiti(0, "Mr F was here again") + .await + }); +} + #[test] fn keystore_validator_creation() { let runtime = build_runtime(); diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index 769d8a1d49..7120ee5f9f 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -468,7 +468,7 @@ fn import_and_delete_conflicting_web3_signer_keystores() { for pubkey in &pubkeys { tester .client - .patch_lighthouse_validators(pubkey, Some(false), None, None) + .patch_lighthouse_validators(pubkey, Some(false), None, None, None) .await .unwrap(); } diff --git a/validator_client/src/initialized_validators.rs b/validator_client/src/initialized_validators.rs index 468fc2b06b..090acbe969 100644 --- a/validator_client/src/initialized_validators.rs +++ b/validator_client/src/initialized_validators.rs @@ -27,6 +27,7 @@ use std::io::{self, Read}; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Duration; +use types::graffiti::GraffitiString; use types::{Address, Graffiti, Keypair, PublicKey, PublicKeyBytes}; use url::{ParseError, Url}; use validator_dir::Builder as ValidatorDirBuilder; @@ -147,6 +148,10 @@ impl InitializedValidator { pub fn get_index(&self) -> Option { self.index } + + pub fn get_graffiti(&self) -> Option { + self.graffiti + } } fn open_keystore(path: &Path) -> Result { @@ -671,8 +676,8 @@ impl InitializedValidators { self.validators.get(public_key) } - /// Sets the `InitializedValidator` and `ValidatorDefinition` `enabled`, `gas_limit`, and `builder_proposals` - /// values. + /// Sets the `InitializedValidator` and `ValidatorDefinition` `enabled`, `gas_limit`, + /// `builder_proposals`, and `graffiti` values. /// /// ## Notes /// @@ -682,7 +687,7 @@ impl InitializedValidators { /// /// If a `gas_limit` is included in the call to this function, it will also be updated and saved /// to disk. If `gas_limit` is `None` the `gas_limit` *will not* be unset in `ValidatorDefinition` - /// or `InitializedValidator`. The same logic applies to `builder_proposals`. + /// or `InitializedValidator`. The same logic applies to `builder_proposals` and `graffiti`. /// /// Saves the `ValidatorDefinitions` to file, even if no definitions were changed. pub async fn set_validator_definition_fields( @@ -691,6 +696,7 @@ impl InitializedValidators { enabled: Option, gas_limit: Option, builder_proposals: Option, + graffiti: Option, ) -> Result<(), Error> { if let Some(def) = self .definitions @@ -708,6 +714,9 @@ impl InitializedValidators { if let Some(builder_proposals) = builder_proposals { def.builder_proposals = Some(builder_proposals); } + if let Some(graffiti) = graffiti.clone() { + def.graffiti = Some(graffiti); + } } self.update_validators().await?; @@ -723,6 +732,9 @@ impl InitializedValidators { if let Some(builder_proposals) = builder_proposals { val.builder_proposals = Some(builder_proposals); } + if let Some(graffiti) = graffiti { + val.graffiti = Some(graffiti.into()); + } } self.definitions