Merge branch 'yuji/fix-ibc-masp-tx-extraction' (#3490)

* origin/yuji/fix-ibc-masp-tx-extraction:
  fix KEY name
  check ibc masp action
  add IbcTxDataRefs

.changelog/v0.40.0/bug-fixes/3488-fix-ibc-masp-tx-extraction.md

@@ -0,0 +1,2 @@
+- Fix to extract MASP transaction when IBC shielding transfer
+  ([\#3488](https://github.com/anoma/namada/issues/3488))

crates/core/src/ibc.rs

@@ -1,5 +1,6 @@
 //! IBC-related data types
 
+use std::fmt::Display;
 use std::str::FromStr;
 
 use borsh::{BorshDeserialize, BorshSchema, BorshSerialize};
@@ -11,6 +12,7 @@ use namada_migrations::*;
 use serde::{Deserialize, Serialize};
 
 use super::address::HASH_LEN;
+use crate::hash::Hash;
 
 /// IBC token hash derived from a denomination.
 #[derive(
@@ -47,3 +49,24 @@ impl FromStr for IbcTokenHash {
         Ok(IbcTokenHash(output))
     }
 }
+
+/// IBC transaction data section hash
+pub type IbcTxDataHash = Hash;
+
+/// IBC transaction data references to retrieve IBC messages
+#[derive(Default, Clone, Serialize, Deserialize)]
+pub struct IbcTxDataRefs(pub Vec<IbcTxDataHash>);
+
+impl Display for IbcTxDataRefs {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", serde_json::to_string(self).unwrap())
+    }
+}
+
+impl FromStr for IbcTxDataRefs {
+    type Err = serde_json::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        serde_json::from_str(s)
+    }
+}

crates/events/src/extend.rs

@@ -8,6 +8,7 @@ use std::str::FromStr;
 use namada_core::address::Address;
 use namada_core::collections::HashMap;
 use namada_core::hash::Hash;
+use namada_core::ibc::IbcTxDataRefs;
 use namada_core::masp::MaspTxRefs;
 use namada_core::storage::{BlockHeight, TxIndex};
 use serde::Deserializer;
@@ -517,6 +518,20 @@ impl EventAttributeEntry<'static> for MaspTxBatchRefs {
     }
 }
 
+/// Extend an [`Event`] with data sections for IBC shielding transfer.
+pub struct IbcMaspTxBatchRefs(pub IbcTxDataRefs);
+
+impl EventAttributeEntry<'static> for IbcMaspTxBatchRefs {
+    type Value = IbcTxDataRefs;
+    type ValueOwned = Self::Value;
+
+    const KEY: &'static str = "ibc_masp_tx_batch_refs";
+
+    fn into_value(self) -> Self::Value {
+        self.0
+    }
+}
+
 /// Extend an [`Event`] with success data.
 pub struct Success(pub bool);
 

crates/namada/src/ledger/mod.rs

