Make pk_d validity an invariant of PaymentAddress

Introduces a PaymentAddress::from_parts constructor, and getters for
the diversifier and pk_d fields (which are now private).

librustzcash/src/tests/key_agreement.rs

@@ -47,7 +47,7 @@ fn test_key_agreement() {
 
     // Serialize pk_d for the call to librustzcash_sapling_ka_agree
     let mut addr_pk_d = [0u8; 32];
-    addr.pk_d.write(&mut addr_pk_d[..]).unwrap();
+    addr.pk_d().write(&mut addr_pk_d[..]).unwrap();
 
     assert!(librustzcash_sapling_ka_agree(
         &addr_pk_d,
@@ -59,7 +59,7 @@ fn test_key_agreement() {
     // using the diversifier and esk.
     let mut epk = [0u8; 32];
     assert!(librustzcash_sapling_ka_derivepublic(
-        &addr.diversifier.0,
+        &addr.diversifier().0,
         &esk,
         &mut epk
     ));

librustzcash/src/tests/key_components.rs

@@ -707,7 +707,7 @@ fn key_components() {
         let addr = fvk.to_payment_address(diversifier, &JUBJUB).unwrap();
         {
             let mut vec = Vec::new();
-            addr.pk_d.write(&mut vec).unwrap();
+            addr.pk_d().write(&mut vec).unwrap();
             assert_eq!(&vec, &tv.default_pk_d);
         }
         {

zcash_client_backend/src/encoding.rs

@@ -110,10 +110,11 @@ pub fn decode_extended_full_viewing_key(
 ///     0xbc, 0xe5,
 /// ]);
 ///
-/// let pa = PaymentAddress {
+/// let pa = PaymentAddress::from_parts(
-///     diversifier: Diversifier([0u8; 11]),
+///     Diversifier([0u8; 11]),
-///     pk_d: edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
+///     edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
-/// };
+/// )
+/// .unwrap();
 ///
 /// assert_eq!(
 ///     encode_payment_address(HRP_SAPLING_PAYMENT_ADDRESS, &pa),
@@ -147,10 +148,11 @@ pub fn encode_payment_address(hrp: &str, addr: &PaymentAddress<Bls12>) -> String
 ///     0xbc, 0xe5,
 /// ]);
 ///
-/// let pa = PaymentAddress {
+/// let pa = PaymentAddress::from_parts(
-///     diversifier: Diversifier([0u8; 11]),
+///     Diversifier([0u8; 11]),
-///     pk_d: edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
+///     edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
-/// };
+/// )
+/// .unwrap();
 ///
 /// assert_eq!(
 ///     decode_payment_address(
@@ -193,10 +195,11 @@ mod tests {
             0xbc, 0xe5,
         ]);
 
-        let addr = PaymentAddress {
+        let addr = PaymentAddress::from_parts(
-            diversifier: Diversifier([0u8; 11]),
+            Diversifier([0u8; 11]),
-            pk_d: edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
+            edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
-        };
+        )
+        .unwrap();
 
         let encoded_main =
             "zs1qqqqqqqqqqqqqqqqqrjq05nyfku05msvu49mawhg6kr0wwljahypwyk2h88z6975u563j8nfaxd";
@@ -237,10 +240,11 @@ mod tests {
             0xbc, 0xe5,
         ]);
 
-        let addr = PaymentAddress {
+        let addr = PaymentAddress::from_parts(
-            diversifier: Diversifier([1u8; 11]),
+            Diversifier([1u8; 11]),
-            pk_d: edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
+            edwards::Point::<Bls12, _>::rand(rng, &JUBJUB).mul_by_cofactor(&JUBJUB),
-        };
+        )
+        .unwrap();
 
         let encoded_main =
             encode_payment_address(constants::mainnet::HRP_SAPLING_PAYMENT_ADDRESS, &addr);

zcash_primitives/src/note_encryption.rs

@@ -232,10 +232,7 @@ fn prf_ock(
 ///
 /// let diversifier = Diversifier([0; 11]);
 /// let pk_d = diversifier.g_d::<Bls12>(&JUBJUB).unwrap();
-/// let to = PaymentAddress {
+/// let to = PaymentAddress::from_parts(diversifier, pk_d).unwrap();
-///     pk_d,
-///     diversifier,
-/// };
 /// let ovk = OutgoingViewingKey([0; 32]);
 ///
 /// let value = 1000;
@@ -294,14 +291,14 @@ impl SaplingNoteEncryption {
 
     /// Generates `encCiphertext` for this note.
     pub fn encrypt_note_plaintext(&self) -> [u8; ENC_CIPHERTEXT_SIZE] {
-        let shared_secret = sapling_ka_agree(&self.esk, &self.to.pk_d);
+        let shared_secret = sapling_ka_agree(&self.esk, self.to.pk_d());
         let key = kdf_sapling(shared_secret, &self.epk);
 
         // Note plaintext encoding is defined in section 5.5 of the Zcash Protocol
         // Specification.
         let mut input = [0; NOTE_PLAINTEXT_SIZE];
         input[0] = 1;
-        input[1..12].copy_from_slice(&self.to.diversifier.0);
+        input[1..12].copy_from_slice(&self.to.diversifier().0);
         (&mut input[12..20])
             .write_u64::<LittleEndian>(self.note.value)
             .unwrap();
@@ -375,7 +372,7 @@ fn parse_note_plaintext_without_memo(
         .g_d::<Bls12>(&JUBJUB)?
         .mul(ivk.into_repr(), &JUBJUB);
 
-    let to = PaymentAddress { pk_d, diversifier };
+    let to = PaymentAddress::from_parts(diversifier, pk_d)?;
     let note = to.create_note(v, rcm, &JUBJUB).unwrap();
 
     if note.cm(&JUBJUB) != *cmu {
@@ -535,7 +532,7 @@ pub fn try_sapling_output_recovery(
         return None;
     }
 
-    let to = PaymentAddress { pk_d, diversifier };
+    let to = PaymentAddress::from_parts(diversifier, pk_d)?;
     let note = to.create_note(v, rcm, &JUBJUB).unwrap();
 
     if note.cm(&JUBJUB) != *cmu {
@@ -701,7 +698,7 @@ mod tests {
         let diversifier = Diversifier([0; 11]);
         let ivk = Fs::random(&mut rng);
         let pk_d = diversifier.g_d::<Bls12>(&JUBJUB).unwrap().mul(ivk, &JUBJUB);
-        let pa = PaymentAddress { diversifier, pk_d };
+        let pa = PaymentAddress::from_parts(diversifier, pk_d).unwrap();
 
         // Construct the value commitment for the proof instance
         let value = 100;
@@ -1317,10 +1314,7 @@ mod tests {
             let ock = prf_ock(&ovk, &cv, &cmu, &epk);
             assert_eq!(ock.as_bytes(), tv.ock);
 
-            let to = PaymentAddress {
+            let to = PaymentAddress::from_parts(Diversifier(tv.default_d), pk_d).unwrap();
-                pk_d,
-                diversifier: Diversifier(tv.default_d),
-            };
             let note = to.create_note(tv.v, rcm, &JUBJUB).unwrap();
             assert_eq!(note.cm(&JUBJUB), cmu);
 

zcash_primitives/src/primitives.rs

@@ -94,10 +94,10 @@ impl<E: JubjubEngine> ViewingKey<E> {
         diversifier: Diversifier,
         params: &E::Params,
     ) -> Option<PaymentAddress<E>> {
-        diversifier.g_d(params).map(|g_d| {
+        diversifier.g_d(params).and_then(|g_d| {
             let pk_d = g_d.mul(self.ivk(), params);
 
-            PaymentAddress { pk_d, diversifier }
+            PaymentAddress::from_parts(diversifier, pk_d)
         })
     }
 }
@@ -118,10 +118,16 @@ impl Diversifier {
     }
 }
 
+/// A Sapling payment address.
+///
+/// # Invariants
+///
+/// `pk_d` is guaranteed to be prime-order (i.e. in the prime-order subgroup of Jubjub,
+/// and not the identity).
 #[derive(Clone, Debug)]
 pub struct PaymentAddress<E: JubjubEngine> {
-    pub pk_d: edwards::Point<E, PrimeOrder>,
+    pk_d: edwards::Point<E, PrimeOrder>,
-    pub diversifier: Diversifier,
+    diversifier: Diversifier,
 }
 
 impl<E: JubjubEngine> PartialEq for PaymentAddress<E> {
@@ -131,6 +137,20 @@ impl<E: JubjubEngine> PartialEq for PaymentAddress<E> {
 }
 
 impl<E: JubjubEngine> PaymentAddress<E> {
+    /// Constructs a PaymentAddress from a diversifier and a Jubjub point.
+    ///
+    /// Returns None if `pk_d` is the identity.
+    pub fn from_parts(
+        diversifier: Diversifier,
+        pk_d: edwards::Point<E, PrimeOrder>,
+    ) -> Option<Self> {
+        if pk_d == edwards::Point::zero() {
+            None
+        } else {
+            Some(PaymentAddress { pk_d, diversifier })
+        }
+    }
+
     /// Parses a PaymentAddress from bytes.
     pub fn from_bytes(bytes: &[u8; 43], params: &E::Params) -> Option<Self> {
         let diversifier = {
@@ -146,13 +166,7 @@ impl<E: JubjubEngine> PaymentAddress<E> {
         edwards::Point::<E, _>::read(&bytes[11..43], params)
             .ok()?
             .as_prime_order(params)
-            .and_then(|pk_d| {
+            .and_then(|pk_d| PaymentAddress::from_parts(diversifier, pk_d))
-                if pk_d == edwards::Point::zero() {
-                    None
-                } else {
-                    Some(PaymentAddress { pk_d, diversifier })
-                }
-            })
     }
 
     /// Returns the byte encoding of this `PaymentAddress`.
@@ -163,6 +177,16 @@ impl<E: JubjubEngine> PaymentAddress<E> {
         bytes
     }
 
+    /// Returns the [`Diversifier`] for this `PaymentAddress`.
+    pub fn diversifier(&self) -> &Diversifier {
+        &self.diversifier
+    }
+
+    /// Returns `pk_d` for this `PaymentAddress`.
+    pub fn pk_d(&self) -> &edwards::Point<E, PrimeOrder> {
+        &self.pk_d
+    }
+
     pub fn g_d(&self, params: &E::Params) -> Option<edwards::Point<E, PrimeOrder>> {
         self.diversifier.g_d(params)
     }

zcash_primitives/src/transaction/builder.rs

@@ -77,7 +77,7 @@ impl SaplingOutput {
 
         let note = Note {
             g_d,
-            pk_d: to.pk_d.clone(),
+            pk_d: to.pk_d().clone(),
             value: value.into(),
             r: rcm,
         };
@@ -344,10 +344,11 @@ impl<R: RngCore + CryptoRng> Builder<R> {
             } else if !self.spends.is_empty() {
                 (
                     self.spends[0].extsk.expsk.ovk,
-                    PaymentAddress {
+                    PaymentAddress::from_parts(
-                        diversifier: self.spends[0].diversifier,
+                        self.spends[0].diversifier,
-                        pk_d: self.spends[0].note.pk_d.clone(),
+                        self.spends[0].note.pk_d.clone(),
-                    },
+                    )
+                    .ok_or(Error::InvalidAddress)?,
                 )
             } else {
                 return Err(Error::NoChangeAddress);
@@ -450,16 +451,16 @@ impl<R: RngCore + CryptoRng> Builder<R> {
                         (diversifier, g_d)
                     };
 
-                    let pk_d = {
+                    let (pk_d, payment_address) = loop {
                         let dummy_ivk = Fs::random(&mut self.rng);
-                        g_d.mul(dummy_ivk, &JUBJUB)
+                        let pk_d = g_d.mul(dummy_ivk, &JUBJUB);
+                        if let Some(addr) = PaymentAddress::from_parts(diversifier, pk_d.clone()) {
+                            break (pk_d, addr);
+                        }
                     };
 
                     (
-                        PaymentAddress {
+                        payment_address,
-                            diversifier,
-                            pk_d: pk_d.clone(),
-                        },
                         Note {
                             g_d,
                             pk_d,
@@ -644,7 +645,7 @@ mod tests {
             builder
                 .add_sapling_spend(
                     extsk.clone(),
-                    to.diversifier,
+                    *to.diversifier(),
                     note1.clone(),
                     witness1.clone(),
                 )
@@ -683,10 +684,10 @@ mod tests {
         {
             let mut builder = Builder::new(0);
             builder
-                .add_sapling_spend(extsk.clone(), to.diversifier, note1, witness1)
+                .add_sapling_spend(extsk.clone(), *to.diversifier(), note1, witness1)
                 .unwrap();
             builder
-                .add_sapling_spend(extsk, to.diversifier, note2, witness2)
+                .add_sapling_spend(extsk, *to.diversifier(), note2, witness2)
                 .unwrap();
             builder
                 .add_sapling_output(ovk, to, Amount::from_u64(30000).unwrap(), None)

zcash_primitives/src/zip32.rs

@@ -548,7 +548,7 @@ mod tests {
         let (j_m, addr_m) = xsk_m.default_address().unwrap();
         assert_eq!(j_m.0, [0; 11]);
         assert_eq!(
-            addr_m.diversifier.0,
+            addr_m.diversifier().0,
             // Computed using this Rust implementation
             [59, 246, 250, 31, 131, 191, 69, 99, 200, 167, 19]
         );

zcash_proofs/src/circuit/sapling.rs

@@ -464,7 +464,7 @@ impl<'a, E: JubjubEngine> Circuit<E> for Output<'a, E> {
         // they would like.
         {
             // Just grab pk_d from the witness
-            let pk_d = self.payment_address.as_ref().map(|e| e.pk_d.to_xy());
+            let pk_d = self.payment_address.as_ref().map(|e| e.pk_d().to_xy());
 
             // Witness the y-coordinate, encoded as little
             // endian bits (to match the representation)
@@ -584,7 +584,7 @@ fn test_input_circuit_with_bls12_381() {
             }
         }
 
-        let g_d = payment_address.diversifier.g_d(params).unwrap();
+        let g_d = payment_address.diversifier().g_d(params).unwrap();
         let commitment_randomness = fs::Fs::random(rng);
         let auth_path = vec![Some((Fr::random(rng), rng.next_u32() % 2 != 0)); tree_depth];
         let ar = fs::Fs::random(rng);
@@ -595,7 +595,7 @@ fn test_input_circuit_with_bls12_381() {
             let note = Note {
                 value: value_commitment.value,
                 g_d: g_d.clone(),
-                pk_d: payment_address.pk_d.clone(),
+                pk_d: payment_address.pk_d().clone(),
                 r: commitment_randomness.clone(),
             };
 

zcash_proofs/src/sapling/prover.rs

@@ -100,7 +100,7 @@ impl SaplingProvingContext {
             g_d: diversifier
                 .g_d::<Bls12>(params)
                 .expect("was a valid diversifier before"),
-            pk_d: payment_address.pk_d.clone(),
+            pk_d: payment_address.pk_d().clone(),
             r: rcm,
         };