From 3bba6791e94d1665aa07d89146c61965450291e7 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 6 Feb 2025 09:32:34 -0700 Subject: [PATCH] Address comments from code review. --- zcash_client_backend/src/data_api.rs | 10 ++-- .../src/data_api/testing/pool.rs | 2 +- zcash_client_sqlite/CHANGELOG.md | 2 + zcash_client_sqlite/src/error.rs | 16 ++++-- zcash_client_sqlite/src/lib.rs | 51 ++++++++++++++++--- zcash_client_sqlite/src/testing/pool.rs | 2 +- zcash_client_sqlite/src/wallet.rs | 5 +- zcash_client_sqlite/src/wallet/db.rs | 30 +++++------ zcash_client_sqlite/src/wallet/init.rs | 2 +- .../migrations/fix_broken_commitment_trees.rs | 11 +++- .../transparent_gap_limit_handling.rs | 9 ++-- zcash_client_sqlite/src/wallet/transparent.rs | 43 ++++++++-------- zcash_transparent/CHANGELOG.md | 1 + 13 files changed, 123 insertions(+), 61 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 26f60d6a50..408959b8ad 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1390,7 +1390,7 @@ pub trait WalletRead { /// This is equivalent to (but may be implemented more efficiently than): /// ```compile_fail /// Ok( - /// if let Some(result) = self.get_transparent_receivers(account, true, true)?.get(address) { + /// if let Some(result) = self.get_transparent_receivers(account, true)?.get(address) { /// result.clone() /// } else { /// self.get_known_ephemeral_addresses(account, None)? @@ -2386,10 +2386,10 @@ pub trait WalletWrite: WalletRead { /// at the provided diversifier index. If the `request` parameter is `None`, an address should /// be generated using all of the available receivers for the account's UFVK. /// - /// In the case that the diversifier index is outside of the range of valid transparent address - /// indexes, no transparent receiver should be generated in the resulting unified address. If a - /// transparent receiver is specifically requested for such a diversifier index, - /// implementations of this method should return an error. + /// Returns `Ok(None)` in the case that it is not possible to generate an address conforming + /// to the provided request at the specified diversifier index. Such a result might arise from + /// either a key not being available or the diversifier index not being valid for a + /// [`ReceiverRequirement::Require`]'ed receiver. /// /// Address generation should fail if a transparent receiver would be generated that violates /// the backend's internally configured gap limit for HD-seed-based recovery. diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 1005ae1592..977aef9037 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -796,7 +796,6 @@ pub fn send_multi_step_proposed_transfer( // Now, store the transaction, pretending it has been mined (we will actually mine the block // next). This will cause the the gap start to move & a new `EPHEMERAL_ADDR_GAP_LIMIT` of - // addresses to be created. let target_height = st.latest_cached_block().unwrap().height() + 1; st.wallet_mut() @@ -815,6 +814,7 @@ pub fn send_multi_step_proposed_transfer( // `store_decrypted_tx` into the database. let (h, _) = st.generate_next_block_including(txid); st.scan_cached_blocks(h, 1); + assert_eq!(h, target_height); // At this point the start of the gap should be at index `EPHEMERAL_ADDR_GAP_LIMIT` and the new // size of the known address set should be `EPHEMERAL_ADDR_GAP_LIMIT * 2`. diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 778c2a2041..786f5269d7 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -9,6 +9,8 @@ and this library adheres to Rust's notion of ### Added - `zcash_client_sqlite::WalletDb::from_connection` +- `zcash_client_sqlite::WalletDb::with_gap_limits` +- `zcash_client_sqlite::GapLimits` ### Changed - MSRV is now 1.81.0. diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index f37d2a2913..75502c597e 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -14,7 +14,10 @@ use zcash_protocol::{consensus::BlockHeight, value::BalanceError, PoolType, TxId use crate::{wallet::commitment_tree, AccountUuid}; #[cfg(feature = "transparent-inputs")] -use {::transparent::address::TransparentAddress, zcash_keys::encoding::TransparentCodecError}; +use { + crate::wallet::KeyScope, ::transparent::address::TransparentAddress, + zcash_keys::encoding::TransparentCodecError, zip32::Scope, +}; /// The primary error type for the SQLite wallet backend. #[derive(Debug)] @@ -121,7 +124,7 @@ pub enum SqliteClientError { /// containing outputs belonging to a previously reserved address has been mined. The error /// contains the index that could not safely be reserved. #[cfg(feature = "transparent-inputs")] - ReachedGapLimit(u32), + ReachedGapLimit(u32, KeyScope), /// The wallet attempted to create a transaction that would use of one of the wallet's /// previously-used addresses, potentially creating a problem with on-chain transaction @@ -184,9 +187,14 @@ impl fmt::Display for SqliteClientError { SqliteClientError::BalanceError(e) => write!(f, "Balance error: {}", e), SqliteClientError::NoteFilterInvalid(s) => write!(f, "Could not evaluate filter query: {:?}", s), #[cfg(feature = "transparent-inputs")] - SqliteClientError::ReachedGapLimit(bad_index) => write!(f, - "The proposal cannot be constructed until a transaction with outputs to a previously reserved addresse has been mined. \ + SqliteClientError::ReachedGapLimit(bad_index, key_scope) => write!(f, + "The proposal cannot be constructed until a transaction with outputs to a previously reserved {} address has been mined. \ The address at index {bad_index} could not be safely reserved.", + match key_scope { + KeyScope::Zip32(Scope::External) => "external transparent", + KeyScope::Zip32(Scope::Internal) => "transparent change", + KeyScope::Ephemeral => "ephemeral transparent", + } ), SqliteClientError::AddressReuse(address_str, txids) => { write!(f, "The address {address_str} previously used in txid(s) {:?} would be reused.", txids) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 2c06373f0a..afb6536c1c 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -147,9 +147,10 @@ use { pub mod chain; pub mod error; +pub mod wallet; + #[cfg(test)] mod testing; -pub mod wallet; /// The maximum number of blocks the wallet is allowed to rewind. This is /// consistent with the bound in zcashd, and allows block data deeper than @@ -271,7 +272,18 @@ pub struct GapLimits { #[cfg(feature = "transparent-inputs")] impl GapLimits { - pub fn new(external: u32, internal: u32, ephemeral: u32) -> Self { + /// Constructs a new `GapLimits` value from its constituent parts. + /// + /// The following gap limits are recommended for use with this crate, and are supplied by the + /// [`Default`] implementation for this type. + /// - external addresses: 10 + /// - transparent internal (change) addresses: 5 + /// - ephemeral addresses: 3 + /// + /// This constructor is only available under the `unstable` feature, as it is not recommended + /// for general use. + #[cfg(feature = "unstable")] + pub fn from_parts(external: u32, internal: u32, ephemeral: u32) -> Self { Self { external, internal, @@ -294,9 +306,27 @@ impl GapLimits { /// The default gap limits supported by this implementation are: /// -/// external addresses: 10 -/// transparent internal (change) addresses: 5 -/// ephemeral addresses: 3 +/// - external addresses: 10 +/// - transparent internal (change) addresses: 5 +/// - ephemeral addresses: 3 +/// +/// These limits are chosen with the following rationale: +/// - At present, many wallets query light wallet servers with a set of addresses, because querying +/// for each address independently and in a fashion that is not susceptible to clustering via +/// timing correlation leads to undesirable delays in discovery of received funds. As such, it is +/// desirable to minimize the number of addresses that can be "linked", i.e. understood by the +/// light wallet server to all belong to the same wallet. +/// - For transparent change addresses and ephemeral addresses, it is always expected that an +/// address will receive funds immediately following its generation except in the case of wallet +/// failure. +/// - For externally-scoped transparent addresses, it is desirable to use a slightly larger gap +/// limit to account for addresses that were shared with counterparties never having been used. +/// However, we don't want to use the full 20-address gap limit space because it's possible that +/// in the future, changes to the light wallet protocol will obviate the need to query for UTXOs +/// in a fashion that links those addresses. In such a circumstance, the gap limit will be +/// adjusted upward and address rotation should then choose an address that is outside the +/// current gap limit; after that change, newly generated addresses will not be exposed as +/// linked in the view of the light wallet server. #[cfg(feature = "transparent-inputs")] impl Default for GapLimits { fn default() -> Self { @@ -353,6 +383,7 @@ impl WalletDb { #[cfg(feature = "transparent-inputs")] impl WalletDb { + /// Sets the gap limits to be used by the wallet in transparent address generation. pub fn with_gap_limits(mut self, gap_limits: GapLimits) -> Self { self.gap_limits = gap_limits; self @@ -1670,7 +1701,15 @@ impl, P: consensus::Parameters> WalletWrite f } fn truncate_to_height(&mut self, max_height: BlockHeight) -> Result { - self.transactionally(|wdb| wallet::truncate_to_height(wdb.conn.0, &wdb.params, max_height)) + self.transactionally(|wdb| { + wallet::truncate_to_height( + wdb.conn.0, + &wdb.params, + #[cfg(feature = "transparent-inputs")] + &wdb.gap_limits, + max_height, + ) + }) } #[cfg(feature = "transparent-inputs")] diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 95edb8be85..c721898346 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -51,7 +51,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { |e, _, expected_bad_index| { matches!( e, - crate::error::SqliteClientError::ReachedGapLimit(bad_index) + crate::error::SqliteClientError::ReachedGapLimit(bad_index, _) if bad_index == &expected_bad_index) }, ) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 74007d19d5..0bb72abd68 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -359,7 +359,7 @@ pub(crate) fn pool_code(pool_type: PoolType) -> i64 { /// This extends the [`zip32::Scope`] type to include the custom scope used to generate keys for /// ephemeral transparent addresses. #[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) enum KeyScope { +pub enum KeyScope { /// A key scope corresponding to a [`zip32::Scope`]. Zip32(zip32::Scope), /// An ephemeral transparent address, which is derived from an account's transparent @@ -2625,6 +2625,7 @@ pub(crate) fn set_transaction_status( pub(crate) fn truncate_to_height( conn: &rusqlite::Transaction, params: &P, + #[cfg(feature = "transparent-inputs")] gap_limits: &GapLimits, max_height: BlockHeight, ) -> Result { // Determine a checkpoint to which we can rewind, if any. @@ -2742,7 +2743,7 @@ pub(crate) fn truncate_to_height( conn: SqlTransaction(conn), params: params.clone(), #[cfg(feature = "transparent-inputs")] - gap_limits: GapLimits::default(), + gap_limits: *gap_limits, }; wdb.with_sapling_tree_mut(|tree| { tree.truncate_to_checkpoint(&truncation_height)?; diff --git a/zcash_client_sqlite/src/wallet/db.rs b/zcash_client_sqlite/src/wallet/db.rs index 24dea283e0..968350b699 100644 --- a/zcash_client_sqlite/src/wallet/db.rs +++ b/zcash_client_sqlite/src/wallet/db.rs @@ -86,19 +86,19 @@ pub(super) const INDEX_HD_ACCOUNT: &str = /// cannot use SQL integer operations on it for gap limit calculations, and thus need it as an /// integer as well. /// - `cached_transparent_receiver_address`: the transparent address derived from the same -/// viewing key and at the same diversifier index as `address`. This may be the same as -/// `address` in the case of an internal-scope transparent change address or a -/// ZIP 320 interstitial address, and it may be a receiver within `address` in the case of a -/// Unified Address with transparent receiver. It is cached directly in the table to make account lookups for +/// viewing key and at the same diversifier index as `address`. This may be the same as `address` +/// in the case of an internal-scope transparent change address or a ZIP 320 interstitial +/// address, and it may be a receiver within `address` in the case of a Unified Address with +/// transparent receiver. It is cached directly in the table to make account lookups for /// transparent outputs more efficient, enabling joins to [`TABLE_TRANSPARENT_RECEIVED_OUTPUTS`]. -/// - `exposed_at_height`: Our best knowledge as to when this address was first exposed to -/// the wider ecosystem. -/// - For user-generated addresses, this is the chain tip height at the time that the address -/// was generated by an explicit request by the user or reserved for use in a ZIP 320 -/// transaction. These heights are not recoverable from chain. -/// - In the case of an address with its first use discovered in a transaction obtained by -/// scanning the chain, this will be set to the mined height of that transaction. In recover -/// from seed cases, this is what user-generated addresses will be assigned. +/// - `exposed_at_height`: Our best knowledge as to when this address was first exposed to the +/// wider ecosystem. +/// - For user-generated addresses, this is the chain tip height at the time that the address was +/// generated by an explicit request by the user or reserved for use in a ZIP 320 transaction. +/// These heights are not recoverable from chain. +/// - In the case of an address with its first use discovered in a transaction obtained by scanning +/// the chain, this will be set to the mined height of that transaction. In recover from seed +/// cases, this is what user-generated addresses will be assigned. pub(super) const TABLE_ADDRESSES: &str = r#" CREATE TABLE "addresses" ( id INTEGER NOT NULL PRIMARY KEY, @@ -198,7 +198,7 @@ CREATE TABLE "transactions" ( /// - `nf`: the nullifier that will be exposed when the note is spent /// - `is_change`: a flag indicating whether the note was received in a transaction where /// the receiving account also spent notes. -/// - `memo`: the memo output associated with the note +/// - `memo`: the memo output associated with the note, if known /// - `commitment_tree_position`: the 0-based index of the note in the leaves of the note /// commitment tree. /// - `receipient_key_scope`: the ZIP 32 key scope of the key that decrypted this output, @@ -271,7 +271,7 @@ CREATE TABLE sapling_received_note_spends ( /// - `nf`: the nullifier that will be exposed when the note is spent /// - `is_change`: a flag indicating whether the note was received in a transaction where /// the receiving account also spent notes. -/// - `memo`: the memo output associated with the note +/// - `memo`: the memo output associated with the note, if known /// - `commitment_tree_position`: the 0-based index of the note in the leaves of the note /// commitment tree. /// - `receipient_key_scope`: the ZIP 32 key scope of the key that decrypted this output, @@ -357,7 +357,7 @@ CREATE TABLE orchard_received_note_spends ( /// transaction that spends it is not detected by the wallet. /// - `address_id`: a foreign key to the address that this note was sent to; non-null because /// we can only find transparent outputs for known addresses (and therefore we must record -/// both internal and external addresses in the `addresses` table.) +/// both internal and external addresses in the `addresses` table). pub(super) const TABLE_TRANSPARENT_RECEIVED_OUTPUTS: &str = r#" CREATE TABLE "transparent_received_outputs" ( id INTEGER PRIMARY KEY, diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 5ab3dc0ea8..fcd5c618d4 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -221,7 +221,7 @@ fn sqlite_client_error_to_wallet_migration_error(e: SqliteClientError) -> Wallet unreachable!("we don't call methods that require a known chain height") } #[cfg(feature = "transparent-inputs")] - SqliteClientError::ReachedGapLimit(_) => { + SqliteClientError::ReachedGapLimit(_, _) => { unreachable!("we don't do ephemeral address tracking") } SqliteClientError::AddressReuse(_, _) => { diff --git a/zcash_client_sqlite/src/wallet/init/migrations/fix_broken_commitment_trees.rs b/zcash_client_sqlite/src/wallet/init/migrations/fix_broken_commitment_trees.rs index 39e1e0285c..9d8f841177 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/fix_broken_commitment_trees.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/fix_broken_commitment_trees.rs @@ -12,6 +12,9 @@ use crate::wallet::{ init::{migrations::support_legacy_sqlite, WalletMigrationError}, }; +#[cfg(feature = "transparent-inputs")] +use crate::GapLimits; + pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x9fa43ce0_a387_45d1_be03_57a3edc76d01); const DEPENDENCIES: &[Uuid] = &[support_legacy_sqlite::MIGRATION_ID]; @@ -59,7 +62,13 @@ impl RusqliteMigration for Migration

{ .flatten(); if let Some(h) = max_block_height { - wallet::truncate_to_height(transaction, &self.params, h)?; + wallet::truncate_to_height( + transaction, + &self.params, + #[cfg(feature = "transparent-inputs")] + &GapLimits::default(), + h, + )?; } Ok(()) diff --git a/zcash_client_sqlite/src/wallet/init/migrations/transparent_gap_limit_handling.rs b/zcash_client_sqlite/src/wallet/init/migrations/transparent_gap_limit_handling.rs index 08a630dfb5..afd39c14ce 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/transparent_gap_limit_handling.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/transparent_gap_limit_handling.rs @@ -18,9 +18,12 @@ use crate::{ #[cfg(feature = "transparent-inputs")] use { - crate::wallet::{ - decode_diversifier_index_be, encode_diversifier_index_be, - transparent::generate_gap_addresses, GapLimits, + crate::{ + wallet::{ + decode_diversifier_index_be, encode_diversifier_index_be, + transparent::generate_gap_addresses, + }, + GapLimits, }, ::transparent::keys::{IncomingViewingKey as _, NonHardenedChildIndex}, zcash_keys::encoding::AddressCodec as _, diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 79111f9447..045c2675bb 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -300,6 +300,8 @@ pub(crate) fn reserve_next_n_addresses( ":account_id": account_id.0, ":key_scope": key_scope.encode(), ":gap_start": gap_start.index(), + // NOTE: this approach means that the address at index 2^31 - 1 will never be + // allocated. I think that's fine. ":gap_end": gap_start.saturating_add(gap_limit).index(), ":n": n }, @@ -329,6 +331,7 @@ pub(crate) fn reserve_next_n_addresses( if addresses_to_reserve.len() < n { return Err(SqliteClientError::ReachedGapLimit( gap_start.index() + gap_limit, + key_scope, )); } @@ -572,8 +575,7 @@ pub(crate) fn utxo_query_height( match (h_external, h_internal) { (Some(ext), Some(int)) => Ok(std::cmp::min(ext, int)), - (Some(ext), None) => Ok(ext), - (None, Some(int)) => Ok(int), + (Some(h), None) | (None, Some(h)) => Ok(h), (None, None) => account_birthday_internal(conn, account_ref), } } @@ -1088,12 +1090,6 @@ pub(crate) fn find_account_uuid_for_transparent_address( // Unlike the shielded pools, we only can receive transparent outputs on addresses for which we // have an `addresses` table entry, so we can just query for that here. - let (address_id, account_id, key_scope_code) = conn.query_row( - "SELECT id, account_id, key_scope - FROM addresses - WHERE cached_transparent_receiver_address = :transparent_address", - named_params! {":transparent_address": addr_str}, - |row| { - Ok(( - row.get("id").map(AddressRef)?, - row.get("account_id").map(AccountRef)?, - row.get("key_scope")?, - )) - }, - )?; + let (address_id, account_id, key_scope_code) = conn + .query_row( + "SELECT id, account_id, key_scope + FROM addresses + WHERE cached_transparent_receiver_address = :transparent_address", + named_params! {":transparent_address": addr_str}, + |row| { + Ok(( + row.get("id").map(AddressRef)?, + row.get("account_id").map(AccountRef)?, + row.get("key_scope")?, + )) + }, + ) + .optional()? + .ok_or(SqliteClientError::AddressNotRecognized(*address))?; let key_scope = KeyScope::decode(key_scope_code)?; @@ -1421,7 +1420,7 @@ mod tests { db.gap_limits.ephemeral(), db.gap_limits.ephemeral() as usize ), - Err(SqliteClientError::ReachedGapLimit(_)) + Err(SqliteClientError::ReachedGapLimit(..)) ); }; diff --git a/zcash_transparent/CHANGELOG.md b/zcash_transparent/CHANGELOG.md index 511e2106ce..efacb3096d 100644 --- a/zcash_transparent/CHANGELOG.md +++ b/zcash_transparent/CHANGELOG.md @@ -18,6 +18,7 @@ and this library adheres to Rust's notion of - `zcash_transparent::keys::NonHardenedChildIndex::MAX` - `impl From for zip32::DiversifierIndex` - `impl TryFrom for NonHardenedChildIndex` +- `impl {PartialOrd, Ord} for NonHardenedChildIndex` ### Changed - MSRV is now 1.81.0.