Correctly construct outputs when spending internal notes.

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>`.
@@ -174,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

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 };

zcash_client_backend/src/data_api/error.rs

@@ -86,6 +86,11 @@ pub enum Error<NoteId> {
     /// 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<N: fmt::Display> fmt::Display for Error<N> {
             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 => {

zcash_client_backend/src/data_api/wallet.rs

@@ -307,18 +307,28 @@ 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(n.commitment()))
+                .or_else(|| {
+                    internal_note.filter(|n| expected_root == merkle_path.root(n.commitment()))
+                })
+                .ok_or_else(|| E::from(Error::NoteMismatch))
+        }?;
+
         builder
             .add_sapling_spend(
                 usk.sapling().clone(),

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
@@ -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";

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")]

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::{
@@ -15,10 +14,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 +94,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<Node>,
-    ) -> Self::Nf {
-        note.nf(key, witness.position() as u64)
-    }
-}
-
 /// The [`ScanningKey`] implementation for [`SaplingIvk`]s.
 /// Nullifiers cannot be derived when scanning with these keys.
 ///
@@ -359,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();
                 }
@@ -430,7 +403,7 @@ mod tests {
             Note, Nullifier, SaplingIvk,
         },
         transaction::components::Amount,
-        zip32::{AccountId, ExtendedFullViewingKey, ExtendedSpendingKey},
+        zip32::{AccountId, DiversifiableFullViewingKey, ExtendedSpendingKey},
     };
 
     use crate::{
@@ -480,11 +453,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 +469,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 +527,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 +542,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 +559,7 @@ mod tests {
             let txs = scan_block_with_runner(
                 &Network::TestNetwork,
                 cb,
-                &[(&account, &extfvk)],
+                &[(&account, &dfvk)],
                 &[],
                 &mut tree,
                 &mut [],
@@ -618,12 +590,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 +605,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 +622,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 +651,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![];
 

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) = {

zcash_client_sqlite/src/wallet/init.rs

@@ -175,7 +175,7 @@ fn init_wallet_db_internal<P: consensus::Parameters + 'static>(
 ///
 /// 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<P: consensus::Parameters + 'static>(
 /// 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(