From 3c837381db1418dc212fcbb1a53edc55f04e47ed Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 Oct 2022 09:07:11 -0600 Subject: [PATCH] Disallow invalid pool/address combinations with `Recipient`. --- zcash_client_backend/CHANGELOG.md | 2 + zcash_client_backend/src/data_api.rs | 31 +++++---- zcash_client_backend/src/data_api/wallet.rs | 18 ++--- zcash_client_backend/src/encoding.rs | 76 ++++++++++++++++----- zcash_client_sqlite/src/lib.rs | 10 ++- zcash_client_sqlite/src/prepared.rs | 45 +++++++++--- zcash_client_sqlite/src/wallet.rs | 13 +--- zcash_client_sqlite/src/wallet/init.rs | 7 +- 8 files changed, 131 insertions(+), 71 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index ce03b8e3b..8af79ca45 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -31,6 +31,8 @@ and this library adheres to Rust's notion of - `zcash_client_backend::address`: - `RecipientAddress::Unified` - `zcash_client_backend::data_api`: + - `Recipient` + - `SentTransactionOutput` - `WalletRead::get_unified_full_viewing_keys` - `WalletRead::get_current_address` - `WalletRead::get_all_nullifiers` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 0450061a4..8f2040bc0 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -11,15 +11,16 @@ use secrecy::SecretVec; use zcash_primitives::{ block::BlockHash, consensus::BlockHeight, + legacy::TransparentAddress, memo::{Memo, MemoBytes}, merkle_tree::{CommitmentTree, IncrementalWitness}, - sapling::{Node, Nullifier}, + sapling::{Node, Nullifier, PaymentAddress}, transaction::{components::Amount, Transaction, TxId}, zip32::{AccountId, ExtendedFullViewingKey}, }; use crate::{ - address::{RecipientAddress, UnifiedAddress}, + address::UnifiedAddress, decrypt::DecryptedOutput, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, @@ -27,10 +28,7 @@ use crate::{ }; #[cfg(feature = "transparent-inputs")] -use { - crate::wallet::WalletTransparentOutput, - zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}, -}; +use {crate::wallet::WalletTransparentOutput, zcash_primitives::transaction::components::OutPoint}; pub mod chain; pub mod error; @@ -254,13 +252,6 @@ pub struct SentTransaction<'a> { pub utxos_spent: Vec, } -#[derive(Debug, Clone)] -#[allow(clippy::large_enum_variant)] -pub enum Recipient { - Address(RecipientAddress), - InternalAccount(AccountId), -} - /// A value pool to which the wallet supports sending transaction outputs. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum PoolType { @@ -270,9 +261,19 @@ pub enum PoolType { Sapling, } +/// A type that represents the recipient of a transaction output; a recipient address (and, for +/// unified addresses, the pool to which the payment is sent) in the case of outgoing output, or an +/// internal account ID and the pool to which funds were sent in the case of a wallet-internal +/// output. +#[derive(Debug, Clone)] +pub enum Recipient { + Transparent(TransparentAddress), + Sapling(PaymentAddress), + Unified(UnifiedAddress, PoolType), + InternalAccount(AccountId, PoolType), +} + pub struct SentTransactionOutput { - /// The value in which the output has been created - pub output_pool: PoolType, /// The index within the transaction that contains the recipient output. /// /// - If `recipient_address` is a Sapling address, this is an index into the Sapling diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 47bc7aa2c..84df3e2c5 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -374,13 +374,17 @@ where let (tx, tx_metadata) = builder.build(&prover).map_err(Error::Builder)?; let sent_outputs = request.payments().iter().enumerate().map(|(i, payment)| { - let (output_index, output_pool) = match &payment.recipient_address { + let (output_index, recipient) = match &payment.recipient_address { // Sapling outputs are shuffled, so we need to look up where the output ended up. // TODO: When we add Orchard support, we will need to trial-decrypt to find them, // and return the appropriate pool type. - RecipientAddress::Shielded(_) | RecipientAddress::Unified(_) => { + RecipientAddress::Shielded(addr) => { let idx = tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."); - (idx, PoolType::Sapling) + (idx, Recipient::Sapling(addr.clone())) + } + RecipientAddress::Unified(addr) => { + let idx = tx_metadata.output_index(i).expect("An output should exist in the transaction for each shielded payment."); + (idx, Recipient::Unified(addr.clone(), PoolType::Sapling)) } RecipientAddress::Transparent(addr) => { let script = addr.script(); @@ -394,14 +398,13 @@ where .map(|(index, _)| index) .expect("An output should exist in the transaction for each transparent payment."); - (idx, PoolType::Transparent) + (idx, Recipient::Transparent(*addr)) } }; SentTransactionOutput { - output_pool, output_index, - recipient: Recipient::Address(payment.recipient_address.clone()), + recipient, value: payment.amount, memo: payment.memo.clone() } @@ -532,10 +535,9 @@ where created: time::OffsetDateTime::now_utc(), account, outputs: vec![SentTransactionOutput { - output_pool: PoolType::Sapling, output_index, value: amount_to_shield, - recipient: Recipient::InternalAccount(account), + recipient: Recipient::InternalAccount(account, PoolType::Sapling), memo: Some(memo.clone()), }], fee_amount: fee, diff --git a/zcash_client_backend/src/encoding.rs b/zcash_client_backend/src/encoding.rs index b63a4aff8..6350c47e6 100644 --- a/zcash_client_backend/src/encoding.rs +++ b/zcash_client_backend/src/encoding.rs @@ -5,14 +5,16 @@ //! //! [constants]: zcash_primitives::constants +use crate::address::UnifiedAddress; use bech32::{self, Error, FromBase32, ToBase32, Variant}; use bs58::{self, decode::Error as Bs58Error}; use std::fmt; use std::io::{self, Write}; +use zcash_address::unified::{self, Encoding}; use zcash_primitives::{ consensus, legacy::TransparentAddress, - sapling::PaymentAddress, + sapling, zip32::{ExtendedFullViewingKey, ExtendedSpendingKey}, }; @@ -132,6 +134,41 @@ impl AddressCodec

