Merge branch 'grarco/masp-cross-epoch-proof-fix' (anoma#2222)

* origin/grarco/masp-cross-epoch-proof-fix:
  Changelog anoma#2222
  Updates masp fixtures for tests
  Refactors `convert_amount`
  Tests masp cross-epoch unshield
  Allows unshielding assets from previous epochs

.changelog/unreleased/improvements/2222-masp-cross-epoch-proof-fix.md

@@ -0,0 +1,2 @@
+- Allowed the unshielding of previous epochs assets from the masp.
+  ([\#2222](https://github.com/anoma/namada/pull/2222))

shared/src/ledger/native_vp/masp.rs

@@ -45,7 +45,7 @@ where
     pub ctx: Ctx<'a, DB, H, CA>,
 }
 
-/// Generates the current asset type given the current epoch and an
+/// Generates the current asset type given the provided epoch and an
 /// unique token address
 fn asset_type_from_epoched_address(
     epoch: Epoch,
@@ -58,23 +58,6 @@ fn asset_type_from_epoched_address(
     AssetType::new(token_bytes.as_ref()).expect("unable to create asset type")
 }
 
-/// Checks if the asset type matches the expected asset type, Adds a
-/// debug log if the values do not match.
-fn valid_asset_type(
-    asset_type: &AssetType,
-    asset_type_to_test: &AssetType,
-) -> bool {
-    let res =
-        asset_type.get_identifier() == asset_type_to_test.get_identifier();
-    if !res {
-        tracing::debug!(
-            "The asset type must be derived from the token address and \
-             current epoch"
-        );
-    }
-    res
-}
-
 /// Checks if the reported transparent amount and the unshielded
 /// values agree, if not adds to the debug log
 fn valid_transfer_amount(
@@ -99,13 +82,12 @@ fn convert_amount(
     token: &Address,
     val: token::Amount,
     denom: token::MaspDenom,
-) -> (AssetType, I128Sum) {
+) -> I128Sum {
     let asset_type = asset_type_from_epoched_address(epoch, token, denom);
     // Combine the value and unit into one amount
-    let amount =
-        I128Sum::from_nonnegative(asset_type, denom.denominate(&val) as i128)
-            .expect("invalid value or asset type for amount");
-    (asset_type, amount)
+
+    I128Sum::from_nonnegative(asset_type, denom.denominate(&val) as i128)
+        .expect("invalid value or asset type for amount")
 }
 
 impl<'a, DB, H, CA> NativeVp for MaspVp<'a, DB, H, CA>
@@ -134,7 +116,7 @@ where
             // where the shielded value has an incorrect timestamp
             // are automatically rejected
             for denom in token::MaspDenom::iter() {
-                let (_transp_asset, transp_amt) = convert_amount(
+                let transp_amt = convert_amount(
                     epoch,
                     &transfer.token,
                     transfer.amount.into(),
@@ -196,29 +178,32 @@ where
                     None => continue,
                 };
 
-                let expected_asset_type: AssetType =
-                    asset_type_from_epoched_address(
-                        epoch,
-                        &transfer.token,
-                        denom,
-                    );
-
                 // Satisfies 2. and 3.
-                if !valid_asset_type(&expected_asset_type, &out.asset_type) {
-                    // we don't know which masp denoms are necessary
-                    // apriori. This is encoded via
-                    // the asset types.
-                    continue;
-                }
+                let conversion_state = self.ctx.storage.get_conversion_state();
+                let asset_epoch =
+                    match conversion_state.assets.get(&out.asset_type) {
+                        Some(((address, _), asset_epoch, _, _))
+                            if address == &transfer.token =>
+                        {
+                            asset_epoch
+                        }
+                        _ => {
+                            // we don't know which masp denoms are necessary
+                            // apriori. This is encoded via
+                            // the asset types.
+                            continue;
+                        }
+                    };
+
                 if !valid_transfer_amount(
                     out.value,
                     denom.denominate(&transfer.amount.amount),
                 ) {
                     return Ok(false);
                 }
 
-                let (_transp_asset, transp_amt) = convert_amount(
-                    epoch,
+                let transp_amt = convert_amount(
+                    *asset_epoch,
                     &transfer.token,
                     transfer.amount.amount,
                     denom,
@@ -285,6 +270,7 @@ where
             }
             _ => {}
         }
+
         // Verify the proofs and charge the gas for the expensive execution
         self.ctx
             .charge_gas(MASP_VERIFY_SHIELDED_TX_GAS)

tests/src/integration/masp.rs

@@ -1163,9 +1163,6 @@ fn wrapper_fee_unshielding() -> Result<()> {
     let validator_one_rpc = "127.0.0.1:26567";
     // Download the shielded pool parameters before starting node
     let _ = FsShieldedUtils::new(PathBuf::new());
-    // Lengthen epoch to ensure that a transaction can be constructed and
-    // submitted within the same block. Necessary to ensure that conversion is
-    // not invalidated.
     let (mut node, _services) = setup::setup()?;
     _ = node.next_epoch();
 
@@ -1248,3 +1245,90 @@ fn wrapper_fee_unshielding() -> Result<()> {
     assert!(tx_run);
     Ok(())
 }
+
+// Test that a masp unshield transaction can be succesfully executed even across
+// an epoch boundary.
+#[test]
+fn cross_epoch_tx() -> Result<()> {
+    // This address doesn't matter for tests. But an argument is required.
+    let validator_one_rpc = "127.0.0.1:26567";
+    // Download the shielded pool parameters before starting node
+    let _ = FsShieldedUtils::new(PathBuf::new());
+    let (mut node, _services) = setup::setup()?;
+    _ = node.next_epoch();
+
+    // 1. Shield some tokens
+    run(
+        &node,
+        Bin::Client,
+        vec![
+            "transfer",
+            "--source",
+            ALBERT,
+            "--target",
+            AA_PAYMENT_ADDRESS,
+            "--token",
+            NAM,
+            "--amount",
+            "1000",
+            "--ledger-address",
+            validator_one_rpc,
+        ],
+    )?;
+    node.assert_success();
+
+    // 2. Generate the tx in the current epoch
+    let tempdir = tempfile::tempdir().unwrap();
+    run(
+        &node,
+        Bin::Client,
+        vec![
+            "transfer",
+            "--source",
+            A_SPENDING_KEY,
+            "--target",
+            BERTHA,
+            "--token",
+            NAM,
+            "--amount",
+            "100",
+            "--gas-payer",
+            ALBERT_KEY,
+            "--output-folder-path",
+            tempdir.path().to_str().unwrap(),
+            "--dump-tx",
+            "--ledger-address",
+            validator_one_rpc,
+        ],
+    )?;
+    node.assert_success();
+
+    // Look for the only file in the temp dir
+    let tx_path = tempdir
+        .path()
+        .read_dir()
+        .unwrap()
+        .next()
+        .unwrap()
+        .unwrap()
+        .path();
+
+    // 3. Submit the unshielding in the following epoch
+    _ = node.next_epoch();
+    run(
+        &node,
+        Bin::Client,
+        vec![
+            "tx",
+            "--owner",
+            ALBERT_KEY,
+            "--tx-path",
+            tx_path.to_str().unwrap(),
+            "--ledger-address",
+            validator_one_rpc,
+        ],
+    )?;
+    node.assert_success();
+
+    Ok(())
+}