From e78a5ecd2a33061e4cad519d437372baea09924d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 25 Jan 2024 17:23:05 -0700 Subject: [PATCH] zcash_keys: Box `Address` elements to avoid large variations in enum variant sizes --- zcash_client_backend/CHANGELOG.md | 2 ++ zcash_client_backend/src/data_api/wallet.rs | 12 ++++--- .../src/data_api/wallet/input_selection.rs | 2 +- zcash_client_backend/src/wallet.rs | 6 ++-- zcash_client_backend/src/zip321.rs | 6 ++-- zcash_client_sqlite/src/chain.rs | 2 +- zcash_client_sqlite/src/lib.rs | 4 +-- zcash_client_sqlite/src/wallet.rs | 2 +- zcash_client_sqlite/src/wallet/init.rs | 4 +-- .../wallet/init/migrations/addresses_table.rs | 6 ++-- .../init/migrations/receiving_key_scopes.rs | 2 +- .../wallet/init/migrations/ufvk_support.rs | 8 ++--- zcash_client_sqlite/src/wallet/sapling.rs | 14 ++++----- zcash_keys/CHANGELOG.md | 3 +- zcash_keys/src/address.rs | 31 +++++++++---------- zcash_keys/src/keys.rs | 5 +-- 16 files changed, 55 insertions(+), 54 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index ff7c83239f..5dbb0a52a4 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -220,6 +220,8 @@ and this library adheres to Rust's notion of - The fields of `ReceivedSaplingNote` are now private. Use `ReceivedSaplingNote::from_parts` for construction instead. Accessor methods are provided for each previously public field. + - The address variants of `Recipient` now `Box` their contents to avoid large + discrepancies in enum variant sizing. - `zcash_client_backend::scanning::ScanError` has a new variant, `TreeSizeInvalid`. - `zcash_client_backend::zip321::TransactionRequest::payments` now returns a `BTreeMap` instead of `&[Payment]` so that parameter diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 4015bc6c39..2602fa79c1 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -775,7 +775,7 @@ where .expect("Payment step references are checked at construction") .recipient_address { - Address::Transparent(t) => Some(t), + Address::Transparent(t) => Some(t.as_ref()), Address::Unified(uaddr) => uaddr.transparent(), _ => None, } @@ -849,8 +849,12 @@ where .memo .as_ref() .map_or_else(MemoBytes::empty, |m| m.clone()); - builder.add_sapling_output(external_ovk, *addr, payment.amount, memo.clone())?; - sapling_output_meta.push((Recipient::Sapling(*addr), payment.amount, Some(memo))); + builder.add_sapling_output(external_ovk, **addr, payment.amount, memo.clone())?; + sapling_output_meta.push(( + Recipient::Sapling(addr.clone()), + payment.amount, + Some(memo), + )); } Address::Transparent(to) => { if payment.memo.is_some() { @@ -950,7 +954,7 @@ where SentTransactionOutput::from_parts( output_index, - Recipient::Transparent(*addr), + Recipient::Transparent(addr.clone()), value, None, None, diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs index c38cd970c2..cef234bb72 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -381,7 +381,7 @@ where } return Err(InputSelectorError::Selection( - GreedyInputSelectorError::UnsupportedAddress(Box::new(addr.clone())), + GreedyInputSelectorError::UnsupportedAddress(addr.clone()), )); } } diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 35b694b619..cef229120a 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -66,9 +66,9 @@ impl NoteId { /// output. #[derive(Debug, Clone)] pub enum Recipient { - Transparent(TransparentAddress), - Sapling(sapling::PaymentAddress), - Unified(UnifiedAddress, PoolType), + Transparent(Box), + Sapling(Box), + Unified(Box, PoolType), InternalAccount(AccountId, PoolType), } diff --git a/zcash_client_backend/src/zip321.rs b/zcash_client_backend/src/zip321.rs index 29d5eadec7..74bd77cc6b 100644 --- a/zcash_client_backend/src/zip321.rs +++ b/zcash_client_backend/src/zip321.rs @@ -891,7 +891,7 @@ mod tests { let expected = TransactionRequest::new( vec![ Payment { - recipient_address: Address::Sapling(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), + recipient_address: Address::from(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), amount: NonNegativeAmount::const_from_u64(376876902796286), memo: None, label: None, @@ -912,7 +912,7 @@ mod tests { let expected = TransactionRequest::new( vec![ Payment { - recipient_address: Address::Sapling(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), + recipient_address: Address::from(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), amount: NonNegativeAmount::ZERO, memo: None, label: None, @@ -930,7 +930,7 @@ mod tests { let req = TransactionRequest::new( vec![ Payment { - recipient_address: Address::Sapling(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), + recipient_address: Address::from(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), amount: NonNegativeAmount::ZERO, memo: None, label: None, diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 52a0a7df57..fcaece6092 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -520,7 +520,7 @@ mod tests { // We can spend the received notes let req = TransactionRequest::new(vec![Payment { - recipient_address: Address::Sapling(dfvk.default_address().1), + recipient_address: Address::from(dfvk.default_address().1), amount: NonNegativeAmount::const_from_u64(110_000), memo: None, label: None, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 18f4ef34f3..8ccb29ddf1 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -589,7 +589,7 @@ impl WalletWrite for WalletDb match output.transfer_type { TransferType::Outgoing | TransferType::WalletInternal => { let recipient = if output.transfer_type == TransferType::Outgoing { - Recipient::Sapling(output.note.recipient()) + Recipient::Sapling(Box::new(output.note.recipient())) } else { Recipient::InternalAccount( output.account, @@ -658,7 +658,7 @@ impl WalletWrite for WalletDb *account_id, tx_ref, output_index, - &Recipient::Transparent(address), + &Recipient::Transparent(Box::new(address)), txout.value, None )?; diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 665456473c..8b29d57d11 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -298,7 +298,7 @@ pub(crate) fn get_current_address( SqliteClientError::CorruptedData("Not a valid Zcash recipient address".to_owned()) }) .and_then(|addr| match addr { - Address::Unified(ua) => Ok(ua), + Address::Unified(ua) => Ok(*ua), _ => Err(SqliteClientError::CorruptedData(format!( "Addresses table contains {} which is not a unified address", addr_str, diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 14bf46d30f..925a427926 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -1008,7 +1008,7 @@ mod tests { let ufvk_str = ufvk.encode(&wdb.params); let address_str = - Address::Unified(ufvk.default_address(DEFAULT_UA_REQUEST).0).encode(&wdb.params); + Address::from(ufvk.default_address(DEFAULT_UA_REQUEST).0).encode(&wdb.params); wdb.conn.execute( "INSERT INTO accounts (account, ufvk, address, transparent_address) VALUES (?, ?, ?, '')", @@ -1022,7 +1022,7 @@ mod tests { // add a transparent "sent note" #[cfg(feature = "transparent-inputs")] { - let taddr = Address::Transparent( + let taddr = Address::from( *ufvk .default_address(DEFAULT_UA_REQUEST) .0 diff --git a/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs b/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs index 5cc820d792..1205bd3d27 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs @@ -82,7 +82,7 @@ impl RusqliteMigration for Migration

