Append client version info to user graffiti by default (#9313)

Closes issue #9287


  


Co-Authored-By: Tan Chee Keong <tanck@sigmaprime.io>

Co-Authored-By: chonghe <44791194+chong-he@users.noreply.github.com>

Co-Authored-By: Michael Sproul <michael@sigmaprime.io>

beacon_node/beacon_chain/src/graffiti_calculator.rs

@@ -62,7 +62,7 @@ impl GraffitiSettings {
         validator_graffiti
             .map(|graffiti| Self::Specified {
                 graffiti,
-                policy: policy.unwrap_or(GraffitiPolicy::PreserveUserGraffiti),
+                policy: policy.unwrap_or_default(),
             })
             .unwrap_or(Self::Unspecified)
     }
@@ -480,9 +480,9 @@ mod tests {
                 // for the case of empty append_graffiti_string, i.e., user-specified graffiti is 30-32 characters
                 graffiti.to_string()
             } else {
-                // There is a space between the client version info and user graffiti
+                // There is a space between the user graffiti and client version info
                 // as defined in calculate_graffiti function in engine_api.rs
-                format!("{} {}", append_graffiti_string, graffiti)
+                format!("{} {}", graffiti, append_graffiti_string)
             };
 
             let expected_graffiti_prefix_bytes = expected_graffiti_string.as_bytes();

beacon_node/execution_layer/src/engine_api.rs

@@ -832,16 +832,16 @@ impl ClientVersionV1 {
             // 12 characters for append_graffiti_full, plus one character for spacing
             // that leaves user specified graffiti to be 32-12-1 = 19 characters max, i.e., <20
             if graffiti_length < 20 {
-                format!("{} {}", append_graffiti_full, graffiti_str)
+                format!("{} {}", graffiti_str, append_graffiti_full)
             // user-specified graffiti is between 20-23 characters
             } else if (20..24).contains(&graffiti_length) {
-                format!("{} {}", append_graffiti_one_byte, graffiti_str)
+                format!("{} {}", graffiti_str, append_graffiti_one_byte)
             // user-specified graffiti is between 24-27 characters
             } else if (24..28).contains(&graffiti_length) {
-                format!("{} {}", append_graffiti_no_commit, graffiti_str)
+                format!("{} {}", graffiti_str, append_graffiti_no_commit)
             // user-specified graffiti is between 28-29 characters
             } else if (28..30).contains(&graffiti_length) {
-                format!("{} {}", append_graffiti_only_el, graffiti_str)
+                format!("{} {}", graffiti_str, append_graffiti_only_el)
             // if user-specified graffiti is between 30-32 characters, append nothing
             } else {
                 return graffiti;

beacon_node/http_api/tests/tests.rs

@@ -7967,8 +7967,8 @@ impl ApiTester {
         let graffiti = Some(Graffiti::from([0; GRAFFITI_BYTES_LEN]));
         let builder_boost_factor = None;
 
-        // Default case where GraffitiPolicy is None
-        let default_path = self
+        // When GraffitiPolicy is None
+        let no_graffiti_policy_path = self
             .client
             .get_validator_blocks_v3_path(
                 slot,
@@ -7981,13 +7981,30 @@ impl ApiTester {
             .await
             .unwrap();
 
+        // Default case where GraffitiPolicy is AppendClientVersions
+        let default_path = self
+            .client
+            .get_validator_blocks_v3_path(
+                slot,
+                &randao_reveal,
+                graffiti.as_ref(),
+                SkipRandaoVerification::Yes,
+                builder_boost_factor,
+                Some(GraffitiPolicy::AppendClientVersions),
+            )
+            .await
+            .unwrap();
+
+        let query_none_path = no_graffiti_policy_path.query().unwrap_or("");
         let query_default_path = default_path.query().unwrap_or("");
-        // When GraffitiPolicy is None, the HTTP API query path should not contain "graffiti_policy"
+        // When GraffitiPolicy is AppendClientVersions (default GraffitiPolicy), the HTTP API query path should not contain "graffiti_policy"
         assert!(
             !query_default_path.contains("graffiti_policy"),
             "URL should not contain graffiti_policy parameter (same as PreserveUserGraffiti). URL is: {}",
             query_default_path
         );
+        // The HTTP API query path for GraffiliPolicy is None should be the same as the default (GraffitiPolicy = AppendClientVersions)
+        assert_eq!(query_none_path, query_default_path);
 
         let preserve_path = self
             .client
@@ -8003,36 +8020,86 @@ impl ApiTester {
             .unwrap();
 
         let query_preserve_path = preserve_path.query().unwrap_or("");
-        // When GraffitiPolicy is set to PreserveUserGraffiti, the HTTP API query path should not contain "graffiti_policy"
+        // When GraffitiPolicy is set to PreserveUserGraffiti, the HTTP API query path should contain "graffiti_policy"
         assert!(
-            !query_preserve_path.contains("graffiti_policy"),
+            query_preserve_path.contains("graffiti_policy"),
             "URL should not contain graffiti_policy parameter when using PreserveUserGraffiti. URL is: {}",
             query_preserve_path
         );
 
-        // The HTTP API query path for PreserveUserGraffiti should be the same as the default
-        assert_eq!(query_default_path, query_preserve_path);
+        self
+    }
 
-        let append_path = self
+    async fn get_validator_blocks_v4_path_graffiti_policy(self) -> Self {
+        let slot = self.chain.slot().unwrap();
+        let epoch = self.chain.epoch().unwrap();
+        let (_, randao_reveal) = self.get_test_randao(slot, epoch).await;
+        let graffiti = Some(Graffiti::from([0; GRAFFITI_BYTES_LEN]));
+        let builder_boost_factor = None;
+
+        // When GraffitiPolicy is None
+        let no_graffiti_policy_path = self
             .client
-            .get_validator_blocks_v3_path(
+            .get_validator_blocks_v4_path(
                 slot,
                 &randao_reveal,
                 graffiti.as_ref(),
-                SkipRandaoVerification::No,
+                SkipRandaoVerification::Yes,
+                None,
+                builder_boost_factor,
+                None,
+            )
+            .await
+            .unwrap();
+
+        // Default case where GraffitiPolicy is AppendClientVersions
+        let default_path = self
+            .client
+            .get_validator_blocks_v4_path(
+                slot,
+                &randao_reveal,
+                graffiti.as_ref(),
+                SkipRandaoVerification::Yes,
+                None,
                 builder_boost_factor,
                 Some(GraffitiPolicy::AppendClientVersions),
             )
             .await
             .unwrap();
 
-        let query_append_path = append_path.query().unwrap_or("");
-        // When GraffitiPolicy is AppendClientVersions, the HTTP API query path should contain "graffiti_policy"
+        let query_none_path = no_graffiti_policy_path.query().unwrap_or("");
+        let query_default_path = default_path.query().unwrap_or("");
+        // When GraffitiPolicy is AppendClientVersions (default GraffitiPolicy), the HTTP API query path should not contain "graffiti_policy"
         assert!(
-            query_append_path.contains("graffiti_policy"),
-            "URL should contain graffiti_policy=AppendClientVersions parameter. URL is: {}",
-            query_append_path
+            !query_default_path.contains("graffiti_policy"),
+            "URL should not contain graffiti_policy parameter (same as PreserveUserGraffiti). URL is: {}",
+            query_default_path
         );
+        // The HTTP API query path for GraffiliPolicy is None should be the same as the default (GraffitiPolicy = AppendClientVersions)
+        assert_eq!(query_none_path, query_default_path);
+
+        let preserve_path = self
+            .client
+            .get_validator_blocks_v4_path(
+                slot,
+                &randao_reveal,
+                graffiti.as_ref(),
+                SkipRandaoVerification::Yes,
+                None,
+                builder_boost_factor,
+                Some(GraffitiPolicy::PreserveUserGraffiti),
+            )
+            .await
+            .unwrap();
+
+        let query_preserve_path = preserve_path.query().unwrap_or("");
+        // When GraffitiPolicy is set to PreserveUserGraffiti, the HTTP API query path should contain "graffiti_policy"
+        assert!(
+            query_preserve_path.contains("graffiti_policy"),
+            "URL should not contain graffiti_policy parameter when using PreserveUserGraffiti. URL is: {}",
+            query_preserve_path
+        );
+
         self
     }
 }
@@ -9531,10 +9598,12 @@ async fn get_beacon_rewards_attestations_fulu() {
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn get_validator_blocks_v3_http_api_path() {
+async fn get_validator_blocks_http_api_path() {
     ApiTester::new()
         .await
         .get_validator_blocks_v3_path_graffiti_policy()
+        .await
+        .get_validator_blocks_v4_path_graffiti_policy()
         .await;
 }
 

book/src/help_vc.md

@@ -227,10 +227,10 @@ Flags:
           automatically enabled for <= 64 validators. Enabling this metric for
           higher validator counts will lead to higher volume of prometheus
           metrics being collected.
-      --graffiti-append
-          When used, client version info will be prepended to user custom
-          graffiti, with a space in between. This should only be used with a
-          Lighthouse beacon node.
+      --graffiti-append [<GRAFFITI_APPEND>]
+          Client version info will be appended to user custom graffiti, with a
+          space in between. This should only be set to false when using a
+          Lighthouse beacon node. [default: true] [possible values: true, false]
   -h, --help
           Prints help information
       --http

book/src/validator_graffiti.md

@@ -60,7 +60,7 @@ Usage: `lighthouse vc --graffiti example`
 
 ## 4. Using the "--graffiti" flag on the beacon node
 
-Users can also specify a common graffiti using the `--graffiti` flag on the beacon node as a common  graffiti for all validators.
+Users can also specify a common graffiti using the `--graffiti` flag on the beacon node as a common graffiti for all validators.
 
 Usage: `lighthouse bn --graffiti fortytwo`
 
@@ -93,3 +93,39 @@ curl -X PATCH "http://localhost:5062/lighthouse/validators/0xb0148e6348264131bf4
 ```
 
 A `null` response indicates that the request is successful.
+
+## Automatically append client version info to user graffiti
+
+> Note: this feature only works when a Lighthouse validator client is connected to a Lighthouse beacon node.
+
+In the interest of obtaining client diversity data, Lighthouse will by default automatically append client version info
+to user graffiti in proposed blocks.
+
+For example, you set the graffiti in the validator client as `This is my graffiti`. You are using Lighthouse (`LH`) v8.1.3
+with commit hash `176cce5` and Reth (`RH`) v2.2.0 with commit hash `88505c7`. The appended graffiti will include:
+
+- Execution layer client code
+- First two bytes of the execution layer commit hash
+- Consensus layer client code
+- First two bytes of the consensus layer commit hash
+
+When the user graffiti is less than 20 characters, as in the above example, the appended graffiti when proposing a block
+will be: `This is my graffiti RH8850LH176c`.
+
+Given that the total size of the graffiti is 32 bytes, if the appended graffiti exceeds the size, the appended
+client version info will automatically be shortened. Some examples are as follows, where the last part of the graffiti is the
+appended client version info.
+
+When the user graffiti is between 20-23 characters:
+`This is my graffiti yo RH88LH17`
+
+When the user graffiti is between 24-27 characters:
+`This is my graffiti string RHLH`
+
+When the user graffiti is between 28-29 characters:
+`This is my graffiti string yo RH`
+
+When the user graffiti is between 30-32 characters, no client version info will be appended:
+`This is my graffiti string yo yo`
+
+To opt out from this, when using a Lighthouse beacon node, use the flag `--graffiti-append false` on the validator client.  This will retain your own graffiti when proposing a block, without appending any client version info.

common/eth2/src/lib.rs

@@ -2393,12 +2393,12 @@ impl BeaconNodeHttpClient {
                 .append_pair("builder_boost_factor", &builder_booster_factor.to_string());
         }
 
-        // Only append the HTTP URL request if the graffiti_policy is to AppendClientVersions
-        // If PreserveUserGraffiti (default), then the HTTP URL request does not contain graffiti_policy
+        // Only append the HTTP URL request if the graffiti_policy is PreserveUserGraffiti
+        // If AppendClientVersions (default), then we do not modify the HTTP URL request
         // so that the default case is compliant to the spec
-        if let Some(GraffitiPolicy::AppendClientVersions) = graffiti_policy {
+        if let Some(GraffitiPolicy::PreserveUserGraffiti) = graffiti_policy {
             path.query_pairs_mut()
-                .append_pair("graffiti_policy", "AppendClientVersions");
+                .append_pair("graffiti_policy", "PreserveUserGraffiti");
         }
 
         Ok(path)
@@ -2600,9 +2600,12 @@ impl BeaconNodeHttpClient {
                 .append_pair("builder_boost_factor", &builder_booster_factor.to_string());
         }
 
-        if let Some(GraffitiPolicy::AppendClientVersions) = graffiti_policy {
+        // Only append the HTTP URL request if the graffiti_policy is PreserveUserGraffiti
+        // If AppendClientVersions (default), then we do not modify the HTTP URL request
+        // so that the default case is compliant to the spec
+        if let Some(GraffitiPolicy::PreserveUserGraffiti) = graffiti_policy {
             path.query_pairs_mut()
-                .append_pair("graffiti_policy", "AppendClientVersions");
+                .append_pair("graffiti_policy", "PreserveUserGraffiti");
         }
 
         Ok(path)

common/eth2/src/types.rs

@@ -758,10 +758,10 @@ pub struct ProposerData {
     pub slot: Slot,
 }
 
-#[derive(Clone, Copy, Serialize, Deserialize, Default, Debug)]
+#[derive(Clone, Copy, Serialize, Deserialize, Default, Debug, PartialEq)]
 pub enum GraffitiPolicy {
-    #[default]
     PreserveUserGraffiti,
+    #[default]
     AppendClientVersions,
 }
 

lighthouse/tests/validator_client.rs

@@ -2,6 +2,7 @@ use beacon_node_fallback::{ApiTopic, beacon_node_health::BeaconNodeSyncDistanceT
 
 use crate::exec::CommandLineTestExec;
 use bls::{Keypair, PublicKeyBytes};
+use eth2::types::GraffitiPolicy;
 use initialized_validators::DEFAULT_WEB3SIGNER_KEEP_ALIVE;
 use sensitive_url::SensitiveUrl;
 use std::fs::File;
@@ -261,6 +262,58 @@ fn graffiti_file_with_pk_flag() {
         });
 }
 
+// Tests for graffiti-append flag
+#[test]
+fn graffiti_append_default() {
+    CommandLineTest::new().run().with_config(|config| {
+        assert_eq!(
+            config.graffiti_policy,
+            Some(GraffitiPolicy::AppendClientVersions)
+        );
+    });
+}
+
+#[test]
+fn graffiti_append_true_flag() {
+    CommandLineTest::new()
+        .flag("graffiti-append", Some("true"))
+        .run()
+        .with_config(|config| {
+            assert_eq!(
+                config.graffiti_policy,
+                Some(GraffitiPolicy::AppendClientVersions)
+            );
+        });
+}
+
+#[test]
+fn graffiti_append_false_flag() {
+    CommandLineTest::new()
+        .flag("graffiti-append", Some("false"))
+        .run()
+        .with_config(|config| {
+            assert_eq!(
+                config.graffiti_policy,
+                Some(GraffitiPolicy::PreserveUserGraffiti)
+            );
+        });
+}
+
+// Retain previous behaviour: `--graffiti-append` with no value is the same as
+// `--graffiti-append true`.
+#[test]
+fn graffiti_append_no_value() {
+    CommandLineTest::new()
+        .flag("graffiti-append", None)
+        .run()
+        .with_config(|config| {
+            assert_eq!(
+                config.graffiti_policy,
+                Some(GraffitiPolicy::AppendClientVersions)
+            );
+        });
+}
+
 // Tests for suggested-fee-recipient flags.
 #[test]
 fn fee_recipient_flag() {

validator_client/src/cli.rs

@@ -152,13 +152,14 @@ pub struct ValidatorClient {
 
     #[clap(
         long,
-        requires = "graffiti",
-        help = "When used, client version info will be prepended to user custom graffiti, with a space in between. \
-        This should only be used with a Lighthouse beacon node.",
+        num_args = 0..=1,
+        help = "Client version info will be appended to user custom graffiti, with a space in between. \
+        This should only be set to false when using a Lighthouse beacon node.",
         display_order = 0,
+        default_value = "true",
         help_heading = FLAG_HEADER
     )]
-    pub graffiti_append: bool,
+    pub graffiti_append: Option<bool>,
 
     #[clap(
         long,

validator_client/src/config.rs

@@ -239,7 +239,7 @@ impl Config {
             }
         }
 
-        config.graffiti_policy = if validator_client_config.graffiti_append {
+        config.graffiti_policy = if validator_client_config.graffiti_append.unwrap_or(true) {
             Some(GraffitiPolicy::AppendClientVersions)
         } else {
             Some(GraffitiPolicy::PreserveUserGraffiti)