Check jubjub key correctness independent of redjubjub / jubjub (#3154)

* Ensure that sapling::keys::TransmissionKey jubjub point is always in the prime order group

* Adjust TransmissionKey check; add AuthorizingKey check

* Move ValueCommitment small order check to deserialization

* Apply suggestions from code review

Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>

* Use is_torsion_free() instead of is_identity() and is_prime_order()

* Add EphemeralPublicKey small order check on instantiation; remove old checks

* Use VerificationKey instead of VerificationKeyBytes; fix tests

* Use ValidatingKey instead of VerificationKeyBytes for rk

* Reject identity when creating an Orchard EphemeralPublicKey

* Make documentation more consistent, use generator in tests

* s/JubJub/Jubjub/

* Fix zebra-consensus tests (insert_fake_orchard_shielded_data)

* Create NotSmallOrderValueCommitment, since intermediate values can be the identity

* Clarify documentation

* rustdoc

* Tidy rustdoc

Co-authored-by: Conrado Gouvea <conrado@zfnd.org>
Co-authored-by: Pili Guerra <mpguerra@users.noreply.github.com>

zebra-chain/src/orchard/arbitrary.rs

@@ -29,7 +29,7 @@ impl Arbitrary for Action {
                 nullifier,
                 rk,
                 cm_x: NoteCommitment(pallas::Affine::identity()).extract_x(),
-                ephemeral_key: keys::EphemeralPublicKey(pallas::Affine::identity()),
+                ephemeral_key: keys::EphemeralPublicKey(pallas::Affine::generator()),
                 enc_ciphertext,
                 out_ciphertext,
             })

zebra-chain/src/orchard/keys.rs

@@ -16,7 +16,7 @@ use aes::Aes256;
 use bech32::{self, ToBase32, Variant};
 use bitvec::prelude::*;
 use fpe::ff1::{BinaryNumeralString, FF1};
-use group::{Group, GroupEncoding};
+use group::{prime::PrimeCurveAffine, Group, GroupEncoding};
 use halo2::{
     arithmetic::{Coordinates, CurveAffine, FieldExt},
     pasta::pallas,
@@ -1085,11 +1085,30 @@ impl PartialEq<[u8; 32]> for EphemeralPublicKey {
 impl TryFrom<[u8; 32]> for EphemeralPublicKey {
     type Error = &'static str;
 
+    /// Convert an array into a [`EphemeralPublicKey`].
+    ///
+    /// Returns an error if the encoding is malformed or if [it encodes the
+    /// identity point][1].
+    ///
+    /// > epk cannot be 𝒪_P
+    ///
+    /// Note that this is [intrinsic to the EphemeralPublicKey][2] type and it is not
+    /// a separate consensus rule:
+    ///
+    /// > Define KA^{Orchard}.Public := P^*.
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#actiondesc
+    /// [2]: https://zips.z.cash/protocol/protocol.pdf#concreteorchardkeyagreement
     fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
         let possible_point = pallas::Affine::from_bytes(&bytes);
 
         if possible_point.is_some().into() {
+            let point = possible_point.unwrap();
+            if point.to_curve().is_identity().into() {
+                Err("pallas::Affine value for Orchard EphemeralPublicKey is the identity")
+            } else {
                 Ok(Self(possible_point.unwrap()))
+            }
         } else {
             Err("Invalid pallas::Affine value for Orchard EphemeralPublicKey")
         }

zebra-chain/src/sapling/address.rs

@@ -1,6 +1,7 @@
 //! Shielded addresses.
 
 use std::{
+    convert::TryFrom,
     fmt,
     io::{self, Read, Write},
 };
@@ -81,7 +82,8 @@ impl std::str::FromStr for Address {
                         _ => Network::Testnet,
                     },
                     diversifier: keys::Diversifier::from(diversifier_bytes),
-                    transmission_key: keys::TransmissionKey::from(transmission_key_bytes),
+                    transmission_key: keys::TransmissionKey::try_from(transmission_key_bytes)
+                        .unwrap(),
                 })
             }
             _ => Err(SerializationError::Parse("bech32 decoding error")),
