Reject V5 transactions before NU5 activation (#2285)

* Add a `Transaction::version` getter

Returns the version of the transaction as a `u32`.

* Add `Transaction::is_overwintered` helper method

Returns if the `fOverwintered` flag should be set for the transaction's
version.

* Use new helpers to serialize transaction version

Reduce the repeated code and make it less error-prone with future
changes.

* Add getter methods to `transaction::Request` type

Refactor to move the type deconstruction code into the `Request` type.
The main objective is to make it easier to split the call handler into
methods that receive the request directly.

* Refactor to create `verify_v4_transaction` helper

Split the code specific to V4 transactions into a separate helper
method.

* Create `verify_v5_transaction` helper method

Prepare a separate method to have the validation code.

* Add `UnsupportedByNetworkUpgrade` error variant

An error for when a transaction's version isn't supported by the network
upgrade of the block it's included or for the current network upgrade if
the transaction is for the mempool.

* Verify a V5 transaction's network upgrade

For now, only NU5 supports V5 transactions.

* Test that V5 transaction is rejected on Canopy

Create a fake V5 transaction and try to verify it using a block height
from Canopy's activation. The verifier should reject the transaction
with an error saying that the network upgrade does not support that
transaction version.

* Test if V5 tx. is accepted after NU5 activation

Create a fake V5 transaction and pretend it is placed in a block that
has a height after the NU5 activation. The test should succeed, but
since the NU5 activation height has not been specified yet (neither for
the testnet nor the mainnet), for now this test is marked as
`should_panic`.

* Add `TODO` comment to the code

Add more detail to what's left to do, and link to the appropriate PRs.

* Use `u32` to store transaction version

Use a type consistent with how the version is specified.

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

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

zebra-chain/src/transaction.rs

@@ -151,6 +151,25 @@ impl Transaction {
 
     // header
 
+    /// Return if the `fOverwintered` flag of this transaction is set.
+    pub fn is_overwintered(&self) -> bool {
+        match self {
+            Transaction::V1 { .. } | Transaction::V2 { .. } => false,
+            Transaction::V3 { .. } | Transaction::V4 { .. } | Transaction::V5 { .. } => true,
+        }
+    }
+
+    /// Return the version of this transaction.
+    pub fn version(&self) -> u32 {
+        match self {
+            Transaction::V1 { .. } => 1,
+            Transaction::V2 { .. } => 2,
+            Transaction::V3 { .. } => 3,
+            Transaction::V4 { .. } => 4,
+            Transaction::V5 { .. } => 5,
+        }
+    }
+
     /// Get this transaction's lock time.
     pub fn lock_time(&self) -> LockTime {
         match self {

zebra-chain/src/transaction/serialize.rs

@@ -371,13 +371,19 @@ impl ZcashSerialize for Transaction {
         //
         // Since we checkpoint on Canopy activation, we won't ever need
         // to check the smaller pre-Sapling transaction size limit.
+
+        // header: Write version and set the fOverwintered bit if necessary
+        let overwintered_flag = if self.is_overwintered() { 1 << 31 } else { 0 };
+        let version = overwintered_flag | self.version();
+
+        writer.write_u32::<LittleEndian>(version)?;
+
         match self {
             Transaction::V1 {
                 inputs,
                 outputs,
                 lock_time,
             } => {
-                writer.write_u32::<LittleEndian>(1)?;
                 inputs.zcash_serialize(&mut writer)?;
                 outputs.zcash_serialize(&mut writer)?;
                 lock_time.zcash_serialize(&mut writer)?;
@@ -388,7 +394,6 @@ impl ZcashSerialize for Transaction {
                 lock_time,
                 joinsplit_data,
             } => {
-                writer.write_u32::<LittleEndian>(2)?;
                 inputs.zcash_serialize(&mut writer)?;
                 outputs.zcash_serialize(&mut writer)?;
                 lock_time.zcash_serialize(&mut writer)?;
@@ -405,8 +410,6 @@ impl ZcashSerialize for Transaction {
                 expiry_height,
                 joinsplit_data,
             } => {
-                // Write version 3 and set the fOverwintered bit.
-                writer.write_u32::<LittleEndian>(3 | (1 << 31))?;
                 writer.write_u32::<LittleEndian>(OVERWINTER_VERSION_GROUP_ID)?;
                 inputs.zcash_serialize(&mut writer)?;
                 outputs.zcash_serialize(&mut writer)?;
@@ -426,8 +429,6 @@ impl ZcashSerialize for Transaction {
                 sapling_shielded_data,
                 joinsplit_data,
             } => {
-                // Write version 4 and set the fOverwintered bit.
-                writer.write_u32::<LittleEndian>(4 | (1 << 31))?;
                 writer.write_u32::<LittleEndian>(SAPLING_VERSION_GROUP_ID)?;
                 inputs.zcash_serialize(&mut writer)?;
                 outputs.zcash_serialize(&mut writer)?;
@@ -492,8 +493,6 @@ impl ZcashSerialize for Transaction {
                 // Transaction V5 spec:
                 // https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
 
-                // header: Write version 5 and set the fOverwintered bit
-                writer.write_u32::<LittleEndian>(5 | (1 << 31))?;
                 writer.write_u32::<LittleEndian>(TX_V5_VERSION_GROUP_ID)?;
 
                 // header: Write the nConsensusBranchId

zebra-consensus/src/error.rs

@@ -47,6 +47,9 @@ pub enum TransactionError {
     #[error("transaction version number MUST be >= 4")]
     WrongVersion,
 
+    #[error("transaction version {0} not supported by the network upgrade {1:?}")]
+    UnsupportedByNetworkUpgrade(u32, zebra_chain::parameters::NetworkUpgrade),
+
     #[error("must have at least one input: transparent, shielded spend, or joinsplit")]
     NoInputs,
 

zebra-consensus/src/transaction.rs

@@ -16,6 +16,8 @@ use tracing::Instrument;
 use zebra_chain::{
     block,
     parameters::{Network, NetworkUpgrade},
+    primitives::Groth16Proof,
+    sapling,
     transaction::{self, HashType, Transaction},
     transparent,
 };
@@ -87,6 +89,29 @@ pub enum Request {
     },
 }
 
+impl Request {
+    pub fn transaction(&self) -> Arc<Transaction> {
+        match self {
+            Request::Block { transaction, .. } => transaction.clone(),
+            Request::Mempool { transaction, .. } => transaction.clone(),
+        }
+    }
+
+    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(),
+        }
+    }
+
+    pub fn upgrade(&self, network: Network) -> NetworkUpgrade {
+        match self {
+            Request::Block { height, .. } => NetworkUpgrade::current(network, *height),
+            Request::Mempool { upgrade, .. } => *upgrade,
+        }
+    }
+}
+
 impl<ZS> Service<Request> for Verifier<ZS>
 where
     ZS: Service<zs::Request, Response = zs::Response, Error = BoxError> + Send + Clone + 'static,
@@ -112,34 +137,15 @@ where
             unimplemented!();
         }
 
-        let (tx, known_utxos, upgrade) = match req {
+        let script_verifier = self.script_verifier.clone();
-            Request::Block {
+        let network = self.network;
-                transaction,
-                known_utxos,
-                height,
-            } => {
-                let upgrade = NetworkUpgrade::current(self.network, height);
-                (transaction, known_utxos, upgrade)
-            }
-            Request::Mempool {
-                transaction,
-                known_utxos,
-                upgrade,
-            } => (transaction, known_utxos, upgrade),
-        };
-
-        let mut spend_verifier = primitives::groth16::SPEND_VERIFIER.clone();
-        let mut output_verifier = primitives::groth16::OUTPUT_VERIFIER.clone();
-
-        let mut ed25519_verifier = primitives::ed25519::VERIFIER.clone();
-        let mut redjubjub_verifier = primitives::redjubjub::VERIFIER.clone();
-        let mut script_verifier = self.script_verifier.clone();
 
+        let tx = req.transaction();
         let span = tracing::debug_span!("tx", hash = %tx.hash());
 
         async move {
             tracing::trace!(?tx);
-            match &*tx {
+            match tx.as_ref() {
                 Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => {
                     tracing::debug!(?tx, "got transaction with wrong version");
                     Err(TransactionError::WrongVersion)
@@ -153,10 +159,50 @@ where
                     sapling_shielded_data,
                     ..
                 } => {
+                    Self::verify_v4_transaction(
+                        req,
+                        network,
+                        script_verifier,
+                        inputs,
+                        joinsplit_data,
+                        sapling_shielded_data,
+                    )
+                    .await
+                }
+                Transaction::V5 { .. } => Self::verify_v5_transaction(req, network).await,
+            }
+        }
+        .instrument(span)
+        .boxed()
+    }
+}
+
+impl<ZS> Verifier<ZS>
+where
+    ZS: Service<zs::Request, Response = zs::Response, Error = BoxError> + Send + Clone + 'static,
+    ZS::Future: Send + 'static,
+{
+    async fn verify_v4_transaction(
+        request: Request,
+        network: Network,
+        mut 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> {
+        let mut spend_verifier = primitives::groth16::SPEND_VERIFIER.clone();
+        let mut output_verifier = primitives::groth16::OUTPUT_VERIFIER.clone();
+
+        let mut ed25519_verifier = primitives::ed25519::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 = FuturesUnordered::new();
 
+        let tx = request.transaction();
+        let upgrade = request.upgrade(network);
+
         // Do basic checks first
         check::has_inputs_and_outputs(&tx)?;
 
@@ -166,13 +212,12 @@ where
         } 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 =
+            let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(tx.clone()));
-                            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: known_utxos.clone(),
+                    known_utxos: request.known_utxos(),
                     cached_ffi_transaction: cached_ffi_transaction.clone(),
                     input_index,
                 });
@@ -181,11 +226,7 @@ where
             }
         }
 
-                    let shielded_sighash = tx.sighash(
+        let shielded_sighash = tx.sighash(upgrade, HashType::ALL, None);
-                        upgrade,
-                        HashType::ALL,
-                        None,
-                    );
 
         if let Some(joinsplit_data) = joinsplit_data {
             // XXX create a method on JoinSplitData
@@ -206,10 +247,14 @@ where
             //
             // https://zips.z.cash/protocol/protocol.pdf#sproutnonmalleability
             // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
-                        let rsp = ed25519_verifier
+            let rsp = ed25519_verifier.ready_and().await?.call(
-                            .ready_and()
+                (
-                            .await?
+                    joinsplit_data.pub_key,
-                            .call((joinsplit_data.pub_key, joinsplit_data.sig, &shielded_sighash).into());
+                    joinsplit_data.sig,
+                    &shielded_sighash,
+                )
+                    .into(),
+            );
 
             async_checks.push(rsp.boxed());
         }
@@ -285,7 +330,8 @@ where
 
             // 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();
+                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");
@@ -316,12 +362,45 @@ where
 
         Ok(tx.hash())
     }
-                Transaction::V5 { .. } => {
+
-                    unimplemented!("v5 transaction validation as specified in ZIP-216, ZIP-224, ZIP-225, and ZIP-244")
+    async fn verify_v5_transaction(
+        request: Request,
+        network: Network,
+    ) -> Result<transaction::Hash, TransactionError> {
+        Self::verify_v5_transaction_network_upgrade(
+            &request.transaction(),
+            request.upgrade(network),
+        )?;
+
+        // TODO:
+        // - verify transparent pool (#1981)
+        // - verify sapling shielded pool (#1981)
+        // - verify orchard shielded pool (ZIP-224) (#2105)
+        // - ZIP-216 (#1798)
+        // - ZIP-244 (#1874)
+
+        unimplemented!("V5 transaction validation is not yet complete");
+    }
+
+    fn verify_v5_transaction_network_upgrade(
+        transaction: &Transaction,
+        upgrade: NetworkUpgrade,
+    ) -> Result<(), TransactionError> {
+        match upgrade {
+            // Supports V5 transactions
+            NetworkUpgrade::Nu5 => Ok(()),
+
+            // Does not support V5 transactions
+            NetworkUpgrade::Genesis
+            | NetworkUpgrade::BeforeOverwinter
+            | NetworkUpgrade::Overwinter
+            | NetworkUpgrade::Sapling
+            | NetworkUpgrade::Blossom
+            | NetworkUpgrade::Heartwood
+            | NetworkUpgrade::Canopy => Err(TransactionError::UnsupportedByNetworkUpgrade(
+                transaction.version(),
+                upgrade,
+            )),
         }
     }
 }
-        .instrument(span)
-        .boxed()
-    }
-}

zebra-consensus/src/transaction/tests.rs

@@ -1,15 +1,19 @@
+use std::{collections::HashMap, sync::Arc};
+
+use tower::{service_fn, ServiceExt};
+
 use zebra_chain::{
     orchard,
-    parameters::Network,
+    parameters::{Network, NetworkUpgrade},
     transaction::{
         arbitrary::{fake_v5_transactions_for_network, insert_fake_orchard_shielded_data},
         Transaction,
     },
 };
 
-use super::check;
+use super::{check, Request, Verifier};
 
-use crate::error::TransactionError;
+use crate::{error::TransactionError, script};
 use color_eyre::eyre::Report;
 
 #[test]
@@ -160,3 +164,80 @@ fn v5_coinbase_transaction_with_enable_spends_flag_fails_validation() {
         Err(TransactionError::CoinbaseHasEnableSpendsOrchard)
     );
 }
