Ban peer after disconnect

smol-dot · Nov 18, 2023 · fc9b986 · fc9b986
1 parent 6e53dcf
commit fc9b986
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 5 deletions.
diff --git a/lib/src/network/service.rs b/lib/src/network/service.rs
@@ -901,6 +901,21 @@ where
         &self.inner[id].address
     }
 
+    /// Returns the number of connections with the given peer.
+    ///
+    /// Both connections that have and have not finished their handshaking phase are considered.
+    pub fn num_potential_and_established_connections(&self, peer_id: &PeerId) -> usize {
+        let Some(&peer_index) = self.peers_by_peer_id.get(peer_id) else {
+            return 0;
+        };
+
+        self.connections_by_peer_id
+            .range(
+                (peer_index, ConnectionId::min_value())..=(peer_index, ConnectionId::max_value()),
+            )
+            .count()
+    }
+
     /// Pulls a message that must be sent to a connection.
     ///
     /// The message must be passed to [`SingleStreamConnectionTask::inject_coordinator_message`]

diff --git a/light-base/src/network_service.rs b/light-base/src/network_service.rs
@@ -1526,6 +1526,7 @@ async fn background_task<TPlat: PlatformRef>(mut task: BackgroundTask<TPlat>) {
 
                     task.peering_strategy
                         .remove_address(expected_peer_id, remote_addr.as_ref());
+                    // TODO: if Bob says that its address is the same as Alice's, and we try to connect to both Alice and Bob, then the Bob connection will reach this path and set Alice's address as connected even though it's already connected; this will later cause a state mismatch when disconnecting
                     let _ = task.peering_strategy.insert_or_set_connected_address(
                         &peer_id,
                         remote_addr.clone().into_vec(),
@@ -1540,12 +1541,31 @@ async fn background_task<TPlat: PlatformRef>(mut task: BackgroundTask<TPlat>) {
                 expected_peer_id,
                 ..
             }) => {
-                if let Some(expected_peer_id) = expected_peer_id {
+                let Some(expected_peer_id) = expected_peer_id else {
+                    debug_assert!(false);
+                    continue;
+                };
+
+                task.peering_strategy
+                    .disconnect_addr(&expected_peer_id, &address)
+                    .unwrap();
+                let address = Multiaddr::try_from(address).unwrap();
+                log::debug!(target: "network", "Connections({}, {}) => Shutdown(handshake_finished=false)", expected_peer_id, address);
+
+                // Due to race conditions and peerid mismatches, it is possible that there is
+                // already a connection or a connection attempt with the same peer.
+                // If this isn't the case and that this was the last connection attempt, then
+                // we ban the peer.
+                // Banning the peer is necessary in order to avoid trying over and over again the
+                // same address(es).
+                if task
+                    .network
+                    .num_potential_and_established_connections(&expected_peer_id)
+                    == 0
+                {
+                    // TODO: log the slots unassigned
                     task.peering_strategy
-                        .disconnect_addr(&expected_peer_id, &address)
-                        .unwrap();
-                    let address = Multiaddr::try_from(address).unwrap();
-                    log::debug!(target: "network", "Connections({}, {}) => Shutdown(handshake_finished=false)", expected_peer_id, address);
+                        .unassign_slots_and_ban(&expected_peer_id, when_unban);
                 }
             }
             WakeUpReason::NetworkEvent(service::Event::Disconnected {