From 2cabfb8911aa7613f788c7efa1d716a599b599c3 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 20 Sep 2024 14:40:05 +0200 Subject: [PATCH] `from_namada_tx` returns error instead of silently ignoring wrong masp txs --- chain/src/main.rs | 8 ++- chain/src/services/cometbft.rs | 4 +- shared/src/block.rs | 17 ++---- shared/src/transaction.rs | 98 ++++++++++------------------------ 4 files changed, 35 insertions(+), 92 deletions(-) diff --git a/chain/src/main.rs b/chain/src/main.rs index 6077f36..f5d12ad 100644 --- a/chain/src/main.rs +++ b/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( diff --git a/chain/src/services/cometbft.rs b/chain/src/services/cometbft.rs index a958545..55ccce6 100644 --- a/chain/src/services/cometbft.rs +++ b/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( diff --git a/shared/src/block.rs b/shared/src/block.rs index f8c2a05..2e09fa1 100644 --- a/shared/src/block.rs +++ b/shared/src/block.rs @@ -19,7 +19,7 @@ impl Block { pub fn new( raw_block: block::Response, raw_results: block_results::Response, - ) -> Self { + ) -> Result { 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) } } diff --git a/shared/src/transaction.rs b/shared/src/transaction.rs index c2477c5..30ad9a9 100644 --- a/shared/src/transaction.rs +++ b/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, } impl Transaction { pub fn from_namada_tx( nam_tx_bytes: &[u8], valid_masp_tx_refs: &[MaspTxRef], - ) -> Option { - let transaction = NamadaTx::try_from(nam_tx_bytes) - .map_err(|e| e.to_string()) - .ok()?; + ) -> Result { + 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 { - 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,