from_namada_tx returns error instead of silently ignoring wrong mas…

…p txs

chain/src/main.rs

@@ -13,7 +13,7 @@ use shared::error::{IntoMainError, MainError};
 use shared::height::{BlockHeight, FollowingHeights};
 use shared::indexed_tx::IndexedTx;
 use shared::transaction::Transaction;
-use shared::tx_index::TxIndex;
+use shared::tx_index::{MaspTxIndex, TxIndex};
 use tendermint_rpc::HttpClient;
 use tokio::signal;
 use tokio_retry::strategy::{jitter, FixedInterval};
@@ -218,13 +218,11 @@ async fn build_and_commit_masp_data_at_height(
     for (idx, Transaction { masp_txs, .. }) in
         block_data.transactions.into_iter()
     {
-        for (masp_tx_index, masp_tx) in masp_txs {
-            // TODO: handle fee unshielding
-
+        for (masp_tx_index, masp_tx) in masp_txs.into_iter().enumerate() {
             let indexed_tx = IndexedTx {
                 block_height,
                 block_index: TxIndex(idx as u32),
-                masp_tx_index,
+                masp_tx_index: MaspTxIndex(masp_tx_index),
             };
 
             update_witness_map(

chain/src/services/cometbft.rs

@@ -1,4 +1,4 @@
-use anyhow::Context;
+use anyhow::{anyhow, Context};
 use shared::block::Block;
 use shared::height::BlockHeight;
 use tendermint_rpc::endpoint::{block, block_results};
@@ -13,7 +13,7 @@ pub async fn query_masp_txs_in_block(
         query_raw_block_results_at_height(client, height),
     )?;
 
-    Ok(Block::new(raw_block, raw_block_results))
+    Block::new(raw_block, raw_block_results).map_err(|err| anyhow!(err))
 }
 
 async fn query_raw_block(

shared/src/block.rs

@@ -19,7 +19,7 @@ impl Block {
     pub fn new(
         raw_block: block::Response,
         raw_results: block_results::Response,
-    ) -> Self {
+    ) -> Result<Self, String> {
         let indexed_masp_txs = locate_masp_txs(&raw_results);
 
         let mut block = Block {
@@ -35,23 +35,12 @@ impl Block {
         {
             let block_index = tx_index.0 as usize;
             let tx_bytes = &raw_block.block.data[block_index];
-
-            let tx = match Transaction::from_namada_tx(tx_bytes, &masp_refs.0) {
-                Some(tx) => tx,
-                None => {
-                    tracing::warn!(
-                        block_hash = %block.hash,
-                        block_index,
-                        "Invalid Namada transaction in block"
-                    );
-                    continue;
-                }
-            };
+            let tx = Transaction::from_namada_tx(tx_bytes, &masp_refs.0)?;
 
             block.transactions.push((block_index, tx));
         }
 
-        block
+        Ok(block)
     }
 }
 

shared/src/transaction.rs

@@ -1,89 +1,60 @@
 use std::borrow::Cow;
 use std::fmt::Display;
 
-use namada_core::borsh::BorshDeserialize;
-use namada_core::collections::HashMap;
 use namada_core::hash::Hash;
-use namada_core::masp::MaspTxId;
 use namada_core::masp_primitives::transaction::Transaction as NamadaMaspTransaction;
 use namada_sdk::events::extend::MaspTxRef;
 use namada_sdk::token::Transfer;
-use namada_tx::{Data, Section, Tx as NamadaTx, TxCommitments};
+use namada_tx::{Data, Section, Tx as NamadaTx};
 
 use crate::id::Id;
-use crate::tx_index::MaspTxIndex;
 
 #[derive(Debug, Clone)]
 pub struct Transaction {
     pub hash: Id,
-    pub masp_txs: Vec<(MaspTxIndex, NamadaMaspTransaction)>,
+    pub masp_txs: Vec<NamadaMaspTransaction>,
 }
 
 impl Transaction {
     pub fn from_namada_tx(
         nam_tx_bytes: &[u8],
         valid_masp_tx_refs: &[MaspTxRef],
-    ) -> Option<Self> {
-        let transaction = NamadaTx::try_from(nam_tx_bytes)
-            .map_err(|e| e.to_string())
-            .ok()?;
+    ) -> Result<Self, String> {
+        let transaction =
+            NamadaTx::try_from(nam_tx_bytes).map_err(|e| e.to_string())?;
         let transaction_id = transaction.header_hash();
 
-        let all_masp_txs: HashMap<_, _> = transaction
-            .header
-            .batch
-            .iter()
-            .enumerate()
-            .filter_map(|(masp_tx_index, cmt)| {
-                let masp_tx_id = get_shielded_tx_id(&transaction, cmt)?;
-                Some((masp_tx_id, MaspTxIndex(masp_tx_index)))
-            })
-            .collect();
-
-        let masp_txs = valid_masp_tx_refs
-            .iter()
-            .filter_map(|masp_tx_ref| {
-                let masp_tx = match masp_tx_ref {
+        let masp_txs = valid_masp_tx_refs.iter().try_fold(
+            vec![],
+            |mut acc, masp_tx_ref| {
+                let masp_tx = match &masp_tx_ref {
                     MaspTxRef::MaspSection(masp_tx_id) => {
-                        let Some(masp_tx) = transaction.get_masp_section(masp_tx_id)
-                        else {
-                            tracing::warn!(
-                                %transaction_id,
-                                ?masp_tx_id,
-                                "Shielded tx not found in Namada transaction"
-                            );
-                            return None;
-                        };
+                        let masp_tx = transaction
+                            .get_masp_section(masp_tx_id)
+                            .ok_or_else(|| {
+                                "Missing expected masp section with id: {id}"
+                                    .to_string()
+                            })?;
                         Cow::Borrowed(masp_tx)
                     }
                     MaspTxRef::IbcData(sechash) => {
-                        let Some(masp_tx) = get_masp_tx_from_ibc_data(&transaction, sechash) else {
-                            tracing::warn!(
-                                %transaction_id,
-                                ibc_data_sechash = ?sechash,
-                                "IBC shielding tx not found in Namada transaction"
-                            );
-                            return None;
-                        };
+                        let masp_tx =
+                            get_masp_tx_from_ibc_data(&transaction, sechash)
+                                .ok_or_else(|| {
+                                    "Missing expected data section with hash: \
+                                     {sechash}"
+                                        .to_string()
+                                })?;
                         Cow::Owned(masp_tx)
                     }
                 };
 
-                let masp_tx_index =
-                    all_masp_txs.get(&MaspTxId::from(masp_tx.txid())).cloned().or_else(|| {
-                        tracing::warn!(
-                            %transaction_id,
-                            ?masp_tx,
-                            "Shielded tx not found in Namada transaction"
-                        );
-                        None
-                    })?;
-
-                Some((masp_tx_index, masp_tx.into_owned()))
-            })
-            .collect();
+                acc.push(masp_tx.into_owned());
+                Result::<_, String>::Ok(acc)
+            },
+        )?;
 
-        Some(Transaction {
+        Ok(Transaction {
             masp_txs,
             hash: Id::from(transaction_id),
         })
@@ -96,21 +67,6 @@ impl Display for Transaction {
     }
 }
 
-fn get_shielded_tx_id(
-    transaction: &NamadaTx,
-    cmt: &TxCommitments,
-) -> Option<MaspTxId> {
-    let tx_data = get_namada_tx_data(transaction, &cmt.data_hash)?;
-
-    Transfer::try_from_slice(tx_data)
-        .ok()
-        .and_then(|tx| tx.shielded_section_hash)
-        .or_else(|| {
-            get_masp_tx_from_ibc_data(transaction, &cmt.data_hash)
-                .map(|tx| MaspTxId::from(tx.txid()))
-        })
-}
-
 fn get_masp_tx_from_ibc_data(
     transaction: &NamadaTx,
     data_sechash: &Hash,