added check for fee recipient per validator and added unit tests

common/account_utils/src/validator_definitions.rs

@@ -12,7 +12,7 @@ use std::collections::HashSet;
 use std::fs::{self, File, create_dir_all};
 use std::io;
 use std::path::{Path, PathBuf};
-use tracing::error;
+use tracing::{debug, error};
 use types::{Address, graffiti::GraffitiString};
 use validator_dir::VOTING_KEYSTORE_FILE;
 use zeroize::Zeroizing;
@@ -212,6 +212,16 @@ impl ValidatorDefinition {
             },
         })
     }
+
+    pub fn check_fee_recipient(&self, global_fee_recipient: Option<Address>) -> Option<&PublicKey> {
+        // Skip disabled validators. Also skip if validator has its own fee set, or the global flag is set
+        if !self.enabled || self.suggested_fee_recipient.is_some() || global_fee_recipient.is_some()
+        {
+            return None;
+        }
+
+        Some(&self.voting_public_key)
+    }
 }
 
 /// A list of `ValidatorDefinition` that serves as a serde-able configuration file which defines a
@@ -410,6 +420,52 @@ impl ValidatorDefinitions {
             .iter()
             .filter_map(|def| def.signing_definition.voting_keystore_password_path())
     }
+
+    /// Called after loading to run safety checks on all validators
+    pub fn check_all_fee_recipients(
+        &self,
+        global_fee_recipient: Option<Address>,
+    ) -> Result<(), String> {
+        let missing: Vec<&PublicKey> = self
+            .0
+            .iter()
+            .filter_map(|def| def.check_fee_recipient(global_fee_recipient))
+            .collect();
+
+        if !missing.is_empty() {
+            let pubkeys = missing
+                .iter()
+                .map(|pk| pk.to_string())
+                .collect::<Vec<_>>()
+                .join(", ");
+
+            return Err(format!(
+                "The following validators are missing a `suggested_fee_recipient`: {}. \
+                 Fix this by adding a `suggested_fee_recipient` in the \
+                 `validator_definitions.yml` or by supplying a fallback fee \
+                 recipient via the `--suggested-fee-recipient` flag.",
+                pubkeys
+            ));
+        }
+
+        // Friendly reminder for users using the fallback flag
+        if global_fee_recipient.is_some() {
+            let count = self
+                .0
+                .iter()
+                .filter(|d| d.enabled && d.suggested_fee_recipient.is_none())
+                .count();
+            if count > 0 {
+                info!(
+                    "The fallback --suggested-fee-recipient is being used for {} validator(s). \
+                     You may alternatively set the fee recipient for each validator individually via `validator_definitions.yml`.",
+                    count
+                );
+            }
+        }
+
+        Ok(())
+    }
 }
 
 /// Perform an exhaustive tree search of `dir`, adding any discovered voting keystore paths to
