From ab38d700c85c5e2fe0da44cf1c057b27fa9a082f Mon Sep 17 00:00:00 2001
From: Deirdre Connolly <durumcrustulum@gmail.com>
Date: Fri, 3 Dec 2021 16:20:54 -0500
Subject: [PATCH 01/16] Ensure that sapling::keys::TransmissionKey jubjub point
 is always in the prime order group

---
 zebra-chain/src/sapling/address.rs    |  7 ++++--
 zebra-chain/src/sapling/keys.rs       | 34 ++++++++++++++++++---------
 zebra-chain/src/sapling/keys/tests.rs |  5 ++--
 3 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/zebra-chain/src/sapling/address.rs b/zebra-chain/src/sapling/address.rs
index 6937460862a..7cb27c77e6a 100644
--- a/zebra-chain/src/sapling/address.rs
+++ b/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,
diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index cb6806f10b6..617bc203d83 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -837,14 +837,20 @@ impl fmt::Debug for TransmissionKey {
 
 impl Eq for TransmissionKey {}
 
-impl From<[u8; 32]> for TransmissionKey {
-    /// Attempts to interpret a byte representation of an
-    /// affine point, failing if the element is not on
-    /// the curve or non-canonical.
+impl TryFrom<[u8; 32]> for TransmissionKey {
+    type Error = &'static str;
+
+    /// Attempts to interpret a byte representation of an affine Jubjub point, failing if the
+    /// element is not on the curve, non-prime, the identity, or non-canonical.
     ///
     /// https://github.com/zkcrypto/jubjub/blob/master/src/lib.rs#L411
-    fn from(bytes: [u8; 32]) -> Self {
-        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();
+        if affine_point.is_prime_order().into() {
+            Ok(Self(affine_point))
+        } else {
+            Err("derived an invalid Sapling transmission key")
+        }
     }
 }
 
@@ -854,16 +860,22 @@ 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 {
-        Self(jubjub::AffinePoint::from(
-            diversify_hash(d.0).unwrap() * ivk.scalar,
-        ))
+    fn try_from((ivk, d): (IncomingViewingKey, Diversifier)) -> Result<Self, Self::Error> {
+        let affine_point = jubjub::AffinePoint::from(diversify_hash(d.0).unwrap() * ivk.scalar);
+
+        if affine_point.is_prime_order().into() {
+            Ok(Self(affine_point))
+        } else {
+            Err("derived an invalid Sapling transmission key")
+        }
     }
 }
 
diff --git a/zebra-chain/src/sapling/keys/tests.rs b/zebra-chain/src/sapling/keys/tests.rs
index 8b8f43228ff..9b4ba3170f6 100644
--- a/zebra-chain/src/sapling/keys/tests.rs
+++ b/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 {

From eab3bae09f95e4a30863081221e14431726054bd Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Wed, 15 Dec 2021 17:55:45 -0300
Subject: [PATCH 02/16] Adjust TransmissionKey check; add AuthorizingKey check

---
 zebra-chain/src/sapling/keys.rs | 57 +++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index 617bc203d83..305ed5d3718 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -472,9 +472,29 @@ pub struct AuthorizingKey(pub(crate) redjubjub::VerificationKey<SpendAuth>);
 
 impl Eq for AuthorizingKey {}
 
-impl From<[u8; 32]> for AuthorizingKey {
-    fn from(bytes: [u8; 32]) -> Self {
-        Self(redjubjub::VerificationKey::try_from(bytes).unwrap())
+impl TryFrom<[u8; 32]> for AuthorizingKey {
+    type Error = &'static str;
+
+    /// Decode an AuthorizingKey from a byte array.
+    ///
+    /// It must decode to a prime-order point:
+    ///
+    /// > 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)*}
+    ///
+    /// 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("derived an invalid Sapling transmission key");
+        }
+        if affine_point.unwrap().is_prime_order().into() {
+            Ok(Self(redjubjub::VerificationKey::try_from(bytes).map_err(
+                |_e| "derived an invalid Sapling transmission key",
+            )?))
+        } else {
+            Err("derived an invalid Sapling transmission key")
+        }
     }
 }
 
@@ -822,7 +842,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);
 
@@ -841,12 +867,13 @@ impl TryFrom<[u8; 32]> for TransmissionKey {
     type Error = &'static str;
 
     /// Attempts to interpret a byte representation of an affine Jubjub point, failing if the
-    /// element is not on the curve, non-prime, the identity, or non-canonical.
+    /// 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
+    /// https://zips.z.cash/zip-0216
     fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
         let affine_point = jubjub::AffinePoint::from_bytes(bytes).unwrap();
-        if affine_point.is_prime_order().into() {
+        if (affine_point.is_identity() | affine_point.is_prime_order()).into() {
             Ok(Self(affine_point))
         } else {
             Err("derived an invalid Sapling transmission key")
@@ -869,13 +896,14 @@ impl TryFrom<(IncomingViewingKey, Diversifier)> for TransmissionKey {
     /// https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
     /// https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
     fn try_from((ivk, d): (IncomingViewingKey, Diversifier)) -> Result<Self, Self::Error> {
-        let affine_point = jubjub::AffinePoint::from(diversify_hash(d.0).unwrap() * ivk.scalar);
-
-        if affine_point.is_prime_order().into() {
-            Ok(Self(affine_point))
-        } else {
-            Err("derived an invalid Sapling transmission key")
-        }
+        let affine_point = jubjub::AffinePoint::from(
+            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))
     }
 }
 
@@ -958,7 +986,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,
                     ),

From 7c7903f9d47b236ff7e0609b4311f048bc912a75 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Wed, 15 Dec 2021 18:50:18 -0300
Subject: [PATCH 03/16] Move ValueCommitment small order check to
 deserialization

---
 zebra-chain/src/sapling/arbitrary.rs     | 20 ++++++++++++----
 zebra-chain/src/sapling/commitment.rs    | 29 +++++++++++++++++++-----
 zebra-consensus/src/transaction/check.rs | 24 ++++++++------------
 3 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/zebra-chain/src/sapling/arbitrary.rs b/zebra-chain/src/sapling/arbitrary.rs
index 8d681ef0e6f..560cdbd72dd 100644
--- a/zebra-chain/src/sapling/arbitrary.rs
+++ b/zebra-chain/src/sapling/arbitrary.rs
@@ -8,8 +8,9 @@ use rand_chacha::ChaChaRng;
 use crate::primitives::Groth16Proof;
 
 use super::{
-    keys, note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4,
-    PerSpendAnchor, SharedAnchor, Spend, ValueCommitment,
+    keys::{self, find_group_hash},
+    note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4, PerSpendAnchor,
+    SharedAnchor, Spend,
 };
 
 impl Arbitrary for Spend<PerSpendAnchor> {
@@ -25,7 +26,10 @@ impl Arbitrary for Spend<PerSpendAnchor> {
         )
             .prop_map(|(per_spend_anchor, nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor,
-                cv: ValueCommitment(AffinePoint::identity()),
+                // Use an arbitrary string to generate a dummy point
+                cv: find_group_hash(*b"arbitrar", b"a")
+                    .try_into()
+                    .expect("find_group_hash returns point in prime-order subgroup"),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -53,7 +57,10 @@ impl Arbitrary for Spend<SharedAnchor> {
         )
             .prop_map(|(nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor: FieldNotPresent,
-                cv: ValueCommitment(AffinePoint::identity()),
+                // Use an arbitrary string to generate a dummy point
+                cv: find_group_hash(*b"arbitrar", b"a")
+                    .try_into()
+                    .expect("find_group_hash returns point in prime-order subgroup"),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -79,7 +86,10 @@ impl Arbitrary for Output {
             any::<Groth16Proof>(),
         )
             .prop_map(|(enc_ciphertext, out_ciphertext, zkproof)| Self {
-                cv: ValueCommitment(AffinePoint::identity()),
+                // Use an arbitrary string to generate a dummy point
+                cv: find_group_hash(*b"arbitrar", b"a")
+                    .try_into()
+                    .expect("find_group_hash returns point in prime-order subgroup"),
                 cm_u: NoteCommitment(AffinePoint::identity()).extract_u(),
                 ephemeral_key: keys::EphemeralPublicKey(AffinePoint::identity()),
                 enc_ciphertext,
diff --git a/zebra-chain/src/sapling/commitment.rs b/zebra-chain/src/sapling/commitment.rs
index 9c232b3bbc6..be56d2bda8c 100644
--- a/zebra-chain/src/sapling/commitment.rs
+++ b/zebra-chain/src/sapling/commitment.rs
@@ -151,7 +151,7 @@ impl NoteCommitment {
 ///
 /// 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;
@@ -185,9 +185,18 @@ impl fmt::Debug for ValueCommitment {
     }
 }
 
-impl From<jubjub::ExtendedPoint> for ValueCommitment {
-    fn from(extended_point: jubjub::ExtendedPoint) -> Self {
-        Self(jubjub::AffinePoint::from(extended_point))
+impl TryFrom<jubjub::ExtendedPoint> for ValueCommitment {
+    type Error = &'static str;
+
+    /// Convert a JubJub point into a ValueCommitment.
+    ///
+    /// Returns an error if the point is of small order.
+    fn try_from(extended_point: jubjub::ExtendedPoint) -> Result<Self, Self::Error> {
+        if extended_point.is_small_order().into() {
+            Err("small order point")
+        } else {
+            Ok(Self(jubjub::AffinePoint::from(extended_point)))
+        }
     }
 }
 
@@ -248,7 +257,12 @@ 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();
+            if point.is_small_order().into() {
+                Err("small order point")
+            } else {
+                Ok(Self(possible_point.unwrap()))
+            }
         } else {
             Err("Invalid jubjub::AffinePoint value")
         }
@@ -293,7 +307,10 @@ impl ValueCommitment {
         let V = find_group_hash(*b"Zcash_cv", b"v");
         let R = find_group_hash(*b"Zcash_cv", b"r");
 
-        Self::from(V * v + R * rcv)
+        // find_group_hash() returns points in the prime-order subgroup,
+        // so we know this result is also in the same subgroup and we don't
+        // need to check the error.
+        Self::try_from(V * v + R * rcv).expect("is in prime-order subgroup")
     }
 }
 
diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs
index a20ba6b1b56..fa16bb990e8 100644
--- a/zebra-consensus/src/transaction/check.rs
+++ b/zebra-consensus/src/transaction/check.rs
@@ -133,13 +133,11 @@ pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), Tr
 ///
 /// 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(),
-        )
-    {
+    if bool::from(
+        jubjub::AffinePoint::from_bytes(spend.rk.into())
+            .unwrap()
+            .is_small_order(),
+    ) {
         Err(TransactionError::SmallOrder)
     } else {
         Ok(())
@@ -151,13 +149,11 @@ pub fn spend_cv_rk_not_small_order(spend: &Spend<PerSpendAnchor>) -> Result<(),
 ///
 /// 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(),
-        )
-    {
+    if bool::from(
+        jubjub::AffinePoint::from_bytes(output.ephemeral_key.into())
+            .unwrap()
+            .is_small_order(),
+    ) {
         Err(TransactionError::SmallOrder)
     } else {
         Ok(())

From 0356ef7b9ac308e09b0ac49c33921fa8d41edbf2 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Fri, 17 Dec 2021 14:15:15 -0300
Subject: [PATCH 04/16] Apply suggestions from code review

Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
---
 zebra-chain/src/sapling/keys.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index 305ed5d3718..445c89a8872 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -479,10 +479,10 @@ impl TryFrom<[u8; 32]> for AuthorizingKey {
     ///
     /// It must decode to a prime-order point:
     ///
-    /// > When decoding this representation, the key MUST be considered invalid
+    /// > 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)*}
     ///
-    /// https://zips.z.cash/protocol/protocol.pdf#saplingfullviewingkeyencoding
+    /// [MUST]: 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() {

From fc05f0a258b7c6daea41e3d3aa6d589ddd45d7aa Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Fri, 17 Dec 2021 14:16:11 -0300
Subject: [PATCH 05/16] Use is_torsion_free() instead of is_identity() and
 is_prime_order()

---
 zebra-chain/src/sapling/keys.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index 445c89a8872..52c945edfd5 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -873,7 +873,8 @@ impl TryFrom<[u8; 32]> for TransmissionKey {
     /// https://zips.z.cash/zip-0216
     fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
         let affine_point = jubjub::AffinePoint::from_bytes(bytes).unwrap();
-        if (affine_point.is_identity() | affine_point.is_prime_order()).into() {
+        // 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("derived an invalid Sapling transmission key")

From a0733690e031cefd340f0c6353d3a04d4bddbb0c Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Fri, 17 Dec 2021 14:58:11 -0300
Subject: [PATCH 06/16] Add EphemeralPublicKey small order check on
 instantiation; remove old checks

---
 zebra-chain/src/sapling/commitment.rs    |  9 ++++++++-
 zebra-chain/src/sapling/keys.rs          | 19 ++++++++++++++++---
 zebra-consensus/src/transaction.rs       |  7 -------
 zebra-consensus/src/transaction/check.rs | 18 +-----------------
 zebra-consensus/src/transaction/tests.rs |  3 ---
 5 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/zebra-chain/src/sapling/commitment.rs b/zebra-chain/src/sapling/commitment.rs
index be56d2bda8c..be070e42403 100644
--- a/zebra-chain/src/sapling/commitment.rs
+++ b/zebra-chain/src/sapling/commitment.rs
@@ -149,6 +149,8 @@ impl NoteCommitment {
 /// A Homomorphic Pedersen commitment to the value of a note, used in Spend and
 /// Output descriptions.
 ///
+/// This is denoted by `cv` in the specification.
+///
 /// https://zips.z.cash/protocol/protocol.pdf#concretehomomorphiccommit
 #[derive(Clone, Copy, Deserialize, PartialEq, Serialize)]
 pub struct ValueCommitment(#[serde(with = "serde_helpers::AffinePoint")] jubjub::AffinePoint);
@@ -190,7 +192,12 @@ impl TryFrom<jubjub::ExtendedPoint> for ValueCommitment {
 
     /// Convert a JubJub point into a ValueCommitment.
     ///
-    /// Returns an error if the point is of small order.
+    /// Returns an error if [the point 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(extended_point: jubjub::ExtendedPoint) -> Result<Self, Self::Error> {
         if extended_point.is_small_order().into() {
             Err("small order point")
diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index 52c945edfd5..21a1f3403eb 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -1002,6 +1002,8 @@ impl FromStr for FullViewingKey {
 
 /// An ephemeral public key for Sapling key agreement.
 ///
+/// It is denoted by `epk` in the specification.
+///
 /// https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
 #[derive(Copy, Clone, Deserialize, PartialEq, Serialize)]
 pub struct EphemeralPublicKey(
@@ -1040,13 +1042,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() {
-            Ok(Self(possible_point.unwrap()))
+        if possible_point.is_none().into() {
+            return Err("Invalid jubjub::AffinePoint value");
+        }
+        if possible_point.unwrap().is_small_order().into() {
+            Err("point has small order")
         } else {
-            Err("Invalid jubjub::AffinePoint value")
+            Ok(Self(possible_point.unwrap()))
         }
     }
 }
diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs
index 734521e6804..7dc02ec27a7 100644
--- a/zebra-consensus/src/transaction.rs
+++ b/zebra-consensus/src/transaction.rs
@@ -733,13 +733,6 @@ where
             }
 
             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.
diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs
index fa16bb990e8..f81d14bceb9 100644
--- a/zebra-consensus/src/transaction/check.rs
+++ b/zebra-consensus/src/transaction/check.rs
@@ -12,7 +12,7 @@ use zebra_chain::{
     orchard::Flags,
     parameters::{Network, NetworkUpgrade},
     primitives::zcash_note_encryption,
-    sapling::{Output, PerSpendAnchor, Spend},
+    sapling::{PerSpendAnchor, Spend},
     transaction::{LockTime, Transaction},
 };
 
@@ -144,22 +144,6 @@ pub fn spend_cv_rk_not_small_order(spend: &Spend<PerSpendAnchor>) -> Result<(),
     }
 }
 
-/// 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(
-        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.
 ///
diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs
index 8024deead46..ad977114c4a 100644
--- a/zebra-consensus/src/transaction/tests.rs
+++ b/zebra-consensus/src/transaction/tests.rs
@@ -56,9 +56,6 @@ fn v5_fake_transactions() -> Result<(), Report> {
                 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");
             }

From 08b9ec21d1d725d6601cd09308158d2f5ea618c2 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Fri, 17 Dec 2021 15:31:56 -0300
Subject: [PATCH 07/16] Use VerificationKey instead of VerificationKeyBytes;
 fix tests

---
 zebra-chain/src/sapling/arbitrary.rs | 11 +++++++---
 zebra-chain/src/sapling/keys.rs      |  2 +-
 zebra-chain/src/sapling/spend.rs     | 30 +++++++++++++++++++++-------
 zebra-consensus/src/transaction.rs   |  2 +-
 4 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/zebra-chain/src/sapling/arbitrary.rs b/zebra-chain/src/sapling/arbitrary.rs
index 560cdbd72dd..0a93582160b 100644
--- a/zebra-chain/src/sapling/arbitrary.rs
+++ b/zebra-chain/src/sapling/arbitrary.rs
@@ -91,7 +91,12 @@ impl Arbitrary for Output {
                     .try_into()
                     .expect("find_group_hash returns point in prime-order subgroup"),
                 cm_u: NoteCommitment(AffinePoint::identity()).extract_u(),
-                ephemeral_key: keys::EphemeralPublicKey(AffinePoint::identity()),
+                // Use an arbitrary string to generate a dummy point
+                ephemeral_key: keys::EphemeralPublicKey(
+                    find_group_hash(*b"arbitrar", b"b")
+                        .try_into()
+                        .expect("find_group_hash returns point in prime-order subgroup"),
+                ),
                 enc_ciphertext,
                 out_ciphertext,
                 zkproof,
@@ -115,11 +120,11 @@ 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(
-) -> impl Strategy<Value = redjubjub::VerificationKeyBytes<redjubjub::SpendAuth>> {
+) -> impl Strategy<Value = redjubjub::VerificationKey<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)
     })
 }
 
diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index 21a1f3403eb..e8882280686 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -1057,7 +1057,7 @@ impl TryFrom<[u8; 32]> for EphemeralPublicKey {
             return Err("Invalid jubjub::AffinePoint value");
         }
         if possible_point.unwrap().is_small_order().into() {
-            Err("point has small order")
+            Err("EphemeralPublicKey point has small order")
         } else {
             Ok(Self(possible_point.unwrap()))
         }
diff --git a/zebra-chain/src/sapling/spend.rs b/zebra-chain/src/sapling/spend.rs
index 830f29a0980..d8fa5bd97e1 100644
--- a/zebra-chain/src/sapling/spend.rs
+++ b/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,
@@ -31,7 +31,7 @@ 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,
@@ -48,7 +48,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: redjubjub::VerificationKey<SpendAuth>,
     /// The ZK spend proof.
     pub zkproof: Groth16Proof,
     /// A signature authorizing this spend.
@@ -63,16 +63,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,
     /// 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: redjubjub::VerificationKey<SpendAuth>,
 }
 
+// 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,
@@ -165,7 +173,11 @@ impl ZcashDeserialize for Spend<PerSpendAnchor> {
             cv: commitment::ValueCommitment::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: redjubjub::VerificationKeyBytes::<SpendAuth>::from(reader.read_32_bytes()?)
+                .try_into()
+                .map_err(|_: redjubjub::Error| {
+                    SerializationError::Parse("malformed verification key")
+                })?,
             zkproof: Groth16Proof::zcash_deserialize(&mut reader)?,
             spend_auth_sig: reader.read_64_bytes()?.into(),
         })
@@ -192,7 +204,11 @@ impl ZcashDeserialize for SpendPrefixInTransactionV5 {
         Ok(SpendPrefixInTransactionV5 {
             cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
             nullifier: note::Nullifier::from(reader.read_32_bytes()?),
-            rk: reader.read_32_bytes()?.into(),
+            rk: redjubjub::VerificationKeyBytes::<SpendAuth>::from(reader.read_32_bytes()?)
+                .try_into()
+                .map_err(|_: redjubjub::Error| {
+                    SerializationError::Parse("malformed verification key")
+                })?,
         })
     }
 }
diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs
index 7dc02ec27a7..e4a64f62350 100644
--- a/zebra-consensus/src/transaction.rs
+++ b/zebra-consensus/src/transaction.rs
@@ -728,7 +728,7 @@ 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()),
                 );
             }
 

From 4dd57af877e21886c220c80626654a253927aae4 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Fri, 17 Dec 2021 16:42:11 -0300
Subject: [PATCH 08/16] Use ValidatingKey instead of VerificationKeyBytes for
 rk

---
 zebra-chain/src/sapling/arbitrary.rs      |  7 +--
 zebra-chain/src/sapling/keys.rs           | 56 ++++++++++++++++++++++-
 zebra-chain/src/sapling/spend.rs          | 27 +++++------
 zebra-consensus/src/primitives/groth16.rs |  2 +-
 zebra-consensus/src/transaction.rs        |  8 +---
 zebra-consensus/src/transaction/check.rs  | 17 -------
 zebra-consensus/src/transaction/tests.rs  |  9 ----
 7 files changed, 75 insertions(+), 51 deletions(-)

diff --git a/zebra-chain/src/sapling/arbitrary.rs b/zebra-chain/src/sapling/arbitrary.rs
index 0a93582160b..09227f43914 100644
--- a/zebra-chain/src/sapling/arbitrary.rs
+++ b/zebra-chain/src/sapling/arbitrary.rs
@@ -8,7 +8,7 @@ use rand_chacha::ChaChaRng;
 use crate::primitives::Groth16Proof;
 
 use super::{
-    keys::{self, find_group_hash},
+    keys::{self, find_group_hash, ValidatingKey},
     note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4, PerSpendAnchor,
     SharedAnchor, Spend,
 };
@@ -119,12 +119,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(
-) -> impl Strategy<Value = redjubjub::VerificationKey<redjubjub::SpendAuth>> {
+fn spendauth_verification_key_bytes() -> impl Strategy<Value = ValidatingKey> {
     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)
+            .try_into()
+            .unwrap()
     })
 }
 
diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index e8882280686..ef0b173d962 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/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,
@@ -1076,3 +1076,57 @@ impl ZcashDeserialize for EphemeralPublicKey {
         Self::try_from(reader.read_32_bytes()?).map_err(SerializationError::Parse)
     }
 }
+
+/// A randomized [validating][1] key that should be used to validate `spendAuthSig`.
+///
+/// It is denoted by `rk` in the specification.
+///
+/// [1]: https://zips.z.cash/protocol/protocol.pdf#spenddesc
+#[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("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(|_| "malformed 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()
+    }
+}
diff --git a/zebra-chain/src/sapling/spend.rs b/zebra-chain/src/sapling/spend.rs
index d8fa5bd97e1..d20924b0729 100644
--- a/zebra-chain/src/sapling/spend.rs
+++ b/zebra-chain/src/sapling/spend.rs
@@ -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].
 ///
@@ -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::VerificationKey<SpendAuth>,
+    pub rk: ValidatingKey,
     /// The ZK spend proof.
     pub zkproof: Groth16Proof,
     /// A signature authorizing this spend.
@@ -70,7 +73,7 @@ pub struct SpendPrefixInTransactionV5 {
     /// The nullifier of the input note.
     pub nullifier: note::Nullifier,
     /// The randomized public key for `spend_auth_sig`.
-    pub rk: redjubjub::VerificationKey<SpendAuth>,
+    pub rk: ValidatingKey,
 }
 
 // We can't derive Eq because `VerificationKey` does not implement it,
@@ -160,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(())
@@ -173,11 +176,10 @@ impl ZcashDeserialize for Spend<PerSpendAnchor> {
             cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
             per_spend_anchor: tree::Root(reader.read_32_bytes()?),
             nullifier: note::Nullifier::from(reader.read_32_bytes()?),
-            rk: redjubjub::VerificationKeyBytes::<SpendAuth>::from(reader.read_32_bytes()?)
+            rk: reader
+                .read_32_bytes()?
                 .try_into()
-                .map_err(|_: redjubjub::Error| {
-                    SerializationError::Parse("malformed verification key")
-                })?,
+                .map_err(SerializationError::Parse)?,
             zkproof: Groth16Proof::zcash_deserialize(&mut reader)?,
             spend_auth_sig: reader.read_64_bytes()?.into(),
         })
