From 301e8b497c3c25a8c75945b1a699a4f5b0ff4068 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 29 Jul 2024 11:41:56 -0600 Subject: [PATCH 01/18] zcash_client_sqlite: Add an internal newtype for transaction primary key values. --- zcash_client_sqlite/src/lib.rs | 7 +++++-- zcash_client_sqlite/src/wallet.rs | 19 ++++++++++--------- .../init/migrations/receiving_key_scopes.rs | 4 ++-- zcash_client_sqlite/src/wallet/orchard.rs | 14 +++++++------- zcash_client_sqlite/src/wallet/sapling.rs | 14 +++++++------- zcash_client_sqlite/src/wallet/transparent.rs | 5 +++-- .../src/wallet/transparent/ephemeral.rs | 9 +++++---- 7 files changed, 39 insertions(+), 33 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index ec20c4b845..1655d1be32 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -177,11 +177,14 @@ impl fmt::Display for ReceivedNoteId { } } -/// A newtype wrapper for sqlite primary key values for the utxos -/// table. +/// A newtype wrapper for sqlite primary key values for the utxos table. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct UtxoId(pub i64); +/// A newtype wrapper for sqlite primary key values for the transactions table. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +struct TxRef(pub i64); + /// A wrapper for the SQLite connection to the wallet database. pub struct WalletDb { conn: C, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 9067ad324a..f41ec47798 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -108,6 +108,7 @@ use zcash_primitives::{ }; use zip32::{self, DiversifierIndex, Scope}; +use crate::TxRef; use crate::{ error::SqliteClientError, wallet::commitment_tree::{get_max_checkpointed_height, SqliteShardStore}, @@ -2199,7 +2200,7 @@ pub(crate) fn put_tx_meta( conn: &rusqlite::Connection, tx: &WalletTx, height: BlockHeight, -) -> Result { +) -> Result { // It isn't there, so insert our transaction into the database. let mut stmt_upsert_tx_meta = conn.prepare_cached( "INSERT INTO transactions (txid, block, mined_height, tx_index) @@ -2219,7 +2220,7 @@ pub(crate) fn put_tx_meta( ]; stmt_upsert_tx_meta - .query_row(tx_params, |row| row.get::<_, i64>(0)) + .query_row(tx_params, |row| row.get::<_, i64>(0).map(TxRef)) .map_err(SqliteClientError::from) } @@ -2271,7 +2272,7 @@ pub(crate) fn put_tx_data( tx: &Transaction, fee: Option, created_at: Option, -) -> Result { +) -> Result { let mut stmt_upsert_tx_data = conn.prepare_cached( "INSERT INTO transactions (txid, created, expiry_height, raw, fee) VALUES (:txid, :created_at, :expiry_height, :raw, :fee) @@ -2295,7 +2296,7 @@ pub(crate) fn put_tx_data( ]; stmt_upsert_tx_data - .query_row(tx_params, |row| row.get::<_, i64>(0)) + .query_row(tx_params, |row| row.get::<_, i64>(0).map(TxRef)) .map_err(SqliteClientError::from) } @@ -2332,7 +2333,7 @@ fn recipient_params( pub(crate) fn insert_sent_output( conn: &rusqlite::Connection, params: &P, - tx_ref: i64, + tx_ref: TxRef, from_account: AccountId, output: &SentTransactionOutput, ) -> Result<(), SqliteClientError> { @@ -2347,7 +2348,7 @@ pub(crate) fn insert_sent_output( let (to_address, to_account_id, pool_type) = recipient_params(params, output.recipient()); let sql_args = named_params![ - ":tx": &tx_ref, + ":tx": tx_ref.0, ":output_pool": &pool_code(pool_type), ":output_index": &i64::try_from(output.output_index()).unwrap(), ":from_account_id": from_account.0, @@ -2378,7 +2379,7 @@ pub(crate) fn put_sent_output( conn: &rusqlite::Connection, params: &P, from_account: AccountId, - tx_ref: i64, + tx_ref: TxRef, output_index: usize, recipient: &Recipient, value: NonNegativeAmount, @@ -2401,7 +2402,7 @@ pub(crate) fn put_sent_output( let (to_address, to_account_id, pool_type) = recipient_params(params, recipient); let sql_args = named_params![ - ":tx": &tx_ref, + ":tx": tx_ref.0, ":output_pool": &pool_code(pool_type), ":output_index": &i64::try_from(output_index).unwrap(), ":from_account_id": from_account.0, @@ -2515,7 +2516,7 @@ pub(crate) fn query_nullifier_map>( conn: &rusqlite::Transaction<'_>, spend_pool: ShieldedProtocol, nf: &N, -) -> Result, SqliteClientError> { +) -> Result, SqliteClientError> { let mut stmt_select_locator = conn.prepare_cached( "SELECT block_height, tx_index, txid FROM nullifier_map diff --git a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs index 722c609bb5..3389af518a 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs @@ -516,7 +516,7 @@ mod tests { // Don't need to bother with sent outputs for this test. if output.transfer_type() != TransferType::Outgoing { put_received_note_before_migration( - wdb.conn.0, output, tx_ref, None, + wdb.conn.0, output, tx_ref.0, None, ) .unwrap(); } @@ -529,7 +529,7 @@ mod tests { } } - put_received_note_before_migration(wdb.conn.0, output, tx_ref, None) + put_received_note_before_migration(wdb.conn.0, output, tx_ref.0, None) .unwrap(); } } diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index 88e7f66f75..cbd8b23b4b 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -21,7 +21,7 @@ use zcash_protocol::{ }; use zip32::Scope; -use crate::{error::SqliteClientError, AccountId, ReceivedNoteId}; +use crate::{error::SqliteClientError, AccountId, ReceivedNoteId, TxRef}; use super::{memo_repr, parse_scope, scope_code}; @@ -227,8 +227,8 @@ pub(crate) fn select_spendable_orchard_notes( pub(crate) fn put_received_note( conn: &Transaction, output: &T, - tx_ref: i64, - spent_in: Option, + tx_ref: TxRef, + spent_in: Option, ) -> Result<(), SqliteClientError> { let mut stmt_upsert_received_note = conn.prepare_cached( "INSERT INTO orchard_received_notes @@ -263,7 +263,7 @@ pub(crate) fn put_received_note( let diversifier = to.diversifier(); let sql_args = named_params![ - ":tx": &tx_ref, + ":tx": tx_ref.0, ":action_index": i64::try_from(output.index()).expect("output indices are representable as i64"), ":account_id": output.account_id().0, ":diversifier": diversifier.as_array(), @@ -288,7 +288,7 @@ pub(crate) fn put_received_note( ON CONFLICT (orchard_received_note_id, transaction_id) DO NOTHING", named_params![ ":orchard_received_note_id": received_note_id, - ":transaction_id": spent_in + ":transaction_id": spent_in.0 ], )?; } @@ -366,7 +366,7 @@ pub(crate) fn detect_spending_accounts<'a>( /// spending transaction has been mined. pub(crate) fn mark_orchard_note_spent( conn: &Connection, - tx_ref: i64, + tx_ref: TxRef, nf: &Nullifier, ) -> Result { let mut stmt_mark_orchard_note_spent = conn.prepare_cached( @@ -377,7 +377,7 @@ pub(crate) fn mark_orchard_note_spent( match stmt_mark_orchard_note_spent.execute(named_params![ ":nf": nf.to_bytes(), - ":transaction_id": tx_ref + ":transaction_id": tx_ref.0 ])? { 0 => Ok(false), 1 => Ok(true), diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index b43ce4a908..1e51df25c3 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -20,7 +20,7 @@ use zcash_protocol::{ }; use zip32::Scope; -use crate::{error::SqliteClientError, AccountId, ReceivedNoteId}; +use crate::{error::SqliteClientError, AccountId, ReceivedNoteId, TxRef}; use super::{memo_repr, parse_scope, scope_code}; @@ -300,7 +300,7 @@ pub(crate) fn detect_spending_accounts<'a>( /// spending transaction has been mined. pub(crate) fn mark_sapling_note_spent( conn: &Connection, - tx_ref: i64, + tx_ref: TxRef, nf: &sapling::Nullifier, ) -> Result { let mut stmt_mark_sapling_note_spent = conn.prepare_cached( @@ -311,7 +311,7 @@ pub(crate) fn mark_sapling_note_spent( match stmt_mark_sapling_note_spent.execute(named_params![ ":nf": &nf.0[..], - ":transaction_id": tx_ref + ":transaction_id": tx_ref.0 ])? { 0 => Ok(false), 1 => Ok(true), @@ -327,8 +327,8 @@ pub(crate) fn mark_sapling_note_spent( pub(crate) fn put_received_note( conn: &Transaction, output: &T, - tx_ref: i64, - spent_in: Option, + tx_ref: TxRef, + spent_in: Option, ) -> Result<(), SqliteClientError> { let mut stmt_upsert_received_note = conn.prepare_cached( "INSERT INTO sapling_received_notes @@ -366,7 +366,7 @@ pub(crate) fn put_received_note( let diversifier = to.diversifier(); let sql_args = named_params![ - ":tx": &tx_ref, + ":tx": tx_ref.0, ":output_index": i64::try_from(output.index()).expect("output indices are representable as i64"), ":account_id": output.account_id().0, ":diversifier": &diversifier.0.as_ref(), @@ -390,7 +390,7 @@ pub(crate) fn put_received_note( ON CONFLICT (sapling_received_note_id, transaction_id) DO NOTHING", named_params![ ":sapling_received_note_id": received_note_id, - ":transaction_id": spent_in + ":transaction_id": spent_in.0 ], )?; } diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 0db0fca048..4f0d95b9bb 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -20,6 +20,7 @@ use zcash_primitives::{ }; use zcash_protocol::consensus::{self, BlockHeight}; +use crate::TxRef; use crate::{error::SqliteClientError, AccountId, UtxoId}; use super::{chain_tip_height, get_account_ids}; @@ -429,7 +430,7 @@ pub(crate) fn add_transparent_account_balances( /// Marks the given UTXO as having been spent. pub(crate) fn mark_transparent_utxo_spent( conn: &rusqlite::Connection, - tx_ref: i64, + tx_ref: TxRef, outpoint: &OutPoint, ) -> Result<(), SqliteClientError> { let mut stmt_mark_transparent_utxo_spent = conn.prepare_cached( @@ -443,7 +444,7 @@ pub(crate) fn mark_transparent_utxo_spent( )?; let sql_args = named_params![ - ":spent_in_tx": &tx_ref, + ":spent_in_tx": tx_ref.0, ":prevout_txid": &outpoint.hash().to_vec(), ":prevout_idx": &outpoint.n(), ]; diff --git a/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs b/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs index 017ad002f9..f796d931a9 100644 --- a/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs +++ b/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs @@ -15,6 +15,7 @@ use zcash_primitives::{ }; use zcash_protocol::consensus; +use crate::TxRef; use crate::{ error::SqliteClientError, wallet::{get_account, GAP_LIMIT}, @@ -360,7 +361,7 @@ fn ephemeral_address_reuse_check( pub(crate) fn mark_ephemeral_address_as_used( wdb: &mut WalletDb, P>, ephemeral_address: &TransparentAddress, - tx_ref: i64, + tx_ref: TxRef, ) -> Result<(), SqliteClientError> { let address_str = ephemeral_address.encode(&wdb.params); ephemeral_address_reuse_check(wdb, &address_str)?; @@ -377,7 +378,7 @@ pub(crate) fn mark_ephemeral_address_as_used( SET used_in_tx = :tx_ref, seen_in_tx = :tx_ref WHERE address = :address RETURNING account_id, address_index", - named_params![":tx_ref": &tx_ref, ":address": address_str], + named_params![":tx_ref": tx_ref.0, ":address": address_str], |row| Ok((AccountId(row.get::<_, u32>(0)?), row.get::<_, u32>(1)?)), ) .optional()?; @@ -398,7 +399,7 @@ pub(crate) fn mark_ephemeral_address_as_used( pub(crate) fn mark_ephemeral_address_as_seen( wdb: &mut WalletDb, P>, address: &TransparentAddress, - tx_ref: i64, + tx_ref: TxRef, ) -> Result<(), SqliteClientError> { let address_str = address.encode(&wdb.params); @@ -419,7 +420,7 @@ pub(crate) fn mark_ephemeral_address_as_seen( tx_index ASC NULLS LAST, e.seen_in_tx ASC NULLS LAST LIMIT 1", - named_params![":tx_ref": &tx_ref, ":address": address_str], + named_params![":tx_ref": tx_ref.0, ":address": address_str], |row| row.get::<_, i64>(0), )?; From 1057ddb516cb86c491dfe03756c3c5c7c069ab6a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 29 Jul 2024 15:07:36 -0600 Subject: [PATCH 02/18] zcash_client_sqlite: Add a table to track transaction status and enrichment requests. --- zcash_client_sqlite/src/lib.rs | 75 ++++++++++++++- zcash_client_sqlite/src/wallet.rs | 92 ++++++++++++++++++- zcash_client_sqlite/src/wallet/db.rs | 19 ++++ zcash_client_sqlite/src/wallet/init.rs | 1 + .../src/wallet/init/migrations.rs | 6 +- .../init/migrations/tx_retrieval_queue.rs | 61 ++++++++++++ zcash_client_sqlite/src/wallet/transparent.rs | 3 +- zcash_primitives/CHANGELOG.md | 1 + .../src/transaction/components/transparent.rs | 7 +- 9 files changed, 257 insertions(+), 8 deletions(-) create mode 100644 zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 1655d1be32..cef2e8a9f1 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -118,7 +118,7 @@ pub mod error; pub mod wallet; use wallet::{ commitment_tree::{self, put_shard_roots}, - SubtreeScanProgress, + notify_tx_retrieved, SubtreeScanProgress, TxQueryType, }; #[cfg(feature = "transparent-inputs")] @@ -787,6 +787,12 @@ impl WalletWrite for WalletDb for tx in block.transactions() { let tx_row = wallet::put_tx_meta(wdb.conn.0, tx, block.height())?; + wallet::queue_tx_retrieval( + wdb.conn.0, + std::iter::once(tx.txid()), + TxQueryType::Enhancement, + None, + )?; // Mark notes as spent and remove them from the scanning cache for spend in tx.sapling_spends() { @@ -1183,7 +1189,27 @@ impl WalletWrite for WalletDb ) } + // A flag used to determine whether it is necessary to query for transactions that + // provided transparent inputs to this transaction, in order to be able to correctly + // recover transparent transaction history. + #[cfg(feature = "transparent-inputs")] + let mut tx_has_wallet_outputs = false; + + #[cfg(feature = "transparent-inputs")] + let detectable_via_scanning = { + let mut detectable_via_scanning = d_tx.tx().sapling_bundle().is_some(); + #[cfg(feature = "orchard")] { + detectable_via_scanning |= d_tx.tx().orchard_bundle().is_some(); + } + + detectable_via_scanning + }; + for output in d_tx.sapling_outputs() { + #[cfg(feature = "transparent-inputs")] + { + tx_has_wallet_outputs = true; + } match output.transfer_type() { TransferType::Outgoing => { let recipient = { @@ -1268,6 +1294,10 @@ impl WalletWrite for WalletDb #[cfg(feature = "orchard")] for output in d_tx.orchard_outputs() { + #[cfg(feature = "transparent-inputs")] + { + tx_has_wallet_outputs = true; + } match output.transfer_type() { TransferType::Outgoing => { let recipient = { @@ -1395,6 +1425,11 @@ impl WalletWrite for WalletDb &address, account_id )?; + + // Since the wallet created the transparent output, we need to ensure + // that any transparent inputs belonging to the wallet will be + // discovered. + tx_has_wallet_outputs = true; } // If a transaction we observe contains spends from our wallet, we will @@ -1428,11 +1463,49 @@ impl WalletWrite for WalletDb txout.value, None, )?; + + // Even though we know the funding account, we don't know that we have + // information for all of the transparent inputs to the transaction. + #[cfg(feature = "transparent-inputs")] + { + tx_has_wallet_outputs = true; + } + + // If the decrypted transaction is unmined and has no shielded + // components, add it to the queue for status retrieval. + #[cfg(feature = "transparent-inputs")] + if d_tx.mined_height().is_none() && !detectable_via_scanning { + wallet::queue_tx_retrieval( + wdb.conn.0, + std::iter::once(d_tx.tx().txid()), + TxQueryType::Status, + None + )?; + } } } } } + // If the transaction has outputs that belong to the wallet as well as transparent + // inputs, we may need to download the transactions corresponding to the transparent + // prevout references to determine whether the transaction was created (at least in + // part) by this wallet. + #[cfg(feature = "transparent-inputs")] + if tx_has_wallet_outputs { + if let Some(b) = d_tx.tx().transparent_bundle() { + // queue the transparent inputs for enhancement + wallet::queue_tx_retrieval( + wdb.conn.0, + b.vin.iter().map(|txin| *txin.prevout.txid()), + TxQueryType::Enhancement, + Some(tx_ref) + )?; + } + } + + notify_tx_retrieved(wdb.conn.0, d_tx.tx().txid())?; + Ok(()) }) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index f41ec47798..b963d6631a 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1858,6 +1858,8 @@ pub(crate) fn store_transaction_to_be_sent( Some(sent_tx.created()), )?; + let mut detectable_via_scanning = false; + // Mark notes as spent. // // This locks the notes so they aren't selected again by a subsequent call to @@ -1867,14 +1869,18 @@ pub(crate) fn store_transaction_to_be_sent( // Assumes that create_spend_to_address() will never be called in parallel, which is a // reasonable assumption for a light client such as a mobile phone. if let Some(bundle) = sent_tx.tx().sapling_bundle() { + detectable_via_scanning = true; for spend in bundle.shielded_spends() { sapling::mark_sapling_note_spent(wdb.conn.0, tx_ref, spend.nullifier())?; } } if let Some(_bundle) = sent_tx.tx().orchard_bundle() { #[cfg(feature = "orchard")] - for action in _bundle.actions() { - orchard::mark_orchard_note_spent(wdb.conn.0, tx_ref, action.nullifier())?; + { + detectable_via_scanning = true; + for action in _bundle.actions() { + orchard::mark_orchard_note_spent(wdb.conn.0, tx_ref, action.nullifier())?; + } } #[cfg(not(feature = "orchard"))] @@ -1965,6 +1971,18 @@ pub(crate) fn store_transaction_to_be_sent( } } + // Add the transaction to the set to be queried for transaction status. This is only necessary + // at present for fully-transparent transactions, because any transaction with a shielded + // component will be detected via ordinary chain scanning and/or nullifier checking. + if !detectable_via_scanning { + queue_tx_retrieval( + wdb.conn.0, + std::iter::once(sent_tx.tx().txid()), + TxQueryType::Status, + None, + )?; + } + Ok(()) } @@ -2300,6 +2318,76 @@ pub(crate) fn put_tx_data( .map_err(SqliteClientError::from) } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum TxQueryType { + Status, + Enhancement, +} + +impl TxQueryType { + pub(crate) fn code(&self) -> i64 { + match self { + TxQueryType::Status => 0, + TxQueryType::Enhancement => 1, + } + } + + pub(crate) fn from_code(code: i64) -> Option { + match code { + 0 => Some(TxQueryType::Status), + 1 => Some(TxQueryType::Enhancement), + _ => None, + } + } +} + +pub(crate) fn queue_tx_retrieval( + conn: &rusqlite::Transaction<'_>, + txids: impl Iterator, + query_type: TxQueryType, + dependent_tx_ref: Option, +) -> Result<(), SqliteClientError> { + // Add an entry to the transaction retrieval queue if we don't already have raw transaction data. + let mut stmt_insert_tx = conn.prepare_cached( + "INSERT INTO tx_retrieval_queue (txid, query_type, dependent_transaction_id) + SELECT :txid, :query_type, :dependent_transaction_id + -- do not queue enhancement requests if we already have the raw transaction + WHERE NOT EXISTS ( + SELECT 1 FROM transactions + WHERE txid = :txid + AND :query_type = :enhancement_type + AND raw IS NOT NULL + ) + -- if there is already a status request, we can upgrade it to an enhancement request + ON CONFLICT (txid) DO UPDATE + SET query_type = MAX(:query_type, query_type), + dependent_transaction_id = IFNULL(:dependent_transaction_id, dependent_transaction_id)", + )?; + + for txid in txids { + stmt_insert_tx.execute(named_params! { + ":txid": txid.as_ref(), + ":query_type": query_type.code(), + ":dependent_transaction_id": dependent_tx_ref.map(|r| r.0), + ":enhancement_type": TxQueryType::Enhancement.code(), + })?; + } + + Ok(()) +} + +pub(crate) fn notify_tx_retrieved( + conn: &rusqlite::Connection, + txid: TxId, +) -> Result<(), SqliteClientError> { + conn.execute( + "DELETE FROM tx_retrieval_queue WHERE txid = :txid", + named_params![":txid": &txid.as_ref()[..]], + )?; + + Ok(()) +} + // A utility function for creation of parameters for use in `insert_sent_output` // and `put_sent_output` fn recipient_params( diff --git a/zcash_client_sqlite/src/wallet/db.rs b/zcash_client_sqlite/src/wallet/db.rs index e713fb965a..0b0f4613cc 100644 --- a/zcash_client_sqlite/src/wallet/db.rs +++ b/zcash_client_sqlite/src/wallet/db.rs @@ -390,6 +390,25 @@ pub(super) const INDEX_SENT_NOTES_TO_ACCOUNT: &str = r#"CREATE INDEX sent_notes_to_account ON "sent_notes" (to_account_id)"#; pub(super) const INDEX_SENT_NOTES_TX: &str = r#"CREATE INDEX sent_notes_tx ON "sent_notes" (tx)"#; +/// Stores the set of transaction ids for which the backend required additional data. +/// +/// ### Columns: +/// - `txid`: The transaction identifier for the transaction to retrieve state information for. +/// - `query_type`: +/// - `0` for raw transaction (enhancement) data, +/// - `1` for transaction mined-ness information. +/// - `dependent_transaction_id`: If the transaction data request is searching for information +/// about transparent inputs to a transaction, this is a reference to that transaction record. +/// NULL for transactions where the request for enhancement data is based on discovery due +/// to blockchain scanning. +pub(super) const TABLE_TX_RETRIEVAL_QUEUE: &str = r#" +CREATE TABLE tx_retrieval_queue ( + txid BLOB NOT NULL UNIQUE, + query_type INTEGER NOT NULL, + dependent_transaction_id INTEGER, + FOREIGN KEY (dependent_transaction_id) REFERENCES transactions(id_tx) +)"#; + // // State for shard trees // diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index ea6c935c2a..c63a1591e5 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -407,6 +407,7 @@ mod tests { db::TABLE_TRANSPARENT_RECEIVED_OUTPUT_SPENDS, db::TABLE_TRANSPARENT_RECEIVED_OUTPUTS, db::TABLE_TX_LOCATOR_MAP, + db::TABLE_TX_RETRIEVAL_QUEUE, ]; let rows = describe_tables(&st.wallet().conn).unwrap(); diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index ad533c025e..43bd77d872 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -15,6 +15,7 @@ mod sapling_memo_consistency; mod sent_notes_to_internal; mod shardtree_support; mod spend_key_available; +mod tx_retrieval_queue; mod ufvk_support; mod utxos_table; mod utxos_to_txos; @@ -68,8 +69,8 @@ pub(super) fn all_migrations( // orchard_received_notes spend_key_available // / \ // ensure_orchard_ua_receiver utxos_to_txos - // | - // ephemeral_addresses + // / \ + // ephemeral_addresses tx_retrieval_queue vec![ Box::new(initial_setup::Migration {}), Box::new(utxos_table::Migration {}), @@ -124,6 +125,7 @@ pub(super) fn all_migrations( params: params.clone(), }), Box::new(spend_key_available::Migration), + Box::new(tx_retrieval_queue::Migration), ] } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs new file mode 100644 index 0000000000..1ce330babc --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs @@ -0,0 +1,61 @@ +//! A migration to add the `tx_retrieval_queue` table to the database. + +use rusqlite::Transaction; +use schemer_rusqlite::RusqliteMigration; +use std::collections::HashSet; +use uuid::Uuid; + +use crate::wallet::init::WalletMigrationError; + +use super::utxos_to_txos; + +pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xfec02b61_3988_4b4f_9699_98977fac9e7f); + +pub(crate) struct Migration; + +impl schemer::Migration for Migration { + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + [utxos_to_txos::MIGRATION_ID].into_iter().collect() + } + + fn description(&self) -> &'static str { + "Adds a table for tracking transactions to be downloaded for transparent output and/or memo retrieval." + } +} + +impl RusqliteMigration for Migration { + type Error = WalletMigrationError; + + fn up(&self, transaction: &Transaction) -> Result<(), WalletMigrationError> { + transaction.execute_batch( + "CREATE TABLE tx_retrieval_queue ( + txid BLOB NOT NULL UNIQUE, + query_type INTEGER NOT NULL, + dependent_transaction_id INTEGER, + FOREIGN KEY (dependent_transaction_id) REFERENCES transactions(id_tx) + );", + )?; + + Ok(()) + } + + fn down(&self, transaction: &Transaction) -> Result<(), WalletMigrationError> { + transaction.execute_batch("DROP TABLE tx_retrieval_queue;")?; + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use crate::wallet::init::migrations::tests::test_migrate; + + #[test] + fn migrate() { + test_migrate(&[super::MIGRATION_ID]); + } +} diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 4f0d95b9bb..a83967a25b 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -228,8 +228,7 @@ pub(crate) fn get_wallet_transparent_output( OR tx.expiry_height >= :mempool_height -- the spending tx has not yet expired ) ) - ) - ", + )", )?; let result: Result, SqliteClientError> = stmt_select_utxo diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index ddd5850c71..9d8db7bae9 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -20,6 +20,7 @@ and this library adheres to Rust's notion of - `InputSize` - `InputView::serialized_size` - `OutputView::serialized_size` +- `zcash_primitives::transaction::component::transparent::OutPoint::txid` ### Changed - MSRV is now 1.70.0. diff --git a/zcash_primitives/src/transaction/components/transparent.rs b/zcash_primitives/src/transaction/components/transparent.rs index 1c140f4f1b..e2c82fd848 100644 --- a/zcash_primitives/src/transaction/components/transparent.rs +++ b/zcash_primitives/src/transaction/components/transparent.rs @@ -142,10 +142,15 @@ impl OutPoint { self.n } - /// Returns the txid of the transaction containing this `OutPoint`. + /// Returns the byte representation of the txid of the transaction containing this `OutPoint`. pub fn hash(&self) -> &[u8; 32] { self.hash.as_ref() } + + /// Returns the txid of the transaction containing this `OutPoint`. + pub fn txid(&self) -> &TxId { + &self.hash + } } #[derive(Debug, Clone, PartialEq)] From 18207883ea7706dbdabf1cd02f8adeb7916273f9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 31 Jul 2024 20:10:29 -0600 Subject: [PATCH 03/18] zcash_client_backend: Add `block_height` argument to `decrypt_and_store_transaction` In the case that `decrypt_and_store_transaction` is being used to add data for fully-transparent transactions to the database, it may be the case that there is no existing relationship between a transaction and the block height at which it was mined (unlike for transactions being enhanced after discovery via block scanning.) This change makes it possible to set the mined height for transactions that do not have any shielded components that involve the wallet. --- zcash_client_backend/CHANGELOG.md | 4 ++++ zcash_client_backend/proto/service.proto | 4 ++++ zcash_client_backend/src/data_api/wallet.rs | 13 ++++++++----- zcash_client_backend/src/proto/service.rs | 4 ++++ zcash_client_sqlite/src/testing/pool.rs | 2 +- 5 files changed, 21 insertions(+), 6 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 5b7d5e007c..3d14d69326 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -75,6 +75,10 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail `zcash_client_sqlite`) to improve transactionality of writes for multi-step proposals. - `wallet::input_selection::InputSelectorError` has a new `Address` variant. + - `wallet::decrypt_and_store_transaction` now takes an additional optional + `mined_height` argument that can be used to provide the mined height + returned by the light wallet server in a `RawTransaction` value directly to + the back end. - `DecryptedTransaction::new` takes an additional `mined_height` argument. - `SentTransaction` now stores its `outputs` and `utxos_spent` fields as references to slices, with a corresponding change to `SentTransaction::new`. diff --git a/zcash_client_backend/proto/service.proto b/zcash_client_backend/proto/service.proto index 0945661478..2f11c25a11 100644 --- a/zcash_client_backend/proto/service.proto +++ b/zcash_client_backend/proto/service.proto @@ -34,6 +34,10 @@ message TxFilter { // RawTransaction contains the complete transaction data. It also optionally includes // the block height in which the transaction was included, or, when returned // by GetMempoolStream(), the latest block height. +// +// FIXME: the documentation here about mempool status contradicts the documentation +// for the `height` field (which I hope is correct, and this doesn't give the latest +// block height for mempool transactions.) message RawTransaction { bytes data = 1; // exact data returned by Zcash 'getrawtransaction' uint64 height = 2; // height that the transaction was mined (or -1) diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index ab2ca1189e..4508d0d9ec 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -92,6 +92,7 @@ pub fn decrypt_and_store_transaction( params: &ParamsT, data: &mut DbT, tx: &Transaction, + mined_height: Option, ) -> Result<(), DbT::Error> where ParamsT: consensus::Parameters, @@ -102,11 +103,13 @@ where // Height is block height for mined transactions, and the "mempool height" (chain height + 1) // for mempool transactions. - let height = data - .get_tx_height(tx.txid())? - .or(data.chain_height()?.map(|max_height| max_height + 1)) - .or_else(|| params.activation_height(NetworkUpgrade::Sapling)) - .expect("Sapling activation height must be known."); + let height = mined_height.map(Ok).unwrap_or_else(|| { + Ok(data + .get_tx_height(tx.txid())? + .or(data.chain_height()?.map(|max_height| max_height + 1)) + .or_else(|| params.activation_height(NetworkUpgrade::Sapling)) + .expect("Sapling activation height must be known.")) + })?; data.store_decrypted_tx(decrypt_transaction(params, height, tx, &ufvks))?; diff --git a/zcash_client_backend/src/proto/service.rs b/zcash_client_backend/src/proto/service.rs index 46314ad57c..85652abc39 100644 --- a/zcash_client_backend/src/proto/service.rs +++ b/zcash_client_backend/src/proto/service.rs @@ -38,6 +38,10 @@ pub struct TxFilter { /// RawTransaction contains the complete transaction data. It also optionally includes /// the block height in which the transaction was included, or, when returned /// by GetMempoolStream(), the latest block height. +/// +/// FIXME: the documentation here about mempool status contradicts the documentation +/// for the `height` field (which I hope is correct, and this doesn't give the latest +/// block height for mempool transactions.) #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct RawTransaction { diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 43e9768ebe..16b718da3c 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -295,7 +295,7 @@ pub(crate) fn send_single_step_proposed_transfer() { assert_eq!(tx_history.len(), 2); assert_matches!( - decrypt_and_store_transaction(&st.network(), st.wallet_mut(), &tx), + decrypt_and_store_transaction(&st.network(), st.wallet_mut(), &tx, None), Ok(_) ); } From 7fbc6568aae53293089235f49e26c0c8fd413fe7 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 31 Jul 2024 20:24:51 -0600 Subject: [PATCH 04/18] zcash_client_sqlite: Ensure that max_observed_unspent height is set correctly. --- zcash_client_sqlite/src/lib.rs | 3 +- zcash_client_sqlite/src/wallet.rs | 1 + zcash_client_sqlite/src/wallet/transparent.rs | 43 +++++++++++++++++-- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index cef2e8a9f1..e62550d3c3 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -1423,7 +1423,8 @@ impl WalletWrite for WalletDb txout, d_tx.mined_height(), &address, - account_id + account_id, + false )?; // Since the wallet created the transparent output, we need to ensure diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index b963d6631a..db63511010 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1960,6 +1960,7 @@ pub(crate) fn store_transaction_to_be_sent( None, ephemeral_address, *receiving_account, + true, )?; transparent::ephemeral::mark_ephemeral_address_as_used( wdb, diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index a83967a25b..d295369ed0 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -468,6 +468,7 @@ pub(crate) fn put_received_transparent_utxo( output.mined_height(), address, receiving_account, + true, ) } else { // The UTXO was not for any of our transparent addresses. @@ -573,6 +574,7 @@ pub(crate) fn find_account_for_transparent_address( /// /// `output_height` may be None if this is an ephemeral output from a /// transaction we created, that we do not yet know to have been mined. +#[allow(clippy::too_many_arguments)] pub(crate) fn put_transparent_output( conn: &rusqlite::Connection, params: &P, @@ -581,6 +583,7 @@ pub(crate) fn put_transparent_output( output_height: Option, address: &TransparentAddress, receiving_account: AccountId, + known_unspent: bool, ) -> Result { let output_height = output_height.map(u32::from); @@ -613,6 +616,40 @@ pub(crate) fn put_transparent_output( |row| row.get::<_, i64>(0), )?; + let spent_height = conn + .query_row( + "SELECT t.mined_height + FROM transactions t + JOIN transparent_received_output_spends ts ON ts.transaction_id = t.id_tx + JOIN transparent_received_outputs tro ON tro.id = ts.transparent_received_output_id + WHERE tro.transaction_id = :transaction_id + AND tro.output_index = :output_index", + named_params![ + ":transaction_id": id_tx, + ":output_index": &outpoint.n(), + ], + |row| { + row.get::<_, Option>(0) + .map(|o| o.map(BlockHeight::from)) + }, + ) + .optional()? + .flatten(); + + // The max observed unspent height is either the spending transaction's mined height - 1, or + // the current chain tip height if the UTXO was received via a path that confirmed that it was + // unspent, such as by querying the UTXO set of the network. + let max_observed_unspent = match spent_height { + Some(h) => Some(h - 1), + None => { + if known_unspent { + chain_tip_height(conn)? + } else { + None + } + } + }; + let mut stmt_upsert_transparent_output = conn.prepare_cached( "INSERT INTO transparent_received_outputs ( transaction_id, output_index, @@ -622,14 +659,14 @@ pub(crate) fn put_transparent_output( VALUES ( :transaction_id, :output_index, :account_id, :address, :script, - :value_zat, :height + :value_zat, :max_observed_unspent_height ) ON CONFLICT (transaction_id, output_index) DO UPDATE SET account_id = :account_id, address = :address, script = :script, value_zat = :value_zat, - max_observed_unspent_height = :height + max_observed_unspent_height = IFNULL(:max_observed_unspent_height, max_observed_unspent_height) RETURNING id", )?; @@ -640,7 +677,7 @@ pub(crate) fn put_transparent_output( ":address": &address.encode(params), ":script": &txout.script_pubkey.0, ":value_zat": &i64::from(Amount::from(txout.value)), - ":height": output_height, + ":max_observed_unspent_height": max_observed_unspent.map(u32::from), ]; let utxo_id = stmt_upsert_transparent_output From 3da29d25060f499c1532ec5eeb0a77ffdb3930a0 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 31 Jul 2024 20:33:20 -0600 Subject: [PATCH 05/18] zcash_client_backend: Add transaction data requests. This adds a mechanism that can be used by the backend to request additional transaction data or status information from the client, for the purpose of being to explore transparent transaction history that is otherwise currently unavailable. --- zcash_client_backend/CHANGELOG.md | 6 +- zcash_client_backend/src/data_api.rs | 98 ++++++++++++++++++- zcash_client_sqlite/src/lib.rs | 19 +++- zcash_client_sqlite/src/wallet.rs | 84 +++++++++++++++- zcash_client_sqlite/src/wallet/transparent.rs | 4 +- 5 files changed, 199 insertions(+), 12 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 3d14d69326..5010448c04 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -28,6 +28,8 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `chain::BlockCache` trait, behind the `sync` feature flag. - `WalletRead::get_spendable_transparent_outputs` - `DecryptedTransaction::mined_height` + - `TransactionDataRequest` + - `TransactionStatus` - `zcash_client_backend::fees`: - `EphemeralBalance` - `ChangeValue::shielded, is_ephemeral` @@ -61,12 +63,14 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail have changed as a consequence of this extraction; please see the `zip321` CHANGELOG for details. - `zcash_client_backend::data_api`: + - `WalletRead` has a new `transaction_data_requests` method. - `WalletRead` has new `get_known_ephemeral_addresses`, `find_account_for_ephemeral_address`, and `get_transparent_address_metadata` methods when the "transparent-inputs" feature is enabled. - `WalletWrite` has a new `reserve_next_n_ephemeral_addresses` method when the "transparent-inputs" feature is enabled. - - `WalletWrite` has new methods `import_account_hd` and `import_account_ufvk`. + - `WalletWrite` has new methods `import_account_hd`, `import_account_ufvk`, + and `set_transaction_status`. - `error::Error` has new `Address` and (when the "transparent-inputs" feature is enabled) `PaysEphemeralTransparentAddress` variants. - The `WalletWrite::store_sent_tx` method has been renamed to diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index b64a96d11e..16cbea4f80 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -541,6 +541,49 @@ pub struct SpendableNotes { orchard: Vec>, } +/// A request for transaction data enhancement, spentness check, or discovery +/// of spends from a given transparent address within a specific block range. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum TransactionDataRequest { + /// Information about the chain's view of a transaction is requested. + /// + /// The caller evaluating this request on behalf of the wallet backend should respond to this + /// request by determining the status of the specified transaction with respect to the main + /// chain; if using `lightwalletd` for access to chain data, this may be performed interpreting + /// the results of the [`GetTransaction`] RPC method. It should then call + /// [`WalletWrite::set_transaction_status`] to provide the resulting transaction status + /// information to the wallet backend. + /// + /// [`GetTransaction`]: crate::proto::service::compact_tx_streamer_client::CompactTxStreamerClient::get_transaction + GetStatus(TxId), + /// Transaction enhancement (download of complete raw transaction data) is requested. + /// + /// The caller evaluating this request on behalf of the wallet backend should respond to this + /// request by providing complete data for the specified transaction to + /// [`wallet::decrypt_and_store_transaction`]; if using `lightwalletd` for access to chain + /// state, this may be obtained via the [`GetTransaction`] RPC method. If no data is available + /// for the specified transaction, this should be reported to the backend using + /// [`WalletWrite::set_transaction_status`]. A [`TransactionDataRequest::Enhancement`] request + /// subsumes any previously existing [`TransactionDataRequest::GetStatus`] request. + /// + /// [`GetTransaction`]: crate::proto::service::compact_tx_streamer_client::CompactTxStreamerClient::get_transaction + Enhancement(TxId), +} + +/// Metadata about the status of a transaction obtained by inspecting the chain state. +#[derive(Clone, Copy, Debug)] +pub enum TransactionStatus { + /// The requested transaction ID was not recognized by the node. + TxidNotRecognized, + /// The requested transaction ID corresponds to a transaction that is recognized by the node, + /// but is in the mempool or is otherwise not mined in the main chain (but may have been mined + /// on a fork that was reorged away). + NotInMainChain, + /// The requested transaction ID corresponds to a transaction that has been included in the + /// block at the provided height. + Mined(BlockHeight), +} + impl SpendableNotes { /// Construct a new empty [`SpendableNotes`]. pub fn empty() -> Self { @@ -1039,6 +1082,20 @@ pub trait WalletRead { } Ok(None) } + + /// Returns a vector of [`TransactionDataRequest`] values that describe information needed by + /// the wallet to complete its view of transaction history. + /// + /// Requests for the same transaction data may be returned repeatedly by successive data + /// requests. The caller of this method should consider the latest set of requests returned + /// by this method to be authoritative and to subsume that returned by previous calls. + /// + /// Callers should poll this method on a regular interval, not as part of ordinary chain + /// scanning, which already produces requests for transaction data enhancement. Note that + /// responding to a set of transaction data requests may result in the creation of new + /// transaction data requests, such as when it is necessary to fill in purely-transparent + /// transaction history by walking the chain backwards via transparent inputs. + fn transaction_data_requests(&self) -> Result, Self::Error>; } /// The relevance of a seed to a given wallet. @@ -1811,9 +1868,27 @@ pub trait WalletWrite: WalletRead { #[cfg(feature = "transparent-inputs")] fn reserve_next_n_ephemeral_addresses( &mut self, - account_id: Self::AccountId, - n: usize, - ) -> Result, Self::Error>; + _account_id: Self::AccountId, + _n: usize, + ) -> Result, Self::Error> { + // Default impl is required for feature-flagged trait methods to prevent + // breakage due to inadvertent activation of features by transitive dependencies + // of the implementing crate. + Ok(vec![]) + } + + /// Updates the wallet backend with respect to the status of a specific transaction, from the + /// perspective of the main chain. + /// + /// Fully transparent transactions, and transactions that do not contain either shielded inputs + /// or shielded outputs belonging to the wallet, may not be discovered by the process of chain + /// scanning; as a consequence, the wallet must actively query to determine whether such + /// transactions have been mined. + fn set_transaction_status( + &mut self, + _txid: TxId, + _status: TransactionStatus, + ) -> Result<(), Self::Error>; } /// This trait describes a capability for manipulating wallet note commitment trees. @@ -1913,8 +1988,9 @@ pub mod testing { chain::{ChainState, CommitmentTreeRoot}, scanning::ScanRange, AccountBirthday, BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, - ScannedBlock, SeedRelevance, SentTransaction, SpendableNotes, WalletCommitmentTrees, - WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, + ScannedBlock, SeedRelevance, SentTransaction, SpendableNotes, TransactionDataRequest, + TransactionStatus, WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, + SAPLING_SHARD_HEIGHT, }; #[cfg(feature = "transparent-inputs")] @@ -2170,6 +2246,10 @@ pub mod testing { ) -> Result, Self::Error> { Ok(None) } + + fn transaction_data_requests(&self) -> Result, Self::Error> { + Ok(vec![]) + } } impl WalletWrite for MockWalletDb { @@ -2259,6 +2339,14 @@ pub mod testing { ) -> Result, Self::Error> { Err(()) } + + fn set_transaction_status( + &mut self, + _txid: TxId, + _status: TransactionStatus, + ) -> Result<(), Self::Error> { + Ok(()) + } } impl WalletCommitmentTrees for MockWalletDb { diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index e62550d3c3..12a28224f7 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -52,7 +52,8 @@ use zcash_client_backend::{ scanning::{ScanPriority, ScanRange}, Account, AccountBirthday, AccountSource, BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance, SentTransaction, SpendableNotes, - WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, + TransactionDataRequest, WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, + SAPLING_SHARD_HEIGHT, }, keys::{ AddressGenerationError, UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey, @@ -590,6 +591,14 @@ impl, P: consensus::Parameters> WalletRead for W &address.encode(&self.params), ) } + + fn transaction_data_requests(&self) -> Result, Self::Error> { + let iter = std::iter::empty(); + + let iter = iter.chain(wallet::transaction_data_requests(self.conn.borrow())?.into_iter()); + + Ok(iter.collect()) + } } impl WalletWrite for WalletDb { @@ -1539,6 +1548,14 @@ impl WalletWrite for WalletDb wallet::transparent::ephemeral::reserve_next_n_ephemeral_addresses(wdb, account_id, n) }) } + + fn set_transaction_status( + &mut self, + txid: TxId, + status: data_api::TransactionStatus, + ) -> Result<(), Self::Error> { + self.transactionally(|wdb| wallet::set_transaction_status(wdb.conn.0, txid, status)) + } } impl WalletCommitmentTrees for WalletDb { diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index db63511010..90cfaf058e 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -68,6 +68,7 @@ use incrementalmerkletree::{Marking, Retention}; use rusqlite::{self, named_params, params, OptionalExtension}; use secrecy::{ExposeSecret, SecretVec}; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; +use zcash_client_backend::data_api::{TransactionDataRequest, TransactionStatus}; use zip32::fingerprint::SeedFingerprint; use std::collections::{HashMap, HashSet}; @@ -108,13 +109,13 @@ use zcash_primitives::{ }; use zip32::{self, DiversifierIndex, Scope}; -use crate::TxRef; use crate::{ error::SqliteClientError, wallet::commitment_tree::{get_max_checkpointed_height, SqliteShardStore}, AccountId, SqlTransaction, TransferType, WalletCommitmentTrees, WalletDb, DEFAULT_UA_REQUEST, PRUNING_DEPTH, SAPLING_TABLES_PREFIX, }; +use crate::{TxRef, VERIFY_LOOKAHEAD}; #[cfg(feature = "transparent-inputs")] use zcash_primitives::transaction::components::TxOut; @@ -1760,7 +1761,7 @@ pub(crate) fn get_tx_height( ) -> Result, rusqlite::Error> { conn.query_row( "SELECT block FROM transactions WHERE txid = ?", - [txid.as_ref().to_vec()], + [txid.as_ref()], |row| Ok(row.get::<_, Option>(0)?.map(BlockHeight::from)), ) .optional() @@ -1987,6 +1988,58 @@ pub(crate) fn store_transaction_to_be_sent( Ok(()) } +pub(crate) fn set_transaction_status( + conn: &rusqlite::Transaction, + txid: TxId, + status: TransactionStatus, +) -> Result<(), SqliteClientError> { + match status { + TransactionStatus::TxidNotRecognized | TransactionStatus::NotInMainChain => { + // If the transaction is now expired, remove it from the retrieval queue. + if let Some(chain_tip) = chain_tip_height(conn)? { + conn.execute( + "DELETE FROM tx_retrieval_queue WHERE txid IN ( + SELECT txid FROM transactions + WHERE txid = :txid AND expiry_height < :chain_tip_minus_lookahead + )", + named_params![ + ":txid": txid.as_ref(), + ":chain_tip_minus_lookahead": u32::from(chain_tip).saturating_sub(VERIFY_LOOKAHEAD) + ], + )?; + } + } + TransactionStatus::Mined(height) => { + // The transaction has been mined, so we can set its mined height, associate it with + // the appropriate block, and remove it from the retrieval queue. + let sql_args = named_params![ + ":txid": txid.as_ref(), + ":height": u32::from(height) + ]; + + conn.execute( + "UPDATE transactions + SET mined_height = :height + WHERE txid = :txid", + sql_args, + )?; + + conn.execute( + "UPDATE transactions + SET block = blocks.height + FROM blocks + WHERE txid = :txid + AND blocks.height = :height", + sql_args, + )?; + + notify_tx_retrieved(conn, txid)?; + } + } + + Ok(()) +} + /// Truncates the database to the given height. /// /// If the requested height is greater than or equal to the height of the last scanned @@ -2377,6 +2430,33 @@ pub(crate) fn queue_tx_retrieval( Ok(()) } +/// Returns the vector of [`TransactionDataRequest`]s that represents the information needed by the +/// wallet backend in order to be able to present a complete view of wallet history and memo data. +pub(crate) fn transaction_data_requests( + conn: &rusqlite::Connection, +) -> Result, SqliteClientError> { + let mut tx_retrieval_stmt = + conn.prepare_cached("SELECT txid, query_type FROM tx_retrieval_queue")?; + + let result = tx_retrieval_stmt + .query_and_then([], |row| { + let txid = row.get(0).map(TxId::from_bytes)?; + let query_type = row.get(1).map(TxQueryType::from_code)?.ok_or_else(|| { + SqliteClientError::CorruptedData( + "Unrecognized transaction data request type.".to_owned(), + ) + })?; + + Ok::(match query_type { + TxQueryType::Status => TransactionDataRequest::GetStatus(txid), + TxQueryType::Enhancement => TransactionDataRequest::Enhancement(txid), + }) + })? + .collect::, _>>()?; + + Ok(result) +} + pub(crate) fn notify_tx_retrieved( conn: &rusqlite::Connection, txid: TxId, diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index d295369ed0..275456d043 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -20,10 +20,8 @@ use zcash_primitives::{ }; use zcash_protocol::consensus::{self, BlockHeight}; -use crate::TxRef; -use crate::{error::SqliteClientError, AccountId, UtxoId}; - use super::{chain_tip_height, get_account_ids}; +use crate::{error::SqliteClientError, AccountId, TxRef, UtxoId}; pub(crate) mod ephemeral; From 69828bc0d0b7559a5f20c264b755d9564035c0f5 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 1 Aug 2024 16:40:35 -0600 Subject: [PATCH 06/18] zcash_client_sqlite: Add `target_height` column to `transactions` table. --- zcash_client_sqlite/src/lib.rs | 2 +- zcash_client_sqlite/src/wallet.rs | 7 +++- zcash_client_sqlite/src/wallet/db.rs | 6 +++ .../init/migrations/receiving_key_scopes.rs | 40 ++++++++++++++++++- .../init/migrations/tx_retrieval_queue.rs | 23 +++++++++-- zcash_primitives/CHANGELOG.md | 1 + zcash_primitives/src/transaction/builder.rs | 2 +- 7 files changed, 72 insertions(+), 9 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 12a28224f7..7637a45b19 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -1185,7 +1185,7 @@ impl WalletWrite for WalletDb d_tx: DecryptedTransaction, ) -> Result<(), Self::Error> { self.transactionally(|wdb| { - let tx_ref = wallet::put_tx_data(wdb.conn.0, d_tx.tx(), None, None)?; + let tx_ref = wallet::put_tx_data(wdb.conn.0, d_tx.tx(), None, None, None)?; let funding_accounts = wallet::get_funding_accounts(wdb.conn.0, d_tx.tx())?; // TODO(#1305): Correctly track accounts that fund each transaction output. diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 90cfaf058e..92b4ff9348 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1857,6 +1857,7 @@ pub(crate) fn store_transaction_to_be_sent( sent_tx.tx(), Some(sent_tx.fee_amount()), Some(sent_tx.created()), + chain_tip_height(wdb.conn.0)?.map(|h| h + 1), )?; let mut detectable_via_scanning = false; @@ -2344,10 +2345,11 @@ pub(crate) fn put_tx_data( tx: &Transaction, fee: Option, created_at: Option, + target_height: Option, ) -> Result { let mut stmt_upsert_tx_data = conn.prepare_cached( - "INSERT INTO transactions (txid, created, expiry_height, raw, fee) - VALUES (:txid, :created_at, :expiry_height, :raw, :fee) + "INSERT INTO transactions (txid, created, expiry_height, raw, fee, target_height) + VALUES (:txid, :created_at, :expiry_height, :raw, :fee, :target_height) ON CONFLICT (txid) DO UPDATE SET expiry_height = :expiry_height, raw = :raw, @@ -2365,6 +2367,7 @@ pub(crate) fn put_tx_data( ":expiry_height": u32::from(tx.expiry_height()), ":raw": raw_tx, ":fee": fee.map(u64::from), + ":target_height": target_height.map(u32::from), ]; stmt_upsert_tx_data diff --git a/zcash_client_sqlite/src/wallet/db.rs b/zcash_client_sqlite/src/wallet/db.rs index 0b0f4613cc..4c3715d4a8 100644 --- a/zcash_client_sqlite/src/wallet/db.rs +++ b/zcash_client_sqlite/src/wallet/db.rs @@ -174,6 +174,11 @@ CREATE TABLE blocks ( /// foreign key constraint on `block` prevents that column from being populated prior to complete /// scanning of the block. This is constrained to be equal to the `block` column if `block` is /// non-null. +/// - `target_height`: stores the target height for which the transaction was constructed, if +/// known. This will ordinarily be null for transactions discovered via chain scanning; it +/// will only be set for transactions created using this wallet specifically, and not any +/// other wallet that uses the same seed (including previous installations of the same +/// wallet application.) pub(super) const TABLE_TRANSACTIONS: &str = r#" CREATE TABLE "transactions" ( id_tx INTEGER PRIMARY KEY, @@ -185,6 +190,7 @@ CREATE TABLE "transactions" ( expiry_height INTEGER, raw BLOB, fee INTEGER, + target_height INTEGER, FOREIGN KEY (block) REFERENCES blocks(height), CONSTRAINT height_consistency CHECK (block IS NULL OR mined_height = block) )"#; diff --git a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs index 3389af518a..d270153838 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs @@ -307,6 +307,7 @@ mod tests { builder::{BuildConfig, BuildResult, Builder}, components::{amount::NonNegativeAmount, transparent}, fees::fixed, + Transaction, }, zip32::{self, Scope}, }; @@ -322,7 +323,7 @@ mod tests { memo_repr, parse_scope, sapling::ReceivedSaplingOutput, }, - AccountId, WalletDb, + AccountId, TxRef, WalletDb, }; // These must be different. @@ -471,6 +472,41 @@ mod tests { Ok(()) } + /// This reproduces [`crate::wallet::put_tx_data`] as it was at the time + /// of the creation of this migration. + fn put_tx_data( + conn: &rusqlite::Connection, + tx: &Transaction, + fee: Option, + created_at: Option, + ) -> Result { + let mut stmt_upsert_tx_data = conn.prepare_cached( + "INSERT INTO transactions (txid, created, expiry_height, raw, fee) + VALUES (:txid, :created_at, :expiry_height, :raw, :fee) + ON CONFLICT (txid) DO UPDATE + SET expiry_height = :expiry_height, + raw = :raw, + fee = IFNULL(:fee, fee) + RETURNING id_tx", + )?; + + let txid = tx.txid(); + let mut raw_tx = vec![]; + tx.write(&mut raw_tx)?; + + let tx_params = named_params![ + ":txid": &txid.as_ref()[..], + ":created_at": created_at, + ":expiry_height": u32::from(tx.expiry_height()), + ":raw": raw_tx, + ":fee": fee.map(u64::from), + ]; + + stmt_upsert_tx_data + .query_row(tx_params, |row| row.get::<_, i64>(0).map(TxRef)) + .map_err(SqliteClientError::from) + } + #[test] fn receiving_key_scopes_migration_enhanced() { let params = Network::TestNetwork; @@ -504,7 +540,7 @@ mod tests { db_data .transactionally::<_, _, rusqlite::Error>(|wdb| { - let tx_ref = crate::wallet::put_tx_data(wdb.conn.0, d_tx.tx(), None, None).unwrap(); + let tx_ref = put_tx_data(wdb.conn.0, d_tx.tx(), None, None).unwrap(); let mut spending_account_id: Option = None; diff --git a/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs index 1ce330babc..f8b66fdf1c 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs @@ -1,9 +1,10 @@ //! A migration to add the `tx_retrieval_queue` table to the database. -use rusqlite::Transaction; +use rusqlite::{named_params, Transaction}; use schemer_rusqlite::RusqliteMigration; use std::collections::HashSet; use uuid::Uuid; +use zcash_primitives::transaction::builder::DEFAULT_TX_EXPIRY_DELTA; use crate::wallet::init::WalletMigrationError; @@ -37,14 +38,30 @@ impl RusqliteMigration for Migration { query_type INTEGER NOT NULL, dependent_transaction_id INTEGER, FOREIGN KEY (dependent_transaction_id) REFERENCES transactions(id_tx) - );", + ); + + ALTER TABLE transactions ADD COLUMN target_height INTEGER;", + )?; + + // Add estimated target height information for each transaction we know to + // have been created by the wallet; transactions that were discovered via + // chain scanning will have their `created` field set to `NULL`. + transaction.execute( + "UPDATE transactions + SET target_height = expiry_height - :default_expiry_delta + WHERE expiry_height > :default_expiry_delta + AND created IS NOT NULL", + named_params![":default_expiry_delta": DEFAULT_TX_EXPIRY_DELTA], )?; Ok(()) } fn down(&self, transaction: &Transaction) -> Result<(), WalletMigrationError> { - transaction.execute_batch("DROP TABLE tx_retrieval_queue;")?; + transaction.execute_batch( + "ALTER TABLE transactions DROP COLUMN target_height; + DROP TABLE tx_retrieval_queue;", + )?; Ok(()) } diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 9d8db7bae9..1f1ccc99ed 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -21,6 +21,7 @@ and this library adheres to Rust's notion of - `InputView::serialized_size` - `OutputView::serialized_size` - `zcash_primitives::transaction::component::transparent::OutPoint::txid` +- `zcash_primitives::transaction::builder::DEFAULT_TX_EXPIRY_DELTA` ### Changed - MSRV is now 1.70.0. diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 47c0995028..b40655dc55 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -55,7 +55,7 @@ use super::components::sapling::zip212_enforcement; /// Since Blossom activation, the default transaction expiry delta should be 40 blocks. /// -const DEFAULT_TX_EXPIRY_DELTA: u32 = 40; +pub const DEFAULT_TX_EXPIRY_DELTA: u32 = 40; /// Errors that can occur during fee calculation. #[derive(Debug)] From a3109cfb784579ee89f77b670df236657bcb8f2a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 1 Aug 2024 16:44:44 -0600 Subject: [PATCH 07/18] zcash_client_sqlite: Add queue for transparent spend detection by address/outpoint. --- zcash_client_backend/src/data_api.rs | 22 +++++ zcash_client_sqlite/src/lib.rs | 18 ++++ zcash_client_sqlite/src/wallet/db.rs | 17 ++++ zcash_client_sqlite/src/wallet/init.rs | 1 + .../init/migrations/tx_retrieval_queue.rs | 13 ++- zcash_client_sqlite/src/wallet/transparent.rs | 89 +++++++++++++++++-- 6 files changed, 152 insertions(+), 8 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 16cbea4f80..dc55922cc3 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -568,6 +568,28 @@ pub enum TransactionDataRequest { /// /// [`GetTransaction`]: crate::proto::service::compact_tx_streamer_client::CompactTxStreamerClient::get_transaction Enhancement(TxId), + /// Information about transactions that receive or spend funds belonging to the specified + /// transparent address is requested. + /// + /// Fully transparent transactions, and transactions that do not contain either shielded inputs + /// or shielded outputs belonging to the wallet, may not be discovered by the process of chain + /// scanning; as a consequence, the wallet must actively query to find transactions that spend + /// such funds. Ideally we'd be able to query by [`OutPoint`] but this is not currently + /// functionality that is supported by the light wallet server. + /// + /// The caller evaluating this request on behalf of the wallet backend should respond to this + /// request by detecting transactions involving the specified address within the provided block + /// range; if using `lightwalletd` for access to chain data, this may be performed using the + /// [`GetTaddressTxids`] RPC method. It should then call [`wallet::decrypt_and_store_transaction`] + /// for each transaction so detected. + /// + /// [`GetTaddressTxids`]: crate::proto::service::compact_tx_streamer_client::CompactTxStreamerClient::get_taddress_txids + #[cfg(feature = "transparent-inputs")] + SpendsFromAddress { + address: TransparentAddress, + block_range_start: BlockHeight, + block_range_end: Option, + }, } /// Metadata about the status of a transaction obtained by inspecting the chain state. diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 7637a45b19..6ce88f0e52 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -596,6 +596,11 @@ impl, P: consensus::Parameters> WalletRead for W let iter = std::iter::empty(); let iter = iter.chain(wallet::transaction_data_requests(self.conn.borrow())?.into_iter()); + #[cfg(feature = "transparent-inputs")] + let iter = iter.chain( + wallet::transparent::transaction_data_requests(self.conn.borrow(), &self.params)? + .into_iter(), + ); Ok(iter.collect()) } @@ -1440,6 +1445,19 @@ impl WalletWrite for WalletDb // that any transparent inputs belonging to the wallet will be // discovered. tx_has_wallet_outputs = true; + + // When we receive transparent funds (particularly as ephemeral outputs + // in transaction pairs sending to a ZIP 320 address) it becomes + // possible that the spend of these outputs is not then later detected + // if the transaction that spends them is purely transparent. This is + // particularly a problem in wallet recovery. + wallet::transparent::queue_transparent_spend_detection( + wdb.conn.0, + &wdb.params, + address, + tx_ref, + output_index.try_into().unwrap() + )?; } // If a transaction we observe contains spends from our wallet, we will diff --git a/zcash_client_sqlite/src/wallet/db.rs b/zcash_client_sqlite/src/wallet/db.rs index 4c3715d4a8..44d44b9275 100644 --- a/zcash_client_sqlite/src/wallet/db.rs +++ b/zcash_client_sqlite/src/wallet/db.rs @@ -415,6 +415,23 @@ CREATE TABLE tx_retrieval_queue ( FOREIGN KEY (dependent_transaction_id) REFERENCES transactions(id_tx) )"#; +/// Stores the set of transaction outputs received by the wallet for which spend information +/// (if any) should be retrieved. +/// +/// This table is populated in the process of wallet recovery when a deshielding transaction +/// with transparent outputs belonging to the wallet (e.g., the deshielding half of a ZIP 320 +/// transaction pair) is discovered. It is expected that such a transparent output will be +/// spent soon after it is received in a purely transparent transaction, which the wallet +/// currently has no means of detecting otherwise. +pub(super) const TABLE_TRANSPARENT_SPEND_SEARCH_QUEUE: &str = r#" +CREATE TABLE transparent_spend_search_queue ( + address TEXT NOT NULL, + transaction_id INTEGER NOT NULL, + output_index INTEGER NOT NULL, + FOREIGN KEY (transaction_id) REFERENCES transactions(id_tx), + CONSTRAINT value_received_height UNIQUE (transaction_id, output_index) +)"#; + // // State for shard trees // diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index c63a1591e5..9832a04ef1 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -406,6 +406,7 @@ mod tests { db::TABLE_TRANSACTIONS, db::TABLE_TRANSPARENT_RECEIVED_OUTPUT_SPENDS, db::TABLE_TRANSPARENT_RECEIVED_OUTPUTS, + db::TABLE_TRANSPARENT_SPEND_SEARCH_QUEUE, db::TABLE_TX_LOCATOR_MAP, db::TABLE_TX_RETRIEVAL_QUEUE, ]; diff --git a/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs index f8b66fdf1c..d03c9d17f7 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs @@ -40,7 +40,15 @@ impl RusqliteMigration for Migration { FOREIGN KEY (dependent_transaction_id) REFERENCES transactions(id_tx) ); - ALTER TABLE transactions ADD COLUMN target_height INTEGER;", + ALTER TABLE transactions ADD COLUMN target_height INTEGER; + + CREATE TABLE transparent_spend_search_queue ( + address TEXT NOT NULL, + transaction_id INTEGER NOT NULL, + output_index INTEGER NOT NULL, + FOREIGN KEY (transaction_id) REFERENCES transactions(id_tx), + CONSTRAINT value_received_height UNIQUE (transaction_id, output_index) + );", )?; // Add estimated target height information for each transaction we know to @@ -59,7 +67,8 @@ impl RusqliteMigration for Migration { fn down(&self, transaction: &Transaction) -> Result<(), WalletMigrationError> { transaction.execute_batch( - "ALTER TABLE transactions DROP COLUMN target_height; + "DROP TABLE transparent_spend_search_queue; + ALTER TABLE transactions DROP COLUMN target_height; DROP TABLE tx_retrieval_queue;", )?; diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 275456d043..19433c6264 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -3,6 +3,8 @@ use std::collections::{HashMap, HashSet}; use rusqlite::OptionalExtension; use rusqlite::{named_params, Connection, Row}; +use zcash_client_backend::data_api::TransactionDataRequest; +use zcash_primitives::transaction::builder::DEFAULT_TX_EXPIRY_DELTA; use zip32::{DiversifierIndex, Scope}; use zcash_address::unified::{Encoding, Ivk, Uivk}; @@ -439,14 +441,26 @@ pub(crate) fn mark_transparent_utxo_spent( AND txo.output_index = :prevout_idx ON CONFLICT (transparent_received_output_id, transaction_id) DO NOTHING", )?; - - let sql_args = named_params![ + stmt_mark_transparent_utxo_spent.execute(named_params![ ":spent_in_tx": tx_ref.0, - ":prevout_txid": &outpoint.hash().to_vec(), - ":prevout_idx": &outpoint.n(), - ]; + ":prevout_txid": outpoint.hash().as_ref(), + ":prevout_idx": outpoint.n(), + ])?; + + // Since we know that the output is spent, we no longer need to search for + // it to find out if it has been spent. + let mut stmt_remove_spend_detection = conn.prepare_cached( + "DELETE FROM transparent_spend_search_queue + WHERE output_index = :prevout_idx + AND transaction_id IN ( + SELECT id_tx FROM transactions WHERE txid = :prevout_txid + )", + )?; + stmt_remove_spend_detection.execute(named_params![ + ":prevout_txid": outpoint.hash().as_ref(), + ":prevout_idx": outpoint.n(), + ])?; - stmt_mark_transparent_utxo_spent.execute(sql_args)?; Ok(()) } @@ -474,6 +488,42 @@ pub(crate) fn put_received_transparent_utxo( } } +/// Returns the vector of [`TransactionDataRequest`]s that represents the information needed by the +/// wallet backend in order to be able to present a complete view of wallet history and memo data. +pub(crate) fn transaction_data_requests( + conn: &rusqlite::Connection, + params: &P, +) -> Result, SqliteClientError> { + // We cannot construct transaction data requests for the case where we cannot determine the + // height at which to begin, so we require that either the target height or mined height be + // set. In practice, we should not encounter entries in `transparent_spend_search_queue` + // because under ordinary circumstances, it is populated via a call from + // `decrypt_and_store_transaction` on ordinary mined transaction data retrieved from the chain. + let mut address_request_stmt = conn.prepare_cached( + "SELECT ssq.address, IFNULL(t.target_height, t.mined_height + 1) + FROM transparent_spend_search_queue ssq + JOIN transactions t ON t.id_tx = ssq.transaction_id + WHERE t.target_height IS NOT NULL + OR t.mined_height IS NOT NULL", + )?; + + let result = address_request_stmt + .query_and_then([], |row| { + let address = TransparentAddress::decode(params, &row.get::<_, String>(0)?)?; + let block_range_start = BlockHeight::from(row.get::<_, u32>(1)?); + Ok::( + TransactionDataRequest::SpendsFromAddress { + address, + block_range_start, + block_range_end: Some(block_range_start + DEFAULT_TX_EXPIRY_DELTA), + }, + ) + })? + .collect::, _>>()?; + + Ok(result) +} + pub(crate) fn get_transparent_address_metadata( conn: &rusqlite::Connection, params: &P, @@ -683,6 +733,33 @@ pub(crate) fn put_transparent_output( Ok(utxo_id) } +pub(crate) fn queue_transparent_spend_detection( + conn: &rusqlite::Transaction<'_>, + params: &P, + receiving_address: TransparentAddress, + tx_ref: TxRef, + output_index: u32, +) -> Result<(), SqliteClientError> { + // Add an entry to the transaction retrieval queue if we don't already have raw transaction + // data. + let mut stmt = conn.prepare_cached( + "INSERT INTO transparent_spend_search_queue + (address, transaction_id, output_index) + VALUES + (:address, :transaction_id, :output_index) + ON CONFLICT (transaction_id, output_index) DO NOTHING", + )?; + + let addr_str = receiving_address.encode(params); + stmt.execute(named_params! { + ":address": addr_str, + ":transaction_id": tx_ref.0, + ":output_index": output_index + })?; + + Ok(()) +} + #[cfg(test)] mod tests { use crate::testing::{AddressType, TestBuilder, TestState}; From 52f7a77cae3d2ff4c0e92cead7a86c29e69a2312 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 7 Aug 2024 09:56:07 -0600 Subject: [PATCH 08/18] Apply suggestions from code review Co-authored-by: Daira-Emma Hopwood --- zcash_client_backend/proto/service.proto | 3 +-- zcash_client_backend/src/data_api.rs | 4 ++-- zcash_client_backend/src/proto/service.rs | 3 +-- zcash_client_sqlite/src/lib.rs | 6 +++--- zcash_client_sqlite/src/wallet.rs | 2 +- zcash_client_sqlite/src/wallet/transparent.rs | 2 +- 6 files changed, 9 insertions(+), 11 deletions(-) diff --git a/zcash_client_backend/proto/service.proto b/zcash_client_backend/proto/service.proto index 2f11c25a11..0a34be42a8 100644 --- a/zcash_client_backend/proto/service.proto +++ b/zcash_client_backend/proto/service.proto @@ -36,8 +36,7 @@ message TxFilter { // by GetMempoolStream(), the latest block height. // // FIXME: the documentation here about mempool status contradicts the documentation -// for the `height` field (which I hope is correct, and this doesn't give the latest -// block height for mempool transactions.) +// for the `height` field. See https://github.com/zcash/librustzcash/issues/1484 message RawTransaction { bytes data = 1; // exact data returned by Zcash 'getrawtransaction' uint64 height = 2; // height that the transaction was mined (or -1) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index dc55922cc3..47b26c4ef9 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -549,8 +549,8 @@ pub enum TransactionDataRequest { /// /// The caller evaluating this request on behalf of the wallet backend should respond to this /// request by determining the status of the specified transaction with respect to the main - /// chain; if using `lightwalletd` for access to chain data, this may be performed interpreting - /// the results of the [`GetTransaction`] RPC method. It should then call + /// chain; if using `lightwalletd` for access to chain data, this may be obtained by + /// interpreting the results of the [`GetTransaction`] RPC method. It should then call /// [`WalletWrite::set_transaction_status`] to provide the resulting transaction status /// information to the wallet backend. /// diff --git a/zcash_client_backend/src/proto/service.rs b/zcash_client_backend/src/proto/service.rs index 85652abc39..bf55bc163e 100644 --- a/zcash_client_backend/src/proto/service.rs +++ b/zcash_client_backend/src/proto/service.rs @@ -40,8 +40,7 @@ pub struct TxFilter { /// by GetMempoolStream(), the latest block height. /// /// FIXME: the documentation here about mempool status contradicts the documentation -/// for the `height` field (which I hope is correct, and this doesn't give the latest -/// block height for mempool transactions.) +/// for the `height` field. See #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct RawTransaction { diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 6ce88f0e52..3c5a8a482b 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -593,9 +593,8 @@ impl, P: consensus::Parameters> WalletRead for W } fn transaction_data_requests(&self) -> Result, Self::Error> { - let iter = std::iter::empty(); + let iter = wallet::transaction_data_requests(self.conn.borrow())?.into_iter(); - let iter = iter.chain(wallet::transaction_data_requests(self.conn.borrow())?.into_iter()); #[cfg(feature = "transparent-inputs")] let iter = iter.chain( wallet::transparent::transaction_data_requests(self.conn.borrow(), &self.params)? @@ -1211,6 +1210,7 @@ impl WalletWrite for WalletDb #[cfg(feature = "transparent-inputs")] let detectable_via_scanning = { + #[allow(unused_mut)] let mut detectable_via_scanning = d_tx.tx().sapling_bundle().is_some(); #[cfg(feature = "orchard")] { detectable_via_scanning |= d_tx.tx().orchard_bundle().is_some(); @@ -1450,7 +1450,7 @@ impl WalletWrite for WalletDb // in transaction pairs sending to a ZIP 320 address) it becomes // possible that the spend of these outputs is not then later detected // if the transaction that spends them is purely transparent. This is - // particularly a problem in wallet recovery. + // especially a problem in wallet recovery. wallet::transparent::queue_transparent_spend_detection( wdb.conn.0, &wdb.params, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 92b4ff9348..28341dbeae 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1975,7 +1975,7 @@ pub(crate) fn store_transaction_to_be_sent( } // Add the transaction to the set to be queried for transaction status. This is only necessary - // at present for fully-transparent transactions, because any transaction with a shielded + // at present for fully transparent transactions, because any transaction with a shielded // component will be detected via ordinary chain scanning and/or nullifier checking. if !detectable_via_scanning { queue_tx_retrieval( diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 19433c6264..1c2ab79dbe 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -500,7 +500,7 @@ pub(crate) fn transaction_data_requests( // because under ordinary circumstances, it is populated via a call from // `decrypt_and_store_transaction` on ordinary mined transaction data retrieved from the chain. let mut address_request_stmt = conn.prepare_cached( - "SELECT ssq.address, IFNULL(t.target_height, t.mined_height + 1) + "SELECT ssq.address, IFNULL(t.target_height, t.mined_height) FROM transparent_spend_search_queue ssq JOIN transactions t ON t.id_tx = ssq.transaction_id WHERE t.target_height IS NOT NULL From 366a3e3323d2e09112eda9c3491895c1e633699b Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Aug 2024 09:02:32 -0600 Subject: [PATCH 09/18] Apply suggestions from code review Co-authored-by: Daira-Emma Hopwood --- zcash_client_sqlite/src/lib.rs | 30 ++++++++++--------- zcash_client_sqlite/src/wallet.rs | 7 ++--- zcash_client_sqlite/src/wallet/transparent.rs | 10 +++++-- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 3c5a8a482b..139af897ac 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -1209,15 +1209,12 @@ impl WalletWrite for WalletDb let mut tx_has_wallet_outputs = false; #[cfg(feature = "transparent-inputs")] - let detectable_via_scanning = { - #[allow(unused_mut)] - let mut detectable_via_scanning = d_tx.tx().sapling_bundle().is_some(); - #[cfg(feature = "orchard")] { - detectable_via_scanning |= d_tx.tx().orchard_bundle().is_some(); - } + let detectable_via_scanning = d_tx.tx().sapling_bundle().is_some(); + #[cfg(all(feature = "transparent-inputs", feature = "orchard"))] + let detectable_via_scanning = detectable_via_scanning | d_tx.tx().orchard_bundle().is_some(); - detectable_via_scanning - }; + #[cfg(feature = "transparent-inputs")] + let mut queue_status_retrieval = false; for output in d_tx.sapling_outputs() { #[cfg(feature = "transparent-inputs")] @@ -1503,12 +1500,7 @@ impl WalletWrite for WalletDb // components, add it to the queue for status retrieval. #[cfg(feature = "transparent-inputs")] if d_tx.mined_height().is_none() && !detectable_via_scanning { - wallet::queue_tx_retrieval( - wdb.conn.0, - std::iter::once(d_tx.tx().txid()), - TxQueryType::Status, - None - )?; + queue_status_retrieval = true; } } } @@ -1534,6 +1526,16 @@ impl WalletWrite for WalletDb notify_tx_retrieved(wdb.conn.0, d_tx.tx().txid())?; + #[cfg(feature = "transparent-inputs")] + if queue_status_retrieval { + wallet::queue_tx_retrieval( + wdb.conn.0, + std::iter::once(d_tx.tx().txid()), + TxQueryType::Status, + None + )?; + } + Ok(()) }) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 28341dbeae..f646a5a133 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -2404,15 +2404,14 @@ pub(crate) fn queue_tx_retrieval( query_type: TxQueryType, dependent_tx_ref: Option, ) -> Result<(), SqliteClientError> { - // Add an entry to the transaction retrieval queue if we don't already have raw transaction data. + // Add an entry to the transaction retrieval queue if it would not be redundant. let mut stmt_insert_tx = conn.prepare_cached( "INSERT INTO tx_retrieval_queue (txid, query_type, dependent_transaction_id) SELECT :txid, :query_type, :dependent_transaction_id -- do not queue enhancement requests if we already have the raw transaction - WHERE NOT EXISTS ( + WHERE :query_type <> :enhancement_type OR NOT EXISTS ( SELECT 1 FROM transactions WHERE txid = :txid - AND :query_type = :enhancement_type AND raw IS NOT NULL ) -- if there is already a status request, we can upgrade it to an enhancement request @@ -2461,7 +2460,7 @@ pub(crate) fn transaction_data_requests( } pub(crate) fn notify_tx_retrieved( - conn: &rusqlite::Connection, + conn: &rusqlite::Transaction<'_>, txid: TxId, ) -> Result<(), SqliteClientError> { conn.execute( diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 1c2ab79dbe..6bc1e2af05 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -733,6 +733,14 @@ pub(crate) fn put_transparent_output( Ok(utxo_id) } +/// Adds a request to retrieve transactions involving the specified address to the transparent +/// spend search queue. Note that such requests are _not_ for data related to `tx_ref`, but instead +/// a request to find where the UTXO with the outpoint `(tx_ref, output_index)` is spent. +/// +/// ### Parameters +/// - `receiving_address`: The address that received the UTXO. +/// - `tx_ref`: The transaction in which the UTXO was received. +/// - `output_index`: The index of the output within `vout` of the specified transaction. pub(crate) fn queue_transparent_spend_detection( conn: &rusqlite::Transaction<'_>, params: &P, @@ -740,8 +748,6 @@ pub(crate) fn queue_transparent_spend_detection( tx_ref: TxRef, output_index: u32, ) -> Result<(), SqliteClientError> { - // Add an entry to the transaction retrieval queue if we don't already have raw transaction - // data. let mut stmt = conn.prepare_cached( "INSERT INTO transparent_spend_search_queue (address, transaction_id, output_index) From dada9804735b7030c290dc01e3bec53e0076f30c Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Aug 2024 11:36:34 -0600 Subject: [PATCH 10/18] zcash_client_sqlite: Add testing for transaction data request generation. --- zcash_client_sqlite/src/testing/pool.rs | 39 ++++++++++++++++++++----- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 16b718da3c..01a106c51a 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -306,6 +306,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { use rand_core::OsRng; use zcash_client_backend::{ + data_api::TransactionDataRequest, fees::ChangeValue, wallet::{TransparentAddressMetadata, WalletTx}, }; @@ -436,6 +437,13 @@ pub(crate) fn send_multi_step_proposed_transfer() { }) .collect(); + // Verify that a status request has been generated for the second transaction of + // the ZIP 320 pair. + let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); + assert!(tx_data_requests + .iter() + .any(|r| r == &TransactionDataRequest::GetStatus(*txids.last()))); + assert!(expected_step0_change < expected_ephemeral); assert_eq!(confirmed_sent.len(), 2); assert_eq!(confirmed_sent[0].len(), 2); @@ -572,6 +580,13 @@ pub(crate) fn send_multi_step_proposed_transfer() { script_pubkey: default_addr.script(), value, }; + // Add the fake input to our UTXO set so that we can ensure we recognize the outpoint. + st.wallet_mut() + .put_received_transparent_utxo( + &WalletTransparentOutput::from_parts(outpoint.clone(), txout.clone(), None).unwrap(), + ) + .unwrap(); + assert_matches!(builder.add_transparent_input(sk, outpoint, txout), Ok(_)); let test_prover = test_prover(); let build_result = builder @@ -583,14 +598,22 @@ pub(crate) fn send_multi_step_proposed_transfer() { ) .unwrap(); let txid = build_result.transaction().txid(); - let decrypted_tx = DecryptedTransaction::::new( - None, - build_result.transaction(), - vec![], - #[cfg(feature = "orchard")] - vec![], - ); - st.wallet_mut().store_decrypted_tx(decrypted_tx).unwrap(); + st.wallet_mut() + .store_decrypted_tx(DecryptedTransaction::::new( + None, + build_result.transaction(), + vec![], + #[cfg(feature = "orchard")] + vec![], + )) + .unwrap(); + + // Verify that storing the fully transparent transaction causes a transaction + // status request to be generated. + let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); + assert!(tx_data_requests + .iter() + .any(|r| r == &TransactionDataRequest::GetStatus(txid))); // We call get_wallet_transparent_output with `allow_unspendable = true` to verify // storage because the decrypted transaction has not yet been mined. From d96bc110555cee26d405896e8e027e055061e654 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Fri, 9 Aug 2024 19:03:30 +0100 Subject: [PATCH 11/18] Add a comment explaining why it is safe to unconditionally delete the request in `set_transaction_status`. Signed-off-by: Daira-Emma Hopwood --- zcash_client_sqlite/src/wallet.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index f646a5a133..318bc63f05 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1994,6 +1994,13 @@ pub(crate) fn set_transaction_status( txid: TxId, status: TransactionStatus, ) -> Result<(), SqliteClientError> { + // In order to have made a Status request, we must already have had the + // raw transaction (the only callers of `queue_tx_retrieval` are in contexts + // where we must have already called `put_tx_data`). Therefore, it is safe + // to unconditionally delete the request from `tx_retrieval_queue` below + // (both in the expired case and the case where it has been mined), because + // we already have all the data we need about this transaction. + match status { TransactionStatus::TxidNotRecognized | TransactionStatus::NotInMainChain => { // If the transaction is now expired, remove it from the retrieval queue. From 8752ad7e19a93b5b9dbc2976daa2f7a5b2ec5a29 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Aug 2024 14:09:31 -0600 Subject: [PATCH 12/18] zcash_client_backend: Add `target_height` to `SentTransaction` --- zcash_client_backend/CHANGELOG.md | 2 ++ zcash_client_backend/src/data_api.rs | 8 ++++++++ zcash_client_backend/src/data_api/wallet.rs | 1 + zcash_client_sqlite/src/wallet.rs | 2 +- .../src/wallet/init/migrations/tx_retrieval_queue.rs | 4 ++-- 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 5010448c04..92dc478feb 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -86,6 +86,8 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `DecryptedTransaction::new` takes an additional `mined_height` argument. - `SentTransaction` now stores its `outputs` and `utxos_spent` fields as references to slices, with a corresponding change to `SentTransaction::new`. + - `SentTransaction` takes an additional `target_height` argument, which is used + to record the target height used in transaction generation. - `zcash_client_backend::data_api::fees` - When the "transparent-inputs" feature is enabled, `ChangeValue` can also represent an ephemeral transparent output in a proposal. Accordingly, the diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 47b26c4ef9..524e9e6844 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1395,6 +1395,7 @@ impl<'a, AccountId> DecryptedTransaction<'a, AccountId> { pub struct SentTransaction<'a, AccountId> { tx: &'a Transaction, created: time::OffsetDateTime, + target_height: BlockHeight, account: AccountId, outputs: &'a [SentTransactionOutput], fee_amount: NonNegativeAmount, @@ -1407,6 +1408,7 @@ impl<'a, AccountId> SentTransaction<'a, AccountId> { pub fn new( tx: &'a Transaction, created: time::OffsetDateTime, + target_height: BlockHeight, account: AccountId, outputs: &'a [SentTransactionOutput], fee_amount: NonNegativeAmount, @@ -1415,6 +1417,7 @@ impl<'a, AccountId> SentTransaction<'a, AccountId> { Self { tx, created, + target_height, account, outputs, fee_amount, @@ -1448,6 +1451,11 @@ impl<'a, AccountId> SentTransaction<'a, AccountId> { pub fn utxos_spent(&self) -> &[OutPoint] { self.utxos_spent } + + /// Returns the block height that this transaction was created to target. + pub fn target_height(&self) -> BlockHeight { + self.target_height + } } /// An output of a transaction generated by the wallet. diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 4508d0d9ec..e94dc96a5b 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -668,6 +668,7 @@ where transactions.push(SentTransaction::new( tx, created, + proposal.min_target_height(), account_id, &step_result.outputs, step_result.fee_amount, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 318bc63f05..aa92b7466e 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1857,7 +1857,7 @@ pub(crate) fn store_transaction_to_be_sent( sent_tx.tx(), Some(sent_tx.fee_amount()), Some(sent_tx.created()), - chain_tip_height(wdb.conn.0)?.map(|h| h + 1), + Some(sent_tx.target_height()), )?; let mut detectable_via_scanning = false; diff --git a/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs index d03c9d17f7..3962dbeca6 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/tx_retrieval_queue.rs @@ -1,4 +1,4 @@ -//! A migration to add the `tx_retrieval_queue` table to the database. +//! Adds tables for tracking transactions to be downloaded for transparent output and/or memo retrieval. use rusqlite::{named_params, Transaction}; use schemer_rusqlite::RusqliteMigration; @@ -24,7 +24,7 @@ impl schemer::Migration for Migration { } fn description(&self) -> &'static str { - "Adds a table for tracking transactions to be downloaded for transparent output and/or memo retrieval." + "Adds tables for tracking transactions to be downloaded for transparent output and/or memo retrieval." } } From 171cc3d18574ce7855bf4ce9c907d662244f3f1b Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Aug 2024 15:10:53 -0600 Subject: [PATCH 13/18] Address review comments in `transaction_data_requests` --- zcash_client_sqlite/src/wallet/transparent.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 6bc1e2af05..88e6c93522 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -494,11 +494,9 @@ pub(crate) fn transaction_data_requests( conn: &rusqlite::Connection, params: &P, ) -> Result, SqliteClientError> { - // We cannot construct transaction data requests for the case where we cannot determine the - // height at which to begin, so we require that either the target height or mined height be - // set. In practice, we should not encounter entries in `transparent_spend_search_queue` - // because under ordinary circumstances, it is populated via a call from - // `decrypt_and_store_transaction` on ordinary mined transaction data retrieved from the chain. + // We cannot construct address-based transaction data requests for the case where we cannot + // determine the height at which to begin, so we require that either the target height or mined + // height be set. let mut address_request_stmt = conn.prepare_cached( "SELECT ssq.address, IFNULL(t.target_height, t.mined_height) FROM transparent_spend_search_queue ssq @@ -515,7 +513,7 @@ pub(crate) fn transaction_data_requests( TransactionDataRequest::SpendsFromAddress { address, block_range_start, - block_range_end: Some(block_range_start + DEFAULT_TX_EXPIRY_DELTA), + block_range_end: Some(block_range_start + DEFAULT_TX_EXPIRY_DELTA + 1), }, ) })? From 239f7ff40475e4164c738ad501fa27e0a94d3c1f Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Fri, 9 Aug 2024 22:31:58 +0100 Subject: [PATCH 14/18] Improve the accuracy of a comment in `set_transaction_status`. Signed-off-by: Daira-Emma Hopwood --- zcash_client_sqlite/src/wallet.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 318bc63f05..4d22390af3 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1994,12 +1994,13 @@ pub(crate) fn set_transaction_status( txid: TxId, status: TransactionStatus, ) -> Result<(), SqliteClientError> { - // In order to have made a Status request, we must already have had the - // raw transaction (the only callers of `queue_tx_retrieval` are in contexts - // where we must have already called `put_tx_data`). Therefore, it is safe - // to unconditionally delete the request from `tx_retrieval_queue` below - // (both in the expired case and the case where it has been mined), because - // we already have all the data we need about this transaction. + // In order to have made a Status request, we must already have had the raw + // transaction (the only callers of `queue_tx_retrieval` with query type + // GetStatus are in contexts where we must have already called `put_tx_data`). + // Therefore, it is safe to unconditionally delete the request from + // `tx_retrieval_queue` below (both in the expired case and the case where + // it has been mined), because we already have all the data we need about + // this transaction. match status { TransactionStatus::TxidNotRecognized | TransactionStatus::NotInMainChain => { From 56e42336c47dff708895731e306de858ebd8bcd2 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Fri, 9 Aug 2024 22:33:00 +0100 Subject: [PATCH 15/18] Minor simplifications. Signed-off-by: Daira-Emma Hopwood --- zcash_client_sqlite/src/testing/pool.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 01a106c51a..0ae31980e3 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -440,9 +440,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { // Verify that a status request has been generated for the second transaction of // the ZIP 320 pair. let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); - assert!(tx_data_requests - .iter() - .any(|r| r == &TransactionDataRequest::GetStatus(*txids.last()))); + assert!(tx_data_requests.contains(&TransactionDataRequest::GetStatus(*txids.last()))); assert!(expected_step0_change < expected_ephemeral); assert_eq!(confirmed_sent.len(), 2); @@ -611,9 +609,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { // Verify that storing the fully transparent transaction causes a transaction // status request to be generated. let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); - assert!(tx_data_requests - .iter() - .any(|r| r == &TransactionDataRequest::GetStatus(txid))); + assert!(tx_data_requests.contains(&TransactionDataRequest::GetStatus(txid))); // We call get_wallet_transparent_output with `allow_unspendable = true` to verify // storage because the decrypted transaction has not yet been mined. From 1023ce7f9982e4dee104ef4ad6b796a79ab38616 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Fri, 9 Aug 2024 22:34:36 +0100 Subject: [PATCH 16/18] Replace the `put_tx_meta` workaround in `send_multi_step_proposed_transfer`. fixes #1485 Signed-off-by: Daira-Emma Hopwood --- zcash_client_sqlite/src/testing/pool.rs | 49 +++++++++---------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 0ae31980e3..ca0aa2845a 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -306,9 +306,9 @@ pub(crate) fn send_multi_step_proposed_transfer() { use rand_core::OsRng; use zcash_client_backend::{ - data_api::TransactionDataRequest, + data_api::{TransactionDataRequest, TransactionStatus}, fees::ChangeValue, - wallet::{TransparentAddressMetadata, WalletTx}, + wallet::TransparentAddressMetadata, }; use zcash_primitives::{ legacy::keys::{NonHardenedChildIndex, TransparentKeyScope}, @@ -683,40 +683,25 @@ pub(crate) fn send_multi_step_proposed_transfer() { let (h, _) = st.generate_next_block_from_tx(tx_index, tx); st.scan_cached_blocks(h, 1); - // The rest of this test would currently fail without the explicit call to - // `put_tx_meta` below. Ideally the above `scan_cached_blocks` would be - // sufficient, but it does not detect the transaction as interesting to the + // The above `scan_cached_blocks` does not detect `tx` as interesting to the // wallet. If a transaction is in the database with a null `mined_height`, // as in this case, its `mined_height` will remain null unless `put_tx_meta` - // is called on it. Normally `put_tx_meta` would be called via `put_blocks` - // as a result of scanning, but that won't happen for any fully transparent - // transaction, and currently it also will not happen for a partially shielded - // transaction unless it is interesting to the wallet for another reason. - // Therefore we will not currently detect either collisions with uses of - // ephemeral outputs by other wallets, or refunds of funds sent to TEX - // addresses. (#1354, #1379) - - // Check that what we say in the above paragraph remains true, so that we - // don't accidentally fix it without updating this test. + // is called on it. This happens either via `put_blocks` as a result of + // scanning, or via `set_transaction_status` in response to processing the + // `transaction_data_requests` queue. For a fully transparent transaction, + // the latter is required. + + // The reservation should fail because `tx` is not yet seen as mined. reservation_should_fail(&mut st, 1, 22); - // For now, we demonstrate that this problem is the only obstacle to the rest - // of the ZIP 320 code doing the right thing, by manually calling `put_tx_meta`: - crate::wallet::put_tx_meta( - &st.wallet_mut().conn, - &WalletTx::new( - tx.txid(), - tx_index, - vec![], - vec![], - #[cfg(feature = "orchard")] - vec![], - #[cfg(feature = "orchard")] - vec![], - ), - h, - ) - .unwrap(); + // Simulate the wallet processing the `transaction_data_requests` queue. + let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); + assert!(tx_data_requests.contains(&TransactionDataRequest::GetStatus(tx.txid()))); + + // Respond to the GetStatus request. + st.wallet_mut() + .set_transaction_status(tx.txid(), TransactionStatus::Mined(h)) + .unwrap(); // We already reserved 22 addresses, so mining the transaction with the // ephemeral output at index 10 should allow 9 more (..31). From 5a1645ac76199a571a5a9270e12114e553f019e1 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Aug 2024 16:55:35 -0600 Subject: [PATCH 17/18] zcash_client_sqlite: Make transaction retrieval depend upon presence of raw tx data. --- zcash_client_backend/src/data_api.rs | 10 ++++++++ zcash_client_sqlite/src/lib.rs | 11 ++------- zcash_client_sqlite/src/wallet.rs | 35 ++++++++++++++-------------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 524e9e6844..0ca7083e2a 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1405,6 +1405,16 @@ pub struct SentTransaction<'a, AccountId> { impl<'a, AccountId> SentTransaction<'a, AccountId> { /// Constructs a new [`SentTransaction`] from its constituent parts. + /// + /// ### Parameters + /// - `tx`: the raw transaction data + /// - `created`: the system time at which the transaction was created + /// - `target_height`: the target height that was used in the construction of the transaction + /// - `account`: the account that spent funds in creation of the transaction + /// - `outputs`: the outputs created by the transaction, including those sent to external + /// recipients which may not otherwise be recoverable + /// - `fee_amount`: the fee value paid by the transaction + /// - `utxos_spent`: the UTXOs controlled by the wallet that were spent in this transaction pub fn new( tx: &'a Transaction, created: time::OffsetDateTime, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 139af897ac..e4728625d3 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -119,7 +119,7 @@ pub mod error; pub mod wallet; use wallet::{ commitment_tree::{self, put_shard_roots}, - notify_tx_retrieved, SubtreeScanProgress, TxQueryType, + notify_tx_retrieved, SubtreeScanProgress, }; #[cfg(feature = "transparent-inputs")] @@ -800,12 +800,7 @@ impl WalletWrite for WalletDb for tx in block.transactions() { let tx_row = wallet::put_tx_meta(wdb.conn.0, tx, block.height())?; - wallet::queue_tx_retrieval( - wdb.conn.0, - std::iter::once(tx.txid()), - TxQueryType::Enhancement, - None, - )?; + wallet::queue_tx_retrieval(wdb.conn.0, std::iter::once(tx.txid()), None)?; // Mark notes as spent and remove them from the scanning cache for spend in tx.sapling_spends() { @@ -1518,7 +1513,6 @@ impl WalletWrite for WalletDb wallet::queue_tx_retrieval( wdb.conn.0, b.vin.iter().map(|txin| *txin.prevout.txid()), - TxQueryType::Enhancement, Some(tx_ref) )?; } @@ -1531,7 +1525,6 @@ impl WalletWrite for WalletDb wallet::queue_tx_retrieval( wdb.conn.0, std::iter::once(d_tx.tx().txid()), - TxQueryType::Status, None )?; } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index cbe1011fff..1825763329 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1978,12 +1978,7 @@ pub(crate) fn store_transaction_to_be_sent( // at present for fully transparent transactions, because any transaction with a shielded // component will be detected via ordinary chain scanning and/or nullifier checking. if !detectable_via_scanning { - queue_tx_retrieval( - wdb.conn.0, - std::iter::once(sent_tx.tx().txid()), - TxQueryType::Status, - None, - )?; + queue_tx_retrieval(wdb.conn.0, std::iter::once(sent_tx.tx().txid()), None)?; } Ok(()) @@ -2409,31 +2404,35 @@ impl TxQueryType { pub(crate) fn queue_tx_retrieval( conn: &rusqlite::Transaction<'_>, txids: impl Iterator, - query_type: TxQueryType, dependent_tx_ref: Option, ) -> Result<(), SqliteClientError> { // Add an entry to the transaction retrieval queue if it would not be redundant. let mut stmt_insert_tx = conn.prepare_cached( "INSERT INTO tx_retrieval_queue (txid, query_type, dependent_transaction_id) - SELECT :txid, :query_type, :dependent_transaction_id - -- do not queue enhancement requests if we already have the raw transaction - WHERE :query_type <> :enhancement_type OR NOT EXISTS ( - SELECT 1 FROM transactions - WHERE txid = :txid - AND raw IS NOT NULL - ) - -- if there is already a status request, we can upgrade it to an enhancement request + SELECT + :txid, + IIF( + EXISTS (SELECT 1 FROM transactions WHERE txid = :txid AND raw IS NOT NULL), + :status_type, + :enhancement_type + ), + :dependent_transaction_id ON CONFLICT (txid) DO UPDATE - SET query_type = MAX(:query_type, query_type), + SET query_type = + IIF( + EXISTS (SELECT 1 FROM transactions WHERE txid = :txid AND raw IS NOT NULL), + :status_type, + :enhancement_type + ), dependent_transaction_id = IFNULL(:dependent_transaction_id, dependent_transaction_id)", )?; for txid in txids { stmt_insert_tx.execute(named_params! { ":txid": txid.as_ref(), - ":query_type": query_type.code(), - ":dependent_transaction_id": dependent_tx_ref.map(|r| r.0), + ":status_type": TxQueryType::Status.code(), ":enhancement_type": TxQueryType::Enhancement.code(), + ":dependent_transaction_id": dependent_tx_ref.map(|r| r.0), })?; } From b47c7bfe8d6b7c5f3442e1e77319b561dead8de6 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Aug 2024 17:30:37 -0600 Subject: [PATCH 18/18] zcash_client_sqlite: Improve the internal documentation of `set_transaction_status` --- zcash_client_sqlite/src/wallet.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 1825763329..f3260863c8 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1989,14 +1989,22 @@ pub(crate) fn set_transaction_status( txid: TxId, status: TransactionStatus, ) -> Result<(), SqliteClientError> { - // In order to have made a Status request, we must already have had the raw - // transaction (the only callers of `queue_tx_retrieval` with query type - // GetStatus are in contexts where we must have already called `put_tx_data`). - // Therefore, it is safe to unconditionally delete the request from - // `tx_retrieval_queue` below (both in the expired case and the case where - // it has been mined), because we already have all the data we need about - // this transaction. - + // It is safe to unconditionally delete the request from `tx_retrieval_queue` below (both in + // the expired case and the case where it has been mined), because we already have all the data + // we need about this transaction: + // * if the status is being set in response to a `GetStatus` request, we know that we already + // have the transaction data (`GetStatus` requests are only generated if we already have that + // data) + // * if it is being set in response to an `Enhancement` request, we know that the status must + // be `TxidNotRecognized` because otherwise the transaction data should have been provided to + // the backend directly instead of calling `set_transaction_status` + // + // In general `Enhancement` requests are only generated in response to situations where a + // transaction has already been mined - either the transaction was detected by scanning the + // chain of `CompactBlock` values, or was discovered by walking backward from the inputs of a + // transparent transaction; in the case that a transaction was read from the mempool, complete + // transaction data will have been available and the only question that we are concerned with + // is whether that transaction ends up being mined or expires. match status { TransactionStatus::TxidNotRecognized | TransactionStatus::NotInMainChain => { // If the transaction is now expired, remove it from the retrieval queue.