From f1f9465f375a0aa77e86b7f586f990d5e2d47a57 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 12 Oct 2022 22:40:51 -0600 Subject: [PATCH 1/3] Remove `received_by_account` field from WalletTransparentOutput Due to how the wallets retrieved unspent transparent outputs from the light wallet server, the account associated with a particular UTXO may not be known by the light wallet. Instead of requiring the caller to perform a separate lookup and match the address of the received UTXO with a known account, it's simpler to perform this lookup internally at the time of insertion or update. In order to make this operation more efficient, the `addresses_table` migration is modified to add a column to cache the transparent receiver so that it may be used in the joins in the UTXO insert and update operations. --- zcash_client_backend/src/wallet.rs | 1 - zcash_client_sqlite/src/prepared.rs | 87 ++++++++++++++----- zcash_client_sqlite/src/wallet.rs | 41 +++------ zcash_client_sqlite/src/wallet/init.rs | 24 ++--- .../wallet/init/migrations/addresses_table.rs | 1 + 5 files changed, 93 insertions(+), 61 deletions(-) diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 6a3419a0e..6f7eb441f 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -31,7 +31,6 @@ pub struct WalletTx { #[cfg(feature = "transparent-inputs")] pub struct WalletTransparentOutput { - pub received_by_account: AccountId, pub outpoint: OutPoint, pub txout: TxOut, pub height: BlockHeight, diff --git a/zcash_client_sqlite/src/prepared.rs b/zcash_client_sqlite/src/prepared.rs index 741829106..5d99dca43 100644 --- a/zcash_client_sqlite/src/prepared.rs +++ b/zcash_client_sqlite/src/prepared.rs @@ -33,6 +33,52 @@ use { zcash_primitives::transaction::components::transparent::OutPoint, }; +pub(crate) struct InsertAddress<'a> { + stmt: Statement<'a>, +} + +impl<'a> InsertAddress<'a> { + pub(crate) fn new(conn: &'a rusqlite::Connection) -> Result { + Ok(InsertAddress { + stmt: conn.prepare( + "INSERT INTO addresses ( + account, + diversifier_index_be, + address, + cached_transparent_receiver_address + ) + VALUES ( + :account, + :diversifier_index_be, + :address, + :cached_transparent_receiver_address + )", + )?, + }) + } + + /// Adds the given address and diversifier index to the addresses table. + /// + /// Returns the database row for the newly-inserted address. + pub(crate) fn execute( + &mut self, + params: &P, + account: AccountId, + mut diversifier_index: DiversifierIndex, + address: &UnifiedAddress, + ) -> Result<(), rusqlite::Error> { + diversifier_index.0.reverse(); + self.stmt.execute(named_params![ + ":account": &u32::from(account), + ":diversifier_index_be": &&diversifier_index.0[..], + ":address": &address.encode(params), + ":cached_transparent_receiver_address": &address.transparent().map(|r| r.encode(params)), + ])?; + + Ok(()) + } +} + /// The primary type used to implement [`WalletWrite`] for the SQLite database. /// /// A data structure that stores the SQLite prepared statements that are @@ -70,7 +116,7 @@ pub struct DataConnStmtCache<'a, P> { stmt_prune_witnesses: Statement<'a>, stmt_update_expired: Statement<'a>, - stmt_insert_address: Statement<'a>, + stmt_insert_address: InsertAddress<'a>, } impl<'a, P> DataConnStmtCache<'a, P> { @@ -119,22 +165,26 @@ impl<'a, P> DataConnStmtCache<'a, P> { received_by_account, address, prevout_txid, prevout_idx, script, value_zat, height) - VALUES ( - :received_by_account, :address, + SELECT + addresses.account, :address, :prevout_txid, :prevout_idx, :script, - :value_zat, :height) + :value_zat, :height + FROM addresses + WHERE addresses.cached_transparent_receiver_address = :address RETURNING id_utxo" )?, #[cfg(feature = "transparent-inputs")] stmt_update_received_transparent_utxo: wallet_db.conn.prepare( "UPDATE utxos - SET received_by_account = :received_by_account, + SET received_by_account = addresses.account, height = :height, address = :address, script = :script, value_zat = :value_zat + FROM addresses WHERE prevout_txid = :prevout_txid AND prevout_idx = :prevout_idx + AND addresses.cached_transparent_receiver_address = :address RETURNING id_utxo" )?, stmt_insert_received_note: wallet_db.conn.prepare( @@ -187,10 +237,7 @@ impl<'a, P> DataConnStmtCache<'a, P> { WHERE id_tx = received_notes.spent AND block IS NULL AND expiry_height < ? )", )?, - stmt_insert_address: wallet_db.conn.prepare( - "INSERT INTO addresses (account, diversifier_index_be, address) - VALUES (:account, :diversifier_index_be, :address)", - )?, + stmt_insert_address: InsertAddress::new(&wallet_db.conn)? } ) } @@ -465,7 +512,6 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { self.stmt_insert_received_transparent_utxo .query_row( named_params![ - ":received_by_account": &u32::from(output.received_by_account), ":address": &output.address().encode(&self.wallet_db.params), ":prevout_txid": &output.outpoint.hash().to_vec(), ":prevout_idx": &output.outpoint.n(), @@ -495,7 +541,6 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { named_params![ ":prevout_txid": &output.outpoint.hash().to_vec(), ":prevout_idx": &output.outpoint.n(), - ":received_by_account": &u32::from(output.received_by_account), ":address": &output.address().encode(&self.wallet_db.params), ":script": &output.txout.script_pubkey.0, ":value_zat": &i64::from(output.txout.value), @@ -516,19 +561,17 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { pub(crate) fn stmt_insert_address( &mut self, account: AccountId, - mut diversifier_index: DiversifierIndex, + diversifier_index: DiversifierIndex, address: &UnifiedAddress, - ) -> Result { - diversifier_index.0.reverse(); - let sql_args: &[(&str, &dyn ToSql)] = &[ - (":account", &u32::from(account)), - (":diversifier_index_be", &&diversifier_index.0[..]), - (":address", &address.encode(&self.wallet_db.params)), - ]; + ) -> Result<(), SqliteClientError> { + self.stmt_insert_address.execute( + &self.wallet_db.params, + account, + diversifier_index, + address, + )?; - self.stmt_insert_address.execute(sql_args)?; - - Ok(self.wallet_db.conn.last_insert_rowid()) + Ok(()) } } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 5e4b23691..2321c87fe 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -33,7 +33,10 @@ use zcash_client_backend::{ DecryptedOutput, }; -use crate::{error::SqliteClientError, DataConnStmtCache, NoteId, WalletDb, PRUNING_HEIGHT}; +use crate::{ + error::SqliteClientError, prepared::InsertAddress, DataConnStmtCache, NoteId, WalletDb, + PRUNING_HEIGHT, +}; #[cfg(feature = "transparent-inputs")] use { @@ -207,19 +210,8 @@ pub(crate) fn add_account_internal::from(account), - ":diversifier_index_be": &&idx.0[..], - ":address": &address_str, - ], - )?; + let (address, d_idx) = key.default_address(); + InsertAddress::new(conn)?.execute(network, account, d_idx, &address)?; Ok(()) } @@ -929,8 +921,7 @@ pub(crate) fn get_unspent_transparent_outputs( max_height: BlockHeight, ) -> Result, SqliteClientError> { let mut stmt_blocks = wdb.conn.prepare( - "SELECT u.received_by_account, - u.prevout_txid, u.prevout_idx, u.script, + "SELECT u.prevout_txid, u.prevout_idx, u.script, u.value_zat, u.height, tx.block as block FROM utxos u LEFT OUTER JOIN transactions tx @@ -943,19 +934,16 @@ pub(crate) fn get_unspent_transparent_outputs( let addr_str = address.encode(&wdb.params); let rows = stmt_blocks.query_map(params![addr_str, u32::from(max_height)], |row| { - let received_by_account: u32 = row.get(0)?; - - let txid: Vec = row.get(1)?; + let txid: Vec = row.get(0)?; let mut txid_bytes = [0u8; 32]; txid_bytes.copy_from_slice(&txid); - let index: u32 = row.get(2)?; - let script_pubkey = Script(row.get(3)?); - let value = Amount::from_i64(row.get(4)?).unwrap(); - let height: u32 = row.get(5)?; + let index: u32 = row.get(1)?; + let script_pubkey = Script(row.get(2)?); + let value = Amount::from_i64(row.get(3)?).unwrap(); + let height: u32 = row.get(4)?; Ok(WalletTransparentOutput { - received_by_account: AccountId::from(received_by_account), outpoint: OutPoint::new(txid_bytes, index), txout: TxOut { value, @@ -1267,12 +1255,11 @@ mod tests { // Add an account to the wallet let mut ops = db_data.get_update_ops().unwrap(); let seed = Secret::new([0u8; 32].to_vec()); - let (account_id, usk) = ops.create_account(&seed).unwrap(); - let (uaddr, _) = usk.to_unified_full_viewing_key().default_address(); + let (account_id, _usk) = ops.create_account(&seed).unwrap(); + let uaddr = db_data.get_current_address(account_id).unwrap().unwrap(); let taddr = uaddr.transparent().unwrap(); let mut utxo = WalletTransparentOutput { - received_by_account: account_id, outpoint: OutPoint::new([1u8; 32], 1), txout: TxOut { value: Amount::from_u64(100000).unwrap(), diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 97b12dcc4..ce71d50fa 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -299,8 +299,6 @@ mod tests { use std::collections::HashMap; use tempfile::NamedTempFile; - use zcash_address::test_vectors; - use zcash_client_backend::{ address::RecipientAddress, encoding::{encode_extended_full_viewing_key, encode_payment_address}, @@ -309,27 +307,30 @@ mod tests { use zcash_primitives::{ block::BlockHash, - consensus::{BlockHeight, BranchId, Network, Parameters}, + consensus::{BlockHeight, BranchId, Parameters}, transaction::{TransactionData, TxVersion}, - zip32::{ - sapling::{DiversifiableFullViewingKey, ExtendedFullViewingKey}, - DiversifierIndex, - }, + zip32::sapling::{DiversifiableFullViewingKey, ExtendedFullViewingKey}, }; use crate::{ error::SqliteClientError, tests::{self, network}, - wallet::{self, get_address}, - AccountId, WalletDb, WalletWrite, + wallet::get_address, + AccountId, WalletDb, }; use super::{init_accounts_table, init_blocks_table, init_wallet_db}; #[cfg(feature = "transparent-inputs")] use { - crate::wallet::{pool_code, PoolType}, - zcash_primitives::legacy::keys as transparent, + crate::{ + wallet::{self, pool_code, PoolType}, + WalletWrite, + }, + zcash_address::test_vectors, + zcash_primitives::{ + consensus::Network, legacy::keys as transparent, zip32::DiversifierIndex, + }, }; #[test] @@ -350,6 +351,7 @@ mod tests { account INTEGER NOT NULL, diversifier_index_be BLOB NOT NULL, address TEXT NOT NULL, + cached_transparent_receiver_address TEXT, FOREIGN KEY (account) REFERENCES accounts(account), CONSTRAINT diversification UNIQUE (account, diversifier_index_be) )", diff --git a/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs b/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs index 511877082..3d3a7fa95 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs @@ -52,6 +52,7 @@ impl RusqliteMigration for Migration

