Release Blocker: Stop trying to verify coinbase inputs using the script verifier (#2404)

* Stop trying to verify coinbase inputs using the script verifier

And create tests to catch similar bugs earier.

* Use Testnet in NU5 tests that temporarily should_panic

We've marked these tests as should_panic until there is a NU5 activation
height. But Testnet will have an activation height first, so we should
prefer it in the tests. (Or use both networks.)

zebra-chain/src/transparent.rs

@@ -34,6 +34,16 @@ pub struct CoinbaseData(
     pub(super) Vec<u8>,
 );
 
+#[cfg(any(test, feature = "proptest-impl"))]
+impl CoinbaseData {
+    /// Create a new `CoinbaseData` containing `data`.
+    ///
+    /// Only for use in tests.
+    pub fn new(data: Vec<u8>) -> CoinbaseData {
+        CoinbaseData(data)
+    }
+}
+
 impl AsRef<[u8]> for CoinbaseData {
     fn as_ref(&self) -> &[u8] {
         self.0.as_ref()

zebra-consensus/src/script.rs

@@ -53,6 +53,8 @@ pub struct Request {
     /// A cached transaction, in the format required by the script verifier FFI interface.
     pub cached_ffi_transaction: Arc<CachedFfiTransaction>,
     /// The index of an input in `cached_ffi_transaction`, used for verifying this request
+    ///
+    /// Coinbase inputs are rejected by the script verifier, because they do not spend a UTXO.
     pub input_index: usize,
     /// A set of additional UTXOs known in the context of this verification request.
     ///

zebra-consensus/src/transaction.rs

@@ -453,27 +453,33 @@ where
     ) -> Result<AsyncChecks, TransactionError> {
         let transaction = request.transaction();
 
-        // feed all of the inputs to the script and shielded verifiers
+        if transaction.is_coinbase() {
-        // the script_verifier also checks transparent sighashes, using its own implementation
+            // The script verifier only verifies PrevOut inputs and their corresponding UTXOs.
-        let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(transaction));
+            // Coinbase transactions don't have any PrevOut inputs.
-        let known_utxos = request.known_utxos();
+            Ok(AsyncChecks::new())
-        let upgrade = request.upgrade(network);
+        } 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())
+            let script_checks = (0..inputs.len())
-            .into_iter()
+                .into_iter()
-            .map(move |input_index| {
+                .map(move |input_index| {
-                let request = script::Request {
+                    let request = script::Request {
-                    upgrade,
+                        upgrade,
-                    known_utxos: known_utxos.clone(),
+                        known_utxos: known_utxos.clone(),
-                    cached_ffi_transaction: cached_ffi_transaction.clone(),
+                        cached_ffi_transaction: cached_ffi_transaction.clone(),
-                    input_index,
+                        input_index,
-                };
+                    };
 
-                script_verifier.clone().oneshot(request).boxed()
+                    script_verifier.clone().oneshot(request).boxed()
-            })
+                })
-            .collect();
+                .collect();
 
-        Ok(script_checks)
+            Ok(script_checks)
+        }
     }
 
     /// Verifies a transaction's Sprout shielded join split data.

zebra-consensus/src/transaction/tests.rs

@@ -13,7 +13,7 @@ use zebra_chain::{
         arbitrary::{fake_v5_transactions_for_network, insert_fake_orchard_shielded_data},
         Hash, HashType, JoinSplitData, LockTime, Transaction,
     },
-    transparent,
+    transparent::{self, CoinbaseData},
 };
 use zebra_state::Utxo;
 
@@ -321,6 +321,49 @@ async fn v4_transaction_with_transparent_transfer_is_accepted() {
     assert_eq!(result, Ok(transaction_hash));
 }
 
+/// Test if V4 coinbase transaction is accepted.
+#[tokio::test]
+async fn v4_coinbase_transaction_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");
+
+    // Create a fake transparent coinbase that should succeed
+    let (input, output) = mock_coinbase_transparent_output(transaction_block_height);
+
+    // Create a V4 coinbase transaction
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::Height(block::Height(0)),
+        expiry_height: transaction_block_height,
+        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(HashMap::new()),
+            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
@@ -379,7 +422,7 @@ async fn v4_transaction_with_transparent_transfer_is_rejected_by_the_script() {
 // defined.
 #[should_panic]
 async fn v5_transaction_with_transparent_transfer_is_accepted() {
-    let network = Network::Mainnet;
+    let network = Network::Testnet;
     let network_upgrade = NetworkUpgrade::Nu5;
 
     let nu5_activation_height = network_upgrade
@@ -424,6 +467,55 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() {
     assert_eq!(result, Ok(transaction_hash));
 }
 
+/// Test if V5 coinbase transaction 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_coinbase_transaction_is_accepted() {
+    let network = Network::Testnet;
+    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");
+
+    // Create a fake transparent coinbase that should succeed
+    let (input, output) = mock_coinbase_transparent_output(transaction_block_height);
+    let known_utxos = HashMap::new();
+
+    // Create a V5 coinbase transaction
+    let transaction = Transaction::V5 {
+        network_upgrade,
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::Height(block::Height(0)),
+        expiry_height: transaction_block_height,
+        sapling_shielded_data: None,
+        orchard_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 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
@@ -433,7 +525,7 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() {
 // defined.
 #[should_panic]
 async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() {
-    let network = Network::Mainnet;
+    let network = Network::Testnet;
     let network_upgrade = NetworkUpgrade::Nu5;
 
     let nu5_activation_height = network_upgrade
@@ -570,7 +662,7 @@ async fn v4_with_sprout_transfers() {
 /// 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
+/// Then, a [`transparent::Input::PrevOut`] is created that attempts to spend 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
@@ -635,6 +727,34 @@ fn mock_transparent_transfer(
     (input, output, known_utxos)
 }
 
+/// Create a mock coinbase input with a transparent output.
+///
+/// Create a [`transparent::Input::Coinbase`] at `coinbase_height`.
+/// Then create UTXO with a [`transparent::Output`] spending some coinbase funds.
+///
+/// Returns the generated coinbase input and transparent output.
+fn mock_coinbase_transparent_output(
+    coinbase_height: block::Height,
+) -> (transparent::Input, transparent::Output) {
+    // A script with a single opcode that rejects the transaction (OP_FALSE)
+    let rejecting_script = transparent::Script::new(&[0]);
+
+    let input = transparent::Input::Coinbase {
+        height: coinbase_height,
+        data: CoinbaseData::new(Vec::new()),
+        sequence: u32::MAX,
+    };
+
+    // 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,
+    };
+
+    (input, output)
+}
+
 /// Create a mock [`sprout::JoinSplit`] and include it in a [`transaction::JoinSplitData`].
 ///
 /// This creates a dummy join split. By itself it is invalid, but it is useful for including in a