From 06e43a572a3cdf645ec6564ae774f4240990882a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 26 Sep 2022 10:59:50 -0600 Subject: [PATCH 1/8] Track inputs sent to wallet-internal recipients. Ensure that we're attempting trial-decryption with the internal IVK and correctly track internal vs. external recipients in the wallet database. --- zcash_client_backend/src/data_api.rs | 12 +- zcash_client_backend/src/data_api/wallet.rs | 14 +- zcash_client_backend/src/decrypt.rs | 47 +++-- zcash_client_backend/src/lib.rs | 2 +- zcash_client_sqlite/src/lib.rs | 92 +++++----- zcash_client_sqlite/src/prepared.rs | 163 +++++++++--------- zcash_client_sqlite/src/wallet.rs | 28 ++- zcash_client_sqlite/src/wallet/init.rs | 11 +- .../src/wallet/init/migrations.rs | 2 + .../init/migrations/sent_notes_to_internal.rs | 88 ++++++++++ 10 files changed, 290 insertions(+), 169 deletions(-) create mode 100644 zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index a2eea7f09..baa007c87 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -248,13 +248,19 @@ pub struct SentTransaction<'a> { pub tx: &'a Transaction, pub created: time::OffsetDateTime, pub account: AccountId, - pub outputs: Vec>, + pub outputs: Vec, pub fee_amount: Amount, #[cfg(feature = "transparent-inputs")] pub utxos_spent: Vec, } -pub struct SentTransactionOutput<'a> { +#[derive(Debug, Clone)] +pub enum Recipient { + Address(RecipientAddress), + InternalAccount(AccountId), +} + +pub struct SentTransactionOutput { /// The index within the transaction that contains the recipient output. /// /// - If `recipient_address` is a Sapling address, this is an index into the Sapling @@ -262,7 +268,7 @@ pub struct SentTransactionOutput<'a> { /// - If `recipient_address` is a transparent address, this is an index into the /// transparent outputs of the transaction. pub output_index: usize, - pub recipient_address: &'a RecipientAddress, + pub recipient: Recipient, pub value: Amount, pub memo: Option, } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 675a9ffad..a986ccfd1 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -23,7 +23,8 @@ use { use crate::{ address::RecipientAddress, data_api::{ - error::Error, DecryptedTransaction, SentTransaction, SentTransactionOutput, WalletWrite, + error::Error, DecryptedTransaction, Recipient, SentTransaction, SentTransactionOutput, + WalletWrite, }, decrypt_transaction, wallet::OvkPolicy, @@ -394,7 +395,7 @@ where SentTransactionOutput { output_index: idx, - recipient_address: &payment.recipient_address, + recipient: Recipient::Address(payment.recipient_address.clone()), value: payment.amount, memo: payment.memo.clone() } @@ -512,12 +513,7 @@ where // add the sapling output to shield the funds builder - .add_sapling_output( - Some(ovk), - shielding_address.clone(), - amount_to_shield, - memo.clone(), - ) + .add_sapling_output(Some(ovk), shielding_address, amount_to_shield, memo.clone()) .map_err(Error::Builder)?; let (tx, tx_metadata) = builder.build(&prover).map_err(Error::Builder)?; @@ -531,8 +527,8 @@ where account, outputs: vec![SentTransactionOutput { output_index, - recipient_address: &RecipientAddress::Shielded(shielding_address), value: amount_to_shield, + recipient: Recipient::InternalAccount(account), memo: Some(memo.clone()), }], fee_amount: fee, diff --git a/zcash_client_backend/src/decrypt.rs b/zcash_client_backend/src/decrypt.rs index 64086c166..bd06cbd25 100644 --- a/zcash_client_backend/src/decrypt.rs +++ b/zcash_client_backend/src/decrypt.rs @@ -10,11 +10,22 @@ use zcash_primitives::{ Note, PaymentAddress, }, transaction::Transaction, - zip32::AccountId, + zip32::{AccountId, Scope}, }; use crate::keys::UnifiedFullViewingKey; +/// An enumeration of the possible relationships a TXO can have to the wallet. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum TransferType { + /// The transfer was received on one of the wallet's external addresses. + Incoming, + /// The transfer was received on one of the wallet's internal-only addresses. + WalletInternal, + /// The transfer was decrypted using one of the wallet's outgoing viewing keys. + Outgoing, +} + /// A decrypted shielded output. pub struct DecryptedOutput { /// The index of the output within [`shielded_outputs`]. @@ -33,7 +44,7 @@ pub struct DecryptedOutput { /// this is a logical output of the transaction. /// /// [`OutgoingViewingKey`]: zcash_primitives::keys::OutgoingViewingKey - pub outgoing: bool, + pub transfer_type: TransferType, } /// Scans a [`Transaction`] for any information that can be decrypted by the set of @@ -49,19 +60,29 @@ pub fn decrypt_transaction( if let Some(bundle) = tx.sapling_bundle() { for (account, ufvk) in ufvks.iter() { if let Some(dfvk) = ufvk.sapling() { - let ivk = PreparedIncomingViewingKey::new(&dfvk.fvk().vk.ivk()); + let ivk_external = PreparedIncomingViewingKey::new(&dfvk.to_ivk(Scope::External)); + let ivk_internal = PreparedIncomingViewingKey::new(&dfvk.to_ivk(Scope::Internal)); let ovk = dfvk.fvk().ovk; for (index, output) in bundle.shielded_outputs.iter().enumerate() { - let ((note, to, memo), outgoing) = - match try_sapling_note_decryption(params, height, &ivk, output) { - Some(ret) => (ret, false), - None => match try_sapling_output_recovery(params, height, &ovk, output) - { - Some(ret) => (ret, true), - None => continue, - }, - }; + let decryption_result = + try_sapling_note_decryption(params, height, &ivk_external, output) + .map(|ret| (ret, TransferType::Incoming)) + .or_else(|| { + try_sapling_note_decryption(params, height, &ivk_internal, output) + .map(|ret| (ret, TransferType::WalletInternal)) + }) + .or_else(|| { + try_sapling_output_recovery(params, height, &ovk, output) + .map(|ret| (ret, TransferType::Outgoing)) + }); + + let ((note, to, memo), transfer_type) = match decryption_result { + Some(result) => result, + None => { + continue; + } + }; decrypted.push(DecryptedOutput { index, @@ -69,7 +90,7 @@ pub fn decrypt_transaction( account: *account, to, memo, - outgoing, + transfer_type, }) } } diff --git a/zcash_client_backend/src/lib.rs b/zcash_client_backend/src/lib.rs index 2bcf755e9..1eb5163c7 100644 --- a/zcash_client_backend/src/lib.rs +++ b/zcash_client_backend/src/lib.rs @@ -19,4 +19,4 @@ pub mod wallet; pub mod welding_rig; pub mod zip321; -pub use decrypt::{decrypt_transaction, DecryptedOutput}; +pub use decrypt::{decrypt_transaction, DecryptedOutput, TransferType}; diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 9583cf65a..db4808714 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -55,11 +55,13 @@ use zcash_primitives::{ use zcash_client_backend::{ address::{RecipientAddress, UnifiedAddress}, data_api::{ - BlockSource, DecryptedTransaction, PrunedBlock, SentTransaction, WalletRead, WalletWrite, + BlockSource, DecryptedTransaction, PrunedBlock, Recipient, SentTransaction, WalletRead, + WalletWrite, }, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, wallet::SpendableNote, + TransferType, }; use crate::error::SqliteClientError; @@ -529,29 +531,38 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { let mut spending_account_id: Option = None; for output in d_tx.sapling_outputs { - if output.outgoing { - wallet::put_sent_note( - up, - tx_ref, - output.index, - output.account, - &output.to, - Amount::from_u64(output.note.value) - .map_err(|_| SqliteClientError::CorruptedData("Note value invalid.".to_string()))?, - Some(&output.memo), - )?; - } else { - match spending_account_id { - Some(id) => - if id != output.account { - panic!("Unable to determine a unique account identifier for z->t spend."); - } - None => { - spending_account_id = Some(output.account); - } - } + match output.transfer_type { + TransferType::Outgoing | TransferType::WalletInternal => { + let recipient = if output.transfer_type == TransferType::Outgoing { + Recipient::Address(RecipientAddress::Shielded(output.to.clone())) + } else { + Recipient::InternalAccount(output.account) + }; - wallet::put_received_note(up, output, tx_ref)?; + wallet::put_sent_note( + up, + tx_ref, + output.index, + output.account, + &recipient, + Amount::from_u64(output.note.value) + .map_err(|_| SqliteClientError::CorruptedData("Note value invalid.".to_string()))?, + Some(&output.memo), + )?; + } + TransferType::Incoming => { + match spending_account_id { + Some(id) => + if id != output.account { + panic!("Unable to determine a unique account identifier for z->t spend."); + } + None => { + spending_account_id = Some(output.account); + } + } + + wallet::put_received_note(up, output, tx_ref)?; + } } } @@ -611,35 +622,26 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { } for output in &sent_tx.outputs { - match output.recipient_address { - // TODO: Store the entire UA, not just the Sapling component. - // This will require more info about the output index. - RecipientAddress::Unified(ua) => wallet::insert_sent_note( + match &output.recipient { + Recipient::Address(RecipientAddress::Transparent(addr)) => { + wallet::insert_sent_utxo( + up, + tx_ref, + output.output_index, + sent_tx.account, + addr, + output.value, + )? + } + shielded_recipient => wallet::insert_sent_note( up, tx_ref, output.output_index, sent_tx.account, - ua.sapling().expect("TODO: Add Orchard support"), + shielded_recipient, output.value, output.memo.as_ref(), )?, - RecipientAddress::Shielded(addr) => wallet::insert_sent_note( - up, - tx_ref, - output.output_index, - sent_tx.account, - addr, - output.value, - output.memo.as_ref(), - )?, - RecipientAddress::Transparent(addr) => wallet::insert_sent_utxo( - up, - tx_ref, - output.output_index, - sent_tx.account, - addr, - output.value, - )?, } } diff --git a/zcash_client_sqlite/src/prepared.rs b/zcash_client_sqlite/src/prepared.rs index 67de34861..8a20b5d3c 100644 --- a/zcash_client_sqlite/src/prepared.rs +++ b/zcash_client_sqlite/src/prepared.rs @@ -7,7 +7,7 @@ //! - Build the statement in [`DataConnStmtCache::new`]. //! - Add a crate-private helper method to `DataConnStmtCache` for running the statement. -use rusqlite::{params, Statement, ToSql}; +use rusqlite::{named_params, params, Statement, ToSql}; use zcash_primitives::{ block::BlockHash, consensus::{self, BlockHeight}, @@ -18,14 +18,14 @@ use zcash_primitives::{ zip32::{AccountId, DiversifierIndex}, }; -use zcash_client_backend::address::UnifiedAddress; +use zcash_client_backend::{address::UnifiedAddress, data_api::Recipient}; use crate::{error::SqliteClientError, wallet::PoolType, NoteId, WalletDb}; #[cfg(feature = "transparent-inputs")] use { crate::UtxoId, - rusqlite::{named_params, OptionalExtension}, + rusqlite::OptionalExtension, zcash_client_backend::{encoding::AddressCodec, wallet::WalletTransparentOutput}, zcash_primitives::transaction::components::transparent::OutPoint, }; @@ -155,7 +155,8 @@ impl<'a, P> DataConnStmtCache<'a, P> { stmt_update_sent_note: wallet_db.conn.prepare( "UPDATE sent_notes SET from_account = :account, - address = :address, + to_address = :to_address, + to_account = :to_account, value = :value, memo = IFNULL(:memo, memo) WHERE tx = :tx @@ -163,8 +164,12 @@ impl<'a, P> DataConnStmtCache<'a, P> { AND output_index = :output_index", )?, stmt_insert_sent_note: wallet_db.conn.prepare( - "INSERT INTO sent_notes (tx, output_pool, output_index, from_account, address, value, memo) - VALUES (:tx, :output_pool, :output_index, :from_account, :address, :value, :memo)" + "INSERT INTO sent_notes ( + tx, output_pool, output_index, from_account, + to_address, to_account, value, memo) + VALUES ( + :tx, :output_pool, :output_index, :from_account, + :to_address, :to_account, :value, :memo)" )?, stmt_insert_witness: wallet_db.conn.prepare( "INSERT INTO sapling_witnesses (note, block, witness) @@ -346,6 +351,80 @@ impl<'a, P> DataConnStmtCache<'a, P> { } impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { + /// Inserts a sent note into the wallet database. + /// + /// `output_index` is the index within the transaction that contains the recipient output: + /// + /// - If `to` is a Sapling address, this is an index into the Sapling outputs of the + /// transaction. + /// - If `to` is a transparent address, this is an index into the transparent outputs of + /// the transaction. + /// - If `to` is an internal account, this is an index into the Sapling outputs of the + /// transaction. + #[allow(clippy::too_many_arguments)] + pub(crate) fn stmt_insert_sent_note( + &mut self, + tx_ref: i64, + pool_type: PoolType, + output_index: usize, + from_account: AccountId, + to: &Recipient, + value: Amount, + memo: Option<&MemoBytes>, + ) -> Result<(), SqliteClientError> { + let (to_address, to_account) = match to { + Recipient::Address(addr) => (Some(addr.encode(&self.wallet_db.params)), None), + Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), + }; + + self.stmt_insert_sent_note.execute(named_params![ + ":tx": &tx_ref, + ":output_pool": &pool_type.typecode(), + ":output_index": &i64::try_from(output_index).unwrap(), + ":from_account": &u32::from(from_account), + ":to_address": &to_address, + ":to_account": &to_account, + ":value": &i64::from(value), + ":memo": &memo.filter(|m| *m != &MemoBytes::empty()).map(|m| m.as_slice()), + ])?; + + Ok(()) + } + + /// Updates the data for the given sent note. + /// + /// Returns `false` if the transaction doesn't exist in the wallet. + #[allow(clippy::too_many_arguments)] + pub(crate) fn stmt_update_sent_note( + &mut self, + account: AccountId, + to: &Recipient, + value: Amount, + memo: Option<&MemoBytes>, + tx_ref: i64, + pool_type: PoolType, + output_index: usize, + ) -> Result { + let (to_address, to_account) = match to { + Recipient::Address(addr) => (Some(addr.encode(&self.wallet_db.params)), None), + Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), + }; + match self.stmt_update_sent_note.execute(named_params![ + ":account": &u32::from(account), + ":to_address": &to_address, + ":to_account": &to_account, + ":value": &i64::from(value), + ":memo": &memo.filter(|m| *m != &MemoBytes::empty()).map(|m| m.as_slice()), + ":tx": &tx_ref, + ":output_pool": &pool_type.typecode(), + ":output_index": &i64::try_from(output_index).unwrap(), + ])? { + 0 => Ok(false), + 1 => Ok(true), + _ => unreachable!("tx_output constraint is marked as UNIQUE"), + } + } + /// Adds the given received UTXO to the datastore. /// /// Returns the database row for the newly-inserted UTXO, or an error if the UTXO @@ -528,78 +607,6 @@ impl<'a, P> DataConnStmtCache<'a, P> { .map_err(SqliteClientError::from) } - /// Inserts a sent note into the wallet database. - /// - /// `output_index` is the index within the transaction that contains the recipient output: - /// - /// - If `to` is a Sapling address, this is an index into the Sapling outputs of the - /// transaction. - /// - If `to` is a transparent address, this is an index into the transparent outputs of - /// the transaction. - #[allow(clippy::too_many_arguments)] - pub(crate) fn stmt_insert_sent_note( - &mut self, - tx_ref: i64, - pool_type: PoolType, - output_index: usize, - account: AccountId, - to_str: &str, - value: Amount, - memo: Option<&MemoBytes>, - ) -> Result<(), SqliteClientError> { - let sql_args: &[(&str, &dyn ToSql)] = &[ - (":tx", &tx_ref), - (":output_pool", &pool_type.typecode()), - (":output_index", &i64::try_from(output_index).unwrap()), - (":from_account", &u32::from(account)), - (":address", &to_str), - (":value", &i64::from(value)), - ( - ":memo", - &memo - .filter(|m| *m != &MemoBytes::empty()) - .map(|m| m.as_slice()), - ), - ]; - self.stmt_insert_sent_note.execute(sql_args)?; - Ok(()) - } - - /// Updates the data for the given sent note. - /// - /// Returns `false` if the transaction doesn't exist in the wallet. - #[allow(clippy::too_many_arguments)] - pub(crate) fn stmt_update_sent_note( - &mut self, - account: AccountId, - to_str: &str, - value: Amount, - memo: Option<&MemoBytes>, - tx_ref: i64, - pool_type: PoolType, - output_index: usize, - ) -> Result { - let sql_args: &[(&str, &dyn ToSql)] = &[ - (":account", &u32::from(account)), - (":address", &to_str), - (":value", &i64::from(value)), - ( - ":memo", - &memo - .filter(|m| *m != &MemoBytes::empty()) - .map(|m| m.as_slice()), - ), - (":tx", &tx_ref), - (":output_pool", &pool_type.typecode()), - (":output_index", &i64::try_from(output_index).unwrap()), - ]; - match self.stmt_update_sent_note.execute(sql_args)? { - 0 => Ok(false), - 1 => Ok(true), - _ => unreachable!("tx_output constraint is marked as UNIQUE"), - } - } - /// Records the incremental witness for the specified note, as of the given block /// height. /// diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 6afa1db65..3879083fd 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -27,8 +27,7 @@ use zcash_primitives::{ use zcash_client_backend::{ address::{RecipientAddress, UnifiedAddress}, - data_api::error::Error, - encoding::{encode_payment_address_p, encode_transparent_address_p}, + data_api::{error::Error, Recipient}, keys::UnifiedFullViewingKey, wallet::{WalletShieldedOutput, WalletTx}, DecryptedOutput, @@ -1163,14 +1162,14 @@ pub fn put_sent_note<'a, P: consensus::Parameters>( tx_ref: i64, output_index: usize, account: AccountId, - to: &PaymentAddress, + to: &Recipient, value: Amount, memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { // Try updating an existing sent note. if !stmts.stmt_update_sent_note( account, - &encode_payment_address_p(&stmts.wallet_db.params, to), + to, value, memo, tx_ref, @@ -1203,7 +1202,7 @@ pub fn put_sent_utxo<'a, P: consensus::Parameters>( // Try updating an existing sent UTXO. if !stmts.stmt_update_sent_note( account, - &encode_transparent_address_p(&stmts.wallet_db.params, to), + &Recipient::Address(RecipientAddress::Transparent(*to)), value, None, tx_ref, @@ -1225,26 +1224,21 @@ pub fn put_sent_utxo<'a, P: consensus::Parameters>( /// transaction. /// - If `to` is a transparent address, this is an index into the transparent outputs of /// the transaction. -#[deprecated( - note = "This method will be removed in a future update. Use zcash_client_backend::data_api::WalletWrite::store_sent_tx instead." -)] -pub fn insert_sent_note<'a, P: consensus::Parameters>( +pub(crate) fn insert_sent_note<'a, P: consensus::Parameters>( stmts: &mut DataConnStmtCache<'a, P>, tx_ref: i64, output_index: usize, - account: AccountId, - to: &PaymentAddress, + from_account: AccountId, + to: &Recipient, value: Amount, memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { - let to_str = encode_payment_address_p(&stmts.wallet_db.params, to); - stmts.stmt_insert_sent_note( tx_ref, PoolType::Sapling, output_index, - account, - &to_str, + from_account, + to, value, memo, ) @@ -1264,14 +1258,12 @@ pub fn insert_sent_utxo<'a, P: consensus::Parameters>( to: &TransparentAddress, value: Amount, ) -> Result<(), SqliteClientError> { - let to_str = encode_transparent_address_p(&stmts.wallet_db.params, to); - stmts.stmt_insert_sent_note( tx_ref, PoolType::Transparent, output_index, account, - &to_str, + &Recipient::Address(RecipientAddress::Transparent(*to)), value, None, ) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index cf0fb1fa3..ba0e84f92 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -122,8 +122,13 @@ fn init_wallet_db_internal( seed: Option>, target_migration: Option, ) -> Result<(), MigratorError> { + // Turn off foreign keys, and ensure that table replacement/modification + // does not break views wdb.conn - .execute("PRAGMA foreign_keys = OFF", []) + .execute_batch( + "PRAGMA foreign_keys = OFF; + PRAGMA legacy_alter_table = TRUE;", + ) .map_err(|e| MigratorError::Adapter(WalletMigrationError::from(e)))?; let adapter = RusqliteAdapter::new(&mut wdb.conn, Some("schemer_migrations".to_string())); adapter.init().expect("Migrations table setup succeeds."); @@ -375,11 +380,13 @@ mod tests { output_pool INTEGER NOT NULL , output_index INTEGER NOT NULL, from_account INTEGER NOT NULL, - address TEXT NOT NULL, + to_address TEXT, + to_account INTEGER, value INTEGER NOT NULL, memo BLOB, FOREIGN KEY (tx) REFERENCES transactions(id_tx), FOREIGN KEY (from_account) REFERENCES accounts(account), + FOREIGN KEY (to_account) REFERENCES accounts(account), CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index) )", "CREATE TABLE transactions ( diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index 0f78213e4..a5b122f17 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -2,6 +2,7 @@ mod add_transaction_views; mod add_utxo_account; mod addresses_table; mod initial_setup; +mod sent_notes_to_internal; mod ufvk_support; mod utxos_table; @@ -31,5 +32,6 @@ pub(super) fn all_migrations( Box::new(add_utxo_account::Migration { _params: params.clone(), }), + Box::new(sent_notes_to_internal::Migration {}), ] } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs b/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs new file mode 100644 index 000000000..e8054e92e --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs @@ -0,0 +1,88 @@ +//! A migration that adds an identifier for the account that received a sent note +//! on an internal address to the sent_notes table. +use std::collections::HashSet; + +use rusqlite; +use schemer; +use schemer_rusqlite::RusqliteMigration; +use uuid::Uuid; + +use super::{addresses_table, utxos_table}; +use crate::wallet::init::WalletMigrationError; + +/// This migration adds the `to_account` field to the `sent_notes` table. +/// +/// 0ddbe561-8259-4212-9ab7-66fdc4a74e1d +pub(super) const MIGRATION_ID: Uuid = Uuid::from_fields( + 0x0ddbe561, + 0x8259, + 0x4212, + b"\x9a\xb7\x66\xfd\xc4\xa7\x4e\x1d", +); + +pub(super) struct Migration; + +impl schemer::Migration for Migration { + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + [utxos_table::MIGRATION_ID, addresses_table::MIGRATION_ID] + .into_iter() + .collect() + } + + fn description(&self) -> &'static str { + "Adds an identifier for the account that received an internal note to the sent_notes table" + } +} + +impl RusqliteMigration for Migration { + type Error = WalletMigrationError; + + fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> { + transaction.execute_batch("ALTER TABLE sent_notes ADD COLUMN to_account INTEGER;")?; + + // `to_account` should be null for all migrated rows, since internal addresses + // have not been used for change or shielding prior to this migration. + transaction.execute_batch( + "CREATE TABLE sent_notes_new ( + id_note INTEGER PRIMARY KEY, + tx INTEGER NOT NULL, + output_pool INTEGER NOT NULL , + output_index INTEGER NOT NULL, + from_account INTEGER NOT NULL, + to_address TEXT, + to_account INTEGER, + value INTEGER NOT NULL, + memo BLOB, + FOREIGN KEY (tx) REFERENCES transactions(id_tx), + FOREIGN KEY (from_account) REFERENCES accounts(account), + FOREIGN KEY (to_account) REFERENCES accounts(account), + CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index) + ); + INSERT INTO sent_notes_new ( + id_note, tx, output_pool, output_index, + from_account, to_address, + value, memo) + SELECT + id_note, tx, output_pool, output_index, + from_account, address, + value, memo + FROM sent_notes;", + )?; + + transaction.execute_batch( + "DROP TABLE sent_notes; + ALTER TABLE sent_notes_new RENAME TO sent_notes;", + )?; + + Ok(()) + } + + fn down(&self, _transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> { + // TODO: something better than just panic? + panic!("Cannot revert this migration."); + } +} From ee9869bbeb73d954a5624a891380cd58f6596a94 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 30 Sep 2022 13:36:20 -0600 Subject: [PATCH 2/8] Use declarative style for `decrypt_transaction` --- zcash_client_backend/src/decrypt.rs | 87 +++++++++++++++-------------- 1 file changed, 46 insertions(+), 41 deletions(-) diff --git a/zcash_client_backend/src/decrypt.rs b/zcash_client_backend/src/decrypt.rs index bd06cbd25..97594c076 100644 --- a/zcash_client_backend/src/decrypt.rs +++ b/zcash_client_backend/src/decrypt.rs @@ -55,47 +55,52 @@ pub fn decrypt_transaction( tx: &Transaction, ufvks: &HashMap, ) -> Vec { - let mut decrypted = vec![]; + tx.sapling_bundle() + .iter() + .flat_map(|bundle| { + ufvks + .iter() + .flat_map(move |(account, ufvk)| { + ufvk.sapling().into_iter().map(|dfvk| (*account, dfvk)) + }) + .flat_map(move |(account, dfvk)| { + let ivk_external = + PreparedIncomingViewingKey::new(&dfvk.to_ivk(Scope::External)); + let ivk_internal = + PreparedIncomingViewingKey::new(&dfvk.to_ivk(Scope::Internal)); + let ovk = dfvk.fvk().ovk; - if let Some(bundle) = tx.sapling_bundle() { - for (account, ufvk) in ufvks.iter() { - if let Some(dfvk) = ufvk.sapling() { - let ivk_external = PreparedIncomingViewingKey::new(&dfvk.to_ivk(Scope::External)); - let ivk_internal = PreparedIncomingViewingKey::new(&dfvk.to_ivk(Scope::Internal)); - let ovk = dfvk.fvk().ovk; - - for (index, output) in bundle.shielded_outputs.iter().enumerate() { - let decryption_result = - try_sapling_note_decryption(params, height, &ivk_external, output) - .map(|ret| (ret, TransferType::Incoming)) - .or_else(|| { - try_sapling_note_decryption(params, height, &ivk_internal, output) + bundle + .shielded_outputs + .iter() + .enumerate() + .flat_map(move |(index, output)| { + try_sapling_note_decryption(params, height, &ivk_external, output) + .map(|ret| (ret, TransferType::Incoming)) + .or_else(|| { + try_sapling_note_decryption( + params, + height, + &ivk_internal, + output, + ) .map(|ret| (ret, TransferType::WalletInternal)) - }) - .or_else(|| { - try_sapling_output_recovery(params, height, &ovk, output) - .map(|ret| (ret, TransferType::Outgoing)) - }); - - let ((note, to, memo), transfer_type) = match decryption_result { - Some(result) => result, - None => { - continue; - } - }; - - decrypted.push(DecryptedOutput { - index, - note, - account: *account, - to, - memo, - transfer_type, - }) - } - } - } - } - - decrypted + }) + .or_else(|| { + try_sapling_output_recovery(params, height, &ovk, output) + .map(|ret| (ret, TransferType::Outgoing)) + }) + .into_iter() + .map(move |((note, to, memo), transfer_type)| DecryptedOutput { + index, + note, + account, + to, + memo, + transfer_type, + }) + }) + }) + }) + .collect() } From 56b2edd49829753eb6e5d29e286c0aa6bbea7ecc Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 10 Oct 2022 12:38:43 -0600 Subject: [PATCH 3/8] Simplify sqlite backend storage for sent notes & utxos. The currently deprecated implementations of `insert_sent_utxo`, `insert_sent_note`, `put_sent_utxo` and `put_sent_note` all store to the same `sent_notes` table internally. Since there's no immediate plan to change this arrangement, it's better to have a single pair of internal `insert_sent_output` and `put_sent_output` methods instead. --- zcash_client_backend/src/data_api.rs | 16 ++ zcash_client_backend/src/data_api/wallet.rs | 25 ++- zcash_client_sqlite/CHANGELOG.md | 16 +- zcash_client_sqlite/src/lib.rs | 49 ++---- zcash_client_sqlite/src/prepared.rs | 37 ++-- zcash_client_sqlite/src/wallet.rs | 165 +++++------------- zcash_client_sqlite/src/wallet/init.rs | 10 +- .../init/migrations/sent_notes_to_internal.rs | 12 +- .../wallet/init/migrations/ufvk_support.rs | 10 +- 9 files changed, 145 insertions(+), 195 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index baa007c87..0450061a4 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -255,12 +255,24 @@ pub struct SentTransaction<'a> { } #[derive(Debug, Clone)] +#[allow(clippy::large_enum_variant)] pub enum Recipient { Address(RecipientAddress), InternalAccount(AccountId), } +/// A value pool to which the wallet supports sending transaction outputs. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum PoolType { + /// The transparent value pool + Transparent, + /// The Sapling value pool + Sapling, +} + pub struct SentTransactionOutput { + /// The value in which the output has been created + pub output_pool: PoolType, /// The index within the transaction that contains the recipient output. /// /// - If `recipient_address` is a Sapling address, this is an index into the Sapling @@ -268,8 +280,12 @@ pub struct SentTransactionOutput { /// - If `recipient_address` is a transparent address, this is an index into the /// transparent outputs of the transaction. pub output_index: usize, + /// The recipient address of the transaction, or the account + /// id for wallet-internal transactions. pub recipient: Recipient, + /// The value of the newly created output pub value: Amount, + /// The memo that was attached to the output, if any pub memo: Option, } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index a986ccfd1..47bc7aa2c 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -23,8 +23,8 @@ use { use crate::{ address::RecipientAddress, data_api::{ - error::Error, DecryptedTransaction, Recipient, SentTransaction, SentTransactionOutput, - WalletWrite, + error::Error, DecryptedTransaction, PoolType, Recipient, SentTransaction, + SentTransactionOutput, WalletWrite, }, decrypt_transaction, wallet::OvkPolicy, @@ -374,14 +374,17 @@ where let (tx, tx_metadata) = builder.build(&prover).map_err(Error::Builder)?; let sent_outputs = request.payments().iter().enumerate().map(|(i, payment)| { - let idx = match &payment.recipient_address { + let (output_index, output_pool) = match &payment.recipient_address { // Sapling outputs are shuffled, so we need to look up where the output ended up. - // TODO: When we add Orchard support, we will need to trial-decrypt to find them. - RecipientAddress::Shielded(_) | RecipientAddress::Unified(_) => - tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."), + // TODO: When we add Orchard support, we will need to trial-decrypt to find them, + // and return the appropriate pool type. + RecipientAddress::Shielded(_) | RecipientAddress::Unified(_) => { + let idx = tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."); + (idx, PoolType::Sapling) + } RecipientAddress::Transparent(addr) => { let script = addr.script(); - tx.transparent_bundle() + let idx = tx.transparent_bundle() .and_then(|b| { b.vout .iter() @@ -389,12 +392,15 @@ where .find(|(_, tx_out)| tx_out.script_pubkey == script) }) .map(|(index, _)| index) - .expect("An output should exist in the transaction for each transparent payment.") + .expect("An output should exist in the transaction for each transparent payment."); + + (idx, PoolType::Transparent) } }; SentTransactionOutput { - output_index: idx, + output_pool, + output_index, recipient: Recipient::Address(payment.recipient_address.clone()), value: payment.amount, memo: payment.memo.clone() @@ -526,6 +532,7 @@ where created: time::OffsetDateTime::now_utc(), account, outputs: vec![SentTransactionOutput { + output_pool: PoolType::Sapling, output_index, value: amount_to_shield, recipient: Recipient::InternalAccount(account), diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index e104a3447..a617b76c0 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -89,12 +89,15 @@ and this library adheres to Rust's notion of migration process. ### Removed -- `zcash_client_sqlite::wallet`: - - `get_extended_full_viewing_keys` (use - `zcash_client_backend::data_api::WalletRead::get_unified_full_viewing_keys` - instead). - - `delete_utxos_above` (use - `zcash_client_backend::data_api::WalletWrite::rewind_to_height` instead) +- The following functions have been removed from the public interface of + `zcash_client_sqlite::wallet`. Prefer methods defined on + `zcash_client_backend::data_api::{WalletRead, WalletWrite}` instead. + - `get_extended_full_viewing_keys` (use `WalletRead::get_unified_full_viewing_keys` instead). + - `insert_sent_note` (use `WalletWrite::store_sent_tx` instead) + - `insert_sent_utxo` (use `WalletWrite::store_sent_tx` instead) + - `put_sent_note` (use `WalletWrite::store_decrypted_tx` instead) + - `put_sent_utxo` (use `WalletWrite::store_decrypted_tx` instead) + - `delete_utxos_above` (use `WalletWrite::rewind_to_height` instead) - `zcash_client_sqlite::with_blocks` (use `zcash_client_backend::data_api::BlockSource::with_blocks` instead) @@ -131,7 +134,6 @@ and this library adheres to Rust's notion of - `insert_witness` - `prune_witnesses` - `update_expired_notes` - - `put_sent_note` - `put_sent_utxo` - `insert_sent_note` - `insert_sent_utxo` diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index db4808714..ee3a3aa38 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -55,8 +55,8 @@ use zcash_primitives::{ use zcash_client_backend::{ address::{RecipientAddress, UnifiedAddress}, data_api::{ - BlockSource, DecryptedTransaction, PrunedBlock, Recipient, SentTransaction, WalletRead, - WalletWrite, + BlockSource, DecryptedTransaction, PoolType, PrunedBlock, Recipient, SentTransaction, + WalletRead, WalletWrite, }, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, @@ -539,15 +539,15 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { Recipient::InternalAccount(output.account) }; - wallet::put_sent_note( + wallet::put_sent_output( up, - tx_ref, - output.index, output.account, + tx_ref, + PoolType::Sapling, + output.index, &recipient, - Amount::from_u64(output.note.value) - .map_err(|_| SqliteClientError::CorruptedData("Note value invalid.".to_string()))?, - Some(&output.memo), + Amount::from_u64(output.note.value).unwrap(), + Some(&output.memo) )?; } TransferType::Incoming => { @@ -577,13 +577,16 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { .any(|input| *nf == input.nullifier) ) { for (output_index, txout) in d_tx.tx.transparent_bundle().iter().flat_map(|b| b.vout.iter()).enumerate() { - wallet::put_sent_utxo( + let recipient = Recipient::Address(RecipientAddress::Transparent(txout.script_pubkey.address().unwrap())); + wallet::put_sent_output( up, - tx_ref, - output_index, *account_id, - &txout.script_pubkey.address().unwrap(), + tx_ref, + PoolType::Transparent, + output_index, + &recipient, txout.value, + None )?; } } @@ -622,27 +625,7 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { } for output in &sent_tx.outputs { - match &output.recipient { - Recipient::Address(RecipientAddress::Transparent(addr)) => { - wallet::insert_sent_utxo( - up, - tx_ref, - output.output_index, - sent_tx.account, - addr, - output.value, - )? - } - shielded_recipient => wallet::insert_sent_note( - up, - tx_ref, - output.output_index, - sent_tx.account, - shielded_recipient, - output.value, - output.memo.as_ref(), - )?, - } + wallet::insert_sent_output(up, tx_ref, sent_tx.account, output)?; } // Return the row number of the transaction, so the caller can fetch it for sending. diff --git a/zcash_client_sqlite/src/prepared.rs b/zcash_client_sqlite/src/prepared.rs index 8a20b5d3c..870a3275f 100644 --- a/zcash_client_sqlite/src/prepared.rs +++ b/zcash_client_sqlite/src/prepared.rs @@ -18,9 +18,12 @@ use zcash_primitives::{ zip32::{AccountId, DiversifierIndex}, }; -use zcash_client_backend::{address::UnifiedAddress, data_api::Recipient}; +use zcash_client_backend::{ + address::UnifiedAddress, + data_api::{PoolType, Recipient}, +}; -use crate::{error::SqliteClientError, wallet::PoolType, NoteId, WalletDb}; +use crate::{error::SqliteClientError, wallet::pool_code, NoteId, WalletDb}; #[cfg(feature = "transparent-inputs")] use { @@ -60,8 +63,8 @@ pub struct DataConnStmtCache<'a, P> { stmt_update_received_note: Statement<'a>, stmt_select_received_note: Statement<'a>, - stmt_insert_sent_note: Statement<'a>, - stmt_update_sent_note: Statement<'a>, + stmt_insert_sent_output: Statement<'a>, + stmt_update_sent_output: Statement<'a>, stmt_insert_witness: Statement<'a>, stmt_prune_witnesses: Statement<'a>, @@ -152,9 +155,9 @@ impl<'a, P> DataConnStmtCache<'a, P> { stmt_select_received_note: wallet_db.conn.prepare( "SELECT id_note FROM received_notes WHERE tx = ? AND output_index = ?" )?, - stmt_update_sent_note: wallet_db.conn.prepare( + stmt_update_sent_output: wallet_db.conn.prepare( "UPDATE sent_notes - SET from_account = :account, + SET from_account = :from_account, to_address = :to_address, to_account = :to_account, value = :value, @@ -163,7 +166,7 @@ impl<'a, P> DataConnStmtCache<'a, P> { AND output_pool = :output_pool AND output_index = :output_index", )?, - stmt_insert_sent_note: wallet_db.conn.prepare( + stmt_insert_sent_output: wallet_db.conn.prepare( "INSERT INTO sent_notes ( tx, output_pool, output_index, from_account, to_address, to_account, value, memo) @@ -362,7 +365,7 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { /// - If `to` is an internal account, this is an index into the Sapling outputs of the /// transaction. #[allow(clippy::too_many_arguments)] - pub(crate) fn stmt_insert_sent_note( + pub(crate) fn stmt_insert_sent_output( &mut self, tx_ref: i64, pool_type: PoolType, @@ -373,13 +376,13 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { let (to_address, to_account) = match to { - Recipient::Address(addr) => (Some(addr.encode(&self.wallet_db.params)), None), Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), + Recipient::Address(shielded) => (Some(shielded.encode(&self.wallet_db.params)), None), }; - self.stmt_insert_sent_note.execute(named_params![ + self.stmt_insert_sent_output.execute(named_params![ ":tx": &tx_ref, - ":output_pool": &pool_type.typecode(), + ":output_pool": &pool_code(pool_type), ":output_index": &i64::try_from(output_index).unwrap(), ":from_account": &u32::from(from_account), ":to_address": &to_address, @@ -395,9 +398,9 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { /// /// Returns `false` if the transaction doesn't exist in the wallet. #[allow(clippy::too_many_arguments)] - pub(crate) fn stmt_update_sent_note( + pub(crate) fn stmt_update_sent_output( &mut self, - account: AccountId, + from_account: AccountId, to: &Recipient, value: Amount, memo: Option<&MemoBytes>, @@ -406,17 +409,17 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { output_index: usize, ) -> Result { let (to_address, to_account) = match to { - Recipient::Address(addr) => (Some(addr.encode(&self.wallet_db.params)), None), Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), + Recipient::Address(shielded) => (Some(shielded.encode(&self.wallet_db.params)), None), }; - match self.stmt_update_sent_note.execute(named_params![ - ":account": &u32::from(account), + match self.stmt_update_sent_output.execute(named_params![ + ":from_account": &u32::from(from_account), ":to_address": &to_address, ":to_account": &to_account, ":value": &i64::from(value), ":memo": &memo.filter(|m| *m != &MemoBytes::empty()).map(|m| m.as_slice()), ":tx": &tx_ref, - ":output_pool": &pool_type.typecode(), + ":output_pool": &pool_code(pool_type), ":output_index": &i64::try_from(output_index).unwrap(), ])? { 0 => Ok(false), diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 3879083fd..b325cf86b 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -27,7 +27,7 @@ use zcash_primitives::{ use zcash_client_backend::{ address::{RecipientAddress, UnifiedAddress}, - data_api::{error::Error, Recipient}, + data_api::{error::Error, PoolType, Recipient, SentTransactionOutput}, keys::UnifiedFullViewingKey, wallet::{WalletShieldedOutput, WalletTx}, DecryptedOutput, @@ -35,8 +35,6 @@ use zcash_client_backend::{ use crate::{error::SqliteClientError, DataConnStmtCache, NoteId, WalletDb, PRUNING_HEIGHT}; -use zcash_primitives::legacy::TransparentAddress; - #[cfg(feature = "transparent-inputs")] use { crate::UtxoId, @@ -44,7 +42,7 @@ use { std::collections::HashSet, zcash_client_backend::{encoding::AddressCodec, wallet::WalletTransparentOutput}, zcash_primitives::{ - legacy::{keys::IncomingViewingKey, Script}, + legacy::{keys::IncomingViewingKey, Script, TransparentAddress}, transaction::components::{OutPoint, TxOut}, }, }; @@ -52,20 +50,13 @@ use { pub mod init; pub mod transact; -pub(crate) enum PoolType { - Transparent, - Sapling, -} - -impl PoolType { - pub(crate) fn typecode(&self) -> i64 { - // These constants are *incidentally* shared with the typecodes - // for unified addresses, but this is exclusively an internal - // implementation detail. - match self { - PoolType::Transparent => 0i64, - PoolType::Sapling => 2i64, - } +pub(crate) fn pool_code(pool_type: PoolType) -> i64 { + // These constants are *incidentally* shared with the typecodes + // for unified addresses, but this is exclusively an internal + // implementation detail. + match pool_type { + PoolType::Transparent => 0i64, + PoolType::Sapling => 2i64, } } @@ -1152,121 +1143,61 @@ pub fn update_expired_notes

( stmts.stmt_update_expired(height) } -/// Records information about a note that your wallet created. -#[deprecated( - note = "This method will be removed in a future update. Use zcash_client_backend::data_api::WalletWrite::store_decrypted_tx instead." -)] -#[allow(deprecated)] -pub fn put_sent_note<'a, P: consensus::Parameters>( +/// Records information about a transaction output that your wallet created. +/// +/// This is a crate-internal convenience method. +pub(crate) fn insert_sent_output<'a, P: consensus::Parameters>( stmts: &mut DataConnStmtCache<'a, P>, tx_ref: i64, - output_index: usize, - account: AccountId, - to: &Recipient, - value: Amount, - memo: Option<&MemoBytes>, -) -> Result<(), SqliteClientError> { - // Try updating an existing sent note. - if !stmts.stmt_update_sent_note( - account, - to, - value, - memo, - tx_ref, - PoolType::Sapling, - output_index, - )? { - // It isn't there, so insert. - insert_sent_note(stmts, tx_ref, output_index, account, to, value, memo)? - } - - Ok(()) -} - -/// Adds information about a sent transparent UTXO to the database if it does not already -/// exist, or updates it if a record for the UTXO already exists. -/// -/// `output_index` is the index within transparent UTXOs of the transaction that contains the recipient output. -#[deprecated( - note = "This method will be removed in a future update. Use zcash_client_backend::data_api::WalletWrite::store_decrypted_tx instead." -)] -#[allow(deprecated)] -pub fn put_sent_utxo<'a, P: consensus::Parameters>( - stmts: &mut DataConnStmtCache<'a, P>, - tx_ref: i64, - output_index: usize, - account: AccountId, - to: &TransparentAddress, - value: Amount, -) -> Result<(), SqliteClientError> { - // Try updating an existing sent UTXO. - if !stmts.stmt_update_sent_note( - account, - &Recipient::Address(RecipientAddress::Transparent(*to)), - value, - None, - tx_ref, - PoolType::Transparent, - output_index, - )? { - // It isn't there, so insert. - insert_sent_utxo(stmts, tx_ref, output_index, account, to, value)? - } - - Ok(()) -} - -/// Inserts a sent note into the wallet database. -/// -/// `output_index` is the index within the transaction that contains the recipient output: -/// -/// - If `to` is a Sapling address, this is an index into the Sapling outputs of the -/// transaction. -/// - If `to` is a transparent address, this is an index into the transparent outputs of -/// the transaction. -pub(crate) fn insert_sent_note<'a, P: consensus::Parameters>( - stmts: &mut DataConnStmtCache<'a, P>, - tx_ref: i64, - output_index: usize, from_account: AccountId, - to: &Recipient, - value: Amount, - memo: Option<&MemoBytes>, + output: &SentTransactionOutput, ) -> Result<(), SqliteClientError> { - stmts.stmt_insert_sent_note( + stmts.stmt_insert_sent_output( tx_ref, - PoolType::Sapling, - output_index, + output.output_pool, + output.output_index, from_account, - to, - value, - memo, + &output.recipient, + output.value, + output.memo.as_ref(), ) } -/// Inserts information about a sent transparent UTXO into the wallet database. +/// Records information about a transaction output that your wallet created. /// -/// `output_index` is the index within transparent UTXOs of the transaction that contains the recipient output. -#[deprecated( - note = "This method will be removed in a future update. Use zcash_client_backend::data_api::WalletWrite::store_sent_tx instead." -)] -pub fn insert_sent_utxo<'a, P: consensus::Parameters>( +/// This is a crate-internal convenience method. +#[allow(clippy::too_many_arguments)] +pub(crate) fn put_sent_output<'a, P: consensus::Parameters>( stmts: &mut DataConnStmtCache<'a, P>, + from_account: AccountId, tx_ref: i64, + output_pool: PoolType, output_index: usize, - account: AccountId, - to: &TransparentAddress, + recipient: &Recipient, value: Amount, + memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { - stmts.stmt_insert_sent_note( - tx_ref, - PoolType::Transparent, - output_index, - account, - &Recipient::Address(RecipientAddress::Transparent(*to)), + if !stmts.stmt_update_sent_output( + from_account, + recipient, value, - None, - ) + memo, + tx_ref, + output_pool, + output_index, + )? { + stmts.stmt_insert_sent_output( + tx_ref, + output_pool, + output_index, + from_account, + recipient, + value, + memo, + )?; + } + + Ok(()) } #[cfg(test)] diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index ba0e84f92..6773af3df 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -309,7 +309,7 @@ mod tests { use crate::{ error::SqliteClientError, tests::{self, network}, - wallet::get_address, + wallet::{get_address, pool_code}, AccountId, WalletDb, }; @@ -387,7 +387,11 @@ mod tests { FOREIGN KEY (tx) REFERENCES transactions(id_tx), FOREIGN KEY (from_account) REFERENCES accounts(account), FOREIGN KEY (to_account) REFERENCES accounts(account), - CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index) + CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index), + CONSTRAINT note_recipient CHECK ( + (to_address IS NOT NULL OR to_account IS NOT NULL) + AND NOT (to_address IS NOT NULL AND to_account IS NOT NULL) + ) )", "CREATE TABLE transactions ( id_tx INTEGER PRIMARY KEY, @@ -950,7 +954,7 @@ mod tests { wdb.conn.execute( "INSERT INTO sent_notes (tx, output_pool, output_index, from_account, address, value) VALUES (0, ?, 0, ?, ?, 0)", - [PoolType::Transparent.typecode().to_sql()?, u32::from(account).to_sql()?, taddr.to_sql()?])?; + [pool_code(PoolType::Transparent).to_sql()?, u32::from(account).to_sql()?, taddr.to_sql()?])?; } Ok(()) diff --git a/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs b/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs index e8054e92e..5debe5db6 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs @@ -42,10 +42,8 @@ impl RusqliteMigration for Migration { type Error = WalletMigrationError; fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> { - transaction.execute_batch("ALTER TABLE sent_notes ADD COLUMN to_account INTEGER;")?; - - // `to_account` should be null for all migrated rows, since internal addresses - // have not been used for change or shielding prior to this migration. + // Adds the `to_account` column to the `sent_notes` table and establishes the + // foreign key relationship with the `account` table. transaction.execute_batch( "CREATE TABLE sent_notes_new ( id_note INTEGER PRIMARY KEY, @@ -60,7 +58,11 @@ impl RusqliteMigration for Migration { FOREIGN KEY (tx) REFERENCES transactions(id_tx), FOREIGN KEY (from_account) REFERENCES accounts(account), FOREIGN KEY (to_account) REFERENCES accounts(account), - CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index) + CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index), + CONSTRAINT note_recipient CHECK ( + (to_address IS NOT NULL OR to_account IS NOT NULL) + AND NOT (to_address IS NOT NULL AND to_account IS NOT NULL) + ) ); INSERT INTO sent_notes_new ( id_note, tx, output_pool, output_index, diff --git a/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs b/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs index 24936b590..e4774bda8 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs @@ -7,7 +7,9 @@ use schemer_rusqlite::RusqliteMigration; use secrecy::{ExposeSecret, SecretVec}; use uuid::Uuid; -use zcash_client_backend::{address::RecipientAddress, keys::UnifiedSpendingKey}; +use zcash_client_backend::{ + address::RecipientAddress, data_api::PoolType, keys::UnifiedSpendingKey, +}; use zcash_primitives::{consensus, zip32::AccountId}; #[cfg(feature = "transparent-inputs")] @@ -18,7 +20,7 @@ use zcash_client_backend::encoding::AddressCodec; use crate::wallet::{ init::{migrations::utxos_table, WalletMigrationError}, - PoolType, + pool_code, }; pub(super) const MIGRATION_ID: Uuid = Uuid::from_fields( @@ -227,8 +229,8 @@ impl RusqliteMigration for Migration

{ )) })?; let output_pool = match decoded_address { - RecipientAddress::Shielded(_) => Ok(PoolType::Sapling.typecode()), - RecipientAddress::Transparent(_) => Ok(PoolType::Transparent.typecode()), + RecipientAddress::Shielded(_) => Ok(pool_code(PoolType::Sapling)), + RecipientAddress::Transparent(_) => Ok(pool_code(PoolType::Transparent)), RecipientAddress::Unified(_) => Err(WalletMigrationError::CorruptedData( "Unified addresses should not yet appear in the sent_notes table." .to_string(), From 3c837381db1418dc212fcbb1a53edc55f04e47ed Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 Oct 2022 09:07:11 -0600 Subject: [PATCH 4/8] Disallow invalid pool/address combinations with `Recipient`. --- zcash_client_backend/CHANGELOG.md | 2 + zcash_client_backend/src/data_api.rs | 31 +++++---- zcash_client_backend/src/data_api/wallet.rs | 18 ++--- zcash_client_backend/src/encoding.rs | 76 ++++++++++++++++----- zcash_client_sqlite/src/lib.rs | 10 ++- zcash_client_sqlite/src/prepared.rs | 45 +++++++++--- zcash_client_sqlite/src/wallet.rs | 13 +--- zcash_client_sqlite/src/wallet/init.rs | 7 +- 8 files changed, 131 insertions(+), 71 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index ce03b8e3b..8af79ca45 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -31,6 +31,8 @@ and this library adheres to Rust's notion of - `zcash_client_backend::address`: - `RecipientAddress::Unified` - `zcash_client_backend::data_api`: + - `Recipient` + - `SentTransactionOutput` - `WalletRead::get_unified_full_viewing_keys` - `WalletRead::get_current_address` - `WalletRead::get_all_nullifiers` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 0450061a4..8f2040bc0 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -11,15 +11,16 @@ use secrecy::SecretVec; use zcash_primitives::{ block::BlockHash, consensus::BlockHeight, + legacy::TransparentAddress, memo::{Memo, MemoBytes}, merkle_tree::{CommitmentTree, IncrementalWitness}, - sapling::{Node, Nullifier}, + sapling::{Node, Nullifier, PaymentAddress}, transaction::{components::Amount, Transaction, TxId}, zip32::{AccountId, ExtendedFullViewingKey}, }; use crate::{ - address::{RecipientAddress, UnifiedAddress}, + address::UnifiedAddress, decrypt::DecryptedOutput, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, @@ -27,10 +28,7 @@ use crate::{ }; #[cfg(feature = "transparent-inputs")] -use { - crate::wallet::WalletTransparentOutput, - zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}, -}; +use {crate::wallet::WalletTransparentOutput, zcash_primitives::transaction::components::OutPoint}; pub mod chain; pub mod error; @@ -254,13 +252,6 @@ pub struct SentTransaction<'a> { pub utxos_spent: Vec, } -#[derive(Debug, Clone)] -#[allow(clippy::large_enum_variant)] -pub enum Recipient { - Address(RecipientAddress), - InternalAccount(AccountId), -} - /// A value pool to which the wallet supports sending transaction outputs. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum PoolType { @@ -270,9 +261,19 @@ pub enum PoolType { Sapling, } +/// A type that represents the recipient of a transaction output; a recipient address (and, for +/// unified addresses, the pool to which the payment is sent) in the case of outgoing output, or an +/// internal account ID and the pool to which funds were sent in the case of a wallet-internal +/// output. +#[derive(Debug, Clone)] +pub enum Recipient { + Transparent(TransparentAddress), + Sapling(PaymentAddress), + Unified(UnifiedAddress, PoolType), + InternalAccount(AccountId, PoolType), +} + pub struct SentTransactionOutput { - /// The value in which the output has been created - pub output_pool: PoolType, /// The index within the transaction that contains the recipient output. /// /// - If `recipient_address` is a Sapling address, this is an index into the Sapling diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 47bc7aa2c..84df3e2c5 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -374,13 +374,17 @@ where let (tx, tx_metadata) = builder.build(&prover).map_err(Error::Builder)?; let sent_outputs = request.payments().iter().enumerate().map(|(i, payment)| { - let (output_index, output_pool) = match &payment.recipient_address { + let (output_index, recipient) = match &payment.recipient_address { // Sapling outputs are shuffled, so we need to look up where the output ended up. // TODO: When we add Orchard support, we will need to trial-decrypt to find them, // and return the appropriate pool type. - RecipientAddress::Shielded(_) | RecipientAddress::Unified(_) => { + RecipientAddress::Shielded(addr) => { let idx = tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."); - (idx, PoolType::Sapling) + (idx, Recipient::Sapling(addr.clone())) + } + RecipientAddress::Unified(addr) => { + let idx = tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."); + (idx, Recipient::Unified(addr.clone(), PoolType::Sapling)) } RecipientAddress::Transparent(addr) => { let script = addr.script(); @@ -394,14 +398,13 @@ where .map(|(index, _)| index) .expect("An output should exist in the transaction for each transparent payment."); - (idx, PoolType::Transparent) + (idx, Recipient::Transparent(*addr)) } }; SentTransactionOutput { - output_pool, output_index, - recipient: Recipient::Address(payment.recipient_address.clone()), + recipient, value: payment.amount, memo: payment.memo.clone() } @@ -532,10 +535,9 @@ where created: time::OffsetDateTime::now_utc(), account, outputs: vec![SentTransactionOutput { - output_pool: PoolType::Sapling, output_index, value: amount_to_shield, - recipient: Recipient::InternalAccount(account), + recipient: Recipient::InternalAccount(account, PoolType::Sapling), memo: Some(memo.clone()), }], fee_amount: fee, diff --git a/zcash_client_backend/src/encoding.rs b/zcash_client_backend/src/encoding.rs index b63a4aff8..6350c47e6 100644 --- a/zcash_client_backend/src/encoding.rs +++ b/zcash_client_backend/src/encoding.rs @@ -5,14 +5,16 @@ //! //! [constants]: zcash_primitives::constants +use crate::address::UnifiedAddress; use bech32::{self, Error, FromBase32, ToBase32, Variant}; use bs58::{self, decode::Error as Bs58Error}; use std::fmt; use std::io::{self, Write}; +use zcash_address::unified::{self, Encoding}; use zcash_primitives::{ consensus, legacy::TransparentAddress, - sapling::PaymentAddress, + sapling, zip32::{ExtendedFullViewingKey, ExtendedSpendingKey}, }; @@ -132,6 +134,41 @@ impl AddressCodec

for TransparentAddress { } } +impl AddressCodec

for sapling::PaymentAddress { + type Error = Bech32DecodeError; + + fn encode(&self, params: &P) -> String { + encode_payment_address(params.hrp_sapling_payment_address(), self) + } + + fn decode(params: &P, address: &str) -> Result { + decode_payment_address(params.hrp_sapling_payment_address(), address) + } +} + +impl AddressCodec

for UnifiedAddress { + type Error = String; + + fn encode(&self, params: &P) -> String { + self.encode(params) + } + + fn decode(params: &P, address: &str) -> Result { + unified::Address::decode(address) + .map_err(|e| format!("{}", e)) + .and_then(|(network, addr)| { + if params.address_network() == Some(network) { + UnifiedAddress::try_from(addr).map_err(|e| e.to_owned()) + } else { + Err(format!( + "Address {} is for a different network: {:?}", + address, network + )) + } + }) + } +} + /// Writes an [`ExtendedSpendingKey`] as a Bech32-encoded string. /// /// # Examples @@ -232,16 +269,18 @@ pub fn decode_extended_full_viewing_key( /// ); /// ``` /// [`PaymentAddress`]: zcash_primitives::sapling::PaymentAddress -pub fn encode_payment_address(hrp: &str, addr: &PaymentAddress) -> String { +pub fn encode_payment_address(hrp: &str, addr: &sapling::PaymentAddress) -> String { bech32_encode(hrp, |w| w.write_all(&addr.to_bytes())) } /// Writes a [`PaymentAddress`] as a Bech32-encoded string /// using the human-readable prefix values defined in the specified /// network parameters. +/// +/// [`PaymentAddress`]: zcash_primitives::sapling::PaymentAddress pub fn encode_payment_address_p( params: &P, - addr: &PaymentAddress, + addr: &sapling::PaymentAddress, ) -> String { encode_payment_address(params.hrp_sapling_payment_address(), addr) } @@ -259,7 +298,7 @@ pub fn encode_payment_address_p( /// encoding::decode_payment_address, /// }; /// use zcash_primitives::{ -/// constants::testnet::HRP_SAPLING_PAYMENT_ADDRESS, +/// consensus::{TEST_NETWORK, Parameters}, /// sapling::{Diversifier, PaymentAddress}, /// }; /// @@ -276,14 +315,17 @@ pub fn encode_payment_address_p( /// /// assert_eq!( /// decode_payment_address( -/// HRP_SAPLING_PAYMENT_ADDRESS, +/// TEST_NETWORK.hrp_sapling_payment_address(), /// "ztestsapling1qqqqqqqqqqqqqqqqqqcguyvaw2vjk4sdyeg0lc970u659lvhqq7t0np6hlup5lusxle75ss7jnk", /// ), /// Ok(pa), /// ); /// ``` /// [`PaymentAddress`]: zcash_primitives::sapling::PaymentAddress -pub fn decode_payment_address(hrp: &str, s: &str) -> Result { +pub fn decode_payment_address( + hrp: &str, + s: &str, +) -> Result { bech32_decode(hrp, s, |data| { if data.len() != 43 { return None; @@ -291,7 +333,7 @@ pub fn decode_payment_address(hrp: &str, s: &str) -> Result Result Result( /// /// ``` /// use zcash_primitives::{ -/// constants::testnet::{B58_PUBKEY_ADDRESS_PREFIX, B58_SCRIPT_ADDRESS_PREFIX}, +/// consensus::{TEST_NETWORK, Parameters}, /// }; /// use zcash_client_backend::{ /// encoding::decode_transparent_address, @@ -378,8 +420,8 @@ pub fn encode_transparent_address_p( /// /// assert_eq!( /// decode_transparent_address( -/// &B58_PUBKEY_ADDRESS_PREFIX, -/// &B58_SCRIPT_ADDRESS_PREFIX, +/// &TEST_NETWORK.b58_pubkey_address_prefix(), +/// &TEST_NETWORK.b58_script_address_prefix(), /// "tm9iMLAuYMzJ6jtFLcA7rzUmfreGuKvr7Ma", /// ), /// Ok(Some(TransparentAddress::PublicKey([0; 20]))), @@ -387,8 +429,8 @@ pub fn encode_transparent_address_p( /// /// assert_eq!( /// decode_transparent_address( -/// &B58_PUBKEY_ADDRESS_PREFIX, -/// &B58_SCRIPT_ADDRESS_PREFIX, +/// &TEST_NETWORK.b58_pubkey_address_prefix(), +/// &TEST_NETWORK.b58_script_address_prefix(), /// "t26YoyZ1iPgiMEWL4zGUm74eVWfhyDMXzY2", /// ), /// Ok(Some(TransparentAddress::Script([0; 20]))), diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index ee3a3aa38..c3956cfef 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -53,7 +53,7 @@ use zcash_primitives::{ }; use zcash_client_backend::{ - address::{RecipientAddress, UnifiedAddress}, + address::UnifiedAddress, data_api::{ BlockSource, DecryptedTransaction, PoolType, PrunedBlock, Recipient, SentTransaction, WalletRead, WalletWrite, @@ -534,16 +534,15 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { match output.transfer_type { TransferType::Outgoing | TransferType::WalletInternal => { let recipient = if output.transfer_type == TransferType::Outgoing { - Recipient::Address(RecipientAddress::Shielded(output.to.clone())) + Recipient::Sapling(output.to.clone()) } else { - Recipient::InternalAccount(output.account) + Recipient::InternalAccount(output.account, PoolType::Sapling) }; wallet::put_sent_output( up, output.account, tx_ref, - PoolType::Sapling, output.index, &recipient, Amount::from_u64(output.note.value).unwrap(), @@ -577,12 +576,11 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { .any(|input| *nf == input.nullifier) ) { for (output_index, txout) in d_tx.tx.transparent_bundle().iter().flat_map(|b| b.vout.iter()).enumerate() { - let recipient = Recipient::Address(RecipientAddress::Transparent(txout.script_pubkey.address().unwrap())); + let recipient = Recipient::Transparent(txout.script_pubkey.address().unwrap()); wallet::put_sent_output( up, *account_id, tx_ref, - PoolType::Transparent, output_index, &recipient, txout.value, diff --git a/zcash_client_sqlite/src/prepared.rs b/zcash_client_sqlite/src/prepared.rs index 870a3275f..e2b11c3f9 100644 --- a/zcash_client_sqlite/src/prepared.rs +++ b/zcash_client_sqlite/src/prepared.rs @@ -21,15 +21,15 @@ use zcash_primitives::{ use zcash_client_backend::{ address::UnifiedAddress, data_api::{PoolType, Recipient}, + encoding::AddressCodec, }; use crate::{error::SqliteClientError, wallet::pool_code, NoteId, WalletDb}; #[cfg(feature = "transparent-inputs")] use { - crate::UtxoId, - rusqlite::OptionalExtension, - zcash_client_backend::{encoding::AddressCodec, wallet::WalletTransparentOutput}, + crate::UtxoId, rusqlite::OptionalExtension, + zcash_client_backend::wallet::WalletTransparentOutput, zcash_primitives::transaction::components::transparent::OutPoint, }; @@ -368,16 +368,27 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { pub(crate) fn stmt_insert_sent_output( &mut self, tx_ref: i64, - pool_type: PoolType, output_index: usize, from_account: AccountId, to: &Recipient, value: Amount, memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { - let (to_address, to_account) = match to { - Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), - Recipient::Address(shielded) => (Some(shielded.encode(&self.wallet_db.params)), None), + let (to_address, to_account, pool_type) = match to { + Recipient::Transparent(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Transparent, + ), + Recipient::Sapling(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Sapling, + ), + Recipient::Unified(addr, pool) => { + (Some(addr.encode(&self.wallet_db.params)), None, *pool) + } + Recipient::InternalAccount(id, pool) => (None, Some(u32::from(*id)), *pool), }; self.stmt_insert_sent_output.execute(named_params![ @@ -405,13 +416,25 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { value: Amount, memo: Option<&MemoBytes>, tx_ref: i64, - pool_type: PoolType, output_index: usize, ) -> Result { - let (to_address, to_account) = match to { - Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), - Recipient::Address(shielded) => (Some(shielded.encode(&self.wallet_db.params)), None), + let (to_address, to_account, pool_type) = match to { + Recipient::Transparent(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Transparent, + ), + Recipient::Sapling(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Sapling, + ), + Recipient::Unified(addr, pool) => { + (Some(addr.encode(&self.wallet_db.params)), None, *pool) + } + Recipient::InternalAccount(id, pool) => (None, Some(u32::from(*id)), *pool), }; + match self.stmt_update_sent_output.execute(named_params![ ":from_account": &u32::from(from_account), ":to_address": &to_address, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index b325cf86b..70fb64814 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1154,7 +1154,6 @@ pub(crate) fn insert_sent_output<'a, P: consensus::Parameters>( ) -> Result<(), SqliteClientError> { stmts.stmt_insert_sent_output( tx_ref, - output.output_pool, output.output_index, from_account, &output.recipient, @@ -1171,24 +1170,14 @@ pub(crate) fn put_sent_output<'a, P: consensus::Parameters>( stmts: &mut DataConnStmtCache<'a, P>, from_account: AccountId, tx_ref: i64, - output_pool: PoolType, output_index: usize, recipient: &Recipient, value: Amount, memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { - if !stmts.stmt_update_sent_output( - from_account, - recipient, - value, - memo, - tx_ref, - output_pool, - output_index, - )? { + if !stmts.stmt_update_sent_output(from_account, recipient, value, memo, tx_ref, output_index)? { stmts.stmt_insert_sent_output( tx_ref, - output_pool, output_index, from_account, recipient, diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 6773af3df..ccd92629f 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -309,14 +309,17 @@ mod tests { use crate::{ error::SqliteClientError, tests::{self, network}, - wallet::{get_address, pool_code}, + wallet::get_address, AccountId, WalletDb, }; use super::{init_accounts_table, init_blocks_table, init_wallet_db}; #[cfg(feature = "transparent-inputs")] - use {crate::wallet::PoolType, zcash_primitives::legacy::keys as transparent}; + use { + crate::wallet::{pool_code, PoolType}, + zcash_primitives::legacy::keys as transparent, + }; #[test] fn verify_schema() { From fd1640c242481adab9f39deb823ebc713b453963 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 Oct 2022 13:08:15 -0600 Subject: [PATCH 5/8] Update CHANGELOGs --- zcash_client_backend/CHANGELOG.md | 8 +++++++- zcash_client_sqlite/CHANGELOG.md | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 8af79ca45..0e94b70de 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -31,6 +31,7 @@ and this library adheres to Rust's notion of - `zcash_client_backend::address`: - `RecipientAddress::Unified` - `zcash_client_backend::data_api`: + - `PoolType` - `Recipient` - `SentTransactionOutput` - `WalletRead::get_unified_full_viewing_keys` @@ -39,6 +40,8 @@ and this library adheres to Rust's notion of - `WalletWrite::create_account` - `WalletWrite::remove_unmined_tx` (behind the `unstable` feature flag). - `WalletWrite::get_next_available_address` +- `zcash_client_backend::decrypt`: + - `TransferType` - `zcash_client_backend::proto`: - `actions` field on `compact_formats::CompactTx` - `compact_formats::CompactOrchardAction` @@ -46,7 +49,10 @@ and this library adheres to Rust's notion of - `TransactionRequest::new` for constructing a request from `Vec`. - `TransactionRequest::payments` for accessing the `Payments` that make up a request. -- `zcash_client_backend::encoding::KeyError` +- `zcash_client_backend::encoding` + - `KeyError` + - `AddressCodec` implementations for `sapling::PaymentAddress` and + `UnifiedAddress` - New experimental APIs that should be considered unstable, and are likely to be modified and/or moved to a different module in a future release: diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index a617b76c0..dd9379fa5 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -90,7 +90,7 @@ and this library adheres to Rust's notion of ### Removed - The following functions have been removed from the public interface of - `zcash_client_sqlite::wallet`. Prefer methods defined on + `zcash_client_sqlite::wallet`. Prefer methods defined on `zcash_client_backend::data_api::{WalletRead, WalletWrite}` instead. - `get_extended_full_viewing_keys` (use `WalletRead::get_unified_full_viewing_keys` instead). - `insert_sent_note` (use `WalletWrite::store_sent_tx` instead) From 62e1f99eb09a16e824744f57814c4017295742d1 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 Oct 2022 14:42:06 -0600 Subject: [PATCH 6/8] Improve TransferType documentation. --- zcash_client_backend/src/decrypt.rs | 9 ++++++--- zcash_client_sqlite/src/wallet/init.rs | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/zcash_client_backend/src/decrypt.rs b/zcash_client_backend/src/decrypt.rs index 97594c076..7f25f14d0 100644 --- a/zcash_client_backend/src/decrypt.rs +++ b/zcash_client_backend/src/decrypt.rs @@ -18,11 +18,14 @@ use crate::keys::UnifiedFullViewingKey; /// An enumeration of the possible relationships a TXO can have to the wallet. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum TransferType { - /// The transfer was received on one of the wallet's external addresses. + /// The output was received on one of the wallet's external addresses via decryption using the + /// associated incoming viewing key, or at one of the wallet's transparent addresses. Incoming, - /// The transfer was received on one of the wallet's internal-only addresses. + /// The output was received on one of the wallet's internal-only shielded addresses via trial + /// decryption using one of the wallet's internal incoming viewing keys. WalletInternal, - /// The transfer was decrypted using one of the wallet's outgoing viewing keys. + /// The output was decrypted using one of the wallet's outgoing viewing keys, or was created + /// in a transaction constructed by this wallet. Outgoing, } diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index ccd92629f..609897d4a 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -127,7 +127,7 @@ fn init_wallet_db_internal( wdb.conn .execute_batch( "PRAGMA foreign_keys = OFF; - PRAGMA legacy_alter_table = TRUE;", + PRAGMA legacy_alter_table = TRUE;", ) .map_err(|e| MigratorError::Adapter(WalletMigrationError::from(e)))?; let adapter = RusqliteAdapter::new(&mut wdb.conn, Some("schemer_migrations".to_string())); From fdf5aa7b8e6f27dd56d657d8125db466f782a556 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 Oct 2022 15:49:52 -0600 Subject: [PATCH 7/8] Fix exclusive or in sent_notes recipient check. --- zcash_client_sqlite/src/wallet/init.rs | 3 +-- .../src/wallet/init/migrations/sent_notes_to_internal.rs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 609897d4a..cb50cb77c 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -392,8 +392,7 @@ mod tests { FOREIGN KEY (to_account) REFERENCES accounts(account), CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index), CONSTRAINT note_recipient CHECK ( - (to_address IS NOT NULL OR to_account IS NOT NULL) - AND NOT (to_address IS NOT NULL AND to_account IS NOT NULL) + (to_address IS NOT NULL) != (to_account IS NOT NULL) ) )", "CREATE TABLE transactions ( diff --git a/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs b/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs index 5debe5db6..c67777660 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/sent_notes_to_internal.rs @@ -60,8 +60,7 @@ impl RusqliteMigration for Migration { FOREIGN KEY (to_account) REFERENCES accounts(account), CONSTRAINT tx_output UNIQUE (tx, output_pool, output_index), CONSTRAINT note_recipient CHECK ( - (to_address IS NOT NULL OR to_account IS NOT NULL) - AND NOT (to_address IS NOT NULL AND to_account IS NOT NULL) + (to_address IS NOT NULL) != (to_account IS NOT NULL) ) ); INSERT INTO sent_notes_new ( From e666e692301f7fde2e873d04c84f95e3c24a24a0 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 Oct 2022 16:08:47 -0600 Subject: [PATCH 8/8] Address comments from code review. --- zcash_client_backend/src/data_api/wallet.rs | 4 ++-- zcash_client_sqlite/CHANGELOG.md | 9 --------- zcash_client_sqlite/src/lib.rs | 5 +++-- zcash_client_sqlite/src/prepared.rs | 2 ++ 4 files changed, 7 insertions(+), 13 deletions(-) diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 84df3e2c5..bfcb81ee2 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -376,13 +376,13 @@ where let sent_outputs = request.payments().iter().enumerate().map(|(i, payment)| { let (output_index, recipient) = match &payment.recipient_address { // Sapling outputs are shuffled, so we need to look up where the output ended up. - // TODO: When we add Orchard support, we will need to trial-decrypt to find them, - // and return the appropriate pool type. RecipientAddress::Shielded(addr) => { let idx = tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."); (idx, Recipient::Sapling(addr.clone())) } RecipientAddress::Unified(addr) => { + // TODO: When we add Orchard support, we will need to trial-decrypt to find them, + // and return the appropriate pool type. let idx = tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."); (idx, Recipient::Unified(addr.clone(), PoolType::Sapling)) } diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index dd9379fa5..8a72b9ea8 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -76,11 +76,6 @@ and this library adheres to Rust's notion of - `zcash_client_sqlite::wallet`: - `get_spendable_notes` to `get_spendable_sapling_notes`. - `select_spendable_notes` to `select_spendable_sapling_notes`. -- Altered the arguments to `zcash_client_sqlite::wallet::put_sent_note` - to take the components of a `DecryptedOutput` value to allow this - method to be used in contexts where a transaction has just been - constructed, rather than only in the case that a transaction has - been decrypted after being retrieved from the network. - `zcash_client_sqlite::wallet::init_wallet_db` has been modified to take the wallet seed as an argument so that it can correctly perform migrations that require re-deriving key material. In particular for @@ -114,7 +109,6 @@ and this library adheres to Rust's notion of `zcash_client_backend::data_api` traits mentioned above instead. - Deprecated in `zcash_client_sqlite::wallet`: - `get_address` - - `get_extended_full_viewing_keys` - `is_valid_account_extfvk` - `get_balance` - `get_balance_at` @@ -134,9 +128,6 @@ and this library adheres to Rust's notion of - `insert_witness` - `prune_witnesses` - `update_expired_notes` - - `put_sent_utxo` - - `insert_sent_note` - - `insert_sent_utxo` - `get_address` - Deprecated in `zcash_client_sqlite::wallet::transact`: - `get_spendable_sapling_notes` diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index c3956cfef..19c5e29b3 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -545,8 +545,9 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { tx_ref, output.index, &recipient, - Amount::from_u64(output.note.value).unwrap(), - Some(&output.memo) + Amount::from_u64(output.note.value).map_err(|_| + SqliteClientError::CorruptedData("Note value is not a valid Zcash amount.".to_string()))?, + Some(&output.memo), )?; } TransferType::Incoming => { diff --git a/zcash_client_sqlite/src/prepared.rs b/zcash_client_sqlite/src/prepared.rs index e2b11c3f9..741829106 100644 --- a/zcash_client_sqlite/src/prepared.rs +++ b/zcash_client_sqlite/src/prepared.rs @@ -358,6 +358,8 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { /// /// `output_index` is the index within the transaction that contains the recipient output: /// + /// - If `to` is a Unified address, this is an index into the outputs of the transaction + /// within the bundle associated with the recipient's output pool. /// - If `to` is a Sapling address, this is an index into the Sapling outputs of the /// transaction. /// - If `to` is a transparent address, this is an index into the transparent outputs of