From ab471b0db0a9579275e4c648f61c8795f5d65a04 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 1 Dec 2021 15:09:54 +1000 Subject: [PATCH] Revert "Stop returning NotFound errors, use the response instead" (#3124) * Revert "Stop returning NotFound errors, use the response instead" This reverts commit 45871f6915c0b294502bf04917c42fdcd3b1075c. * Fix clippy warnings * Downgrade a frequent log to debug level --- zebra-network/src/peer/connection.rs | 107 ++++++++---------- zebra-network/src/peer/error.rs | 9 ++ .../src/protocol/external/message.rs | 21 ++++ .../src/protocol/internal/request.rs | 22 +++- .../src/protocol/internal/response.rs | 25 ++++ 5 files changed, 118 insertions(+), 66 deletions(-) diff --git a/zebra-network/src/peer/connection.rs b/zebra-network/src/peer/connection.rs index bc55a8a98..e7ce38fb7 100644 --- a/zebra-network/src/peer/connection.rs +++ b/zebra-network/src/peer/connection.rs @@ -160,20 +160,19 @@ impl Handler { // connection open, so the inbound service can process transactions from good // peers (case 2). ignored_msg = Some(Message::Tx(transaction)); - - if !pending_ids.is_empty() { + if !transactions.is_empty() { // if our peers start sending mixed solicited and unsolicited transactions, // we should update this code to handle those responses - info!( - "unexpected transaction from peer: \ - transaction responses should be sent in a continuous sequence, \ - followed by notfound. \ - Using partial received transactions as the peer response" - ); + error!("unexpected transaction from peer: transaction responses should be sent in a continuous batch, followed by notfound. Using partial received transactions as the peer response"); + // TODO: does the caller need a list of missing transactions? (#1515) + Handler::Finished(Ok(Response::Transactions(transactions))) + } 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? + // Should we fake a NotFound response here? (#1515) + let missing_transaction_ids = pending_ids.iter().map(Into::into).collect(); + Handler::Finished(Err(PeerError::NotFound(missing_transaction_ids))) } - - // TODO: does the caller need a list of missing transactions? (#1515) - Handler::Finished(Ok(Response::Transactions(transactions))) } } // `zcashd` peers actually return this response @@ -194,27 +193,24 @@ impl Handler { // 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_transaction_ids: HashSet<_> = transaction_ids(&missing_invs).collect(); - if missing_transaction_ids != pending_ids { trace!(?missing_invs, ?missing_transaction_ids, ?pending_ids); - info!( - "unexpected notfound message from peer: \ - all remaining transaction hashes should be listed in the notfound. \ - Using partial received transactions as the peer response" - ); + // 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_transaction_ids.len() != missing_invs.len() { trace!(?missing_invs, ?missing_transaction_ids, ?pending_ids); - info!( - "unexpected notfound message from peer: \ - notfound contains duplicate hashes or non-transaction hashes. \ - Using partial received transactions as the peer response" - ); + error!("unexpected notfound message from peer: notfound contains duplicate hashes or non-transaction hashes. Using partial received transactions as the peer response"); } - // TODO: does the caller need a list of missing transactions? (#1515) - Handler::Finished(Ok(Response::Transactions(transactions))) + if !transactions.is_empty() { + // TODO: does the caller need a list of missing transactions? (#1515) + Handler::Finished(Ok(Response::Transactions(transactions))) + } 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(missing_invs))) + } } // `zcashd` returns requested blocks in a single batch of messages. // Other blocks or non-blocks messages can come before or after the batch. @@ -256,19 +252,19 @@ impl Handler { // But we keep the connection open, so the inbound service can process blocks // from good peers (case 2). ignored_msg = Some(Message::Block(block)); - - if !pending_hashes.is_empty() { - // if our peers start sending mixed solicited and unsolicited blocks, - // we should update this code to handle those responses - info!( - "unexpected block from peer: \ - block responses should be sent in a continuous sequence. \ - Using partial received blocks as the peer response" - ); + if !blocks.is_empty() { + // TODO: does the caller need a list of missing blocks? (#1515) + Handler::Finished(Ok(Response::Blocks(blocks))) + } else { + // TODO: is it really an error if we ask for a block 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 = pending_hashes + .iter() + .map(|h| InventoryHash::Block(*h)) + .collect(); + Handler::Finished(Err(PeerError::NotFound(items))) } - - // TODO: does the caller need a list of missing blocks? (#1515) - Handler::Finished(Ok(Response::Blocks(blocks))) } } // peers are allowed to return this response, but `zcashd` never does @@ -296,37 +292,24 @@ impl Handler { }) .cloned() .collect(); - if missing_blocks != pending_hashes { trace!(?items, ?missing_blocks, ?pending_hashes); - info!( - "unexpected notfound message from peer: \ - all remaining block hashes should be listed in the notfound. \ - Using partial received blocks as the peer response" - ); + // if these errors are noisy, we should replace them with debugs + error!("unexpected notfound message from peer: all remaining block hashes should be listed in the notfound. Using partial received blocks as the peer response"); } - if missing_blocks.len() != items.len() { trace!(?items, ?missing_blocks, ?pending_hashes); - info!( - "unexpected notfound message from peer: \ - notfound contains duplicate hashes or non-block hashes. \ - Using partial received blocks as the peer response" - ); + error!("unexpected notfound message from peer: notfound contains duplicate hashes or non-block hashes. Using partial received blocks as the peer response"); } - if !pending_hashes.is_empty() { - // if our peers start sending mixed solicited and unsolicited blocks, - // we should update this code to handle those responses - info!( - "unexpected block from peer: \ - block responses should be sent in a continuous sequence. \ - Using partial received blocks as the peer response" - ); + if !blocks.is_empty() { + // TODO: does the caller need a list of missing blocks? (#1515) + Handler::Finished(Ok(Response::Blocks(blocks))) + } else { + // TODO: is it really an error if we ask for a block 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))) } - - // TODO: does the caller need a list of missing blocks? (#1515) - Handler::Finished(Ok(Response::Blocks(blocks))) } (Handler::FindBlocks, Message::Inv(items)) if items @@ -556,11 +539,11 @@ where pending @ State::AwaitingResponse { .. } => { // Drop the un-consumed request message, // because we can't process multiple messages at the same time. - info!( + debug!( new_request = %request_msg .as_ref() .map(|m| m.to_string()) - .unwrap_or_else(|| "None".to_string()), + .unwrap_or_else(|| "None".into()), awaiting_response = %pending, "ignoring new request while awaiting a response" ); diff --git a/zebra-network/src/peer/error.rs b/zebra-network/src/peer/error.rs index 2bf1d3a79..a4bc01fd1 100644 --- a/zebra-network/src/peer/error.rs +++ b/zebra-network/src/peer/error.rs @@ -5,6 +5,8 @@ use thiserror::Error; use tracing_error::TracedError; use zebra_chain::serialization::SerializationError; +use crate::protocol::external::InventoryHash; + /// A wrapper around `Arc` that implements `Error`. #[derive(Error, Debug, Clone)] #[error(transparent)] @@ -44,6 +46,13 @@ pub enum PeerError { /// to shed load. #[error("Internal services over capacity")] Overloaded, + + // TODO: stop closing connections on these errors (#2107) + // log info or debug logs instead + // + /// We requested data that the peer couldn't find. + #[error("Remote peer could not find items: {0:?}")] + NotFound(Vec), } /// A shared error slot for peer errors. diff --git a/zebra-network/src/protocol/external/message.rs b/zebra-network/src/protocol/external/message.rs index f14235692..89a2ea6cc 100644 --- a/zebra-network/src/protocol/external/message.rs +++ b/zebra-network/src/protocol/external/message.rs @@ -145,6 +145,9 @@ pub enum Message { /// - parses received `addrv2` messages, ignoring some address types, /// - but does not send `addrv2` messages. /// + /// + /// The list contains `0..=MAX_META_ADDR` addresses. + /// /// Because some address types are ignored, the deserialized vector can be empty, /// even if the peer sent addresses. This is not an error. /// @@ -163,6 +166,8 @@ pub enum Message { /// If supplied, the `stop` parameter specifies the last header to request. /// Otherwise, an inv packet with the maximum number (500) are sent. /// + /// The known blocks list contains zero or more block hashes. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#getheaders) GetBlocks { /// Hashes of known blocks, ordered from highest height to lowest height. @@ -177,6 +182,8 @@ pub enum Message { /// objects. It can be received unsolicited, or in reply to /// `getblocks`. /// + /// The list contains zero or more inventory hashes. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#inv) /// [ZIP-239](https://zips.z.cash/zip-0239) Inv(Vec), @@ -192,6 +199,8 @@ pub enum Message { /// If supplied, the `stop` parameter specifies the last header to request. /// Otherwise, the maximum number of block headers (160) are sent. /// + /// The known blocks list contains zero or more block hashes. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#getheaders) GetHeaders { /// Hashes of known blocks, ordered from highest height to lowest height. @@ -206,6 +215,8 @@ pub enum Message { /// /// Each block header is accompanied by a transaction count. /// + /// The list contains zero or more headers. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#headers) Headers(Vec), @@ -220,6 +231,8 @@ pub enum Message { /// included in a single `NotFound` message following the transactions. /// Other item or non-item messages can come before or after the batch. /// + /// The list contains zero or more inventory hashes. + /// /// [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) @@ -247,6 +260,8 @@ pub enum Message { /// But when a peer requests blocks or headers, any missing items are /// silently skipped, without any `NotFound` messages. /// + /// The list contains zero or more inventory hashes. + /// /// [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) @@ -265,6 +280,8 @@ pub enum Message { /// /// This was defined in [BIP37], which is included in Zcash. /// + /// Zebra currently ignores this message. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#filterload.2C_filteradd.2C_filterclear.2C_merkleblock) /// [BIP37]: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki FilterLoad { @@ -288,6 +305,8 @@ pub enum Message { /// /// This was defined in [BIP37], which is included in Zcash. /// + /// Zebra currently ignores this message. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#filterload.2C_filteradd.2C_filterclear.2C_merkleblock) /// [BIP37]: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki FilterAdd { @@ -304,6 +323,8 @@ pub enum Message { /// /// This was defined in [BIP37], which is included in Zcash. /// + /// Zebra currently ignores this message. + /// /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#filterload.2C_filteradd.2C_filterclear.2C_merkleblock) /// [BIP37]: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki FilterClear, diff --git a/zebra-network/src/protocol/internal/request.rs b/zebra-network/src/protocol/internal/request.rs index b0330ee12..3cdddd54d 100644 --- a/zebra-network/src/protocol/internal/request.rs +++ b/zebra-network/src/protocol/internal/request.rs @@ -63,6 +63,8 @@ pub enum Request { /// block. This routing is only used for request sets of size 1. /// Otherwise, it is routed using the normal load-balancing strategy. /// + /// The list contains zero or more block hashes. + /// /// # Returns /// /// Returns [`Response::Blocks`](super::Response::Blocks). @@ -80,6 +82,8 @@ pub enum Request { /// the transaction. This routing is only used for request sets of size 1. /// Otherwise, it is routed using the normal load-balancing strategy. /// + /// The list contains zero or more unmined transaction IDs. + /// /// # Returns /// /// Returns [`Response::Transactions`](super::Response::Transactions). @@ -88,6 +92,8 @@ pub enum Request { /// Request block hashes of subsequent blocks in the chain, given hashes of /// known blocks. /// + /// The known blocks list contains zero or more block hashes. + /// /// # Returns /// /// Returns @@ -106,6 +112,8 @@ pub enum Request { /// `inv` messages will always have exactly one block hash. FindBlocks { /// Hashes of known blocks, ordered from highest height to lowest height. + // + // TODO: make this into an IndexMap - an ordered unique list of hashes (#2244) known_blocks: Vec, /// Optionally, the last block hash to request. stop: Option, @@ -114,12 +122,16 @@ pub enum Request { /// Request headers of subsequent blocks in the chain, given hashes of /// known blocks. /// + /// The known blocks list contains zero or more block hashes. + /// /// # Returns /// /// Returns /// [`Response::BlockHeaders`](super::Response::BlockHeaders). FindHeaders { /// Hashes of known blocks, ordered from highest height to lowest height. + // + // TODO: make this into an IndexMap - an ordered unique list of hashes (#2244) known_blocks: Vec, /// Optionally, the last header to request. stop: Option, @@ -150,8 +162,10 @@ pub enum Request { /// 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. + /// The list contains zero or more transaction IDs. + /// + /// The peer set routes this request specially, sending it to *half of* + /// the available peers. /// /// # Returns /// @@ -166,8 +180,8 @@ pub enum Request { /// [`Request::BlocksByHash`] against the "inbound" service passed to /// [`zebra_network::init`]. /// - /// The peer set routes this request specially, sending it to *every* - /// available peer. + /// The peer set routes this request specially, sending it to *half of* + /// the available peers. /// /// # Returns /// diff --git a/zebra-network/src/protocol/internal/response.rs b/zebra-network/src/protocol/internal/response.rs index b266d371a..85b10680d 100644 --- a/zebra-network/src/protocol/internal/response.rs +++ b/zebra-network/src/protocol/internal/response.rs @@ -26,24 +26,49 @@ pub enum Response { Nil, /// A list of peers, used to respond to `GetPeers`. + /// + /// The list contains `0..=MAX_META_ADDR` peers. + // + // TODO: make this into a HashMap - a unique list of peer addresses (#2244) Peers(Vec), /// A list of blocks. + /// + /// The list contains zero or more blocks. + // + // TODO: split this into found and not found (#2726) Blocks(Vec>), /// A list of block hashes. + /// + /// The list contains zero or more block hashes. + // + // TODO: make this into an IndexMap - an ordered unique list of hashes (#2244) BlockHashes(Vec), /// A list of block headers. + /// + /// The list contains zero or more block headers. + // + // TODO: make this into a HashMap - a unique list of headers (#2244) + // split this into found and not found (#2726) BlockHeaders(Vec), /// A list of unmined transactions. + /// + /// The list contains zero or more unmined transactions. + // + // TODO: split this into found and not found (#2726) Transactions(Vec), /// A list of unmined transaction IDs. /// /// v4 transactions use a legacy transaction ID, and /// v5 transactions use a witnessed transaction ID. + /// + /// The list contains zero or more transaction IDs. + // + // TODO: make this into a HashSet - a unique list of transaction IDs (#2244) TransactionIds(Vec), }