diff --git a/zebra-chain/src/transaction/hash.rs b/zebra-chain/src/transaction/hash.rs index 9d1c57b21..a95f2dd24 100644 --- a/zebra-chain/src/transaction/hash.rs +++ b/zebra-chain/src/transaction/hash.rs @@ -1,15 +1,25 @@ //! Transaction identifiers for Zcash. //! //! Zcash has two different transaction identifiers, with different widths: -//! * [`Hash`]: a 32-byte narrow transaction ID, which uniquely identifies mined transactions +//! * [`Hash`]: a 32-byte transaction ID, which uniquely identifies mined transactions //! (transactions that have been committed to the blockchain in blocks), and -//! * [`WtxId`]: a 64-byte wide transaction ID, which uniquely identifies unmined transactions +//! * [`WtxId`]: a 64-byte witnessed transaction ID, which uniquely identifies unmined transactions //! (transactions that are sent by wallets or stored in node mempools). //! -//! Transaction version 5 is uniquely identified by [`WtxId`] when unmined, and [`Hash`] in the blockchain. -//! Transaction versions 1-4 are uniquely identified by narrow transaction IDs, -//! whether they have been mined or not, -//! so Zebra and the Zcash network protocol don't use wide transaction IDs for them. +//! Transaction version 5 uses both these unique identifiers: +//! * [`Hash`] uniquely identifies the effects of a v5 transaction (spends and outputs), +//! so it uniquely identifies the transaction's data after it has been mined into a block; +//! * [`WtxId`] uniquely identifies the effects and authorizing data of a v5 transaction +//! (signatures, proofs, and scripts), so it uniquely identifies the transaction's data +//! outside a block. (For example, transactions produced by Zcash wallets, or in node mempools.) +//! +//! Transaction versions 1-4 are uniquely identified by legacy [`Hash`] transaction IDs, +//! whether they have been mined or not. So Zebra, and the Zcash network protocol, +//! don't use witnessed transaction IDs for them. +//! +//! There is no unique identifier that only covers the effects of a v1-4 transaction, +//! so their legacy IDs are malleable, if submitted with different authorizing data. +//! So the same spends and outputs can have a completely different [`Hash`]. //! //! Zebra's [`UnminedTxId`] and [`UnminedTx`] enums provide the correct unique ID for //! unmined transactions. They can be used to handle transactions regardless of version, @@ -31,7 +41,7 @@ use crate::serialization::{ use super::{txid::TxIdBuilder, AuthDigest, Transaction}; -/// A narrow transaction ID, which uniquely identifies mined v5 transactions, +/// A transaction ID, which uniquely identifies mined v5 transactions, /// and all v1-v4 transactions. /// /// Note: Zebra displays transaction and block hashes in big-endian byte-order, @@ -127,9 +137,9 @@ impl ZcashDeserialize for Hash { } } -/// A wide transaction ID, which uniquely identifies unmined v5 transactions. +/// A witnessed transaction ID, which uniquely identifies unmined v5 transactions. /// -/// Wide transaction IDs are not used for transaction versions 1-4. +/// Witnessed transaction IDs are not used for transaction versions 1-4. /// /// "A v5 transaction also has a wtxid (used for example in the peer-to-peer protocol) /// as defined in [ZIP-239]." @@ -148,14 +158,14 @@ pub struct WtxId { } impl WtxId { - /// Return this wide transaction ID as a serialized byte array. + /// Return this witnessed transaction ID as a serialized byte array. pub fn as_bytes(&self) -> [u8; 64] { <[u8; 64]>::from(self) } } impl From for WtxId { - /// Computes the wide transaction ID for a transaction. + /// Computes the witnessed transaction ID for a transaction. /// /// # Panics /// @@ -167,7 +177,7 @@ impl From for WtxId { } impl From<&Transaction> for WtxId { - /// Computes the wide transaction ID for a transaction. + /// Computes the witnessed transaction ID for a transaction. /// /// # Panics /// diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index e5b9afa42..7f1bc3a46 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -1,9 +1,12 @@ //! Unmined Zcash transaction identifiers and transactions. //! -//! Transaction version 5 is uniquely identified by [`WtxId`] when unmined, and [`Hash`] in the blockchain. -//! Transaction versions 1-4 are uniquely identified by narrow transaction IDs, -//! whether they have been mined or not, -//! so Zebra and the Zcash network protocol don't use wide transaction IDs for them. +//! Transaction version 5 is uniquely identified by [`WtxId`] when unmined, +//! and [`Hash`] in the blockchain. The effects of a v5 transaction (spends and outputs) +//! are uniquely identified by the same [`Hash`] in both cases. +//! +//! Transaction versions 1-4 are uniquely identified by legacy [`Hash`] transaction IDs, +//! whether they have been mined or not. So Zebra, and the Zcash network protocol, +//! don't use witnessed transaction IDs for them. //! //! Zebra's [`UnminedTxId`] and [`UnminedTx`] enums provide the correct unique ID for //! unmined transactions. They can be used to handle transactions regardless of version, @@ -39,19 +42,21 @@ use UnminedTxId::*; #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))] pub enum UnminedTxId { - /// A narrow unmined transaction identifier. + /// A legacy unmined transaction identifier. /// /// Used to uniquely identify unmined version 1-4 transactions. - /// (After v1-4 transactions are mined, they can be uniquely identified using the same [`transaction::Hash`].) - Narrow(Hash), + /// (After v1-4 transactions are mined, they can be uniquely identified + /// using the same [`transaction::Hash`].) + Legacy(Hash), - /// A wide unmined transaction identifier. + /// A witnessed unmined transaction identifier. /// /// Used to uniquely identify unmined version 5 transactions. - /// (After v5 transactions are mined, they can be uniquely identified using only their `WtxId.id`.) + /// (After v5 transactions are mined, they can be uniquely identified + /// using only the [`transaction::Hash`] in their `WtxId.id`.) /// /// For more details, see [`WtxId`]. - Wide(WtxId), + Witnessed(WtxId), } impl From for UnminedTxId { @@ -64,15 +69,15 @@ impl From for UnminedTxId { impl From<&Transaction> for UnminedTxId { fn from(transaction: &Transaction) -> Self { match transaction { - V1 { .. } | V2 { .. } | V3 { .. } | V4 { .. } => Narrow(transaction.into()), - V5 { .. } => Wide(transaction.into()), + V1 { .. } | V2 { .. } | V3 { .. } | V4 { .. } => Legacy(transaction.into()), + V5 { .. } => Witnessed(transaction.into()), } } } impl From for UnminedTxId { fn from(wtx_id: WtxId) -> Self { - Wide(wtx_id) + Witnessed(wtx_id) } } @@ -91,23 +96,23 @@ impl UnminedTxId { /// [`Hash`] does not uniquely identify unmined v5 transactions. #[allow(dead_code)] pub fn from_legacy_id(legacy_tx_id: Hash) -> UnminedTxId { - Narrow(legacy_tx_id) + Legacy(legacy_tx_id) } - /// Return the unique ID for this transaction's effects. + /// Return the unique ID that will be used if this transaction gets mined into a block. /// /// # Correctness /// - /// This method returns an ID which uniquely identifies - /// the effects (spends and outputs) and - /// authorizing data (signatures, proofs, and scripts) for v1-v4 transactions. + /// For v1-v4 transactions, this method returns an ID which changes + /// if this transaction's effects (spends and outputs) change, or + /// if its authorizing data changes (signatures, proofs, and scripts). /// - /// But for v5 transactions, this ID only identifies the transaction's effects. + /// But for v5 transactions, this ID uniquely identifies the transaction's effects. #[allow(dead_code)] - pub fn effect_id(&self) -> Hash { + pub fn mined_id(&self) -> Hash { match self { - Narrow(effect_id) => *effect_id, - Wide(wtx_id) => wtx_id.id, + Legacy(legacy_id) => *legacy_id, + Witnessed(wtx_id) => wtx_id.id, } } @@ -116,8 +121,8 @@ impl UnminedTxId { #[allow(dead_code)] pub fn auth_digest(&self) -> Option { match self { - Narrow(_effect_id) => None, - Wide(wtx_id) => Some(wtx_id.auth_digest), + Legacy(_) => None, + Witnessed(wtx_id) => Some(wtx_id.auth_digest), } } } diff --git a/zebra-network/src/peer/connection.rs b/zebra-network/src/peer/connection.rs index 4fa9c0135..6635f49ba 100644 --- a/zebra-network/src/peer/connection.rs +++ b/zebra-network/src/peer/connection.rs @@ -21,7 +21,7 @@ use tracing_futures::Instrument; use zebra_chain::{ block::{self, Block}, serialization::SerializationError, - transaction::{self, Transaction}, + transaction::{UnminedTx, UnminedTxId}, }; use crate::{ @@ -51,11 +51,11 @@ pub(super) enum Handler { hashes: HashSet, blocks: Vec>, }, - TransactionsByHash { - hashes: HashSet, - transactions: Vec>, + TransactionsById { + pending_ids: HashSet, + transactions: Vec, }, - MempoolTransactions, + MempoolTransactionIds, } impl Handler { @@ -91,8 +91,8 @@ impl Handler { // After the transaction batch, `zcashd` sends `NotFound` if any transactions are missing: // https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5617 ( - Handler::TransactionsByHash { - mut hashes, + Handler::TransactionsById { + mut pending_ids, mut transactions, }, Message::Tx(transaction), @@ -100,14 +100,14 @@ impl Handler { // assumptions: // - the transaction messages are sent in a single continous batch // - missing transaction hashes are included in a `NotFound` message - if hashes.remove(&transaction.hash()) { + if pending_ids.remove(&transaction.id) { // we are in the middle of the continous transaction messages transactions.push(transaction); - if hashes.is_empty() { + if pending_ids.is_empty() { Handler::Finished(Ok(Response::Transactions(transactions))) } else { - Handler::TransactionsByHash { - hashes, + Handler::TransactionsById { + pending_ids, transactions, } } @@ -137,18 +137,18 @@ impl Handler { // TODO: is it really an error if we ask for a transaction hash, but the peer // doesn't know it? Should we close the connection on that kind of error? // Should we fake a NotFound response here? (#1515) - let items = hashes.iter().map(|h| InventoryHash::Tx(*h)).collect(); - Handler::Finished(Err(PeerError::NotFound(items))) + let missing_transaction_ids = pending_ids.iter().map(Into::into).collect(); + Handler::Finished(Err(PeerError::NotFound(missing_transaction_ids))) } } } // `zcashd` peers actually return this response ( - Handler::TransactionsByHash { - hashes, + Handler::TransactionsById { + pending_ids, transactions, }, - Message::NotFound(items), + Message::NotFound(missing_invs), ) => { // assumptions: // - the peer eventually returns a transaction or a `NotFound` entry @@ -159,21 +159,14 @@ impl Handler { // If we're in sync with the peer, then the `NotFound` should contain the remaining // hashes from the handler. If we're not in sync with the peer, we should return // what we got so far, and log an error. - let missing_transactions: HashSet<_> = items - .iter() - .filter_map(|inv| match &inv { - InventoryHash::Tx(tx) => Some(tx), - _ => None, - }) - .cloned() - .collect(); - if missing_transactions != hashes { - trace!(?items, ?missing_transactions, ?hashes); + let missing_transaction_ids: HashSet<_> = transaction_ids(&missing_invs).collect(); + if missing_transaction_ids != pending_ids { + trace!(?missing_invs, ?missing_transaction_ids, ?pending_ids); // if these errors are noisy, we should replace them with debugs error!("unexpected notfound message from peer: all remaining transaction hashes should be listed in the notfound. Using partial received transactions as the peer response"); } - if missing_transactions.len() != items.len() { - trace!(?items, ?missing_transactions, ?hashes); + if missing_transaction_ids.len() != missing_invs.len() { + trace!(?missing_invs, ?missing_transaction_ids, ?pending_ids); error!("unexpected notfound message from peer: notfound contains duplicate hashes or non-transaction hashes. Using partial received transactions as the peer response"); } @@ -183,7 +176,7 @@ impl Handler { } else { // TODO: is it really an error if we ask for a transaction hash, but the peer // doesn't know it? Should we close the connection on that kind of error? (#1515) - Handler::Finished(Err(PeerError::NotFound(items))) + Handler::Finished(Err(PeerError::NotFound(missing_invs))) } } // `zcashd` returns requested blocks in a single batch of messages. @@ -282,13 +275,11 @@ impl Handler { block_hashes(&items[..]).collect(), ))) } - (Handler::MempoolTransactions, Message::Inv(items)) - if items - .iter() - .all(|item| matches!(item, InventoryHash::Tx(_))) => + (Handler::MempoolTransactionIds, Message::Inv(items)) + if items.iter().all(|item| item.unmined_tx_id().is_some()) => { - Handler::Finished(Ok(Response::TransactionHashes( - transaction_hashes(&items[..]).collect(), + Handler::Finished(Ok(Response::TransactionIds( + transaction_ids(&items).collect(), ))) } (Handler::FindHeaders, Message::Headers(headers)) => { @@ -667,19 +658,19 @@ where Err(e) => Err((e, tx)), } } - (AwaitingRequest, TransactionsByHash(hashes)) => { + (AwaitingRequest, TransactionsById(ids)) => { match self .peer_tx .send(Message::GetData( - hashes.iter().map(|h| (*h).into()).collect(), + ids.iter().map(Into::into).collect(), )) .await { Ok(()) => Ok(( AwaitingResponse { - handler: Handler::TransactionsByHash { - transactions: Vec::with_capacity(hashes.len()), - hashes, + handler: Handler::TransactionsById { + transactions: Vec::with_capacity(ids.len()), + pending_ids: ids, }, tx, span, @@ -723,11 +714,11 @@ where Err(e) => Err((e, tx)), } } - (AwaitingRequest, MempoolTransactions) => { + (AwaitingRequest, MempoolTransactionIds) => { match self.peer_tx.send(Message::Mempool).await { Ok(()) => Ok(( AwaitingResponse { - handler: Handler::MempoolTransactions, + handler: Handler::MempoolTransactionIds, tx, span, }, @@ -742,7 +733,7 @@ where Err(e) => Err((e, tx)), } } - (AwaitingRequest, AdvertiseTransactions(hashes)) => { + (AwaitingRequest, AdvertiseTransactionIds(hashes)) => { match self .peer_tx .send(Message::Inv(hashes.iter().map(|h| (*h).into()).collect())) @@ -858,10 +849,11 @@ where // We don't expect to be advertised multiple blocks at a time, // so we ignore any advertisements of multiple blocks. [InventoryHash::Block(hash)] => Request::AdvertiseBlock(*hash), - [InventoryHash::Tx(_), rest @ ..] - if rest.iter().all(|item| matches!(item, InventoryHash::Tx(_))) => + tx_ids + if tx_ids.iter().all(|item| item.unmined_tx_id().is_some()) + && !tx_ids.is_empty() => { - Request::TransactionsByHash(transaction_hashes(&items).collect()) + Request::TransactionsById(transaction_ids(&items).collect()) } _ => { self.fail_with(PeerError::WrongMessage("inv with mixed item types")); @@ -876,10 +868,11 @@ where { Request::BlocksByHash(block_hashes(&items).collect()) } - [InventoryHash::Tx(_), rest @ ..] - if rest.iter().all(|item| matches!(item, InventoryHash::Tx(_))) => + tx_ids + if tx_ids.iter().all(|item| item.unmined_tx_id().is_some()) + && !tx_ids.is_empty() => { - Request::TransactionsByHash(transaction_hashes(&items).collect()) + Request::TransactionsById(transaction_ids(&items).collect()) } _ => { self.fail_with(PeerError::WrongMessage("getdata with mixed item types")); @@ -891,7 +884,7 @@ where Message::GetHeaders { known_blocks, stop } => { Request::FindHeaders { known_blocks, stop } } - Message::Mempool => Request::MempoolTransactions, + Message::Mempool => Request::MempoolTransactionIds, }; self.drive_peer_request(req).await @@ -973,7 +966,7 @@ where self.fail_with(e) } } - Response::TransactionHashes(hashes) => { + Response::TransactionIds(hashes) => { if let Err(e) = self .peer_tx .send(Message::Inv(hashes.into_iter().map(Into::into).collect())) @@ -986,16 +979,17 @@ where } } -fn transaction_hashes(items: &'_ [InventoryHash]) -> impl Iterator + '_ { - items.iter().filter_map(|item| { - if let InventoryHash::Tx(hash) = item { - Some(*hash) - } else { - None - } - }) +/// Map a list of inventory hashes to the corresponding unmined transaction IDs. +/// Non-transaction inventory hashes are skipped. +/// +/// v4 transactions use a legacy transaction ID, and +/// v5 transactions use a witnessed transaction ID. +fn transaction_ids(items: &'_ [InventoryHash]) -> impl Iterator + '_ { + items.iter().filter_map(InventoryHash::unmined_tx_id) } +/// Map a list of inventory hashes to the corresponding block hashes. +/// Non-block inventory hashes are skipped. fn block_hashes(items: &'_ [InventoryHash]) -> impl Iterator + '_ { items.iter().filter_map(|item| { if let InventoryHash::Block(hash) = item { diff --git a/zebra-network/src/peer/handshake.rs b/zebra-network/src/peer/handshake.rs index d0035b71b..ba5c5b9a5 100644 --- a/zebra-network/src/peer/handshake.rs +++ b/zebra-network/src/peer/handshake.rs @@ -803,20 +803,21 @@ where // // TODO: zcashd has a bug where it merges queued inv messages of // the same or different types. So Zebra should split small - // merged inv messages into separate inv messages. (#1799) + // merged inv messages into separate inv messages. (#1768) match hashes.as_slice() { [hash @ InventoryHash::Block(_)] => { + debug!(?hash, "registering gossiped block inventory for peer"); let _ = inv_collector.send((*hash, transient_addr)); } [hashes @ ..] => { for hash in hashes { - if matches!(hash, InventoryHash::Tx(_)) { - debug!(?hash, "registering Tx inventory hash"); + if let Some(unmined_tx_id) = hash.unmined_tx_id() { + debug!(?unmined_tx_id, "registering unmined transaction inventory for peer"); // The peer set and inv collector use the peer's remote // address as an identifier let _ = inv_collector.send((*hash, transient_addr)); } else { - trace!(?hash, "ignoring non Tx inventory hash") + trace!(?hash, "ignoring non-transaction inventory hash in multi-hash list") } } } diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index 5b5044cf6..485a3b683 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -517,12 +517,16 @@ where let hash = InventoryHash::from(*hashes.iter().next().unwrap()); self.route_inv(req, hash) } - Request::TransactionsByHash(ref hashes) if hashes.len() == 1 => { + Request::TransactionsById(ref hashes) if hashes.len() == 1 => { let hash = InventoryHash::from(*hashes.iter().next().unwrap()); self.route_inv(req, hash) } - Request::AdvertiseTransactions(_) => self.route_all(req), + + // Broadcast advertisements to all peers + Request::AdvertiseTransactionIds(_) => self.route_all(req), Request::AdvertiseBlock(_) => self.route_all(req), + + // Choose a random less-loaded peer for all other requests _ => self.route_p2c(req), }; self.update_metrics(); diff --git a/zebra-network/src/protocol/external/codec.rs b/zebra-network/src/protocol/external/codec.rs index 070d70b4d..8288a8f9e 100644 --- a/zebra-network/src/protocol/external/codec.rs +++ b/zebra-network/src/protocol/external/codec.rs @@ -276,7 +276,7 @@ impl Codec { Message::Inv(hashes) => hashes.zcash_serialize(&mut writer)?, Message::GetData(hashes) => hashes.zcash_serialize(&mut writer)?, Message::NotFound(hashes) => hashes.zcash_serialize(&mut writer)?, - Message::Tx(transaction) => transaction.zcash_serialize(&mut writer)?, + Message::Tx(transaction) => transaction.transaction.zcash_serialize(&mut writer)?, Message::Mempool => { /* Empty payload -- no-op */ } Message::FilterLoad { filter, @@ -910,17 +910,17 @@ mod tests { #[test] fn max_msg_size_round_trip() { - use std::sync::Arc; use zebra_chain::serialization::ZcashDeserializeInto; + zebra_test::init(); let rt = Runtime::new().unwrap(); // make tests with a Tx message - let tx = zebra_test::vectors::DUMMY_TX1 + let tx: Transaction = zebra_test::vectors::DUMMY_TX1 .zcash_deserialize_into() .unwrap(); - let msg = Message::Tx(Arc::new(tx)); + let msg = Message::Tx(tx.into()); use tokio_util::codec::{FramedRead, FramedWrite}; diff --git a/zebra-network/src/protocol/external/inv.rs b/zebra-network/src/protocol/external/inv.rs index d02cadd36..ed4de5d30 100644 --- a/zebra-network/src/protocol/external/inv.rs +++ b/zebra-network/src/protocol/external/inv.rs @@ -10,7 +10,11 @@ use zebra_chain::{ ReadZcashExt, SerializationError, TrustedPreallocate, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize, }, - transaction, + transaction::{ + self, + UnminedTxId::{self, *}, + WtxId, + }, }; use super::MAX_PROTOCOL_MESSAGE_LEN; @@ -51,6 +55,29 @@ pub enum InventoryHash { } impl InventoryHash { + /// Creates a new inventory hash from a legacy transaction ID. + /// + /// # Correctness + /// + /// This method must only be used for v1-v4 transaction IDs. + /// [`transaction::Hash`] does not uniquely identify unmined v5 transactions. + #[allow(dead_code)] + pub fn from_legacy_tx_id(legacy_tx_id: transaction::Hash) -> InventoryHash { + InventoryHash::Tx(legacy_tx_id) + } + + /// Returns the unmined transaction ID for this inventory hash, + /// if this inventory hash is a transaction variant. + pub fn unmined_tx_id(&self) -> Option { + match self { + InventoryHash::Error => None, + InventoryHash::Tx(legacy_tx_id) => Some(UnminedTxId::from_legacy_id(*legacy_tx_id)), + InventoryHash::Block(_hash) => None, + InventoryHash::FilteredBlock(_hash) => None, + InventoryHash::Wtx(wtx_id) => Some(UnminedTxId::from(wtx_id)), + } + } + /// Returns the serialized Zcash network protocol code for the current variant. fn code(&self) -> u32 { match self { @@ -63,9 +90,30 @@ impl InventoryHash { } } -impl From for InventoryHash { - fn from(tx: transaction::Hash) -> InventoryHash { - InventoryHash::Tx(tx) +impl From for InventoryHash { + fn from(wtx_id: WtxId) -> InventoryHash { + InventoryHash::Wtx(wtx_id) + } +} + +impl From<&WtxId> for InventoryHash { + fn from(wtx_id: &WtxId) -> InventoryHash { + InventoryHash::from(*wtx_id) + } +} + +impl From for InventoryHash { + fn from(tx_id: UnminedTxId) -> InventoryHash { + match tx_id { + Legacy(hash) => InventoryHash::Tx(hash), + Witnessed(wtx_id) => InventoryHash::Wtx(wtx_id), + } + } +} + +impl From<&UnminedTxId> for InventoryHash { + fn from(tx_id: &UnminedTxId) -> InventoryHash { + InventoryHash::from(*tx_id) } } @@ -77,12 +125,6 @@ impl From for InventoryHash { } } -impl From for InventoryHash { - fn from(wtx_id: transaction::WtxId) -> InventoryHash { - InventoryHash::Wtx(wtx_id) - } -} - impl ZcashSerialize for InventoryHash { fn zcash_serialize(&self, mut writer: W) -> Result<(), std::io::Error> { writer.write_u32::(self.code())?; diff --git a/zebra-network/src/protocol/external/message.rs b/zebra-network/src/protocol/external/message.rs index e3a75d947..33ea058c2 100644 --- a/zebra-network/src/protocol/external/message.rs +++ b/zebra-network/src/protocol/external/message.rs @@ -1,21 +1,21 @@ //! Definitions of network messages. -use std::error::Error; -use std::{fmt, net, sync::Arc}; +use std::{error::Error, fmt, net, sync::Arc}; use chrono::{DateTime, Utc}; use zebra_chain::{ block::{self, Block}, - transaction::Transaction, + transaction::UnminedTx, }; -use super::inv::InventoryHash; -use super::types::*; use crate::meta_addr::MetaAddr; +use super::{inv::InventoryHash, types::*}; + #[cfg(any(test, feature = "proptest-impl"))] use proptest_derive::Arbitrary; + #[cfg(any(test, feature = "proptest-impl"))] use zebra_chain::serialization::arbitrary::datetime_full; @@ -169,6 +169,7 @@ pub enum Message { /// `getblocks`. /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#inv) + /// [ZIP-239](https://zips.z.cash/zip-0239) Inv(Vec), /// A `getheaders` message. @@ -211,6 +212,7 @@ pub enum Message { /// Other item or non-item messages can come before or after the batch. /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#getdata) + /// [ZIP-239](https://zips.z.cash/zip-0239) /// [zcashd code](https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5523) GetData(Vec), @@ -221,8 +223,10 @@ pub enum Message { /// A `tx` message. /// + /// This message is used to advertise unmined transactions for the mempool. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#tx) - Tx(Arc), + Tx(UnminedTx), /// A `notfound` message. /// @@ -235,6 +239,7 @@ pub enum Message { /// silently skipped, without any `NotFound` messages. /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#notfound) + /// [ZIP-239](https://zips.z.cash/zip-0239) /// [zcashd code](https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5632) // See note above on `Inventory`. NotFound(Vec), diff --git a/zebra-network/src/protocol/internal/request.rs b/zebra-network/src/protocol/internal/request.rs index a31f7aeaa..976b3f96d 100644 --- a/zebra-network/src/protocol/internal/request.rs +++ b/zebra-network/src/protocol/internal/request.rs @@ -1,8 +1,8 @@ -use std::{collections::HashSet, sync::Arc}; +use std::collections::HashSet; use zebra_chain::{ block, - transaction::{self, Transaction}, + transaction::{UnminedTx, UnminedTxId}, }; use super::super::types::Nonce; @@ -68,7 +68,10 @@ pub enum Request { /// Returns [`Response::Blocks`](super::Response::Blocks). BlocksByHash(HashSet), - /// Request transactions by hash. + /// Request transactions by their unmined transaction ID. + /// + /// v4 transactions use a legacy transaction ID, and + /// v5 transactions use a witnessed transaction ID. /// /// This uses a `HashSet` for the same reason as [`Request::BlocksByHash`]. /// @@ -80,7 +83,7 @@ pub enum Request { /// # Returns /// /// Returns [`Response::Transactions`](super::Response::Transactions). - TransactionsByHash(HashSet), + TransactionsById(HashSet), /// Request block hashes of subsequent blocks in the chain, given hashes of /// known blocks. @@ -122,16 +125,16 @@ pub enum Request { stop: Option, }, - /// Push a transaction to a remote peer, without advertising it to them first. + /// Push an unmined transaction to a remote peer, without advertising it to them first. /// /// This is implemented by sending an unsolicited `tx` message. /// /// # Returns /// /// Returns [`Response::Nil`](super::Response::Nil). - PushTransaction(Arc), + PushTransaction(UnminedTx), - /// Advertise a set of transactions to all peers. + /// Advertise a set of unmined transactions to all peers. /// /// This is intended to be used in Zebra with a single transaction at a time /// (set of size 1), but multiple transactions are permitted because this is @@ -139,18 +142,21 @@ pub enum Request { /// multiple transactions at once. /// /// This is implemented by sending an `inv` message containing the - /// transaction hash, allowing the remote peer to choose whether to download + /// unmined transaction ID, allowing the remote peer to choose whether to download /// it. Remote peers who choose to download the transaction will generate a - /// [`Request::TransactionsByHash`] against the "inbound" service passed to + /// [`Request::TransactionsById`] against the "inbound" service passed to /// [`zebra_network::init`]. /// + /// v4 transactions use a legacy transaction ID, and + /// v5 transactions use a witnessed transaction ID. + /// /// The peer set routes this request specially, sending it to *every* /// available peer. /// /// # Returns /// /// Returns [`Response::Nil`](super::Response::Nil). - AdvertiseTransactions(HashSet), + AdvertiseTransactionIds(HashSet), /// Advertise a block to all peers. /// @@ -172,6 +178,6 @@ pub enum Request { /// /// # Returns /// - /// Returns [`Response::TransactionHashes`](super::Response::TransactionHashes). - MempoolTransactions, + /// Returns [`Response::TransactionIds`](super::Response::TransactionIds). + MempoolTransactionIds, } diff --git a/zebra-network/src/protocol/internal/response.rs b/zebra-network/src/protocol/internal/response.rs index 4cb3c17b5..048fd9486 100644 --- a/zebra-network/src/protocol/internal/response.rs +++ b/zebra-network/src/protocol/internal/response.rs @@ -1,6 +1,6 @@ use zebra_chain::{ block::{self, Block}, - transaction::{self, Transaction}, + transaction::{UnminedTx, UnminedTxId}, }; use crate::meta_addr::MetaAddr; @@ -18,7 +18,11 @@ pub enum Response { /// /// Either: /// * the request does not need a response, or - /// * we have no useful data to provide in response to the request. + /// * we have no useful data to provide in response to the request + /// + /// When Zebra doesn't have any useful data, it always sends no response, + /// instead of sending `notfound`. `zcashd` sometimes sends no response, + /// and sometimes sends `notfound`. Nil, /// A list of peers, used to respond to `GetPeers`. @@ -33,9 +37,12 @@ pub enum Response { /// A list of block headers. BlockHeaders(Vec), - /// A list of transactions. - Transactions(Vec>), + /// A list of unmined transactions. + Transactions(Vec), - /// A list of transaction hashes. - TransactionHashes(Vec), + /// A list of unmined transaction IDs. + /// + /// v4 transactions use a legacy transaction ID, and + /// v5 transactions use a witnessed transaction ID. + TransactionIds(Vec), } diff --git a/zebrad/src/components/inbound.rs b/zebrad/src/components/inbound.rs index e9b341eab..b6372efd6 100644 --- a/zebrad/src/components/inbound.rs +++ b/zebrad/src/components/inbound.rs @@ -281,7 +281,7 @@ impl Service for Inbound { .map_ok(zn::Response::Blocks) .boxed() } - zn::Request::TransactionsByHash(_transactions) => { + zn::Request::TransactionsById(_transactions) => { // `zcashd` returns a list of found transactions, followed by a // `NotFound` message if any transactions are missing. `zcashd` // says that Simplified Payment Verification (SPV) clients rely on @@ -314,7 +314,7 @@ impl Service for Inbound { debug!("ignoring unimplemented request"); async { Ok(zn::Response::Nil) }.boxed() } - zn::Request::AdvertiseTransactions(_transactions) => { + zn::Request::AdvertiseTransactionIds(_transactions) => { debug!("ignoring unimplemented request"); async { Ok(zn::Response::Nil) }.boxed() } @@ -329,7 +329,7 @@ impl Service for Inbound { } async { Ok(zn::Response::Nil) }.boxed() } - zn::Request::MempoolTransactions => { + zn::Request::MempoolTransactionIds => { debug!("ignoring unimplemented request"); async { Ok(zn::Response::Nil) }.boxed() }