fix(connection-limits): correct connection tracking

The connection limit behaviour was not taking into account connection errors. Also, it was using the behaviour events to indicate established connections which is not always going to be the case because other behaviours can deny the connections (thanks @divagant-martian).

Closes #4249

Pull-Request: #4250.

Author: AgeManning

Cargo.lock

@@ -65,7 +65,7 @@ rust-version = "1.65.0"
 [workspace.dependencies]
 libp2p-allow-block-list = { version = "0.2.0", path = "misc/allow-block-list" }
 libp2p-autonat = { version = "0.11.0", path = "protocols/autonat" }
-libp2p-connection-limits = { version = "0.2.0", path = "misc/connection-limits" }
+libp2p-connection-limits = { version = "0.2.1", path = "misc/connection-limits" }
 libp2p-core = { version = "0.40.0", path = "core" }
 libp2p-dcutr = { version = "0.10.0", path = "protocols/dcutr" }
 libp2p-deflate = { version = "0.40.0", path = "transports/deflate" }

Cargo.toml

@@ -1,4 +1,18 @@
-## 0.2.0 
+## 0.2.1
+
+- Do not count a connection as established when it is denied by another sibling `NetworkBehaviour`.
+  In other words, do not increase established connection counter in `handle_established_outbound_connection` or `handle_established_inbound_connection`, but in `FromSwarm::ConnectionEstablished` instead.
+
+  See [PR 4250].
+
+- Decrease `pending_inbound_connections` on `FromSwarm::ListenFailure` and `pending_outbound_connections` on `FromSwarm::DialFailure`.
+
+  See [PR 4250].
+
+[PR 4250]: https://github.com/libp2p/rust-libp2p/pull/4250
+
+## 0.2.0
+
 
 - Raise MSRV to 1.65.
   See [PR 3715].

misc/connection-limits/CHANGELOG.md

@@ -3,7 +3,7 @@ name = "libp2p-connection-limits"
 edition = "2021"
 rust-version = { workspace = true }
 description = "Connection limits for libp2p."
-version = "0.2.0"
+version = "0.2.1"
 license = "MIT"
 repository = "https://github.com/libp2p/rust-libp2p"
 keywords = ["peer-to-peer", "libp2p", "networking"]

misc/connection-limits/Cargo.toml

@@ -18,9 +18,10 @@
 // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 // DEALINGS IN THE SOFTWARE.
 
-use libp2p_core::{Endpoint, Multiaddr};
+use libp2p_core::{ConnectedPoint, Endpoint, Multiaddr};
 use libp2p_identity::PeerId;
 use libp2p_swarm::{
+    behaviour::{ConnectionEstablished, DialFailure, ListenFailure},
     dummy, ConnectionClosed, ConnectionDenied, ConnectionId, FromSwarm, NetworkBehaviour,
     PollParameters, THandler, THandlerInEvent, THandlerOutEvent, ToSwarm,
 };
@@ -249,12 +250,6 @@ impl NetworkBehaviour for Behaviour {
             Kind::EstablishedTotal,
         )?;
 
-        self.established_inbound_connections.insert(connection_id);
-        self.established_per_peer
-            .entry(peer)
-            .or_default()
-            .insert(connection_id);
-
         Ok(dummy::ConnectionHandler)
     }
 
@@ -305,12 +300,6 @@ impl NetworkBehaviour for Behaviour {
             Kind::EstablishedTotal,
         )?;
 
-        self.established_outbound_connections.insert(connection_id);
-        self.established_per_peer
-            .entry(peer)
-            .or_default()
-            .insert(connection_id);
-
         Ok(dummy::ConnectionHandler)
     }
 
@@ -328,10 +317,33 @@ impl NetworkBehaviour for Behaviour {
                     .or_default()
                     .remove(&connection_id);
             }
-            FromSwarm::ConnectionEstablished(_) => {}
+            FromSwarm::ConnectionEstablished(ConnectionEstablished {
+                peer_id,
+                endpoint,
+                connection_id,
+                ..
+            }) => {
+                match endpoint {
+                    ConnectedPoint::Listener { .. } => {
+                        self.established_inbound_connections.insert(connection_id);
+                    }
+                    ConnectedPoint::Dialer { .. } => {
+                        self.established_outbound_connections.insert(connection_id);
+                    }
+                }
+
+                self.established_per_peer
+                    .entry(peer_id)
+                    .or_default()
+                    .insert(connection_id);
+            }
+            FromSwarm::DialFailure(DialFailure { connection_id, .. }) => {
+                self.pending_outbound_connections.remove(&connection_id);
+            }
             FromSwarm::AddressChange(_) => {}
-            FromSwarm::DialFailure(_) => {}
-            FromSwarm::ListenFailure(_) => {}
+            FromSwarm::ListenFailure(ListenFailure { connection_id, .. }) => {
+                self.pending_inbound_connections.remove(&connection_id);
+            }
             FromSwarm::NewListener(_) => {}
             FromSwarm::NewListenAddr(_) => {}
             FromSwarm::ExpiredListenAddr(_) => {}