+
+#[tokio::test]
+async fn v5_transaction_is_rejected_before_nu5_activation() {
+    const V5_TRANSACTION_VERSION: u32 = 5;
+
+    let canopy = NetworkUpgrade::Canopy;
+    let networks = vec![
+        (Network::Mainnet, zebra_test::vectors::MAINNET_BLOCKS.iter()),
+        (Network::Testnet, zebra_test::vectors::TESTNET_BLOCKS.iter()),
+    ];
+
+    for (network, blocks) in networks {
+        let state_service = service_fn(|_| async { unreachable!("Service should not be called") });
+        let script_verifier = script::Verifier::new(state_service);
+        let verifier = Verifier::new(network, script_verifier);
+
+        let transaction = fake_v5_transactions_for_network(network, blocks)
+            .rev()
+            .next()
+            .expect("At least one fake V5 transaction in the test vectors");
+
+        let result = verifier
+            .oneshot(Request::Block {
+                transaction: Arc::new(transaction),
+                known_utxos: Arc::new(HashMap::new()),
+                height: canopy
+                    .activation_height(network)
+                    .expect("Canopy activation height is specified"),
+            })
+            .await;
+
+        assert_eq!(
+            result,
+            Err(TransactionError::UnsupportedByNetworkUpgrade(
+                V5_TRANSACTION_VERSION,
+                canopy
+            ))
+        );
+    }
+}
+
+#[tokio::test]
+// TODO: Remove `should_panic` once the NU5 activation heights for testnet and mainnet have been
+// defined.
+#[should_panic]
+async fn v5_transaction_is_accepted_after_nu5_activation() {
+    let nu5 = NetworkUpgrade::Nu5;
+    let networks = vec![
+        (Network::Mainnet, zebra_test::vectors::MAINNET_BLOCKS.iter()),
+        (Network::Testnet, zebra_test::vectors::TESTNET_BLOCKS.iter()),
+    ];
+
+    for (network, blocks) in networks {
+        let state_service = service_fn(|_| async { unreachable!("Service should not be called") });
+        let script_verifier = script::Verifier::new(state_service);
+        let verifier = Verifier::new(network, script_verifier);
+
+        let transaction = fake_v5_transactions_for_network(network, blocks)
+            .rev()
+            .next()
+            .expect("At least one fake V5 transaction in the test vectors");
+
+        let expected_hash = transaction.hash();
+
+        let result = verifier
+            .oneshot(Request::Block {
+                transaction: Arc::new(transaction),
+                known_utxos: Arc::new(HashMap::new()),
+                height: nu5
+                    .activation_height(network)
+                    .expect("NU5 activation height is specified"),
+            })
+            .await;
+
+        assert_eq!(result, Ok(expected_hash));
+    }
+}