Merge of #4036

.changelog/unreleased/bug-fixes/4036-validate-validator-metadata.md

@@ -0,0 +1,2 @@
+- Validate validator metadata from on-chain validator creation and metadata
+  changes. ([\#4036](https://github.com/anoma/namada/pull/4036))

crates/apps_lib/src/client/tx.rs

@@ -639,11 +639,10 @@ pub async fn submit_become_validator(
             .is_applied_and_valid(wrapper_hash.as_ref(), &cmt)
             .is_none()
         {
-            display_line!(
-                namada.io(),
+            return Err(error::Error::Tx(error::TxSubmitError::Other(
                 "Transaction failed. No key or addresses have been saved."
-            );
-            safe_exit(1)
+                    .to_string(),
+            )));
         }
 
         // add validator address and keys to the wallet

crates/apps_lib/src/config/genesis/transactions.rs

@@ -20,7 +20,6 @@ use namada_sdk::collections::HashSet;
 use namada_sdk::dec::Dec;
 use namada_sdk::key::common::PublicKey;
 use namada_sdk::key::{common, ed25519, RefTo, SerializeWithBorsh, SigScheme};
-use namada_sdk::proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN;
 use namada_sdk::proof_of_stake::types::ValidatorMetaData;
 use namada_sdk::signing::{sign_tx, SigningTxData};
 use namada_sdk::string_encoding::StringEncoded;
@@ -1385,60 +1384,14 @@ pub fn validate_validator_account(
 
     // Check that the validator metadata is not too large
     let metadata = &signed_tx.data.metadata;
-    if metadata.email.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+    let errors = metadata.validate();
+    if !errors.is_empty() {
         panic!(
-            "The email metadata of the validator with address {} is too long, \
-             must be within {MAX_VALIDATOR_METADATA_LEN} characters",
+            "Metadata of the validator with address {} are invalid: \
+             {errors:#?}",
             signed_tx.data.address
         );
     }
-    if let Some(description) = metadata.description.as_ref() {
-        if description.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
-            panic!(
-                "The description metadata of the validator with address {} is \
-                 too long, must be within {MAX_VALIDATOR_METADATA_LEN} \
-                 characters",
-                signed_tx.data.address
-            );
-        }
-    }
-    if let Some(website) = metadata.website.as_ref() {
-        if website.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
-            panic!(
-                "The website metadata of the validator with address {} is too \
-                 long, must be within {MAX_VALIDATOR_METADATA_LEN} characters",
-                signed_tx.data.address
-            );
-        }
-    }
-    if let Some(discord_handle) = metadata.discord_handle.as_ref() {
-        if discord_handle.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
-            panic!(
-                "The discord handle metadata of the validator with address {} \
-                 is too long, must be within {MAX_VALIDATOR_METADATA_LEN} \
-                 characters",
-                signed_tx.data.address
-            );
-        }
-    }
-    if let Some(avatar) = metadata.avatar.as_ref() {
-        if avatar.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
-            panic!(
-                "The avatar metadata of the validator with address {} is too \
-                 long, must be within {MAX_VALIDATOR_METADATA_LEN} characters",
-                signed_tx.data.address
-            );
-        }
-    }
-    if let Some(name) = metadata.name.as_ref() {
-        if name.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
-            panic!(
-                "The name metadata of the validator with address {} is too \
-                 long, must be within {MAX_VALIDATOR_METADATA_LEN} characters",
-                signed_tx.data.address
-            );
-        }
-    }
 
     // Check signature
     let mut is_valid = {

crates/proof_of_stake/src/error.rs

@@ -6,6 +6,7 @@ use namada_core::chain::Epoch;
 use namada_core::dec::Dec;
 use thiserror::Error;
 
+use crate::parameters::MAX_VALIDATOR_METADATA_LEN;
 use crate::types::ValidatorState;
 use crate::{rewards, Error};
 
@@ -165,6 +166,16 @@ pub enum ConsensusKeyChangeError {
     MustBeEd25519,
 }
 
+#[allow(missing_docs)]
+#[derive(Error, Debug)]
+pub enum ValidatorMetaDataError {
+    #[error(
+        "The {0} metadata is too long, must be within \
+         {MAX_VALIDATOR_METADATA_LEN} characters"
+    )]
+    FieldTooLong(&'static str),
+}
+
 impl From<BecomeValidatorError> for Error {
     fn from(err: BecomeValidatorError) -> Self {
         Self::new(err)

crates/proof_of_stake/src/storage.rs

@@ -963,6 +963,35 @@ where
     Ok(())
 }
 
+/// Read validator's metadata.
+pub fn read_validator_metadata<S>(
+    storage: &S,
+    validator: &Address,
+) -> Result<Option<ValidatorMetaData>>
+where
+    S: StorageRead,
+{
+    let email = read_validator_email(storage, validator)?;
+    let description = read_validator_description(storage, validator)?;
+    let website = read_validator_website(storage, validator)?;
+    let discord_handle = read_validator_discord_handle(storage, validator)?;
+    let avatar = read_validator_avatar(storage, validator)?;
+    let name = read_validator_name(storage, validator)?;
+
+    // Email is the only required field for a validator in storage
+    match email {
+        Some(email) => Ok(Some(ValidatorMetaData {
+            email,
+            description,
+            website,
+            discord_handle,
+            avatar,
+            name,
+        })),
+        None => Ok(None),
+    }
+}
+
 /// Get the last epoch in which rewards were claimed from storage, if any
 pub fn get_last_reward_claim_epoch<S>(
     storage: &S,

crates/proof_of_stake/src/types/mod.rs

@@ -21,8 +21,8 @@ pub use rev_order::ReverseOrdTokenAmount;
 use serde::{Deserialize, Serialize};
 
 use crate::lazy_map::NestedMap;
-use crate::parameters::PosParams;
-use crate::{Epoch, KeySeg, LazyMap, LazySet, LazyVec};
+use crate::parameters::{PosParams, MAX_VALIDATOR_METADATA_LEN};
+use crate::{Epoch, KeySeg, LazyMap, LazySet, LazyVec, ValidatorMetaDataError};
 
 /// Stored positions of validators in validator sets
 pub type ValidatorSetPositions = crate::epoched::NestedEpoched<
@@ -419,6 +419,46 @@ pub struct ValidatorMetaData {
     pub name: Option<String>,
 }
 
+impl ValidatorMetaData {
+    /// Validator validator metadata. Returns an empty vec only if all fields
+    /// are valid.
+    pub fn validate(&self) -> Vec<ValidatorMetaDataError> {
+        let mut errors = vec![];
+        if self.email.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+            errors.push(ValidatorMetaDataError::FieldTooLong("email"));
+        }
+        if let Some(description) = self.description.as_ref() {
+            if description.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+                errors
+                    .push(ValidatorMetaDataError::FieldTooLong("description"));
+            }
+        }
+        if let Some(website) = self.website.as_ref() {
+            if website.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+                errors.push(ValidatorMetaDataError::FieldTooLong("website"));
+            }
+        }
+        if let Some(discord_handle) = self.discord_handle.as_ref() {
+            if discord_handle.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+                errors.push(ValidatorMetaDataError::FieldTooLong(
+                    "discord handle",
+                ));
+            }
+        }
+        if let Some(avatar) = self.avatar.as_ref() {
+            if avatar.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+                errors.push(ValidatorMetaDataError::FieldTooLong("avatar"));
+            }
+        }
+        if let Some(name) = self.name.as_ref() {
+            if name.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+                errors.push(ValidatorMetaDataError::FieldTooLong("name"));
+            }
+        }
+        errors
+    }
+}
+
 #[cfg(any(test, feature = "testing"))]
 impl Default for ValidatorMetaData {
     fn default() -> Self {

crates/proof_of_stake/src/vp.rs

@@ -14,7 +14,7 @@ use namada_tx::BatchedTxRef;
 use namada_vp_env::{Error, Result, VpEnv};
 use thiserror::Error;
 
-use crate::storage::read_owned_pos_params;
+use crate::storage::{read_owned_pos_params, read_validator_metadata};
 use crate::storage_key::is_params_key;
 use crate::types::BondId;
 use crate::{storage_key, token};
@@ -299,6 +299,23 @@ where
             }
         }
 
