diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 2b4de630f..c5cc60cd1 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -53,14 +53,14 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail have changed as a consequence of this extraction; please see the `zip321` CHANGELOG for details. - `zcash_client_backend::data_api`: - - `WalletRead` has new `get_known_ephemeral_addresses` and - `get_transparent_address_metadata` methods. - - `WalletWrite` has a new `reserve_next_n_ephemeral_addresses` method. - - `error::Error` has a new `Address` variant. + - `WalletRead` has new `get_known_ephemeral_addresses`, + `find_account_for_ephemeral_address`, and `get_transparent_address_metadata` + methods when the "transparent-inputs" feature is enabled. + - `WalletWrite` has a new `reserve_next_n_ephemeral_addresses` method when + the "transparent-inputs" feature is enabled. + - `error::Error` has new `Address` and (when the "transparent-inputs" feature + is enabled) `PaysEphemeralTransparentAddress` variants. - `wallet::input_selection::InputSelectorError` has a new `Address` variant. -- `zcash_client_backend::proto::proposal::Proposal::{from_standard_proposal, - try_into_standard_proposal}` each no longer require a `consensus::Parameters` - argument. - `zcash_client_backend::data_api::fees` - When the "transparent-inputs" feature is enabled, `ChangeValue` can also represent an ephemeral transparent output in a proposal. Accordingly, the @@ -78,8 +78,10 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `zcash_client_backend::proposal::ProposalError` has new variants `SpendsChange`, `EphemeralOutputLeftUnspent`, and `PaysTexFromShielded`. (the last two are conditional on the "transparent-inputs" feature). -- `zcash_client_backend::proto::ProposalDecodingError` has a new variant - `InvalidEphemeralRecipient`. +- `zcash_client_backend::proto`: + - `ProposalDecodingError` has a new variant `InvalidEphemeralRecipient`. + - `proposal::Proposal::{from_standard_proposal, try_into_standard_proposal}` + each no longer require a `consensus::Parameters` argument. - `zcash_client_backend::wallet::Recipient` variants have changed. Instead of wrapping protocol-address types, the `External` and `InternalAccount` variants now wrap a `zcash_address::ZcashAddress`. This simplifies the process of diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 19ee52bad..3140ad648 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -998,6 +998,36 @@ pub trait WalletRead { ) -> Result, Self::Error> { Ok(HashMap::new()) } + + /// If a given transparent address has been reserved, i.e. would be included in + /// the map returned by `get_known_ephemeral_addresses(account_id, false)` for any + /// of the wallet's accounts, then return `Ok(Some(account_id))`. Otherwise return + /// `Ok(None)`. + /// + /// This is equivalent to (but may be implemented more efficiently than): + /// ```compile_fail + /// for account_id in self.get_account_ids()? { + /// if self.get_known_ephemeral_addresses(account_id, false)?.contains_key(address)? { + /// return Ok(Some(account_id)); + /// } + /// } + /// Ok(None) + /// ``` + #[cfg(feature = "transparent-inputs")] + fn find_account_for_ephemeral_address( + &self, + address: &TransparentAddress, + ) -> Result, Self::Error> { + for account_id in self.get_account_ids()? { + if self + .get_known_ephemeral_addresses(account_id, false)? + .contains_key(address) + { + return Ok(Some(account_id)); + } + } + Ok(None) + } } /// The relevance of a seed to a given wallet. @@ -1998,6 +2028,14 @@ pub mod testing { ) -> Result, Self::Error> { Ok(HashMap::new()) } + + #[cfg(feature = "transparent-inputs")] + fn find_account_for_ephemeral_address( + &self, + _address: &TransparentAddress, + ) -> Result, Self::Error> { + Ok(None) + } } impl WalletWrite for MockWalletDb { diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index 371d57706..acf451a3e 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -92,6 +92,11 @@ pub enum Error { /// belonging to the wallet. #[cfg(feature = "transparent-inputs")] AddressNotRecognized(TransparentAddress), + + /// The wallet tried to pay to an ephemeral transparent address as a normal + /// output. + #[cfg(feature = "transparent-inputs")] + PaysEphemeralTransparentAddress(String), } impl fmt::Display for Error @@ -161,6 +166,10 @@ where Error::AddressNotRecognized(_) => { write!(f, "The specified transparent address was not recognized as belonging to the wallet.") } + #[cfg(feature = "transparent-inputs")] + Error::PaysEphemeralTransparentAddress(addr) => { + write!(f, "The wallet tried to pay to an ephemeral transparent address as a normal output: {}", addr) + } } } } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 2712608a9..f2332405c 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -77,6 +77,7 @@ use { core::convert::Infallible, input_selection::ShieldingSelector, std::collections::HashMap, + zcash_keys::encoding::AddressCodec, zcash_primitives::transaction::components::TxOut, }; @@ -1017,11 +1018,19 @@ where transparent_output_meta: &mut Vec<_>, to: TransparentAddress| -> Result<(), ErrorT> { + // Always reject sending to one of our known ephemeral addresses. + #[cfg(feature = "transparent-inputs")] + if wallet_db + .find_account_for_ephemeral_address(&to) + .map_err(Error::DataSource)? + .is_some() + { + return Err(Error::PaysEphemeralTransparentAddress(to.encode(params))); + } if payment.memo().is_some() { return Err(Error::MemoForbidden); - } else { - builder.add_transparent_output(&to, payment.amount())?; } + builder.add_transparent_output(&to, payment.amount())?; transparent_output_meta.push(( Recipient::External(recipient_address.clone(), PoolType::TRANSPARENT), to, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index ad0d03597..8b73b7781 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -88,6 +88,7 @@ use { #[cfg(feature = "transparent-inputs")] use { zcash_client_backend::wallet::TransparentAddressMetadata, + zcash_keys::encoding::AddressCodec, zcash_primitives::{ legacy::TransparentAddress, transaction::components::{OutPoint, TxOut}, @@ -561,6 +562,17 @@ impl, P: consensus::Parameters> WalletRead for W for_detection, ) } + + #[cfg(feature = "transparent-inputs")] + fn find_account_for_ephemeral_address( + &self, + address: &TransparentAddress, + ) -> Result, Self::Error> { + wallet::transparent::ephemeral::find_account_for_ephemeral_address_str( + self.conn.borrow(), + &address.encode(&self.params), + ) + } } impl WalletWrite for WalletDb { @@ -1470,14 +1482,6 @@ impl WalletWrite for WalletDb tx_ref, )?; } - #[cfg(feature = "transparent-inputs")] - Recipient::External(zcash_address, PoolType::Transparent) => { - // Always reject sending to one of our ephemeral addresses. - wallet::transparent::ephemeral::check_address_is_not_ephemeral( - wdb, - &zcash_address.encode(), - )?; - } _ => {} } } diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index bf21663b9..ac4683ff5 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -443,7 +443,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { }; // Each transfer should use a different ephemeral address. - let (ephemeral0, txid0) = run_test(&mut st, 0); + let (ephemeral0, _) = run_test(&mut st, 0); let (ephemeral1, _) = run_test(&mut st, 1); assert!(ephemeral0 != ephemeral1); @@ -455,7 +455,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { Address::Transparent(TransparentAddress::PublicKeyHash(_)) ); - // Attempting to use the same address again should cause an error. + // Attempting to pay to an ephemeral address should cause an error. let proposal = st .propose_standard_transfer::( account.account_id(), @@ -476,7 +476,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { ); assert_matches!( &create_proposed_result, - Err(Error::DataSource(SqliteClientError::EphemeralAddressReuse(addr, Some(txid)))) if addr == &ephemeral0 && txid == &txid0); + Err(Error::PaysEphemeralTransparentAddress(address_str)) if address_str == &ephemeral0); } #[cfg(feature = "transparent-inputs")] diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index a1f9b15cf..0559bb086 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -68,7 +68,6 @@ use incrementalmerkletree::{Marking, Retention}; use rusqlite::{self, named_params, OptionalExtension}; use secrecy::{ExposeSecret, SecretVec}; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; -use zcash_keys::encoding::encode_transparent_address_p; use zip32::fingerprint::SeedFingerprint; use std::collections::{HashMap, HashSet}; @@ -2148,7 +2147,7 @@ fn recipient_params( ephemeral_address, .. } => ( - Some(encode_transparent_address_p(params, ephemeral_address)), + Some(ephemeral_address.encode(params)), Some(*receiving_account), PoolType::TRANSPARENT, ), diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index c52d4ec98..d4476ff87 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -3,7 +3,6 @@ use std::collections::{HashMap, HashSet}; use rusqlite::OptionalExtension; use rusqlite::{named_params, Connection, Row}; -use zcash_keys::encoding::encode_transparent_address_p; use zip32::{DiversifierIndex, Scope}; use zcash_address::unified::{Encoding, Ivk, Uivk}; @@ -468,7 +467,7 @@ pub(crate) fn get_transparent_address_metadata( account_id: AccountId, address: &TransparentAddress, ) -> Result, SqliteClientError> { - let address_str = encode_transparent_address_p(params, address); + let address_str = address.encode(params); if let Some(di_vec) = conn .query_row( @@ -494,14 +493,8 @@ pub(crate) fn get_transparent_address_metadata( } // Search ephemeral addresses that have already been reserved. - if let Some(raw_index) = conn - .query_row( - "SELECT address_index FROM ephemeral_addresses - WHERE account_id = :account_id AND address = :address", - named_params![":account_id": account_id.0, ":address": &address_str], - |row| row.get::<_, u32>(0), - ) - .optional()? + if let Some(raw_index) = + ephemeral::find_index_for_ephemeral_address_str(conn, account_id, &address_str)? { let address_index = NonHardenedChildIndex::from_index(raw_index).unwrap(); return Ok(Some(ephemeral::metadata(address_index))); @@ -542,13 +535,7 @@ pub(crate) fn find_account_for_transparent_output( } // Search ephemeral addresses that have already been reserved. - if let Some(account_id) = conn - .query_row( - "SELECT account_id FROM ephemeral_addresses WHERE address = :address", - named_params![":address": &address_str], - |row| Ok(AccountId(row.get(0)?)), - ) - .optional()? + if let Some(account_id) = ephemeral::find_account_for_ephemeral_address_str(conn, &address_str)? { return Ok(Some(account_id)); } diff --git a/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs b/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs index 83fa19ee4..6d5af2335 100644 --- a/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs +++ b/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs @@ -6,10 +6,7 @@ use std::ops::RangeInclusive; use rusqlite::{named_params, OptionalExtension}; use zcash_client_backend::{data_api::Account, wallet::TransparentAddressMetadata}; -use zcash_keys::{ - encoding::{encode_transparent_address_p, AddressCodec}, - keys::AddressGenerationError, -}; +use zcash_keys::{encoding::AddressCodec, keys::AddressGenerationError}; use zcash_primitives::{ legacy::{ keys::{EphemeralIvk, NonHardenedChildIndex, TransparentKeyScope}, @@ -158,6 +155,37 @@ pub(crate) fn get_known_ephemeral_addresses( Ok(result) } +/// If this is an ephemeral address in any account, return its account id. +pub(crate) fn find_account_for_ephemeral_address_str( + conn: &rusqlite::Connection, + address_str: &str, +) -> Result, SqliteClientError> { + // Search ephemeral addresses that have already been reserved. + Ok(conn + .query_row( + "SELECT account_id FROM ephemeral_addresses WHERE address = :address", + named_params![":address": &address_str], + |row| Ok(AccountId(row.get(0)?)), + ) + .optional()?) +} + +#[cfg(feature = "transparent-inputs")] +pub(crate) fn find_index_for_ephemeral_address_str( + conn: &rusqlite::Connection, + account_id: AccountId, + address_str: &str, +) -> Result, SqliteClientError> { + Ok(conn + .query_row( + "SELECT address_index FROM ephemeral_addresses + WHERE account_id = :account_id AND address = :address", + named_params![":account_id": account_id.0, ":address": &address_str], + |row| row.get::<_, u32>(0), + ) + .optional()?) +} + /// Returns a vector with the next `n` previously unreserved ephemeral addresses for /// the given account. /// @@ -215,29 +243,18 @@ pub(crate) fn reserve_next_n_ephemeral_addresses( stmt_insert_ephemeral_address.execute(named_params![ ":account_id": account_id.0, ":address_index": raw_index, - ":address": encode_transparent_address_p(&wdb.params, &address) + ":address": address.encode(&wdb.params), ])?; Ok((address, metadata(address_index))) }) .collect() } -/// Returns a `SqliteClientError::EphemeralAddressReuse` error if `address` is -/// an ephemeral transparent address. -pub(crate) fn check_address_is_not_ephemeral( - wdb: &mut WalletDb, P>, - address_str: &str, -) -> Result<(), SqliteClientError> { - ephemeral_address_check_internal(wdb, address_str, true) -} - /// Returns a `SqliteClientError::EphemeralAddressReuse` error if the address was -/// already used. If `reject_all_ephemeral` is set, return an error if the address -/// is ephemeral at all, regardless of reuse. -fn ephemeral_address_check_internal( +/// already used. +fn ephemeral_address_reuse_check( wdb: &mut WalletDb, P>, address_str: &str, - reject_all_ephemeral: bool, ) -> Result<(), SqliteClientError> { // It is intentional that we don't require `t.mined_height` to be non-null. // That is, we conservatively treat an ephemeral address as potentially @@ -263,25 +280,21 @@ fn ephemeral_address_check_internal( named_params![":address": address_str], |row| row.get::<_, Option>>(0), ) - .optional()?; + .optional()? + .flatten(); - match res { - Some(Some(txid_bytes)) => { - let txid = TxId::from_bytes( - txid_bytes - .try_into() - .map_err(|_| SqliteClientError::CorruptedData("invalid txid".to_owned()))?, - ); - Err(SqliteClientError::EphemeralAddressReuse( - address_str.to_owned(), - Some(txid), - )) - } - Some(None) if reject_all_ephemeral => Err(SqliteClientError::EphemeralAddressReuse( + if let Some(txid_bytes) = res { + let txid = TxId::from_bytes( + txid_bytes + .try_into() + .map_err(|_| SqliteClientError::CorruptedData("invalid txid".to_owned()))?, + ); + Err(SqliteClientError::EphemeralAddressReuse( address_str.to_owned(), - None, - )), - _ => Ok(()), + Some(txid), + )) + } else { + Ok(()) } } @@ -296,8 +309,8 @@ pub(crate) fn mark_ephemeral_address_as_used( ephemeral_address: &TransparentAddress, tx_ref: i64, ) -> Result<(), SqliteClientError> { - let address_str = encode_transparent_address_p(&wdb.params, ephemeral_address); - ephemeral_address_check_internal(wdb, &address_str, false)?; + let address_str = ephemeral_address.encode(&wdb.params); + ephemeral_address_reuse_check(wdb, &address_str)?; wdb.conn.0.execute( "UPDATE ephemeral_addresses SET used_in_tx = :used_in_tx WHERE address = :address", @@ -316,7 +329,7 @@ pub(crate) fn mark_ephemeral_address_as_mined( address: &TransparentAddress, tx_ref: i64, ) -> Result<(), SqliteClientError> { - let address_str = encode_transparent_address_p(&wdb.params, address); + let address_str = address.encode(&wdb.params); // Figure out which transaction was mined earlier: `tx_ref`, or any existing // tx referenced by `mined_in_tx` for the given address. Prefer the existing