diff --git a/.changelog/v0.40.0/bug-fixes/3488-fix-ibc-masp-tx-extraction.md b/.changelog/v0.40.0/bug-fixes/3488-fix-ibc-masp-tx-extraction.md new file mode 100644 index 0000000000..413db843d5 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/core/src/ibc.rs b/crates/core/src/ibc.rs index fe29f61fe7..4f88c57c89 100644 --- a/crates/core/src/ibc.rs +++ b/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); + +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) + } +} diff --git a/crates/events/src/extend.rs b/crates/events/src/extend.rs index 2ebf5ed5e3..83a9834c2e 100644 --- a/crates/events/src/extend.rs +++ b/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 IbcTxBatchRefs(pub IbcTxDataRefs); + +impl EventAttributeEntry<'static> for IbcTxBatchRefs { + type Value = IbcTxDataRefs; + type ValueOwned = Self::Value; + + const KEY: &'static str = "ibc_tx_batch_refs"; + + fn into_value(self) -> Self::Value { + self.0 + } +} + /// Extend an [`Event`] with success data. pub struct Success(pub bool); diff --git a/crates/namada/src/ledger/mod.rs b/crates/namada/src/ledger/mod.rs index 71304bc1e8..e358965210 100644 --- a/crates/namada/src/ledger/mod.rs +++ b/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, diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 97616f3b34..b6ac832058 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/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, @@ -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, @@ -342,9 +344,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 { 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(); @@ -369,9 +372,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, @@ -424,9 +431,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 namada_tx::action::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(); @@ -447,6 +459,12 @@ where Ok(extended_tx_result) } +/// Transaction result for masp transfer +pub struct MaspTxResult { + tx_result: BatchedTxResult, + masp_section_ref: Either, +} + /// Performs the required operation on a wrapper transaction: /// - replay protection /// - fee payment @@ -487,18 +505,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( || (BatchResults::default(), None), - |(batched_result, masp_section_ref)| { + |masp_tx_result| { let mut batch = BatchResults::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) }, ); @@ -512,7 +535,7 @@ where gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(), batch_results, } - .to_extended_result(masp_tx_refs)) + .to_extended_result(masp_section_refs)) } /// Perform the actual transfer of fees from the fee payer to the block @@ -524,7 +547,7 @@ pub fn transfer_fee( tx: &Tx, wrapper: &WrapperTx, tx_index: &TxIndex, -) -> Result> +) -> Result> where S: State + StorageRead @@ -677,7 +700,7 @@ fn try_masp_fee_payment( }: &mut ShellParams<'_, S, D, H, CA>, tx: &Tx, tx_index: &TxIndex, -) -> Result> +) -> Result> where S: State + StorageRead @@ -713,9 +736,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, @@ -740,16 +765,20 @@ where ); } - let masp_ref = namada_tx::action::get_masp_section_ref(*state) - .map_err(Error::StateError)?; + let masp_section_ref = + match namada_tx::action::get_masp_section_ref(*state) + .map_err(Error::StateError)? + { + Some(masp_tx_id) => Either::Left(masp_tx_id), + None => Either::Right(*first_tx.cmt.data_sechash()), + }; // 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)) + .then_some(MaspTxResult { + tx_result: result, + masp_section_ref, }) - .flatten() } Err(e) => { state.write_log_mut().drop_tx(); @@ -807,7 +836,7 @@ pub fn check_fees( shell_params: &mut ShellParams<'_, S, D, H, CA>, tx: &Tx, wrapper: &WrapperTx, -) -> Result> +) -> Result> where S: State + StorageRead @@ -877,7 +906,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 @@ -895,7 +924,7 @@ where } = shell_params; let verifiers = execute_tx( - &batched_tx, + batched_tx, tx_index, state, tx_gas_meter, @@ -904,7 +933,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(), diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index c32ab0d12c..20fa39460b 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/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, IbcTxBatchRefs, Info, MaspTxBatchRefs, + MaspTxBlockIndex, TxHash, }; use namada::ledger::events::EmitEvents; use namada::ledger::gas::GasMetering; @@ -1056,9 +1057,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(IbcTxBatchRefs( + extended_tx_result.ibc_tx_data_refs.clone(), + )); } flags diff --git a/crates/sdk/src/masp.rs b/crates/sdk/src/masp.rs index bcf28db17b..a943fbf9b0 100644 --- a/crates/sdk/src/masp.rs +++ b/crates/sdk/src/masp.rs @@ -39,6 +39,7 @@ use namada_core::address::Address; use namada_core::arith::CheckedAdd; use namada_core::collections::{HashMap, HashSet}; use namada_core::dec::Dec; +use namada_core::ibc::IbcTxDataRefs; pub use namada_core::masp::{ encode_asset_type, AssetData, BalanceOwner, ExtendedViewingKey, PaymentAddress, TAddrData, TransferSource, TransferTarget, TxId, @@ -48,6 +49,7 @@ use namada_core::storage::{BlockHeight, TxIndex}; use namada_core::time::DateTimeUtc; use namada_core::uint::Uint; use namada_events::extend::{ + IbcTxBatchRefs as IbcTxBatchRefsAttr, MaspTxBatchRefs as MaspTxBatchRefsAttr, MaspTxBlockIndex as MaspTxBlockIndexAttr, ReadFromEventAttributes, }; @@ -654,15 +656,23 @@ impl ShieldedContext { .block .data; - for (idx, masp_sections_refs) in txs_results { + for (idx, masp_sections_refs, ibc_tx_data_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 extracted_masp_txs = - if let Some(masp_sections_refs) = masp_sections_refs { - Self::extract_masp_tx(&tx, &masp_sections_refs).await? - } else { - Self::extract_masp_tx_from_ibc_message(&tx)? - }; + let mut extracted_masp_txs = vec![]; + if let Some(masp_sections_refs) = masp_sections_refs { + extracted_masp_txs.extend( + Self::extract_masp_tx(&tx, &masp_sections_refs).await?, + ); + } + if let Some(ibc_tx_data_refs) = ibc_tx_data_refs { + extracted_masp_txs.extend( + Self::extract_masp_tx_from_ibc_message( + &tx, + ibc_tx_data_refs, + )?, + ); + } // Collect the current transactions shielded_txs.insert( IndexedTx { @@ -709,10 +719,11 @@ impl ShieldedContext { /// Extract the relevant shield portions from the IBC messages in [`Tx`] fn extract_masp_tx_from_ibc_message( tx: &Tx, + ibc_tx_data_refs: IbcTxDataRefs, ) -> Result, Error> { let mut masp_txs = Vec::new(); - for cmt in &tx.header.batch { - let tx_data = tx.data(cmt).ok_or_else(|| { + for section in ibc_tx_data_refs.0 { + let tx_data = tx.get_data_section(§ion).ok_or_else(|| { Error::Other("Missing transaction data".to_string()) })?; let ibc_msg = decode_message(&tx_data) @@ -2363,7 +2374,10 @@ async fn get_indexed_masp_events_at_height( client: &C, height: BlockHeight, first_idx_to_query: Option, -) -> Result)>>, Error> { +) -> Result< + Option, Option)>>, + Error, +> { let first_idx_to_query = first_idx_to_query.unwrap_or_default(); Ok(client @@ -2388,8 +2402,13 @@ async fn get_indexed_masp_events_at_height( &event.attributes, ) .ok(); + let ibc_tx_data_refs = + IbcTxBatchRefsAttr::read_from_event_attributes( + &event.attributes, + ) + .ok(); - Some((tx_index, masp_section_refs)) + Some((tx_index, masp_section_refs, ibc_tx_data_refs)) } else { None } diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index 2cf49ec745..150ecb7139 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -22,6 +22,7 @@ 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::Event; @@ -333,8 +334,8 @@ pub struct ExtendedTxResult { pub tx_result: TxResult, /// The optional references to masp sections pub masp_tx_refs: MaspTxRefs, - /// The flag for IBC shielding transfer - pub is_ibc_shielding: bool, + /// The optional data section hashes of IBC transaction + pub ibc_tx_data_refs: IbcTxDataRefs, } impl Default for ExtendedTxResult { @@ -342,7 +343,7 @@ impl Default for ExtendedTxResult { Self { tx_result: Default::default(), masp_tx_refs: Default::default(), - is_ibc_shielding: Default::default(), + ibc_tx_data_refs: Default::default(), } } } @@ -390,12 +391,21 @@ impl TxResult { /// Converts this result to [`ExtendedTxResult`] pub fn to_extended_result( self, - masp_tx_refs: Option, + masp_section_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: masp_tx_refs.unwrap_or_default(), - is_ibc_shielding: false, + masp_tx_refs, + ibc_tx_data_refs, } } } diff --git a/crates/tx/src/types.rs b/crates/tx/src/types.rs index a8b3198006..ed185b9a4f 100644 --- a/crates/tx/src/types.rs +++ b/crates/tx/src/types.rs @@ -1298,7 +1298,15 @@ impl Tx { /// Get the data designated by the transaction data hash in the header at /// the specified commitment pub fn data(&self, cmt: &TxCommitments) -> Option> { - match self.get_section(&cmt.data_hash).as_ref().map(Cow::as_ref) { + self.get_data_section(&cmt.data_hash) + } + + /// Get the data designated by the transaction data hash + pub fn get_data_section( + &self, + data_hash: &namada_core::hash::Hash, + ) -> Option> { + match self.get_section(data_hash).as_ref().map(Cow::as_ref) { Some(Section::Data(data)) => Some(data.data.clone()), _ => None, }