Add support for updating validator graffiti (sigp#4417)

## Issue Addressed

sigp#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

Author: jimmygchen

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
 

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.

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<bool>,
         gas_limit: Option<u64>,
         builder_proposals: Option<bool>,
+        graffiti: Option<GraffitiString>,
     ) -> Result<(), Error> {
         let mut path = self.server.full.clone();
 
@@ -482,6 +484,7 @@ impl ValidatorClientHttpClient {
                 enabled,
                 gas_limit,
                 builder_proposals,
+                graffiti,
             },
         )
         .await

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<bool>,
+    #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub graffiti: Option<GraffitiString>,
 }
 
 #[derive(Clone, PartialEq, Serialize, Deserialize)]

validator_client/src/http_api/mod.rs

@@ -357,7 +357,7 @@ pub fn serve<T: 'static + SlotClock + Clone, E: EthSpec>(
         .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<T: 'static + SlotClock + Clone, E: EthSpec>(
         .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<ValidatorStore<T, E>>,
+             graffiti_file: Option<GraffitiFile>,
              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<T: 'static + SlotClock + Clone, E: EthSpec>(
                             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<T: 'static + SlotClock + Clone, E: EthSpec>(
                                             body.enabled,
                                             body.gas_limit,
                                             body.builder_proposals,
+                                            body.graffiti,
                                         ),
                                     )
                                     .map_err(|e| {

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();

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();
         }

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<u64> {
         self.index
     }
+
+    pub fn get_graffiti(&self) -> Option<Graffiti> {
+        self.graffiti
+    }
 }
 
 fn open_keystore(path: &Path) -> Result<Keystore, Error> {
@@ -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<bool>,
         gas_limit: Option<u64>,
         builder_proposals: Option<bool>,
+        graffiti: Option<GraffitiString>,
     ) -> 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