for TransparentAddress { } } +impl AddressCodec

for sapling::PaymentAddress { + type Error = Bech32DecodeError; + + fn encode(&self, params: &P) -> String { + encode_payment_address(params.hrp_sapling_payment_address(), self) + } + + fn decode(params: &P, address: &str) -> Result { + decode_payment_address(params.hrp_sapling_payment_address(), address) + } +} + +impl AddressCodec

for UnifiedAddress { + type Error = String; + + fn encode(&self, params: &P) -> String { + self.encode(params) + } + + fn decode(params: &P, address: &str) -> Result { + unified::Address::decode(address) + .map_err(|e| format!("{}", e)) + .and_then(|(network, addr)| { + if params.address_network() == Some(network) { + UnifiedAddress::try_from(addr).map_err(|e| e.to_owned()) + } else { + Err(format!( + "Address {} is for a different network: {:?}", + address, network + )) + } + }) + } +} + /// Writes an [`ExtendedSpendingKey`] as a Bech32-encoded string. /// /// # Examples @@ -232,16 +269,18 @@ pub fn decode_extended_full_viewing_key( /// ); /// ``` /// [`PaymentAddress`]: zcash_primitives::sapling::PaymentAddress -pub fn encode_payment_address(hrp: &str, addr: &PaymentAddress) -> String { +pub fn encode_payment_address(hrp: &str, addr: &sapling::PaymentAddress) -> String { bech32_encode(hrp, |w| w.write_all(&addr.to_bytes())) } /// Writes a [`PaymentAddress`] as a Bech32-encoded string /// using the human-readable prefix values defined in the specified /// network parameters. +/// +/// [`PaymentAddress`]: zcash_primitives::sapling::PaymentAddress pub fn encode_payment_address_p( params: &P, - addr: &PaymentAddress, + addr: &sapling::PaymentAddress, ) -> String { encode_payment_address(params.hrp_sapling_payment_address(), addr) } @@ -259,7 +298,7 @@ pub fn encode_payment_address_p( /// encoding::decode_payment_address, /// }; /// use zcash_primitives::{ -/// constants::testnet::HRP_SAPLING_PAYMENT_ADDRESS, +/// consensus::{TEST_NETWORK, Parameters}, /// sapling::{Diversifier, PaymentAddress}, /// }; /// @@ -276,14 +315,17 @@ pub fn encode_payment_address_p( /// /// assert_eq!( /// decode_payment_address( -/// HRP_SAPLING_PAYMENT_ADDRESS, +/// TEST_NETWORK.hrp_sapling_payment_address(), /// "ztestsapling1qqqqqqqqqqqqqqqqqqcguyvaw2vjk4sdyeg0lc970u659lvhqq7t0np6hlup5lusxle75ss7jnk", /// ), /// Ok(pa), /// ); /// ``` /// [`PaymentAddress`]: zcash_primitives::sapling::PaymentAddress -pub fn decode_payment_address(hrp: &str, s: &str) -> Result { +pub fn decode_payment_address( + hrp: &str, + s: &str, +) -> Result { bech32_decode(hrp, s, |data| { if data.len() != 43 { return None; @@ -291,7 +333,7 @@ pub fn decode_payment_address(hrp: &str, s: &str) -> Result Result Result( /// /// ``` /// use zcash_primitives::{ -/// constants::testnet::{B58_PUBKEY_ADDRESS_PREFIX, B58_SCRIPT_ADDRESS_PREFIX}, +/// consensus::{TEST_NETWORK, Parameters}, /// }; /// use zcash_client_backend::{ /// encoding::decode_transparent_address, @@ -378,8 +420,8 @@ pub fn encode_transparent_address_p( /// /// assert_eq!( /// decode_transparent_address( -/// &B58_PUBKEY_ADDRESS_PREFIX, -/// &B58_SCRIPT_ADDRESS_PREFIX, +/// &TEST_NETWORK.b58_pubkey_address_prefix(), +/// &TEST_NETWORK.b58_script_address_prefix(), /// "tm9iMLAuYMzJ6jtFLcA7rzUmfreGuKvr7Ma", /// ), /// Ok(Some(TransparentAddress::PublicKey([0; 20]))), @@ -387,8 +429,8 @@ pub fn encode_transparent_address_p( /// /// assert_eq!( /// decode_transparent_address( -/// &B58_PUBKEY_ADDRESS_PREFIX, -/// &B58_SCRIPT_ADDRESS_PREFIX, +/// &TEST_NETWORK.b58_pubkey_address_prefix(), +/// &TEST_NETWORK.b58_script_address_prefix(), /// "t26YoyZ1iPgiMEWL4zGUm74eVWfhyDMXzY2", /// ), /// Ok(Some(TransparentAddress::Script([0; 20]))), diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index ee3a3aa38..c3956cfef 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -53,7 +53,7 @@ use zcash_primitives::{ }; use zcash_client_backend::{ - address::{RecipientAddress, UnifiedAddress}, + address::UnifiedAddress, data_api::{ BlockSource, DecryptedTransaction, PoolType, PrunedBlock, Recipient, SentTransaction, WalletRead, WalletWrite, @@ -534,16 +534,15 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { match output.transfer_type { TransferType::Outgoing | TransferType::WalletInternal => { let recipient = if output.transfer_type == TransferType::Outgoing { - Recipient::Address(RecipientAddress::Shielded(output.to.clone())) + Recipient::Sapling(output.to.clone()) } else { - Recipient::InternalAccount(output.account) + Recipient::InternalAccount(output.account, PoolType::Sapling) }; wallet::put_sent_output( up, output.account, tx_ref, - PoolType::Sapling, output.index, &recipient, Amount::from_u64(output.note.value).unwrap(), @@ -577,12 +576,11 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { .any(|input| *nf == input.nullifier) ) { for (output_index, txout) in d_tx.tx.transparent_bundle().iter().flat_map(|b| b.vout.iter()).enumerate() { - let recipient = Recipient::Address(RecipientAddress::Transparent(txout.script_pubkey.address().unwrap())); + let recipient = Recipient::Transparent(txout.script_pubkey.address().unwrap()); wallet::put_sent_output( up, *account_id, tx_ref, - PoolType::Transparent, output_index, &recipient, txout.value, diff --git a/zcash_client_sqlite/src/prepared.rs b/zcash_client_sqlite/src/prepared.rs index 870a3275f..e2b11c3f9 100644 --- a/zcash_client_sqlite/src/prepared.rs +++ b/zcash_client_sqlite/src/prepared.rs @@ -21,15 +21,15 @@ use zcash_primitives::{ use zcash_client_backend::{ address::UnifiedAddress, data_api::{PoolType, Recipient}, + encoding::AddressCodec, }; use crate::{error::SqliteClientError, wallet::pool_code, NoteId, WalletDb}; #[cfg(feature = "transparent-inputs")] use { - crate::UtxoId, - rusqlite::OptionalExtension, - zcash_client_backend::{encoding::AddressCodec, wallet::WalletTransparentOutput}, + crate::UtxoId, rusqlite::OptionalExtension, + zcash_client_backend::wallet::WalletTransparentOutput, zcash_primitives::transaction::components::transparent::OutPoint, }; @@ -368,16 +368,27 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { pub(crate) fn stmt_insert_sent_output( &mut self, tx_ref: i64, - pool_type: PoolType, output_index: usize, from_account: AccountId, to: &Recipient, value: Amount, memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { - let (to_address, to_account) = match to { - Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), - Recipient::Address(shielded) => (Some(shielded.encode(&self.wallet_db.params)), None), + let (to_address, to_account, pool_type) = match to { + Recipient::Transparent(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Transparent, + ), + Recipient::Sapling(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Sapling, + ), + Recipient::Unified(addr, pool) => { + (Some(addr.encode(&self.wallet_db.params)), None, *pool) + } + Recipient::InternalAccount(id, pool) => (None, Some(u32::from(*id)), *pool), }; self.stmt_insert_sent_output.execute(named_params![ @@ -405,13 +416,25 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { value: Amount, memo: Option<&MemoBytes>, tx_ref: i64, - pool_type: PoolType, output_index: usize, ) -> Result { - let (to_address, to_account) = match to { - Recipient::InternalAccount(id) => (None, Some(u32::from(*id))), - Recipient::Address(shielded) => (Some(shielded.encode(&self.wallet_db.params)), None), + let (to_address, to_account, pool_type) = match to { + Recipient::Transparent(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Transparent, + ), + Recipient::Sapling(addr) => ( + Some(addr.encode(&self.wallet_db.params)), + None, + PoolType::Sapling, + ), + Recipient::Unified(addr, pool) => { + (Some(addr.encode(&self.wallet_db.params)), None, *pool) + } + Recipient::InternalAccount(id, pool) => (None, Some(u32::from(*id)), *pool), }; + match self.stmt_update_sent_output.execute(named_params![ ":from_account": &u32::from(from_account), ":to_address": &to_address, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index b325cf86b..70fb64814 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1154,7 +1154,6 @@ pub(crate) fn insert_sent_output<'a, P: consensus::Parameters>( ) -> Result<(), SqliteClientError> { stmts.stmt_insert_sent_output( tx_ref, - output.output_pool, output.output_index, from_account, &output.recipient, @@ -1171,24 +1170,14 @@ pub(crate) fn put_sent_output<'a, P: consensus::Parameters>( stmts: &mut DataConnStmtCache<'a, P>, from_account: AccountId, tx_ref: i64, - output_pool: PoolType, output_index: usize, recipient: &Recipient, value: Amount, memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { - if !stmts.stmt_update_sent_output( - from_account, - recipient, - value, - memo, - tx_ref, - output_pool, - output_index, - )? { + if !stmts.stmt_update_sent_output(from_account, recipient, value, memo, tx_ref, output_index)? { stmts.stmt_insert_sent_output( tx_ref, - output_pool, output_index, from_account, recipient, diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 6773af3df..ccd92629f 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -309,14 +309,17 @@ mod tests { use crate::{ error::SqliteClientError, tests::{self, network}, - wallet::{get_address, pool_code}, + wallet::get_address, AccountId, WalletDb, }; use super::{init_accounts_table, init_blocks_table, init_wallet_db}; #[cfg(feature = "transparent-inputs")] - use {crate::wallet::PoolType, zcash_primitives::legacy::keys as transparent}; + use { + crate::wallet::{pool_code, PoolType}, + zcash_primitives::legacy::keys as transparent, + }; #[test] fn verify_schema() {