Add DropReason and DropPeer structs to pass reason string to logs about Dropping a Neighbor

CHANGELOG.md

@@ -7,10 +7,14 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 ## [Unreleased]
 
-### Added
+## Added
 
 ### Changed
 
+- Logging improvements:
+  - P2P logs now includes a reason for dropping a peer or neighbor
+  - Imrpovements to how a PeerAddress is logged (human readable format vs hex)
+
 ### Fixed
 
 ## [3.1.0.0.5]

stacks-common/src/types/net.rs

@@ -118,15 +118,12 @@ impl PeerAddress {
         )
     }
 
-    /// Convert to SocketAddr
-    pub fn to_socketaddr(&self, port: u16) -> SocketAddr {
+    /// Convert to IpAddr
+    pub fn to_ipaddr(&self) -> IpAddr {
         if self.is_ipv4() {
-            SocketAddr::new(
-                IpAddr::V4(Ipv4Addr::new(
-                    self.0[12], self.0[13], self.0[14], self.0[15],
-                )),
-                port,
-            )
+            IpAddr::V4(Ipv4Addr::new(
+                self.0[12], self.0[13], self.0[14], self.0[15],
+            ))
         } else {
             let addr_words: [u16; 8] = [
                 ((self.0[0] as u16) << 8) | (self.0[1] as u16),
@@ -138,23 +135,25 @@ impl PeerAddress {
                 ((self.0[12] as u16) << 8) | (self.0[13] as u16),
                 ((self.0[14] as u16) << 8) | (self.0[15] as u16),
             ];
-
-            SocketAddr::new(
-                IpAddr::V6(Ipv6Addr::new(
-                    addr_words[0],
-                    addr_words[1],
-                    addr_words[2],
-                    addr_words[3],
-                    addr_words[4],
-                    addr_words[5],
-                    addr_words[6],
-                    addr_words[7],
-                )),
-                port,
-            )
+            IpAddr::V6(Ipv6Addr::new(
+                addr_words[0],
+                addr_words[1],
+                addr_words[2],
+                addr_words[3],
+                addr_words[4],
+                addr_words[5],
+                addr_words[6],
+                addr_words[7],
+            ))
         }
     }
 
+    /// Convert to SocketAddr
+    pub fn to_socketaddr(&self, port: u16) -> SocketAddr {
+        let ip_addr = self.to_ipaddr();
+        SocketAddr::new(ip_addr, port)
+    }
+
     /// Convert from socket address
     pub fn from_socketaddr(addr: &SocketAddr) -> PeerAddress {
         PeerAddress::from_ip(&addr.ip())
@@ -228,6 +227,10 @@ impl PeerAddress {
     pub fn to_bin(&self) -> String {
         to_bin(&self.0)
     }
+
+    pub fn pretty_print(&self) -> String {
+        self.to_ipaddr().to_string()
+    }
 }
 
 /// Peer address variants for the Host: header

stackslib/src/net/download/epoch2x.rs

@@ -22,6 +22,7 @@ use std::sync::mpsc::{
     sync_channel, Receiver, RecvError, RecvTimeoutError, SyncSender, TryRecvError, TrySendError,
 };
 
+use p2p::DropSource;
 use rand::seq::SliceRandom;
 use rand::{thread_rng, RngCore};
 use stacks_common::types::chainstate::{BlockHeaderHash, PoxId, SortitionId, StacksBlockId};
@@ -233,7 +234,7 @@ pub struct BlockDownloader {
     /// statistics on peers' data-plane endpoints
     pub(crate) dead_peers: Vec<usize>,
     pub(crate) broken_peers: Vec<usize>,
-    broken_neighbors: Vec<NeighborKey>, // disconnect peers who report invalid block inventories too
+    broken_neighbors: Vec<DropNeighbor>, // disconnect peers who report invalid block inventories too
 
     pub(crate) blocked_urls: HashMap<UrlString, u64>, // URLs that chronically don't work, and when we can try them again
 
@@ -500,7 +501,13 @@ impl BlockDownloader {
                                         {
                                             info!("Invalid block from {:?} ({:?}): did not ask for block {}/{}", &block_key.neighbor, &block_key.data_url, block_key.consensus_hash, block.block_hash());
                                             self.broken_peers.push(event_id);
-                                            self.broken_neighbors.push(block_key.neighbor.clone());
+                                            self.broken_neighbors.push(DropNeighbor {
+                                                key: block_key.neighbor.clone(),
+                                                reason: DropReason::BrokenConnection(
+                                                    "Remote neighbor sent an invalid block".into(),
+                                                ),
+                                                source: DropSource::BlockDownloaderGetBlocks,
+                                            });
                                         } else {
                                             // got the block
                                             debug!(
@@ -519,14 +526,25 @@ impl BlockDownloader {
                                         // the fact that we asked this peer means that it's block inv indicated
                                         // it was present, so the absence is the mark of a broken peer
                                         self.broken_peers.push(event_id);
-                                        self.broken_neighbors.push(block_key.neighbor.clone());
+                                        self.broken_neighbors.push(DropNeighbor {
+                                            key: block_key.neighbor.clone(),
+                                            reason: DropReason::BrokenConnection(
+                                                "Remote neighbor was missing an expected block"
+                                                    .into(),
+                                            ),
+                                            source: DropSource::BlockDownloaderGetBlocks,
+                                        });
                                     }
                                     Err(e) => {
                                         info!("Error decoding response from remote neighbor {:?} (at {}): {:?}", &block_key.neighbor, &block_key.data_url, &e;
                                             "consensus_hash" => %block_key.consensus_hash
                                         );
                                         self.broken_peers.push(event_id);
-                                        self.broken_neighbors.push(block_key.neighbor.clone());
+                                        self.broken_neighbors.push(DropNeighbor {
+                                            key: block_key.neighbor.clone(),
+                                            reason: DropReason::BrokenConnection(format!("Error occurred decoding block response from neighbor: {e}")),
+                                            source: DropSource::BlockDownloaderGetBlocks
+                                        });
                                     }
                                 }
                             }
@@ -632,7 +650,11 @@ impl BlockDownloader {
                                                 "consensus_hash" => %block_key.consensus_hash
                                             );
                                             self.broken_peers.push(event_id);
-                                            self.broken_neighbors.push(block_key.neighbor.clone());
+                                            self.broken_neighbors.push(DropNeighbor {
+                                                key: block_key.neighbor.clone(),
+                                                reason: DropReason::BrokenConnection("Remote neighbor sent an unexpected zero-length microblock stream".into()),
+                                                source: DropSource::BlockDownloaderGetMicroblocks
+                                            });
                                         } else {
                                             // have microblocks (but we don't know yet if they're well-formed)
                                             debug!(
@@ -664,7 +686,11 @@ impl BlockDownloader {
                                             "consensus_hash" => %block_key.consensus_hash
                                         );
                                         self.broken_peers.push(event_id);
-                                        self.broken_neighbors.push(block_key.neighbor.clone());
+                                        self.broken_neighbors.push(DropNeighbor {
+                                            key: block_key.neighbor.clone(),
+                                            reason: DropReason::BrokenConnection(format!("Error occurred decoding microblock response from neighbor: {e}")),
+                                            source: DropSource::BlockDownloaderGetMicroblocks
+                                        });
                                     }
                                 }
                             }
@@ -890,7 +916,7 @@ impl BlockDownloader {
     }
 
     /// Clear out broken peers that told us they had blocks, but didn't serve them.
-    fn clear_broken_peers(&mut self) -> (Vec<usize>, Vec<NeighborKey>) {
+    fn clear_broken_peers(&mut self) -> (Vec<usize>, Vec<DropNeighbor>) {
         // remove dead/broken peers
         let mut disconnect = vec![];
         let mut disconnect_neighbors = vec![];
@@ -2341,7 +2367,7 @@ impl PeerNetwork {
             Vec<(ConsensusHash, StacksBlock, u64)>,
             Vec<(ConsensusHash, Vec<StacksMicroblock>, u64)>,
             Vec<usize>,
-            Vec<NeighborKey>,
+            Vec<DropNeighbor>,
         ),
         net_error,
     > {

stackslib/src/net/download/nakamoto/download_state_machine.rs

@@ -63,7 +63,7 @@ use crate::net::inv::epoch2x::InvState;
 use crate::net::inv::nakamoto::{NakamotoInvStateMachine, NakamotoTenureInv};
 use crate::net::neighbors::rpc::NeighborRPC;
 use crate::net::neighbors::NeighborComms;
-use crate::net::p2p::{CurrentRewardSet, PeerNetwork};
+use crate::net::p2p::{CurrentRewardSet, DropReason, DropSource, PeerNetwork};
 use crate::net::server::HttpPeer;
 use crate::net::{Error as NetError, Neighbor, NeighborAddress, NeighborKey};
 use crate::util_lib::db::{DBConn, Error as DBError};
@@ -1204,7 +1204,12 @@ impl NakamotoDownloadStateMachine {
                     "Downloader for {} failed; this peer is dead: {:?}",
                     &naddr, &e
                 );
-                neighbor_rpc.add_dead(network, naddr);
+                neighbor_rpc.add_dead(
+                    network,
+                    naddr,
+                    DropReason::DeadConnection(format!("Failed to send download request: {e}")),
+                    DropSource::NakamotoDownloadStateMachine,
+                );
                 continue;
             };
         }
@@ -1236,12 +1241,24 @@ impl NakamotoDownloadStateMachine {
             ) {
                 Ok(blocks_opt) => blocks_opt,
                 Err(NetError::StaleView) => {
-                    neighbor_rpc.add_dead(network, &naddr);
+                    neighbor_rpc.add_dead(
+                        network,
+                        &naddr,
+                        DropReason::DeadConnection("Stale view".into()),
+                        DropSource::NakamotoDownloadStateMachine,
+                    );
                     continue;
                 }
                 Err(e) => {
                     debug!("Failed to handle next download response from unconfirmed downloader for {:?} in state {:?}: {:?}", &naddr, &downloader.state, &e);
-                    neighbor_rpc.add_dead(network, &naddr);
+                    neighbor_rpc.add_dead(
+                        network,
+                        &naddr,
+                        DropReason::DeadConnection(format!(
+                            "Failed to handle next download response: {e}"
+                        )),
+                        DropSource::NakamotoDownloadStateMachine,
+                    );
                     continue;
                 }
             };

stackslib/src/net/download/nakamoto/mod.rs

@@ -152,7 +152,7 @@ use crate::net::inv::epoch2x::InvState;
 use crate::net::inv::nakamoto::{NakamotoInvStateMachine, NakamotoTenureInv};
 use crate::net::neighbors::rpc::NeighborRPC;
 use crate::net::neighbors::NeighborComms;
-use crate::net::p2p::PeerNetwork;
+use crate::net::p2p::{DropReason, PeerNetwork};
 use crate::net::server::HttpPeer;
 use crate::net::{Error as NetError, Neighbor, NeighborAddress, NeighborKey};
 use crate::util_lib::db::{DBConn, Error as DBError};
@@ -236,11 +236,11 @@ impl PeerNetwork {
         };
 
         for broken in block_downloader.neighbor_rpc.take_broken() {
-            self.deregister_and_ban_neighbor(&broken);
+            self.deregister_and_ban_neighbor(&broken.key, broken.reason, broken.source);
         }
 
         for dead in block_downloader.neighbor_rpc.take_dead() {
-            self.deregister_neighbor(&dead);
+            self.deregister_neighbor(&dead.key, dead.reason, dead.source);
         }
 
         self.block_downloader_nakamoto = Some(block_downloader);

stackslib/src/net/download/nakamoto/tenure_downloader.rs

@@ -57,7 +57,7 @@ use crate::net::inv::epoch2x::InvState;
 use crate::net::inv::nakamoto::{NakamotoInvStateMachine, NakamotoTenureInv};
 use crate::net::neighbors::rpc::NeighborRPC;
 use crate::net::neighbors::NeighborComms;
-use crate::net::p2p::{CurrentRewardSet, PeerNetwork};
+use crate::net::p2p::{CurrentRewardSet, DropReason, DropSource, PeerNetwork};
 use crate::net::server::HttpPeer;
 use crate::net::{Error as NetError, Neighbor, NeighborAddress, NeighborKey};
 use crate::util_lib::db::{DBConn, Error as DBError};
@@ -745,7 +745,12 @@ impl NakamotoTenureDownloader {
 
         let Some(peerhost) = NeighborRPC::get_peer_host(network, &self.naddr) else {
             // no conversation open to this neighbor
-            neighbor_rpc.add_dead(network, &self.naddr);
+            neighbor_rpc.add_dead(
+                network,
+                &self.naddr,
+                DropReason::DeadConnection("No authenticated connection open".into()),
+                DropSource::NakamotoTenureDownloader,
+            );
             return Err(NetError::PeerNotConnected);
         };