From b68d37a0b1d7153e5442f16944bc3e67035f7df8 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 31 Oct 2022 14:54:33 -0600 Subject: [PATCH 1/4] Use DiversifiableFullViewingKey instead of ExtFVK where possible. --- zcash_client_backend/src/address.rs | 6 +-- zcash_client_backend/src/encoding.rs | 3 +- zcash_client_backend/src/keys.rs | 9 ++-- zcash_client_backend/src/welding_rig.rs | 58 ++++++-------------- zcash_client_sqlite/src/lib.rs | 4 +- zcash_client_sqlite/src/wallet/init.rs | 12 ++--- zcash_client_sqlite/src/wallet/transact.rs | 8 +-- zcash_primitives/CHANGELOG.md | 2 + zcash_primitives/src/transaction/builder.rs | 18 +++---- zcash_primitives/src/zip32/sapling.rs | 59 ++++++++++----------- 10 files changed, 75 insertions(+), 104 deletions(-) diff --git a/zcash_client_backend/src/address.rs b/zcash_client_backend/src/address.rs index ac5f1e3bb..b83cea085 100644 --- a/zcash_client_backend/src/address.rs +++ b/zcash_client_backend/src/address.rs @@ -253,7 +253,7 @@ impl RecipientAddress { #[cfg(test)] mod tests { use zcash_address::test_vectors; - use zcash_primitives::{consensus::MAIN_NETWORK, zip32::ExtendedFullViewingKey}; + use zcash_primitives::consensus::MAIN_NETWORK; use super::{RecipientAddress, UnifiedAddress}; use crate::keys::sapling; @@ -268,8 +268,8 @@ mod tests { let sapling = { let extsk = sapling::spending_key(&[0; 32], 0, 0.into()); - let extfvk = ExtendedFullViewingKey::from(&extsk); - Some(extfvk.default_address().1) + let dfvk = extsk.to_diversifiable_full_viewing_key(); + Some(dfvk.default_address().1) }; let transparent = { None }; diff --git a/zcash_client_backend/src/encoding.rs b/zcash_client_backend/src/encoding.rs index 6350c47e6..053f6d85e 100644 --- a/zcash_client_backend/src/encoding.rs +++ b/zcash_client_backend/src/encoding.rs @@ -517,8 +517,9 @@ mod tests { } #[test] + #[allow(deprecated)] fn extended_full_viewing_key() { - let extfvk = (&ExtendedSpendingKey::master(&[0; 32][..])).into(); + let extfvk = ExtendedSpendingKey::master(&[0; 32][..]).to_extended_full_viewing_key(); let encoded_main = "zxviews1qqqqqqqqqqqqqq8n3zjjmvhhr854uy3qhpda3ml34haf0x388z5r7h4st4kpsf6qy3zw4wc246aw9rlfyg5ndlwvne7mwdq0qe6vxl42pqmcf8pvmmd5slmjxduqa9evgej6wa3th2505xq4nggrxdm93rxk4rpdjt5nmq2vn44e2uhm7h0hsagfvkk4n7n6nfer6u57v9cac84t7nl2zth0xpyfeg0w2p2wv2yn6jn923aaz0vdaml07l60ahapk6efchyxwysrvjsxmansf"; let encoded_test = "zxviewtestsapling1qqqqqqqqqqqqqq8n3zjjmvhhr854uy3qhpda3ml34haf0x388z5r7h4st4kpsf6qy3zw4wc246aw9rlfyg5ndlwvne7mwdq0qe6vxl42pqmcf8pvmmd5slmjxduqa9evgej6wa3th2505xq4nggrxdm93rxk4rpdjt5nmq2vn44e2uhm7h0hsagfvkk4n7n6nfer6u57v9cac84t7nl2zth0xpyfeg0w2p2wv2yn6jn923aaz0vdaml07l60ahapk6efchyxwysrvjs8evfkz"; diff --git a/zcash_client_backend/src/keys.rs b/zcash_client_backend/src/keys.rs index 7051bd029..54a585dcb 100644 --- a/zcash_client_backend/src/keys.rs +++ b/zcash_client_backend/src/keys.rs @@ -175,7 +175,7 @@ impl UnifiedSpendingKey { UnifiedFullViewingKey { #[cfg(feature = "transparent-inputs")] transparent: Some(self.transparent.to_account_pubkey()), - sapling: Some(sapling::ExtendedFullViewingKey::from(&self.sapling).into()), + sapling: Some(self.sapling.to_diversifiable_full_viewing_key()), orchard: Some((&self.orchard).into()), unknown: vec![], } @@ -584,10 +584,7 @@ mod tests { use proptest::prelude::proptest; use super::{sapling, UnifiedFullViewingKey}; - use zcash_primitives::{ - consensus::MAIN_NETWORK, - zip32::{AccountId, ExtendedFullViewingKey}, - }; + use zcash_primitives::{consensus::MAIN_NETWORK, zip32::AccountId}; #[cfg(feature = "transparent-inputs")] use { @@ -647,7 +644,7 @@ mod tests { let sapling = { let extsk = sapling::spending_key(&[0; 32], 0, account); - Some(ExtendedFullViewingKey::from(&extsk).into()) + Some(extsk.to_diversifiable_full_viewing_key()) }; #[cfg(feature = "transparent-inputs")] diff --git a/zcash_client_backend/src/welding_rig.rs b/zcash_client_backend/src/welding_rig.rs index 09cf89fba..0a0b9fa72 100644 --- a/zcash_client_backend/src/welding_rig.rs +++ b/zcash_client_backend/src/welding_rig.rs @@ -15,10 +15,7 @@ use zcash_primitives::{ Node, Note, Nullifier, NullifierDerivingKey, SaplingIvk, }, transaction::components::sapling::CompactOutputDescription, - zip32::{ - sapling::{DiversifiableFullViewingKey, ExtendedFullViewingKey}, - AccountId, Scope, - }, + zip32::{sapling::DiversifiableFullViewingKey, AccountId, Scope}, }; use crate::{ @@ -98,29 +95,6 @@ impl ScanningKey for DiversifiableFullViewingKey { } } -/// The [`ScanningKey`] implementation for [`ExtendedFullViewingKey`]s. -/// Nullifiers may be derived when scanning with these keys. -/// -/// [`ExtendedFullViewingKey`]: zcash_primitives::zip32::ExtendedFullViewingKey -impl ScanningKey for ExtendedFullViewingKey { - type Scope = Scope; - type SaplingNk = NullifierDerivingKey; - type SaplingKeys = [(Self::Scope, SaplingIvk, Self::SaplingNk); 1]; - type Nf = sapling::Nullifier; - - fn to_sapling_keys(&self) -> Self::SaplingKeys { - [(Scope::External, self.fvk.vk.ivk(), self.fvk.vk.nk)] - } - - fn sapling_nf( - key: &Self::SaplingNk, - note: &Note, - witness: &IncrementalWitness, - ) -> Self::Nf { - note.nf(key, witness.position() as u64) - } -} - /// The [`ScanningKey`] implementation for [`SaplingIvk`]s. /// Nullifiers cannot be derived when scanning with these keys. /// @@ -430,7 +404,7 @@ mod tests { Note, Nullifier, SaplingIvk, }, transaction::components::Amount, - zip32::{AccountId, ExtendedFullViewingKey, ExtendedSpendingKey}, + zip32::{AccountId, DiversifiableFullViewingKey, ExtendedSpendingKey}, }; use crate::{ @@ -480,11 +454,11 @@ mod tests { fn fake_compact_block( height: BlockHeight, nf: Nullifier, - extfvk: ExtendedFullViewingKey, + dfvk: &DiversifiableFullViewingKey, value: Amount, tx_after: bool, ) -> CompactBlock { - let to = extfvk.default_address().1; + let to = dfvk.default_address().1; // Create a fake Note for the account let mut rng = OsRng; @@ -496,7 +470,7 @@ mod tests { rseed, }; let encryptor = sapling_note_encryption::<_, Network>( - Some(extfvk.fvk.ovk), + Some(dfvk.fvk().ovk), note.clone(), to, MemoBytes::empty(), @@ -554,12 +528,12 @@ mod tests { fn go(scan_multithreaded: bool) { let account = AccountId::from(0); let extsk = ExtendedSpendingKey::master(&[]); - let extfvk = ExtendedFullViewingKey::from(&extsk); + let dfvk = extsk.to_diversifiable_full_viewing_key(); let cb = fake_compact_block( 1u32.into(), Nullifier([0; 32]), - extfvk.clone(), + &dfvk, Amount::from_u64(5).unwrap(), false, ); @@ -569,8 +543,7 @@ mod tests { let mut batch_runner = if scan_multithreaded { let mut runner = BatchRunner::<_, _, _, ()>::new( 10, - extfvk - .to_sapling_keys() + dfvk.to_sapling_keys() .iter() .map(|(scope, ivk, _)| ((account, *scope), ivk)) .map(|(tag, ivk)| (tag, PreparedIncomingViewingKey::new(ivk))), @@ -587,7 +560,7 @@ mod tests { let txs = scan_block_with_runner( &Network::TestNetwork, cb, - &[(&account, &extfvk)], + &[(&account, &dfvk)], &[], &mut tree, &mut [], @@ -618,12 +591,12 @@ mod tests { fn go(scan_multithreaded: bool) { let account = AccountId::from(0); let extsk = ExtendedSpendingKey::master(&[]); - let extfvk = ExtendedFullViewingKey::from(&extsk); + let dfvk = extsk.to_diversifiable_full_viewing_key(); let cb = fake_compact_block( 1u32.into(), Nullifier([0; 32]), - extfvk.clone(), + &dfvk, Amount::from_u64(5).unwrap(), true, ); @@ -633,8 +606,7 @@ mod tests { let mut batch_runner = if scan_multithreaded { let mut runner = BatchRunner::<_, _, _, ()>::new( 10, - extfvk - .to_sapling_keys() + dfvk.to_sapling_keys() .iter() .map(|(scope, ivk, _)| ((account, *scope), ivk)) .map(|(tag, ivk)| (tag, PreparedIncomingViewingKey::new(ivk))), @@ -651,7 +623,7 @@ mod tests { let txs = scan_block_with_runner( &Network::TestNetwork, cb, - &[(&AccountId::from(0), &extfvk)], + &[(&AccountId::from(0), &dfvk)], &[], &mut tree, &mut [], @@ -680,11 +652,11 @@ mod tests { #[test] fn scan_block_with_my_spend() { let extsk = ExtendedSpendingKey::master(&[]); - let extfvk = ExtendedFullViewingKey::from(&extsk); + let dfvk = extsk.to_diversifiable_full_viewing_key(); let nf = Nullifier([7; 32]); let account = AccountId::from(12); - let cb = fake_compact_block(1u32.into(), nf, extfvk, Amount::from_u64(5).unwrap(), false); + let cb = fake_compact_block(1u32.into(), nf, &dfvk, Amount::from_u64(5).unwrap(), false); assert_eq!(cb.vtx.len(), 2); let vks: Vec<(&AccountId, &SaplingIvk)> = vec![]; diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index ec06bddfd..57363b4e8 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -933,7 +933,7 @@ mod tests { PaymentAddress, }, transaction::components::Amount, - zip32::sapling::{DiversifiableFullViewingKey, ExtendedFullViewingKey}, + zip32::sapling::DiversifiableFullViewingKey, }; use zcash_client_backend::{ @@ -990,7 +990,7 @@ mod tests { let seed = [0u8; 32]; let account = AccountId::from(0); let extsk = sapling::spending_key(&seed, network().coin_type(), account); - let dfvk = DiversifiableFullViewingKey::from(ExtendedFullViewingKey::from(&extsk)); + let dfvk = extsk.to_diversifiable_full_viewing_key(); #[cfg(feature = "transparent-inputs")] let (tkey, taddr) = { diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index f1167cd3f..125488e7f 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -175,7 +175,7 @@ fn init_wallet_db_internal( /// /// use zcash_primitives::{ /// consensus::{Network, Parameters}, -/// zip32::{AccountId, ExtendedFullViewingKey, ExtendedSpendingKey} +/// zip32::{AccountId, ExtendedSpendingKey} /// }; /// /// use zcash_client_backend::{ @@ -197,7 +197,7 @@ fn init_wallet_db_internal( /// let seed = [0u8; 32]; // insecure; replace with a strong random seed /// let account = AccountId::from(0); /// let extsk = sapling::spending_key(&seed, Network::TestNetwork.coin_type(), account); -/// let dfvk = ExtendedFullViewingKey::from(&extsk).into(); +/// let dfvk = extsk.to_diversifiable_full_viewing_key(); /// let ufvk = UnifiedFullViewingKey::new(None, Some(dfvk), None).unwrap(); /// let ufvks = HashMap::from([(account, ufvk)]); /// init_accounts_table(&db_data, &ufvks).unwrap(); @@ -309,7 +309,7 @@ mod tests { block::BlockHash, consensus::{BlockHeight, BranchId, Parameters}, transaction::{TransactionData, TxVersion}, - zip32::sapling::{DiversifiableFullViewingKey, ExtendedFullViewingKey}, + zip32::sapling::ExtendedFullViewingKey, }; use crate::{ @@ -693,7 +693,7 @@ mod tests { let seed = [0xab; 32]; let account = AccountId::from(0); let secret_key = sapling::spending_key(&seed, tests::network().coin_type(), account); - let extfvk = ExtendedFullViewingKey::from(&secret_key); + let extfvk = secret_key.to_extended_full_viewing_key(); let data_file = NamedTempFile::new().unwrap(); let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap(); init_0_3_0(&mut db_data, &extfvk, account).unwrap(); @@ -856,7 +856,7 @@ mod tests { let seed = [0xab; 32]; let account = AccountId::from(0); let secret_key = sapling::spending_key(&seed, tests::network().coin_type(), account); - let extfvk = ExtendedFullViewingKey::from(&secret_key); + let extfvk = secret_key.to_extended_full_viewing_key(); let data_file = NamedTempFile::new().unwrap(); let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap(); init_autoshielding(&db_data, &extfvk, account).unwrap(); @@ -1025,7 +1025,7 @@ mod tests { // First call with data should initialise the accounts table let extsk = sapling::spending_key(&seed, network().coin_type(), account); - let dfvk = DiversifiableFullViewingKey::from(ExtendedFullViewingKey::from(&extsk)); + let dfvk = extsk.to_diversifiable_full_viewing_key(); #[cfg(feature = "transparent-inputs")] let ufvk = UnifiedFullViewingKey::new( diff --git a/zcash_client_sqlite/src/wallet/transact.rs b/zcash_client_sqlite/src/wallet/transact.rs index b57fd6d3e..923bf14f0 100644 --- a/zcash_client_sqlite/src/wallet/transact.rs +++ b/zcash_client_sqlite/src/wallet/transact.rs @@ -167,7 +167,7 @@ mod tests { legacy::TransparentAddress, sapling::{note_encryption::try_sapling_output_recovery, prover::TxProver}, transaction::{components::Amount, Transaction}, - zip32::sapling::{ExtendedFullViewingKey, ExtendedSpendingKey}, + zip32::sapling::ExtendedSpendingKey, }; use zcash_client_backend::{ @@ -525,7 +525,7 @@ mod tests { let (cb, _) = fake_compact_block( sapling_activation_height() + i, cb.hash(), - &ExtendedFullViewingKey::from(&ExtendedSpendingKey::master(&[i as u8])).into(), + &ExtendedSpendingKey::master(&[i as u8]).to_diversifiable_full_viewing_key(), value, ); insert_into_cache(&db_cache, &cb); @@ -558,7 +558,7 @@ mod tests { let (cb, _) = fake_compact_block( sapling_activation_height() + 22, cb.hash(), - &ExtendedFullViewingKey::from(&ExtendedSpendingKey::master(&[22])).into(), + &ExtendedSpendingKey::master(&[22]).to_diversifiable_full_viewing_key(), value, ); insert_into_cache(&db_cache, &cb); @@ -674,7 +674,7 @@ mod tests { let (cb, _) = fake_compact_block( sapling_activation_height() + i, cb.hash(), - &ExtendedFullViewingKey::from(&ExtendedSpendingKey::master(&[i as u8])).into(), + &ExtendedSpendingKey::master(&[i as u8]).to_diversifiable_full_viewing_key(), value, ); insert_into_cache(&db_cache, &cb); diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 1f6593f10..f4a9a110f 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -50,6 +50,8 @@ and this library adheres to Rust's notion of - `Error::NoChangeAddress` - `zcash_primitives::transaction::components::sapling::builder::SaplingBuilder::get_candidate_change_address` has been removed; change outputs must now be added by the caller. +- The `From<&ExtendedSpendingKey>` instance for `ExtendedFullViewingKey` has been + removed. Use `ExtendedSpendingKey::to_diversifiable_full_viewing_key` instead. ## [0.8.1] - 2022-10-19 ### Added diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index ccaf7c1ca..a6e0fe1ae 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -558,7 +558,7 @@ mod tests { sapling::builder::{self as build_s}, transparent::builder::{self as build_t}, }, - zip32::{ExtendedFullViewingKey, ExtendedSpendingKey}, + zip32::ExtendedSpendingKey, }; use super::{Builder, Error}; @@ -583,9 +583,9 @@ mod tests { #[test] fn fails_on_negative_output() { let extsk = ExtendedSpendingKey::master(&[]); - let extfvk = ExtendedFullViewingKey::from(&extsk); - let ovk = extfvk.fvk.ovk; - let to = extfvk.default_address().1; + let dfvk = extsk.to_diversifiable_full_viewing_key(); + let ovk = dfvk.fvk().ovk; + let to = dfvk.default_address().1; let sapling_activation_height = TEST_NETWORK .activation_height(NetworkUpgrade::Sapling) @@ -665,8 +665,8 @@ mod tests { #[test] fn binding_sig_present_if_shielded_spend() { let extsk = ExtendedSpendingKey::master(&[]); - let extfvk = ExtendedFullViewingKey::from(&extsk); - let to = extfvk.default_address().1; + let dfvk = extsk.to_diversifiable_full_viewing_key(); + let to = dfvk.default_address().1; let mut rng = OsRng; @@ -738,9 +738,9 @@ mod tests { ); } - let extfvk = ExtendedFullViewingKey::from(&extsk); - let ovk = Some(extfvk.fvk.ovk); - let to = extfvk.default_address().1; + let dfvk = extsk.to_diversifiable_full_viewing_key(); + let ovk = Some(dfvk.fvk().ovk); + let to = dfvk.default_address().1; // Fail if there is only a Sapling output // 0.0005 z-ZEC out, 0.00001 t-ZEC fee diff --git a/zcash_primitives/src/zip32/sapling.rs b/zcash_primitives/src/zip32/sapling.rs index e2b9dde81..c286249a2 100644 --- a/zcash_primitives/src/zip32/sapling.rs +++ b/zcash_primitives/src/zip32/sapling.rs @@ -417,7 +417,7 @@ impl ExtendedSpendingKey { /// Returns the address with the lowest valid diversifier index, along with /// the diversifier index that generated that address. pub fn default_address(&self) -> (DiversifierIndex, PaymentAddress) { - ExtendedFullViewingKey::from(self).default_address() + self.to_diversifiable_full_viewing_key().default_address() } /// Derives an internal spending key given an external spending key. @@ -456,15 +456,22 @@ impl ExtendedSpendingKey { } } + #[deprecated(note = "Use `to_diversifiable_full_viewing_key` instead.")] pub fn to_extended_full_viewing_key(&self) -> ExtendedFullViewingKey { - self.into() + ExtendedFullViewingKey { + depth: self.depth, + parent_fvk_tag: self.parent_fvk_tag, + child_index: self.child_index, + chain_code: self.chain_code, + fvk: FullViewingKey::from_expanded_spending_key(&self.expsk), + dk: self.dk, + } } pub fn to_diversifiable_full_viewing_key(&self) -> DiversifiableFullViewingKey { - let extfvk = self.to_extended_full_viewing_key(); DiversifiableFullViewingKey { - fvk: extfvk.fvk, - dk: extfvk.dk, + fvk: FullViewingKey::from_expanded_spending_key(&self.expsk), + dk: self.dk, } } } @@ -503,19 +510,6 @@ impl std::fmt::Debug for ExtendedFullViewingKey { } } -impl<'a> From<&'a ExtendedSpendingKey> for ExtendedFullViewingKey { - fn from(xsk: &ExtendedSpendingKey) -> Self { - ExtendedFullViewingKey { - depth: xsk.depth, - parent_fvk_tag: xsk.parent_fvk_tag, - child_index: xsk.child_index, - chain_code: xsk.chain_code, - fvk: FullViewingKey::from_expanded_spending_key(&xsk.expsk), - dk: xsk.dk, - } - } -} - impl ExtendedFullViewingKey { pub fn read(mut reader: R) -> io::Result { let depth = reader.read_u8()?; @@ -798,24 +792,26 @@ mod tests { use group::GroupEncoding; #[test] + #[allow(deprecated)] fn derive_nonhardened_child() { let seed = [0; 32]; let xsk_m = ExtendedSpendingKey::master(&seed); - let xfvk_m = ExtendedFullViewingKey::from(&xsk_m); + let xfvk_m = xsk_m.to_extended_full_viewing_key(); let i_5 = ChildIndex::NonHardened(5); let xsk_5 = xsk_m.derive_child(i_5); let xfvk_5 = xfvk_m.derive_child(i_5); assert!(xfvk_5.is_ok()); - assert_eq!(ExtendedFullViewingKey::from(&xsk_5), xfvk_5.unwrap()); + assert_eq!(xsk_5.to_extended_full_viewing_key(), xfvk_5.unwrap()); } #[test] + #[allow(deprecated)] fn derive_hardened_child() { let seed = [0; 32]; let xsk_m = ExtendedSpendingKey::master(&seed); - let xfvk_m = ExtendedFullViewingKey::from(&xsk_m); + let xfvk_m = xsk_m.to_extended_full_viewing_key(); let i_5h = ChildIndex::Hardened(5); let xsk_5h = xsk_m.derive_child(i_5h); @@ -823,7 +819,7 @@ mod tests { // Cannot derive a hardened child from an ExtendedFullViewingKey assert!(xfvk_5h.is_err()); - let xfvk_5h = ExtendedFullViewingKey::from(&xsk_5h); + let xfvk_5h = xsk_5h.to_extended_full_viewing_key(); let i_7 = ChildIndex::NonHardened(7); let xsk_5h_7 = xsk_5h.derive_child(i_7); @@ -831,7 +827,7 @@ mod tests { // But we *can* derive a non-hardened child from a hardened parent assert!(xfvk_5h_7.is_ok()); - assert_eq!(ExtendedFullViewingKey::from(&xsk_5h_7), xfvk_5h_7.unwrap()); + assert_eq!(xsk_5h_7.to_extended_full_viewing_key(), xfvk_5h_7.unwrap()); } #[test] @@ -933,7 +929,8 @@ mod tests { fn dfvk_round_trip() { let dfvk = { let extsk = ExtendedSpendingKey::master(&[]); - let extfvk = ExtendedFullViewingKey::from(&extsk); + #[allow(deprecated)] + let extfvk = extsk.to_extended_full_viewing_key(); DiversifiableFullViewingKey::from(extfvk) }; @@ -953,7 +950,7 @@ mod tests { fn address() { let seed = [0; 32]; let xsk_m = ExtendedSpendingKey::master(&seed); - let xfvk_m = ExtendedFullViewingKey::from(&xsk_m); + let xfvk_m = xsk_m.to_diversifiable_full_viewing_key(); let j_0 = DiversifierIndex::new(); let addr_m = xfvk_m.address(j_0).unwrap(); assert_eq!( @@ -980,10 +977,11 @@ mod tests { } #[test] + #[allow(deprecated)] fn read_write() { let seed = [0; 32]; let xsk = ExtendedSpendingKey::master(&seed); - let fvk = ExtendedFullViewingKey::from(&xsk); + let fvk = xsk.to_extended_full_viewing_key(); let mut ser = vec![]; xsk.write(&mut ser).unwrap(); @@ -997,6 +995,7 @@ mod tests { } #[test] + #[allow(deprecated)] fn test_vectors() { struct TestVector { ask: Option<[u8; 32]>, @@ -1689,13 +1688,13 @@ mod tests { let m = ExtendedSpendingKey::master(&seed); let m_1 = m.derive_child(i1); let m_1_2h = ExtendedSpendingKey::from_path(&m, &[i1, i2h]); - let m_1_2hv = ExtendedFullViewingKey::from(&m_1_2h); + let m_1_2hv = m_1_2h.to_extended_full_viewing_key(); let m_1_2hv_3 = m_1_2hv.derive_child(i3).unwrap(); let xfvks = [ - ExtendedFullViewingKey::from(&m), - ExtendedFullViewingKey::from(&m_1), - ExtendedFullViewingKey::from(&m_1_2h), + m.to_extended_full_viewing_key(), + m_1.to_extended_full_viewing_key(), + m_1_2h.to_extended_full_viewing_key(), m_1_2hv, // Appears twice so we can de-duplicate test code below m_1_2hv_3, ]; From 1be86b7a544042f2b15c2b3810cfc64c2d0b0f72 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 31 Oct 2022 15:09:35 -0600 Subject: [PATCH 2/4] Derive the correct note when spending from a change address. --- zcash_client_backend/CHANGELOG.md | 3 ++ zcash_client_backend/src/data_api/error.rs | 6 ++++ zcash_client_backend/src/data_api/wallet.rs | 33 ++++++++++++++------- zcash_primitives/CHANGELOG.md | 4 +++ zcash_primitives/src/sapling.rs | 6 ++++ zcash_primitives/src/zip32/sapling.rs | 19 ++++++++++++ 6 files changed, 60 insertions(+), 11 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 65bd02a8b..d255a165e 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -131,6 +131,9 @@ and this library adheres to Rust's notion of with this wallet. - `Error::ChildIndexOutOfRange` to indicate that a diversifier index for an address is out of range for valid transparent child indices. + - `Error::NoteMismatch` to indicate that a note being spent is not associated + with either the internal or external full viewing keys corresponding to the + provided spending key. - `zcash_client_backend::decrypt`: - `decrypt_transaction` now takes a `HashMap<_, UnifiedFullViewingKey>` instead of `HashMap<_, ExtendedFullViewingKey>`. diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index 8186a33e7..efb5da5c3 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -86,6 +86,11 @@ pub enum Error { /// identifier. KeyDerivationError(AccountId), + /// A note being spent does not correspond to either the internal or external + /// full viewing key for an account. + // TODO: Return the note id for the note that caused the failure + NoteMismatch, + /// An error indicating that a call was attempted to a method providing /// support #[cfg(not(feature = "transparent-inputs"))] @@ -151,6 +156,7 @@ impl fmt::Display for Error { Error::SaplingNotActive => write!(f, "Could not determine Sapling upgrade activation height."), Error::MemoForbidden => write!(f, "It is not possible to send a memo to a transparent address."), Error::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id), + Error::NoteMismatch => write!(f, "A note being spent does not correspond to either the internal or external full viewing key for the provided spending key."), #[cfg(not(feature = "transparent-inputs"))] Error::TransparentInputsNotSupported => { diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index e765c985c..f8b968242 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -2,7 +2,7 @@ use std::fmt::Debug; use zcash_primitives::{ consensus::{self, NetworkUpgrade}, memo::MemoBytes, - sapling::prover::TxProver, + sapling::{prover::TxProver, Node}, transaction::{ builder::Builder, components::amount::{Amount, BalanceError, DEFAULT_FEE}, @@ -307,18 +307,29 @@ where // Create the transaction let mut builder = Builder::new(params.clone(), target_height); for selected in spendable_notes { - let from = dfvk - .fvk() - .vk - .to_payment_address(selected.diversifier) - .unwrap(); //DiversifyHash would have to unexpectedly return the zero point for this to be None - - let note = from - .create_note(selected.note_value.into(), selected.rseed) - .unwrap(); - let merkle_path = selected.witness.path().expect("the tree is not empty"); + // Attempt to reconstruct the note being spent using both the internal and external dfvks + // corresponding to the unified spending key, checking against the witness we are using + // to spend the note that we've used the correct key. + let note = { + let external_note = dfvk + .diversified_address(selected.diversifier) + .and_then(|addr| addr.create_note(selected.note_value.into(), selected.rseed)); + let internal_note = dfvk + .diversified_change_address(selected.diversifier) + .and_then(|addr| addr.create_note(selected.note_value.into(), selected.rseed)); + + let expected_root = selected.witness.root(); + external_note + .filter(|n| expected_root == merkle_path.root(Node::from_scalar(n.cmu()))) + .or_else(|| { + internal_note + .filter(|n| expected_root == merkle_path.root(Node::from_scalar(n.cmu()))) + }) + .ok_or_else(|| E::from(Error::NoteMismatch)) + }?; + builder .add_sapling_spend( usk.sapling().clone(), diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index f4a9a110f..202795533 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -39,6 +39,10 @@ and this library adheres to Rust's notion of longer fixes the fee for transactions to 0.00001 ZEC; the builder instead computes the fee using a `FeeRule` implementation at build time. +### Deprecated +- `zcash_primitives::zip32::sapling::to_extended_full_viewing_key` Use + `to_diversifiable_full_viewing_key` instead. + ### Removed - Removed from `zcash_primitives::transaction::builder::Builder` - `Builder::{new_with_fee, new_with_rng_and_fee`} (use `Builder::{new, new_with_rng}` diff --git a/zcash_primitives/src/sapling.rs b/zcash_primitives/src/sapling.rs index f4d33e4db..258ac71cd 100644 --- a/zcash_primitives/src/sapling.rs +++ b/zcash_primitives/src/sapling.rs @@ -80,6 +80,12 @@ impl Node { pub fn new(repr: [u8; 32]) -> Self { Node { repr } } + + pub fn from_scalar(cmu: bls12_381::Scalar) -> Self { + Self { + repr: cmu.to_repr(), + } + } } impl incrementalmerkletree::Hashable for Node { diff --git a/zcash_primitives/src/zip32/sapling.rs b/zcash_primitives/src/zip32/sapling.rs index c286249a2..2ce812759 100644 --- a/zcash_primitives/src/zip32/sapling.rs +++ b/zcash_primitives/src/zip32/sapling.rs @@ -746,6 +746,14 @@ impl DiversifiableFullViewingKey { sapling_default_address(&self.fvk, &self.dk) } + /// Returns the payment address corresponding to the specified diversifier, if any. + /// + /// In general, it is preferable to use `find_address` instead, but this method is + /// useful in some cases for matching keys to existing payment addresses. + pub fn diversified_address(&self, diversifier: Diversifier) -> Option { + self.fvk.vk.to_payment_address(diversifier) + } + /// Returns the internal address corresponding to the smallest valid diversifier index, /// along with that index. /// @@ -756,6 +764,17 @@ impl DiversifiableFullViewingKey { sapling_default_address(&internal_dfvk.fvk, &internal_dfvk.dk) } + /// Returns the change address corresponding to the specified diversifier, if any. + /// + /// In general, it is preferable to use `change_address` instead, but this method is + /// useful in some cases for matching keys to existing payment addresses. + pub fn diversified_change_address(&self, diversifier: Diversifier) -> Option { + self.derive_internal() + .fvk + .vk + .to_payment_address(diversifier) + } + /// Attempts to decrypt the given address's diversifier with this full viewing key. /// /// This method extracts the diversifier from the given address and decrypts it as a From cdfca848eae42e7b3396da558859695cf36767c3 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 2 Nov 2022 09:33:28 -0600 Subject: [PATCH 3/4] Improve type safety of note commitment tree node construction. --- zcash_client_backend/CHANGELOG.md | 3 ++- zcash_client_backend/src/data_api/wallet.rs | 7 +++---- zcash_client_backend/src/encoding.rs | 2 +- zcash_client_backend/src/welding_rig.rs | 3 +-- zcash_extensions/src/transparent/demo.rs | 6 +++--- zcash_primitives/CHANGELOG.md | 2 ++ zcash_primitives/src/merkle_tree.rs | 6 +++--- zcash_primitives/src/sapling.rs | 16 +++++++++++++--- zcash_primitives/src/transaction/builder.rs | 10 +++++----- .../transaction/components/sapling/builder.rs | 6 +++--- 10 files changed, 36 insertions(+), 25 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index d255a165e..f68c6a16b 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -177,7 +177,8 @@ and this library adheres to Rust's notion of - Setters (use direct field access instead). - The hardcoded `data_api::wallet::ANCHOR_OFFSET` constant. - `zcash_client_backend::wallet::AccountId` (moved to `zcash_primitives::zip32::AccountId`). - +- The implementation of `welding_rig::ScanningKey` for `ExtendedFullViewingKey` + has been removed. Use `DiversifiableFullViewingKey` instead. ## [0.5.0] - 2021-03-26 ### Added diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index f8b968242..be344acb2 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -2,7 +2,7 @@ use std::fmt::Debug; use zcash_primitives::{ consensus::{self, NetworkUpgrade}, memo::MemoBytes, - sapling::{prover::TxProver, Node}, + sapling::prover::TxProver, transaction::{ builder::Builder, components::amount::{Amount, BalanceError, DEFAULT_FEE}, @@ -322,10 +322,9 @@ where let expected_root = selected.witness.root(); external_note - .filter(|n| expected_root == merkle_path.root(Node::from_scalar(n.cmu()))) + .filter(|n| expected_root == merkle_path.root(n.commitment())) .or_else(|| { - internal_note - .filter(|n| expected_root == merkle_path.root(Node::from_scalar(n.cmu()))) + internal_note.filter(|n| expected_root == merkle_path.root(n.commitment())) }) .ok_or_else(|| E::from(Error::NoteMismatch)) }?; diff --git a/zcash_client_backend/src/encoding.rs b/zcash_client_backend/src/encoding.rs index 053f6d85e..63c3becae 100644 --- a/zcash_client_backend/src/encoding.rs +++ b/zcash_client_backend/src/encoding.rs @@ -217,7 +217,7 @@ pub fn decode_extended_spending_key( /// use zcash_primitives::zip32::ExtendedFullViewingKey; /// /// let extsk = sapling::spending_key(&[0; 32][..], COIN_TYPE, AccountId::from(0)); -/// let extfvk = ExtendedFullViewingKey::from(&extsk); +/// let extfvk = extsk.to_extended_full_viewing_key(); /// let encoded = encode_extended_full_viewing_key(HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY, &extfvk); /// ``` /// [`ExtendedFullViewingKey`]: zcash_primitives::zip32::ExtendedFullViewingKey diff --git a/zcash_client_backend/src/welding_rig.rs b/zcash_client_backend/src/welding_rig.rs index 0a0b9fa72..f3cdf1083 100644 --- a/zcash_client_backend/src/welding_rig.rs +++ b/zcash_client_backend/src/welding_rig.rs @@ -3,7 +3,6 @@ use std::collections::{HashMap, HashSet}; use std::convert::TryFrom; -use group::ff::PrimeField; use subtle::{ConditionallySelectable, ConstantTimeEq, CtOption}; use zcash_note_encryption::batch; use zcash_primitives::{ @@ -333,7 +332,7 @@ pub(crate) fn scan_block_with_runner< .collect(); // Increment tree and witnesses - let node = Node::new(output.cmu.to_repr()); + let node = Node::from_scalar(output.cmu); for witness in &mut *existing_witnesses { witness.append(node).unwrap(); } diff --git a/zcash_extensions/src/transparent/demo.rs b/zcash_extensions/src/transparent/demo.rs index 79e0aafbd..764f4d408 100644 --- a/zcash_extensions/src/transparent/demo.rs +++ b/zcash_extensions/src/transparent/demo.rs @@ -477,7 +477,7 @@ impl<'a, B: ExtensionTxBuilder<'a>> DemoBuilder { #[cfg(test)] mod tests { use blake2b_simd::Params; - use ff::{Field, PrimeField}; + use ff::Field; use rand_core::OsRng; use zcash_proofs::prover::LocalTxProver; @@ -488,7 +488,7 @@ mod tests { extensions::transparent::{self as tze, Extension, FromPayload, ToPayload}, legacy::TransparentAddress, merkle_tree::{CommitmentTree, IncrementalWitness}, - sapling::{Node, Rseed}, + sapling::Rseed, transaction::{ builder::Builder, components::{ @@ -817,7 +817,7 @@ mod tests { let note1 = to .create_note(101000, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng))) .unwrap(); - let cm1 = Node::new(note1.cmu().to_repr()); + let cm1 = note1.commitment(); let mut tree = CommitmentTree::empty(); // fake that the note appears in some previous // shielded output diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 202795533..db8c1df90 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -56,6 +56,8 @@ and this library adheres to Rust's notion of has been removed; change outputs must now be added by the caller. - The `From<&ExtendedSpendingKey>` instance for `ExtendedFullViewingKey` has been removed. Use `ExtendedSpendingKey::to_diversifiable_full_viewing_key` instead. +- `zcash_primitives::sapling::Node::new` has been removed. Use + `Node::from_scalar` or (preferably) `Note::commitment` instead. ## [0.8.1] - 2022-10-19 ### Added diff --git a/zcash_primitives/src/merkle_tree.rs b/zcash_primitives/src/merkle_tree.rs index 512f6a0c1..5e638c1f5 100644 --- a/zcash_primitives/src/merkle_tree.rs +++ b/zcash_primitives/src/merkle_tree.rs @@ -323,13 +323,13 @@ impl CommitmentTree { /// /// let mut tree = CommitmentTree::::empty(); /// -/// tree.append(Node::new(bls12_381::Scalar::random(&mut rng).to_repr())); -/// tree.append(Node::new(bls12_381::Scalar::random(&mut rng).to_repr())); +/// tree.append(Node::from_scalar(bls12_381::Scalar::random(&mut rng))); +/// tree.append(Node::from_scalar(bls12_381::Scalar::random(&mut rng))); /// let mut witness = IncrementalWitness::from_tree(&tree); /// assert_eq!(witness.position(), 1); /// assert_eq!(tree.root(), witness.root()); /// -/// let cmu = Node::new(bls12_381::Scalar::random(&mut rng).to_repr()); +/// let cmu = Node::from_scalar(bls12_381::Scalar::random(&mut rng)); /// tree.append(cmu); /// witness.append(cmu); /// assert_eq!(tree.root(), witness.root()); diff --git a/zcash_primitives/src/sapling.rs b/zcash_primitives/src/sapling.rs index 258ac71cd..33225b61f 100644 --- a/zcash_primitives/src/sapling.rs +++ b/zcash_primitives/src/sapling.rs @@ -77,10 +77,12 @@ pub struct Node { } impl Node { - pub fn new(repr: [u8; 32]) -> Self { + #[cfg(test)] + pub(crate) fn new(repr: [u8; 32]) -> Self { Node { repr } } + /// Constructs a new note commitment tree node from a [`bls12_381::Scalar`] pub fn from_scalar(cmu: bls12_381::Scalar) -> Self { Self { repr: cmu.to_repr(), @@ -110,7 +112,7 @@ impl HashSer for Node { fn read(mut reader: R) -> io::Result { let mut repr = [0u8; 32]; reader.read_exact(&mut repr)?; - Ok(Node::new(repr)) + Ok(Node { repr }) } fn write(&self, mut writer: W) -> io::Result<()> { @@ -523,6 +525,12 @@ impl Note { )), } } + + pub fn commitment(&self) -> Node { + Node { + repr: self.cmu().to_repr(), + } + } } #[cfg(any(test, feature = "test-dependencies"))] @@ -569,7 +577,9 @@ pub mod testing { prop_compose! { pub fn arb_node()(value in prop::array::uniform32(prop::num::u8::ANY)) -> Node { - Node::new(value) + Node { + repr: value + } } } diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index a6e0fe1ae..91dce2122 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -544,7 +544,7 @@ mod testing { #[cfg(test)] mod tests { - use ff::{Field, PrimeField}; + use ff::Field; use rand_core::OsRng; use crate::{ @@ -552,7 +552,7 @@ mod tests { legacy::TransparentAddress, memo::MemoBytes, merkle_tree::{CommitmentTree, IncrementalWitness}, - sapling::{Node, Rseed}, + sapling::Rseed, transaction::components::{ amount::{Amount, DEFAULT_FEE}, sapling::builder::{self as build_s}, @@ -673,7 +673,7 @@ mod tests { let note1 = to .create_note(50000, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng))) .unwrap(); - let cmu1 = Node::new(note1.cmu().to_repr()); + let cmu1 = note1.commitment(); let mut tree = CommitmentTree::empty(); tree.append(cmu1).unwrap(); let witness1 = IncrementalWitness::from_tree(&tree); @@ -783,7 +783,7 @@ mod tests { let note1 = to .create_note(50999, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng))) .unwrap(); - let cmu1 = Node::new(note1.cmu().to_repr()); + let cmu1 = note1.commitment(); let mut tree = CommitmentTree::empty(); tree.append(cmu1).unwrap(); let mut witness1 = IncrementalWitness::from_tree(&tree); @@ -823,7 +823,7 @@ mod tests { let note2 = to .create_note(1, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng))) .unwrap(); - let cmu2 = Node::new(note2.cmu().to_repr()); + let cmu2 = note2.commitment(); tree.append(cmu2).unwrap(); witness1.append(cmu2).unwrap(); let witness2 = IncrementalWitness::from_tree(&tree); diff --git a/zcash_primitives/src/transaction/components/sapling/builder.rs b/zcash_primitives/src/transaction/components/sapling/builder.rs index 358999439..9351af856 100644 --- a/zcash_primitives/src/transaction/components/sapling/builder.rs +++ b/zcash_primitives/src/transaction/components/sapling/builder.rs @@ -272,14 +272,14 @@ impl SaplingBuilder

{ merkle_path: MerklePath, ) -> Result<(), Error> { // Consistency check: all anchors must equal the first one - let cmu = Node::new(note.cmu().into()); + let node = note.commitment(); if let Some(anchor) = self.anchor { - let path_root: bls12_381::Scalar = merkle_path.root(cmu).into(); + let path_root: bls12_381::Scalar = merkle_path.root(node).into(); if path_root != anchor { return Err(Error::AnchorMismatch); } } else { - self.anchor = Some(merkle_path.root(cmu).into()) + self.anchor = Some(merkle_path.root(node).into()) } let alpha = jubjub::Fr::random(&mut rng); From 981d45e9661445ecaef757c497d66af1f249ef3c Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 3 Nov 2022 19:27:17 -0600 Subject: [PATCH 4/4] Documentation & changelog fixes. --- zcash_primitives/CHANGELOG.md | 3 +++ zcash_primitives/src/sapling.rs | 2 ++ 2 files changed, 5 insertions(+) diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index db8c1df90..903bee6e1 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -30,6 +30,9 @@ and this library adheres to Rust's notion of - Added to `zcash_primitives::transaction::components::transparent::builder` - `TransparentBuilder::{inputs, outputs}`: accessors for transparent builder state. - `zcash_primitives::transaction::components::transparent::fees` +- `zcash_primitives::sapling::Note::commitment` +- Added to `zcash_primitives::zip32::sapling::DiversifiableFullViewingKey` + - `DiversifiableFullViewingKey::{diversified_address, diversified_change_address}` ### Changed - `zcash_primitives::transaction::builder::Builder::build` now takes a `FeeRule` diff --git a/zcash_primitives/src/sapling.rs b/zcash_primitives/src/sapling.rs index 33225b61f..30a1afc4f 100644 --- a/zcash_primitives/src/sapling.rs +++ b/zcash_primitives/src/sapling.rs @@ -526,6 +526,8 @@ impl Note { } } + /// Returns [`self.cmu`] in the correct representation for inclusion in the Sapling + /// note commitment tree. pub fn commitment(&self) -> Node { Node { repr: self.cmu().to_repr(),