Make address generation infallible again

DiversifyHash is altered to replace the identity with another fixed
point that is known to not be the identity.

src/keys.rs

@@ -7,7 +7,7 @@ use aes::Aes256;
 use fpe::ff1::{BinaryNumeralString, FF1};
 use group::GroupEncoding;
 use halo2::{arithmetic::FieldExt, pasta::pallas};
-use subtle::{Choice, CtOption};
+use subtle::CtOption;
 
 use crate::{
     address::Address,
@@ -32,17 +32,9 @@ impl SpendingKey {
     /// Returns `None` if the bytes do not correspond to a valid Orchard spending key.
     pub fn from_bytes(sk: [u8; 32]) -> CtOption<Self> {
         let sk = SpendingKey(sk);
-        // If ask = 0, or the default address would be ⊥, discard this key.
-        let ask_not_zero = !SpendAuthorizingKey::derive_inner(&sk).ct_is_zero();
-        let have_default_address = Choice::from({
-            let fvk = FullViewingKey::from(&sk);
-            if fvk.default_address_inner().is_some() {
-                1
-            } else {
-                0
-            }
-        });
-        CtOption::new(sk, ask_not_zero & have_default_address)
+        // If ask = 0, discard this key.
+        let ask = SpendAuthorizingKey::derive_inner(&sk);
+        CtOption::new(sk, !ask.ct_is_zero())
     }
 }
 
@@ -164,31 +156,16 @@ impl FullViewingKey {
 
     /// Returns the default payment address for this key.
     pub fn default_address(&self) -> Address {
-        self.default_address_inner()
-            .expect("Default address works by construction")
-    }
-
-    fn default_address_inner(&self) -> Option<Address> {
         self.address(DiversifierKey::from(self).default_diversifier())
     }
 
     /// Returns the payment address for this key at the given index.
-    ///
-    /// Returns `None` if the diversifier does not correspond to an address. This happens
-    /// with negligible probability; in most cases unwrapping the result will be fine, but
-    /// if you have specific stability requirements then you can either convert this into
-    /// an error, or try another diversifier index (e.g. incrementing).
-    pub fn address_at(&self, j: impl Into<DiversifierIndex>) -> Option<Address> {
+    pub fn address_at(&self, j: impl Into<DiversifierIndex>) -> Address {
         self.address(DiversifierKey::from(self).get(j))
     }
 
     /// Returns the payment address for this key corresponding to the given diversifier.
-    ///
-    /// Returns `None` if the diversifier does not correspond to an address. This happens
-    /// with negligible probability; in most cases unwrapping the result will be fine, but
-    /// if you have specific stability requirements then you can either convert this into
-    /// an error, or try another diversifier.
-    pub fn address(&self, d: Diversifier) -> Option<Address> {
+    pub fn address(&self, d: Diversifier) -> Address {
         IncomingViewingKey::from(self).address(d)
     }
 }
@@ -275,13 +252,9 @@ impl From<&FullViewingKey> for IncomingViewingKey {
 
 impl IncomingViewingKey {
     /// Returns the payment address for this key corresponding to the given diversifier.
-    ///
-    /// Returns `None` if the diversifier does not correspond to an address. This happens
-    /// with negligible probability; in most cases unwrapping the result will be fine, but
-    /// if you have specific stability requirements then you can either convert this into
-    /// an error, or try another diversifier.
-    pub fn address(&self, d: Diversifier) -> Option<Address> {
-        DiversifiedTransmissionKey::derive(self, &d).map(|pk_d| Address::from_parts(d, pk_d))
+    pub fn address(&self, d: Diversifier) -> Address {
+        let pk_d = DiversifiedTransmissionKey::derive(self, &d);
+        Address::from_parts(d, pk_d)
     }
 }
 
@@ -315,7 +288,8 @@ impl DiversifiedTransmissionKey {
     /// Defined in [Zcash Protocol Spec § 4.2.3: Orchard Key Components][orchardkeycomponents].
     ///
     /// [orchardkeycomponents]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
-    fn derive(ivk: &IncomingViewingKey, d: &Diversifier) -> Option<Self> {
-        diversify_hash(&d.0).map(|g_d| DiversifiedTransmissionKey(ka_orchard(&ivk.0, &g_d)))
+    fn derive(ivk: &IncomingViewingKey, d: &Diversifier) -> Self {
+        let g_d = diversify_hash(&d.0);
+        DiversifiedTransmissionKey(ka_orchard(&ivk.0, &g_d))
     }
 }

src/spec.rs

@@ -58,12 +58,13 @@ pub(crate) fn commit_ivk(
 /// Defined in [Zcash Protocol Spec § 5.4.1.6: DiversifyHash^Sapling and DiversifyHash^Orchard Hash Functions][concretediversifyhash].
 ///
 /// [concretediversifyhash]: https://zips.z.cash/protocol/nu5.pdf#concretediversifyhash
-pub(crate) fn diversify_hash(d: &[u8; 11]) -> Option<pallas::Point> {
+pub(crate) fn diversify_hash(d: &[u8; 11]) -> pallas::Point {
     let pk_d = pallas::Point::hash_to_curve("z.cash:Orchard-gd")(d);
     if pk_d.is_identity().into() {
-        None
+        // If the identity occurs, we replace it with a different fixed point.
+        pallas::Point::hash_to_curve("z.cash:Orchard-gd")(&[])
     } else {
-        Some(pk_d)
+        pk_d
     }
 }
 
@@ -107,3 +108,16 @@ pub(crate) fn extract_p(point: &pallas::Point) -> pallas::Base {
         pallas::Base::zero()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use group::Group;
+    use halo2::{arithmetic::CurveExt, pasta::pallas};
+
+    #[test]
+    fn diversify_hash_substitution() {
+        assert!(!bool::from(
+            pallas::Point::hash_to_curve("z.cash:Orchard-gd")(&[]).is_identity()
+        ));
+    }
+}