Refactor validation of Sapling shielded data in `transaction::Verifier` (#2419)

* Refactor to create `verify_sapling_shielded_data`

Move the code to verify Sapling shielded data into a new helper method
that returns `AsyncChecks`.

* Test verifying a Sapling transaction with spends

Use the test vectors to find a transaction that has Sapling spends and
test if it the verifier considers it valid.

* Create a helper method to list test transactions

Transforms the block test vectors into a list of transactions and block
heights for each transaction.

* Use new helper function in V4 Sapling spend test

Also use the block height for that transaction as specified in the test
vector.

* Test V4 tx. with Sapling outputs but no spends

Find a transaction V4 vector that has Sapling outputs but no spends, and
check that the verifier accepts it.

zebra-chain/Cargo.toml

@@ -9,7 +9,7 @@ edition = "2018"
 
 [features]
 default = []
-proptest-impl = ["proptest", "proptest-derive", "itertools"]
+proptest-impl = ["proptest", "proptest-derive", "itertools", "zebra-test"]
 bench = ["zebra-test"]
 
 [dependencies]

zebra-chain/src/transaction/arbitrary.rs

@@ -567,6 +567,27 @@ fn sapling_spend_v4_to_fake_v5(
     }
 }
 
+/// Iterate over V4 transactions in the block test vectors for the specified `network`.
+pub fn test_transactions(
+    network: Network,
+) -> impl DoubleEndedIterator<Item = (block::Height, Arc<Transaction>)> {
+    let blocks = match network {
+        Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(),
+        Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(),
+    };
+
+    blocks.flat_map(|(&block_height, &block_bytes)| {
+        let block = block_bytes
+            .zcash_deserialize_into::<block::Block>()
+            .expect("block is structurally valid");
+
+        block
+            .transactions
+            .into_iter()
+            .map(move |transaction| (block::Height(block_height), transaction))
+    })
+}
+
 /// Generate an iterator over fake V5 transactions.
 ///
 /// These transactions are converted from non-V5 transactions that exist in the provided network

zebra-consensus/src/transaction.rs

@@ -252,11 +252,6 @@ where
         joinsplit_data: &Option<transaction::JoinSplitData<Groth16Proof>>,
         sapling_shielded_data: &Option<sapling::ShieldedData<sapling::PerSpendAnchor>>,
     ) -> Result<AsyncChecks, TransactionError> {
-        let mut spend_verifier = primitives::groth16::SPEND_VERIFIER.clone();
-        let mut output_verifier = primitives::groth16::OUTPUT_VERIFIER.clone();
-
-        let mut redjubjub_verifier = primitives::redjubjub::VERIFIER.clone();
-
         // A set of asynchronous checks which must all succeed.
         // We finish by waiting on these below.
         let mut async_checks = AsyncChecks::new();
@@ -279,99 +274,9 @@ where
             &shielded_sighash,
         ));
 
