Cleanup

Author: ImplOfAnImpl

CHANGELOG.md

@@ -68,6 +68,10 @@ The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/
   - Node bootstrapping:
     - The format of the bootstrap file was changed and the legacy format is no longer supported.
 
+  - P2p:
+    - The logic of initiating new outbound connections has been improved to prevent the node from
+      constantly attempting to re-establish a connection with a peer that has banned it.
+
 ### Fixed
   - P2p: when a peer sends a message that can't be decoded, it will now be discouraged (which is what
     is normally done for misbehaving peers) and the node won't try connecting to it again.\

p2p/src/net/default_backend/peer.rs

@@ -424,8 +424,6 @@ where
                     .await?
             }
             CategorizedMessage::BlockSyncMessage(msg) => {
-                block_sync_msg_sender.send(msg).await?;
-
                 if !self.sync_message_received {
                     self.peer_event_sender
                         .send(PeerEvent::MessageReceived(
@@ -434,10 +432,10 @@ where
                         .await?;
                     self.sync_message_received = true;
                 }
+
+                block_sync_msg_sender.send(msg).await?;
             }
             CategorizedMessage::TransactionSyncMessage(msg) => {
-                transaction_sync_msg_sender.send(msg).await?;
-
                 if !self.sync_message_received {
                     self.peer_event_sender
                         .send(PeerEvent::MessageReceived(
@@ -446,6 +444,8 @@ where
                         .await?;
                     self.sync_message_received = true;
                 }
+
+                transaction_sync_msg_sender.send(msg).await?;
             }
         }
 

p2p/src/net/types.rs

@@ -62,10 +62,10 @@ impl PeerRole {
     pub fn as_outbound(&self) -> Option<OutboundPeerRole> {
         match self {
             Self::Inbound => None,
-            Self::OutboundFullRelay => Some(OutboundPeerRole::OutboundFullRelay),
-            Self::OutboundBlockRelay => Some(OutboundPeerRole::OutboundBlockRelay),
-            Self::OutboundReserved => Some(OutboundPeerRole::OutboundReserved),
-            Self::OutboundManual => Some(OutboundPeerRole::OutboundManual),
+            Self::OutboundFullRelay => Some(OutboundPeerRole::FullRelay),
+            Self::OutboundBlockRelay => Some(OutboundPeerRole::BlockRelay),
+            Self::OutboundReserved => Some(OutboundPeerRole::Reserved),
+            Self::OutboundManual => Some(OutboundPeerRole::Manual),
             Self::Feeler => Some(OutboundPeerRole::Feeler),
         }
     }
@@ -88,13 +88,35 @@ impl PeerRole {
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, strum::EnumIter)]
 pub enum OutboundPeerRole {
-    OutboundFullRelay,
-    OutboundBlockRelay,
-    OutboundReserved,
-    OutboundManual,
+    FullRelay,
+    BlockRelay,
+    Reserved,
+    Manual,
     Feeler,
 }
 
+impl OutboundPeerRole {
+    /// Return true if for this connection type some message exchange is expected (besides
+    /// the handshake and WillDisconnect), i.e. the node is supposed to send at least one message
+    /// and get back a response.
+    ///
+    /// This is used by peerdb's AddressData to determine whether the "no activity" counter
+    /// should be increased after a connection with no peer activity.
+    pub fn is_message_exchange_expected(&self) -> bool {
+        match self {
+            Self::FullRelay | Self::BlockRelay | Self::Reserved | Self::Manual => true,
+            Self::Feeler => false,
+        }
+    }
+
+    pub fn is_manual(&self) -> bool {
+        match self {
+            Self::Manual => true,
+            | Self::FullRelay | Self::BlockRelay | Self::Reserved | Self::Feeler => false,
+        }
+    }
+}
+
 /// Peer information learned during handshaking
 ///
 /// When an inbound/outbound connection succeeds, the networking service handshakes with the remote

p2p/src/peer_manager/mod.rs

@@ -1552,6 +1552,7 @@ where
             );
             if let Some(address) = address_opt {
                 log::debug!("Need to establish a feeler connection to address {address}");
+
                 self.connect(address, OutboundConnectType::Feeler);
                 self.next_feeler_connection_time = Self::choose_next_feeler_connection_time(
                     &self.p2p_config,
@@ -1581,7 +1582,7 @@ where
         // just in case, so that if things go horribly wrong, only the debug build will panic.
         if let Some(peer) = self.peers.get(&peer_id) {
             if !is_disconnection_message && peer.peer_role.is_outbound() {
-                self.peerdb.outbound_peer_has_activity(
+                self.peerdb.outbound_peer_had_activity(
                     peer.peer_address,
                     Self::lock_rng(&self.rng).deref_mut(),
                 );

p2p/src/peer_manager/peerdb/address_data/mod.rs

@@ -81,7 +81,7 @@ pub enum AddressState {
 #[strum_discriminants(name(AddressStateTransitionToTag), derive(strum::EnumIter))]
 pub enum AddressStateTransitionTo {
     Connected { peer_role: OutboundPeerRole },
-    HasActivity,
+    HadActivity,
     Disconnected,
     ConnectionFailed,
     SetReserved,
@@ -239,7 +239,7 @@ impl AddressData {
                 }
             },
 
-            AddressStateTransitionTo::HasActivity => match self.state {
+            AddressStateTransitionTo::HadActivity => match self.state {
                 AddressState::Connected {
                     had_activity: _,
                     peer_role,
@@ -253,13 +253,13 @@ impl AddressData {
                     was_reachable: _,
                 } => {
                     debug_panic_or_log!(
-                        "Unexpected address state transition: Disconnected -> HasActivity"
+                        "Unexpected address state transition: Disconnected -> HadActivity"
                     );
                     self.state.clone()
                 }
                 AddressState::Unreachable { erase_after: _ } => {
                     debug_panic_or_log!(
-                        "Unexpected address state transition: Unreachable -> HasActivity"
+                        "Unexpected address state transition: Unreachable -> HadActivity"
                     );
                     self.state.clone()
                 }
@@ -273,19 +273,20 @@ impl AddressData {
                     if had_activity {
                         self.connections_without_activity_count = 0;
                     } else {
-                        // We don't increase the counter for:
-                        // 1) feeler connections (because it gets disconnected immediately, so the peer may
-                        // not have the chance to send us anything);
-                        // 2) manual connections;
-                        let is_regular_auto_connection = match peer_role {
-                            OutboundPeerRole::OutboundFullRelay
-                            | OutboundPeerRole::OutboundBlockRelay
-                            | OutboundPeerRole::OutboundReserved => true,
-
-                            OutboundPeerRole::Feeler | OutboundPeerRole::OutboundManual => false,
-                        };
-
-                        if is_regular_auto_connection {
+                        // Note:
+                        // 1) We don't increase the counter for manual connections.
+                        // 2) Since `is_message_exchange_expected` doesn't know the actual services
+                        //    that the peer provides, it's technically possible to punish an
+                        //    "innocent" peer here, e.g. when the connection type is supposed to involve
+                        //    block exchange, but the peer's actual services don't include Blocks.
+                        //    However:
+                        //    a) The worst punishment it can get is that the next connection will be
+                        //       postponed for (roughly) MAX_DELAY_REACHABLE, which is currently 1 hour.
+                        //    b) Such a peer will be rather useless anyway.
+                        //    c) The connection has to be short enough, so that even a single PingRequest message
+                        //       (which requires a response) could not be sent.
+                        //    So it's not a big deal.
+                        if peer_role.is_message_exchange_expected() && !peer_role.is_manual() {
                             self.connections_without_activity_count += 1;
                         }
                     }

p2p/src/peer_manager/peerdb/address_data/tests.rs

@@ -53,7 +53,7 @@ fn randomized(#[case] seed: Seed) {
                 AddressStateTransitionToTag::Connected => AddressStateTransitionTo::Connected {
                     peer_role: OutboundPeerRole::iter().choose(&mut rng).unwrap(),
                 },
-                AddressStateTransitionToTag::HasActivity => AddressStateTransitionTo::HasActivity,
+                AddressStateTransitionToTag::HadActivity => AddressStateTransitionTo::HadActivity,
                 AddressStateTransitionToTag::Disconnected => AddressStateTransitionTo::Disconnected,
                 AddressStateTransitionToTag::ConnectionFailed => {
                     AddressStateTransitionTo::ConnectionFailed
@@ -68,7 +68,7 @@ fn randomized(#[case] seed: Seed) {
                 AddressStateTransitionTo::Connected { peer_role: _ } => {
                     !address_data.is_connected()
                 }
-                AddressStateTransitionTo::HasActivity => address_data.is_connected(),
+                AddressStateTransitionTo::HadActivity => address_data.is_connected(),
                 AddressStateTransitionTo::Disconnected => address_data.is_connected(),
                 AddressStateTransitionTo::ConnectionFailed => !address_data.is_connected(),
                 AddressStateTransitionTo::SetReserved => true,

p2p/src/peer_manager/peerdb/mod.rs

@@ -425,8 +425,8 @@ impl<S: PeerDbStorage> PeerDb<S> {
     }
 
     /// Report that the peer has sent us a message other than the one indicating an intent to disconnect.
-    pub fn outbound_peer_has_activity(&mut self, address: SocketAddress, rng: &mut impl Rng) {
-        self.change_address_state(address, AddressStateTransitionTo::HasActivity, rng);
+    pub fn outbound_peer_had_activity(&mut self, address: SocketAddress, rng: &mut impl Rng) {
+        self.change_address_state(address, AddressStateTransitionTo::HadActivity, rng);
     }
 
     pub fn remove_address(&mut self, address: &SocketAddress) {

p2p/src/peer_manager/tests/unsuccessful_connection_counter_update.rs

@@ -286,7 +286,9 @@ async fn auto_connection_fails_peer_state_becomes_disconnected(
 //    Keep peer address's `was_reachable` field at false.
 // 3) Advance the time so that the corresponding connection would be attempted; make the
 //    connection fail; check that the peer address state is now `Unreachable`.
-// 4) Check that no further connection attempts are made.
+// 4) Optionally, accept an inbound connection from the peer address; expect that it doesn't
+//    affect the current address state.
+// 5) Check that no further connection attempts are made.
 #[tracing::instrument(skip(seed))]
 #[rstest]
 #[trace]
@@ -510,7 +512,6 @@ async fn manual_connection_fails(#[case] seed: Seed, #[values(false, true)] make
 
     if make_reachable {
         let peer_discovery_time = time_getter.get_time_getter().get_time();
-
         discover_peer(&peer_mgr_event_sender, peer_address, true).await;
 
         let peer_addr_state =
@@ -609,7 +610,7 @@ async fn manual_connection_fails(#[case] seed: Seed, #[values(false, true)] make
 //    Make the connection succeed and close it immediately; check that `connections_without_activity_count`
 //    has been incremented.
 // 3) Occasionally, make a successful incoming or manual outgoing connection without peer activity, or an unsuccessful
-//    manual outgoing one. Expect that this doesn't affect `connections_without_activity_count`.
+//    manual outgoing connection. Expect that this doesn't affect `connections_without_activity_count`.
 // 4) On the final iteration make the peer actually send a message. Check that `connections_without_activity_count` has
 //    been reset to zero.
 // Note that feeler connections are not checked in this test because once a feeler connection succeeds, it won't
@@ -765,7 +766,7 @@ async fn auto_connection_without_peer_activity(
 
         // An inbound or manual connection without peer activity shouldn't affect connections_without_activity_count.
         // Same for a failed outbound connection.
-        let extra_outboun_connection_failed = match rng.gen_range(0..4) {
+        let extra_outbound_connection_failed = match rng.gen_range(0..4) {
             0 => {
                 log::debug!("Accepting an extra inbound connection");
 
@@ -816,7 +817,7 @@ async fn auto_connection_without_peer_activity(
         };
 
         let expected_connections_without_activity_count = if is_last_iteration { 0 } else { i + 1 };
-        let expected_fail_count = if extra_outboun_connection_failed {
+        let expected_fail_count = if extra_outbound_connection_failed {
             1
         } else {
             0

p2p/src/peer_manager/tests/utils.rs

@@ -34,7 +34,6 @@ use crate::{
         types::{ConnectivityEvent, PeerInfo},
     },
     peer_manager::PeerManagerInterface,
-    peer_manager_event::PeerDisconnectionDbAction,
     test_helpers::TEST_PROTOCOL_VERSION,
     tests::helpers::PeerManagerNotification,
     utils::oneshot_nofail,
@@ -299,25 +298,6 @@ pub fn start_manually_connecting(
     result_receiver
 }
 
-pub fn disconnect_manually(
-    peer_mgr_event_sender: &mpsc::UnboundedSender<PeerManagerEvent>,
-    peer_id: PeerId,
-    peerdb_action: PeerDisconnectionDbAction,
-) -> oneshot_nofail::Receiver<crate::Result<()>> {
-    let (result_sender, result_receiver) = oneshot_nofail::channel();
-
-    peer_mgr_event_sender
-        .send(PeerManagerEvent::Disconnect(
-            peer_id,
-            peerdb_action,
-            None,
-            result_sender,
-        ))
-        .unwrap();
-
-    result_receiver
-}
-
 pub async fn adjust_peer_score(
     peer_mgr_event_sender: &mpsc::UnboundedSender<PeerManagerEvent>,
     peer_id: PeerId,

p2p/src/tests/peer_mgr_events.rs

@@ -65,7 +65,7 @@ enum TestParamForFirstSyncMessageReceivedMustBeSent {
 // is sent to the peer manager.
 // 1) Send a random message from the peer.
 // 2) If the sent message was a sync one, expect that `PeerManagerNotification::MessageReceived` is emitted with
-// message equal to `PeerManagerMessageExtTag::FirstSyncMessageReceived`. If the sent message was not a sync one,
+// the message equal to `PeerManagerMessageExtTag::FirstSyncMessageReceived`. If the sent message was not a sync one,
 // expect that no such notification is emitted.
 #[tracing::instrument(skip(seed))]
 #[rstest]
@@ -233,7 +233,7 @@ async fn first_sync_message_received_must_be_sent(
 
 // Check that `PeerManagerMessageExt::FirstSyncMessageReceived` will be sent only once even if
 // many sync messages are received from the peer.
-// 1) Send a few HeaderListRequest and/or NewTransaction (legit messages that a peer can send
+// 1) Send a few `HeaderListRequest` and/or `NewTransaction` messages (i.e. some legit messages that a peer can send
 // in any number).
 // 2) Expect that only one `PeerManagerNotification::MessageReceived` with `PeerManagerMessageExtTag::FirstSyncMessageReceived`
 // is produced.