Use unmined types for transaction verifier mempool requests and responses (#2666)

* Add `Arc<Transaction>` conversions for Transaction IDs

* Use UnminedTxId as the transaction verifier response type

* Use UnminedTx in transaction verifier mempool requests

* Refactor is_mempool into a transaction verifier request method

* Order derives in alphabetical order

Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>

zebra-chain/src/transaction.rs

@@ -132,13 +132,24 @@ pub enum Transaction {
 }
 
 impl Transaction {
-    // hashes
+    // identifiers and hashes
 
-    /// Compute the hash (id) of this transaction.
+    /// Compute the hash (mined transaction ID) of this transaction.
+    ///
+    /// The hash uniquely identifies mined v5 transactions,
+    /// and all v1-v4 transactions, whether mined or unmined.
     pub fn hash(&self) -> Hash {
         Hash::from(self)
     }
 
+    /// Compute the unmined transaction ID of this transaction.
+    ///
+    /// This ID uniquely identifies unmined transactions,
+    /// regardless of version.
+    pub fn unmined_id(&self) -> UnminedTxId {
+        UnminedTxId::from(self)
+    }
+
     /// Calculate the sighash for the current transaction
     ///
     /// # Details

zebra-chain/src/transaction/auth_digest.rs

@@ -1,4 +1,4 @@
-use std::fmt;
+use std::{fmt, sync::Arc};
 
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
@@ -45,6 +45,17 @@ impl From<&Transaction> for AuthDigest {
     }
 }
 
+impl From<Arc<Transaction>> for AuthDigest {
+    /// Computes the authorizing data commitment for a transaction.
+    ///
+    /// # Panics
+    ///
+    /// If passed a pre-v5 transaction.
+    fn from(transaction: Arc<Transaction>) -> Self {
+        transaction.as_ref().into()
+    }
+}
+
 impl From<[u8; 32]> for AuthDigest {
     fn from(bytes: [u8; 32]) -> Self {
         Self(bytes)

zebra-chain/src/transaction/hash.rs

@@ -28,6 +28,7 @@
 use std::{
     convert::{TryFrom, TryInto},
     fmt,
+    sync::Arc,
 };
 
 #[cfg(any(test, feature = "proptest-impl"))]
@@ -75,6 +76,12 @@ impl From<&Transaction> for Hash {
     }
 }
 
+impl From<Arc<Transaction>> for Hash {
+    fn from(transaction: Arc<Transaction>) -> Self {
+        Hash::from(transaction.as_ref())
+    }
+}
+
 impl From<[u8; 32]> for Hash {
     fn from(bytes: [u8; 32]) -> Self {
         Self(bytes)
@@ -190,6 +197,17 @@ impl From<&Transaction> for WtxId {
     }
 }
 
+impl From<Arc<Transaction>> for WtxId {
+    /// Computes the witnessed transaction ID for a transaction.
+    ///
+    /// # Panics
+    ///
+    /// If passed a pre-v5 transaction.
+    fn from(transaction: Arc<Transaction>) -> Self {
+        transaction.as_ref().into()
+    }
+}
+
 impl From<[u8; 64]> for WtxId {
     fn from(bytes: [u8; 64]) -> Self {
         let id: [u8; 32] = bytes[0..32].try_into().expect("length is 64");

zebra-chain/src/transaction/unmined.rs

@@ -75,6 +75,12 @@ impl From<&Transaction> for UnminedTxId {
     }
 }
 
+impl From<Arc<Transaction>> for UnminedTxId {
+    fn from(transaction: Arc<Transaction>) -> Self {
+        transaction.as_ref().into()
+    }
+}
+
 impl From<WtxId> for UnminedTxId {
     fn from(wtx_id: WtxId) -> Self {
         Witnessed(wtx_id)

zebra-consensus/src/transaction.rs

@@ -19,7 +19,7 @@ use zebra_chain::{
     parameters::{Network, NetworkUpgrade},
     primitives::Groth16Proof,
     sapling,
-    transaction::{self, HashType, SigHash, Transaction},
+    transaction::{self, HashType, SigHash, Transaction, UnminedTx, UnminedTxId},
     transparent,
 };
 
@@ -63,7 +63,7 @@ where
 ///
 /// Transaction verification has slightly different consensus rules, depending on
 /// whether the transaction is to be included in a block on in the mempool.
-#[allow(dead_code)]
+#[derive(Clone, Debug, Eq, PartialEq)]
 pub enum Request {
     /// Verify the supplied transaction as part of a block.
     Block {
@@ -81,7 +81,7 @@ pub enum Request {
     /// Note: coinbase transactions are invalid in the mempool
     Mempool {
         /// The transaction itself.
-        transaction: Arc<Transaction>,
+        transaction: UnminedTx,
         /// The height of the next block.
         ///
         /// The next block is the first block that could possibly contain a
@@ -92,14 +92,29 @@ pub enum Request {
 
 /// The response type for the transaction verifier service.
 /// Responses identify the transaction that was verified.
-pub type Response = zebra_chain::transaction::Hash;
+///
+/// [`Block`] requests can be uniquely identified by [`UnminedTxId::mined_id`],
+/// because the block's authorizing data root will be checked during contextual validation.
+///
+/// [`Mempool`] requests are uniquely identified by the [`UnminedTxId`]
+/// variant for their transaction version.
+pub type Response = zebra_chain::transaction::UnminedTxId;
 
 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(),
+            Request::Mempool { transaction, .. } => transaction.transaction.clone(),
+        }
+    }
+
+    /// The unmined transaction ID for the transaction in this request.
+    pub fn tx_id(&self) -> UnminedTxId {
+        match self {
+            // TODO: get the precalculated ID from the block verifier
+            Request::Block { transaction, .. } => transaction.unmined_id(),
+            Request::Mempool { transaction, .. } => transaction.id,
         }
     }
 
@@ -124,6 +139,14 @@ impl Request {
     pub fn upgrade(&self, network: Network) -> NetworkUpgrade {
         NetworkUpgrade::current(network, self.height())
     }
+
+    /// Returns true if the request is a mempool request.
+    pub fn is_mempool(&self) -> bool {
+        match self {
+            Request::Block { .. } => false,
+            Request::Mempool { .. } => true,
+        }
+    }
 }
 
 impl<ZS> Service<Request> for Verifier<ZS>
@@ -142,11 +165,7 @@ where
 
     // TODO: break up each chunk into its own method
     fn call(&mut self, req: Request) -> Self::Future {
-        let is_mempool = match req {
+        if req.is_mempool() {
-            Request::Block { .. } => false,
-            Request::Mempool { .. } => true,
-        };
-        if is_mempool {
             // XXX determine exactly which rules apply to mempool transactions
             unimplemented!("Zebra does not yet have a mempool (#2309)");
         }
@@ -155,10 +174,11 @@ where
         let network = self.network;
 
         let tx = req.transaction();
-        let span = tracing::debug_span!("tx", hash = %tx.hash());
+        let id = req.tx_id();
+        let span = tracing::debug_span!("tx", ?id);
 
         async move {
-            tracing::trace!(?tx);
+            tracing::trace!(?req);
 
             // Do basic checks first
             check::has_inputs_and_outputs(&tx)?;
@@ -218,7 +238,7 @@ where
 
             async_checks.check().await?;
 
-            Ok(tx.hash())
+            Ok(id)
         }
         .instrument(span)
         .boxed()

zebra-consensus/src/transaction/tests.rs

@@ -254,7 +254,7 @@ async fn v5_transaction_is_accepted_after_nu5_activation() {
             .next()
             .expect("At least one fake V5 transaction in the test vectors");
 
-        let expected_hash = transaction.hash();
+        let expected_hash = transaction.unmined_id();
 
         let result = verifier
             .oneshot(Request::Block {
@@ -298,7 +298,7 @@ async fn v4_transaction_with_transparent_transfer_is_accepted() {
         sapling_shielded_data: None,
     };
 
-    let transaction_hash = transaction.hash();
+    let transaction_hash = transaction.unmined_id();
 
     let state_service =
         service_fn(|_| async { unreachable!("State service should not be called") });
@@ -341,7 +341,7 @@ async fn v4_coinbase_transaction_is_accepted() {
         sapling_shielded_data: None,
     };
 
-    let transaction_hash = transaction.hash();
+    let transaction_hash = transaction.unmined_id();
 
     let state_service =
         service_fn(|_| async { unreachable!("State service should not be called") });
@@ -445,7 +445,7 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() {
         network_upgrade,
     };
 
-    let transaction_hash = transaction.hash();
+    let transaction_hash = transaction.unmined_id();
 
     let state_service =
         service_fn(|_| async { unreachable!("State service should not be called") });
@@ -494,7 +494,7 @@ async fn v5_coinbase_transaction_is_accepted() {
         orchard_shielded_data: None,
     };
 
-    let transaction_hash = transaction.hash();
+    let transaction_hash = transaction.unmined_id();
 
     let state_service =
         service_fn(|_| async { unreachable!("State service should not be called") });
@@ -616,7 +616,7 @@ fn v4_with_signed_sprout_transfer_is_accepted() {
             _ => unreachable!("Mock transaction was created incorrectly"),
         }
 
-        let expected_hash = transaction.hash();
+        let expected_hash = transaction.unmined_id();
 
         // Test the transaction verifier
         let result = verifier
@@ -707,7 +707,7 @@ fn v4_with_sapling_spends() {
             .find(|(_, transaction)| transaction.sapling_spends_per_anchor().next().is_some())
             .expect("No transaction found with Sapling spends");
 
-        let expected_hash = transaction.hash();
+        let expected_hash = transaction.unmined_id();
 
         // Initialize the verifier
         let state_service =
@@ -747,7 +747,7 @@ fn v4_with_sapling_outputs_and_no_spends() {
             })
             .expect("No transaction found with Sapling outputs and no Sapling spends");
 
-        let expected_hash = transaction.hash();
+        let expected_hash = transaction.unmined_id();
 
         // Initialize the verifier
         let state_service =
@@ -785,7 +785,7 @@ fn v5_with_sapling_spends() {
                 .find(|transaction| transaction.sapling_spends_per_anchor().next().is_some())
                 .expect("No transaction found with Sapling spends");
 
-        let expected_hash = transaction.hash();
+        let expected_hash = transaction.unmined_id();
         let height = transaction
             .expiry_height()
             .expect("Transaction is missing expiry height");