{ )) })?; let decoded_address = if let Address::Unified(ua) = decoded { - ua + *ua } else { return Err(WalletMigrationError::CorruptedData( "Address in accounts table was not a Unified Address.".to_string(), @@ -95,7 +95,7 @@ impl RusqliteMigration for Migration

{ return Err(WalletMigrationError::CorruptedData(format!( "Decoded UA {} does not match the UFVK's default address {} at {:?}.", address, - Address::Unified(expected_address).encode(&self.params), + Address::from(expected_address).encode(&self.params), idx, ))); } @@ -113,7 +113,7 @@ impl RusqliteMigration for Migration

{ let decoded_transparent_address = if let Address::Transparent(addr) = decoded_transparent { - addr + *addr } else { return Err(WalletMigrationError::CorruptedData( "Address in transparent_address column of accounts table was not a transparent address.".to_string(), diff --git a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs index 9836bb8d57..87e0ec5c43 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs @@ -500,7 +500,7 @@ mod tests { match output.transfer_type { TransferType::Outgoing | TransferType::WalletInternal => { let recipient = if output.transfer_type == TransferType::Outgoing { - Recipient::Sapling(output.note.recipient()) + Recipient::Sapling(Box::new(output.note.recipient())) } else { Recipient::InternalAccount( output.account, diff --git a/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs b/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs index 380b9737c9..ed54ba3c20 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs @@ -98,11 +98,11 @@ impl RusqliteMigration for Migration

{ "Derivation should have produced a UFVK containing a Sapling component.", ); let (idx, expected_address) = dfvk.default_address(); - if decoded_address != expected_address { + if *decoded_address != expected_address { return Err(WalletMigrationError::CorruptedData( format!("Decoded Sapling address {} does not match the ufvk's Sapling address {} at {:?}.", address, - Address::Sapling(expected_address).encode(&self.params), + Address::from(expected_address).encode(&self.params), idx))); } } @@ -112,11 +112,11 @@ impl RusqliteMigration for Migration

{ } Address::Unified(decoded_address) => { let (expected_address, idx) = ufvk.default_address(ua_request); - if decoded_address != expected_address { + if *decoded_address != expected_address { return Err(WalletMigrationError::CorruptedData( format!("Decoded unified address {} does not match the ufvk's default address {} at {:?}.", address, - Address::Unified(expected_address).encode(&self.params), + Address::from(expected_address).encode(&self.params), idx))); } } diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index b7401cf7ff..b8f83ab4ee 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -718,7 +718,7 @@ pub(crate) mod tests { // spends the first step's output. // The first step will deshield to the wallet's default transparent address - let to0 = Address::Transparent(usk.default_transparent_address().0); + let to0 = Address::Transparent(Box::new(usk.default_transparent_address().0)); let request0 = zip321::TransactionRequest::new(vec![Payment { recipient_address: to0, amount: NonNegativeAmount::const_from_u64(50000), @@ -754,14 +754,14 @@ pub(crate) mod tests { // We'll use an internal transparent address that hasn't been added to the wallet // to simulate an external transparent recipient. - let to1 = Address::Transparent( + let to1 = Address::Transparent(Box::new( usk.transparent() .to_account_pubkey() .derive_internal_ivk() .unwrap() .default_address() .0, - ); + )); let request1 = zip321::TransactionRequest::new(vec![Payment { recipient_address: to1, amount: NonNegativeAmount::const_from_u64(40000), @@ -1433,7 +1433,7 @@ pub(crate) mod tests { let req = TransactionRequest::new(vec![ // payment to an external recipient Payment { - recipient_address: Address::Sapling(addr2), + recipient_address: Address::from(addr2), amount: amount_sent, memo: None, label: None, @@ -1442,7 +1442,7 @@ pub(crate) mod tests { }, // payment back to the originating wallet, simulating legacy change Payment { - recipient_address: Address::Sapling(addr), + recipient_address: Address::from(addr), amount: amount_legacy_change, memo: None, label: None, @@ -1556,7 +1556,7 @@ pub(crate) mod tests { // This first request will fail due to insufficient non-dust funds let req = TransactionRequest::new(vec![Payment { - recipient_address: Address::Sapling(dfvk.default_address().1), + recipient_address: Address::from(dfvk.default_address().1), amount: NonNegativeAmount::const_from_u64(50000), memo: None, label: None, @@ -1581,7 +1581,7 @@ pub(crate) mod tests { // This request will succeed, spending a single dust input to pay the 10000 // ZAT fee in addition to the 41000 ZAT output to the recipient let req = TransactionRequest::new(vec![Payment { - recipient_address: Address::Sapling(dfvk.default_address().1), + recipient_address: Address::from(dfvk.default_address().1), amount: NonNegativeAmount::const_from_u64(41000), memo: None, label: None, diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 705214b829..3102aeef99 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -24,7 +24,8 @@ The entries below are relative to the `zcash_client_backend` crate as of ### Changed - `zcash_keys::address`: - - `RecipientAddress` has been renamed to `Address` + - `RecipientAddress` has been renamed to `Address`. Also, each variant now + `Box`es its contents to avoid large discrepancies in enum variant sizing. - `Address::Shielded` has been renamed to `Address::Sapling` - `UnifiedAddress::from_receivers` has been renamed to `UnifiedAddress::new`. It no longer takes an Orchard receiver argument unless the `orchard` feature diff --git a/zcash_keys/src/address.rs b/zcash_keys/src/address.rs index 82d9d82192..50677347c8 100644 --- a/zcash_keys/src/address.rs +++ b/zcash_keys/src/address.rs @@ -234,10 +234,7 @@ impl UnifiedAddress { self.expiry_height .map(|h| unified::MetadataItem::ExpiryHeight(u32::from(h))), ) - .chain( - self.expiry_time - .map(unified::MetadataItem::ExpiryTime), - ) + .chain(self.expiry_time.map(unified::MetadataItem::ExpiryTime)) .map(Item::Metadata); data_items.chain(meta_items).collect() @@ -250,26 +247,26 @@ impl UnifiedAddress { /// An address that funds can be sent to. #[derive(Debug, PartialEq, Eq, Clone)] pub enum Address { - Sapling(PaymentAddress), - Transparent(TransparentAddress), - Unified(UnifiedAddress), + Sapling(Box), + Transparent(Box), + Unified(Box), } impl From for Address { fn from(addr: PaymentAddress) -> Self { - Address::Sapling(addr) + Address::Sapling(Box::new(addr)) } } impl From for Address { fn from(addr: TransparentAddress) -> Self { - Address::Transparent(addr) + Address::Transparent(Box::new(addr)) } } impl From for Address { fn from(addr: UnifiedAddress) -> Self { - Address::Unified(addr) + Address::Unified(Box::new(addr)) } } @@ -312,12 +309,12 @@ impl Address { match self { Address::Sapling(pa) => ZcashAddress::from_sapling(net, pa.to_bytes()), - Address::Transparent(addr) => match addr { + Address::Transparent(addr) => match **addr { TransparentAddress::PublicKeyHash(data) => { - ZcashAddress::from_transparent_p2pkh(net, *data) + ZcashAddress::from_transparent_p2pkh(net, data) } TransparentAddress::ScriptHash(data) => { - ZcashAddress::from_transparent_p2sh(net, *data) + ZcashAddress::from_transparent_p2sh(net, data) } }, Address::Unified(ua) => ua.to_address(net), @@ -345,9 +342,9 @@ pub mod testing { pub fn arb_addr(request: UnifiedAddressRequest) -> impl Strategy { prop_oneof![ - arb_payment_address().prop_map(Address::Sapling), - arb_transparent_addr().prop_map(Address::Transparent), - arb_unified_addr(Network::TestNetwork, request).prop_map(Address::Unified), + arb_payment_address().prop_map(Address::from), + arb_transparent_addr().prop_map(Address::from), + arb_unified_addr(Network::TestNetwork, request).prop_map(Address::from), ] } } @@ -384,7 +381,7 @@ mod tests { #[cfg(not(feature = "orchard"))] let ua = UnifiedAddress::new(sapling, transparent, None, None).unwrap(); - let addr = Address::Unified(ua); + let addr = Address::from(ua); let addr_str = addr.encode(&MAIN_NETWORK); assert_eq!(Address::decode(&MAIN_NETWORK, &addr_str), Some(addr)); } diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index 33a304d9a9..2462ec24ef 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -646,10 +646,7 @@ impl UnifiedFullViewingKey { self.expiry_height .map(|h| unified::MetadataItem::ExpiryHeight(u32::from(h))), ) - .chain( - self.expiry_time - .map(unified::MetadataItem::ExpiryTime), - ); + .chain(self.expiry_time.map(unified::MetadataItem::ExpiryTime)); let ufvk = unified::Ufvk::try_from_items( data_items