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,6 +99,7 @@ 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(),
@@ -103,6 +107,7 @@ impl Request {
         }
     }
 
+    /// 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(),
@@ -110,6 +115,9 @@ impl Request {
         }
     }
 
+    /// 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
+        // Add asynchronous checks of the transparent inputs and outputs
-        check::has_inputs_and_outputs(&tx)?;
+        async_checks.extend(Self::verify_transparent_inputs_and_outputs(
-
+            &request,
-        // Handle transparent inputs and outputs.
+            network,
-        if tx.is_coinbase() {
+            inputs,
-            check::coinbase_tx_no_prevout_joinsplit_spend(&tx)?;
+            script_verifier,
-        } 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);
-            }
-        }
 
         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
+        Ok(async_checks)
-        // 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())
     }
 
+    /// 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(())
+    }
 }

zebra-consensus/src/transaction/tests.rs

@@ -1,15 +1,18 @@
-use std::{collections::HashMap, sync::Arc};
+use std::{collections::HashMap, convert::TryFrom, sync::Arc};
 
 use tower::{service_fn, ServiceExt};
 
 use zebra_chain::{
-    orchard,
+    amount::Amount,
+    block, orchard,
     parameters::{Network, NetworkUpgrade},
     transaction::{
         arbitrary::{fake_v5_transactions_for_network, insert_fake_orchard_shielded_data},
-        Transaction,
+        Hash, LockTime, Transaction,
     },
+    transparent,
 };
+use zebra_state::Utxo;
 
 use super::{check, Request, Verifier};
 
@@ -241,3 +244,283 @@ async fn v5_transaction_is_accepted_after_nu5_activation() {
         assert_eq!(result, Ok(expected_hash));
     }
 }