{ account INTEGER NOT NULL, diversifier_index_be BLOB NOT NULL, address TEXT NOT NULL, + cached_transparent_receiver_address TEXT, FOREIGN KEY (account) REFERENCES accounts(account), CONSTRAINT diversification UNIQUE (account, diversifier_index_be) ); From 306d37e706ae56759ba754b1e35d2d00cb46a04a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 13 Oct 2022 09:13:14 -0600 Subject: [PATCH 2/3] Add a test verifying that update fails on missing associated address. --- zcash_client_sqlite/src/wallet.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 2321c87fe..bf0e1ceef 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1212,7 +1212,9 @@ mod tests { #[cfg(feature = "transparent-inputs")] use { - zcash_client_backend::{data_api::WalletWrite, wallet::WalletTransparentOutput}, + zcash_client_backend::{ + data_api::WalletWrite, encoding::AddressCodec, wallet::WalletTransparentOutput, + }, zcash_primitives::{ consensus::BlockHeight, transaction::components::{OutPoint, TxOut}, @@ -1297,5 +1299,18 @@ mod tests { utxos.iter().any(|rutxo| rutxo.height == utxo.height) } )); + + // Artificially delete the address from the addresses table so that + // we can ensure the update fails if the join doesn't work. + db_data + .conn + .execute( + "DELETE FROM addresses WHERE cached_transparent_receiver_address = ?", + [Some(taddr.encode(&db_data.params))], + ) + .unwrap(); + + let res2 = super::put_received_transparent_utxo(&mut ops, &utxo); + assert!(matches!(res2, Err(_))); } } From 5864e71eecf4bee3521df44faaeec64ac0459c46 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 13 Oct 2022 10:38:59 -0600 Subject: [PATCH 3/3] Add comment to call out the storage details of diversifier indices. Co-authored-by: Daira Hopwood --- zcash_client_sqlite/src/prepared.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zcash_client_sqlite/src/prepared.rs b/zcash_client_sqlite/src/prepared.rs index 5d99dca43..a605a7d67 100644 --- a/zcash_client_sqlite/src/prepared.rs +++ b/zcash_client_sqlite/src/prepared.rs @@ -67,6 +67,7 @@ impl<'a> InsertAddress<'a> { mut diversifier_index: DiversifierIndex, address: &UnifiedAddress, ) -> Result<(), rusqlite::Error> { + // the diversifier index is stored in big-endian order to allow sorting diversifier_index.0.reverse(); self.stmt.execute(named_params![ ":account": &u32::from(account),