From 8cb16d878ef956c5a91ba46f78e1e126d11132db Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 25 Oct 2022 10:54:12 -0600 Subject: [PATCH] Require a source transparent address to shield transparent funds. Previously, `shield_transparent_funds` was only shielding funds associated with the legacy default transparent address. This meant that transparent funds sent to unified addresses could not reliably be shielded, as a unified address will frequently be constructed using a diversifier index greater than zero. This modifies the `get_transparent_receivers` method to return address metadata containing the account ID and diversifier index used to derive each address along with the receiver. --- zcash_client_backend/CHANGELOG.md | 15 ++++-- zcash_client_backend/src/address.rs | 29 ++++++++++- zcash_client_backend/src/data_api.rs | 15 +++--- zcash_client_backend/src/data_api/error.rs | 17 +++++++ zcash_client_backend/src/data_api/wallet.rs | 21 +++++--- zcash_client_sqlite/src/error.rs | 9 +++- zcash_client_sqlite/src/lib.rs | 12 ++--- zcash_client_sqlite/src/wallet.rs | 49 +++++++++++++------ .../init/migrations/add_utxo_account.rs | 2 +- zcash_primitives/CHANGELOG.md | 4 ++ zcash_primitives/src/zip32.rs | 33 +++++++++++++ 11 files changed, 162 insertions(+), 44 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 1e6372c69..a77e6527f 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -25,6 +25,7 @@ and this library adheres to Rust's notion of in any release. - `zcash_client_backend::address`: - `RecipientAddress::Unified` + - `AddressMetadata` - `zcash_client_backend::data_api`: - `PoolType` - `Recipient` @@ -102,9 +103,17 @@ and this library adheres to Rust's notion of `store_decrypted_tx`. - `data_api::ReceivedTransaction` has been renamed to `DecryptedTransaction`, and its `outputs` field has been renamed to `sapling_outputs`. -- An `Error::MemoForbidden` error has been added to the - `data_api::error::Error` enum to report the condition where a memo was - specified to be sent to a transparent recipient. +- `data_api::error::Error` has the following additional cases: + - `Error::MemoForbidden` to report the condition where a memo was + specified to be sent to a transparent recipient. + - `Error::TransparentInputsNotSupported` to represent the condition + where a transparent spend has been requested of a wallet compiled without + the `transparent-inputs` feature. + - `Error::AddressNotRecognized` to indicate that a transparent address from + which funds are being requested to be spent does not appear to be associated + with this wallet. + - `Error::ChildIndexOutOfRange` to indicate that a diversifier index for an + address is out of range for valid transparent child indices. - `zcash_client_backend::decrypt`: - `decrypt_transaction` now takes a `HashMap<_, UnifiedFullViewingKey>` instead of `HashMap<_, ExtendedFullViewingKey>`. diff --git a/zcash_client_backend/src/address.rs b/zcash_client_backend/src/address.rs index 235e52991..ac5f1e3bb 100644 --- a/zcash_client_backend/src/address.rs +++ b/zcash_client_backend/src/address.rs @@ -6,7 +6,34 @@ use zcash_address::{ unified::{self, Container, Encoding}, ConversionError, Network, ToAddress, TryFromRawAddress, ZcashAddress, }; -use zcash_primitives::{consensus, legacy::TransparentAddress, sapling::PaymentAddress}; +use zcash_primitives::{ + consensus, + legacy::TransparentAddress, + sapling::PaymentAddress, + zip32::{AccountId, DiversifierIndex}, +}; + +pub struct AddressMetadata { + account: AccountId, + diversifier_index: DiversifierIndex, +} + +impl AddressMetadata { + pub fn new(account: AccountId, diversifier_index: DiversifierIndex) -> Self { + Self { + account, + diversifier_index, + } + } + + pub fn account(&self) -> AccountId { + self.account + } + + pub fn diversifier_index(&self) -> &DiversifierIndex { + &self.diversifier_index + } +} /// A Unified Address. #[derive(Clone, Debug, PartialEq)] diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 94b63acfd..c788b3495 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1,7 +1,7 @@ //! Interfaces for wallet data persistence & low-level wallet utilities. use std::cmp; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fmt::Debug; use secrecy::SecretVec; @@ -17,7 +17,7 @@ use zcash_primitives::{ }; use crate::{ - address::UnifiedAddress, + address::{AddressMetadata, UnifiedAddress}, decrypt::DecryptedOutput, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, @@ -207,7 +207,7 @@ pub trait WalletRead { fn get_transparent_receivers( &self, account: AccountId, - ) -> Result, Self::Error>; + ) -> Result, Self::Error>; /// Returns a list of unspent transparent UTXOs that appear in the chain at heights up to and /// including `max_height`. @@ -400,9 +400,6 @@ pub mod testing { use secrecy::{ExposeSecret, SecretVec}; use std::collections::HashMap; - #[cfg(feature = "transparent-inputs")] - use std::collections::HashSet; - use zcash_primitives::{ block::BlockHash, consensus::{BlockHeight, Network}, @@ -415,7 +412,7 @@ pub mod testing { }; use crate::{ - address::UnifiedAddress, + address::{AddressMetadata, UnifiedAddress}, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, wallet::{SpendableNote, WalletTransparentOutput}, @@ -555,8 +552,8 @@ pub mod testing { fn get_transparent_receivers( &self, _account: AccountId, - ) -> Result, Self::Error> { - Ok(HashSet::new()) + ) -> Result, Self::Error> { + Ok(HashMap::new()) } fn get_unspent_transparent_outputs( diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index 1c6cbf7a1..55913fb3e 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -10,6 +10,9 @@ use zcash_primitives::{ zip32::AccountId, }; +#[cfg(feature = "transparent-inputs")] +use zcash_primitives::{legacy::TransparentAddress, zip32::DiversifierIndex}; + #[derive(Debug)] pub enum ChainInvalid { /// The hash of the parent block given by a proposed new chain tip does @@ -83,6 +86,12 @@ pub enum Error { /// support #[cfg(not(feature = "transparent-inputs"))] TransparentInputsNotSupported, + + #[cfg(feature = "transparent-inputs")] + AddressNotRecognized(TransparentAddress), + + #[cfg(feature = "transparent-inputs")] + ChildIndexOutOfRange(DiversifierIndex), } impl ChainInvalid { @@ -143,6 +152,14 @@ impl fmt::Display for Error { Error::TransparentInputsNotSupported => { write!(f, "This wallet does not support spending or manipulating transparent UTXOs.") } + #[cfg(feature = "transparent-inputs")] + Error::AddressNotRecognized(_) => { + write!(f, "The specified transparent address was not recognized as belonging to the wallet.") + } + #[cfg(feature = "transparent-inputs")] + Error::ChildIndexOutOfRange(i) => { + write!(f, "The diversifier index {:?} is out of range for transparent addresses.", i) + } } } } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index e54c8730e..05bb22d82 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -23,7 +23,7 @@ use crate::{ }; #[cfg(feature = "transparent-inputs")] -use zcash_primitives::{legacy::keys::IncomingViewingKey, sapling::keys::OutgoingViewingKey}; +use zcash_primitives::{legacy::TransparentAddress, sapling::keys::OutgoingViewingKey}; /// Scans a [`Transaction`] for any information that can be decrypted by the accounts in /// the wallet, and saves it to the wallet. @@ -440,6 +440,7 @@ pub fn shield_transparent_funds( params: &P, prover: impl TxProver, usk: &UnifiedSpendingKey, + from_addr: &TransparentAddress, memo: &MemoBytes, min_confirmations: u32, ) -> Result @@ -465,14 +466,8 @@ where let account_pubkey = usk.transparent().to_account_pubkey(); let ovk = OutgoingViewingKey(account_pubkey.internal_ovk().as_bytes()); - // derive the t-address for the extpubkey at the minimum valid child index - let (taddr, child_index) = account_pubkey - .derive_external_ivk() - .unwrap() - .default_address(); - // get UTXOs from DB - let utxos = wallet_db.get_unspent_transparent_outputs(&taddr, latest_anchor)?; + let utxos = wallet_db.get_unspent_transparent_outputs(from_addr, latest_anchor)?; let total_amount = utxos .iter() .map(|utxo| utxo.txout().value) @@ -488,10 +483,20 @@ where let mut builder = Builder::new_with_fee(params.clone(), latest_scanned_height, fee); + let diversifier_index = *wallet_db + .get_transparent_receivers(account)? + .get(from_addr) + .ok_or(Error::AddressNotRecognized(*from_addr))? + .diversifier_index(); + + let child_index = u32::try_from(diversifier_index) + .map_err(|_| Error::ChildIndexOutOfRange(diversifier_index))?; + let secret_key = usk .transparent() .derive_external_secret_key(child_index) .unwrap(); + for utxo in &utxos { builder .add_transparent_input(secret_key, utxo.outpoint().clone(), utxo.txout().clone()) diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index f3794ecd8..839b32973 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -4,7 +4,6 @@ use std::error; use std::fmt; use zcash_client_backend::{ - address::RecipientAddress, data_api, encoding::{Bech32DecodeError, TransparentCodecError}, }; @@ -12,6 +11,9 @@ use zcash_primitives::{consensus::BlockHeight, zip32::AccountId}; use crate::{NoteId, PRUNING_HEIGHT}; +#[cfg(feature = "transparent-inputs")] +use zcash_primitives::legacy::TransparentAddress; + #[cfg(feature = "unstable")] use zcash_primitives::transaction::TxId; @@ -44,6 +46,7 @@ pub enum SqliteClientError { /// Base58 decoding error Base58(bs58::decode::Error), + /// #[cfg(feature = "transparent-inputs")] HdwalletError(hdwallet::error::Error), @@ -84,7 +87,8 @@ pub enum SqliteClientError { /// The address associated with a record being inserted was not recognized as /// belonging to the wallet - AddressNotRecognized(RecipientAddress), + #[cfg(feature = "transparent-inputs")] + AddressNotRecognized(TransparentAddress), } impl error::Error for SqliteClientError { @@ -127,6 +131,7 @@ impl fmt::Display for SqliteClientError { SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id), SqliteClientError::AccountIdDiscontinuity => write!(f, "Wallet account identifiers must be sequential."), SqliteClientError::AccountIdOutOfRange => write!(f, "Wallet account identifiers must be less than 0x7FFFFFFF."), + #[cfg(feature = "transparent-inputs")] SqliteClientError::AddressNotRecognized(_) => write!(f, "The address associated with a received txo is not identifiable as belonging to the wallet."), } } diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 7224902ad..96da41534 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -34,7 +34,7 @@ use rusqlite::Connection; use secrecy::{ExposeSecret, SecretVec}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fmt; use std::path::Path; @@ -50,7 +50,7 @@ use zcash_primitives::{ }; use zcash_client_backend::{ - address::UnifiedAddress, + address::{AddressMetadata, UnifiedAddress}, data_api::{ BlockSource, DecryptedTransaction, PoolType, PrunedBlock, Recipient, SentTransaction, WalletRead, WalletWrite, @@ -248,7 +248,7 @@ impl WalletRead for WalletDb