-        if let Some(sapling_shielded_data) = sapling_shielded_data {
+        async_checks.extend(
-            for spend in sapling_shielded_data.spends_per_anchor() {
+            Self::verify_sapling_shielded_data(sapling_shielded_data, &shielded_sighash).await?,
-                // 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.
-                //
-                // Queue the verification of the Groth16 spend proof
-                // for each Spend description while adding the
-                // resulting future to our collection of async
-                // checks that (at a minimum) must pass for the
-                // transaction to verify.
-                let spend_rsp = spend_verifier
-                    .ready_and()
-                    .await?
-                    .call(primitives::groth16::ItemWrapper::from(&spend).into());
-
-                async_checks.push(spend_rsp.boxed());
-
-                // Consensus rule: The spend authorization signature
-                // MUST be a valid SpendAuthSig signature over
-                // SigHash using rk as the validating key.
-                //
-                // Queue the validation of the RedJubjub spend
-                // authorization signature for each Spend
-                // description while adding the resulting future to
-                // our collection of async checks that (at a
-                // minimum) must pass for the transaction to verify.
-                let rsp = redjubjub_verifier
-                    .ready_and()
-                    .await?
-                    .call((spend.rk, spend.spend_auth_sig, &shielded_sighash).into());
-
-                async_checks.push(rsp.boxed());
-            }
-
-            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.
-                //
-                // Queue the verification of the Groth16 output
-                // proof for each Output description while adding
-                // the resulting future to our collection of async
-                // checks that (at a minimum) must pass for the
-                // transaction to verify.
-                let output_rsp = output_verifier
-                    .ready_and()
-                    .await?
-                    .call(primitives::groth16::ItemWrapper::from(output).into());
-
-                async_checks.push(output_rsp.boxed());
-            }
-
-            let bvk = sapling_shielded_data.binding_verification_key();
-
-            // TODO: enable async verification and remove this block - #1939
-            {
-                let item: zebra_chain::primitives::redjubjub::batch::Item =
-                    (bvk, sapling_shielded_data.binding_sig, &shielded_sighash).into();
-                item.verify_single().unwrap_or_else(|binding_sig_error| {
-                    let binding_sig_error = binding_sig_error.to_string();
-                    tracing::warn!(%binding_sig_error, "ignoring");
-                    metrics::counter!("zebra.error.sapling.binding",
-                                                  1,
-                                                  "kind" => binding_sig_error);
-                });
-                // Ignore errors until binding signatures are fixed
-                //.map_err(|e| BoxError::from(Box::new(e)))?;
-            }
-
-            let _rsp = redjubjub_verifier
-                .ready_and()
-                .await?
-                .call((bvk, sapling_shielded_data.binding_sig, &shielded_sighash).into())
-                .boxed();
-
-            // TODO: stop ignoring binding signature errors - #1939
-            // async_checks.push(rsp);
-        }
 
         Ok(async_checks)
     }
@@ -518,6 +423,114 @@ where
         checks
     }
 
+    /// Verifies a transaction's Sapling shielded data.
+    async fn verify_sapling_shielded_data(
+        sapling_shielded_data: &Option<sapling::ShieldedData<sapling::PerSpendAnchor>>,
+        shielded_sighash: &blake2b_simd::Hash,
+    ) -> Result<AsyncChecks, TransactionError> {
+        let async_checks = AsyncChecks::new();
+
+        if let Some(sapling_shielded_data) = sapling_shielded_data {
+            let mut spend_verifier = primitives::groth16::SPEND_VERIFIER.clone();
+            let mut output_verifier = primitives::groth16::OUTPUT_VERIFIER.clone();
+            let mut redjubjub_verifier = primitives::redjubjub::VERIFIER.clone();
+
+            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.
+                //
+                // Queue the verification of the Groth16 spend proof
+                // for each Spend description while adding the
+                // resulting future to our collection of async
+                // checks that (at a minimum) must pass for the
+                // transaction to verify.
+                let spend_rsp = spend_verifier
+                    .ready_and()
+                    .await?
+                    .call(primitives::groth16::ItemWrapper::from(&spend).into());
+
+                async_checks.push(spend_rsp.boxed());
+
+                // Consensus rule: The spend authorization signature
+                // MUST be a valid SpendAuthSig signature over
+                // SigHash using rk as the validating key.
+                //
+                // Queue the validation of the RedJubjub spend
+                // authorization signature for each Spend
+                // description while adding the resulting future to
+                // our collection of async checks that (at a
+                // minimum) must pass for the transaction to verify.
+                let rsp = redjubjub_verifier
+                    .ready_and()
+                    .await?
+                    .call((spend.rk, spend.spend_auth_sig, &shielded_sighash).into());
+
+                async_checks.push(rsp.boxed());
+            }
+
+            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.
+                //
+                // Queue the verification of the Groth16 output
+                // proof for each Output description while adding
+                // the resulting future to our collection of async
+                // checks that (at a minimum) must pass for the
+                // transaction to verify.
+                let output_rsp = output_verifier
+                    .ready_and()
+                    .await?
+                    .call(primitives::groth16::ItemWrapper::from(output).into());
+
+                async_checks.push(output_rsp.boxed());
+            }
+
+            let bvk = sapling_shielded_data.binding_verification_key();
+
+            // TODO: enable async verification and remove this block - #1939
+            {
+                let item: zebra_chain::primitives::redjubjub::batch::Item =
+                    (bvk, sapling_shielded_data.binding_sig, &shielded_sighash).into();
+                item.verify_single().unwrap_or_else(|binding_sig_error| {
+                    let binding_sig_error = binding_sig_error.to_string();
+                    tracing::warn!(%binding_sig_error, "ignoring");
+                    metrics::counter!("zebra.error.sapling.binding",
+                                                  1,
+                                                  "kind" => binding_sig_error);
+                });
+                // Ignore errors until binding signatures are fixed
+                //.map_err(|e| BoxError::from(Box::new(e)))?;
+            }
+
+            let _rsp = redjubjub_verifier
+                .ready_and()
+                .await?
+                .call((bvk, sapling_shielded_data.binding_sig, &shielded_sighash).into())
+                .boxed();
+
+            // TODO: stop ignoring binding signature errors - #1939
+            // async_checks.push(rsp);
+        }
+
+        Ok(async_checks)
+    }
+
     /// Await a set of checks that should all succeed.
     ///
     /// If any of the checks fail, this method immediately returns the error and cancels all other

