From d3b7dffa3c582a48dbe633dbcde0b8e8cd8b9d0b Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 2 Aug 2023 13:45:49 -0600 Subject: [PATCH 1/2] zcash_client_backend: Restrict use of backend-specific note identifiers. In general, it is preferable to use globally relevant identifiers where possible. This PR removes the `WalletRead::TxRef` associated type in favor of using `TxId` directly for the transaction identifier, and restricts the use of the `NoteRef` type to those scenarios where the result of one query is intended to be used directly as the input to another query. Closes #834 --- zcash_client_backend/CHANGELOG.md | 23 +++-- zcash_client_backend/src/data_api.rs | 85 +++++++++++-------- zcash_client_backend/src/data_api/wallet.rs | 16 ++-- zcash_client_sqlite/CHANGELOG.md | 4 + zcash_client_sqlite/src/lib.rs | 68 +++++---------- zcash_client_sqlite/src/wallet.rs | 66 ++++++++------ zcash_client_sqlite/src/wallet/init.rs | 9 +- .../migrations/sapling_memo_consistency.rs | 19 +++-- zcash_client_sqlite/src/wallet/sapling.rs | 75 ++++++++-------- 9 files changed, 196 insertions(+), 169 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index f74aed8868..2b1602eb6d 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -12,6 +12,7 @@ and this library adheres to Rust's notion of - `impl Debug` for `zcash_client_backend::{data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote}` - `zcash_client_backend::data_api`: - `BlockMetadata` + - `NoteId` - `NullifierQuery` for use with `WalletRead::get_sapling_nullifiers` - `ScannedBlock` - `ShieldedProtocol` @@ -33,9 +34,14 @@ and this library adheres to Rust's notion of - Bumped dependencies to `hdwallet 0.4`, `zcash_primitives 0.12`, `zcash_note_encryption 0.4`, `incrementalmerkletree 0.4`, `orchard 0.5`, `bs58 0.5` - `zcash_client_backend::data_api`: - - `WalletRead::get_memo` now returns `Result, Self::Error>` - instead of `Result` in order to make representable - wallet states where the full note plaintext is not available. + - `WalletRead::TxRef` has been removed in favor of consistently using `TxId` instead. + - `WalletRead::get_transaction` now takes a `TxId` as its argument. + - `WalletWrite::{store_decrypted_tx, store_sent_tx}` now return `Result<(), Self::Error>` + as the `WalletRead::TxRef` associated type has been removed. Use `TxId` instead. + - `WalletRead::get_memo` now takes a `NoteId` as its argument instead of `Self::NoteRef` + and returns `Result, Self::Error>` instead of `Result` in order to make representable wallet states where the full + note plaintext is not available. - `WalletRead::get_nullifiers` has been renamed to `WalletRead::get_sapling_nullifiers` and its signature has changed; it now subsumes the removed `WalletRead::get_all_nullifiers`. - `WalletRead::get_target_and_anchor_heights` now takes its argument as a `NonZeroU32` @@ -45,15 +51,15 @@ and this library adheres to Rust's notion of - `chain::BlockSource::with_blocks` now takes its limit as an `Option` instead of `Option`. - A new `CommitmentTree` variant has been added to `data_api::error::Error` - - `data_api::wallet::{create_spend_to_address, create_proposed_transaction, + - `wallet::{create_spend_to_address, create_proposed_transaction, shield_transparent_funds}` all now require that `WalletCommitmentTrees` be implemented for the type passed to them for the `wallet_db` parameter. - - `data_api::wallet::create_proposed_transaction` now takes an additional + - `wallet::create_proposed_transaction` now takes an additional `min_confirmations` argument. - - `data_api::wallet::{spend, create_spend_to_address, shield_transparent_funds, + - `wallet::{spend, create_spend_to_address, shield_transparent_funds, propose_transfer, propose_shielding, create_proposed_transaction}` now take their respective `min_confirmations` arguments as `NonZeroU32` - - `data_api::wallet::input_selection::InputSelector::{propose_transaction, propose_shielding}` + - `wallet::input_selection::InputSelector::{propose_transaction, propose_shielding}` now take their respective `min_confirmations` arguments as `NonZeroU32` - A new `Scan` variant has been added to `data_api::chain::error::Error`. - A new `SyncRequired` variant has been added to `data_api::wallet::input_selection::InputSelectorError`. @@ -64,6 +70,9 @@ and this library adheres to Rust's notion of - Arguments to `WalletSaplingOutput::from_parts` have changed. - `zcash_client_backend::data_api::wallet::input_selection::InputSelector`: - Arguments to `{propose_transaction, propose_shielding}` have changed. +- `zcash_client_backend::data_api::wallet::{create_spend_to_address, spend, + create_proposed_transaction, shield_transparent_funds}` now return the `TxId` + for the newly created transaction instead an internal database identifier. - `zcash_client_backend::wallet::ReceivedSaplingNote::note_commitment_tree_position` has replaced the `witness` field in the same struct. - `zcash_client_backend::welding_rig` has been renamed to `zcash_client_backend::scanning` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index fc09fee04f..2e0562257e 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -38,6 +38,8 @@ pub mod wallet; pub const SAPLING_SHARD_HEIGHT: u8 = sapling::NOTE_COMMITMENT_TREE_DEPTH / 2; +/// An enumeration of constraints that can be applied when querying for nullifiers for notes +/// belonging to the wallet. pub enum NullifierQuery { Unspent, All, @@ -58,13 +60,6 @@ pub trait WalletRead { /// or a UUID. type NoteRef: Copy + Debug + Eq + Ord; - /// Backend-specific transaction identifier. - /// - /// For example, this might be a database identifier type - /// or a TxId if the backend is able to support that type - /// directly. - type TxRef: Copy + Debug + Eq + Ord; - /// Returns the minimum and maximum block heights for stored blocks. /// /// This will return `Ok(None)` if no block data is present in the database. @@ -189,14 +184,13 @@ pub trait WalletRead { /// Returns the memo for a note. /// - /// Implementations of this method must return an error if the note identifier - /// does not appear in the backing data store. Returns `Ok(None)` if the note - /// is known to the wallet but memo data has not yet been populated for that - /// note. - fn get_memo(&self, id_note: Self::NoteRef) -> Result, Self::Error>; + /// Returns `Ok(None)` if the note is known to the wallet but memo data has not yet been + /// populated for that note, or if the note identifier does not correspond to a note + /// that is known to the wallet. + fn get_memo(&self, note_id: NoteId) -> Result, Self::Error>; /// Returns a transaction. - fn get_transaction(&self, id_tx: Self::TxRef) -> Result; + fn get_transaction(&self, txid: TxId) -> Result; /// Returns the nullifiers for notes that the wallet is tracking, along with their associated /// account IDs, that are either unspent or have not yet been confirmed as spent (in that a @@ -206,7 +200,7 @@ pub trait WalletRead { query: NullifierQuery, ) -> Result, Self::Error>; - /// Return all unspent Sapling notes. + /// Return all unspent Sapling notes, excluding the specified note IDs. fn get_spendable_sapling_notes( &self, account: AccountId, @@ -380,13 +374,43 @@ pub struct SentTransaction<'a> { } /// A shielded transfer protocol supported by the wallet. -#[derive(Debug, Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] pub enum ShieldedProtocol { /// The Sapling protocol Sapling, // TODO: Orchard } +/// A unique identifier for a shielded transaction output +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct NoteId { + txid: TxId, + protocol: ShieldedProtocol, + output_index: u32, +} + +impl NoteId { + pub fn new(txid: TxId, protocol: ShieldedProtocol, output_index: u32) -> Self { + Self { + txid, + protocol, + output_index, + } + } + + pub fn txid(&self) -> &TxId { + &self.txid + } + + pub fn protocol(&self) -> ShieldedProtocol { + self.protocol + } + + pub fn output_index(&self) -> u32 { + self.output_index + } +} + /// A value pool to which the wallet supports sending transaction outputs. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum PoolType { @@ -508,7 +532,7 @@ pub trait WalletWrite: WalletRead { fn put_blocks( &mut self, blocks: Vec>, - ) -> Result, Self::Error>; + ) -> Result<(), Self::Error>; /// Updates the wallet's view of the blockchain. /// @@ -520,14 +544,11 @@ pub trait WalletWrite: WalletRead { fn update_chain_tip(&mut self, tip_height: BlockHeight) -> Result<(), Self::Error>; /// Caches a decrypted transaction in the persistent wallet store. - fn store_decrypted_tx( - &mut self, - received_tx: DecryptedTransaction, - ) -> Result; + fn store_decrypted_tx(&mut self, received_tx: DecryptedTransaction) -> Result<(), Self::Error>; /// Saves information about a transaction that was constructed and sent by the wallet to the /// persistent wallet store. - fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result; + fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result<(), Self::Error>; /// Truncates the wallet database to the specified height. /// @@ -610,7 +631,7 @@ pub mod testing { use super::{ chain::CommitmentTreeRoot, scanning::ScanRange, BlockMetadata, DecryptedTransaction, - NullifierQuery, ScannedBlock, SentTransaction, WalletCommitmentTrees, WalletRead, + NoteId, NullifierQuery, ScannedBlock, SentTransaction, WalletCommitmentTrees, WalletRead, WalletWrite, SAPLING_SHARD_HEIGHT, }; @@ -635,7 +656,6 @@ pub mod testing { impl WalletRead for MockWalletDb { type Error = (); type NoteRef = u32; - type TxRef = TxId; fn block_height_extrema(&self) -> Result, Self::Error> { Ok(None) @@ -707,11 +727,11 @@ pub mod testing { Ok(Amount::zero()) } - fn get_memo(&self, _id_note: Self::NoteRef) -> Result, Self::Error> { + fn get_memo(&self, _id_note: NoteId) -> Result, Self::Error> { Ok(None) } - fn get_transaction(&self, _id_tx: Self::TxRef) -> Result { + fn get_transaction(&self, _txid: TxId) -> Result { Err(()) } @@ -790,8 +810,8 @@ pub mod testing { fn put_blocks( &mut self, _blocks: Vec>, - ) -> Result, Self::Error> { - Ok(vec![]) + ) -> Result<(), Self::Error> { + Ok(()) } fn update_chain_tip(&mut self, _tip_height: BlockHeight) -> Result<(), Self::Error> { @@ -801,15 +821,12 @@ pub mod testing { fn store_decrypted_tx( &mut self, _received_tx: DecryptedTransaction, - ) -> Result { - Ok(TxId::from_bytes([0u8; 32])) + ) -> Result<(), Self::Error> { + Ok(()) } - fn store_sent_tx( - &mut self, - _sent_tx: &SentTransaction, - ) -> Result { - Ok(TxId::from_bytes([0u8; 32])) + fn store_sent_tx(&mut self, _sent_tx: &SentTransaction) -> Result<(), Self::Error> { + Ok(()) } fn truncate_to_height(&mut self, _block_height: BlockHeight) -> Result<(), Self::Error> { diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 2d09231e8d..c202a597b0 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -1,7 +1,7 @@ -use std::fmt::Debug; use std::{convert::Infallible, num::NonZeroU32}; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; +use zcash_primitives::transaction::TxId; use zcash_primitives::{ consensus::{self, BlockHeight, NetworkUpgrade}, memo::MemoBytes, @@ -203,7 +203,7 @@ pub fn create_spend_to_address( ovk_policy: OvkPolicy, min_confirmations: NonZeroU32, ) -> Result< - DbT::TxRef, + TxId, Error< ::Error, ::Error, @@ -306,7 +306,7 @@ pub fn spend( ovk_policy: OvkPolicy, min_confirmations: NonZeroU32, ) -> Result< - DbT::TxRef, + TxId, Error< ::Error, ::Error, @@ -317,7 +317,6 @@ pub fn spend( > where DbT: WalletWrite + WalletCommitmentTrees, - DbT::TxRef: Copy + Debug, DbT::NoteRef: Copy + Eq + Ord, ParamsT: consensus::Parameters + Clone, InputsT: InputSelector, @@ -441,7 +440,7 @@ pub fn create_proposed_transaction( min_confirmations: NonZeroU32, change_memo: Option, ) -> Result< - DbT::TxRef, + TxId, Error< ::Error, ::Error, @@ -452,7 +451,6 @@ pub fn create_proposed_transaction( > where DbT: WalletWrite + WalletCommitmentTrees, - DbT::TxRef: Copy + Debug, DbT::NoteRef: Copy + Eq + Ord, ParamsT: consensus::Parameters + Clone, FeeRuleT: FeeRule, @@ -670,7 +668,9 @@ where #[cfg(feature = "transparent-inputs")] utxos_spent: utxos.iter().map(|utxo| utxo.outpoint().clone()).collect(), }) - .map_err(Error::DataSource) + .map_err(Error::DataSource)?; + + Ok(tx.txid()) } /// Constructs a transaction that consumes available transparent UTXOs belonging to @@ -720,7 +720,7 @@ pub fn shield_transparent_funds( memo: &MemoBytes, min_confirmations: NonZeroU32, ) -> Result< - DbT::TxRef, + TxId, Error< ::Error, ::Error, diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 8700b10425..8b2da642a9 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -12,6 +12,7 @@ and this library adheres to Rust's notion of - A new default-enabled feature flag `multicore`. This allows users to disable multicore support by setting `default_features = false` on their `zcash_primitives`, `zcash_proofs`, and `zcash_client_sqlite` dependencies. +- `zcash_client_sqlite::ReceivedNoteId` - `zcash_client_sqlite::wallet::commitment_tree` A new module containing a sqlite-backed implementation of `shardtree::store::ShardStore`. @@ -30,6 +31,9 @@ and this library adheres to Rust's notion of ### Removed - The empty `wallet::transact` module has been removed. +- `zcash_client_sqlite::NoteId` has been replaced with `zcash_client_sqlite::ReceivedNoteId` + as the `SentNoteId` variant of is now unused following changes to + `zcash_client_backend::data_api::WalletRead`. ### Fixed - Fixed an off-by-one error in the `BlockSource` implementation for the SQLite-backed diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 5b92d9e3d5..60c96d831c 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -38,7 +38,7 @@ use maybe_rayon::{ }; use rusqlite::{self, Connection}; use secrecy::{ExposeSecret, SecretVec}; -use std::{borrow::Borrow, collections::HashMap, convert::AsRef, fmt, ops::Range, path::Path}; +use std::{borrow::Borrow, collections::HashMap, convert::AsRef, ops::Range, path::Path}; use incrementalmerkletree::Position; use shardtree::{error::ShardTreeError, ShardTree}; @@ -61,9 +61,9 @@ use zcash_client_backend::{ self, chain::{BlockSource, CommitmentTreeRoot}, scanning::{ScanPriority, ScanRange}, - BlockMetadata, DecryptedTransaction, NullifierQuery, PoolType, Recipient, ScannedBlock, - SentTransaction, ShieldedProtocol, WalletCommitmentTrees, WalletRead, WalletWrite, - SAPLING_SHARD_HEIGHT, + BlockMetadata, DecryptedTransaction, NoteId, NullifierQuery, PoolType, Recipient, + ScannedBlock, SentTransaction, ShieldedProtocol, WalletCommitmentTrees, WalletRead, + WalletWrite, SAPLING_SHARD_HEIGHT, }, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, @@ -97,22 +97,9 @@ pub(crate) const VERIFY_LOOKAHEAD: u32 = 10; pub(crate) const SAPLING_TABLES_PREFIX: &str = "sapling"; -/// A newtype wrapper for sqlite primary key values for the notes -/// table. +/// A newtype wrapper for received note identifiers. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] -pub enum NoteId { - SentNoteId(i64), - ReceivedNoteId(i64), -} - -impl fmt::Display for NoteId { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - NoteId::SentNoteId(id) => write!(f, "Sent Note {}", id), - NoteId::ReceivedNoteId(id) => write!(f, "Received Note {}", id), - } - } -} +pub struct ReceivedNoteId(pub(crate) i64); /// A newtype wrapper for sqlite primary key values for the utxos /// table. @@ -160,8 +147,7 @@ impl WalletDb { impl, P: consensus::Parameters> WalletRead for WalletDb { type Error = SqliteClientError; - type NoteRef = NoteId; - type TxRef = i64; + type NoteRef = ReceivedNoteId; fn block_height_extrema(&self) -> Result, Self::Error> { wallet::block_height_extrema(self.conn.borrow()).map_err(SqliteClientError::from) @@ -229,17 +215,17 @@ impl, P: consensus::Parameters> WalletRead for W wallet::get_balance_at(self.conn.borrow(), account, anchor_height) } - fn get_memo(&self, id_note: Self::NoteRef) -> Result, Self::Error> { - match id_note { - NoteId::SentNoteId(id_note) => wallet::get_sent_memo(self.conn.borrow(), id_note), - NoteId::ReceivedNoteId(id_note) => { - wallet::get_received_memo(self.conn.borrow(), id_note) - } + fn get_memo(&self, note_id: NoteId) -> Result, Self::Error> { + let sent_memo = wallet::get_sent_memo(self.conn.borrow(), note_id)?; + if sent_memo.is_some() { + Ok(sent_memo) + } else { + wallet::get_received_memo(self.conn.borrow(), note_id) } } - fn get_transaction(&self, id_tx: i64) -> Result { - wallet::get_transaction(self.conn.borrow(), &self.params, id_tx).map(|(_, tx)| tx) + fn get_transaction(&self, txid: TxId) -> Result { + wallet::get_transaction(self.conn.borrow(), &self.params, txid).map(|(_, tx)| tx) } fn get_sapling_nullifiers( @@ -404,7 +390,7 @@ impl WalletWrite for WalletDb fn put_blocks( &mut self, blocks: Vec>, - ) -> Result, Self::Error> { + ) -> Result<(), Self::Error> { self.transactionally(|wdb| { let start_positions = blocks.first().map(|block| { ( @@ -415,7 +401,6 @@ impl WalletWrite for WalletDb ), ) }); - let mut wallet_note_ids = vec![]; let mut sapling_commitments = vec![]; let mut last_scanned_height = None; let mut note_positions = vec![]; @@ -453,12 +438,7 @@ impl WalletWrite for WalletDb output.nf(), )?; - let received_note_id = wallet::sapling::put_received_note( - wdb.conn.0, output, tx_row, spent_in, - )?; - - // Save witness for note. - wallet_note_ids.push(received_note_id); + wallet::sapling::put_received_note(wdb.conn.0, output, tx_row, spent_in)?; } } @@ -535,7 +515,7 @@ impl WalletWrite for WalletDb )?; } - Ok(wallet_note_ids) + Ok(()) }) } @@ -546,10 +526,7 @@ impl WalletWrite for WalletDb Ok(()) } - fn store_decrypted_tx( - &mut self, - d_tx: DecryptedTransaction, - ) -> Result { + fn store_decrypted_tx(&mut self, d_tx: DecryptedTransaction) -> Result<(), Self::Error> { self.transactionally(|wdb| { let tx_ref = wallet::put_tx_data(wdb.conn.0, d_tx.tx, None, None)?; @@ -636,11 +613,11 @@ impl WalletWrite for WalletDb } } - Ok(tx_ref) + Ok(()) }) } - fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result { + fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result<(), Self::Error> { self.transactionally(|wdb| { let tx_ref = wallet::put_tx_data( wdb.conn.0, @@ -699,8 +676,7 @@ impl WalletWrite for WalletDb } } - // Return the row number of the transaction, so the caller can fetch it for sending. - Ok(tx_ref) + Ok(()) }) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 307eb59bf4..dc399a9ba3 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -70,7 +70,7 @@ use std::convert::TryFrom; use std::io::{self, Cursor}; use zcash_client_backend::data_api::scanning::{ScanPriority, ScanRange}; -use zcash_client_backend::data_api::ShieldedProtocol; +use zcash_client_backend::data_api::{NoteId, ShieldedProtocol}; use zcash_primitives::transaction::TransactionData; use zcash_primitives::{ @@ -474,20 +474,27 @@ pub(crate) fn get_balance_at( } } -/// Returns the memo for a received note. -/// -/// The note is identified by its row index in the `sapling_received_notes` table within the wdb -/// database. +/// Returns the memo for a received note, if the note is known to the wallet. pub(crate) fn get_received_memo( conn: &rusqlite::Connection, - id_note: i64, + note_id: NoteId, ) -> Result, SqliteClientError> { - let memo_bytes: Option> = conn.query_row( - "SELECT memo FROM sapling_received_notes - WHERE id_note = ?", - [id_note], - |row| row.get(0), - )?; + let memo_bytes: Option> = match note_id.protocol() { + ShieldedProtocol::Sapling => conn + .query_row( + "SELECT memo FROM sapling_received_notes + JOIN transactions ON sapling_received_notes.tx = transactions.id_tx + WHERE transactions.txid = :txid + AND sapling_received_notes.output_index = :output_index", + named_params![ + ":txid": note_id.txid().as_ref(), + ":output_index": note_id.output_index() + ], + |row| row.get(0), + ) + .optional()? + .flatten(), + }; memo_bytes .map(|b| { @@ -507,7 +514,7 @@ pub(crate) fn get_received_memo( pub(crate) fn get_transaction( conn: &rusqlite::Connection, params: &P, - id_tx: i64, + txid: TxId, ) -> Result<(BlockHeight, Transaction), SqliteClientError> { let (tx_bytes, block_height, expiry_height): ( Vec<_>, @@ -515,8 +522,8 @@ pub(crate) fn get_transaction( Option, ) = conn.query_row( "SELECT raw, block, expiry_height FROM transactions - WHERE id_tx = ?", - [id_tx], + WHERE txid = ?", + [txid.as_ref()], |row| { let h: Option = row.get(1)?; let expiry: Option = row.get(2)?; @@ -572,20 +579,27 @@ pub(crate) fn get_transaction( } } -/// Returns the memo for a sent note. -/// -/// The note is identified by its row index in the `sent_notes` table within the wdb -/// database. +/// Returns the memo for a sent note, if the sent note is known to the wallet. pub(crate) fn get_sent_memo( conn: &rusqlite::Connection, - id_note: i64, + note_id: NoteId, ) -> Result, SqliteClientError> { - let memo_bytes: Option> = conn.query_row( - "SELECT memo FROM sent_notes - WHERE id_note = ?", - [id_note], - |row| row.get(0), - )?; + let memo_bytes: Option> = conn + .query_row( + "SELECT memo FROM sent_notes + JOIN transactions ON sent_notes.tx = transactions.id_tx + WHERE transactions.txid = :txid + AND sent_notes.output_pool = :pool_code + AND sent_notes.output_index = :output_index", + named_params![ + ":txid": note_id.txid().as_ref(), + ":pool_code": pool_code(PoolType::Shielded(note_id.protocol())), + ":output_index": note_id.output_index() + ], + |row| row.get(0), + ) + .optional()? + .flatten(); memo_bytes .map(|b| { diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 540a8fcb10..a146425169 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -346,7 +346,7 @@ pub fn init_blocks_table( #[cfg(test)] #[allow(deprecated)] mod tests { - use rusqlite::{self, ToSql}; + use rusqlite::{self, named_params, ToSql}; use secrecy::Secret; use std::collections::HashMap; use tempfile::NamedTempFile; @@ -969,8 +969,11 @@ mod tests { let mut tx_bytes = vec![]; tx.write(&mut tx_bytes).unwrap(); wdb.conn.execute( - "INSERT INTO transactions (block, id_tx, txid, raw) VALUES (0, 0, '', ?)", - [&tx_bytes[..]], + "INSERT INTO transactions (block, id_tx, txid, raw) VALUES (0, 0, :txid, :tx_bytes)", + named_params![ + ":txid": tx.txid().as_ref(), + ":tx_bytes": &tx_bytes[..] + ], )?; wdb.conn.execute( "INSERT INTO sent_notes (tx, output_index, from_account, address, value) diff --git a/zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs b/zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs index 4cfc983827..582904359d 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs @@ -8,7 +8,7 @@ use rusqlite::named_params; use schemer_rusqlite::RusqliteMigration; use uuid::Uuid; use zcash_client_backend::{decrypt_transaction, keys::UnifiedFullViewingKey}; -use zcash_primitives::{consensus, zip32::AccountId}; +use zcash_primitives::{consensus, transaction::TxId, zip32::AccountId}; use crate::{ error::SqliteClientError, @@ -46,7 +46,9 @@ impl RusqliteMigration for Migration

{ fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { let mut stmt_raw_tx = transaction.prepare( - "SELECT DISTINCT sent_notes.tx, accounts.account, accounts.ufvk + "SELECT DISTINCT + transactions.id_tx, transactions.txid, + accounts.account, accounts.ufvk FROM sent_notes JOIN accounts ON sent_notes.from_account = accounts.account JOIN transactions ON transactions.id_tx = sent_notes.tx @@ -55,12 +57,13 @@ impl RusqliteMigration for Migration

{ let mut rows = stmt_raw_tx.query([])?; - let mut tx_sent_notes: BTreeMap> = + let mut tx_sent_notes: BTreeMap<(i64, TxId), HashMap> = BTreeMap::new(); while let Some(row) = rows.next()? { let id_tx: i64 = row.get(0)?; - let account: u32 = row.get(1)?; - let ufvk_str: String = row.get(2)?; + let txid = row.get(1).map(TxId::from_bytes)?; + let account: u32 = row.get(2)?; + let ufvk_str: String = row.get(3)?; let ufvk = UnifiedFullViewingKey::decode(&self.params, &ufvk_str).map_err(|e| { WalletMigrationError::CorruptedData(format!( "Could not decode unified full viewing key for account {}: {:?}", @@ -69,7 +72,7 @@ impl RusqliteMigration for Migration

{ })?; tx_sent_notes - .entry(id_tx) + .entry((id_tx, txid)) .or_default() .insert(AccountId::from(account), ufvk); } @@ -81,9 +84,9 @@ impl RusqliteMigration for Migration

{ AND output_index = :output_index", )?; - for (id_tx, ufvks) in tx_sent_notes { + for ((id_tx, txid), ufvks) in tx_sent_notes { let (block_height, tx) = - get_transaction(transaction, &self.params, id_tx).map_err(|err| match err { + get_transaction(transaction, &self.params, txid).map_err(|err| match err { SqliteClientError::CorruptedData(msg) => { WalletMigrationError::CorruptedData(msg) } diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 88cf3b2098..b987322010 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -18,7 +18,7 @@ use zcash_client_backend::{ DecryptedOutput, TransferType, }; -use crate::{error::SqliteClientError, NoteId}; +use crate::{error::SqliteClientError, ReceivedNoteId}; use super::memo_repr; @@ -81,8 +81,8 @@ impl ReceivedSaplingOutput for DecryptedOutput { } } -fn to_spendable_note(row: &Row) -> Result, SqliteClientError> { - let note_id = NoteId::ReceivedNoteId(row.get(0)?); +fn to_spendable_note(row: &Row) -> Result, SqliteClientError> { + let note_id = ReceivedNoteId(row.get(0)?); let diversifier = { let d: Vec<_> = row.get(1)?; if d.len() != 11 { @@ -130,8 +130,8 @@ pub(crate) fn get_spendable_sapling_notes( conn: &Connection, account: AccountId, anchor_height: BlockHeight, - exclude: &[NoteId], -) -> Result>, SqliteClientError> { + exclude: &[ReceivedNoteId], +) -> Result>, SqliteClientError> { let mut stmt_select_notes = conn.prepare_cached( "SELECT id_note, diversifier, value, rcm, commitment_tree_position FROM sapling_received_notes @@ -142,13 +142,7 @@ pub(crate) fn get_spendable_sapling_notes( AND id_note NOT IN rarray(:exclude)", )?; - let excluded: Vec = exclude - .iter() - .filter_map(|n| match n { - NoteId::ReceivedNoteId(i) => Some(Value::from(*i)), - NoteId::SentNoteId(_) => None, - }) - .collect(); + let excluded: Vec = exclude.iter().map(|n| Value::from(n.0)).collect(); let excluded_ptr = Rc::new(excluded); let notes = stmt_select_notes.query_and_then( @@ -168,8 +162,8 @@ pub(crate) fn select_spendable_sapling_notes( account: AccountId, target_value: Amount, anchor_height: BlockHeight, - exclude: &[NoteId], -) -> Result>, SqliteClientError> { + exclude: &[ReceivedNoteId], +) -> Result>, SqliteClientError> { // The goal of this SQL statement is to select the oldest notes until the required // value has been reached, and then fetch the witnesses at the desired height for the // selected notes. This is achieved in several steps: @@ -207,13 +201,7 @@ pub(crate) fn select_spendable_sapling_notes( FROM (SELECT * from eligible WHERE so_far >= :target_value LIMIT 1)", )?; - let excluded: Vec = exclude - .iter() - .filter_map(|n| match n { - NoteId::ReceivedNoteId(i) => Some(Value::from(*i)), - NoteId::SentNoteId(_) => None, - }) - .collect(); + let excluded: Vec = exclude.iter().map(|n| Value::from(n.0)).collect(); let excluded_ptr = Rc::new(excluded); let notes = stmt_select_notes.query_and_then( @@ -313,7 +301,7 @@ pub(crate) fn put_received_note( output: &T, tx_ref: i64, spent_in: Option, -) -> Result { +) -> Result<(), SqliteClientError> { let mut stmt_upsert_received_note = conn.prepare_cached( "INSERT INTO sapling_received_notes (tx, output_index, account, diversifier, value, rcm, memo, nf, is_change, spent, commitment_tree_position) @@ -339,8 +327,7 @@ pub(crate) fn put_received_note( memo = IFNULL(:memo, memo), is_change = IFNULL(:is_change, is_change), spent = IFNULL(:spent, spent), - commitment_tree_position = IFNULL(:commitment_tree_position, commitment_tree_position) - RETURNING id_note", + commitment_tree_position = IFNULL(:commitment_tree_position, commitment_tree_position)", )?; let rcm = output.note().rcm().to_repr(); @@ -362,10 +349,10 @@ pub(crate) fn put_received_note( ]; stmt_upsert_received_note - .query_row(sql_args, |row| { - row.get::<_, i64>(0).map(NoteId::ReceivedNoteId) - }) - .map_err(SqliteClientError::from) + .execute(sql_args) + .map_err(SqliteClientError::from)?; + + Ok(()) } #[cfg(test)] @@ -403,7 +390,7 @@ pub(crate) mod tests { create_proposed_transaction, create_spend_to_address, input_selection::GreedyInputSelector, propose_transfer, spend, }, - WalletRead, WalletWrite, + ShieldedProtocol, WalletRead, WalletWrite, }, decrypt_transaction, fees::{fixed, zip317, DustOutputPolicy}, @@ -566,13 +553,24 @@ pub(crate) mod tests { // Verify that the stored sent notes match what we're expecting let mut stmt_sent_notes = db_data .conn - .prepare("SELECT id_note FROM sent_notes WHERE tx = ?") + .prepare( + "SELECT output_index + FROM sent_notes + JOIN transactions ON transactions.id_tx = sent_notes.tx + WHERE transactions.txid = ?", + ) .unwrap(); let sent_note_ids = stmt_sent_notes - .query(rusqlite::params![sent_tx_id]) + .query(rusqlite::params![sent_tx_id.as_ref()]) .unwrap() - .mapped(|row| row.get::<_, i64>(0).map(NoteId::SentNoteId)) + .mapped(|row| { + Ok(NoteId::new( + sent_tx_id, + ShieldedProtocol::Sapling, + row.get(0)?, + )) + }) .collect::, _>>() .unwrap(); @@ -601,8 +599,11 @@ pub(crate) mod tests { assert!(found_sent_change_memo); assert!(found_sent_empty_memo); - // Check that querying for a nonexistent sent note returns an error - assert_matches!(db_data.get_memo(NoteId::SentNoteId(12345)), Err(_)); + // Check that querying for a nonexistent sent note returns None + assert_matches!( + db_data.get_memo(NoteId::new(sent_tx_id, ShieldedProtocol::Sapling, 12345)), + Ok(None) + ); } #[test] @@ -1101,7 +1102,7 @@ pub(crate) mod tests { let to = addr2.into(); let send_and_recover_with_policy = |db_data: &mut WalletDb, ovk_policy| { - let tx_row = create_spend_to_address( + let txid = create_spend_to_address( db_data, &tests::network(), test_prover(), @@ -1119,8 +1120,8 @@ pub(crate) mod tests { .conn .query_row( "SELECT raw FROM transactions - WHERE id_tx = ?", - [tx_row], + WHERE txid = ?", + [txid.as_ref()], |row| row.get(0), ) .unwrap(); From f602ec125d4de4b32e79ba583113ec57ff01c56d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Aug 2023 11:25:27 -0600 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: str4d --- zcash_client_backend/CHANGELOG.md | 3 ++- zcash_client_backend/src/data_api.rs | 11 ++++++++--- zcash_client_sqlite/src/wallet.rs | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 2b1602eb6d..c76b580eaf 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -37,7 +37,8 @@ and this library adheres to Rust's notion of - `WalletRead::TxRef` has been removed in favor of consistently using `TxId` instead. - `WalletRead::get_transaction` now takes a `TxId` as its argument. - `WalletWrite::{store_decrypted_tx, store_sent_tx}` now return `Result<(), Self::Error>` - as the `WalletRead::TxRef` associated type has been removed. Use `TxId` instead. + as the `WalletRead::TxRef` associated type has been removed. Use + `WalletRead::get_transaction` with the transaction's `TxId` instead. - `WalletRead::get_memo` now takes a `NoteId` as its argument instead of `Self::NoteRef` and returns `Result, Self::Error>` instead of `Result` in order to make representable wallet states where the full diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 2e0562257e..af11160315 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -386,11 +386,12 @@ pub enum ShieldedProtocol { pub struct NoteId { txid: TxId, protocol: ShieldedProtocol, - output_index: u32, + output_index: u16, } impl NoteId { - pub fn new(txid: TxId, protocol: ShieldedProtocol, output_index: u32) -> Self { + /// Constructs a new `NoteId` from its parts. + pub fn new(txid: TxId, protocol: ShieldedProtocol, output_index: u16) -> Self { Self { txid, protocol, @@ -398,15 +399,19 @@ impl NoteId { } } + /// Returns the ID of the transaction containing this note. pub fn txid(&self) -> &TxId { &self.txid } + /// Returns the shielded protocol used by this note. pub fn protocol(&self) -> ShieldedProtocol { self.protocol } - pub fn output_index(&self) -> u32 { + /// Returns the index of this note within its transaction's corresponding list of + /// shielded outputs. + pub fn output_index(&self) -> u16 { self.output_index } } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index dc399a9ba3..ac60f2ced6 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -505,7 +505,7 @@ pub(crate) fn get_received_memo( .transpose() } -/// Looks up a transaction by its internal database identifier. +/// Looks up a transaction by its [`TxId`]. /// /// Returns the decoded transaction, along with the block height that was used in its decoding. /// This is either the block height at which the transaction was mined, or the expiry height if the