Stop asking users to report peer errors, fix a common peer error (#3054)

* Stop treating inv with mixed item types as a connection error

* Remove unused connection errors

* Stop asking users to create bug reports for peer errors

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},

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())
+                // Some peers advertise invs with mixed item types.
-                        && !tx_ids.is_empty() =>
+                // 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 @ ..]
+                // Some peers advertise invs with mixed item types.
-                    if rest
+                // 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
+                tx_ids if tx_ids.iter().any(|item| item.unmined_tx_id().is_some()) => {
-                    if tx_ids.iter().all(|item| item.unmined_tx_id().is_some())
-                        && !tx_ids.is_empty() =>
-                {
                     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;
                 }
             },

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<InventoryHash>),

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::<tower::timeout::error::Elapsed>()
                         || error.is::<tokio::time::error::Elapsed>()
+                        || error.is::<zebra_network::PeerError>()
+                        || error.is::<zebra_network::SharedPeerError>()
+                        || error.is::<zebra_network::HandshakeError>()
                     {
                         return false;
                     }