From 33cab6aa90cb843f8de99ec3aa84d55f3e0c41c5 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 4 Jan 2024 21:29:15 +0100 Subject: [PATCH] WIP fetches masp txs from blocks --- apps/src/lib/client/rpc.rs | 8 +- .../lib/node/ledger/shell/finalize_block.rs | 10 + apps/src/lib/node/ledger/shell/governance.rs | 1 + .../src/lib/node/ledger/shell/testing/node.rs | 6 +- core/src/ledger/ibc/context/storage.rs | 2 + core/src/ledger/ibc/mod.rs | 6 +- core/src/ledger/masp_utils.rs | 21 +- core/src/ledger/vp_env.rs | 1 + core/src/types/ibc.rs | 2 + core/src/types/storage.rs | 21 + core/src/types/token.rs | 14 +- sdk/src/masp.rs | 458 ++++++++++++++---- sdk/src/queries/mod.rs | 7 +- shared/src/ledger/mod.rs | 1 + shared/src/ledger/native_vp/ibc/context.rs | 4 +- shared/src/ledger/native_vp/masp.rs | 46 +- shared/src/vm/host_env.rs | 3 +- tx_prelude/src/ibc.rs | 3 +- wasm/wasm_source/src/tx_transfer.rs | 6 +- 19 files changed, 514 insertions(+), 106 deletions(-) diff --git a/apps/src/lib/client/rpc.rs b/apps/src/lib/client/rpc.rs index bc57f58f14..e0178dd33d 100644 --- a/apps/src/lib/client/rpc.rs +++ b/apps/src/lib/client/rpc.rs @@ -44,7 +44,9 @@ use namada::types::ibc::{is_ibc_denom, IbcTokenHash}; use namada::types::io::Io; use namada::types::key::*; use namada::types::masp::{BalanceOwner, ExtendedViewingKey, PaymentAddress}; -use namada::types::storage::{BlockHeight, BlockResults, Epoch, Key, KeySeg}; +use namada::types::storage::{ + BlockHeight, BlockResults, Epoch, IndexedTx, Key, KeySeg, +}; use namada::types::token::{Change, MaspDenom}; use namada::types::{storage, token}; use namada_sdk::error::{is_pinned_error, Error, PinnedBalanceError}; @@ -145,7 +147,9 @@ pub async fn query_transfers( .map(|fvk| (ExtendedFullViewingKey::from(*fvk).fvk.vk, fvk)) .collect(); // Now display historical shielded and transparent transactions - for ((height, idx), (epoch, tfer_delta, tx_delta)) in transfers { + for (IndexedTx { height, index: idx }, (epoch, tfer_delta, tx_delta)) in + transfers + { // Check if this transfer pertains to the supplied owner let mut relevant = match &query_owner { Either::Left(BalanceOwner::FullViewingKey(fvk)) => tx_delta diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 4683de0be0..8ce68c6ae1 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -470,6 +470,16 @@ where let mut event = Event::from(ibc_event); // Add the height for IBC event query event["height"] = height.to_string(); + if tx_event + .attributes + .contains_key("is_valid_masp_tx") + { + // Add the tx index for masp txs clients + // queries + // FIXME: review this + event["is_valid_masp_tx"] = + tx_index.to_string(); + } event }) // eth bridge events diff --git a/apps/src/lib/node/ledger/shell/governance.rs b/apps/src/lib/node/ledger/shell/governance.rs index 4991310d09..ea64fa0c51 100644 --- a/apps/src/lib/node/ledger/shell/governance.rs +++ b/apps/src/lib/node/ledger/shell/governance.rs @@ -287,6 +287,7 @@ where &mut shell.vp_wasm_cache, &mut shell.tx_wasm_cache, None, + &mut false, ); shell .wl_storage diff --git a/apps/src/lib/node/ledger/shell/testing/node.rs b/apps/src/lib/node/ledger/shell/testing/node.rs index b3ef78027e..5bad5fd829 100644 --- a/apps/src/lib/node/ledger/shell/testing/node.rs +++ b/apps/src/lib/node/ledger/shell/testing/node.rs @@ -761,10 +761,12 @@ impl<'a> Client for &'a MockNode { height: H, ) -> Result where - H: Into + Send, + H: TryInto + Send, { self.drive_mock_services_bg().await; - let height = height.into(); + let height = height.try_into().map_err(|_| { + RpcError::parse("Could not parse block height".to_string()) + })?; let encoded_event = EncodedEvent(height.value()); let locked = self.shell.lock().unwrap(); let events: Vec<_> = locked diff --git a/core/src/ledger/ibc/context/storage.rs b/core/src/ledger/ibc/context/storage.rs index a5e79e6e1f..8ff16cf868 100644 --- a/core/src/ledger/ibc/context/storage.rs +++ b/core/src/ledger/ibc/context/storage.rs @@ -28,9 +28,11 @@ pub trait IbcStorageContext: StorageRead + StorageWrite { ) -> Result<(), Error>; /// Handle masp tx + // FIXME: try again to remove tx_index from some places fn handle_masp_tx( &mut self, shielded: &masp_primitives::transaction::Transaction, + pin_key: Option<&str>, ) -> Result<(), Error>; /// Mint token diff --git a/core/src/ledger/ibc/mod.rs b/core/src/ledger/ibc/mod.rs index 11afd339e1..cd45eb054e 100644 --- a/core/src/ledger/ibc/mod.rs +++ b/core/src/ledger/ibc/mod.rs @@ -102,6 +102,7 @@ where pub fn execute(&mut self, tx_data: &[u8]) -> Result<(), Error> { let message = decode_message(tx_data)?; match &message { + // FIXME: look here for MASP on IBC IbcMessage::Transfer(msg) => { let mut token_transfer_ctx = TokenTransferContext::new(self.ctx.inner.clone()); @@ -282,7 +283,10 @@ where self.ctx .inner .borrow_mut() - .handle_masp_tx(&shielded_transfer.masp_tx) + .handle_masp_tx( + &shielded_transfer.masp_tx, + shielded_transfer.transfer.key.as_deref(), + ) .map_err(|_| { Error::MaspTx("Writing MASP components failed".to_string()) })?; diff --git a/core/src/ledger/masp_utils.rs b/core/src/ledger/masp_utils.rs index 37631e4855..343c7b1c73 100644 --- a/core/src/ledger/masp_utils.rs +++ b/core/src/ledger/masp_utils.rs @@ -6,7 +6,11 @@ use masp_primitives::transaction::Transaction; use super::storage_api::{StorageRead, StorageWrite}; use crate::ledger::storage_api::{Error, Result}; -use crate::types::token::{masp_commitment_tree_key, masp_nullifier_key}; +use crate::types::address::MASP; +use crate::types::storage::{IndexedTx, Key, KeySeg}; +use crate::types::token::{ + masp_commitment_tree_key, masp_nullifier_key, PIN_KEY_PREFIX, +}; // Writes the nullifiers of the provided masp transaction to storage fn reveal_nullifiers( @@ -59,6 +63,7 @@ pub fn update_note_commitment_tree( pub fn handle_masp_tx( ctx: &mut (impl StorageRead + StorageWrite), shielded: &Transaction, + pin_key: Option<&str>, ) -> Result<()> { // TODO: temporarily disabled because of the node aggregation issue in WASM. // Using the host env tx_update_masp_note_commitment_tree or directly the @@ -66,5 +71,19 @@ pub fn handle_masp_tx( // update_note_commitment_tree(ctx, shielded)?; reveal_nullifiers(ctx, shielded)?; + // If storage key has been supplied, then pin this transaction to it + if let Some(key) = pin_key { + let pin_key = Key::from(MASP.to_db_key()) + .push(&(PIN_KEY_PREFIX.to_owned() + key)) + .expect("Cannot obtain a storage key"); + ctx.write( + &pin_key, + IndexedTx { + height: ctx.get_block_height()?, + index: ctx.get_tx_index()?, + }, + )?; + } + Ok(()) } diff --git a/core/src/ledger/vp_env.rs b/core/src/ledger/vp_env.rs index 728e66d9a6..1005e11070 100644 --- a/core/src/ledger/vp_env.rs +++ b/core/src/ledger/vp_env.rs @@ -110,6 +110,7 @@ where /// Get the shielded action including the transfer and the masp tx fn get_shielded_action( + // FIXME: I need this &self, tx_data: &Tx, ) -> Result<(Transfer, Transaction), storage_api::Error> { diff --git a/core/src/types/ibc.rs b/core/src/types/ibc.rs index cfb2357fef..eb060c9f07 100644 --- a/core/src/types/ibc.rs +++ b/core/src/types/ibc.rs @@ -239,6 +239,8 @@ pub fn get_shielded_transfer( return Ok(None); } + // FIXME: I should place the is_masp_tx attribute directly on the ibc event + // not in finalize block FIXME: maybe it's not possible event .attributes .get("memo") diff --git a/core/src/types/storage.rs b/core/src/types/storage.rs index 87d1b80e50..e85cec7b30 100644 --- a/core/src/types/storage.rs +++ b/core/src/types/storage.rs @@ -1433,6 +1433,27 @@ impl GetEventNonce for InnerEthEventsQueue { } } +/// Represents the pointers of an indexed tx, which are the block height and the +/// index inside that block +#[derive( + Default, + Debug, + Copy, + Clone, + BorshSerialize, + BorshDeserialize, + Eq, + PartialEq, + Ord, + PartialOrd, +)] +pub struct IndexedTx { + /// The block height of the indexed tx + pub height: BlockHeight, + /// The index in the block of the tx + pub index: TxIndex, +} + #[cfg(test)] /// Tests and strategies for storage pub mod tests { diff --git a/core/src/types/token.rs b/core/src/types/token.rs index da591466d9..4429a0caa4 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -978,6 +978,8 @@ pub const DENOM_STORAGE_KEY: &str = "denomination"; pub const MINTER_STORAGE_KEY: &str = "minter"; /// Key segment for minted balance pub const MINTED_STORAGE_KEY: &str = "minted"; +/// Key segment prefix for pinned shielded transactions +pub const PIN_KEY_PREFIX: &str = "pin-"; /// Key segment prefix for the nullifiers pub const MASP_NULLIFIERS_KEY: &str = "nullifiers"; /// Key segment prefix for the note commitment merkle tree @@ -1217,7 +1219,10 @@ pub fn is_masp_key(key: &Key) -> bool { pub fn is_masp_allowed_key(key: &Key) -> bool { match &key.segments[..] { [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(key)] - if *addr == MASP && key == MASP_NOTE_COMMITMENT_TREE_KEY => + if *addr == MASP + //FIXME: place the check back in the masp vp if needed + && (key.starts_with(PIN_KEY_PREFIX) + || key == MASP_NOTE_COMMITMENT_TREE_KEY) => { true } @@ -1258,6 +1263,13 @@ pub fn masp_last_inflation_key(token_address: &Address) -> Key { ) } +/// Get a key for a masp pin +pub fn masp_pin_tx_key(key: &str) -> Key { + Key::from(MASP.to_db_key()) + .push(&(PIN_KEY_PREFIX.to_owned() + key)) + .expect("Cannot obtain a storage key") +} + /// Get a key for a masp nullifier pub fn masp_nullifier_key(nullifier: &Nullifier) -> Key { Key::from(MASP.to_db_key()) diff --git a/sdk/src/masp.rs b/sdk/src/masp.rs index 62b4ba1ea2..9fcf72a050 100644 --- a/sdk/src/masp.rs +++ b/sdk/src/masp.rs @@ -5,6 +5,7 @@ use std::env; use std::fmt::Debug; use std::ops::Deref; use std::path::PathBuf; +use std::str::FromStr; // use async_std::io::prelude::WriteExt; // use async_std::io::{self}; @@ -49,12 +50,14 @@ use masp_proofs::bellman::groth16::PreparedVerifyingKey; use masp_proofs::bls12_381::Bls12; use masp_proofs::prover::LocalTxProver; use masp_proofs::sapling::SaplingVerificationContext; +use namada_core::ibc::apps::transfer::types::Memo; use namada_core::types::address::{Address, MASP}; +use namada_core::types::ibc::IbcShieldedTransfer; use namada_core::types::masp::{ BalanceOwner, ExtendedViewingKey, PaymentAddress, TransferSource, TransferTarget, }; -use namada_core::types::storage::{BlockHeight, Epoch, TxIndex}; +use namada_core::types::storage::{BlockHeight, Epoch, IndexedTx, TxIndex}; use namada_core::types::time::{DateTimeUtc, DurationSecs}; use namada_core::types::token; use namada_core::types::token::{Change, MaspDenom, Transfer}; @@ -67,10 +70,11 @@ use thiserror::Error; #[cfg(feature = "testing")] use crate::error::EncodingError; use crate::error::{Error, PinnedBalanceError, QueryError}; +use crate::events::EventType; use crate::io::Io; use crate::proto::Tx; use crate::queries::Client; -use crate::rpc::{query_conversion, query_storage_value}; +use crate::rpc::{query_block, query_conversion, query_epoch_at_height}; use crate::tendermint_rpc::query::Query; use crate::tendermint_rpc::Order; use crate::tx::decode_component; @@ -597,8 +601,8 @@ pub struct ShieldedContext { /// Location where this shielded context is saved #[borsh(skip)] pub utils: U, - /// The last transaction index to be processed in this context - pub last_txidx: u64, + /// The last indexed transaction to be processed in this context + pub last_indexed: IndexedTx, /// The commitment tree produced by scanning all transactions up to tx_pos pub tree: CommitmentTree, /// Maps viewing keys to applicable note positions @@ -614,10 +618,8 @@ pub struct ShieldedContext { /// Maps note positions to their witness (used to make merkle paths) pub witness_map: HashMap>, /// Tracks what each transaction does to various account balances - pub delta_map: BTreeMap< - (BlockHeight, TxIndex), - (Epoch, TransferDelta, TransactionDelta), - >, + pub delta_map: + BTreeMap, /// The set of note positions that have been spent pub spents: HashSet, /// Maps asset types to their decodings @@ -632,7 +634,7 @@ impl Default for ShieldedContext { fn default() -> ShieldedContext { ShieldedContext:: { utils: U::default(), - last_txidx: u64::default(), + last_indexed: IndexedTx::default(), tree: CommitmentTree::empty(), pos_map: HashMap::default(), nf_map: HashMap::default(), @@ -664,7 +666,7 @@ impl ShieldedContext { /// context. It must be the case that the two shielded contexts share the /// same last transaction ID and share identical commitment trees. pub fn merge(&mut self, new_ctx: ShieldedContext) { - debug_assert_eq!(self.last_txidx, new_ctx.last_txidx); + debug_assert_eq!(self.last_indexed, new_ctx.last_indexed); // Merge by simply extending maps. Identical keys should contain // identical values, so overwriting should not be problematic. self.pos_map.extend(new_ctx.pos_map); @@ -679,10 +681,10 @@ impl ShieldedContext { // The deltas are the exception because different keys can reveal // different parts of the same transaction. Hence each delta needs to be // merged separately. - for ((height, idx), (ep, ntfer_delta, ntx_delta)) in new_ctx.delta_map { + for (height, (ep, ntfer_delta, ntx_delta)) in new_ctx.delta_map { let (_ep, tfer_delta, tx_delta) = self .delta_map - .entry((height, idx)) + .entry(height) .or_insert((ep, TransferDelta::new(), TransactionDelta::new())); tfer_delta.extend(ntfer_delta); tx_delta.extend(ntx_delta); @@ -718,7 +720,14 @@ impl ShieldedContext { let (txs, mut tx_iter); if !unknown_keys.is_empty() { // Load all transactions accepted until this point - txs = Self::fetch_shielded_transfers(client, 0).await?; + txs = Self::fetch_shielded_transfers( + client, + IndexedTx { + height: BlockHeight::first(), + index: TxIndex::default(), + }, + ) + .await?; tx_iter = txs.iter(); // Do this by constructing a shielding context only for unknown keys let mut tx_ctx = Self { @@ -729,11 +738,10 @@ impl ShieldedContext { tx_ctx.pos_map.entry(vk).or_insert_with(BTreeSet::new); } // Update this unknown shielded context until it is level with self - while tx_ctx.last_txidx != self.last_txidx { - if let Some(((height, idx), (epoch, tx, stx))) = tx_iter.next() - { + while tx_ctx.last_indexed != self.last_indexed { + if let Some((indexed_tx, (epoch, tx, stx))) = tx_iter.next() { tx_ctx - .scan_tx(client, *height, *idx, *epoch, tx, stx) + .scan_tx(client, *indexed_tx, *epoch, tx, stx) .await?; } else { break; @@ -744,54 +752,270 @@ impl ShieldedContext { self.merge(tx_ctx); } else { // Load only transactions accepted from last_txid until this point - txs = - Self::fetch_shielded_transfers(client, self.last_txidx).await?; + txs = Self::fetch_shielded_transfers(client, self.last_indexed) + .await?; tx_iter = txs.iter(); } // Now that we possess the unspent notes corresponding to both old and // new keys up until tx_pos, proceed to scan the new transactions. - for ((height, idx), (epoch, tx, stx)) in &mut tx_iter { - self.scan_tx(client, *height, *idx, *epoch, tx, stx).await?; + for (indexed_tx, (epoch, tx, stx)) in &mut tx_iter { + self.scan_tx(client, *indexed_tx, *epoch, tx, stx).await?; } Ok(()) } /// Obtain a chronologically-ordered list of all accepted shielded - /// transactions from the ledger. The ledger conceptually stores - /// transactions as a vector. More concretely, the HEAD_TX_KEY location - /// stores the index of the last accepted transaction and each transaction - /// is stored at a key derived from its index. + /// transactions from a node. pub async fn fetch_shielded_transfers( client: &C, - last_txidx: u64, - ) -> Result< - BTreeMap<(BlockHeight, TxIndex), (Epoch, Transfer, Transaction)>, - Error, - > { - // Construct the key where last transaction pointer is stored - let head_tx_key = namada_core::types::token::masp_head_tx_key(); - // Query for the index of the last accepted transaction - let head_txidx = query_storage_value::(client, &head_tx_key) - .await - .unwrap_or(0); + last_indexed_tx: IndexedTx, + ) -> Result, Error> + { + // Query for the last produced block height + let last_block_height = query_block(client) + .await? + .map_or_else(BlockHeight::first, |block| block.height); + let mut shielded_txs = BTreeMap::new(); // Fetch all the transactions we do not have yet - for i in last_txidx..head_txidx { - // Construct the key for where the current transaction is stored - let current_tx_key = namada_core::types::token::masp_tx_key(i); - // Obtain the current transaction - let (tx_epoch, tx_height, tx_index, current_tx, current_stx) = - query_storage_value::< - C, - (Epoch, BlockHeight, TxIndex, Transfer, Transaction), - >(client, ¤t_tx_key) - .await?; - // Collect the current transaction - shielded_txs.insert( - (tx_height, tx_index), - (tx_epoch, current_tx, current_stx), - ); + for height in u64::from(last_indexed_tx.height)..=last_block_height.0 { + // Get the valid masp transactions at the specified height + // FIXME: review if we really need extra key for ibc events + let tx_query = + Query::eq("height", height).and_exists("is_valid_masp_tx"); + + let epoch = query_epoch_at_height(client, height.into()) + .await? + .ok_or_else(|| { + Error::from(QueryError::General(format!( + "Queried height is greater than the last committed \ + block height" + ))) + })?; + + // Paginate the results + for page in 1.. { + let txs_results = client + .tx_search( + tx_query.clone(), + // TODO: currently we are not verifying the merkle + // proof, we should or at least ask a parameter to the + // user ot decide if we want proofs(and their + // verification) or not + false, + page, + 100, + Order::Ascending, + ) + .await + .map_err(|e| { + Error::from(QueryError::General(e.to_string())) + })? + .txs; + + for result in &txs_results { + let tx_index = TxIndex(result.index); + if BlockHeight(height) == last_indexed_tx.height + && tx_index <= last_indexed_tx.index + { + continue; + } + let tx = Tx::try_from(result.tx.as_ref()) + .map_err(|e| Error::Other(e.to_string()))?; + let tx_header = tx.header(); + // 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 in the signed commitments (hashes) + // of the transactions' headers/data sections + if let Some(wrapper_header) = tx_header.wrapper() { + let hash = wrapper_header + .unshield_section_hash + .ok_or_else(|| { + Error::Other( + "Missing expected fee unshielding section \ + hash" + .to_string(), + ) + })?; + + let masp_transaction = tx + .get_section(&hash) + .ok_or_else(|| { + Error::Other( + "Missing expected masp section".to_string(), + ) + })? + .masp_tx() + .ok_or_else(|| { + Error::Other( + "Missing masp transaction".to_string(), + ) + })?; + // FIXME: actually, do I realy need the entire transfer + // object? Probably not mayube we can remove + // FIXME: mroe in general. review everything we are + // storing to see if we actually need it + + // Transfer objects for fee unshielding are absent from + // the tx because they are completely constructed in + // protocol, need to recreate it here + let transfer = Transfer { + source: MASP, + target: wrapper_header.fee_payer(), + token: wrapper_header.fee.token.clone(), + amount: wrapper_header + .get_tx_fee() + .map_err(|e| Error::Other(e.to_string()))?, + key: None, + shielded: Some(hash), + }; + + // Collect the current transaction + shielded_txs.insert( + IndexedTx { + height: height.into(), + index: tx_index, + }, + (epoch, transfer, masp_transaction), + ); + } else if tx_header.decrypted().is_some() { + let (transfer, masp_transaction) = + match Transfer::try_from_slice( + &tx.data().ok_or_else(|| { + Error::Other( + "Missing data section".to_string(), + ) + })?, + ) { + Ok(transfer) => { + let masp_transaction = tx + .get_section( + &transfer.shielded.ok_or_else( + || { + Error::Other( + "Missing masp section \ + hash" + .to_string(), + ) + }, + )?, + ) + .ok_or_else(|| { + Error::Other( + "Missing masp section in \ + transaction" + .to_string(), + ) + })? + .masp_tx() + .ok_or_else(|| { + Error::Other( + "Missing masp transaction" + .to_string(), + ) + })?; + + (transfer, masp_transaction) + } + Err(_) => { + // This should be a MASP over IBC transfer, + // continue for now, we'll scan the ibc + // events later + continue; + } + }; + + // Collect the current transaction + shielded_txs.insert( + IndexedTx { + height: height.into(), + index: tx_index, + }, + (epoch, transfer, masp_transaction), + ); + } + } + + // FIXME: are we already storing masp transactions when we + // submit them? In this case I need to make sure we don't + // reqrite them from here An incomplete page + // signifies no more transactions + if txs_results.len() < 100 { + break; + } + } } + + // Fetch all block events to look for MASP over IBC transactions + for height in u64::from(last_indexed_tx.height)..=last_block_height.0 { + let epoch = query_epoch_at_height(client, height.into()) + .await? + .ok_or_else(|| { + Error::from(QueryError::General(format!( + "Queried height is greater than the last committed \ + block height" + ))) + })?; + + let events = client + .block_results(height) + .await + .map_err(|e| Error::from(QueryError::General(e.to_string())))? + .end_block_events + .unwrap_or_default(); + + for (ibc_masp_event, tx_index) in + events.iter().filter_map(|event| { + if event + .kind + .starts_with(&EventType::Ibc(String::new()).to_string()) + { + event + .attributes + .iter() + .find(|attribute| { + &attribute.key == "is_valid_masp_tx" + }) + .map(|tx_index| (event, &tx_index.value)) + } else { + None + } + }) + { + // FIXME: can we parallelize stuff somewhere when constructing + // the internal state of the ShieldeContext? + // Masp transaction, collect it + let shielded_transfer = ibc_masp_event + .attributes + .iter() + .find(|attribute| &attribute.key == "memo") + .map(|memo| { + IbcShieldedTransfer::try_from(Memo::from( + memo.value.clone(), + )) + }); + + if let Some(Ok(shielded_transfer)) = shielded_transfer { + shielded_txs.insert( + IndexedTx { + height: height.into(), + index: TxIndex( + u32::from_str(tx_index) + .map_err(|e| Error::Other(e.to_string()))?, + ), + }, + ( + epoch, + shielded_transfer.transfer, + shielded_transfer.masp_tx, + ), + ); + } + } + } + Ok(shielded_txs) } @@ -806,8 +1030,7 @@ impl ShieldedContext { pub async fn scan_tx( &mut self, client: &C, - height: BlockHeight, - index: TxIndex, + indexed_tx: IndexedTx, epoch: Epoch, tx: &Transfer, shielded: &Transaction, @@ -933,12 +1156,10 @@ impl ShieldedContext { change: -tx.amount.amount().change(), }, ); - self.last_txidx += 1; + self.last_indexed = indexed_tx; - self.delta_map.insert( - (height, index), - (epoch, transfer_delta, transaction_delta), - ); + self.delta_map + .insert(indexed_tx, (epoch, transfer_delta, transaction_delta)); Ok(()) } @@ -946,10 +1167,7 @@ impl ShieldedContext { /// Transfer in this context pub fn get_tx_deltas( &self, - ) -> &BTreeMap< - (BlockHeight, TxIndex), - (Epoch, TransferDelta, TransactionDelta), - > { + ) -> &BTreeMap { &self.delta_map } @@ -1363,21 +1581,73 @@ impl ShieldedContext { // Obtain the transaction pointer at the key // If we don't discard the error message then a test fails, // however the error underlying this will go undetected - let txidx = rpc::query_storage_value::(client, &pin_key) - .await - .map_err(|_| PinnedBalanceError::NoTransactionPinned)?; - // Construct the key for where the pinned transaction is stored - let tx_key = namada_core::types::token::masp_tx_key(txidx); - // Obtain the pointed to transaction - let (tx_epoch, _tx_height, _tx_index, _tx, shielded) = - rpc::query_storage_value::< - C, - (Epoch, BlockHeight, TxIndex, Transfer, Transaction), - >(client, &tx_key) - .await - .map_err(|_| { - Error::Other("Ill-formed epoch, transaction pair".to_string()) + let indexed_tx = + rpc::query_storage_value::(client, &pin_key) + .await + .map_err(|_| PinnedBalanceError::NoTransactionPinned)?; + let tx_epoch = query_epoch_at_height(client, indexed_tx.height) + .await? + .ok_or_else(|| { + Error::from(QueryError::General(format!( + "Queried height is greater than the last committed block \ + height" + ))) })?; + + let tx_query = Query::eq("height", indexed_tx.height.to_string()) + .and_eq("tx.index", indexed_tx.index.to_string()); + let tx_results = client + .tx_search( + tx_query.clone(), + // TODO: currently we are not verifying the merkle + // proof, we should or at least ask a parameter to the + // user ot decide if we want proofs(and their + // verification) or not + false, + 1, + 100, + Order::Ascending, + ) + .await + .map_err(|e| Error::from(QueryError::General(e.to_string())))? + .txs; + + let tx = Tx::try_from( + tx_results + .first() + .ok_or_else(|| { + Error::Other("Missing expected pinned masp tx".to_string()) + })? + .tx + .as_ref(), + ) + .map_err(|e| Error::Other(e.to_string()))?; + + let shielded = + match Transfer::try_from_slice(&tx.data().ok_or_else(|| { + Error::Other("Missing data section".to_string()) + })?) { + Ok(transfer) => tx + .get_section(&transfer.shielded.ok_or_else(|| { + Error::Other("Missing masp section hash".to_string()) + })?) + .ok_or_else(|| { + Error::Other( + "Missing masp section in transaction".to_string(), + ) + })? + .masp_tx() + .ok_or_else(|| { + Error::Other("Missing masp transaction".to_string()) + })?, + Err(_) => { + // FIXME: add support for pinned ibc masp txs + // FIXME: probably need to review also how we do it in + // fewtch_shielded_transfer + return Err(Error::Other("IBC Masp pinned tx".to_string())); + } + }; + // Accumulate the combined output note value into this Amount let mut val_acc = I128Sum::zero(); for so in shielded @@ -1516,8 +1786,6 @@ impl ShieldedContext { // No shielded components are needed when neither source nor destination // are shielded - use std::str::FromStr; - use rand::rngs::StdRng; use rand_core::SeedableRng; @@ -1884,6 +2152,10 @@ impl ShieldedContext { /// transactions. If an owner is specified, then restrict the set to only /// transactions crediting/debiting the given owner. If token is specified, /// then restrict set to only transactions involving the given token. + // FIXME: need to handle fee unshielding txs here? + // FIXME: I believe this is wrong but it's not caught because it's only used + // by a function that's never used (can only be used from cli). I should + // test this with a testnet pub async fn query_tx_deltas( &mut self, client: &C, @@ -1891,10 +2163,7 @@ impl ShieldedContext { query_token: &Option
, viewing_keys: &HashMap, ) -> Result< - BTreeMap< - (BlockHeight, TxIndex), - (Epoch, TransferDelta, TransactionDelta), - >, + BTreeMap, Error, > { const TXS_PER_PAGE: u8 = 100; @@ -1909,6 +2178,7 @@ impl ShieldedContext { let _ = self.save().await; // Required for filtering out rejected transactions from Tendermint // responses + // FIXME: here we query the reulst of only the last block let block_results = rpc::query_results(client).await?; let mut transfers = self.get_tx_deltas().clone(); // Construct the set of addresses relevant to user's query @@ -1925,6 +2195,8 @@ impl ShieldedContext { for addr in relevant_addrs { for prop in ["transfer.source", "transfer.target"] { // Query transactions involving the current address + // FIXME: bnut it seems like here we query all transactions, not + // only those from the last block let mut tx_query = Query::eq(prop, addr.encode()); // Elaborate the query if requested by the user if let Some(token) = &query_token { @@ -1932,6 +2204,24 @@ impl ShieldedContext { tx_query.and_eq("transfer.token", token.encode()); } for page in 1.. { + // FIXME: it only looks ath transfer, + // source (how does it do it, they seem to not be indexed) + // which works for raw transactions but does not take into + // account wrappers I guess, plus the wrapper might fail bu + // the payment would still be committed + // FIXME: wait do we scan correctly also for wrapper txs + // masp transactions? For the wallet update probably yes + // because we query the storage but for this function + // probably not! FIXME: I think this + // works based on the deserialized transaction, more + // specifically the deserialized Transfer object of a + // transaction. Can I use something similar to query masp? + // Is it fast enough? I don't think so because it would need + // to deserialize evertyhing to match the args of the query + // FIXME: actually I don't think this work, the rpc endpoint + // to query transaction should only filter based on the + // indexed key value pairs, it doesn't deserialize the tx + // (it shouldn't even be able to do that) let txs = &client .tx_search( tx_query.clone(), @@ -1953,7 +2243,7 @@ impl ShieldedContext { // Only process yet unprocessed transactions which have // been accepted by node VPs let should_process = !transfers - .contains_key(&(height, idx)) + .contains_key(&IndexedTx { height, index: idx }) && block_results[u64::from(height) as usize] .is_accepted(idx.0 as usize); if !should_process { @@ -1989,7 +2279,7 @@ impl ShieldedContext { // No shielded accounts are affected by this // Transfer transfers.insert( - (height, idx), + IndexedTx { height, index: idx }, (epoch, delta, TransactionDelta::new()), ); } diff --git a/sdk/src/queries/mod.rs b/sdk/src/queries/mod.rs index 4dbc5173b8..7506b4520d 100644 --- a/sdk/src/queries/mod.rs +++ b/sdk/src/queries/mod.rs @@ -301,10 +301,13 @@ pub trait Client { height: H, ) -> Result where - H: Into + Send, + H: TryInto + Send, { + let height = height.try_into().map_err(|_| { + RpcError::parse("Could not parse to height".to_string()) + })?; self.perform(tendermint_rpc::endpoint::block_results::Request::new( - height.into(), + height, )) .await } diff --git a/shared/src/ledger/mod.rs b/shared/src/ledger/mod.rs index abd96064bb..1a28ff12f4 100644 --- a/shared/src/ledger/mod.rs +++ b/shared/src/ledger/mod.rs @@ -71,6 +71,7 @@ mod dry_run_tx { &mut ctx.tx_wasm_cache, ), None, + &mut false, ) .into_storage_result()?; diff --git a/shared/src/ledger/native_vp/ibc/context.rs b/shared/src/ledger/native_vp/ibc/context.rs index 968ff53329..29b3ab901b 100644 --- a/shared/src/ledger/native_vp/ibc/context.rs +++ b/shared/src/ledger/native_vp/ibc/context.rs @@ -216,8 +216,9 @@ where fn handle_masp_tx( &mut self, shielded: &masp_primitives::transaction::Transaction, + pin_key: Option<&str>, ) -> Result<()> { - masp_utils::handle_masp_tx(self, shielded)?; + masp_utils::handle_masp_tx(self, shielded, pin_key)?; masp_utils::update_note_commitment_tree(self, shielded) } @@ -418,6 +419,7 @@ where fn handle_masp_tx( &mut self, _shielded: &masp_primitives::transaction::Transaction, + _pin_key: Option<&str>, ) -> Result<()> { unimplemented!("Validation doesn't handle a masp tx") } diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index f7811245f8..4bdbc94ca9 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -14,11 +14,12 @@ use namada_core::ledger::storage; use namada_core::ledger::storage_api::OptionExt; use namada_core::ledger::vp_env::VpEnv; use namada_core::proto::Tx; -use namada_core::types::address::Address; use namada_core::types::address::InternalAddress::Masp; -use namada_core::types::storage::{Epoch, Key}; +use namada_core::types::address::{Address, MASP}; +use namada_core::types::storage::{Epoch, IndexedTx, Key, KeySeg}; use namada_core::types::token::{ self, is_masp_allowed_key, is_masp_key, is_masp_nullifier_key, + PIN_KEY_PREFIX, }; use namada_sdk::masp::verify_shielded_tx; use ripemd::Digest as RipemdDigest; @@ -284,6 +285,35 @@ where Ok(true) } + + fn valid_state( + &self, + masp_keys_changed: &[&Key], + pin_key: Option<&str>, + ) -> Result { + // Check that the transaction didn't write unallowed masp keys + if masp_keys_changed + .iter() + .any(|key| !is_masp_allowed_key(key)) + { + return Ok(false); + } + + // Validate pin key + if let Some(key) = pin_key { + let pin_key = Key::from(MASP.to_db_key()) + .push(&(PIN_KEY_PREFIX.to_owned() + key)) + .expect("Cannot obtain a storage key"); + match self.ctx.read_post::(&pin_key)? { + Some(IndexedTx { height, index }) + if height == self.ctx.get_block_height()? + && index == self.ctx.get_tx_index()? => {} + _ => return Ok(false), + } + } + + Ok(true) + } } impl<'a, DB, H, CA> NativeVp for MaspVp<'a, DB, H, CA> @@ -309,15 +339,13 @@ where // The Sapling value balance adds to the transparent tx pool transparent_tx_pool += shielded_tx.sapling_value_balance(); - // Check that the transaction didn't write unallowed masp keys + // Check the validity of the keys let masp_keys_changed: Vec<&Key> = keys_changed.iter().filter(|key| is_masp_key(key)).collect(); - if masp_keys_changed - .iter() - .filter(|key| !is_masp_allowed_key(key)) - .count() - != 0 - { + if !self.valid_state( + masp_keys_changed.as_slice(), + transfer.key.as_deref(), + )? { return Ok(false); } diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index af2b808892..85fda430ea 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -2557,8 +2557,9 @@ where fn handle_masp_tx( &mut self, shielded: &masp_primitives::transaction::Transaction, + pin_key: Option<&str>, ) -> Result<(), storage_api::Error> { - masp_utils::handle_masp_tx(self, shielded)?; + masp_utils::handle_masp_tx(self, shielded, pin_key)?; masp_utils::update_note_commitment_tree(self, shielded) } diff --git a/tx_prelude/src/ibc.rs b/tx_prelude/src/ibc.rs index 4a04e2a980..2418e4b8f0 100644 --- a/tx_prelude/src/ibc.rs +++ b/tx_prelude/src/ibc.rs @@ -52,8 +52,9 @@ impl IbcStorageContext for Ctx { fn handle_masp_tx( &mut self, shielded: &masp_primitives::transaction::Transaction, + pin_key: Option<&str>, ) -> Result<(), Error> { - masp_utils::handle_masp_tx(self, shielded)?; + masp_utils::handle_masp_tx(self, shielded, pin_key)?; masp_utils::update_note_commitment_tree(self, shielded) } diff --git a/wasm/wasm_source/src/tx_transfer.rs b/wasm/wasm_source/src/tx_transfer.rs index f7874e33ec..34c3045dd9 100644 --- a/wasm/wasm_source/src/tx_transfer.rs +++ b/wasm/wasm_source/src/tx_transfer.rs @@ -38,7 +38,11 @@ fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { }) .transpose()?; if let Some(shielded) = shielded { - token::masp_utils::handle_masp_tx(ctx, &shielded)?; + token::masp_utils::handle_masp_tx( + ctx, + &shielded, + transfer.key.as_deref(), + )?; update_masp_note_commitment_tree(&shielded)?; } Ok(())