Append client version info to graffiti (#7558)

* #7201


  


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

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

Co-Authored-By: Jimmy Chen <jimmy@sigmaprime.io>

Co-Authored-By: Tan Chee Keong <tanck2005@gmail.com>

Author: chong-he

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -33,7 +33,7 @@ use crate::events::ServerSentEventHandler;
 use crate::execution_payload::{NotifyExecutionLayer, PreparePayloadHandle, get_execution_payload};
 use crate::fetch_blobs::EngineGetBlobsOutput;
 use crate::fork_choice_signal::{ForkChoiceSignalRx, ForkChoiceSignalTx, ForkChoiceWaitResult};
-use crate::graffiti_calculator::GraffitiCalculator;
+use crate::graffiti_calculator::{GraffitiCalculator, GraffitiSettings};
 use crate::kzg_utils::reconstruct_blobs;
 use crate::light_client_finality_update_verification::{
     Error as LightClientFinalityUpdateError, VerifiedLightClientFinalityUpdate,
@@ -4493,7 +4493,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         self: &Arc<Self>,
         randao_reveal: Signature,
         slot: Slot,
-        validator_graffiti: Option<Graffiti>,
+        graffiti_settings: GraffitiSettings,
         verification: ProduceBlockVerification,
         builder_boost_factor: Option<u64>,
         block_production_version: BlockProductionVersion,
@@ -4527,7 +4527,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             state_root_opt,
             slot,
             randao_reveal,
-            validator_graffiti,
+            graffiti_settings,
             verification,
             builder_boost_factor,
             block_production_version,
@@ -5060,7 +5060,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         state_root_opt: Option<Hash256>,
         produce_at_slot: Slot,
         randao_reveal: Signature,
-        validator_graffiti: Option<Graffiti>,
+        graffiti_settings: GraffitiSettings,
         verification: ProduceBlockVerification,
         builder_boost_factor: Option<u64>,
         block_production_version: BlockProductionVersion,
@@ -5071,7 +5071,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         let chain = self.clone();
         let graffiti = self
             .graffiti_calculator
-            .get_graffiti(validator_graffiti)
+            .get_graffiti(graffiti_settings)
             .await;
         let span = Span::current();
         let mut partial_beacon_block = self

beacon_node/beacon_chain/src/graffiti_calculator.rs

@@ -1,5 +1,6 @@
 use crate::BeaconChain;
 use crate::BeaconChainTypes;
+use eth2::types::GraffitiPolicy;
 use execution_layer::{CommitPrefix, ExecutionLayer, http::ENGINE_GET_CLIENT_VERSION_V1};
 use logging::crit;
 use serde::{Deserialize, Serialize};
@@ -48,6 +49,25 @@ impl Debug for GraffitiOrigin {
     }
 }
 
+pub enum GraffitiSettings {
+    Unspecified,
+    Specified {
+        graffiti: Graffiti,
+        policy: GraffitiPolicy,
+    },
+}
+
+impl GraffitiSettings {
+    pub fn new(validator_graffiti: Option<Graffiti>, policy: Option<GraffitiPolicy>) -> Self {
+        validator_graffiti
+            .map(|graffiti| Self::Specified {
+                graffiti,
+                policy: policy.unwrap_or(GraffitiPolicy::PreserveUserGraffiti),
+            })
+            .unwrap_or(Self::Unspecified)
+    }
+}
+
 pub struct GraffitiCalculator<T: BeaconChainTypes> {
     pub beacon_graffiti: GraffitiOrigin,
     execution_layer: Option<ExecutionLayer<T::EthSpec>>,
@@ -73,11 +93,19 @@ impl<T: BeaconChainTypes> GraffitiCalculator<T> {
     /// 2. Graffiti specified by the user via beacon node CLI options.
     /// 3. The EL & CL client version string, applicable when the EL supports version specification.
     /// 4. The default lighthouse version string, used if the EL lacks version specification support.
-    pub async fn get_graffiti(&self, validator_graffiti: Option<Graffiti>) -> Graffiti {
-        if let Some(graffiti) = validator_graffiti {
-            return graffiti;
+    pub async fn get_graffiti(&self, graffiti_settings: GraffitiSettings) -> Graffiti {
+        match graffiti_settings {
+            GraffitiSettings::Specified { graffiti, policy } => match policy {
+                GraffitiPolicy::PreserveUserGraffiti => graffiti,
+                GraffitiPolicy::AppendClientVersions => {
+                    self.calculate_combined_graffiti(Some(graffiti)).await
+                }
+            },
+            GraffitiSettings::Unspecified => self.calculate_combined_graffiti(None).await,
         }
+    }
 
+    async fn calculate_combined_graffiti(&self, validator_graffiti: Option<Graffiti>) -> Graffiti {
         match self.beacon_graffiti {
             GraffitiOrigin::UserSpecified(graffiti) => graffiti,
             GraffitiOrigin::Calculated(default_graffiti) => {
@@ -133,7 +161,7 @@ impl<T: BeaconChainTypes> GraffitiCalculator<T> {
                             CommitPrefix("00000000".to_string())
                         });
 
-                engine_version.calculate_graffiti(lighthouse_commit_prefix)
+                engine_version.calculate_graffiti(lighthouse_commit_prefix, validator_graffiti)
             }
         }
     }
@@ -224,8 +252,10 @@ async fn engine_version_cache_refresh_service<T: BeaconChainTypes>(
 #[cfg(test)]
 mod tests {
     use crate::ChainConfig;
+    use crate::graffiti_calculator::GraffitiSettings;
     use crate::test_utils::{BeaconChainHarness, EphemeralHarnessType, test_spec};
     use bls::Keypair;
+    use eth2::types::GraffitiPolicy;
     use execution_layer::EngineCapabilities;
     use execution_layer::test_utils::{DEFAULT_CLIENT_VERSION, DEFAULT_ENGINE_CAPABILITIES};
     use std::sync::Arc;
@@ -281,8 +311,12 @@ mod tests {
 
         let version_bytes = std::cmp::min(lighthouse_version::VERSION.len(), GRAFFITI_BYTES_LEN);
         // grab the slice of the graffiti that corresponds to the lighthouse version
-        let graffiti_slice =
-            &harness.chain.graffiti_calculator.get_graffiti(None).await.0[..version_bytes];
+        let graffiti_slice = &harness
+            .chain
+            .graffiti_calculator
+            .get_graffiti(GraffitiSettings::Unspecified)
+            .await
+            .0[..version_bytes];
 
         // convert graffiti bytes slice to ascii for easy debugging if this test should fail
         let graffiti_str =
@@ -303,7 +337,12 @@ mod tests {
         let spec = Arc::new(test_spec::<MinimalEthSpec>());
         let harness = get_harness(VALIDATOR_COUNT, spec, None);
 
-        let found_graffiti_bytes = harness.chain.graffiti_calculator.get_graffiti(None).await.0;
+        let found_graffiti_bytes = harness
+            .chain
+            .graffiti_calculator
+            .get_graffiti(GraffitiSettings::Unspecified)
+            .await
+            .0;
 
         let mock_commit = DEFAULT_CLIENT_VERSION.commit.clone();
         let expected_graffiti_string = format!(
@@ -352,12 +391,109 @@ mod tests {
         let found_graffiti = harness
             .chain
             .graffiti_calculator
-            .get_graffiti(Some(Graffiti::from(graffiti_bytes)))
+            .get_graffiti(GraffitiSettings::new(
+                Some(Graffiti::from(graffiti_bytes)),
+                Some(GraffitiPolicy::PreserveUserGraffiti),
+            ))
             .await;
 
         assert_eq!(
             found_graffiti.to_string(),
             "0x6e6963652067726166666974692062726f000000000000000000000000000000"
         );
     }
+
+    #[tokio::test]
+    async fn check_append_el_version_graffiti_various_length() {
+        let spec = Arc::new(test_spec::<MinimalEthSpec>());
+        let harness = get_harness(VALIDATOR_COUNT, spec, None);
+
+        let graffiti_vec = vec![
+            // less than 20 characters, example below is 19 characters
+            "This is my graffiti",
+            // 20-23 characters, example below is 22 characters
+            "This is my graffiti yo",
+            // 24-27 characters, example below is 26 characters
+            "This is my graffiti string",
+            // 28-29 characters, example below is 29 characters
+            "This is my graffiti string yo",
+            // 30-32 characters, example below is 32 characters
+            "This is my graffiti string yo yo",
+        ];
+
+        for graffiti in graffiti_vec {
+            let mut graffiti_bytes = [0; GRAFFITI_BYTES_LEN];
+            graffiti_bytes[..graffiti.len()].copy_from_slice(graffiti.as_bytes());
+
+            // To test appending client version info with user specified graffiti
+            let policy = GraffitiPolicy::AppendClientVersions;
+            let found_graffiti_bytes = harness
+                .chain
+                .graffiti_calculator
+                .get_graffiti(GraffitiSettings::Specified {
+                    graffiti: Graffiti::from(graffiti_bytes),
+                    policy,
+                })
+                .await
+                .0;
+
+            let mock_commit = DEFAULT_CLIENT_VERSION.commit.clone();
+
+            let graffiti_length = graffiti.len();
+            let append_graffiti_string = match graffiti_length {
+                0..=19 => format!(
+                    "{}{}{}{}",
+                    DEFAULT_CLIENT_VERSION.code,
+                    mock_commit
+                        .strip_prefix("0x")
+                        .unwrap_or("&mock_commit")
+                        .get(0..4)
+                        .expect("should get first 2 bytes in hex"),
+                    "LH",
+                    lighthouse_version::COMMIT_PREFIX
+                        .get(0..4)
+                        .expect("should get first 2 bytes in hex")
+                ),
+                20..=23 => format!(
+                    "{}{}{}{}",
+                    DEFAULT_CLIENT_VERSION.code,
+                    mock_commit
+                        .strip_prefix("0x")
+                        .unwrap_or("&mock_commit")
+                        .get(0..2)
+                        .expect("should get first 2 bytes in hex"),
+                    "LH",
+                    lighthouse_version::COMMIT_PREFIX
+                        .get(0..2)
+                        .expect("should get first 2 bytes in hex")
+                ),
+                24..=27 => format!("{}{}", DEFAULT_CLIENT_VERSION.code, "LH",),
+                28..=29 => DEFAULT_CLIENT_VERSION.code.to_string(),
+                // when user graffiti length is 30-32 characters, append nothing
+                30..=32 => String::new(),
+                _ => panic!(
+                    "graffiti length should be less than or equal to GRAFFITI_BYTES_LEN (32 characters)"
+                ),
+            };
+
+            let expected_graffiti_string = if append_graffiti_string.is_empty() {
+                // 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
+                // as defined in calculate_graffiti function in engine_api.rs
+                format!("{} {}", append_graffiti_string, graffiti)
+            };
+
+            let expected_graffiti_prefix_bytes = expected_graffiti_string.as_bytes();
+            let expected_graffiti_prefix_len =
+                std::cmp::min(expected_graffiti_prefix_bytes.len(), GRAFFITI_BYTES_LEN);
+
+            let found_graffiti_string =
+                std::str::from_utf8(&found_graffiti_bytes[..expected_graffiti_prefix_len])
+                    .expect("bytes should convert nicely to ascii");
+
+            assert_eq!(expected_graffiti_string, found_graffiti_string);
+        }
+    }
 }

beacon_node/beacon_chain/src/test_utils.rs

@@ -2,6 +2,7 @@ use crate::blob_verification::GossipVerifiedBlob;
 use crate::block_verification_types::{AsBlock, RpcBlock};
 use crate::custody_context::NodeCustodyType;
 use crate::data_column_verification::CustodyDataColumn;
+use crate::graffiti_calculator::GraffitiSettings;
 use crate::kzg_utils::build_data_column_sidecars;
 use crate::observed_operations::ObservationOutcome;
 pub use crate::persisted_beacon_chain::PersistedBeaconChain;
@@ -23,7 +24,7 @@ use bls::get_withdrawal_credentials;
 use bls::{
     AggregateSignature, Keypair, PublicKey, PublicKeyBytes, SecretKey, Signature, SignatureBytes,
 };
-use eth2::types::SignedBlockContentsTuple;
+use eth2::types::{GraffitiPolicy, SignedBlockContentsTuple};
 use execution_layer::test_utils::generate_genesis_header;
 use execution_layer::{
     ExecutionLayer,
@@ -943,6 +944,8 @@ where
         // BeaconChain errors out with `DuplicateFullyImported`.  Vary the graffiti so that we produce
         // different blocks each time.
         let graffiti = Graffiti::from(self.rng.lock().random::<[u8; 32]>());
+        let graffiti_settings =
+            GraffitiSettings::new(Some(graffiti), Some(GraffitiPolicy::PreserveUserGraffiti));
 
         let randao_reveal = self.sign_randao_reveal(&state, proposer_index, slot);
 
@@ -956,7 +959,7 @@ where
                 None,
                 slot,
                 randao_reveal,
-                Some(graffiti),
+                graffiti_settings,
                 ProduceBlockVerification::VerifyRandao,
                 builder_boost_factor,
                 BlockProductionVersion::V3,
@@ -1000,6 +1003,8 @@ where
         // BeaconChain errors out with `DuplicateFullyImported`.  Vary the graffiti so that we produce
         // different blocks each time.
         let graffiti = Graffiti::from(self.rng.lock().random::<[u8; 32]>());
+        let graffiti_settings =
+            GraffitiSettings::new(Some(graffiti), Some(GraffitiPolicy::PreserveUserGraffiti));
 
         let randao_reveal = self.sign_randao_reveal(&state, proposer_index, slot);
 
@@ -1010,7 +1015,7 @@ where
                 None,
                 slot,
                 randao_reveal,
-                Some(graffiti),
+                graffiti_settings,
                 ProduceBlockVerification::VerifyRandao,
                 None,
                 BlockProductionVersion::FullV2,
@@ -1059,6 +1064,8 @@ where
         // BeaconChain errors out with `DuplicateFullyImported`.  Vary the graffiti so that we produce
         // different blocks each time.
         let graffiti = Graffiti::from(self.rng.lock().random::<[u8; 32]>());
+        let graffiti_settings =
+            GraffitiSettings::new(Some(graffiti), Some(GraffitiPolicy::PreserveUserGraffiti));
 
         let randao_reveal = self.sign_randao_reveal(&state, proposer_index, slot);
 
@@ -1071,7 +1078,7 @@ where
                 None,
                 slot,
                 randao_reveal,
-                Some(graffiti),
+                graffiti_settings,
                 ProduceBlockVerification::VerifyRandao,
                 None,
                 BlockProductionVersion::FullV2,