From 53df98298ab94f831e508170dbf952209beb2bd8 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Wed, 26 Jul 2023 23:36:14 +1000 Subject: [PATCH] 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. --- Cargo.lock | 2 +- Cargo.toml | 2 +- misc/connection-limits/CHANGELOG.md | 16 ++- misc/connection-limits/Cargo.toml | 2 +- misc/connection-limits/src/lib.rs | 154 +++++++++++++++++++++++++--- 5 files changed, 155 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e89447b4edb..b2fb6e87074 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2594,7 +2594,7 @@ dependencies = [ [[package]] name = "libp2p-connection-limits" -version = "0.2.0" +version = "0.2.1" dependencies = [ "async-std", "libp2p-core", diff --git a/Cargo.toml b/Cargo.toml index 07ac8ab9e85..66b2cfc58ee 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -64,7 +64,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" } diff --git a/misc/connection-limits/CHANGELOG.md b/misc/connection-limits/CHANGELOG.md index e46a94e981a..a8bd071e6fe 100644 --- a/misc/connection-limits/CHANGELOG.md +++ b/misc/connection-limits/CHANGELOG.md @@ -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]. diff --git a/misc/connection-limits/Cargo.toml b/misc/connection-limits/Cargo.toml index db662f518aa..25347e29b16 100644 --- a/misc/connection-limits/Cargo.toml +++ b/misc/connection-limits/Cargo.toml @@ -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"] diff --git a/misc/connection-limits/src/lib.rs b/misc/connection-limits/src/lib.rs index 52d0aa62c39..e4723dd95c6 100644 --- a/misc/connection-limits/src/lib.rs +++ b/misc/connection-limits/src/lib.rs @@ -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,11 +480,57 @@ 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::().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, } impl Behaviour { @@ -478,7 +538,67 @@ mod tests { 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, 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, ConnectionDenied> { + Err(ConnectionDenied::new(std::io::Error::new( + std::io::ErrorKind::Other, + "ConnectionDenier", + ))) + } + + fn on_swarm_event(&mut self, _event: FromSwarm) {} + + fn on_connection_handler_event( + &mut self, + _peer_id: PeerId, + _connection_id: ConnectionId, + event: THandlerOutEvent, + ) { + void::unreachable(event) + } + + fn poll( + &mut self, + _cx: &mut Context<'_>, + _params: &mut impl PollParameters, + ) -> Poll>> { + Poll::Pending + } } }