merge queue: embarking main (b8dabb2) and [#4019 + #4036] together

.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))

.changelog/unreleased/improvements/4019-improve-speculative-shielded-ctx.md

@@ -0,0 +1,3 @@
+- The speculative shielded context now avoids updating its
+  state if the transaction failed. Added a test for it.
+  ([\#4019](https://github.com/anoma/namada/pull/4019))

crates/apps_lib/src/client/rpc.rs

@@ -4,6 +4,7 @@ use std::collections::{BTreeMap, BTreeSet};
 use std::io;
 
 use borsh::BorshDeserialize;
+use color_eyre::owo_colors::OwoColorize;
 use data_encoding::HEXLOWER;
 use either::Either;
 use masp_primitives::asset_type::AssetType;
@@ -395,6 +396,15 @@ async fn query_shielded_balance(
     context: &impl Namada,
     args: args::QueryBalance,
 ) {
+    display_line!(
+        context.io(),
+        "{}: {}\n",
+        "WARNING".bold().underline().yellow(),
+        "The resulting balance could be outdated, make sure to run `namadac \
+         shielded-sync` before querying the balance to get the most recent \
+         value."
+    );
+
     let args::QueryBalance {
         // Token owner (needs to be a viewing key)
         owner,

crates/apps_lib/src/client/tx.rs

@@ -3,7 +3,9 @@ use std::io::Write;
 
 use borsh::BorshDeserialize;
 use borsh_ext::BorshSerializeExt;
+use color_eyre::owo_colors::OwoColorize;
 use ledger_namada_rs::{BIP44Path, NamadaApp};
+use namada_core::masp::MaspTransaction;
 use namada_sdk::address::{Address, ImplicitAddress};
 use namada_sdk::args::TxBecomeValidator;
 use namada_sdk::collections::HashSet;
@@ -637,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
@@ -829,13 +830,33 @@ pub async fn submit_shielded_transfer(
     namada: &impl Namada,
     args: args::TxShieldedTransfer,
 ) -> Result<(), error::Error> {
+    display_line!(
+        namada.io(),
+        "{}: {}\n",
+        "WARNING".bold().underline().yellow(),
+        "Some information might be leaked if your shielded wallet is not up \
+         to date, make sure to run `namadac shielded-sync` before running \
+         this command.",
+    );
+
     let (mut tx, signing_data) = args.clone().build(namada).await?;
 
+    let masp_section = tx
+        .sections
+        .iter()
+        .find_map(|section| section.masp_tx())
+        .ok_or_else(|| {
+            error::Error::Other(
+                "Missing MASP section in shielded transaction".to_string(),
+            )
+        })?;
     if args.tx.dump_tx || args.tx.dump_wrapper_tx {
         tx::dump_tx(namada.io(), &args.tx, tx)?;
+        pre_cache_masp_data(namada, &masp_section).await;
     } else {
         sign(namada, &mut tx, &args.tx, signing_data).await?;
-        namada.submit(tx, &args.tx).await?;
+        let res = namada.submit(tx, &args.tx).await?;
+        pre_cache_masp_data_on_tx_result(namada, &res, &masp_section).await;
     }
     Ok(())
 }
@@ -900,13 +921,33 @@ pub async fn submit_unshielding_transfer(
     namada: &impl Namada,
     args: args::TxUnshieldingTransfer,
 ) -> Result<(), error::Error> {
+    display_line!(
+        namada.io(),
+        "{}: {}\n",
+        "WARNING".bold().underline().yellow(),
+        "Some information might be leaked if your shielded wallet is not up \
+         to date, make sure to run `namadac shielded-sync` before running \
+         this command.",
+    );
+
     let (mut tx, signing_data) = args.clone().build(namada).await?;
 
+    let masp_section = tx
+        .sections
+        .iter()
+        .find_map(|section| section.masp_tx())
+        .ok_or_else(|| {
+            error::Error::Other(
+                "Missing MASP section in shielded transaction".to_string(),
+            )
+        })?;
     if args.tx.dump_tx || args.tx.dump_wrapper_tx {
         tx::dump_tx(namada.io(), &args.tx, tx)?;
+        pre_cache_masp_data(namada, &masp_section).await;
     } else {
         sign(namada, &mut tx, &args.tx, signing_data).await?;
-        namada.submit(tx, &args.tx).await?;
+        let res = namada.submit(tx, &args.tx).await?;
+        pre_cache_masp_data_on_tx_result(namada, &res, &masp_section).await;
     }
     Ok(())
 }
@@ -920,16 +961,25 @@ where
 {
     let (tx, signing_data, _) = args.build(namada).await?;
 
+    let opt_masp_section =
+        tx.sections.iter().find_map(|section| section.masp_tx());
     if args.tx.dump_tx || args.tx.dump_wrapper_tx {
         tx::dump_tx(namada.io(), &args.tx, tx)?;
+        if let Some(masp_section) = opt_masp_section {
+            pre_cache_masp_data(namada, &masp_section).await;
+        }
     } else {
-        batch_opt_reveal_pk_and_submit(
+        let res = batch_opt_reveal_pk_and_submit(
             namada,
             &args.tx,
             &[&args.source.effective_address()],
             (tx, signing_data),
         )
         .await?;
+
+        if let Some(masp_section) = opt_masp_section {
+            pre_cache_masp_data_on_tx_result(namada, &res, &masp_section).await;
+        }
     }
     // NOTE that the tx could fail when its submission epoch doesn't match
     // construction epoch
@@ -1482,3 +1532,42 @@ pub async fn gen_ibc_shielding_transfer(
     }
     Ok(())
 }
+
+// Pre-cache the data for the provided MASP transaction. Log an error on
+// failure.
+async fn pre_cache_masp_data(namada: &impl Namada, masp_tx: &MaspTransaction) {
+    if let Err(e) = namada
+        .shielded_mut()
+        .await
+        .pre_cache_transaction(masp_tx)
+        .await
+    {
+        // Just display the error but do not propagate it
+        edisplay_line!(namada.io(), "Failed to pre-cache masp data: {}.", e);
+    }
+}
+
+// Check the result of a transaction and pre-cache the masp data accordingly
+async fn pre_cache_masp_data_on_tx_result(
+    namada: &impl Namada,
+    tx_result: &ProcessTxResponse,
+    masp_tx: &MaspTransaction,
+) {
+    match tx_result {
+        ProcessTxResponse::Applied(resp) => {
+            if let Some(InnerTxResult::Success(_)) =
+                // If we have the masp data in an ibc transfer it
+                // means we are unshielding, so there's no reveal pk
+                // tx in the batch which contains only the ibc tx
+                resp.batch_result().first().map(|(_, res)| res)
+            {
+                pre_cache_masp_data(namada, masp_tx).await;
+            }
+        }
+        ProcessTxResponse::Broadcast(_) => {
+            pre_cache_masp_data(namada, masp_tx).await;
+        }
+        // Do not pre-cache when dry-running
+        ProcessTxResponse::DryRun(_) => {}
+    }
+}

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/node/src/bench_utils.rs

@@ -1223,7 +1223,6 @@ impl BenchShieldedCtx {
                         vec![masp_transfer_data],
                         None,
                         expiration,
-                        true,
                     )
                     .await
             })

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 {