+
+/// Test if V4 transaction with transparent funds is accepted.
+#[tokio::test]
+async fn v4_transaction_with_transparent_transfer_is_accepted() {
+    let network = Network::Mainnet;
+
+    let canopy_activation_height = NetworkUpgrade::Canopy
+        .activation_height(network)
+        .expect("Canopy activation height is specified");
+
+    let transaction_block_height =
+        (canopy_activation_height + 10).expect("transaction block height is too large");
+
+    let fake_source_fund_height =
+        (transaction_block_height - 1).expect("fake source fund block height is too small");
+
+    // Create a fake transparent transfer that should succeed
+    let (input, output, known_utxos) = mock_transparent_transfer(fake_source_fund_height, true);
+
+    // Create a V4 transaction
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::Height(block::Height(0)),
+        expiry_height: (transaction_block_height + 1).expect("expiry height is too large"),
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let transaction_hash = transaction.hash();
+
+    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);
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction),
+            known_utxos: Arc::new(known_utxos),
+            height: transaction_block_height,
+        })
+        .await;
+
+    assert_eq!(result, Ok(transaction_hash));
+}
+
+/// Test if V4 transaction with transparent funds is rejected if the source script prevents it.
+///
+/// This test simulates the case where the script verifier rejects the transaction because the
+/// script prevents spending the source UTXO.
+#[tokio::test]
+async fn v4_transaction_with_transparent_transfer_is_rejected_by_the_script() {
+    let network = Network::Mainnet;
+
+    let canopy_activation_height = NetworkUpgrade::Canopy
+        .activation_height(network)
+        .expect("Canopy activation height is specified");
+
+    let transaction_block_height =
+        (canopy_activation_height + 10).expect("transaction block height is too large");
+
+    let fake_source_fund_height =
+        (transaction_block_height - 1).expect("fake source fund block height is too small");
+
+    // Create a fake transparent transfer that should not succeed
+    let (input, output, known_utxos) = mock_transparent_transfer(fake_source_fund_height, false);
+
+    // Create a V4 transaction
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::Height(block::Height(0)),
+        expiry_height: (transaction_block_height + 1).expect("expiry height is too large"),
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    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);
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction),
+            known_utxos: Arc::new(known_utxos),
+            height: transaction_block_height,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::InternalDowncastError(
+            "downcast to redjubjub::Error failed, original error: ScriptInvalid".to_string()
+        ))
+    );
+}
+
+/// Test if V5 transaction with transparent funds is accepted.
+#[tokio::test]
+// TODO: Remove `should_panic` once the NU5 activation heights for testnet and mainnet have been
+// defined.
+#[should_panic]
+async fn v5_transaction_with_transparent_transfer_is_accepted() {
+    let network = Network::Mainnet;
+    let network_upgrade = NetworkUpgrade::Nu5;
+
+    let nu5_activation_height = network_upgrade
+        .activation_height(network)
+        .expect("NU5 activation height is specified");
+
+    let transaction_block_height =
+        (nu5_activation_height + 10).expect("transaction block height is too large");
+
+    let fake_source_fund_height =
+        (transaction_block_height - 1).expect("fake source fund block height is too small");
+
+    // Create a fake transparent transfer that should succeed
+    let (input, output, known_utxos) = mock_transparent_transfer(fake_source_fund_height, true);
+
+    // Create a V5 transaction
+    let transaction = Transaction::V5 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::Height(block::Height(0)),
+        expiry_height: (transaction_block_height + 1).expect("expiry height is too large"),
+        sapling_shielded_data: None,
+        orchard_shielded_data: None,
+        network_upgrade,
+    };
+
+    let transaction_hash = transaction.hash();
+
+    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);
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction),
+            known_utxos: Arc::new(known_utxos),
+            height: transaction_block_height,
+        })
+        .await;
+
+    assert_eq!(result, Ok(transaction_hash));
+}
+
+/// Test if V5 transaction with transparent funds is rejected if the source script prevents it.
+///
+/// This test simulates the case where the script verifier rejects the transaction because the
+/// script prevents spending the source UTXO.
+#[tokio::test]
+// TODO: Remove `should_panic` once the NU5 activation heights for testnet and mainnet have been
+// defined.
+#[should_panic]
+async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() {
+    let network = Network::Mainnet;
+    let network_upgrade = NetworkUpgrade::Nu5;
+
+    let nu5_activation_height = network_upgrade
+        .activation_height(network)
+        .expect("NU5 activation height is specified");
+
+    let transaction_block_height =
+        (nu5_activation_height + 10).expect("transaction block height is too large");
+
+    let fake_source_fund_height =
+        (transaction_block_height - 1).expect("fake source fund block height is too small");
+
+    // Create a fake transparent transfer that should not succeed
+    let (input, output, known_utxos) = mock_transparent_transfer(fake_source_fund_height, false);
+
+    // Create a V5 transaction
+    let transaction = Transaction::V5 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::Height(block::Height(0)),
+        expiry_height: (transaction_block_height + 1).expect("expiry height is too large"),
+        sapling_shielded_data: None,
+        orchard_shielded_data: None,
+        network_upgrade,
+    };
+
+    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);
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction),
+            known_utxos: Arc::new(known_utxos),
+            height: transaction_block_height,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::InternalDowncastError(
+            "downcast to redjubjub::Error failed, original error: ScriptInvalid".to_string()
+        ))
+    );
+}
+
+// Utility functions
+
+/// Create a mock transparent transfer to be included in a transaction.
+///
+/// First, this creates a fake unspent transaction output from a fake transaction included in the
+/// specified `previous_utxo_height` block height. This fake [`Utxo`] also contains a simple script
+/// that can either accept or reject any spend attempt, depending on if `script_should_succeed` is
+/// `true` or `false`.
+///
+/// Then, a [`transparent::Input`] is created that attempts to spends the previously created fake
+/// UTXO. A new UTXO is created with the [`transparent::Output`] resulting from the spend.
+///
+/// Finally, the initial fake UTXO is placed in a `known_utxos` [`HashMap`] so that it can be
+/// retrieved during verification.
+///
+/// The function then returns the generated transparent input and output, as well as the
+/// `known_utxos` map.
+fn mock_transparent_transfer(
+    previous_utxo_height: block::Height,
+    script_should_succeed: bool,
+) -> (
+    transparent::Input,
+    transparent::Output,
+    HashMap<transparent::OutPoint, Utxo>,
+) {
+    // A script with a single opcode that accepts the transaction (pushes true on the stack)
+    let accepting_script = transparent::Script::new(&[1, 1]);
+    // A script with a single opcode that rejects the transaction (OP_FALSE)
+    let rejecting_script = transparent::Script::new(&[0]);
+
+    // Mock an unspent transaction output
+    let previous_outpoint = transparent::OutPoint {
+        hash: Hash([1u8; 32]),
+        index: 0,
+    };
+
+    let lock_script = if script_should_succeed {
+        accepting_script.clone()
+    } else {
+        rejecting_script.clone()
+    };
+
+    let previous_output = transparent::Output {
+        value: Amount::try_from(1).expect("1 is an invalid amount"),
+        lock_script,
+    };
+
+    let previous_utxo = Utxo {
+        output: previous_output,
+        height: previous_utxo_height,
+        from_coinbase: false,
+    };
+
+    // Use the `previous_outpoint` as input
+    let input = transparent::Input::PrevOut {
+        outpoint: previous_outpoint,
+        unlock_script: accepting_script,
+        sequence: 0,
+    };
+
+    // The output resulting from the transfer
+    // Using the rejecting script pretends the amount is burned because it can't be spent again
+    let output = transparent::Output {
+        value: Amount::try_from(1).expect("1 is an invalid amount"),
+        lock_script: rejecting_script,
+    };
+
+    // Cache the source of the fund so that it can be used during verification
+    let mut known_utxos = HashMap::new();
+    known_utxos.insert(previous_outpoint, previous_utxo);
+
+    (input, output, known_utxos)
+}