zebra-consensus/src/transaction/tests.rs

@@ -10,7 +10,9 @@ use zebra_chain::{
     serialization::ZcashDeserialize,
     sprout,
     transaction::{
-        arbitrary::{fake_v5_transactions_for_network, insert_fake_orchard_shielded_data},
+        arbitrary::{
+            fake_v5_transactions_for_network, insert_fake_orchard_shielded_data, test_transactions,
+        },
         Hash, HashType, JoinSplitData, LockTime, Transaction,
     },
     transparent::{self, CoinbaseData},
@@ -696,6 +698,83 @@ fn v4_with_unsigned_sprout_transfer_is_rejected() {
     });
 }
 
+/// Test if a V4 transaction with Sapling spends is accepted by the verifier.
+#[test]
+fn v4_with_sapling_spends() {
+    zebra_test::init();
+    zebra_test::RUNTIME.block_on(async {
+        let network = Network::Mainnet;
+
+        let (height, transaction) = test_transactions(network)
+            .rev()
+            .filter(|(_, transaction)| {
+                !transaction.is_coinbase() && transaction.inputs().is_empty()
+            })
+            .find(|(_, transaction)| transaction.sapling_spends_per_anchor().next().is_some())
+            .expect("No transaction found with Sapling spends");
+
+        let expected_hash = transaction.hash();
+
+        // 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,
+            })
+            .await;
+
+        assert_eq!(result, Ok(expected_hash));
+    });
+}
+
+/// Test if a V4 transaction with Sapling outputs but no spends is accepted by the verifier.
+#[test]
+fn v4_with_sapling_outputs_and_no_spends() {
+    zebra_test::init();
+    zebra_test::RUNTIME.block_on(async {
+        let network = Network::Mainnet;
+
+        let (height, transaction) = test_transactions(network)
+            .rev()
+            .filter(|(_, transaction)| {
+                !transaction.is_coinbase() && transaction.inputs().is_empty()
+            })
+            .find(|(_, transaction)| {
+                transaction.sapling_spends_per_anchor().next().is_none()
+                    && transaction.sapling_outputs().next().is_some()
+            })
+            .expect("No transaction found with Sapling spends");
+
+        let expected_hash = transaction.hash();
+
+        // 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,
+            })
+            .await;
+
+        assert_eq!(result, Ok(expected_hash));
+    });
+}
+
 // Utility functions
 
 /// Create a mock transparent transfer to be included in a transaction.