@@ -194,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(())
     }
 }
@@ -204,11 +206,10 @@ impl ZcashDeserialize for SpendPrefixInTransactionV5 {
         Ok(SpendPrefixInTransactionV5 {
             cv: commitment::ValueCommitment::zcash_deserialize(&mut reader)?,
             nullifier: note::Nullifier::from(reader.read_32_bytes()?),
-            rk: redjubjub::VerificationKeyBytes::<SpendAuth>::from(reader.read_32_bytes()?)
+            rk: reader
+                .read_32_bytes()?
                 .try_into()
-                .map_err(|_: redjubjub::Error| {
-                    SerializationError::Parse("malformed verification key")
-                })?,
+                .map_err(SerializationError::Parse)?,
         })
     }
 }
diff --git a/zebra-consensus/src/primitives/groth16.rs b/zebra-consensus/src/primitives/groth16.rs
index 5e7b4fa497b..cecb7e41b72 100644
--- a/zebra-consensus/src/primitives/groth16.rs
+++ b/zebra-consensus/src/primitives/groth16.rs
@@ -162,7 +162,7 @@ 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());
 
diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs
index e4a64f62350..2608a6db3a1 100644
--- a/zebra-consensus/src/transaction.rs
+++ b/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,6 +721,7 @@ where
                 async_checks.push(
                     primitives::redjubjub::VERIFIER
                         .clone()
+                        // TODO: instead of converting rk, change the verifier to receive the correct type
                         .oneshot((spend.rk.into(), spend.spend_auth_sig, shielded_sighash).into()),
                 );
             }
diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs
index f81d14bceb9..c7d1e74625f 100644
--- a/zebra-consensus/src/transaction/check.rs
+++ b/zebra-consensus/src/transaction/check.rs
@@ -12,7 +12,6 @@ use zebra_chain::{
     orchard::Flags,
     parameters::{Network, NetworkUpgrade},
     primitives::zcash_note_encryption,
-    sapling::{PerSpendAnchor, Spend},
     transaction::{LockTime, Transaction},
 };
 
@@ -128,22 +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(
-        jubjub::AffinePoint::from_bytes(spend.rk.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.
 ///
diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs
index ad977114c4a..5b5c5c50f6a 100644
--- a/zebra-consensus/src/transaction/tests.rs
+++ b/zebra-consensus/src/transaction/tests.rs
@@ -50,15 +50,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)?;
-                }
-            } else {
-                panic!("we should have no tx other than 5");
-            }
         }
     }
 

From 8542768aeb7460a4977c43f7f93e24a56929d7b7 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Fri, 17 Dec 2021 17:45:08 -0300
Subject: [PATCH 09/16] Reject identity when creating an Orchard
 EphemeralPublicKey

---
 zebra-chain/src/orchard/arbitrary.rs |  2 +-
 zebra-chain/src/orchard/keys.rs      | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/zebra-chain/src/orchard/arbitrary.rs b/zebra-chain/src/orchard/arbitrary.rs