{ fn get_transparent_receivers( &self, _account: AccountId, - ) -> Result, Self::Error> { + ) -> Result, Self::Error> { #[cfg(feature = "transparent-inputs")] return wallet::get_transparent_receivers(&self.params, &self.conn, _account); @@ -379,7 +379,7 @@ impl<'a, P: consensus::Parameters> WalletRead for DataConnStmtCache<'a, P> { fn get_transparent_receivers( &self, account: AccountId, - ) -> Result, Self::Error> { + ) -> Result, Self::Error> { self.wallet_db.get_transparent_receivers(account) } @@ -1187,10 +1187,10 @@ mod tests { let receivers = db_data.get_transparent_receivers(0.into()).unwrap(); // The receiver for the default UA should be in the set. - assert!(receivers.contains(ufvk.default_address().0.transparent().unwrap())); + assert!(receivers.contains_key(ufvk.default_address().0.transparent().unwrap())); // The default t-addr should be in the set. - assert!(receivers.contains(&taddr)); + assert!(receivers.contains_key(&taddr)); } #[cfg(feature = "unstable")] diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index a108b4da6..39ecb0fe2 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -42,8 +42,9 @@ use crate::{ use { crate::UtxoId, rusqlite::{params, Connection}, - std::collections::HashSet, - zcash_client_backend::{encoding::AddressCodec, wallet::WalletTransparentOutput}, + zcash_client_backend::{ + address::AddressMetadata, encoding::AddressCodec, wallet::WalletTransparentOutput, + }, zcash_primitives::{ legacy::{keys::IncomingViewingKey, Script, TransparentAddress}, transaction::components::{OutPoint, TxOut}, @@ -236,7 +237,7 @@ pub(crate) fn get_current_address( addr.map(|(addr_str, di_vec)| { let mut di_be: [u8; 11] = di_vec.try_into().map_err(|_| { SqliteClientError::CorruptedData( - "Diverisifier index is not an 11-byte value".to_owned(), + "Diversifier index is not an 11-byte value".to_owned(), ) })?; di_be.reverse(); @@ -262,15 +263,24 @@ pub(crate) fn get_transparent_receivers( params: &P, conn: &Connection, account: AccountId, -) -> Result, SqliteClientError> { - let mut ret = HashSet::new(); +) -> Result, SqliteClientError> { + let mut ret = HashMap::new(); // Get all UAs derived - let mut ua_query = conn.prepare("SELECT address FROM addresses WHERE account = :account")?; + let mut ua_query = + conn.prepare("SELECT address, diversifier_index_be FROM addresses WHERE account = :account")?; let mut rows = ua_query.query(named_params![":account": &u32::from(account)])?; while let Some(row) = rows.next()? { let ua_str: String = row.get(0)?; + let di_vec: Vec = row.get(1)?; + let mut di_be: [u8; 11] = di_vec.try_into().map_err(|_| { + SqliteClientError::CorruptedData( + "Diverisifier index is not an 11-byte value".to_owned(), + ) + })?; + di_be.reverse(); + let ua = RecipientAddress::decode(params, &ua_str) .ok_or_else(|| { SqliteClientError::CorruptedData("Not a valid Zcash recipient address".to_owned()) @@ -282,13 +292,18 @@ pub(crate) fn get_transparent_receivers( ua_str, ))), })?; + if let Some(taddr) = ua.transparent() { - ret.insert(*taddr); + ret.insert( + *taddr, + AddressMetadata::new(account, DiversifierIndex(di_be)), + ); } } - if let Some(taddr) = get_legacy_transparent_address(params, conn, account)? { - ret.insert(taddr); + if let Some((taddr, diversifier_index)) = get_legacy_transparent_address(params, conn, account)? + { + ret.insert(taddr, AddressMetadata::new(account, diversifier_index)); } Ok(ret) @@ -299,7 +314,7 @@ pub(crate) fn get_legacy_transparent_address( params: &P, conn: &Connection, account: AccountId, -) -> Result, SqliteClientError> { +) -> Result, SqliteClientError> { // Get the UFVK for the account. let ufvk_str: Option = conn .query_row( @@ -317,7 +332,10 @@ pub(crate) fn get_legacy_transparent_address( ufvk.transparent() .map(|tfvk| { tfvk.derive_external_ivk() - .map(|tivk| tivk.default_address().0) + .map(|tivk| { + let (taddr, child_index) = tivk.default_address(); + (taddr, DiversifierIndex::from(child_index)) + }) .map_err(SqliteClientError::HdwalletError) }) .transpose() @@ -1094,8 +1112,11 @@ pub(crate) fn put_received_transparent_utxo<'a, P: consensus::Parameters>( // so then insert/update it directly. let account = AccountId::from(0u32); get_legacy_transparent_address(&stmts.wallet_db.params, &stmts.wallet_db.conn, account) - .and_then(|taddr| { - if taddr.as_ref() == Some(output.recipient_address()) { + .and_then(|legacy_taddr| { + if legacy_taddr + .iter() + .any(|(taddr, _)| taddr == output.recipient_address()) + { stmts .stmt_update_legacy_transparent_utxo(output, account) .transpose() @@ -1104,7 +1125,7 @@ pub(crate) fn put_received_transparent_utxo<'a, P: consensus::Parameters>( }) } else { Err(SqliteClientError::AddressNotRecognized( - RecipientAddress::Transparent(*output.recipient_address()), + *output.recipient_address(), )) } }) diff --git a/zcash_client_sqlite/src/wallet/init/migrations/add_utxo_account.rs b/zcash_client_sqlite/src/wallet/init/migrations/add_utxo_account.rs index 755481d6c..f658ae103 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/add_utxo_account.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/add_utxo_account.rs @@ -79,7 +79,7 @@ impl RusqliteMigration for Migration

{ )), })?; - for taddr in taddrs { + for (taddr, _) in taddrs { stmt_update_utxo_account.execute(named_params![ ":account": &account, ":address": &taddr.encode(&self._params), diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 62277cbd9..e5fbd6bb9 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -7,6 +7,10 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Added +- Added in `zcash_primitives::zip32` + - An implementation of `TryFrom` for `u32` + ## [0.8.1] - 2022-10-19 ### Added - `zcash_primitives::legacy`: diff --git a/zcash_primitives/src/zip32.rs b/zcash_primitives/src/zip32.rs index 105f005ab..c763177e2 100644 --- a/zcash_primitives/src/zip32.rs +++ b/zcash_primitives/src/zip32.rs @@ -105,6 +105,16 @@ impl From for DiversifierIndex { } } +impl TryFrom for u32 { + type Error = std::num::TryFromIntError; + + fn try_from(di: DiversifierIndex) -> Result { + let mut u128_bytes = [0u8; 16]; + u128_bytes[0..11].copy_from_slice(&di.0[..]); + u128::from_le_bytes(u128_bytes).try_into() + } +} + impl DiversifierIndex { pub fn new() -> Self { DiversifierIndex([0; 11]) @@ -144,3 +154,26 @@ pub enum Scope { } memuse::impl_no_dynamic_usage!(Scope); + +#[cfg(test)] +mod tests { + use super::DiversifierIndex; + + #[test] + fn diversifier_index_to_u32() { + let two = DiversifierIndex([ + 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ]); + assert_eq!(u32::try_from(two), Ok(2)); + + let max_u32 = DiversifierIndex([ + 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ]); + assert_eq!(u32::try_from(max_u32), Ok(u32::MAX)); + + let too_big = DiversifierIndex([ + 0xff, 0xff, 0xff, 0xff, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ]); + assert!(matches!(u32::try_from(too_big), Err(_))); + } +}