+        // Validate new and changed validator metadata
+        for validator in became_validator.iter().chain(&changed_metadata) {
+            let metadata = read_validator_metadata(&ctx.post(), validator)?;
+            let Some(metadata) = metadata else {
+                return Err(Error::new_alloc(format!(
+                    "Missing validator {validator} metadata"
+                )));
+            };
+            let errors = metadata.validate();
+            if !errors.is_empty() {
+                return Err(Error::new_alloc(format!(
+                    "Metadata of the validator with address {validator} are \
+                     invalid: {errors:#?}",
+                )));
+            }
+        }
+
         for key in keys_changed {
             if is_params_key(key) {
                 return Err(Error::new_const(

crates/sdk/src/queries/vp/pos.rs

@@ -22,13 +22,10 @@ use namada_proof_of_stake::storage::{
     read_below_capacity_validator_set_addresses_with_stake,
     read_consensus_validator_set_addresses,
     read_consensus_validator_set_addresses_with_stake, read_pos_params,
-    read_total_active_stake, read_total_stake, read_validator_avatar,
-    read_validator_description, read_validator_discord_handle,
-    read_validator_email, read_validator_last_slash_epoch,
-    read_validator_max_commission_rate_change, read_validator_name,
-    read_validator_stake, read_validator_website, unbond_handle,
-    validator_commission_rate_handle, validator_incoming_redelegations_handle,
-    validator_slashes_handle,
+    read_total_active_stake, read_total_stake, read_validator_last_slash_epoch,
+    read_validator_max_commission_rate_change, read_validator_metadata,
+    read_validator_stake, unbond_handle, validator_commission_rate_handle,
+    validator_incoming_redelegations_handle, validator_slashes_handle,
 };
 pub use namada_proof_of_stake::types::ValidatorStateInfo;
 use namada_proof_of_stake::types::{
@@ -325,25 +322,7 @@ where
     D: 'static + DB + for<'iter> DBIter<'iter> + Sync,
     H: 'static + StorageHasher + Sync,
 {
-    let email = read_validator_email(ctx.state, &validator)?;
-    let description = read_validator_description(ctx.state, &validator)?;
-    let website = read_validator_website(ctx.state, &validator)?;
-    let discord_handle = read_validator_discord_handle(ctx.state, &validator)?;
-    let avatar = read_validator_avatar(ctx.state, &validator)?;
-    let name = read_validator_name(ctx.state, &validator)?;
-
-    // Email is the only required field for a validator in storage
-    match email {
-        Some(email) => Ok(Some(ValidatorMetaData {
-            email,
-            description,
-            website,
-            discord_handle,
-            avatar,
-            name,
-        })),
-        _ => Ok(None),
-    }
+    read_validator_metadata(ctx.state, &validator)
 }
 
 /// Get the validator state

crates/tests/src/integration/ledger_tests.rs

@@ -26,6 +26,7 @@ use namada_sdk::account::AccountPublicKeysMap;
 use namada_sdk::collections::HashMap;
 use namada_sdk::error::TxSubmitError;
 use namada_sdk::migrations;
+use namada_sdk::proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN;
 use namada_sdk::queries::RPC;
 use namada_sdk::token::{self, DenominatedAmount};
 use namada_sdk::tx::{self, Tx, TX_TRANSFER_WASM, VP_USER_WASM};
@@ -2565,6 +2566,95 @@ fn wrap_tx_by_elsewho() -> Result<()> {
     Ok(())
 }
 
+/// Test for PoS validator metadata validation.
+///
+/// 1. Run the ledger node.
+/// 2. Submit a valid metadata change tx.
+/// 3. Check that the metadata has changed.
+/// 4. Submit an invalid metadata change tx.
+/// 5. Check that the metadata has not changed.
+/// 6. Submit a tx to become validator with invalid metadata.
+#[test]
+fn pos_validator_metadata_validation() -> Result<()> {
+    // 1. Run the ledger node.
+    let (node, _services) = setup::setup()?;
+
+    // 2. Submit a valid metadata change tx.
+    let valid_desc: String = "0123456789".repeat(50);
+    assert_eq!(valid_desc.len() as u64, MAX_VALIDATOR_METADATA_LEN);
+    let tx_args = apply_use_device(vec![
+        "change-metadata",
+        "--validator",
+        "validator-0-validator",
+        "--description",
+        &valid_desc,
+    ]);
+    let captured = CapturedOutput::of(|| run(&node, Bin::Client, tx_args));
+    println!("{:?}", captured.result);
+    assert_matches!(captured.result, Ok(_));
+    assert!(captured.contains(TX_APPLIED_SUCCESS));
+
+    // 3. Check that the metadata has changed.
+    let query_args = apply_use_device(vec![
+        "validator-metadata",
+        "--validator",
+        "validator-0-validator",
+    ]);
+    let captured =
+        CapturedOutput::of(|| run(&node, Bin::Client, query_args.clone()));
+    println!("{:?}", captured.result);
+    assert!(captured.contains(&valid_desc));
+
+    // 4. Submit an invalid metadata change tx.
+    let invalid_desc: String = format!("N{valid_desc}");
+    assert!(invalid_desc.len() as u64 > MAX_VALIDATOR_METADATA_LEN);
+    let tx_args = apply_use_device(vec![
+        "change-metadata",
+        "--validator",
+        "validator-0-validator",
+        "--description",
+        &invalid_desc,
+        "--force",
+    ]);
+    let captured = CapturedOutput::of(|| run(&node, Bin::Client, tx_args));
+    println!("{:?}", captured.result);
+    assert_matches!(captured.result, Ok(_));
+    assert!(captured.contains(TX_REJECTED));
+
+    // 5. Check that the metadata has not changed.
+    let captured = CapturedOutput::of(|| run(&node, Bin::Client, query_args));
+    println!("{:?}", captured.result);
+    assert!(captured.contains(&valid_desc));
+
+    // 6. Submit a tx to become validator with invalid metadata.
+    let new_validator = "new-validator";
+    let tx_args = apply_use_device(vec![
+        "init-validator",
+        "--alias",
+        new_validator,
+        "--name",
+        new_validator,
+        "--account-keys",
+        "bertha-key",
+        "--commission-rate",
+        "0.05",
+        "--max-commission-rate-change",
+        "0.01",
+        "--email",
+        "[email protected]",
+        "--signing-keys",
+        "bertha-key",
+        "--description",
+        &invalid_desc,
+        "--unsafe-dont-encrypt",
+    ]);
+    let captured = CapturedOutput::of(|| run(&node, Bin::Client, tx_args));
+    assert_matches!(captured.result, Err(_));
+    assert!(captured.contains(TX_REJECTED));
+
+    Ok(())
+}
+
 fn make_migration_json() -> (Hash, tempfile::NamedTempFile) {
     let file = tempfile::Builder::new().tempfile().expect("Test failed");
     let updates = [migrations::DbUpdateType::Add {