Merge of #3821

.changelog/unreleased/bug-fixes/3821-fix-masp-events.md

@@ -0,0 +1,4 @@
+- The masp ref events are now published in a single collection
+  enforcing a correct ordering. Fixed the shielded sync command
+  to account for multiple masp transactions in a single tx.
+  ([\#3821](https://github.com/anoma/namada/pull/3821))

crates/core/src/ibc.rs

@@ -57,24 +57,6 @@ impl FromStr for IbcTokenHash {
 /// 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)
-    }
-}
-
 /// The target of a PGF payment
 #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 #[derive(

crates/core/src/masp.rs

@@ -76,6 +76,12 @@ impl From<TxIdInner> for MaspTxId {
     }
 }
 
+impl Display for MaspTxId {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.0)
+    }
+}
+
 /// Wrapper type around `Epoch` for type safe operations involving the masp
 /// epoch
 #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
@@ -766,24 +772,6 @@ impl FromStr for MaspValue {
     }
 }
 
-/// The masp transactions' references of a given batch
-#[derive(Default, Clone, Serialize, Deserialize)]
-pub struct MaspTxRefs(pub Vec<MaspTxId>);
-
-impl Display for MaspTxRefs {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}", serde_json::to_string(self).unwrap())
-    }
-}
-
-impl FromStr for MaspTxRefs {
-    type Err = serde_json::Error;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        serde_json::from_str(s)
-    }
-}
-
 #[cfg(test)]
 mod test {
     use super::*;

crates/events/src/extend.rs

@@ -9,8 +9,8 @@ use namada_core::address::Address;
 use namada_core::chain::BlockHeight;
 use namada_core::collections::HashMap;
 use namada_core::hash::Hash;
-use namada_core::ibc::IbcTxDataRefs;
-use namada_core::masp::MaspTxRefs;
+use namada_core::ibc::IbcTxDataHash;
+use namada_core::masp::MaspTxId;
 use namada_core::storage::TxIndex;
 use serde::Deserializer;
 
@@ -503,30 +503,44 @@ impl EventAttributeEntry<'static> for MaspTxBlockIndex {
     }
 }
 
-/// Extend an [`Event`] with `masp_tx_batch_refs` data, indicating the specific
-/// inner transactions inside the batch that are valid masp txs and the
-/// references to the relative masp sections.
-pub struct MaspTxBatchRefs(pub MaspTxRefs);
+/// A type representing the possible reference to some MASP data, either a masp
+/// section or ibc tx data
+#[derive(Clone, Serialize, Deserialize)]
+pub enum MaspTxRef {
+    /// Reference to a MASP section
+    MaspSection(MaspTxId),
+    /// Reference to an ibc tx data section
+    IbcData(IbcTxDataHash),
+}
 
-impl EventAttributeEntry<'static> for MaspTxBatchRefs {
-    type Value = MaspTxRefs;
-    type ValueOwned = Self::Value;
+/// A list of MASP tx references
+#[derive(Default, Clone, Serialize, Deserialize)]
+pub struct MaspTxRefs(pub Vec<MaspTxRef>);
 
-    const KEY: &'static str = "masp_tx_batch_refs";
+impl Display for MaspTxRefs {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", serde_json::to_string(self).unwrap())
+    }
+}
 
-    fn into_value(self) -> Self::Value {
-        self.0
+impl FromStr for MaspTxRefs {
+    type Err = serde_json::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        serde_json::from_str(s)
     }
 }
 
-/// Extend an [`Event`] with data sections for IBC shielding transfer.
-pub struct IbcMaspTxBatchRefs(pub IbcTxDataRefs);
+/// Extend an [`Event`] with `masp_tx_batch_refs` data, indicating the
+/// references to either the MASP sections or the data sections for shielded
+/// actions.
+pub struct MaspTxBatchRefs(pub MaspTxRefs);
 