index 5bb60e3dc1e..30ac3a530a5 100644
--- a/zebra-chain/src/orchard/arbitrary.rs
+++ b/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,
             })
diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs
index 73842b6de88..cc77dbcfc8e 100644
--- a/zebra-chain/src/orchard/keys.rs
+++ b/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,23 @@ 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 point is malformed or [if it is the identity][1].
+    ///
+    /// > epk cannot be 𝒪_P
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#actiondesc
     fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
         let possible_point = pallas::Affine::from_bytes(&bytes);
 
         if possible_point.is_some().into() {
-            Ok(Self(possible_point.unwrap()))
+            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")
         }

From 5ccb38a554dd87b282c7bcb5381a7d68bdfa8e22 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Fri, 17 Dec 2021 18:27:47 -0300
Subject: [PATCH 10/16] Make documentation more consistent, use generator in
 tests

---
 zebra-chain/src/orchard/keys.rs       |  3 ++-
 zebra-chain/src/sapling/arbitrary.rs  | 25 +++++++------------------
 zebra-chain/src/sapling/commitment.rs | 14 +++++++-------
 zebra-chain/src/sapling/keys.rs       | 25 +++++++++++++------------
 zebra-consensus/src/transaction.rs    |  1 -
 5 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs
index cc77dbcfc8e..fa4ea15a3e8 100644
--- a/zebra-chain/src/orchard/keys.rs
+++ b/zebra-chain/src/orchard/keys.rs
@@ -1087,7 +1087,8 @@ impl TryFrom<[u8; 32]> for EphemeralPublicKey {
 
     /// Convert an array into a EphemeralPublicKey.
     ///
-    /// Returns an error if the point is malformed or [if it is the identity][1].
+    /// Returns an error if the encoding is malformed or if [it encodes the
+    /// identity point][1].
     ///
     /// > epk cannot be 𝒪_P
     ///
diff --git a/zebra-chain/src/sapling/arbitrary.rs b/zebra-chain/src/sapling/arbitrary.rs
index 09227f43914..923f49f4deb 100644
--- a/zebra-chain/src/sapling/arbitrary.rs
+++ b/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,7 +9,7 @@ use rand_chacha::ChaChaRng;
 use crate::primitives::Groth16Proof;
 
 use super::{
-    keys::{self, find_group_hash, ValidatingKey},
+    keys::{self, ValidatingKey},
     note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4, PerSpendAnchor,
     SharedAnchor, Spend,
 };
@@ -26,10 +27,7 @@ impl Arbitrary for Spend<PerSpendAnchor> {
         )
             .prop_map(|(per_spend_anchor, nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor,
-                // Use an arbitrary string to generate a dummy point
-                cv: find_group_hash(*b"arbitrar", b"a")
-                    .try_into()
-                    .expect("find_group_hash returns point in prime-order subgroup"),
+                cv: ExtendedPoint::generator().try_into().unwrap(),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -57,10 +55,7 @@ impl Arbitrary for Spend<SharedAnchor> {
         )
             .prop_map(|(nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor: FieldNotPresent,
-                // Use an arbitrary string to generate a dummy point
-                cv: find_group_hash(*b"arbitrar", b"a")
-                    .try_into()
-                    .expect("find_group_hash returns point in prime-order subgroup"),
+                cv: ExtendedPoint::generator().try_into().unwrap(),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -86,16 +81,10 @@ impl Arbitrary for Output {
             any::<Groth16Proof>(),
         )
             .prop_map(|(enc_ciphertext, out_ciphertext, zkproof)| Self {
-                // Use an arbitrary string to generate a dummy point
-                cv: find_group_hash(*b"arbitrar", b"a")
-                    .try_into()
-                    .expect("find_group_hash returns point in prime-order subgroup"),
+                cv: ExtendedPoint::generator().try_into().unwrap(),
                 cm_u: NoteCommitment(AffinePoint::identity()).extract_u(),
-                // Use an arbitrary string to generate a dummy point
                 ephemeral_key: keys::EphemeralPublicKey(
-                    find_group_hash(*b"arbitrar", b"b")
-                        .try_into()
-                        .expect("find_group_hash returns point in prime-order subgroup"),
+                    ExtendedPoint::generator().try_into().unwrap(),
                 ),
                 enc_ciphertext,
                 out_ciphertext,
diff --git a/zebra-chain/src/sapling/commitment.rs b/zebra-chain/src/sapling/commitment.rs
index be070e42403..8727be995da 100644
--- a/zebra-chain/src/sapling/commitment.rs
+++ b/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::{
@@ -200,7 +204,7 @@ impl TryFrom<jubjub::ExtendedPoint> for ValueCommitment {
     /// [1]: https://zips.z.cash/protocol/protocol.pdf#spenddesc
     fn try_from(extended_point: jubjub::ExtendedPoint) -> Result<Self, Self::Error> {
         if extended_point.is_small_order().into() {
-            Err("small order point")
+            Err("jubjub::AffinePoint value for Sapling ValueCommitment is of small order")
         } else {
             Ok(Self(jubjub::AffinePoint::from(extended_point)))
         }
@@ -265,11 +269,7 @@ impl TryFrom<[u8; 32]> for ValueCommitment {
 
         if possible_point.is_some().into() {
             let point = possible_point.unwrap();
-            if point.is_small_order().into() {
-                Err("small order point")
-            } else {
-                Ok(Self(possible_point.unwrap()))
-            }
+            ExtendedPoint::from(point).try_into()
         } else {
             Err("Invalid jubjub::AffinePoint value")
         }
diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index ef0b173d962..8db4fc6c840 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -475,25 +475,26 @@ impl Eq for AuthorizingKey {}
 impl TryFrom<[u8; 32]> for AuthorizingKey {
     type Error = &'static str;
 
-    /// Decode an AuthorizingKey from a byte array.
+    /// Convert an array into an AuthorizingKey.
     ///
-    /// It must decode to a prime-order point:
+    /// 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
+    /// > 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)*}
     ///
-    /// [MUST]: https://zips.z.cash/protocol/protocol.pdf#saplingfullviewingkeyencoding
+    /// [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("derived an invalid Sapling transmission key");
+            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| "derived an invalid Sapling transmission key",
+                |_e| "Invalid jubjub::AffinePoint for Sapling AuthorizingKey",
             )?))
         } else {
-            Err("derived an invalid Sapling transmission key")
+            Err("jubjub::AffinePoint value for Sapling AuthorizingKey is not of prime order")
         }
     }
 }
@@ -877,7 +878,7 @@ impl TryFrom<[u8; 32]> for TransmissionKey {
         if affine_point.is_torsion_free().into() {
             Ok(Self(affine_point))
         } else {
-            Err("derived an invalid Sapling transmission key")
+            Err("Invalid jubjub::AffinePoint value for Sapling TransmissionKey")
         }
     }
 }
@@ -1054,10 +1055,10 @@ impl TryFrom<[u8; 32]> for EphemeralPublicKey {
         let possible_point = jubjub::AffinePoint::from_bytes(bytes);
 
         if possible_point.is_none().into() {
-            return Err("Invalid jubjub::AffinePoint value");
+            return Err("Invalid jubjub::AffinePoint value for Sapling EphemeralPublicKey");
         }
         if possible_point.unwrap().is_small_order().into() {
-            Err("EphemeralPublicKey point has small order")
+            Err("jubjub::AffinePoint value for Sapling EphemeralPublicKey point is of small order")
         } else {
             Ok(Self(possible_point.unwrap()))
         }
@@ -1102,7 +1103,7 @@ impl TryFrom<redjubjub::VerificationKey<SpendAuth>> for ValidatingKey {
                 .unwrap()
                 .is_small_order(),
         ) {
-            Err("ValidatingKey is of small order")
+            Err("jubjub::AffinePoint value for Sapling ValidatingKey is of small order")
         } else {
             Ok(Self(key))
         }
@@ -1114,7 +1115,7 @@ impl TryFrom<[u8; 32]> for ValidatingKey {
 
     fn try_from(value: [u8; 32]) -> Result<Self, Self::Error> {
         let vk = redjubjub::VerificationKey::<SpendAuth>::try_from(value)
-            .map_err(|_| "malformed ValidatingKey")?;
+            .map_err(|_| "Invalid redjubjub::ValidatingKey for Sapling ValidatingKey")?;
         vk.try_into()
     }
 }
diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs
index 2608a6db3a1..96614bd4325 100644
--- a/zebra-consensus/src/transaction.rs
+++ b/zebra-consensus/src/transaction.rs
@@ -721,7 +721,6 @@ where
                 async_checks.push(
                     primitives::redjubjub::VERIFIER
                         .clone()
-                        // TODO: instead of converting rk, change the verifier to receive the correct type
                         .oneshot((spend.rk.into(), spend.spend_auth_sig, shielded_sighash).into()),
                 );
             }

From 4aeff17d840152151be051a75f4a1b8adede8889 Mon Sep 17 00:00:00 2001
From: Deirdre Connolly <deirdre@zfnd.org>
Date: Fri, 17 Dec 2021 18:49:58 -0500
Subject: [PATCH 11/16] s/JubJub/Jubjub/

---
 zebra-chain/src/sapling/commitment.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/zebra-chain/src/sapling/commitment.rs b/zebra-chain/src/sapling/commitment.rs
index 8727be995da..c6851a10590 100644
--- a/zebra-chain/src/sapling/commitment.rs
+++ b/zebra-chain/src/sapling/commitment.rs
@@ -194,7 +194,7 @@ impl fmt::Debug for ValueCommitment {
 impl TryFrom<jubjub::ExtendedPoint> for ValueCommitment {
     type Error = &'static str;
 
-    /// Convert a JubJub point into a ValueCommitment.
+    /// Convert a Jubjub point into a ValueCommitment.
     ///
     /// Returns an error if [the point is of small order][1].
     ///

From ac73bce8731b019abab2c894bfd1e2a895e60a87 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Mon, 20 Dec 2021 10:59:48 -0300
Subject: [PATCH 12/16] Fix zebra-consensus tests
 (insert_fake_orchard_shielded_data)

---
 zebra-chain/src/transaction/arbitrary.rs | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/zebra-chain/src/transaction/arbitrary.rs b/zebra-chain/src/transaction/arbitrary.rs
index 356329fae59..f8fb51443eb 100644
--- a/zebra-chain/src/transaction/arbitrary.rs
+++ b/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
-    let dummy_action_bytes = [0u8; 820];
-    let mut dummy_action_bytes_reader = &dummy_action_bytes[..];
-    let dummy_action = orchard::Action::zcash_deserialize(&mut dummy_action_bytes_reader)
-        .expect("Dummy action should deserialize");
+    // Create a dummy action
+    let mut runner = TestRunner::default();
+    let dummy_action = orchard::Action::arbitrary()
+        .new_tree(&mut runner)
+        .unwrap()
+        .current();
 
     // Pair the dummy action with a fake signature
     let dummy_authorized_action = orchard::AuthorizedAction {

From b8760cea93e5336853b879e62c67be832dfac588 Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Mon, 20 Dec 2021 14:16:55 -0300
Subject: [PATCH 13/16] Create NotSmallOrderValueCommitment, since intermediate
 values can be the identity

---
 zebra-chain/src/sapling/commitment.rs     | 118 +++++++++++++++-------
 zebra-chain/src/sapling/output.rs         |   8 +-
 zebra-chain/src/sapling/shielded_data.rs  |   4 +-
 zebra-chain/src/sapling/spend.rs          |   8 +-
 zebra-consensus/src/primitives/groth16.rs |   4 +-
 5 files changed, 93 insertions(+), 49 deletions(-)

diff --git a/zebra-chain/src/sapling/commitment.rs b/zebra-chain/src/sapling/commitment.rs
index c6851a10590..d0924b8ff7d 100644
--- a/zebra-chain/src/sapling/commitment.rs
+++ b/zebra-chain/src/sapling/commitment.rs
@@ -150,10 +150,11 @@ impl NoteCommitment {
     }
 }
 
-/// A Homomorphic Pedersen commitment to the value of a note, used in Spend and
-/// Output descriptions.
+/// A Homomorphic Pedersen commitment to the value of a note.
 ///
-/// This is denoted by `cv` in the specification.
+/// 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)]
@@ -191,23 +192,10 @@ impl fmt::Debug for ValueCommitment {
     }
 }
 
-impl TryFrom<jubjub::ExtendedPoint> for ValueCommitment {
-    type Error = &'static str;
-
+impl From<jubjub::ExtendedPoint> for ValueCommitment {
     /// Convert a Jubjub point into a ValueCommitment.
-    ///
-    /// Returns an error if [the point 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(extended_point: jubjub::ExtendedPoint) -> Result<Self, Self::Error> {
-        if extended_point.is_small_order().into() {
-            Err("jubjub::AffinePoint value for Sapling ValueCommitment is of small order")
-        } else {
-            Ok(Self(jubjub::AffinePoint::from(extended_point)))
-        }
+    fn from(extended_point: jubjub::ExtendedPoint) -> Self {
+        Self(jubjub::AffinePoint::from(extended_point))
     }
 }
 
@@ -269,26 +257,13 @@ impl TryFrom<[u8; 32]> for ValueCommitment {
 
         if possible_point.is_some().into() {
             let point = possible_point.unwrap();
-            ExtendedPoint::from(point).try_into()
+            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_.
     ///
@@ -314,10 +289,79 @@ impl ValueCommitment {
         let V = find_group_hash(*b"Zcash_cv", b"v");
         let R = find_group_hash(*b"Zcash_cv", b"r");
 
-        // find_group_hash() returns points in the prime-order subgroup,
-        // so we know this result is also in the same subgroup and we don't
-        // need to check the error.
-        Self::try_from(V * v + R * rcv).expect("is in prime-order subgroup")
+        Self::from(V * v + R * rcv)
+    }
+}
+
+/// 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 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)
     }
 }
 
diff --git a/zebra-chain/src/sapling/output.rs b/zebra-chain/src/sapling/output.rs
index 44597d99c93..2535a3d3a01 100644
--- a/zebra-chain/src/sapling/output.rs
+++ b/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)?,
diff --git a/zebra-chain/src/sapling/shielded_data.rs b/zebra-chain/src/sapling/shielded_data.rs
index 6eee9bce85a..c516b58caf3 100644
--- a/zebra-chain/src/sapling/shielded_data.rs
+++ b/zebra-chain/src/sapling/shielded_data.rs
@@ -291,8 +291,8 @@ where
     ///
     /// 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_new: ValueCommitment = self.outputs().map(|output| output.cv).sum();
+        let cv_old: ValueCommitment = self.spends().map(|spend| spend.cv.into()).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);
 
diff --git a/zebra-chain/src/sapling/spend.rs b/zebra-chain/src/sapling/spend.rs
index d20924b0729..d0f1e373ea4 100644
--- a/zebra-chain/src/sapling/spend.rs
+++ b/zebra-chain/src/sapling/spend.rs
@@ -37,7 +37,7 @@ use super::{
 #[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
@@ -69,7 +69,7 @@ pub struct Spend<AnchorV: AnchorVariant> {
 #[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`.
@@ -173,7 +173,7 @@ 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
@@ -204,7 +204,7 @@ 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()?
diff --git a/zebra-consensus/src/primitives/groth16.rs b/zebra-consensus/src/primitives/groth16.rs
index cecb7e41b72..a1fc3ab917a 100644
--- a/zebra-consensus/src/primitives/groth16.rs
+++ b/zebra-consensus/src/primitives/groth16.rs
@@ -166,7 +166,7 @@ impl Description for Spend<PerSpendAnchor> {
         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());
 

From 0531b5fac215babd7f974db5fbf3d7323786940e Mon Sep 17 00:00:00 2001
From: Conrado Gouvea <conrado@zfnd.org>
Date: Mon, 20 Dec 2021 15:13:40 -0300
Subject: [PATCH 14/16] Clarify documentation

---
 zebra-chain/src/orchard/keys.rs       |  6 ++++++
 zebra-chain/src/sapling/commitment.rs |  4 +++-
 zebra-chain/src/sapling/keys.rs       | 18 +++++++++++++-----
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs
index fa4ea15a3e8..539ceb014f1 100644
--- a/zebra-chain/src/orchard/keys.rs
+++ b/zebra-chain/src/orchard/keys.rs
@@ -1092,7 +1092,13 @@ impl TryFrom<[u8; 32]> for EphemeralPublicKey {
     ///
     /// > 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);
 
diff --git a/zebra-chain/src/sapling/commitment.rs b/zebra-chain/src/sapling/commitment.rs
index d0924b8ff7d..1952a522897 100644
--- a/zebra-chain/src/sapling/commitment.rs
+++ b/zebra-chain/src/sapling/commitment.rs
@@ -296,7 +296,9 @@ 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.
+/// 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.
 ///
diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs
index 8db4fc6c840..e8808f1136b 100644
--- a/zebra-chain/src/sapling/keys.rs
+++ b/zebra-chain/src/sapling/keys.rs
@@ -1001,11 +1001,16 @@ impl FromStr for FullViewingKey {
     }
 }
 
-/// An ephemeral public key for Sapling key agreement.
+/// An [ephemeral public key][1] for Sapling key agreement.
 ///
-/// It is denoted by `epk` in the specification.
+/// Public keys containing points of small order are not allowed.
 ///
-/// https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
+/// 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,
@@ -1078,11 +1083,14 @@ impl ZcashDeserialize for EphemeralPublicKey {
     }
 }
 
-/// A randomized [validating][1] key that should be used to validate `spendAuthSig`.
+/// A randomized [validating key][1] that should be used to validate `spendAuthSig`.
 ///
-/// It is denoted by `rk` in the specification.
+/// 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>);
 

From e20d15ce98bfa373be4c2d7bf50712f4cbcb2069 Mon Sep 17 00:00:00 2001
From: Deirdre Connolly <deirdre@zfnd.org>
Date: Tue, 21 Dec 2021 16:18:53 -0500
Subject: [PATCH 15/16] rustdoc

---
 zebra-chain/src/orchard/keys.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs
index 539ceb014f1..b9ba4d3494d 100644
--- a/zebra-chain/src/orchard/keys.rs
+++ b/zebra-chain/src/orchard/keys.rs
@@ -1085,7 +1085,7 @@ impl PartialEq<[u8; 32]> for EphemeralPublicKey {
 impl TryFrom<[u8; 32]> for EphemeralPublicKey {
     type Error = &'static str;
 
-    /// Convert an array into a EphemeralPublicKey.
+    /// Convert an array into a [`EphemeralPublicKey`].
     ///
     /// Returns an error if the encoding is malformed or if [it encodes the
     /// identity point][1].

From 7e82c22463a458f85318fa13e6b0cee8811ce5bc Mon Sep 17 00:00:00 2001
From: Deirdre Connolly <deirdre@zfnd.org>
Date: Tue, 21 Dec 2021 18:58:47 -0500
Subject: [PATCH 16/16] Tidy rustdoc

---
 zebra-chain/src/sapling/shielded_data.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/zebra-chain/src/sapling/shielded_data.rs b/zebra-chain/src/sapling/shielded_data.rs
index c516b58caf3..b35ad0634a9 100644
--- a/zebra-chain/src/sapling/shielded_data.rs
+++ b/zebra-chain/src/sapling/shielded_data.rs
@@ -289,7 +289,7 @@ 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.into()).sum();
         let cv_new: ValueCommitment = self.outputs().map(|output| output.cv.into()).sum();