@@ -364,7 +376,9 @@ impl NetworkBehaviour for Behaviour {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use libp2p_swarm::{dial_opts::DialOpts, DialError, ListenError, Swarm, SwarmEvent};
+    use libp2p_swarm::{
+        behaviour::toggle::Toggle, dial_opts::DialOpts, DialError, ListenError, Swarm, SwarmEvent,
+    };
     use libp2p_swarm_test::SwarmExt;
     use quickcheck::*;
 
@@ -466,19 +480,125 @@ mod tests {
         quickcheck(prop as fn(_));
     }
 
+    /// Another sibling [`NetworkBehaviour`] implementation might deny established connections in
+    /// [`handle_established_outbound_connection`] or [`handle_established_inbound_connection`].
+    /// [`Behaviour`] must not increase the established counters in
+    /// [`handle_established_outbound_connection`] or [`handle_established_inbound_connection`], but
+    /// in [`SwarmEvent::ConnectionEstablished`] as the connection might still be denied by a
+    /// sibling [`NetworkBehaviour`] in the former case. Only in the latter case
+    /// ([`SwarmEvent::ConnectionEstablished`]) can the connection be seen as established.
+    #[test]
+    fn support_other_behaviour_denying_connection() {
+        let mut swarm1 = Swarm::new_ephemeral(|_| {
+            Behaviour::new_with_connection_denier(ConnectionLimits::default())
+        });
+        let mut swarm2 = Swarm::new_ephemeral(|_| Behaviour::new(ConnectionLimits::default()));
+
+        async_std::task::block_on(async {
+            // Have swarm2 dial swarm1.
+            let (listen_addr, _) = swarm1.listen().await;
+            swarm2.dial(listen_addr).unwrap();
+            async_std::task::spawn(swarm2.loop_on_next());
+
+            // Wait for the ConnectionDenier of swarm1 to deny the established connection.
+            let cause = swarm1
+                .wait(|event| match event {
+                    SwarmEvent::IncomingConnectionError {
+                        error: ListenError::Denied { cause },
+                        ..
+                    } => Some(cause),
+                    _ => None,
+                })
+                .await;
+
+            cause.downcast::<std::io::Error>().unwrap();
+
+            assert_eq!(
+                0,
+                swarm1
+                    .behaviour_mut()
+                    .limits
+                    .established_inbound_connections
+                    .len(),
+                "swarm1 connection limit behaviour to not count denied established connection as established connection"
+            )
+        });
+    }
+
     #[derive(libp2p_swarm_derive::NetworkBehaviour)]
     #[behaviour(prelude = "libp2p_swarm::derive_prelude")]
     struct Behaviour {
         limits: super::Behaviour,
         keep_alive: libp2p_swarm::keep_alive::Behaviour,
+        connection_denier: Toggle<ConnectionDenier>,
     }
 
     impl Behaviour {
         fn new(limits: ConnectionLimits) -> Self {
             Self {
                 limits: super::Behaviour::new(limits),
                 keep_alive: libp2p_swarm::keep_alive::Behaviour,
+                connection_denier: None.into(),
             }
         }
+        fn new_with_connection_denier(limits: ConnectionLimits) -> Self {
+            Self {
+                limits: super::Behaviour::new(limits),
+                keep_alive: libp2p_swarm::keep_alive::Behaviour,
+                connection_denier: Some(ConnectionDenier {}).into(),
+            }
+        }
+    }
+
+    struct ConnectionDenier {}
+
+    impl NetworkBehaviour for ConnectionDenier {
+        type ConnectionHandler = dummy::ConnectionHandler;
+        type ToSwarm = Void;
+
+        fn handle_established_inbound_connection(
+            &mut self,
+            _connection_id: ConnectionId,
+            _peer: PeerId,
+            _local_addr: &Multiaddr,
+            _remote_addr: &Multiaddr,
+        ) -> Result<THandler<Self>, ConnectionDenied> {
+            Err(ConnectionDenied::new(std::io::Error::new(
+                std::io::ErrorKind::Other,
+                "ConnectionDenier",
+            )))
+        }
+
+        fn handle_established_outbound_connection(
+            &mut self,
+            _connection_id: ConnectionId,
+            _peer: PeerId,
+            _addr: &Multiaddr,
+            _role_override: Endpoint,
+        ) -> Result<THandler<Self>, ConnectionDenied> {
+            Err(ConnectionDenied::new(std::io::Error::new(
+                std::io::ErrorKind::Other,
+                "ConnectionDenier",
+            )))
+        }
+
+        fn on_swarm_event(&mut self, _event: FromSwarm<Self::ConnectionHandler>) {}
+
+        fn on_connection_handler_event(
+            &mut self,
+            _peer_id: PeerId,
+            _connection_id: ConnectionId,
+            event: THandlerOutEvent<Self>,
+        ) {
+            void::unreachable(event)
+        }
+
+        fn poll(
+            &mut self,
+            _cx: &mut Context<'_>,
+            _params: &mut impl PollParameters,
+        ) -> Poll<ToSwarm<Self::ToSwarm, THandlerInEvent<Self>>> {
+            Poll::Pending
+        }
     }
 }