@@ -109,15 +109,17 @@ mod dry_run_tx {
         let ExtendedTxResult {
             mut tx_result,
             ref masp_tx_refs,
-            is_ibc_shielding: _,
+            ref ibc_tx_data_refs,
         } = extended_tx_result;
         let tx_gas_meter = RefCell::new(tx_gas_meter);
-        for cmt in
-            crate::ledger::protocol::get_batch_txs_to_execute(&tx, masp_tx_refs)
-        {
+        for cmt in crate::ledger::protocol::get_batch_txs_to_execute(
+            &tx,
+            masp_tx_refs,
+            ibc_tx_data_refs,
+        ) {
             let batched_tx = tx.batch_ref_tx(cmt);
             let batched_tx_result = protocol::apply_wasm_tx(
-                batched_tx,
+                &batched_tx,
                 &TxIndex(0),
                 ShellParams::new(
                     &tx_gas_meter,

crates/namada/src/ledger/protocol/mod.rs

@@ -3,9 +3,11 @@ use std::cell::RefCell;
 use std::collections::BTreeSet;
 use std::fmt::Debug;
 
+use either::Either;
 use eyre::{eyre, WrapErr};
 use namada_core::booleans::BoolResultUnitExt;
 use namada_core::hash::Hash;
+use namada_core::ibc::{IbcTxDataHash, IbcTxDataRefs};
 use namada_core::masp::{MaspTxRefs, TxId};
 use namada_events::extend::{
     ComposeEvent, Height as HeightAttr, TxHash as TxHashAttr, UserAccount,
@@ -16,7 +18,7 @@ use namada_parameters::get_gas_scale;
 use namada_state::TxWrites;
 use namada_token::event::{TokenEvent, TokenOperation};
 use namada_token::utils::is_masp_transfer;
-use namada_tx::action::Read;
+use namada_tx::action::{is_ibc_shielding_transfer, Read};
 use namada_tx::data::protocol::{ProtocolTx, ProtocolTxType};
 use namada_tx::data::{
     BatchedTxResult, ExtendedTxResult, TxResult, VpStatusFlags, VpsResult,
@@ -265,7 +267,7 @@ where
                 let cmt =
                     tx.first_commitments().ok_or(Error::MissingInnerTxs)?;
                 let batched_tx_result = apply_wasm_tx(
-                    tx.batch_ref_tx(cmt),
+                    &tx.batch_ref_tx(cmt),
                     &tx_index,
                     ShellParams {
                         tx_gas_meter,
@@ -336,9 +338,10 @@ where
 pub(crate) fn get_batch_txs_to_execute<'a>(
     tx: &'a Tx,
     masp_tx_refs: &MaspTxRefs,
+    ibc_tx_data_refs: &IbcTxDataRefs,
 ) -> impl Iterator<Item = &'a TxCommitments> {
     let mut batch_iter = tx.commitments().iter();
-    if !masp_tx_refs.0.is_empty() {
+    if !masp_tx_refs.0.is_empty() || !ibc_tx_data_refs.0.is_empty() {
         // If fees were paid via masp skip the first transaction of the batch
         // which has already been executed
         batch_iter.next();
@@ -363,9 +366,13 @@ where
     H: 'static + StorageHasher + Sync,
     CA: 'static + WasmCacheAccess + Sync,
 {
-    for cmt in get_batch_txs_to_execute(tx, &extended_tx_result.masp_tx_refs) {
+    for cmt in get_batch_txs_to_execute(
+        tx,
+        &extended_tx_result.masp_tx_refs,
+        &extended_tx_result.ibc_tx_data_refs,
+    ) {
         match apply_wasm_tx(
-            tx.batch_ref_tx(cmt),
+            &tx.batch_ref_tx(cmt),
             &tx_index,
             ShellParams {
                 tx_gas_meter,
@@ -408,9 +415,14 @@ where
                             .0
                             .push(masp_section_ref);
                     }
-                    extended_tx_result.is_ibc_shielding =
-                        namada_tx::action::is_ibc_shielding_transfer(state)
-                            .map_err(Error::StateError)?;
+                    if is_ibc_shielding_transfer(&*state)
+                        .map_err(Error::StateError)?
+                    {
+                        extended_tx_result
+                            .ibc_tx_data_refs
+                            .0
+                            .push(*cmt.data_sechash())
+                    }
                     state.write_log_mut().commit_tx_to_batch();
                 } else {
                     state.write_log_mut().drop_tx();
@@ -431,6 +443,12 @@ where
     Ok(extended_tx_result)
 }
 
+/// Transaction result for masp transfer
+pub struct MaspTxResult {
+    tx_result: BatchedTxResult,
+    masp_section_ref: Either<TxId, IbcTxDataHash>,
+}
+
 /// Performs the required operation on a wrapper transaction:
 ///  - replay protection
 ///  - fee payment
@@ -471,18 +489,23 @@ where
     // have propagated an error)
     shell_params.state.write_log_mut().commit_batch();
 
-    let (batch_results, masp_tx_refs) = payment_result.map_or_else(
+    let (batch_results, masp_section_refs) = payment_result.map_or_else(
         || (TxResult::default(), None),
-        |(batched_result, masp_section_ref)| {
+        |masp_tx_result| {
             let mut batch = TxResult::default();
             batch.insert_inner_tx_result(
                 // Ok to unwrap cause if we have a batched result it means
                 // we've executed the first tx in the batch
                 tx.wrapper_hash().as_ref(),
                 either::Right(tx.first_commitments().unwrap()),
-                Ok(batched_result),
+                Ok(masp_tx_result.tx_result),
             );
-            (batch, Some(MaspTxRefs(vec![masp_section_ref])))
+            let masp_section_refs =
+                Some(masp_tx_result.masp_section_ref.map_either(
+                    |masp_tx_ref| MaspTxRefs(vec![masp_tx_ref]),
+                    |ibc_tx_data| IbcTxDataRefs(vec![ibc_tx_data]),
+                ));
+            (batch, masp_section_refs)
         },
     );
 
@@ -492,7 +515,7 @@ where
         .add_wrapper_gas(tx_bytes)
         .map_err(|err| Error::GasError(err.to_string()))?;
 
-    Ok(batch_results.to_extended_result(masp_tx_refs))
+    Ok(batch_results.to_extended_result(masp_section_refs))
 }
 
 /// Perform the actual transfer of fees from the fee payer to the block
@@ -504,7 +527,7 @@ pub fn transfer_fee<S, D, H, CA>(
     tx: &Tx,
     wrapper: &WrapperTx,
     tx_index: &TxIndex,
-) -> Result<Option<(BatchedTxResult, TxId)>>
+) -> Result<Option<MaspTxResult>>
 where
     S: State<D = D, H = H>
         + StorageRead
@@ -657,7 +680,7 @@ fn try_masp_fee_payment<S, D, H, CA>(
     }: &mut ShellParams<'_, S, D, H, CA>,
     tx: &Tx,
     tx_index: &TxIndex,
-) -> Result<Option<(BatchedTxResult, TxId)>>
+) -> Result<Option<MaspTxResult>>
 where
     S: State<D = D, H = H>
         + StorageRead
@@ -693,9 +716,11 @@ where
     let ref_unshield_gas_meter = RefCell::new(gas_meter);
 
     let valid_batched_tx_result = {
+        let first_tx = tx
+            .batch_ref_first_tx()
+            .ok_or_else(|| Error::MissingInnerTxs)?;
         match apply_wasm_tx(
-            tx.batch_ref_first_tx()
-                .ok_or_else(|| Error::MissingInnerTxs)?,
+            &first_tx,
             tx_index,
             ShellParams {
                 tx_gas_meter: &ref_unshield_gas_meter,
@@ -720,16 +745,32 @@ where
                     );
                 }
 
-                let masp_ref = namada_tx::action::get_masp_section_ref(*state)
-                    .map_err(Error::StateError)?;
                 // Ensure that the transaction is actually a masp one, otherwise
                 // reject
-                (is_masp_transfer(&result.changed_keys) && result.is_accepted())
-                    .then(|| {
-                        masp_ref
-                            .map(|masp_section_ref| (result, masp_section_ref))
-                    })
-                    .flatten()
+                if is_masp_transfer(&result.changed_keys)
+                    && result.is_accepted()
+                {
+                    if let Some(masp_tx_id) =
+                        namada_tx::action::get_masp_section_ref(*state)
+                            .map_err(Error::StateError)?
+                    {
+                        Some(MaspTxResult {
+                            tx_result: result,
+                            masp_section_ref: Either::Left(masp_tx_id),
+                        })
+                    } else {
+                        is_ibc_shielding_transfer(*state)
+                            .map_err(Error::StateError)?
+                            .then_some(MaspTxResult {
+                                tx_result: result,
+                                masp_section_ref: Either::Right(
+                                    *first_tx.cmt.data_sechash(),
+                                ),
+                            })
+                    }
+                } else {
+                    None
+                }
             }
             Err(e) => {
                 state.write_log_mut().drop_tx();
@@ -787,7 +828,7 @@ pub fn check_fees<S, D, H, CA>(
     shell_params: &mut ShellParams<'_, S, D, H, CA>,
     tx: &Tx,
     wrapper: &WrapperTx,
-) -> Result<Option<(BatchedTxResult, TxId)>>
+) -> Result<Option<MaspTxResult>>
 where
     S: State<D = D, H = H>
         + StorageRead
@@ -857,7 +898,7 @@ where
 /// Apply a transaction going via the wasm environment. Gas will be metered and
 /// validity predicates will be triggered in the normal way.
 pub fn apply_wasm_tx<'a, S, D, H, CA>(
-    batched_tx: BatchedTxRef<'_>,
+    batched_tx: &BatchedTxRef<'_>,
     tx_index: &TxIndex,
     shell_params: ShellParams<'a, S, D, H, CA>,
 ) -> Result<BatchedTxResult>
@@ -875,7 +916,7 @@ where
     } = shell_params;
 
     let verifiers = execute_tx(
-        &batched_tx,
+        batched_tx,
         tx_index,
         state,
         tx_gas_meter,
@@ -884,7 +925,7 @@ where
     )?;
 
     let vps_result = check_vps(CheckVps {
-        batched_tx: &batched_tx,
+        batched_tx,
         tx_index,
         state,
         tx_gas_meter: &mut tx_gas_meter.borrow_mut(),

crates/node/src/shell/finalize_block.rs

@@ -9,7 +9,8 @@ use namada::gas::event::GasUsed;
 use namada::governance::pgf::inflation as pgf_inflation;
 use namada::hash::Hash;
 use namada::ledger::events::extend::{
-    ComposeEvent, Height, Info, MaspTxBatchRefs, MaspTxBlockIndex, TxHash,
+    ComposeEvent, Height, IbcMaspTxBatchRefs, Info, MaspTxBatchRefs,
+    MaspTxBlockIndex, TxHash,
 };
 use namada::ledger::events::EmitEvents;
 use namada::ledger::gas::GasMetering;
@@ -1047,9 +1048,12 @@ impl<'finalize> TempTxLogs {
             ));
         }
 
-        if extended_tx_result.is_ibc_shielding {
+        if !extended_tx_result.ibc_tx_data_refs.0.is_empty() {
             self.tx_event
                 .extend(MaspTxBlockIndex(TxIndex::must_from_usize(tx_index)));
+            self.tx_event.extend(IbcMaspTxBatchRefs(
+                extended_tx_result.ibc_tx_data_refs.clone(),
+            ));
         }
 
         flags