Integrate JoinSplit verifier (#3180)

* Integrate JoinSplit verifier with transaction verifier

* Add test with malformed Groth16 Output proof

* Use TryFrom instead of From in ItemWrapper to correctly propagate malformed proof errors

* Simplify by removing ItemWrapper and directly TryFrom into Item

* Fix existing tests to work with JoinSplit validation

* Apply suggestions from code review

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

Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
Co-authored-by: Pili Guerra <mpguerra@users.noreply.github.com>

zebra-chain/src/transaction.rs

@@ -1020,7 +1020,7 @@ impl Transaction {
     }
 
     /// Returns the `vpub_old` fields from `JoinSplit`s in this transaction,
-    /// regardless of version.
+    /// regardless of version, in the order they appear in the transaction.
     ///
     /// These values are added to the sprout chain value pool,
     /// and removed from the value pool of this transaction.
@@ -1067,7 +1067,7 @@ impl Transaction {
     }
 
     /// Modify the `vpub_old` fields from `JoinSplit`s in this transaction,
-    /// regardless of version.
+    /// regardless of version, in the order they appear in the transaction.
     ///
     /// See `output_values_to_sprout` for details.
     #[cfg(any(test, feature = "proptest-impl"))]
@@ -1116,7 +1116,7 @@ impl Transaction {
     }
 
     /// Returns the `vpub_new` fields from `JoinSplit`s in this transaction,
-    /// regardless of version.
+    /// regardless of version, in the order they appear in the transaction.
     ///
     /// These values are removed from the value pool of this transaction.
     /// and added to the sprout chain value pool.
@@ -1163,7 +1163,7 @@ impl Transaction {
     }
 
     /// Modify the `vpub_new` fields from `JoinSplit`s in this transaction,
-    /// regardless of version.
+    /// regardless of version, in the order they appear in the transaction.
     ///
     /// See `input_values_from_sprout` for details.
     #[cfg(any(test, feature = "proptest-impl"))]

zebra-consensus/src/error.rs

@@ -130,10 +130,14 @@ pub enum TransactionError {
     #[error("spend description cv and rk MUST NOT be of small order")]
     SmallOrder,
 
-    // XXX change this when we align groth16 verifier errors with bellman
+    // XXX: the underlying error is bellman::VerificationError, but it does not implement
-    // and add a from annotation when the error type is more precise
+    // Arbitrary as required here.
     #[error("spend proof MUST be valid given a primary input formed from the other fields except spendAuthSig")]
-    Groth16,
+    Groth16(String),
+
+    // XXX: the underlying error is io::Error, but it does not implement Clone as required here.
+    #[error("Groth16 proof is malformed")]
+    MalformedGroth16(String),
 
     #[error(
         "Sprout joinSplitSig MUST represent a valid signature under joinSplitPubKey of dataToBeSigned"
@@ -153,6 +157,9 @@ pub enum TransactionError {
     #[error("Downcast from BoxError to redjubjub::Error failed")]
     InternalDowncastError(String),
 
+    #[error("either vpub_old or vpub_new must be zero")]
+    BothVPubsNonZero,
+
     #[error("adding to the sprout pool is disabled after Canopy")]
     DisabledAddToSproutPool,
 

zebra-consensus/src/primitives/groth16.rs

@@ -1,7 +1,8 @@
 //! Async Groth16 batch verifier service
 
 use std::{
-    convert::TryInto,
+    convert::{TryFrom, TryInto},
+    error::Error,
     fmt,
     future::Future,
     mem,
@@ -22,7 +23,7 @@ use tokio::sync::broadcast::{channel, error::RecvError, Sender};
 use tower::{util::ServiceFn, Service};
 
 use tower_batch::{Batch, BatchControl};
-use tower_fallback::Fallback;
+use tower_fallback::{BoxedError, Fallback};
 
 use zebra_chain::{
     primitives::{
@@ -41,6 +42,8 @@ mod vectors;
 
 pub use params::{Groth16Parameters, GROTH16_PARAMETERS};
 
+use crate::error::TransactionError;
+
 /// Global batch verification context for Groth16 proofs of Spend statements.
 ///
 /// This service transparently batches contemporaneous proof verifications,
@@ -115,7 +118,7 @@ pub static OUTPUT_VERIFIER: Lazy<
 /// Note that making a `Service` call requires mutable access to the service, so
 /// you should call `.clone()` on the global handle to create a local, mutable
 /// handle.
-pub static JOINSPLIT_VERIFIER: Lazy<ServiceFn<fn(Item) -> Ready<Result<(), VerificationError>>>> =
+pub static JOINSPLIT_VERIFIER: Lazy<ServiceFn<fn(Item) -> Ready<Result<(), BoxedError>>>> =
     Lazy::new(|| {
         // We need a Service to use. The obvious way to do this would
         // be to write a closure that returns an async block. But because we
@@ -126,19 +129,19 @@ pub static JOINSPLIT_VERIFIER: Lazy<ServiceFn<fn(Item) -> Ready<Result<(), Verif
         // function (which is possible because it doesn't capture any state).
         tower::service_fn(
             (|item: Item| {
+                // Workaround bug in `bellman::VerificationError` fmt::Display
+                // implementation https://github.com/zkcrypto/bellman/pull/77
+                #[allow(deprecated)]
                 ready(
-                    item.verify_single(&GROTH16_PARAMETERS.sprout.joinsplit_prepared_verifying_key),
+                    item.verify_single(&GROTH16_PARAMETERS.sprout.joinsplit_prepared_verifying_key)
+                        // When that is fixed, change to `e.to_string()`
+                        .map_err(|e| TransactionError::Groth16(e.description().to_string()))
+                        .map_err(tower_fallback::BoxedError::from),
                 )
             }) as fn(_) -> _,
         )
     });
 
-/// A Groth16 verification item, used as the request type of the service.
-pub type Item = batch::Item<Bls12>;
-
-/// A wrapper to workaround the missing `ServiceExt::map_err` method.
-pub struct ItemWrapper(Item);
-
 /// A Groth16 Description (JoinSplit, Spend, or Output) with a Groth16 proof
 /// and its inputs encoded as scalars.
 pub trait Description {
@@ -296,22 +299,26 @@ impl Description for (&JoinSplit<Groth16Proof>, &ed25519::VerificationKeyBytes)
     }
 }
 
-impl<T> From<&T> for ItemWrapper
+/// A Groth16 verification item, used as the request type of the service.
+pub type Item = batch::Item<Bls12>;
+
+/// A wrapper to allow a TryFrom blanket implementation of the [`Description`]
+/// trait for the [`Item`] struct.
+/// See https://github.com/rust-lang/rust/issues/50133 for more details.
+pub struct DescriptionWrapper<T>(pub T);
+
+impl<T> TryFrom<DescriptionWrapper<&T>> for Item
 where
     T: Description,
 {
-    /// Convert a [`Description`] into an [`ItemWrapper`].
+    type Error = TransactionError;
-    fn from(input: &T) -> Self {
-        Self(Item::from((
-            bellman::groth16::Proof::read(&input.proof().0[..]).unwrap(),
-            input.primary_inputs(),
-        )))
-    }
-}
 
-impl From<ItemWrapper> for Item {
+    fn try_from(input: DescriptionWrapper<&T>) -> Result<Self, Self::Error> {
-    fn from(item_wrapper: ItemWrapper) -> Self {
+        Ok(Item::from((
-        item_wrapper.0
+            bellman::groth16::Proof::read(&input.0.proof().0[..])
+                .map_err(|e| TransactionError::MalformedGroth16(e.to_string()))?,
+            input.0.primary_inputs(),
+        )))
     }
 }
 

zebra-consensus/src/primitives/groth16/tests.rs

@@ -11,7 +11,7 @@ use zebra_chain::{
     transaction::Transaction,
 };
 
-use crate::primitives::groth16::{self, *};
+use crate::primitives::groth16::*;
 
 async fn verify_groth16_spends_and_outputs<V>(
     spend_verifier: &mut V,
@@ -37,10 +37,11 @@ where
         for spend in spends {
             tracing::trace!(?spend);
 
-            let spend_rsp = spend_verifier
+            let spend_rsp = spend_verifier.ready().await?.call(
-                .ready()
+                DescriptionWrapper(&spend)
-                .await?
+                    .try_into()
-                .call(groth16::ItemWrapper::from(&spend).into());
+                    .map_err(tower_fallback::BoxedError::from)?,
+            );
 
             async_checks.push(spend_rsp);
         }
@@ -48,10 +49,11 @@ where
         for output in outputs {
             tracing::trace!(?output);
 
-            let output_rsp = output_verifier
+            let output_rsp = output_verifier.ready().await?.call(
-                .ready()
+                DescriptionWrapper(output)
-                .await?
+                    .try_into()
-                .call(groth16::ItemWrapper::from(output).into());
+                    .map_err(tower_fallback::BoxedError::from)?,
+            );
 
             async_checks.push(output_rsp);
         }
@@ -110,9 +112,21 @@ async fn verify_sapling_groth16() {
         .unwrap()
 }
 
+#[derive(Clone, Copy)]
+enum Groth16OutputModification {
+    ZeroCMU,
+    ZeroProof,
+}
+
+static GROTH16_OUTPUT_MODIFICATIONS: [Groth16OutputModification; 2] = [
+    Groth16OutputModification::ZeroCMU,
+    Groth16OutputModification::ZeroProof,
+];
+
 async fn verify_invalid_groth16_output_description<V>(
     output_verifier: &mut V,
     transactions: Vec<std::sync::Arc<Transaction>>,
+    modification: Groth16OutputModification,
 ) -> Result<(), V::Error>
 where
     V: tower::Service<Item, Response = ()>,
@@ -132,14 +146,18 @@ where
             // This changes the primary inputs to the proof
             // verification, causing it to fail for this proof.
             let mut modified_output = output.clone();
-            modified_output.cm_u = jubjub::Fq::zero();
+            match modification {
+                Groth16OutputModification::ZeroCMU => modified_output.cm_u = jubjub::Fq::zero(),
+                Groth16OutputModification::ZeroProof => modified_output.zkproof.0 = [0; 192],
+            }
 
             tracing::trace!(?modified_output);
 
-            let output_rsp = output_verifier
+            let output_rsp = output_verifier.ready().await?.call(
-                .ready()
+                DescriptionWrapper(&modified_output)
-                .await?
+                    .try_into()
-                .call(groth16::ItemWrapper::from(&modified_output).into());
+                    .map_err(tower_fallback::BoxedError::from)?,
+            );
 
             async_checks.push(output_rsp);
         }
@@ -174,9 +192,15 @@ async fn correctly_err_on_invalid_output_proof() {
         .zcash_deserialize_into::<Block>()
         .expect("a valid block");
 
-    verify_invalid_groth16_output_description(&mut output_verifier, block.transactions)
+    for modification in GROTH16_OUTPUT_MODIFICATIONS {
+        verify_invalid_groth16_output_description(
+            &mut output_verifier,
+            block.transactions.clone(),
+            modification,
+        )
         .await
         .expect_err("unexpected success checking invalid groth16 inputs");
+    }
 }
 
 async fn verify_groth16_joinsplits<V>(
@@ -204,10 +228,11 @@ where
             let pub_key = tx
                 .sprout_joinsplit_pub_key()
                 .expect("pub key must exist since there are joinsplits");
-            let joinsplit_rsp = verifier
+            let joinsplit_rsp = verifier.ready().await?.call(
-                .ready()
+                DescriptionWrapper(&(joinsplit, &pub_key))
-                .await?
+                    .try_into()
-                .call(groth16::ItemWrapper::from(&(joinsplit, &pub_key)).into());
+                    .map_err(tower_fallback::BoxedError::from)?,
+            );
 
             async_checks.push(joinsplit_rsp);
         }
@@ -268,10 +293,11 @@ where
 
     tracing::trace!(?joinsplit);
 
-    let joinsplit_rsp = verifier
+    let joinsplit_rsp = verifier.ready().await?.call(
-        .ready()
+        DescriptionWrapper(&(joinsplit, pub_key))
-        .await?
+            .try_into()
-        .call(groth16::ItemWrapper::from(&(joinsplit, pub_key)).into());
+            .map_err(tower_fallback::BoxedError::from)?,
+    );
 
     async_checks.push(joinsplit_rsp);
 
@@ -388,10 +414,11 @@ where
             // Use an arbitrary public key which is not the correct one,
             // which will make the verification fail.
             let modified_pub_key = [0x42; 32].into();
-            let joinsplit_rsp = verifier
+            let joinsplit_rsp = verifier.ready().await?.call(
-                .ready()
+                DescriptionWrapper(&(joinsplit, &modified_pub_key))
-                .await?
+                    .try_into()
-                .call(groth16::ItemWrapper::from(&(joinsplit, &modified_pub_key)).into());
+                    .map_err(tower_fallback::BoxedError::from)?,
+            );
 
             async_checks.push(joinsplit_rsp);
         }

zebra-consensus/src/transaction.rs

@@ -2,6 +2,7 @@
 //!
 use std::{
     collections::HashMap,
+    convert::TryInto,
     future::Future,
     iter::FromIterator,
     pin::Pin,
@@ -33,7 +34,7 @@ use zebra_chain::{
 use zebra_script::CachedFfiTransaction;
 use zebra_state as zs;
 
-use crate::{error::TransactionError, primitives, script, BoxError};
+use crate::{error::TransactionError, groth16::DescriptionWrapper, primitives, script, BoxError};
 
 pub mod check;
 #[cfg(test)]
@@ -314,6 +315,13 @@ where
                 check::non_coinbase_expiry_height(&req.height(), &tx)?;
             }
 
+            // Consensus rule:
+            //
+            // > Either v_{pub}^{old} or v_{pub}^{new} MUST be zero.
+            //
+            // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc
+            check::joinsplit_has_vpub_zero(&tx)?;
+
             // [Canopy onward]: `vpub_old` MUST be zero.
             // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc
             check::disabled_add_to_sprout_pool(&tx, req.height(), network)?;
@@ -461,7 +469,7 @@ where
         .and(Self::verify_sprout_shielded_data(
             joinsplit_data,
             &shielded_sighash,
-        ))
+        )?)
         .and(Self::verify_sapling_shielded_data(
             sapling_shielded_data,
             &shielded_sighash,
@@ -632,16 +640,25 @@ where
     fn verify_sprout_shielded_data(
         joinsplit_data: &Option<transaction::JoinSplitData<Groth16Proof>>,
         shielded_sighash: &SigHash,
-    ) -> AsyncChecks {
+    ) -> Result<AsyncChecks, TransactionError> {
         let mut checks = AsyncChecks::new();
 
         if let Some(joinsplit_data) = joinsplit_data {
-            // XXX create a method on JoinSplitData
+            for joinsplit in joinsplit_data.joinsplits() {
-            // that prepares groth16::Items with the correct proofs
+                // Consensus rule: The proof π_ZKSpend MUST be valid given a
-            // and proof inputs, handling interstitial treestates
+                // primary input formed from the relevant other fields and h_{Sig}
-            // correctly.
+                //
-
+                // Queue the verification of the Groth16 spend proof
-            // Then, pass those items to self.joinsplit to verify them.
+                // for each JoinSplit description while adding the
+                // resulting future to our collection of async
+                // checks that (at a minimum) must pass for the
+                // transaction to verify.
+                //
+                // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc
+                checks.push(primitives::groth16::JOINSPLIT_VERIFIER.oneshot(
+                    DescriptionWrapper(&(joinsplit, &joinsplit_data.pub_key)).try_into()?,
+                ));
+            }
 
             // Consensus rule: The joinSplitSig MUST represent a
             // valid signature, under joinSplitPubKey, of the
@@ -661,7 +678,7 @@ where
             checks.push(ed25519_verifier.oneshot(ed25519_item));
         }
 
-        checks
+        Ok(checks)
     }
 
     /// Verifies a transaction's Sapling shielded data.
@@ -696,7 +713,7 @@ where
                 async_checks.push(
                     primitives::groth16::SPEND_VERIFIER
                         .clone()
-                        .oneshot(primitives::groth16::ItemWrapper::from(&spend).into()),
+                        .oneshot(DescriptionWrapper(&spend).try_into()?),
                 );
 
                 // Consensus rule: The spend authorization signature
@@ -735,7 +752,7 @@ where
                 async_checks.push(
                     primitives::groth16::OUTPUT_VERIFIER
                         .clone()
-                        .oneshot(primitives::groth16::ItemWrapper::from(output).into()),
+                        .oneshot(DescriptionWrapper(output).try_into()?),
                 );
             }
 

zebra-consensus/src/transaction/check.rs

@@ -164,6 +164,27 @@ pub fn output_cv_epk_not_small_order(output: &Output) -> Result<(), TransactionE
     }
 }
 
+/// Check if JoinSplits in the transaction have one of its v_{pub} values equal
+/// to zero.
+///
+/// <https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc>
+pub fn joinsplit_has_vpub_zero(tx: &Transaction) -> Result<(), TransactionError> {
+    let zero = Amount::<NonNegative>::try_from(0).expect("an amount of 0 is always valid");
+
+    let vpub_pairs = tx
+        .output_values_to_sprout()
+        .zip(tx.input_values_from_sprout());
+    for (vpub_old, vpub_new) in vpub_pairs {
+        // > Either v_{pub}^{old} or v_{pub}^{new} MUST be zero.
+        // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc
+        if *vpub_old != zero && *vpub_new != zero {
+            return Err(TransactionError::BothVPubsNonZero);
+        }
+    }
+
+    Ok(())
+}
+
 /// Check if a transaction is adding to the sprout pool after Canopy
 /// network upgrade given a block height and a network.
 ///

zebra-consensus/src/transaction/tests.rs

@@ -904,14 +904,17 @@ fn v4_with_signed_sprout_transfer_is_accepted() {
     zebra_test::init();
     zebra_test::RUNTIME.block_on(async {
         let network = Network::Mainnet;
-        let network_upgrade = NetworkUpgrade::Canopy;
 
-        let canopy_activation_height = network_upgrade
+        let (height, transaction) = test_transactions(network)
-            .activation_height(network)
+            .rev()
-            .expect("Canopy activation height is not set");
+            .filter(|(_, transaction)| {
+                !transaction.has_valid_coinbase_transaction_inputs()
+                    && transaction.inputs().is_empty()
+            })
+            .find(|(_, transaction)| transaction.sprout_groth16_joinsplits().next().is_some())
+            .expect("No transaction found with Groth16 JoinSplits");
 
-        let transaction_block_height =
+        let expected_hash = transaction.unmined_id();
-            (canopy_activation_height + 10).expect("Canopy activation height is too large");
 
         // Initialize the verifier
         let state_service =
@@ -919,38 +922,13 @@ fn v4_with_signed_sprout_transfer_is_accepted() {
         let script_verifier = script::Verifier::new(state_service);
         let verifier = Verifier::new(network, script_verifier);
 
-        // Create a fake Sprout join split
-        let (joinsplit_data, signing_key) = mock_sprout_join_split_data();
-
-        let mut transaction = Transaction::V4 {
-            inputs: vec![],
-            outputs: vec![],
-            lock_time: LockTime::Height(block::Height(0)),
-            expiry_height: (transaction_block_height + 1).expect("expiry height is too large"),
-            joinsplit_data: Some(joinsplit_data),
-            sapling_shielded_data: None,
-        };
-
-        // Sign the transaction
-        let sighash = transaction.sighash(network_upgrade, HashType::ALL, None);
-
-        match &mut transaction {
-            Transaction::V4 {
-                joinsplit_data: Some(joinsplit_data),
-                ..
-            } => joinsplit_data.sig = signing_key.sign(sighash.as_ref()),
-            _ => unreachable!("Mock transaction was created incorrectly"),
-        }
-
-        let expected_hash = transaction.unmined_id();
-
         // Test the transaction verifier
         let result = verifier
             .clone()
             .oneshot(Request::Block {
-                transaction: Arc::new(transaction),
+                transaction,
                 known_utxos: Arc::new(HashMap::new()),
-                height: transaction_block_height,
+                height,
                 time: chrono::MAX_DATETIME,
             })
             .await;
@@ -962,67 +940,76 @@ fn v4_with_signed_sprout_transfer_is_accepted() {
     });
 }
 
-/// Test if an unsigned V4 transaction with a dummy [`sprout::JoinSplit`] is rejected.
+/// Test if an V4 transaction with a modified [`sprout::JoinSplit`] is rejected.
 ///
 /// This test verifies if the transaction verifier correctly rejects the transaction because of the
-/// invalid signature.
+/// invalid JoinSplit.
 #[test]
-fn v4_with_unsigned_sprout_transfer_is_rejected() {
+fn v4_with_modified_joinsplit_is_rejected() {
     zebra_test::init();
     zebra_test::RUNTIME.block_on(async {
-        let network = Network::Mainnet;
+        v4_with_joinsplit_is_rejected_for_modification(
-        let network_upgrade = NetworkUpgrade::Canopy;
+            JoinSplitModification::CorruptSignature,
-
+            // TODO: Fix error downcast
-        let canopy_activation_height = network_upgrade
+            // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature))
-            .activation_height(network)
+            TransactionError::InternalDowncastError(
-            .expect("Canopy activation height is not set");
+                "downcast to known transaction error type failed, original error: InvalidSignature"
-
+                    .to_string(),
-        let transaction_block_height =
+            ),
-            (canopy_activation_height + 10).expect("Canopy activation height is too large");
+        )
-
+        .await;
-        // Initialize the verifier
+        v4_with_joinsplit_is_rejected_for_modification(
-        let state_service =
+            JoinSplitModification::CorruptProof,
-            service_fn(|_| async { unreachable!("State service should not be called") });
+            TransactionError::Groth16("proof verification failed".to_string()),
-        let script_verifier = script::Verifier::new(state_service);
+        )
-        let verifier = Verifier::new(network, script_verifier);
+        .await;
-
+        v4_with_joinsplit_is_rejected_for_modification(
-        // Create a fake Sprout join split
+            JoinSplitModification::ZeroProof,
-        let (joinsplit_data, _) = mock_sprout_join_split_data();
+            TransactionError::MalformedGroth16("invalid G1".to_string()),
-
+        )
-        let transaction = Transaction::V4 {
+        .await;
-            inputs: vec![],
-            outputs: vec![],
-            lock_time: LockTime::Height(block::Height(0)),
-            expiry_height: (transaction_block_height + 1).expect("expiry height is too large"),
-            joinsplit_data: Some(joinsplit_data),
-            sapling_shielded_data: None,
-        };
-
-        // Test the transaction verifier
-        let result = verifier
-            .clone()
-            .oneshot(Request::Block {
-                transaction: Arc::new(transaction),
-                known_utxos: Arc::new(HashMap::new()),
-                height: transaction_block_height,
-                time: chrono::MAX_DATETIME,
-            })
-            .await;
-
-        assert_eq!(
-            result,
-            Err(
-                // TODO: Fix error downcast
-                // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature))
-                TransactionError::InternalDowncastError(
-                    "downcast to known transaction error type failed, original error: InvalidSignature"
-                        .to_string(),
-                )
-            )
-        );
     });
 }
 
+async fn v4_with_joinsplit_is_rejected_for_modification(
+    modification: JoinSplitModification,
+    expected_error: TransactionError,
+) {
+    let network = Network::Mainnet;
+
+    let (height, mut transaction) = test_transactions(network)
+        .rev()
+        .filter(|(_, transaction)| {
+            !transaction.has_valid_coinbase_transaction_inputs() && transaction.inputs().is_empty()
+        })
+        .find(|(_, transaction)| transaction.sprout_groth16_joinsplits().next().is_some())
+        .expect("No transaction found with Groth16 JoinSplits");
+
+    modify_joinsplit(
+        Arc::get_mut(&mut transaction).expect("Transaction only has one active reference"),
+        modification,
+    );
+
+    // Initialize the verifier
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let script_verifier = script::Verifier::new(state_service);
+    let verifier = Verifier::new(network, script_verifier);
+
+    // Test the transaction verifier
+    let result = verifier
+        .clone()
+        .oneshot(Request::Block {
+            transaction,
+            known_utxos: Arc::new(HashMap::new()),
+            height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(result, Err(expected_error));
+}
+
 /// Test if a V4 transaction with Sapling spends is accepted by the verifier.
 #[test]
 fn v4_with_sapling_spends() {
@@ -1476,6 +1463,63 @@ fn mock_sprout_join_split_data() -> (JoinSplitData<Groth16Proof>, ed25519::Signi
     (joinsplit_data, signing_key)
 }
 
+/// A type of JoinSplit modification to test.
+#[derive(Clone, Copy)]
+enum JoinSplitModification {
+    // Corrupt a signature, making it invalid.
+    CorruptSignature,
+    // Corrupt a proof, making it invalid, but still well-formed.
+    CorruptProof,
+    // Make a proof all-zeroes, making it malformed.
+    ZeroProof,
+}
+
+/// Modify a JoinSplit in the transaction following the given modification type.
+fn modify_joinsplit(transaction: &mut Transaction, modification: JoinSplitModification) {
+    match transaction {
+        Transaction::V4 {
+            joinsplit_data: Some(ref mut joinsplit_data),
+            ..
+        } => modify_joinsplit_data(joinsplit_data, modification),
+        _ => unreachable!("Transaction has no JoinSplit shielded data"),
+    }
+}
+
+/// Modify a [`JoinSplitData`] following the given modification type.
+fn modify_joinsplit_data(
+    joinsplit_data: &mut JoinSplitData<Groth16Proof>,
+    modification: JoinSplitModification,
+) {
+    match modification {
+        JoinSplitModification::CorruptSignature => {
+            let mut sig_bytes: [u8; 64] = joinsplit_data.sig.into();
+            // Flip a bit from an arbitrary byte of the signature.
+            sig_bytes[10] ^= 0x01;
+            joinsplit_data.sig = sig_bytes.into();
+        }
+        JoinSplitModification::CorruptProof => {
+            let joinsplit = joinsplit_data
+                .joinsplits_mut()
+                .next()
+                .expect("must have a JoinSplit");
+            {
+                // A proof is composed of three field elements, the first and last having 48 bytes.
+                // (The middle one has 96 bytes.) To corrupt the proof without making it malformed,
+                // simply swap those first and last elements.
+                let (first, rest) = joinsplit.zkproof.0.split_at_mut(48);
+                first.swap_with_slice(&mut rest[96..144]);
+            }
+        }
+        JoinSplitModification::ZeroProof => {
+            let joinsplit = joinsplit_data
+                .joinsplits_mut()
+                .next()
+                .expect("must have a JoinSplit");
+            joinsplit.zkproof.0 = [0; 192];
+        }
+    }
+}
+
 /// Duplicate a Sapling spend inside a `transaction`.
 ///
 /// Returns the nullifier of the duplicate spend.