diff --git a/.changelog/unreleased/bug-fixes/3821-fix-masp-events.md b/.changelog/unreleased/bug-fixes/3821-fix-masp-events.md new file mode 100644 index 0000000000..bf2f093bba --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/core/src/ibc.rs b/crates/core/src/ibc.rs index aa250e38a7..794d664537 100644 --- a/crates/core/src/ibc.rs +++ b/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); - -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 { - serde_json::from_str(s) - } -} - /// The target of a PGF payment #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] #[derive( diff --git a/crates/core/src/masp.rs b/crates/core/src/masp.rs index 1ae82ee00a..ce9d7967c9 100644 --- a/crates/core/src/masp.rs +++ b/crates/core/src/masp.rs @@ -76,6 +76,12 @@ impl From 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); - -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 { - serde_json::from_str(s) - } -} - #[cfg(test)] mod test { use super::*; diff --git a/crates/events/src/extend.rs b/crates/events/src/extend.rs index 8f3fdcbbb7..b8d93878c5 100644 --- a/crates/events/src/extend.rs +++ b/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); - 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 { + 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 diff --git a/crates/node/src/bench_utils.rs b/crates/node/src/bench_utils.rs index 8a50e3142a..37ba5d52ff 100644 --- a/crates/node/src/bench_utils.rs +++ b/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 } diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 15356eab41..6466c75b83 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -8,12 +8,12 @@ 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, @@ -21,7 +21,7 @@ use namada_sdk::state::{ 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 { 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, + 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>( state: &S, cmt: &TxCommitments, is_masp_tx: Either, -) -> Result>> { +) -> Result> { // 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>( 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) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 8acc54e240..ac2324f2c0 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/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 } } diff --git a/crates/sdk/src/masp.rs b/crates/sdk/src/masp.rs index b5237d5183..81440735da 100644 --- a/crates/sdk/src/masp.rs +++ b/crates/sdk/src/masp.rs @@ -2,7 +2,6 @@ mod utilities; -use eyre::eyre; use masp_primitives::asset_type::AssetType; use masp_primitives::merkle_tree::MerklePath; use masp_primitives::sapling::Node; @@ -10,15 +9,14 @@ use masp_primitives::transaction::components::I128Sum; use masp_primitives::transaction::Transaction; use namada_core::address::Address; use namada_core::chain::BlockHeight; -use namada_core::ibc::IbcTxDataRefs; -use namada_core::masp::{MaspEpoch, MaspTxRefs}; +use namada_core::masp::MaspEpoch; use namada_core::storage::TxIndex; use namada_core::time::DurationSecs; use namada_core::token::{Denomination, MaspDigitPos}; use namada_events::extend::{ - IbcMaspTxBatchRefs as IbcMaspTxBatchRefsAttr, MaspTxBatchRefs as MaspTxBatchRefsAttr, - MaspTxBlockIndex as MaspTxBlockIndexAttr, ReadFromEventAttributes, + MaspTxBlockIndex as MaspTxBlockIndexAttr, MaspTxRef, MaspTxRefs, + ReadFromEventAttributes, }; use namada_ibc::{decode_message, extract_masp_tx_from_envelope, IbcMessage}; use namada_io::client::Client; @@ -34,60 +32,89 @@ use crate::rpc::{ }; use crate::{token, MaybeSend, MaybeSync}; -/// Extract the relevant shield portions of a [`Tx`], if any. +/// Extract the relevant shield portions from a [`Tx`] MASP section or an IBC +/// message, if any. +#[allow(clippy::result_large_err)] fn extract_masp_tx( tx: &Tx, - masp_section_refs: &MaspTxRefs, -) -> Result, eyre::Error> { - // NOTE: simply looking for masp sections attached to the tx - // is not safe. We don't validate the sections attached to a - // transaction se we could end up with transactions carrying - // an unnecessary masp section. We must instead look for the - // required masp sections coming from the events - - masp_section_refs + masp_refs: &MaspTxRefs, +) -> Result, Error> { + // NOTE: It is possible to have two identical references in a same batch: + // this is because, some types of MASP data packet can be correctly executed + // more than once (output descriptions). We have to make sure we account for + // this by using collections that allow for duplicates (both in the args + // and in the returned type): if the same reference shows up multiple + // times in the input we must process it the same number of times to + // ensure we contruct the correct state + masp_refs .0 .iter() - .try_fold(vec![], |mut acc, hash| { - match tx.get_masp_section(hash).cloned().ok_or_else(|| { - eyre!("Missing expected masp transaction".to_string()) - }) { - Ok(transaction) => { + .try_fold(vec![], |mut acc, ref masp_ref| { + match masp_ref { + MaspTxRef::MaspSection(id) => { + // Simply looking for masp sections attached to the tx + // is not safe. We don't validate the sections attached to a + // transaction se we could end up with transactions carrying + // an unnecessary masp section. We must instead look for the + // required masp sections published in the events + let transaction = tx + .get_masp_section(id) + .ok_or_else(|| { + Error::Other(format!( + "Missing expected masp transaction with id \ + {id}" + )) + })? + .clone(); acc.push(transaction); + Ok(acc) } - Err(e) => Err(e), + MaspTxRef::IbcData(hash) => { + // Dereference the masp ref to the first instance that + // matches is, even if it is not the exact one that produced + // the event, the data we extract will be exactly the same + let masp_ibc_tx = tx + .commitments() + .iter() + .find(|cmt| cmt.data_sechash() == hash) + .ok_or_else(|| { + Error::Other(format!( + "Couldn't find data section with hash {hash}" + )) + })?; + let tx_data = tx.data(masp_ibc_tx).ok_or_else(|| { + Error::Other( + "Missing expected data section".to_string(), + ) + })?; + + let IbcMessage::Envelope(envelope) = + decode_message::(&tx_data) + .map_err(|e| Error::Other(e.to_string()))? + else { + return Err(Error::Other( + "Expected IBC packet to be an envelope".to_string(), + )); + }; + + if let Some(transaction) = + extract_masp_tx_from_envelope(&envelope) + { + acc.push(transaction); + + Ok(acc) + } else { + Err(Error::Other( + "Failed to retrieve MASP over IBC transaction" + .to_string(), + )) + } + } } }) } -/// Extract the relevant shield portions from the IBC messages in [`Tx`] -#[allow(clippy::result_large_err)] -fn extract_masp_tx_from_ibc_message( - tx: &Tx, -) -> Result, Error> { - let mut masp_txs = Vec::new(); - for cmt in &tx.header.batch { - let tx_data = tx.data(cmt).ok_or_else(|| { - Error::Other("Missing transaction data".to_string()) - })?; - let ibc_msg = decode_message::(&tx_data) - .map_err(|_| Error::Other("Invalid IBC message".to_string()))?; - if let IbcMessage::Envelope(ref envelope) = ibc_msg { - if let Some(masp_tx) = extract_masp_tx_from_envelope(envelope) { - masp_txs.push(masp_tx); - } - } - } - if !masp_txs.is_empty() { - Ok(masp_txs) - } else { - Err(Error::Other( - "IBC message doesn't have masp transaction".to_string(), - )) - } -} - // Retrieves all the indexes at the specified height which refer // to a valid masp transaction. If an index is given, it filters only the // transactions with an index equal or greater to the provided one. @@ -95,10 +122,7 @@ async fn get_indexed_masp_events_at_height( client: &C, height: BlockHeight, first_idx_to_query: Option, -) -> Result< - Option, Option)>>, - Error, -> { +) -> Result, Error> { let first_idx_to_query = first_idx_to_query.unwrap_or_default(); Ok(client @@ -118,24 +142,20 @@ async fn get_indexed_masp_events_at_height( if tx_index >= first_idx_to_query { // Extract the references to the correct masp sections - let masp_section_refs = + let masp_refs = MaspTxBatchRefsAttr::read_from_event_attributes( &event.attributes, ) - .ok(); - let ibc_tx_data_refs = - IbcMaspTxBatchRefsAttr::read_from_event_attributes( - &event.attributes, - ) - .ok(); + .unwrap_or_default(); - Some((tx_index, masp_section_refs, ibc_tx_data_refs)) + Some((tx_index, masp_refs)) } else { None } }) .collect::>() - })) + }) + .unwrap_or_default()) } /// An implementation of a shielded wallet diff --git a/crates/sdk/src/masp/utilities.rs b/crates/sdk/src/masp/utilities.rs index 0cf563e578..a794c0da4c 100644 --- a/crates/sdk/src/masp/utilities.rs +++ b/crates/sdk/src/masp/utilities.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use borsh::BorshDeserialize; use masp_primitives::merkle_tree::{CommitmentTree, IncrementalWitness}; use masp_primitives::sapling::Node; +use masp_primitives::transaction::Transaction as MaspTx; use namada_core::chain::BlockHeight; use namada_core::collections::HashMap; use namada_core::storage::TxIndex; @@ -17,10 +18,7 @@ use namada_tx::{IndexedTx, Tx}; use tokio::sync::Semaphore; use crate::error::{Error, QueryError}; -use crate::masp::{ - extract_masp_tx, extract_masp_tx_from_ibc_message, - get_indexed_masp_events_at_height, -}; +use crate::masp::{extract_masp_tx, get_indexed_masp_events_at_height}; struct LedgerMaspClientInner { client: C, @@ -82,11 +80,9 @@ impl MaspClient for LedgerMaspClient { .await }; - let txs_results = match maybe_txs_results.await? { - Some(events) => events, - None => { - continue; - } + let txs_results = maybe_txs_results.await?; + if txs_results.is_empty() { + continue; }; let block = { @@ -109,28 +105,13 @@ impl MaspClient for LedgerMaspClient { .data }; - for (idx, masp_sections_refs, ibc_tx_data_refs) in txs_results { + for (idx, masp_refs) in txs_results { let tx = Tx::try_from(block[idx.0 as usize].as_ref()) .map_err(|e| Error::Other(e.to_string()))?; - let mut extracted_masp_txs = vec![]; - if let Some(masp_sections_refs) = masp_sections_refs { - extracted_masp_txs.extend( - extract_masp_tx(&tx, &masp_sections_refs) - .map_err(|e| Error::Other(e.to_string()))?, - ); - }; - if ibc_tx_data_refs.is_some() { - extracted_masp_txs - .extend(extract_masp_tx_from_ibc_message(&tx)?); - } + let extracted_masp_txs = extract_masp_tx(&tx, &masp_refs) + .map_err(|e| Error::Other(e.to_string()))?; - txs.push(( - IndexedTx { - height: height.into(), - index: idx, - }, - extracted_masp_txs, - )); + index_txs(&mut txs, extracted_masp_txs, height.into(), idx)?; } } @@ -488,8 +469,6 @@ impl MaspClient for IndexerMaspClient { let mut extracted_masp_txs = Vec::with_capacity(batch.len()); for TransactionSlot { bytes } in batch { - type MaspTx = masp_primitives::transaction::Transaction; - extracted_masp_txs.push( MaspTx::try_from_slice(&bytes).map_err(|err| { Error::Other(format!( @@ -501,13 +480,12 @@ impl MaspClient for IndexerMaspClient { ); } - txs.push(( - IndexedTx { - height: BlockHeight(block_height), - index: TxIndex(block_index), - }, + index_txs( + &mut txs, extracted_masp_txs, - )); + block_height.into(), + block_index.into(), + )?; } } @@ -577,6 +555,7 @@ impl MaspClient for IndexerMaspClient { struct Note { // masp_tx_index: u64, note_position: usize, + batch_index: u32, block_index: u32, block_height: u64, } @@ -619,6 +598,7 @@ impl MaspClient for IndexerMaspClient { .map( |Note { block_index, + batch_index, block_height, note_position, }| { @@ -626,6 +606,7 @@ impl MaspClient for IndexerMaspClient { IndexedTx { index: TxIndex(block_index), height: BlockHeight(block_height), + batch_index: Some(batch_index), }, note_position, ) @@ -695,6 +676,36 @@ impl MaspClient for IndexerMaspClient { ) } } + +#[allow(clippy::result_large_err)] +fn index_txs( + txs: &mut Vec<(IndexedTx, MaspTx)>, + extracted_masp_txs: impl IntoIterator, + height: BlockHeight, + index: TxIndex, +) -> Result<(), Error> { + // Note that the index of the extracted MASP transaction does + // not necessarely match the index of the inner tx in the batch, + // we are only interested in giving a sequential ordering to the + // data + for (batch_index, transaction) in extracted_masp_txs.into_iter().enumerate() + { + txs.push(( + IndexedTx { + height, + index, + batch_index: Some( + u32::try_from(batch_index) + .map_err(|e| Error::Other(e.to_string()))?, + ), + }, + transaction, + )); + } + + Ok(()) +} + #[derive(Copy, Clone)] #[allow(clippy::enum_variant_names)] enum BlockIndex { diff --git a/crates/shielded_token/src/lib.rs b/crates/shielded_token/src/lib.rs index e337d7faad..c4512a3035 100644 --- a/crates/shielded_token/src/lib.rs +++ b/crates/shielded_token/src/lib.rs @@ -32,7 +32,7 @@ use std::str::FromStr; pub use masp_primitives::transaction::Transaction as MaspTransaction; use namada_core::borsh::{BorshDeserialize, BorshSchema, BorshSerialize}; pub use namada_core::dec::Dec; -pub use namada_core::masp::{MaspEpoch, MaspTxId, MaspTxRefs, MaspValue}; +pub use namada_core::masp::{MaspEpoch, MaspTxId, MaspValue}; pub use namada_state::{ ConversionLeaf, ConversionState, Error, Key, OptionExt, Result, ResultExt, StorageRead, StorageWrite, WithConversionState, diff --git a/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs b/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs index 27675a3838..68fe08a8a7 100644 --- a/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs +++ b/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs @@ -617,8 +617,8 @@ where .fetched_tracker .set_upper_limit(number_of_fetches); - for (itx, txs) in self.cache.fetched.iter() { - self.spawn_trial_decryptions(*itx, txs); + for (itx, tx) in self.cache.fetched.iter() { + self.spawn_trial_decryptions(*itx, tx); } } @@ -792,24 +792,22 @@ where spawned_tasks } - fn spawn_trial_decryptions(&self, itx: IndexedTx, txs: &[Transaction]) { - for tx in txs { - for (vk, vk_height) in self.ctx.vk_heights.iter() { - let key_is_outdated = vk_height.as_ref() < Some(&itx); - let cached = self.cache.trial_decrypted.get(&itx, vk).is_some(); - - if key_is_outdated && !cached { - let tx = tx.clone(); - let vk = *vk; - - self.spawn_sync(move |interrupt| { - Message::TrialDecrypt( - itx, - vk, - trial_decrypt(tx, vk, || interrupt.get()), - ) - }) - } + fn spawn_trial_decryptions(&self, itx: IndexedTx, tx: &Transaction) { + for (vk, vk_height) in self.ctx.vk_heights.iter() { + let key_is_outdated = vk_height.as_ref() < Some(&itx); + let cached = self.cache.trial_decrypted.get(&itx, vk).is_some(); + + if key_is_outdated && !cached { + let tx = tx.clone(); + let vk = *vk; + + self.spawn_sync(move |interrupt| { + Message::TrialDecrypt( + itx, + vk, + trial_decrypt(tx, vk, || interrupt.get()), + ) + }) } } } @@ -913,8 +911,9 @@ mod dispatcher_tests { let itx = IndexedTx { height: h.into(), index: Default::default(), + batch_index: None, }; - dispatcher.cache.fetched.insert((itx, vec![])); + dispatcher.cache.fetched.insert((itx, arbitrary_masp_tx())); dispatcher.ctx.note_index.insert(itx, h as usize); dispatcher.cache.trial_decrypted.insert( itx, @@ -1118,6 +1117,7 @@ mod dispatcher_tests { *shielded_ctx.vk_heights.get_mut(&vk).unwrap() = Some(IndexedTx { height: 6.into(), index: TxIndex(0), + batch_index: None, }); // the min height should now be 6 @@ -1197,8 +1197,9 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(1), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ))) .expect("Test failed"); masp_tx_sender @@ -1206,8 +1207,9 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(2), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ))) .expect("Test failed"); config.retry_strategy = RetryStrategy::Times(1); @@ -1224,10 +1226,12 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(1), + batch_index: None, }, IndexedTx { height: 1.into(), index: TxIndex(2), + batch_index: None, }, ]); @@ -1273,8 +1277,9 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(1), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ))) .expect("Test failed"); masp_tx_sender @@ -1282,8 +1287,9 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(2), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ))) .expect("Test failed"); masp_tx_sender.send(None).expect("Test failed"); @@ -1303,15 +1309,17 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(1), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ), ( IndexedTx { height: 1.into(), index: TxIndex(2), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ), ]); assert_eq!(cache.fetched.txs, expected); @@ -1351,8 +1359,9 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(1), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ))) .expect("Test failed"); @@ -1397,8 +1406,9 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(1), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ))) .expect("Test failed"); let (_shutdown_send, shutdown_sig) = shutdown_signal(); @@ -1443,8 +1453,9 @@ mod dispatcher_tests { IndexedTx { height: 1.into(), index: TxIndex(1), + batch_index: None, }, - vec![masp_tx.clone()], + masp_tx.clone(), ))) .expect("Test failed"); shielded_ctx diff --git a/crates/shielded_token/src/masp/shielded_sync/utils.rs b/crates/shielded_token/src/masp/shielded_sync/utils.rs index 138700da6b..3d2c4e5d47 100644 --- a/crates/shielded_token/src/masp/shielded_sync/utils.rs +++ b/crates/shielded_token/src/masp/shielded_sync/utils.rs @@ -11,13 +11,13 @@ use namada_core::collections::HashMap; use namada_tx::{IndexedTx, IndexedTxRange}; /// Type alias for convenience and profit -pub type IndexedNoteData = BTreeMap>; +pub type IndexedNoteData = BTreeMap; /// Type alias for the entries of [`IndexedNoteData`] iterators -pub type IndexedNoteEntry = (IndexedTx, Vec); +pub type IndexedNoteEntry = (IndexedTx, Transaction); /// Borrowed version of an [`IndexedNoteEntry`] -pub type IndexedNoteEntryRefs<'a> = (&'a IndexedTx, &'a Vec); +pub type IndexedNoteEntryRefs<'a> = (&'a IndexedTx, &'a Transaction); /// Type alias for a successful note decryption. pub type DecryptedData = (Note, PaymentAddress, MemoBytes); @@ -315,6 +315,7 @@ mod test_blocks_left_to_fetch { use proptest::prelude::*; use super::*; + use crate::masp::test_utils::arbitrary_masp_tx; struct ArbRange { max_from: u64, @@ -333,6 +334,8 @@ mod test_blocks_left_to_fetch { fn fetched_cache_with_blocks( blocks_in_cache: impl IntoIterator, ) -> Fetched { + let masp_tx = arbitrary_masp_tx(); + let txs = blocks_in_cache .into_iter() .map(|height| { @@ -340,8 +343,9 @@ mod test_blocks_left_to_fetch { IndexedTx { height, index: TxIndex(0), + batch_index: None, }, - vec![], + masp_tx.clone(), ) }) .collect(); diff --git a/crates/shielded_token/src/masp/shielded_wallet.rs b/crates/shielded_token/src/masp/shielded_wallet.rs index 78c5d49782..470e197995 100644 --- a/crates/shielded_token/src/masp/shielded_wallet.rs +++ b/crates/shielded_token/src/masp/shielded_wallet.rs @@ -148,33 +148,32 @@ impl ShieldedWallet { pub(crate) fn update_witness_map( &mut self, indexed_tx: IndexedTx, - shielded: &[Transaction], + shielded: &Transaction, ) -> Result<(), eyre::Error> { let mut note_pos = self.tree.size(); self.note_index.insert(indexed_tx, note_pos); - for tx in shielded { - for so in - tx.sapling_bundle().map_or(&vec![], |x| &x.shielded_outputs) - { - // Create merkle tree leaf node from note commitment - let node = Node::new(so.cmu.to_repr()); - // Update each merkle tree in the witness map with the latest - // addition - for (_, witness) in self.witness_map.iter_mut() { - witness.append(node).map_err(|()| { - eyre!("note commitment tree is full".to_string()) - })?; - } - self.tree.append(node).map_err(|()| { + for so in shielded + .sapling_bundle() + .map_or(&vec![], |x| &x.shielded_outputs) + { + // Create merkle tree leaf node from note commitment + let node = Node::new(so.cmu.to_repr()); + // Update each merkle tree in the witness map with the latest + // addition + for (_, witness) in self.witness_map.iter_mut() { + witness.append(node).map_err(|()| { eyre!("note commitment tree is full".to_string()) })?; - // Finally, make it easier to construct merkle paths to this new - // note - let witness = IncrementalWitness::::from_tree(&self.tree); - self.witness_map.insert(note_pos, witness); - note_pos = checked!(note_pos + 1).unwrap(); } + self.tree.append(node).map_err(|()| { + eyre!("note commitment tree is full".to_string()) + })?; + // Finally, make it easier to construct merkle paths to this new + // note + let witness = IncrementalWitness::::from_tree(&self.tree); + self.witness_map.insert(note_pos, witness); + note_pos = checked!(note_pos + 1).unwrap(); } Ok(()) } @@ -255,16 +254,15 @@ impl ShieldedWallet { } #[allow(missing_docs)] - pub fn save_shielded_spends(&mut self, transactions: &[Transaction]) { - for stx in transactions { - for ss in - stx.sapling_bundle().map_or(&vec![], |x| &x.shielded_spends) - { - // If the shielded spend's nullifier is in our map, then target - // note is rendered unusable - if let Some(note_pos) = self.nf_map.get(&ss.nullifier) { - self.spents.insert(*note_pos); - } + pub fn save_shielded_spends(&mut self, transaction: &Transaction) { + for ss in transaction + .sapling_bundle() + .map_or(&vec![], |x| &x.shielded_spends) + { + // If the shielded spend's nullifier is in our map, then target + // note is rendered unusable + if let Some(note_pos) = self.nf_map.get(&ss.nullifier) { + self.spents.insert(*note_pos); } } } @@ -363,9 +361,9 @@ impl ShieldedWallet { /// tree async fn pre_cache_transaction( &mut self, - masp_txs: &[Transaction], + masp_tx: &Transaction, ) -> Result<(), eyre::Error> { - self.save_shielded_spends(masp_txs); + self.save_shielded_spends(masp_tx); // Save the speculative state for future usage self.sync_status = ContextSyncStatus::Speculative; @@ -1089,7 +1087,7 @@ pub trait ShieldedApi: .map_err(|error| TransferErr::Build { error, data: None })?; if update_ctx { - self.pre_cache_transaction(std::slice::from_ref(&masp_tx)) + self.pre_cache_transaction(&masp_tx) .await .map_err(|e| TransferErr::General(e.to_string()))?; } diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 4bc8292bc1..07690d1e0e 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -3,13 +3,15 @@ use std::str::FromStr; use color_eyre::eyre::Result; use color_eyre::owo_colors::OwoColorize; -use namada_apps_lib::wallet::defaults::christel_keypair; +use namada_apps_lib::wallet::defaults::{albert_keypair, christel_keypair}; use namada_core::dec::Dec; use namada_core::masp::TokenMap; use namada_node::shell::testing::client::run; use namada_node::shell::testing::node::NodeResults; use namada_node::shell::testing::utils::{Bin, CapturedOutput}; +use namada_sdk::account::AccountPublicKeysMap; use namada_sdk::masp::fs::FsShieldedUtils; +use namada_sdk::signing::SigningTxData; use namada_sdk::state::{StorageRead, StorageWrite}; use namada_sdk::time::DateTimeUtc; use namada_sdk::token::storage_key::masp_token_map_key; @@ -3499,3 +3501,260 @@ fn masp_fee_payment_with_different_token() -> Result<()> { Ok(()) } + +// An ouput description of the masp can be replayed (pushed to the commitment +// tree more than once). The nullifiers and merkle paths will be unique. Test +// that a batch containing two identical shielding txs can be executed correctly +// and the two identical notes can be spent with no issues. +#[test] +fn identical_output_descriptions() -> Result<()> { + // This address doesn't matter for tests. But an argument is required. + let validator_one_rpc = "http://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_masp_epoch(); + let tempdir = tempfile::tempdir().unwrap(); + + // Add the relevant viewing keys to the wallet otherwise the shielded + // context won't precache the masp data + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_a", + "--value", + AA_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + + // Generate a tx to shield some tokens + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT_KEY, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "1000", + "--gas-limit", + "300000", + "--gas-payer", + ALBERT_KEY, + "--output-folder-path", + tempdir.path().to_str().unwrap(), + "--dump-tx", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + let file_path = tempdir + .path() + .read_dir() + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + let tx_bytes = std::fs::read(&file_path).unwrap(); + std::fs::remove_file(&file_path).unwrap(); + + // Create a batch that contains the same shielding tx twice + let tx: namada_sdk::tx::Tx = serde_json::from_slice(&tx_bytes).unwrap(); + // Inject some randomness in the cloned tx to chagne the hash + let mut tx_clone = tx.clone(); + let mut cmt = tx_clone.header.batch.first().unwrap().to_owned(); + let random_hash: Vec<_> = (0..namada_sdk::hash::HASH_LENGTH) + .map(|_| rand::random::()) + .collect(); + cmt.memo_hash = namada_sdk::hash::Hash(random_hash.try_into().unwrap()); + tx_clone.header.batch.clear(); + tx_clone.header.batch.insert(cmt); + + let signing_data = SigningTxData { + owner: None, + public_keys: vec![albert_keypair().to_public()], + threshold: 1, + account_public_keys_map: None, + fee_payer: albert_keypair().to_public(), + }; + + let (mut batched_tx, _signing_data) = namada_sdk::tx::build_batch(vec![ + (tx, signing_data.clone()), + (tx_clone, signing_data), + ]) + .unwrap(); + + batched_tx.sign_raw( + vec![albert_keypair()], + AccountPublicKeysMap::from_iter( + vec![(albert_keypair().to_public())].into_iter(), + ), + None, + ); + batched_tx.sign_wrapper(albert_keypair()); + + let wrapper_hash = batched_tx.wrapper_hash(); + let inner_cmts = batched_tx.commitments(); + + let txs = vec![batched_tx.to_bytes()]; + + node.clear_results(); + node.submit_txs(txs); + + // Check that the batch was successful + { + let codes = node.tx_result_codes.lock().unwrap(); + // If empty than failed in process proposal + assert!(!codes.is_empty()); + + for code in codes.iter() { + assert!(matches!(code, NodeResults::Ok)); + } + + let results = node.tx_results.lock().unwrap(); + // We submitted a single batch + assert_eq!(results.len(), 1); + + for result in results.iter() { + // The batch should contain a two inner tx + assert_eq!(result.0.len(), 2); + + for inner_cmt in inner_cmts { + let inner_tx_result = result + .get_inner_tx_result( + wrapper_hash.as_ref(), + itertools::Either::Right(inner_cmt), + ) + .expect("Missing expected tx result") + .as_ref() + .expect("Result is supposed to be Ok"); + + assert!(inner_tx_result.is_accepted()); + } + } + } + + // sync the shielded context + run( + &node, + Bin::Client, + vec![ + "shielded-sync", + "--viewing-keys", + AA_VIEWING_KEY, + "--node", + validator_one_rpc, + ], + )?; + node.assert_success(); + + // Assert NAM balance at VK(A) is 2000 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 2000")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + CHRISTEL, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 2000000")); + + // Spend both notes successfully + run( + &node, + Bin::Client, + vec![ + "unshield", + "--source", + A_SPENDING_KEY, + "--target", + CHRISTEL, + "--token", + NAM, + // Spend the entire shielded amount + "--amount", + "2000", + "--gas-payer", + BERTHA_KEY, + "--node", + validator_one_rpc, + ], + )?; + node.assert_success(); + + // Assert NAM balance at VK(A) is 0 + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + AA_VIEWING_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + CHRISTEL, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 2002000")); + + Ok(()) +} diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index 3299924cdc..364e7f4af9 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -22,9 +22,8 @@ use namada_core::borsh::{ BorshDeserialize, BorshSchema, BorshSerialize, BorshSerializeExt, }; use namada_core::hash::Hash; -use namada_core::ibc::IbcTxDataRefs; -use namada_core::masp::MaspTxRefs; use namada_core::storage; +use namada_events::extend::MaspTxRefs; use namada_events::Event; use namada_gas::WholeGas; use namada_macros::BorshDeserializer; @@ -189,10 +188,14 @@ pub fn compute_inner_tx_hash( pub struct ExtendedTxResult { /// The transaction result pub tx_result: TxResult, - /// The optional references to masp sections + /// The optional references to masp data (either MASP sections or tx Data + /// for shielded actions) + // NOTE: it's paramount to enforce a single, ordered collection for all the + // masp transactions to ensure that the exact view on the tx sequence is + // preserved in the events. Also, it is possible for two refs to be exactly + // the same, we must make sure to emit events for both so that the + // client/indexer can properly construct their internal state pub masp_tx_refs: MaspTxRefs, - /// The optional data section hashes of IBC transaction - pub ibc_tx_data_refs: IbcTxDataRefs, } impl Default for ExtendedTxResult { @@ -200,7 +203,6 @@ impl Default for ExtendedTxResult { Self { tx_result: Default::default(), masp_tx_refs: Default::default(), - ibc_tx_data_refs: Default::default(), } } } @@ -307,21 +309,11 @@ impl TxResult { /// Converts this result to [`ExtendedTxResult`] pub fn to_extended_result( self, - masp_section_refs: Option>, + masp_tx_refs: Option, ) -> ExtendedTxResult { - let (masp_tx_refs, ibc_tx_data_refs) = match masp_section_refs { - Some(Either::Left(masp_tx_refs)) => { - (masp_tx_refs, Default::default()) - } - Some(Either::Right(ibc_tx_data_refs)) => { - (Default::default(), ibc_tx_data_refs) - } - None => (Default::default(), Default::default()), - }; ExtendedTxResult { tx_result: self, - masp_tx_refs, - ibc_tx_data_refs, + masp_tx_refs: masp_tx_refs.unwrap_or_default(), } } } diff --git a/crates/tx/src/types.rs b/crates/tx/src/types.rs index f2dbdab784..156f62c211 100644 --- a/crates/tx/src/types.rs +++ b/crates/tx/src/types.rs @@ -1856,7 +1856,8 @@ impl<'tx> Tx { } /// Represents the pointers to a indexed tx, which are the block height and the -/// index inside that block +/// index inside that block. Optionally points to a specific inner tx inside a +/// batch if such level of granularity is required. #[derive( Debug, Copy, @@ -1875,6 +1876,8 @@ pub struct IndexedTx { pub height: BlockHeight, /// The index in the block of the tx pub index: TxIndex, + /// The optional index of an inner tx within this batc + pub batch_index: Option, } impl IndexedTx { @@ -1884,6 +1887,7 @@ impl IndexedTx { Self { height, index: TxIndex(u32::MAX), + batch_index: None, } } } @@ -1893,6 +1897,7 @@ impl Default for IndexedTx { Self { height: BlockHeight::first(), index: TxIndex(0), + batch_index: None, } } } @@ -1916,10 +1921,12 @@ impl IndexedTxRange { IndexedTx { height: from, index: TxIndex(0), + batch_index: None, }, IndexedTx { height: to, index: TxIndex(u32::MAX), + batch_index: None, }, ) }