@@ -485,6 +541,7 @@ pub fn is_voting_keystore(file_name: &str) -> bool {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use bls::Keypair;
     use std::str::FromStr;
 
     #[test]
@@ -682,4 +739,235 @@ mod tests {
         let def: ValidatorDefinition = serde_yaml::from_str(valid_builder_proposals).unwrap();
         assert_eq!(def.builder_proposals, Some(true));
     }
+
+    #[test]
+    fn fee_recipient_check_enabled_validator_cases() {
+        let def = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: PublicKey::from_str(
+                "0xaf3c7ddab7e293834710fca2d39d068f884455ede270e0d0293dc818e4f2f0f975355067e8437955cb29aec674e5c9e7"
+            ).unwrap(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: None,
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            }
+        };
+
+        // Should return Some(pubkey) when no fee recipient is set
+        let check_result = def.check_fee_recipient(None);
+        assert!(check_result.is_some());
+
+        // Should return None since global fee recipient is set
+        let global_fee_recipient =
+            Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap());
+        let check_result = def.check_fee_recipient(global_fee_recipient);
+        assert!(check_result.is_none());
+    }
+
+    #[test]
+    fn fee_recipient_check_passes_with_validator_specific() {
+        let def = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: PublicKey::from_str(
+                "0xaf3c7ddab7e293834710fca2d39d068f884455ede270e0d0293dc818e4f2f0f975355067e8437955cb29aec674e5c9e7"
+            ).unwrap(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap()),
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        // Should return None because suggested_fee_recipient is set
+        let check_result = def.check_fee_recipient(None);
+        assert!(check_result.is_none());
+    }
+
+    #[test]
+    fn fee_recipient_check_skips_disabled_validators() {
+        let def = ValidatorDefinition {
+            enabled: false,
+            voting_public_key: PublicKey::from_str(
+                "0xaf3c7ddab7e293834710fca2d39d068f884455ede270e0d0293dc818e4f2f0f975355067e8437955cb29aec674e5c9e7"
+            ).unwrap(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: None,
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        // Should return None because validator is disabled
+        let check_result = def.check_fee_recipient(None);
+        assert!(check_result.is_none());
+    }
+
+    #[test]
+    fn check_all_fee_recipients_reports_all_missing() {
+        let keypair1 = Keypair::random();
+        let keypair2 = Keypair::random();
+
+        let def1 = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: keypair1.pk.clone(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: None,
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        let def2 = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: keypair2.pk.clone(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: None, // Missing recipient
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        let defs = ValidatorDefinitions::from(vec![def1, def2]);
+
+        // Should fail because both defs have no fee recipient and no global fee recipient is set
+        let result = defs.check_all_fee_recipients(None);
+        assert!(result.is_err());
+        let err = result.unwrap_err();
+
+        // Check that both public keys are mentioned in the error message
+        let pk1_string = keypair1.pk.to_string();
+        let pk2_string = keypair2.pk.to_string();
+
+        assert!(err.contains(&pk1_string), "Error message missing pubkey 1");
+        assert!(err.contains(&pk2_string), "Error message missing pubkey 2");
+        assert!(err.contains("are missing a `suggested_fee_recipient`"));
+    }
+
+    #[test]
+    fn check_all_fee_recipients_passes_all_configured() {
+        let keypair = Keypair::random();
+        let def1 = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: keypair.pk.clone(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: Some(
+                Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap(),
+            ),
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        let def2 = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: keypair.pk.clone(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: Some(
+                Address::from_str("0xb2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap(),
+            ),
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        let defs = ValidatorDefinitions::from(vec![def1, def2]);
+
+        // Should pass - all validators have fee recipients
+        assert!(defs.check_all_fee_recipients(None).is_ok());
+    }
+
+    #[test]
+    fn check_all_fee_recipients_passes_with_global() {
+        let keypair = Keypair::random();
+        let def1 = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: keypair.pk.clone(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: None,
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        let def2 = ValidatorDefinition {
+            enabled: true,
+            voting_public_key: keypair.pk.clone(),
+            description: String::new(),
+            graffiti: None,
+            suggested_fee_recipient: None,
+            gas_limit: None,
+            builder_proposals: None,
+            builder_boost_factor: None,
+            prefer_builder_proposals: None,
+            signing_definition: SigningDefinition::LocalKeystore {
+                voting_keystore_path: PathBuf::new(),
+                voting_keystore_password_path: None,
+                voting_keystore_password: None,
+            },
+        };
+
+        let defs = ValidatorDefinitions::from(vec![def1, def2]);
+
+        // Should pass - global fee recipient is set
+        let global_fee_recipient =
+            Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap());
+        assert!(defs.check_all_fee_recipients(global_fee_recipient).is_ok());
+    }
 }

validator_client/src/lib.rs

@@ -183,6 +183,9 @@ impl<E: EthSpec> ProductionValidatorClient<E> {
             info!(new_validators, "Completed validator discovery");
         }
 
+        // Check for all validators' fee recipient
+        validator_defs.check_all_fee_recipients(config.validator_store.fee_recipient)?;
+
         let validators = InitializedValidators::from_definitions(
             validator_defs,
             config.validator_dir.clone(),