From 2ed6679069b9c0aac752e748371e96281a9661e1 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 26 Aug 2021 07:02:47 +1000 Subject: [PATCH] Use unmined types for transaction verifier mempool requests and responses (#2666) * Add `Arc` 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 --- zebra-chain/src/transaction.rs | 15 ++++++- zebra-chain/src/transaction/auth_digest.rs | 13 +++++- zebra-chain/src/transaction/hash.rs | 18 +++++++++ zebra-chain/src/transaction/unmined.rs | 6 +++ zebra-consensus/src/transaction.rs | 46 ++++++++++++++++------ zebra-consensus/src/transaction/tests.rs | 18 ++++----- 6 files changed, 91 insertions(+), 25 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index cc002d51..a867d539 100644 --- a/zebra-chain/src/transaction.rs +++ b/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 diff --git a/zebra-chain/src/transaction/auth_digest.rs b/zebra-chain/src/transaction/auth_digest.rs index 914e64e9..d42605ca 100644 --- a/zebra-chain/src/transaction/auth_digest.rs +++ b/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> for AuthDigest { + /// Computes the authorizing data commitment for a transaction. + /// + /// # Panics + /// + /// If passed a pre-v5 transaction. + fn from(transaction: Arc) -> Self { + transaction.as_ref().into() + } +} + impl From<[u8; 32]> for AuthDigest { fn from(bytes: [u8; 32]) -> Self { Self(bytes) diff --git a/zebra-chain/src/transaction/hash.rs b/zebra-chain/src/transaction/hash.rs index a95f2dd2..41298f85 100644 --- a/zebra-chain/src/transaction/hash.rs +++ b/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> for Hash { + fn from(transaction: Arc) -> 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> for WtxId { + /// Computes the witnessed transaction ID for a transaction. + /// + /// # Panics + /// + /// If passed a pre-v5 transaction. + fn from(transaction: Arc) -> 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"); diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index 7f1bc3a4..afddc5e5 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -75,6 +75,12 @@ impl From<&Transaction> for UnminedTxId { } } +impl From> for UnminedTxId { + fn from(transaction: Arc) -> Self { + transaction.as_ref().into() + } +} + impl From for UnminedTxId { fn from(wtx_id: WtxId) -> Self { Witnessed(wtx_id) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 626c6da6..2126218e 100644 --- a/zebra-consensus/src/transaction.rs +++ b/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: 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 { 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 Service for Verifier @@ -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 { - Request::Block { .. } => false, - Request::Mempool { .. } => true, - }; - if is_mempool { + if req.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() diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index e5aec48d..800b06db 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/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");