Merge branch 'murisi/masp-fixes' (#2371)

* origin/murisi/masp-fixes:
  Prevent the backdating of transparent inputs. Be more liberal and allow the unshielding of unepoched assets in any epoch.
  Added support for non-timestamped assets.
  Added changelog entry.
  Unified the functions that construct AssetType to ensure encoding consistency.
  Fixed the printing of shielded balance for specific token across users.
  Improve the handling of trace amounts during conversions of assets to older epochs.
  Shielded balance printing now includes trace assets.
  Allow notes containing older epochs to be spent.
  Use a note only if theres a shortfall and it would help close it.
  Stop creating redundant transaction descriptors.
  Redefined MaspAmount using the ValueSum type from the MASP crate.
  Removed now redundant operations on MaspAmount.
  Reduce the amount of decoding and encoding of AssetTypes.
  Refactored the validation of shielded transaction transparent outputs. Now avoid assuming that transparent outputs are ordered. Avoid assuming that there must be transparent outputs. Ensure that total of transparent output values match the containing transaction. Increased the validation of transparent inputs to be similar to that of transparent outputs.

.changelog/unreleased/improvements/2371-masp-fixes.md

@@ -0,0 +1,2 @@
+- Strengthened the checks in the MASP VP. Allow viewing and spending of
+  unconvertible MASP notes ([\#2371](https://github.com/anoma/namada/pull/2371))

core/src/ledger/masp_conversions.rs

@@ -3,7 +3,6 @@
 use std::collections::BTreeMap;
 
 use borsh::{BorshDeserialize, BorshSerialize};
-use borsh_ext::BorshSerializeExt;
 use masp_primitives::asset_type::AssetType;
 use masp_primitives::convert::AllowedConversion;
 use masp_primitives::merkle_tree::FrozenCommitmentTree;
@@ -212,6 +211,9 @@ where
     };
     use rayon::prelude::ParallelSlice;
 
+    use crate::ledger::storage_api::ResultExt;
+    use crate::types::masp::encode_asset_type;
+
     // The derived conversions will be placed in MASP address space
     let masp_addr = MASP;
 
@@ -242,10 +244,14 @@ where
     // notes clients have to use. This trick works under the assumption that
     // reward tokens will then be reinflated back to the current epoch.
     let reward_assets = [
-        encode_asset_type(native_token.clone(), MaspDenom::Zero, Epoch(0)),
-        encode_asset_type(native_token.clone(), MaspDenom::One, Epoch(0)),
-        encode_asset_type(native_token.clone(), MaspDenom::Two, Epoch(0)),
-        encode_asset_type(native_token.clone(), MaspDenom::Three, Epoch(0)),
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::Zero)
+            .into_storage_result()?,
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::One)
+            .into_storage_result()?,
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::Two)
+            .into_storage_result()?,
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::Three)
+            .into_storage_result()?,
     ];
     // Conversions from the previous to current asset for each address
     let mut current_convs =
@@ -269,15 +275,17 @@ where
             // negative sign allows each instance of the old asset to be
             // cancelled out/replaced with the new asset
             let old_asset = encode_asset_type(
-                addr.clone(),
+                Some(wl_storage.storage.last_epoch),
+                addr,
                 denom,
-                wl_storage.storage.last_epoch,
-            );
+            )
+            .into_storage_result()?;
             let new_asset = encode_asset_type(
-                addr.clone(),
+                Some(wl_storage.storage.block.epoch),
+                addr,
                 denom,
-                wl_storage.storage.block.epoch,
-            );
+            )
+            .into_storage_result()?;
             // Get the last rewarded amount of the native token
             let normed_inflation = wl_storage
                 .storage
@@ -325,9 +333,12 @@ where
                 if denom == MaspDenom::Three {
                     // The reward for each reward.1 units of the current asset
                     // is reward.0 units of the reward token
-                    total_reward += (addr_bal
-                        * (new_normed_inflation, *normed_inflation))
+                    let native_reward =
+                        addr_bal * (new_normed_inflation, *normed_inflation);
+                    total_reward += native_reward
                         .0
+                        .checked_add(native_reward.1)
+                        .unwrap_or(token::Amount::max())
                         .checked_sub(addr_bal)
                         .unwrap_or_default();
                     // Save the new normed inflation
@@ -370,9 +381,7 @@ where
                 if denom == MaspDenom::Three {
                     // The reward for each reward.1 units of the current asset
                     // is reward.0 units of the reward token
-                    total_reward += ((addr_bal * (real_reward, reward.1)).0
-                        * (*normed_inflation, ref_inflation))
-                        .0;
+                    total_reward += (addr_bal * (reward.0, reward.1)).0;
                 }
             }
             // Add a conversion from the previous asset type