@@ -147,7 +149,8 @@ mod tests {
             keys::IncomingViewingKey::from((authorizing_key, nullifier_deriving_key));
 
         let diversifier = keys::Diversifier::new(&mut OsRng);
-        let transmission_key = keys::TransmissionKey::from((incoming_viewing_key, diversifier));
+        let transmission_key = keys::TransmissionKey::try_from((incoming_viewing_key, diversifier))
+            .expect("should be a valid transmission key");
 
         let _sapling_shielded_address = Address {
             network: Network::Mainnet,

zebra-chain/src/sapling/arbitrary.rs

@@ -1,6 +1,7 @@
 use std::convert::TryInto;
 
-use jubjub::AffinePoint;
+use group::Group;
+use jubjub::{AffinePoint, ExtendedPoint};
 use proptest::{arbitrary::any, collection::vec, prelude::*};
 use rand::SeedableRng;
 use rand_chacha::ChaChaRng;
@@ -8,8 +9,9 @@ use rand_chacha::ChaChaRng;
 use crate::primitives::Groth16Proof;
 
 use super::{
-    keys, note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4,
+    keys::{self, ValidatingKey},
-    PerSpendAnchor, SharedAnchor, Spend, ValueCommitment,
+    note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4, PerSpendAnchor,
+    SharedAnchor, Spend,
 };
 
 impl Arbitrary for Spend<PerSpendAnchor> {
@@ -25,7 +27,7 @@ impl Arbitrary for Spend<PerSpendAnchor> {
         )
             .prop_map(|(per_spend_anchor, nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor,
-                cv: ValueCommitment(AffinePoint::identity()),
+                cv: ExtendedPoint::generator().try_into().unwrap(),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -53,7 +55,7 @@ impl Arbitrary for Spend<SharedAnchor> {
         )
             .prop_map(|(nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor: FieldNotPresent,
-                cv: ValueCommitment(AffinePoint::identity()),
+                cv: ExtendedPoint::generator().try_into().unwrap(),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -79,9 +81,11 @@ impl Arbitrary for Output {
             any::<Groth16Proof>(),
         )
             .prop_map(|(enc_ciphertext, out_ciphertext, zkproof)| Self {
-                cv: ValueCommitment(AffinePoint::identity()),
+                cv: ExtendedPoint::generator().try_into().unwrap(),
                 cm_u: NoteCommitment(AffinePoint::identity()).extract_u(),
-                ephemeral_key: keys::EphemeralPublicKey(AffinePoint::identity()),
+                ephemeral_key: keys::EphemeralPublicKey(
+                    ExtendedPoint::generator().try_into().unwrap(),
+                ),
                 enc_ciphertext,
                 out_ciphertext,
                 zkproof,
@@ -104,12 +108,13 @@ impl Arbitrary for OutputInTransactionV4 {
 
 /// Creates Strategy for generation VerificationKeyBytes, since the `redjubjub`
 /// crate does not provide an Arbitrary implementation for it.
-fn spendauth_verification_key_bytes(
+fn spendauth_verification_key_bytes() -> impl Strategy<Value = ValidatingKey> {
-) -> impl Strategy<Value = redjubjub::VerificationKeyBytes<redjubjub::SpendAuth>> {
     prop::array::uniform32(any::<u8>()).prop_map(|bytes| {
         let mut rng = ChaChaRng::from_seed(bytes);
         let sk = redjubjub::SigningKey::<redjubjub::SpendAuth>::new(&mut rng);
-        redjubjub::VerificationKey::<redjubjub::SpendAuth>::from(&sk).into()
+        redjubjub::VerificationKey::<redjubjub::SpendAuth>::from(&sk)
+            .try_into()
+            .unwrap()
     })
 }
 

zebra-chain/src/sapling/commitment.rs

@@ -5,9 +5,13 @@ mod test_vectors;
 
 pub mod pedersen_hashes;
 
-use std::{convert::TryFrom, fmt, io};
+use std::{
+    convert::{TryFrom, TryInto},
+    fmt, io,
+};
 
 use bitvec::prelude::*;
+use jubjub::ExtendedPoint;
 use rand_core::{CryptoRng, RngCore};
 
 use crate::{
@@ -146,12 +150,15 @@ impl NoteCommitment {
     }
 }
 
-/// A Homomorphic Pedersen commitment to the value of a note, used in Spend and
+/// A Homomorphic Pedersen commitment to the value of a note.
-/// Output descriptions.
+///
+/// This can be used as an intermediate value in some computations. For the
+/// type actually stored in Spend and Output descriptions, see
+/// [`NotSmallOrderValueCommitment`].
 ///
 /// https://zips.z.cash/protocol/protocol.pdf#concretehomomorphiccommit
 #[derive(Clone, Copy, Deserialize, PartialEq, Serialize)]
-pub struct ValueCommitment(#[serde(with = "serde_helpers::AffinePoint")] pub jubjub::AffinePoint);
+pub struct ValueCommitment(#[serde(with = "serde_helpers::AffinePoint")] jubjub::AffinePoint);
 
 impl<'a> std::ops::Add<&'a ValueCommitment> for ValueCommitment {
     type Output = Self;
@@ -186,6 +193,7 @@ impl fmt::Debug for ValueCommitment {
 }
 
 impl From<jubjub::ExtendedPoint> for ValueCommitment {
+    /// Convert a Jubjub point into a ValueCommitment.
     fn from(extended_point: jubjub::ExtendedPoint) -> Self {
         Self(jubjub::AffinePoint::from(extended_point))
     }
@@ -248,26 +256,14 @@ impl TryFrom<[u8; 32]> for ValueCommitment {
         let possible_point = jubjub::AffinePoint::from_bytes(bytes);
 
         if possible_point.is_some().into() {
-            Ok(Self(possible_point.unwrap()))
+            let point = possible_point.unwrap();
+            Ok(ExtendedPoint::from(point).into())
         } else {
             Err("Invalid jubjub::AffinePoint value")
         }
     }
 }
 
-impl ZcashSerialize for ValueCommitment {
-    fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
-        writer.write_all(&<[u8; 32]>::from(*self)[..])?;
-        Ok(())
-    }
-}
-
-impl ZcashDeserialize for ValueCommitment {
-    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        Self::try_from(reader.read_32_bytes()?).map_err(SerializationError::Parse)
-    }
-}
-
 impl ValueCommitment {
     /// Generate a new _ValueCommitment_.
     ///
@@ -297,6 +293,80 @@ impl ValueCommitment {
     }
 }
 
+/// A Homomorphic Pedersen commitment to the value of a note, used in Spend and
+/// Output descriptions.
+///
+/// Elements that are of small order are not allowed. This is a separate
+/// consensus rule and not intrinsic of value commitments; which is why this
+/// type exists.
+///
+/// This is denoted by `cv` in the specification.
+///
+/// https://zips.z.cash/protocol/protocol.pdf#spenddesc
+/// https://zips.z.cash/protocol/protocol.pdf#outputdesc
+#[derive(Debug, Clone, Copy, Deserialize, PartialEq, Eq, Serialize)]
+pub struct NotSmallOrderValueCommitment(ValueCommitment);
+
+impl TryFrom<ValueCommitment> for NotSmallOrderValueCommitment {
+    type Error = &'static str;
+
+    /// Convert a ValueCommitment into a NotSmallOrderValueCommitment.
+    ///
+    /// Returns an error if the point is of small order.
+    ///
+    /// > cv and rk [MUST NOT be of small order][1], i.e. [h_J]cv MUST NOT be 𝒪_J
+    /// > and [h_J]rk MUST NOT be 𝒪_J.
+    ///
+    /// > cv and epk [MUST NOT be of small order][2], i.e. [h_J]cv MUST NOT be 𝒪_J
+    /// > and [ℎ_J]epk MUST NOT be 𝒪_J.
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#spenddesc
+    /// [2]: https://zips.z.cash/protocol/protocol.pdf#outputdesc
+    fn try_from(value_commitment: ValueCommitment) -> Result<Self, Self::Error> {
+        if value_commitment.0.is_small_order().into() {
+            Err("jubjub::AffinePoint value for Sapling ValueCommitment is of small order")
+        } else {
+            Ok(Self(value_commitment))
+        }
+    }
+}
+
+impl TryFrom<jubjub::ExtendedPoint> for NotSmallOrderValueCommitment {
+    type Error = &'static str;
+
+    /// Convert a Jubjub point into a NotSmallOrderValueCommitment.
+    fn try_from(extended_point: jubjub::ExtendedPoint) -> Result<Self, Self::Error> {
+        ValueCommitment::from(extended_point).try_into()
+    }
+}
+
+impl From<NotSmallOrderValueCommitment> for ValueCommitment {
+    fn from(cv: NotSmallOrderValueCommitment) -> Self {
+        cv.0
+    }
+}
+
+impl From<NotSmallOrderValueCommitment> for jubjub::AffinePoint {
+    fn from(cv: NotSmallOrderValueCommitment) -> Self {
+        cv.0 .0
+    }
+}
+
+impl ZcashSerialize for NotSmallOrderValueCommitment {
+    fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
+        writer.write_all(&<[u8; 32]>::from(self.0)[..])?;
+        Ok(())
+    }
+}
+
+impl ZcashDeserialize for NotSmallOrderValueCommitment {
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        let vc = ValueCommitment::try_from(reader.read_32_bytes()?)
+            .map_err(SerializationError::Parse)?;
+        vc.try_into().map_err(SerializationError::Parse)
+    }
+}
+
 #[cfg(test)]
 mod tests {
 

zebra-chain/src/sapling/keys.rs

@@ -16,7 +16,7 @@ mod test_vectors;
 mod tests;
 
 use std::{
-    convert::{From, Into, TryFrom},
+    convert::{From, Into, TryFrom, TryInto},
     fmt,
     io::{self, Write},
     str::FromStr,
@@ -472,9 +472,30 @@ pub struct AuthorizingKey(pub(crate) redjubjub::VerificationKey<SpendAuth>);
 
 impl Eq for AuthorizingKey {}
 
-impl From<[u8; 32]> for AuthorizingKey {
+impl TryFrom<[u8; 32]> for AuthorizingKey {
-    fn from(bytes: [u8; 32]) -> Self {
+    type Error = &'static str;
-        Self(redjubjub::VerificationKey::try_from(bytes).unwrap())
+
+    /// Convert an array into an AuthorizingKey.
+    ///
+    /// Returns an error if the encoding is malformed or if [it does not encode
+    /// a prime-order point][1]:
+    ///
+    /// > When decoding this representation, the key MUST be considered invalid
+    /// > if abst_J returns ⊥ for either ak or nk, or if ak not in J^{(r)*}
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#saplingfullviewingkeyencoding
+    fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
+        let affine_point = jubjub::AffinePoint::from_bytes(bytes);
+        if affine_point.is_none().into() {
+            return Err("Invalid jubjub::AffinePoint for Sapling AuthorizingKey");
+        }
+        if affine_point.unwrap().is_prime_order().into() {
+            Ok(Self(redjubjub::VerificationKey::try_from(bytes).map_err(
+                |_e| "Invalid jubjub::AffinePoint for Sapling AuthorizingKey",
+            )?))
+        } else {
+            Err("jubjub::AffinePoint value for Sapling AuthorizingKey is not of prime order")
+        }
     }
 }
 
@@ -822,7 +843,13 @@ impl Diversifier {
 /// Derived by multiplying a JubJub point [derived][ps] from a
 /// _Diversifier_ by the _IncomingViewingKey_ scalar.
 ///
-/// [ps]: https://zips.z.cash/protocol/protocol.pdf#concretediversifyhash
+/// The diversified TransmissionKey is denoted `pk_d` in the specification.
+/// Note that it can be the identity point, since its type is
+/// [`KA^{Sapling}.PublicPrimeSubgroup`][ka] which in turn is [`J^{(r)}`][jubjub].
+///
+/// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
+/// [ka]: https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
+/// [jubjub]: https://zips.z.cash/protocol/protocol.pdf#jubjub
 #[derive(Copy, Clone, PartialEq)]
 pub struct TransmissionKey(pub(crate) jubjub::AffinePoint);
 
@@ -837,14 +864,22 @@ impl fmt::Debug for TransmissionKey {
 
 impl Eq for TransmissionKey {}
 
-impl From<[u8; 32]> for TransmissionKey {
+impl TryFrom<[u8; 32]> for TransmissionKey {
-    /// Attempts to interpret a byte representation of an
+    type Error = &'static str;
-    /// affine point, failing if the element is not on
+
-    /// the curve or non-canonical.
+    /// Attempts to interpret a byte representation of an affine Jubjub point, failing if the
+    /// element is not on the curve, non-canonical, or not in the prime-order subgroup.
     ///
     /// https://github.com/zkcrypto/jubjub/blob/master/src/lib.rs#L411
-    fn from(bytes: [u8; 32]) -> Self {
+    /// https://zips.z.cash/zip-0216
-        Self(jubjub::AffinePoint::from_bytes(bytes).unwrap())
+    fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
+        let affine_point = jubjub::AffinePoint::from_bytes(bytes).unwrap();
+        // Check if it's identity or has prime order (i.e. is in the prime-order subgroup).
+        if affine_point.is_torsion_free().into() {
+            Ok(Self(affine_point))
+        } else {
+            Err("Invalid jubjub::AffinePoint value for Sapling TransmissionKey")
+        }
     }
 }
 
@@ -854,16 +889,23 @@ impl From<TransmissionKey> for [u8; 32] {
     }
 }
 
-impl From<(IncomingViewingKey, Diversifier)> for TransmissionKey {
+impl TryFrom<(IncomingViewingKey, Diversifier)> for TransmissionKey {
+    type Error = &'static str;
+
     /// This includes _KA^Sapling.DerivePublic(ivk, G_d)_, which is just a
     /// scalar mult _\[ivk\]G_d_.
     ///
     /// https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
     /// https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
-    fn from((ivk, d): (IncomingViewingKey, Diversifier)) -> Self {
+    fn try_from((ivk, d): (IncomingViewingKey, Diversifier)) -> Result<Self, Self::Error> {
-        Self(jubjub::AffinePoint::from(
+        let affine_point = jubjub::AffinePoint::from(
-            diversify_hash(d.0).unwrap() * ivk.scalar,
+            diversify_hash(d.0).ok_or("invalid diversifier")? * ivk.scalar,
-        ))
+        );
+        // We need to ensure that the result point is in the prime-order subgroup.
+        // Since diversify_hash() returns a point in the prime-order subgroup,
+        // then the result point will also be in the prime-order subgroup
+        // and there is no need to check anything.
+        Ok(Self(affine_point))
     }
 }
 
@@ -946,7 +988,8 @@ impl FromStr for FullViewingKey {
                         fvk_hrp::MAINNET => Network::Mainnet,
                         _ => Network::Testnet,
                     },
-                    authorizing_key: AuthorizingKey::from(authorizing_key_bytes),
+                    authorizing_key: AuthorizingKey::try_from(authorizing_key_bytes)
+                        .map_err(SerializationError::Parse)?,
                     nullifier_deriving_key: NullifierDerivingKey::from(
                         nullifier_deriving_key_bytes,
                     ),
@@ -958,9 +1001,16 @@ impl FromStr for FullViewingKey {
     }
 }
 
-/// An ephemeral public key for Sapling key agreement.
+/// An [ephemeral public key][1] for Sapling key agreement.
 ///
-/// https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
+/// Public keys containing points of small order are not allowed.
+///
+/// It is denoted by `epk` in the specification. (This type does _not_
+/// represent [KA^{Sapling}.Public][2], which allows any points, including
+/// of small order).
+///
+/// [1]: https://zips.z.cash/protocol/protocol.pdf#outputdesc
+/// [2]: https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
 #[derive(Copy, Clone, Deserialize, PartialEq, Serialize)]
 pub struct EphemeralPublicKey(
     #[serde(with = "serde_helpers::AffinePoint")] pub(crate) jubjub::AffinePoint,
@@ -998,13 +1048,24 @@ impl PartialEq<[u8; 32]> for EphemeralPublicKey {
 impl TryFrom<[u8; 32]> for EphemeralPublicKey {
     type Error = &'static str;
 
+    /// Read an EphemeralPublicKey from a byte array.
+    ///
+    /// Returns an error if the key is non-canonical, or [it is of small order][1].
+    ///
+    /// > Check that a Output description's cv and epk are not of small order,
+    /// > i.e. [h_J]cv MUST NOT be 𝒪_J and [h_J]epk MUST NOT be 𝒪_J.
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#outputdesc
     fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
         let possible_point = jubjub::AffinePoint::from_bytes(bytes);
 
-        if possible_point.is_some().into() {
+        if possible_point.is_none().into() {
-            Ok(Self(possible_point.unwrap()))
+            return Err("Invalid jubjub::AffinePoint value for Sapling EphemeralPublicKey");
+        }
+        if possible_point.unwrap().is_small_order().into() {
+            Err("jubjub::AffinePoint value for Sapling EphemeralPublicKey point is of small order")
         } else {
-            Err("Invalid jubjub::AffinePoint value")
+            Ok(Self(possible_point.unwrap()))
         }
     }
 }
@@ -1021,3 +1082,60 @@ impl ZcashDeserialize for EphemeralPublicKey {
         Self::try_from(reader.read_32_bytes()?).map_err(SerializationError::Parse)
     }
 }
+
+/// A randomized [validating key][1] that should be used to validate `spendAuthSig`.
+///
+/// It is denoted by `rk` in the specification. (This type does _not_
+/// represent [SpendAuthSig^{Sapling}.Public][2], which allows any points, including
+/// of small order).
+///
+/// [1]: https://zips.z.cash/protocol/protocol.pdf#spenddesc
+/// [2]: https://zips.z.cash/protocol/protocol.pdf#concretereddsa
+#[derive(Debug, PartialEq, Serialize, Deserialize, Clone)]
+pub struct ValidatingKey(redjubjub::VerificationKey<SpendAuth>);
+
+impl TryFrom<redjubjub::VerificationKey<SpendAuth>> for ValidatingKey {
+    type Error = &'static str;
+
+    /// Convert an array into a ValidatingKey.
+    ///
+    /// Returns an error if the key is malformed or [is of small order][1].
+    ///
+    /// > Check that a Spend description's cv and rk are not of small order,
+    /// > i.e. [h_J]cv MUST NOT be 𝒪_J and [h_J]rk MUST NOT be 𝒪_J.
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#spenddesc
+    fn try_from(key: redjubjub::VerificationKey<SpendAuth>) -> Result<Self, Self::Error> {
+        if bool::from(
+            jubjub::AffinePoint::from_bytes(key.into())
+                .unwrap()
+                .is_small_order(),
+        ) {
+            Err("jubjub::AffinePoint value for Sapling ValidatingKey is of small order")
+        } else {
+            Ok(Self(key))
+        }
+    }
+}
+
+impl TryFrom<[u8; 32]> for ValidatingKey {
+    type Error = &'static str;
+
+    fn try_from(value: [u8; 32]) -> Result<Self, Self::Error> {
+        let vk = redjubjub::VerificationKey::<SpendAuth>::try_from(value)
+            .map_err(|_| "Invalid redjubjub::ValidatingKey for Sapling ValidatingKey")?;
+        vk.try_into()
+    }
+}
+
+impl From<ValidatingKey> for [u8; 32] {
+    fn from(key: ValidatingKey) -> Self {
+        key.0.into()
+    }
+}
+
+impl From<ValidatingKey> for redjubjub::VerificationKeyBytes<SpendAuth> {
+    fn from(key: ValidatingKey) -> Self {
+        key.0.into()
+    }
+}

zebra-chain/src/sapling/keys/tests.rs

@@ -22,7 +22,7 @@ impl Arbitrary for TransmissionKey {
 
                 let diversifier = Diversifier::from(spending_key);
 
-                Self::from((incoming_viewing_key, diversifier))
+                Self::try_from((incoming_viewing_key, diversifier)).unwrap()
             })
             .boxed()
     }
@@ -60,7 +60,8 @@ mod tests {
             let diversifier = Diversifier::from(spending_key);
             assert_eq!(diversifier, test_vector.default_d);
 
-            let transmission_key = TransmissionKey::from((incoming_viewing_key, diversifier));
+            let transmission_key = TransmissionKey::try_from((incoming_viewing_key, diversifier))
+                .expect("should be a valid transmission key");
             assert_eq!(transmission_key, test_vector.default_pk_d);
 
             let _full_viewing_key = FullViewingKey {

zebra-chain/src/sapling/output.rs

@@ -25,7 +25,7 @@ use super::{commitment, keys, note};
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
 pub struct Output {
     /// A value commitment to the value of the input note.
-    pub cv: commitment::ValueCommitment,
+    pub cv: commitment::NotSmallOrderValueCommitment,
     /// The u-coordinate of the note commitment for the output note.
     #[serde(with = "serde_helpers::Fq")]
     pub cm_u: jubjub::Fq,
@@ -56,7 +56,7 @@ pub struct OutputInTransactionV4(pub Output);
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
 pub struct OutputPrefixInTransactionV5 {
     /// A value commitment to the value of the input note.
-    pub cv: commitment::ValueCommitment,
+    pub cv: commitment::NotSmallOrderValueCommitment,
     /// The u-coordinate of the note commitment for the output note.
     #[serde(with = "serde_helpers::Fq")]
     pub cm_u: jubjub::Fq,
@@ -135,7 +135,7 @@ impl ZcashSerialize for OutputInTransactionV4 {
 impl ZcashDeserialize for OutputInTransactionV4 {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
         Ok(OutputInTransactionV4(Output {
-            cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
+            cv: commitment::NotSmallOrderValueCommitment::zcash_deserialize(&mut reader)?,
             cm_u: jubjub::Fq::zcash_deserialize(&mut reader)?,
             ephemeral_key: keys::EphemeralPublicKey::zcash_deserialize(&mut reader)?,
             enc_ciphertext: note::EncryptedNote::zcash_deserialize(&mut reader)?,
@@ -165,7 +165,7 @@ impl ZcashSerialize for OutputPrefixInTransactionV5 {
 impl ZcashDeserialize for OutputPrefixInTransactionV5 {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
         Ok(OutputPrefixInTransactionV5 {
-            cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
+            cv: commitment::NotSmallOrderValueCommitment::zcash_deserialize(&mut reader)?,
             cm_u: jubjub::Fq::zcash_deserialize(&mut reader)?,
             ephemeral_key: keys::EphemeralPublicKey::zcash_deserialize(&mut reader)?,
             enc_ciphertext: note::EncryptedNote::zcash_deserialize(&mut reader)?,

zebra-chain/src/sapling/shielded_data.rs

@@ -289,10 +289,10 @@ where
     /// of the value commitments in the Spend descriptions and Output
     /// descriptions of the transaction, and the balancing value.
     ///
-    /// https://zips.z.cash/protocol/protocol.pdf#saplingbalance
+    /// <https://zips.z.cash/protocol/protocol.pdf#saplingbalance>
     pub fn binding_verification_key(&self) -> redjubjub::VerificationKeyBytes<Binding> {
-        let cv_old: ValueCommitment = self.spends().map(|spend| spend.cv).sum();
+        let cv_old: ValueCommitment = self.spends().map(|spend| spend.cv.into()).sum();
-        let cv_new: ValueCommitment = self.outputs().map(|output| output.cv).sum();
+        let cv_new: ValueCommitment = self.outputs().map(|output| output.cv.into()).sum();
         let cv_balance: ValueCommitment =
             ValueCommitment::new(jubjub::Fr::zero(), self.value_balance);
 

zebra-chain/src/sapling/spend.rs

@@ -3,7 +3,7 @@
 //! Zebra uses a generic spend type for `V4` and `V5` transactions.
 //! The anchor change is handled using the `AnchorVariant` type trait.
 
-use std::{fmt, io};
+use std::{convert::TryInto, fmt, io};
 
 use crate::{
     block::MAX_BLOCK_BYTES,
@@ -17,7 +17,10 @@ use crate::{
     },
 };
 
-use super::{commitment, note, tree, AnchorVariant, FieldNotPresent, PerSpendAnchor, SharedAnchor};
+use super::{
+    commitment, keys::ValidatingKey, note, tree, AnchorVariant, FieldNotPresent, PerSpendAnchor,
+    SharedAnchor,
+};
 
 /// A _Spend Description_, as described in [protocol specification §7.3][ps].
 ///
@@ -31,10 +34,10 @@ use super::{commitment, note, tree, AnchorVariant, FieldNotPresent, PerSpendAnch
 /// `V5` transactions split them into multiple arrays.
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#spendencoding
-#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
 pub struct Spend<AnchorV: AnchorVariant> {
     /// A value commitment to the value of the input note.
-    pub cv: commitment::ValueCommitment,
+    pub cv: commitment::NotSmallOrderValueCommitment,
     /// An anchor for this spend.
     ///
     /// The anchor is the root of the Sapling note commitment tree in a previous
@@ -48,7 +51,7 @@ pub struct Spend<AnchorV: AnchorVariant> {
     /// The nullifier of the input note.
     pub nullifier: note::Nullifier,
     /// The randomized public key for `spend_auth_sig`.
-    pub rk: redjubjub::VerificationKeyBytes<SpendAuth>,
+    pub rk: ValidatingKey,
     /// The ZK spend proof.
     pub zkproof: Groth16Proof,
     /// A signature authorizing this spend.
@@ -63,16 +66,24 @@ pub struct Spend<AnchorV: AnchorVariant> {
 /// Serialized as `SpendDescriptionV5` in [protocol specification §7.3][ps].
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#spendencoding
-#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
 pub struct SpendPrefixInTransactionV5 {
     /// A value commitment to the value of the input note.
-    pub cv: commitment::ValueCommitment,
+    pub cv: commitment::NotSmallOrderValueCommitment,
     /// The nullifier of the input note.
     pub nullifier: note::Nullifier,
     /// The randomized public key for `spend_auth_sig`.
-    pub rk: redjubjub::VerificationKeyBytes<SpendAuth>,
+    pub rk: ValidatingKey,
 }
 
+// We can't derive Eq because `VerificationKey` does not implement it,
+// even if it is valid for it.
+impl<AnchorV: AnchorVariant + PartialEq> Eq for Spend<AnchorV> {}
+
+// We can't derive Eq because `VerificationKey` does not implement it,
+// even if it is valid for it.
+impl Eq for SpendPrefixInTransactionV5 {}
+
 impl<AnchorV> fmt::Display for Spend<AnchorV>
 where
     AnchorV: AnchorVariant + Clone,
@@ -152,7 +163,7 @@ impl ZcashSerialize for Spend<PerSpendAnchor> {
         self.cv.zcash_serialize(&mut writer)?;
         writer.write_all(&self.per_spend_anchor.0[..])?;
         writer.write_32_bytes(&self.nullifier.into())?;
-        writer.write_all(&<[u8; 32]>::from(self.rk)[..])?;
+        writer.write_all(&<[u8; 32]>::from(self.rk.clone())[..])?;
         self.zkproof.zcash_serialize(&mut writer)?;
         writer.write_all(&<[u8; 64]>::from(self.spend_auth_sig)[..])?;
         Ok(())
@@ -162,10 +173,13 @@ impl ZcashSerialize for Spend<PerSpendAnchor> {
 impl ZcashDeserialize for Spend<PerSpendAnchor> {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
         Ok(Spend {
-            cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
+            cv: commitment::NotSmallOrderValueCommitment::zcash_deserialize(&mut reader)?,
             per_spend_anchor: tree::Root(reader.read_32_bytes()?),
             nullifier: note::Nullifier::from(reader.read_32_bytes()?),
-            rk: reader.read_32_bytes()?.into(),
+            rk: reader
+                .read_32_bytes()?
+                .try_into()
+                .map_err(SerializationError::Parse)?,
             zkproof: Groth16Proof::zcash_deserialize(&mut reader)?,
             spend_auth_sig: reader.read_64_bytes()?.into(),
         })
@@ -182,7 +196,7 @@ impl ZcashSerialize for SpendPrefixInTransactionV5 {
     fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
         self.cv.zcash_serialize(&mut writer)?;
         writer.write_32_bytes(&self.nullifier.into())?;
-        writer.write_all(&<[u8; 32]>::from(self.rk)[..])?;
+        writer.write_all(&<[u8; 32]>::from(self.rk.clone())[..])?;
         Ok(())
     }
 }
@@ -190,9 +204,12 @@ impl ZcashSerialize for SpendPrefixInTransactionV5 {
 impl ZcashDeserialize for SpendPrefixInTransactionV5 {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
         Ok(SpendPrefixInTransactionV5 {
-            cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
+            cv: commitment::NotSmallOrderValueCommitment::zcash_deserialize(&mut reader)?,
             nullifier: note::Nullifier::from(reader.read_32_bytes()?),
-            rk: reader.read_32_bytes()?.into(),
+            rk: reader
+                .read_32_bytes()?
+                .try_into()
+                .map_err(SerializationError::Parse)?,
         })
     }
 }

zebra-chain/src/transaction/arbitrary.rs

@@ -9,7 +9,9 @@ use std::{
 };
 
 use chrono::{TimeZone, Utc};
-use proptest::{arbitrary::any, array, collection::vec, option, prelude::*};
+use proptest::{
+    arbitrary::any, array, collection::vec, option, prelude::*, test_runner::TestRunner,
+};
 
 use crate::{
     amount::{self, Amount, NegativeAllowed, NonNegative},
@@ -22,7 +24,7 @@ use crate::{
         Bctv14Proof, Groth16Proof, Halo2Proof, ZkSnarkProof,
     },
     sapling::{self, AnchorVariant, PerSpendAnchor, SharedAnchor},
-    serialization::{ZcashDeserialize, ZcashDeserializeInto},
+    serialization::ZcashDeserializeInto,
     sprout, transparent,
     value_balance::{ValueBalance, ValueBalanceError},
     LedgerState,
@@ -975,11 +977,12 @@ pub fn fake_v5_transactions_for_network<'b>(
 pub fn insert_fake_orchard_shielded_data(
     transaction: &mut Transaction,
 ) -> &mut orchard::ShieldedData {
-    // Create a dummy action, it doesn't need to be valid
+    // Create a dummy action
-    let dummy_action_bytes = [0u8; 820];
+    let mut runner = TestRunner::default();
-    let mut dummy_action_bytes_reader = &dummy_action_bytes[..];
+    let dummy_action = orchard::Action::arbitrary()
-    let dummy_action = orchard::Action::zcash_deserialize(&mut dummy_action_bytes_reader)
+        .new_tree(&mut runner)
-        .expect("Dummy action should deserialize");
+        .unwrap()
+        .current();
 
     // Pair the dummy action with a fake signature
     let dummy_authorized_action = orchard::AuthorizedAction {

zebra-consensus/src/primitives/groth16.rs

@@ -162,11 +162,11 @@ impl Description for Spend<PerSpendAnchor> {
     fn primary_inputs(&self) -> Vec<jubjub::Fq> {
         let mut inputs = vec![];
 
-        let rk_affine = jubjub::AffinePoint::from_bytes(self.rk.into()).unwrap();
+        let rk_affine = jubjub::AffinePoint::from_bytes(self.rk.clone().into()).unwrap();
         inputs.push(rk_affine.get_u());
         inputs.push(rk_affine.get_v());
 
-        let cv_affine = jubjub::AffinePoint::from_bytes(self.cv.into()).unwrap();
+        let cv_affine = jubjub::AffinePoint::from(self.cv);
         inputs.push(cv_affine.get_u());
         inputs.push(cv_affine.get_v());
 
@@ -197,7 +197,7 @@ impl Description for Output {
     fn primary_inputs(&self) -> Vec<jubjub::Fq> {
         let mut inputs = vec![];
 
-        let cv_affine = jubjub::AffinePoint::from_bytes(self.cv.into()).unwrap();
+        let cv_affine = jubjub::AffinePoint::from(self.cv);
         inputs.push(cv_affine.get_u());
         inputs.push(cv_affine.get_v());
 

zebra-consensus/src/transaction.rs

@@ -694,13 +694,6 @@ where
 
         if let Some(sapling_shielded_data) = sapling_shielded_data {
             for spend in sapling_shielded_data.spends_per_anchor() {
-                // Consensus rule: cv and rk MUST NOT be of small
-                // order, i.e. [h_J]cv MUST NOT be 𝒪_J and [h_J]rk
-                // MUST NOT be 𝒪_J.
-                //
-                // https://zips.z.cash/protocol/protocol.pdf#spenddesc
-                check::spend_cv_rk_not_small_order(&spend)?;
-
                 // Consensus rule: The proof π_ZKSpend MUST be valid
                 // given a primary input formed from the other
                 // fields except spendAuthSig.
@@ -728,18 +721,11 @@ where
                 async_checks.push(
                     primitives::redjubjub::VERIFIER
                         .clone()
-                        .oneshot((spend.rk, spend.spend_auth_sig, shielded_sighash).into()),
+                        .oneshot((spend.rk.into(), spend.spend_auth_sig, shielded_sighash).into()),
                 );
             }
 
             for output in sapling_shielded_data.outputs() {
-                // Consensus rule: cv and wpk MUST NOT be of small
-                // order, i.e. [h_J]cv MUST NOT be 𝒪_J and [h_J]wpk
-                // MUST NOT be 𝒪_J.
-                //
-                // https://zips.z.cash/protocol/protocol.pdf#outputdesc
-                check::output_cv_epk_not_small_order(output)?;
-
                 // Consensus rule: The proof π_ZKOutput MUST be
                 // valid given a primary input formed from the other
                 // fields except C^enc and C^out.

zebra-consensus/src/transaction/check.rs

@@ -12,7 +12,6 @@ use zebra_chain::{
     orchard::Flags,
     parameters::{Network, NetworkUpgrade},
     primitives::zcash_note_encryption,
-    sapling::{Output, PerSpendAnchor, Spend},
     transaction::{LockTime, Transaction},
 };
 
@@ -128,42 +127,6 @@ pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), Tr
     Ok(())
 }
 
-/// Check that a Spend description's cv and rk are not of small order,
-/// i.e. [h_J]cv MUST NOT be 𝒪_J and [h_J]rk MUST NOT be 𝒪_J.
-///
-/// https://zips.z.cash/protocol/protocol.pdf#spenddesc
-pub fn spend_cv_rk_not_small_order(spend: &Spend<PerSpendAnchor>) -> Result<(), TransactionError> {
-    if bool::from(spend.cv.0.is_small_order())
-        || bool::from(
-            jubjub::AffinePoint::from_bytes(spend.rk.into())
-                .unwrap()
-                .is_small_order(),
-        )
-    {
-        Err(TransactionError::SmallOrder)
-    } else {
-        Ok(())
-    }
-}
-
-/// Check that a Output description's cv and epk are not of small order,
-/// i.e. [h_J]cv MUST NOT be 𝒪_J and [h_J]epk MUST NOT be 𝒪_J.
-///
-/// https://zips.z.cash/protocol/protocol.pdf#outputdesc
-pub fn output_cv_epk_not_small_order(output: &Output) -> Result<(), TransactionError> {
-    if bool::from(output.cv.0.is_small_order())
-        || bool::from(
-            jubjub::AffinePoint::from_bytes(output.ephemeral_key.into())
-                .unwrap()
-                .is_small_order(),
-        )
-    {
-        Err(TransactionError::SmallOrder)
-    } else {
-        Ok(())
-    }
-}
-
 /// Check if JoinSplits in the transaction have one of its v_{pub} values equal
 /// to zero.
 ///

zebra-consensus/src/transaction/tests.rs

@@ -53,18 +53,6 @@ fn v5_fake_transactions() -> Result<(), Report> {
 
             // make sure there are no joinsplits nor spends in coinbase
             check::coinbase_tx_no_prevout_joinsplit_spend(&transaction)?;
-
-            // validate the sapling shielded data
-            if transaction.version() == 5 {
-                for spend in transaction.sapling_spends_per_anchor() {
-                    check::spend_cv_rk_not_small_order(&spend)?;
-                }
-                for output in transaction.sapling_outputs() {
-                    check::output_cv_epk_not_small_order(output)?;
-                }
-            } else {
-                panic!("we should have no tx other than 5");
-            }
         }
     }