diff --git a/zebra-network/src/lib.rs b/zebra-network/src/lib.rs index 045e6094..02615017 100644 --- a/zebra-network/src/lib.rs +++ b/zebra-network/src/lib.rs @@ -71,6 +71,7 @@ pub use crate::{ config::Config, isolated::connect_isolated, meta_addr::PeerAddrState, + peer::{HandshakeError, PeerError, SharedPeerError}, peer_set::init, policies::{RetryErrors, RetryLimit}, protocol::internal::{Request, Response}, diff --git a/zebra-network/src/peer/connection.rs b/zebra-network/src/peer/connection.rs index e55f747b..da037930 100644 --- a/zebra-network/src/peer/connection.rs +++ b/zebra-network/src/peer/connection.rs @@ -874,33 +874,57 @@ 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), - tx_ids - if tx_ids.iter().all(|item| item.unmined_tx_id().is_some()) - && !tx_ids.is_empty() => - { + + // Some peers advertise invs with mixed item types. + // But we're just interested in the transaction invs. + // + // TODO: split mixed invs into multiple requests, + // but skip runs of multiple blocks. + tx_ids if tx_ids.iter().any(|item| item.unmined_tx_id().is_some()) => { Request::AdvertiseTransactionIds(transaction_ids(&items).collect()) } + + // Log detailed messages for ignored inv advertisement messages. + [] => { + debug!("ignoring empty inv"); + return; + } + [InventoryHash::Block(_), InventoryHash::Block(_), ..] => { + debug!("ignoring inv with multiple blocks"); + return; + } _ => { - self.fail_with(PeerError::WrongMessage("inv with mixed item types")); + debug!("ignoring inv with no transactions"); return; } }, Message::GetData(items) => match &items[..] { - [InventoryHash::Block(_), rest @ ..] - if rest + // Some peers advertise invs with mixed item types. + // So we suspect they might do the same with getdata. + // + // Since we can only handle one message at a time, + // we treat it as a block request if there are any blocks, + // or a transaction request if there are any transactions. + // + // TODO: split mixed getdata into multiple requests. + b_hashes + if b_hashes .iter() - .all(|item| matches!(item, InventoryHash::Block(_))) => + .any(|item| matches!(item, InventoryHash::Block(_))) => { Request::BlocksByHash(block_hashes(&items).collect()) } - tx_ids - if tx_ids.iter().all(|item| item.unmined_tx_id().is_some()) - && !tx_ids.is_empty() => - { + tx_ids if tx_ids.iter().any(|item| item.unmined_tx_id().is_some()) => { Request::TransactionsById(transaction_ids(&items).collect()) } + + // Log detailed messages for ignored getdata request messages. + [] => { + debug!("ignoring empty getdata"); + return; + } _ => { - self.fail_with(PeerError::WrongMessage("getdata with mixed item types")); + debug!("ignoring getdata with no blocks or transactions"); return; } }, diff --git a/zebra-network/src/peer/error.rs b/zebra-network/src/peer/error.rs index 8bdaad1a..24e638ff 100644 --- a/zebra-network/src/peer/error.rs +++ b/zebra-network/src/peer/error.rs @@ -28,37 +28,32 @@ pub enum PeerError { /// The remote peer closed the connection. #[error("Peer closed connection")] ConnectionClosed, + /// The remote peer did not respond to a [`peer::Client`] request in time. #[error("Client request timed out")] ClientRequestTimeout, + /// A serialization error occurred while reading or writing a message. #[error("Serialization error: {0}")] Serialization(#[from] SerializationError), + /// A badly-behaved remote peer sent a handshake message after the handshake was /// already complete. #[error("Remote peer sent handshake messages after handshake")] DuplicateHandshake, + /// This node's internal services were overloaded, so the connection was dropped /// to shed load. #[error("Internal services over capacity")] Overloaded, + + // TODO: stop closing connections on these errors (#2107) + // log info or debug logs instead + // /// A peer sent us a message we don't support. #[error("Remote peer sent an unsupported message type: {0}")] UnsupportedMessage(&'static str), - /// A peer sent us a message we couldn't interpret in context. - #[error("Remote peer sent an uninterpretable message: {0}")] - WrongMessage(&'static str), - /// We got a `Reject` message. This does not necessarily mean that - /// the peer connection is in a bad state, but for the time being - /// we are considering it a PeerError. - // TODO: Create a different error type (more at the application - // level than network/connection level) that will include the - // appropriate error when a `Reject` message is received. - #[error("Received a Reject message")] - Rejected, - /// The remote peer responded with a block we didn't ask for. - #[error("Remote peer responded with a block we didn't ask for.")] - WrongBlock, + /// We requested data that the peer couldn't find. #[error("Remote peer could not find items: {0:?}")] NotFound(Vec), diff --git a/zebrad/src/application.rs b/zebrad/src/application.rs index 9f5ebb37..250a4a7f 100644 --- a/zebrad/src/application.rs +++ b/zebrad/src/application.rs @@ -286,9 +286,14 @@ impl Application for ZebradApp { true } color_eyre::ErrorKind::Recoverable(error) => { - // type checks should be faster than string conversions + // Type checks should be faster than string conversions. + // + // Don't ask users to create bug reports for timeouts and peer errors. if error.is::() || error.is::() + || error.is::() + || error.is::() + || error.is::() { return false; }