-impl EventAttributeEntry<'static> for IbcMaspTxBatchRefs {
-    type Value = IbcTxDataRefs;
+impl EventAttributeEntry<'static> for MaspTxBatchRefs {
+    type Value = MaspTxRefs;
     type ValueOwned = Self::Value;
 
-    const KEY: &'static str = "ibc_masp_tx_batch_refs";
+    const KEY: &'static str = "masp_tx_batch_refs";
 
     fn into_value(self) -> Self::Value {
         self.0

crates/node/src/bench_utils.rs

@@ -26,7 +26,7 @@ use namada_sdk::args::ShieldedSync;
 use namada_sdk::chain::testing::get_dummy_header;
 use namada_sdk::chain::{BlockHeight, ChainId, Epoch};
 use namada_sdk::events::extend::{
-    ComposeEvent, MaspTxBatchRefs, MaspTxBlockIndex,
+    ComposeEvent, MaspTxBatchRefs, MaspTxBlockIndex, MaspTxRef, MaspTxRefs,
 };
 use namada_sdk::events::Event;
 use namada_sdk::gas::TxGasMeter;
@@ -85,9 +85,7 @@ use namada_sdk::queries::{
 use namada_sdk::state::StorageRead;
 use namada_sdk::storage::{Key, KeySeg, TxIndex};
 use namada_sdk::time::DateTimeUtc;
-use namada_sdk::token::{
-    self, Amount, DenominatedAmount, MaspTxRefs, Transfer,
-};
+use namada_sdk::token::{self, Amount, DenominatedAmount, Transfer};
 use namada_sdk::tx::data::pos::Bond;
 use namada_sdk::tx::data::{BatchedTxResult, Fee, TxResult, VpsResult};
 use namada_sdk::tx::event::{new_tx_event, Batch};
@@ -1046,7 +1044,9 @@ impl Client for BenchShell {
                                         if let Section::MaspTx(transaction) =
                                             section
                                         {
-                                            Some(transaction.txid().into())
+                                            Some(MaspTxRef::MaspSection(
+                                                transaction.txid().into(),
+                                            ))
                                         } else {
                                             None
                                         }

crates/node/src/protocol.rs

@@ -8,20 +8,20 @@ use eyre::{eyre, WrapErr};
 use namada_sdk::address::{Address, InternalAddress};
 use namada_sdk::booleans::BoolResultUnitExt;
 use namada_sdk::events::extend::{
-    ComposeEvent, Height as HeightAttr, TxHash as TxHashAttr, UserAccount,
+    ComposeEvent, Height as HeightAttr, MaspTxRef, MaspTxRefs,
+    TxHash as TxHashAttr, UserAccount,
 };
 use namada_sdk::events::EventLevel;
 use namada_sdk::gas::{self, Gas, GasMetering, TxGasMeter, VpGasMeter};
 use namada_sdk::hash::Hash;
-use namada_sdk::ibc::{IbcTxDataHash, IbcTxDataRefs};
 use namada_sdk::parameters::get_gas_scale;
 use namada_sdk::state::{
     DBIter, State, StorageHasher, StorageRead, TxWrites, WlState, DB,
 };
 use namada_sdk::storage::TxIndex;
 use namada_sdk::token::event::{TokenEvent, TokenOperation};
 use namada_sdk::token::utils::is_masp_transfer;
-use namada_sdk::token::{Amount, MaspTxId, MaspTxRefs};
+use namada_sdk::token::Amount;
 use namada_sdk::tx::action::{self, Read};
 use namada_sdk::tx::data::protocol::{ProtocolTx, ProtocolTxType};
 use namada_sdk::tx::data::{
@@ -318,10 +318,9 @@ 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() || !ibc_tx_data_refs.0.is_empty() {
+    if !masp_tx_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();
@@ -351,11 +350,7 @@ where
     H: 'static + StorageHasher + Sync,
     CA: 'static + WasmCacheAccess + Sync,
 {
-    for cmt in get_batch_txs_to_execute(
-        tx,
-        &extended_tx_result.masp_tx_refs,
-        &extended_tx_result.ibc_tx_data_refs,
-    ) {
+    for cmt in get_batch_txs_to_execute(tx, &extended_tx_result.masp_tx_refs) {
         match apply_wasm_tx(
             &tx.batch_ref_tx(cmt),
             &tx_index,
@@ -400,20 +395,7 @@ where
                             cmt,
                             Either::Right(res),
                         )? {
-                            match masp_ref {
-                                Either::Left(masp_section_ref) => {
-                                    extended_tx_result
-                                        .masp_tx_refs
-                                        .0
-                                        .push(masp_section_ref);
-                                }
-                                Either::Right(data_sechash) => {
-                                    extended_tx_result
-                                        .ibc_tx_data_refs
-                                        .0
-                                        .push(data_sechash)
-                                }
-                            }
+                            extended_tx_result.masp_tx_refs.0.push(masp_ref)
                         }
                         state.write_log_mut().commit_tx_to_batch();
                     }
@@ -442,7 +424,7 @@ where
 /// Transaction result for masp transfer
 pub struct MaspTxResult {
     tx_result: BatchedTxResult,
-    masp_section_ref: Either<MaspTxId, IbcTxDataHash>,
+    masp_section_ref: MaspTxRef,
 }
 
 /// Performs the required operation on a wrapper transaction:
@@ -504,12 +486,10 @@ where
                 either::Right(tx.first_commitments().unwrap()),
                 Ok(masp_tx_result.tx_result),
             );
-            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)
+            (
+                batch,
+                Some(MaspTxRefs(vec![masp_tx_result.masp_section_ref])),
+            )
         },
     );
 
@@ -819,7 +799,7 @@ fn get_optional_masp_ref<S: Read<Err = state::Error>>(
     state: &S,
     cmt: &TxCommitments,
     is_masp_tx: Either<bool, &BatchedTxResult>,
-) -> Result<Option<Either<MaspTxId, Hash>>> {
+) -> Result<Option<MaspTxRef>> {
     // Always check that the transaction was indeed a MASP one by looking at the
     // changed keys. A malicious tx could push a MASP Action without touching
     // any storage keys associated with the shielded pool
@@ -834,14 +814,14 @@ fn get_optional_masp_ref<S: Read<Err = state::Error>>(
     let masp_ref = if action::is_ibc_shielding_transfer(state)
         .map_err(Error::StateError)?
     {
-        Some(Either::Right(cmt.data_sechash().to_owned()))
+        Some(MaspTxRef::IbcData(cmt.data_sechash().to_owned()))
     } else {
         let actions = state.read_actions().map_err(Error::StateError)?;
         action::get_masp_section_ref(&actions)
             .map_err(|msg| {
                 Error::StateError(state::Error::new_alloc(msg.to_string()))
             })?
-            .map(Either::Left)
+            .map(MaspTxRef::MaspSection)
     };
 
     Ok(masp_ref)

crates/node/src/shell/finalize_block.rs

@@ -4,8 +4,7 @@ use data_encoding::HEXUPPER;
 use masp_primitives::merkle_tree::CommitmentTree;
 use masp_primitives::sapling::Node;
 use namada_sdk::events::extend::{
-    ComposeEvent, Height, IbcMaspTxBatchRefs, Info, MaspTxBatchRefs,
-    MaspTxBlockIndex, TxHash,
+    ComposeEvent, Height, Info, MaspTxBatchRefs, MaspTxBlockIndex, TxHash,
 };
 use namada_sdk::events::{EmitEvents, Event};
 use namada_sdk::gas::event::GasUsed;
@@ -1050,14 +1049,6 @@ impl<'finalize> TempTxLogs {
             ));
         }
 
-        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
     }
 }