@@ -461,10 +470,11 @@ where
             // Add the decoding entry for the new asset type. An uncommitted
             // node position is used since this is not a conversion.
             let new_asset = encode_asset_type(
-                addr.clone(),
+                Some(wl_storage.storage.block.epoch),
+                &addr,
                 denom,
-                wl_storage.storage.block.epoch,
-            );
+            )
+            .into_storage_result()?;
             wl_storage.storage.conversion_state.assets.insert(
                 new_asset,
                 (
@@ -480,17 +490,6 @@ where
     Ok(())
 }
 
-/// Construct MASP asset type with given epoch for given token
-pub fn encode_asset_type(
-    addr: Address,
-    denom: MaspDenom,
-    epoch: Epoch,
-) -> AssetType {
-    let new_asset_bytes = (addr, denom, epoch.0).serialize_to_vec();
-    AssetType::new(new_asset_bytes.as_ref())
-        .expect("unable to derive asset identifier")
-}
-
 #[cfg(test)]
 mod tests {
     use std::collections::HashMap;

core/src/ledger/storage/mod.rs

@@ -29,7 +29,7 @@ use crate::ledger::gas::{
     STORAGE_ACCESS_GAS_PER_BYTE, STORAGE_WRITE_GAS_PER_BYTE,
 };
 pub use crate::ledger::masp_conversions::{
-    calculate_masp_rewards, encode_asset_type, ConversionState,
+    calculate_masp_rewards, ConversionState,
 };
 use crate::ledger::parameters::{self, EpochDuration, Parameters};
 use crate::ledger::storage::merkle_tree::{

core/src/proto/types.rs

@@ -769,7 +769,7 @@ pub struct MaspBuilder {
     pub target: crate::types::hash::Hash,
     /// The decoded set of asset types used by the transaction. Useful for
     /// offline wallets trying to display AssetTypes.
-    pub asset_types: HashSet<(Address, MaspDenom, Epoch)>,
+    pub asset_types: HashSet<(Address, MaspDenom, Option<Epoch>)>,
     /// Track how Info objects map to descriptors and outputs
     #[serde(
         serialize_with = "borsh_serde::<SaplingMetadataSerde, _>",

core/src/types/dec.rs

@@ -67,9 +67,10 @@ impl Dec {
     /// the division is impossible (e.g., division by zero or overflow), `None`
     /// is returned.
     ///
-    /// For instance:
+    /// Example:
+    /// ```
+    /// use namada_core::types::dec::Dec;
     ///
-    /// ```ignore
     /// let x = Dec::new(3, 1).unwrap(); // Represents 0.3
     /// let y = Dec::new(2, 1).unwrap(); // Represents 0.2
     /// let result = x.trunc_div(&y).unwrap();

core/src/types/masp.rs

@@ -5,14 +5,34 @@ use std::str::FromStr;
 
 use borsh::{BorshDeserialize, BorshSerialize};
 use borsh_ext::BorshSerializeExt;
+use masp_primitives::asset_type::AssetType;
 use sha2::{Digest, Sha256};
 
 use crate::impl_display_and_from_str_via_format;
 use crate::types::address::{Address, DecodeError, HASH_HEX_LEN, MASP};
+use crate::types::storage::Epoch;
 use crate::types::string_encoding::{
     self, MASP_EXT_FULL_VIEWING_KEY_HRP, MASP_EXT_SPENDING_KEY_HRP,
     MASP_PAYMENT_ADDRESS_HRP,
 };
+use crate::types::token::MaspDenom;
+
+/// Make asset type corresponding to given address and epoch
+pub fn encode_asset_type(
+    epoch: Option<Epoch>,
+    token: &Address,
+    denom: MaspDenom,
+) -> Result<AssetType, std::io::Error> {
+    // Timestamp the chosen token with the current epoch
+    let token_bytes = (token, denom, epoch).serialize_to_vec();
+    // Generate the unique asset identifier from the unique token address
+    AssetType::new(token_bytes.as_ref()).map_err(|_| {
+        std::io::Error::new(
+            std::io::ErrorKind::Other,
+            "unable to create asset type".to_string(),
+        )
+    })
+}
 
 // enough capacity to store the payment address
 // plus the pinned/unpinned discriminant

core/src/types/token.rs

@@ -69,6 +69,11 @@ impl Amount {
         }
     }
 
+    /// Convert a [`u128`] to an [`Amount`].
+    pub fn from_u128(x: u128) -> Self {
+        Self { raw: Uint::from(x) }
+    }
+
     /// Get the amount as a [`Change`]
     pub fn change(&self) -> Change {
         self.raw.try_into().unwrap()
@@ -214,6 +219,26 @@ impl Amount {
         Self { raw: Uint(raw) }
     }
 
+    /// Given a u128 and [`MaspDenom`], construct the corresponding
+    /// amount.
+    pub fn from_masp_denominated_u128(
+        val: u128,
+        denom: MaspDenom,
+    ) -> Option<Self> {
+        let lo = val as u64;
+        let hi = (val >> 64) as u64;
+        let lo_pos = denom as usize;
+        let hi_pos = lo_pos + 1;
+        let mut raw = [0u64; 4];
+        raw[lo_pos] = lo;
+        if hi != 0 && hi_pos >= 4 {
+            return None;
+        } else if hi != 0 {
+            raw[hi_pos] = hi;
+        }
+        Some(Self { raw: Uint(raw) })
+    }
+
     /// Get a string representation of a native token amount.
     pub fn to_string_native(&self) -> String {
         DenominatedAmount {

core/src/types/uint.rs

@@ -8,7 +8,7 @@ use std::ops::{Add, AddAssign, BitAnd, Div, Mul, Neg, Rem, Sub, SubAssign};
 use borsh::{BorshDeserialize, BorshSchema, BorshSerialize};
 use impl_num_traits::impl_uint_num_traits;
 use num_integer::Integer;
-use num_traits::CheckedMul;
+use num_traits::{CheckedAdd, CheckedMul, CheckedSub};
 use uint::construct_uint;
 
 use super::dec::{Dec, POS_DECIMAL_PRECISION};
@@ -555,28 +555,6 @@ impl I256 {
         sign
     }
 
-    /// Adds two [`I256`]'s if the absolute value does
-    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
-    pub fn checked_add(&self, other: &Self) -> Option<Self> {
-        if self.non_negative() == other.non_negative() {
-            self.abs().checked_add(other.abs()).and_then(|val| {
-                Self::try_from(val)
-                    .ok()
-                    .map(|val| if !self.non_negative() { -val } else { val })
-            })
-        } else {
-            Some(*self + *other)
-        }
-    }
-
-    /// Subtracts two [`I256`]'s if the absolute value does
-    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
-    pub fn checked_sub(&self, other: &Self) -> Option<Self> {
-        self.checked_add(&other.neg())
-    }
-
-    ///
-
     /// Changed the inner Uint into a canonical representation.
     fn canonical(self) -> Self {
         Self(self.0.canonical())
@@ -738,6 +716,30 @@ impl Mul<Uint> for I256 {
     }
 }
 
+impl CheckedAdd for I256 {
+    /// Adds two [`I256`]'s if the absolute value does
+    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
+    fn checked_add(&self, other: &Self) -> Option<Self> {
+        if self.non_negative() == other.non_negative() {
+            self.abs().checked_add(other.abs()).and_then(|val| {
+                Self::try_from(val)
+                    .ok()
+                    .map(|val| if !self.non_negative() { -val } else { val })
+            })
+        } else {
+            Some(*self + *other)
+        }
+    }
+}
+
+impl CheckedSub for I256 {
+    /// Subtracts two [`I256`]'s if the absolute value does
+    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
+    fn checked_sub(&self, other: &Self) -> Option<Self> {
+        self.checked_add(&other.neg())
+    }
+}
+
 impl CheckedMul for I256 {
     fn checked_mul(&self, v: &Self) -> Option<Self> {
         let is_negative = self.is_negative() != v.is_negative();

proof_of_stake/Cargo.toml

@@ -22,6 +22,7 @@ namada_core = {path = "../core", default-features = false}
 borsh.workspace = true
 data-encoding.workspace = true
 derivative.workspace = true
+num-traits.workspace = true
 once_cell.workspace = true
 proptest = {workspace = true, optional = true}
 serde.workspace = true

proof_of_stake/src/storage.rs

@@ -16,6 +16,7 @@ use namada_core::types::key::{
 };
 use namada_core::types::storage::Epoch;
 use namada_core::types::token;
+use num_traits::CheckedAdd;
 
 use crate::storage_key::consensus_keys_key;
 use crate::types::{

proof_of_stake/src/validator_set_update.rs

@@ -10,6 +10,7 @@ use namada_core::types::address::Address;
 use namada_core::types::key::PublicKeyTmRawHash;
 use namada_core::types::storage::Epoch;
 use namada_core::types::token;
+use num_traits::ops::checked::CheckedAdd;
 use once_cell::unsync::Lazy;
 
 use crate::storage::{

sdk/Cargo.toml

@@ -75,6 +75,7 @@ namada_core = {path = "../core", default-features = false, features = ["rand"]}
 namada_ethereum_bridge = {path = "../ethereum_bridge", default-features = false}
 namada_proof_of_stake = {path = "../proof_of_stake", default-features = false}
 num256.workspace = true
+num-traits.workspace = true
 orion.workspace = true
 owo-colors = "3.5.0"
 parse_duration = "2.1.1"

sdk/src/eth_bridge/bridge_pool.rs

@@ -20,6 +20,7 @@ use namada_core::types::ethereum_events::EthAddress;
 use namada_core::types::keccak::KeccakHash;
 use namada_core::types::token::{balance_key, Amount};
 use namada_core::types::voting_power::FractionalVotingPower;
+use num_traits::ops::checked::CheckedSub;
 use owo_colors::OwoColorize;
 use serde::Serialize;