Validate transparent inputs and outputs in V5 transactions (#2302)

* Add missing documentation

Document methods to describe what they do and why.

* Create an `AsyncChecks` type alias

Make it simpler to write the `FuturesUnordered` type with boxed futures.
This will also end up being used more when refactoring to return the
checks so that the `call` method can wait on them.

* Create `verify_transparent_inputs_and_outputs`

Refactors the verification of the transparent inputs and outputs into a
separate method.

* Refactor transparent checks to use `call_all`

Instead of pushing the verifications into a stream of unordered futures,
use the `ServiceExt::call_all` method to build an equivalent stream
after building a stream of requests.

* Replace `CallAll` with `FuturesUnordered`

Make it more consistent with the rest of the code, and make sure that
the `len()` method is available to use for tracing.

Co-authored-by: teor <teor@riseup.net>

* Refactor to move wait for checks into a new method

Allow the code snipped to be reused by other transaction
version-specific check methods.

* Verify transparent inputs in V5 transactions

Use the script verifier to check the transparent inputs in a V5
transaction.

* Check `has_inputs_and_outputs` for all versions

Check if a transaction has inputs and outputs, independently of the
transaction version.

* Wait for checks in `call` method

Refactor to move the repeated code into the `call` method. Now the
validation methods return the set of asynchronous checks to wait for.

* Add helper function to mock transparent transfers

Creates a fake source UTXO, and then the input and output that represent
spending that UTXO. The initial UTXO can be configured to have a script
that either accepts or rejects any spend attempt.

* Test if transparent V4 transaction is accepted

Create a fake V4 transaction that includes a fake transparent transfer
of funds. The transfer uses a script to allow any UTXO to spend it.

* Test transaction V4 rejection based on script

Create a fake transparent transfer where the source UTXO has a script
that rejects spending. The script verifier should not accept this
transaction.

* Test if transparent V5 transaction is accepted

Create a mock V5 transaction that includes a transparent transfer of
funds. The transaction should be accepted by the verifier.

* Test transaction V5 rejection based on script

Create a fake transparent transfer where the source UTXO has a script
that rejects spending. The script verifier should not accept this
transaction.

* Update `Request::upgrade` getter documentation

Simplify it so that it won't become updated when #1683 is fixed.

Co-authored-by: teor <teor@riseup.net>

zebra-consensus/src/transaction.rs

@@ -31,6 +31,9 @@ mod check;
 #[cfg(test)]
 mod tests;
 
+/// An alias for a set of asynchronous checks that should succeed.
+type AsyncChecks = FuturesUnordered<Pin<Box<dyn Future<Output = Result<(), BoxError>> + Send>>>;
+
 /// Asynchronous transaction verification.
 ///
 /// # Correctness
@@ -96,20 +99,25 @@ pub enum Request {
 }
 
 impl Request {
+    /// The transaction to verify that's in this request.
     pub fn transaction(&self) -> Arc<Transaction> {
         match self {
             Request::Block { transaction, .. } => transaction.clone(),
             Request::Mempool { transaction, .. } => transaction.clone(),
         }
     }
 
+    /// The set of additional known unspent transaction outputs that's in this request.
     pub fn known_utxos(&self) -> Arc<HashMap<transparent::OutPoint, zs::Utxo>> {
         match self {
             Request::Block { known_utxos, .. } => known_utxos.clone(),
             Request::Mempool { known_utxos, .. } => known_utxos.clone(),
         }
     }
 
+    /// The network upgrade to consider for the verification.
+    ///
+    /// This is based on the block height from the request, and the supplied `network`.
     pub fn upgrade(&self, network: Network) -> NetworkUpgrade {
         match self {
             Request::Block { height, .. } => NetworkUpgrade::current(network, *height),
@@ -151,10 +159,14 @@ where
 
         async move {
             tracing::trace!(?tx);
-            match tx.as_ref() {
+
+            // Do basic checks first
+            check::has_inputs_and_outputs(&tx)?;
+
+            let async_checks = match tx.as_ref() {
                 Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => {
                     tracing::debug!(?tx, "got transaction with wrong version");
-                    Err(TransactionError::WrongVersion)
+                    return Err(TransactionError::WrongVersion);
                 }
                 Transaction::V4 {
                     inputs,
@@ -173,10 +185,16 @@ where
                         joinsplit_data,
                         sapling_shielded_data,
                     )
-                    .await
+                    .await?
                 }
-                Transaction::V5 { .. } => Self::verify_v5_transaction(req, network).await,
-            }
+                Transaction::V5 { inputs, .. } => {
+                    Self::verify_v5_transaction(req, network, script_verifier, inputs).await?
+                }
+            };
+
+            Self::wait_for_checks(async_checks).await?;
+
+            Ok(tx.hash())
         }
         .instrument(span)
         .boxed()
@@ -188,14 +206,32 @@ where
     ZS: Service<zs::Request, Response = zs::Response, Error = BoxError> + Send + Clone + 'static,
     ZS::Future: Send + 'static,
 {
+    /// Verify a V4 transaction.
+    ///
+    /// Returns a set of asynchronous checks that must all succeed for the transaction to be
+    /// considered valid. These checks include:
+    ///
+    /// - transparent transfers
+    /// - sprout shielded data
+    /// - sapling shielded data
+    ///
+    /// The parameters of this method are:
+    ///
+    /// - the `request` to verify (that contains the transaction and other metadata, see [`Request`]
+    ///   for more information)
+    /// - the `network` to consider when verifying
+    /// - the `script_verifier` to use for verifying the transparent transfers
+    /// - the transparent `inputs` in the transaction
+    /// - the Sprout `joinsplit_data` shielded data in the transaction
+    /// - the `sapling_shielded_data` in the transaction
     async fn verify_v4_transaction(
         request: Request,
         network: Network,
-        mut script_verifier: script::Verifier<ZS>,
+        script_verifier: script::Verifier<ZS>,
         inputs: &[transparent::Input],
         joinsplit_data: &Option<transaction::JoinSplitData<Groth16Proof>>,
         sapling_shielded_data: &Option<sapling::ShieldedData<sapling::PerSpendAnchor>>,
-    ) -> Result<transaction::Hash, TransactionError> {
+    ) -> Result<AsyncChecks, TransactionError> {
         let mut spend_verifier = primitives::groth16::SPEND_VERIFIER.clone();
         let mut output_verifier = primitives::groth16::OUTPUT_VERIFIER.clone();
 
@@ -204,33 +240,18 @@ where
 
         // A set of asynchronous checks which must all succeed.
         // We finish by waiting on these below.
-        let mut async_checks = FuturesUnordered::new();
+        let mut async_checks = AsyncChecks::new();
 
         let tx = request.transaction();
         let upgrade = request.upgrade(network);
 
-        // Do basic checks first
-        check::has_inputs_and_outputs(&tx)?;
-
-        // Handle transparent inputs and outputs.
-        if tx.is_coinbase() {
-            check::coinbase_tx_no_prevout_joinsplit_spend(&tx)?;
-        } else {
-            // feed all of the inputs to the script and shielded verifiers
-            // the script_verifier also checks transparent sighashes, using its own implementation
-            let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(tx.clone()));
-
-            for input_index in 0..inputs.len() {
-                let rsp = script_verifier.ready_and().await?.call(script::Request {
-                    upgrade,
-                    known_utxos: request.known_utxos(),
-                    cached_ffi_transaction: cached_ffi_transaction.clone(),
-                    input_index,
-                });
-
-                async_checks.push(rsp);
-            }
-        }
+        // Add asynchronous checks of the transparent inputs and outputs
+        async_checks.extend(Self::verify_transparent_inputs_and_outputs(
+            &request,
+            network,
+            inputs,
+            script_verifier,
+        )?);
 
         let shielded_sighash = tx.sighash(upgrade, HashType::ALL, None);
 
@@ -359,27 +380,45 @@ where
             // async_checks.push(rsp);
         }
 
-        // Finally, wait for all asynchronous checks to complete
-        // successfully, or fail verification if they error.
-        while let Some(check) = async_checks.next().await {
-            tracing::trace!(?check, remaining = async_checks.len());
-            check?;
-        }
-
-        Ok(tx.hash())
+        Ok(async_checks)
     }
 
+    /// Verify a V5 transaction.
+    ///
+    /// Returns a set of asynchronous checks that must all succeed for the transaction to be
+    /// considered valid. These checks include:
+    ///
+    /// - transaction support by the considered network upgrade (see [`Request::upgrade`])
+    /// - transparent transfers
+    /// - sapling shielded data (TODO)
+    /// - orchard shielded data (TODO)
+    ///
+    /// The parameters of this method are:
+    ///
+    /// - the `request` to verify (that contains the transaction and other metadata, see [`Request`]
+    ///   for more information)
+    /// - the `network` to consider when verifying
+    /// - the `script_verifier` to use for verifying the transparent transfers
+    /// - the transparent `inputs` in the transaction
     async fn verify_v5_transaction(
         request: Request,
         network: Network,
-    ) -> Result<transaction::Hash, TransactionError> {
+        script_verifier: script::Verifier<ZS>,
+        inputs: &[transparent::Input],
+    ) -> Result<AsyncChecks, TransactionError> {
         Self::verify_v5_transaction_network_upgrade(
             &request.transaction(),
             request.upgrade(network),
         )?;
 
+        let _async_checks = Self::verify_transparent_inputs_and_outputs(
+            &request,
+            network,
+            inputs,
+            script_verifier,
+        )?;
+
         // TODO:
-        // - verify transparent pool (#1981)
         // - verify sapling shielded pool (#1981)
         // - verify orchard shielded pool (ZIP-224) (#2105)
         // - ZIP-216 (#1798)
@@ -388,11 +427,12 @@ where
         unimplemented!("V5 transaction validation is not yet complete");
     }
 
+    /// Verifies if a V5 `transaction` is supported by `network_upgrade`.
     fn verify_v5_transaction_network_upgrade(
         transaction: &Transaction,
-        upgrade: NetworkUpgrade,
+        network_upgrade: NetworkUpgrade,
     ) -> Result<(), TransactionError> {
-        match upgrade {
+        match network_upgrade {
             // Supports V5 transactions
             NetworkUpgrade::Nu5 => Ok(()),
 
@@ -405,8 +445,62 @@ where
             | NetworkUpgrade::Heartwood
             | NetworkUpgrade::Canopy => Err(TransactionError::UnsupportedByNetworkUpgrade(
                 transaction.version(),
-                upgrade,
+                network_upgrade,
             )),
         }
     }
+
+    /// Verifies if a transaction's transparent `inputs` are valid using the provided
+    /// `script_verifier`.
+    fn verify_transparent_inputs_and_outputs(
+        request: &Request,
+        network: Network,
+        inputs: &[transparent::Input],
+        script_verifier: script::Verifier<ZS>,
+    ) -> Result<AsyncChecks, TransactionError> {
+        let transaction = request.transaction();
+
+        if transaction.is_coinbase() {
+            check::coinbase_tx_no_prevout_joinsplit_spend(&transaction)?;
+
+            Ok(AsyncChecks::new())
+        } else {
+            // feed all of the inputs to the script and shielded verifiers
+            // the script_verifier also checks transparent sighashes, using its own implementation
+            let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(transaction));
+            let known_utxos = request.known_utxos();
+            let upgrade = request.upgrade(network);
+
+            let script_checks = (0..inputs.len())
+                .into_iter()
+                .map(move |input_index| {
+                    let request = script::Request {
+                        upgrade,
+                        known_utxos: known_utxos.clone(),
+                        cached_ffi_transaction: cached_ffi_transaction.clone(),
+                        input_index,
+                    };
+
+                    script_verifier.clone().oneshot(request).boxed()
+                })
+                .collect();
+
+            Ok(script_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
+    /// checks by dropping them.
+    async fn wait_for_checks(mut checks: AsyncChecks) -> Result<(), TransactionError> {
+        // Wait for all asynchronous checks to complete
+        // successfully, or fail verification if they error.
+        while let Some(check) = checks.next().await {
+            tracing::trace!(?check, remaining = checks.len());
+            check?;
+        }
+
+        Ok(())
+    }
 }