Remove duplicated connection limits checks (#6156)

* move main Behaviour to mod.rs for better readibility

and remove connection limits checks after connection has been established,

as those checks have already been done by connection limits Behaviour.

* improve logging wording wrt dial logic

when we call dial_peer we are not yet dialing but just adding the peer to the dial queue

* do not use a constant for MAX_CONNECTIONS_PER_PEER

we only use it at one place, and the function call is explicit.

* address review and re-instate connection limits checks,

but do it before the connection has been established.

* Merge branch 'unstable' of github.com:sigp/lighthouse into remove-dial-error-denied

* Merge branch 'unstable' of github.com:sigp/lighthouse into remove-dial-error-denied

beacon_node/lighthouse_network/src/service/behaviour.rs

@@ -1,39 +0,0 @@
-use crate::discovery::Discovery;
-use crate::peer_manager::PeerManager;
-use crate::rpc::RPC;
-use crate::types::SnappyTransform;
-
-use libp2p::identify;
-use libp2p::swarm::behaviour::toggle::Toggle;
-use libp2p::swarm::NetworkBehaviour;
-use libp2p::upnp::tokio::Behaviour as Upnp;
-use types::EthSpec;
-
-use super::api_types::RequestId;
-
-pub type SubscriptionFilter =
-    gossipsub::MaxCountSubscriptionFilter<gossipsub::WhitelistSubscriptionFilter>;
-pub type Gossipsub = gossipsub::Behaviour<SnappyTransform, SubscriptionFilter>;
-
-#[derive(NetworkBehaviour)]
-pub(crate) struct Behaviour<E>
-where
-    E: EthSpec,
-{
-    /// Keep track of active and pending connections to enforce hard limits.
-    pub connection_limits: libp2p::connection_limits::Behaviour,
-    /// The peer manager that keeps track of peer's reputation and status.
-    pub peer_manager: PeerManager<E>,
-    /// The Eth2 RPC specified in the wire-0 protocol.
-    pub eth2_rpc: RPC<RequestId, E>,
-    /// Discv5 Discovery protocol.
-    pub discovery: Discovery<E>,
-    /// Keep regular connection to peers and disconnect if absent.
-    // NOTE: The id protocol is used for initial interop. This will be removed by mainnet.
-    /// Provides IP addresses and peer information.
-    pub identify: identify::Behaviour,
-    /// Libp2p UPnP port mapping.
-    pub upnp: Toggle<Upnp>,
-    /// The routing pub-sub mechanism for eth2.
-    pub gossipsub: Gossipsub,
-}

beacon_node/lighthouse_network/src/service/mod.rs

@@ -1,4 +1,3 @@
-use self::behaviour::Behaviour;
 use self::gossip_cache::GossipCache;
 use crate::config::{gossipsub_config, GossipsubConfigParams, NetworkLoad};
 use crate::discovery::{
@@ -14,8 +13,6 @@ use crate::rpc::{
     self, GoodbyeReason, HandlerErr, NetworkParams, Protocol, RPCError, RPCMessage, RPCReceived,
     RequestType, ResponseTermination, RpcErrorResponse, RpcResponse, RpcSuccessResponse, RPC,
 };
-use crate::service::behaviour::BehaviourEvent;
-pub use crate::service::behaviour::Gossipsub;
 use crate::types::{
     attestation_sync_committee_topics, fork_core_topics, subnet_from_topic_hash, GossipEncoding,
     GossipKind, GossipTopic, SnappyTransform, Subnet, SubnetDiscovery, ALTAIR_CORE_TOPICS,
@@ -33,7 +30,8 @@ use gossipsub::{
 use gossipsub_scoring_parameters::{lighthouse_gossip_thresholds, PeerScoreSettings};
 use libp2p::multiaddr::{self, Multiaddr, Protocol as MProtocol};
 use libp2p::swarm::behaviour::toggle::Toggle;
-use libp2p::swarm::{Swarm, SwarmEvent};
+use libp2p::swarm::{NetworkBehaviour, Swarm, SwarmEvent};
+use libp2p::upnp::tokio::Behaviour as Upnp;
 use libp2p::{identify, PeerId, SwarmBuilder};
 use slog::{crit, debug, info, o, trace, warn};
 use std::num::{NonZeroU8, NonZeroUsize};
@@ -47,10 +45,9 @@ use types::{
     consts::altair::SYNC_COMMITTEE_SUBNET_COUNT, EnrForkId, EthSpec, ForkContext, Slot, SubnetId,
 };
 use types::{ChainSpec, ForkName};
-use utils::{build_transport, strip_peer_id, Context as ServiceContext, MAX_CONNECTIONS_PER_PEER};
+use utils::{build_transport, strip_peer_id, Context as ServiceContext};
 
 pub mod api_types;
-mod behaviour;
 mod gossip_cache;
 pub mod gossipsub_scoring_parameters;
 pub mod utils;
@@ -109,6 +106,41 @@ pub enum NetworkEvent<E: EthSpec> {
     ZeroListeners,
 }
 
+pub type Gossipsub = gossipsub::Behaviour<SnappyTransform, SubscriptionFilter>;
+pub type SubscriptionFilter =
+    gossipsub::MaxCountSubscriptionFilter<gossipsub::WhitelistSubscriptionFilter>;
+
+#[derive(NetworkBehaviour)]
+pub(crate) struct Behaviour<E>
+where
+    E: EthSpec,
+{
+    // NOTE: The order of the following list of behaviours has meaning,
+    // `NetworkBehaviour::handle_{pending, established}_{inbound, outbound}` methods
+    // are called sequentially for each behaviour and they are fallible,
+    // therefore we want `connection_limits` and `peer_manager` running first,
+    // which are the behaviours that may reject a connection, so that
+    // when the subsequent behaviours are called they are certain the connection won't be rejected.
+
+    //
+    /// Keep track of active and pending connections to enforce hard limits.
+    pub connection_limits: libp2p::connection_limits::Behaviour,
+    /// The peer manager that keeps track of peer's reputation and status.
+    pub peer_manager: PeerManager<E>,
+    /// The Eth2 RPC specified in the wire-0 protocol.
+    pub eth2_rpc: RPC<RequestId, E>,
+    /// Discv5 Discovery protocol.
+    pub discovery: Discovery<E>,
+    /// Keep regular connection to peers and disconnect if absent.
+    // NOTE: The id protocol is used for initial interop. This will be removed by mainnet.
+    /// Provides IP addresses and peer information.
+    pub identify: identify::Behaviour,
+    /// Libp2p UPnP port mapping.
+    pub upnp: Toggle<Upnp>,
+    /// The routing pub-sub mechanism for eth2.
+    pub gossipsub: Gossipsub,
+}
+
 /// Builds the network behaviour that manages the core protocols of eth2.
 /// This core behaviour is managed by `Behaviour` which adds peer management to all core
 /// behaviours.
@@ -396,7 +428,7 @@ impl<E: EthSpec> Network<E> {
                     (config.target_peers as f32 * (1.0 + PEER_EXCESS_FACTOR + PRIORITY_PEER_EXCESS))
                         .ceil() as u32,
                 ))
-                .with_max_established_per_peer(Some(MAX_CONNECTIONS_PER_PEER));
+                .with_max_established_per_peer(Some(1));
 
             libp2p::connection_limits::Behaviour::new(limits)
         };
@@ -1198,7 +1230,7 @@ impl<E: EthSpec> Network<E> {
             self.discovery_mut().remove_cached_enr(&enr.peer_id());
             let peer_id = enr.peer_id();
             if self.peer_manager_mut().dial_peer(enr) {
-                debug!(self.log, "Dialing cached ENR peer"; "peer_id" => %peer_id);
+                debug!(self.log, "Added cached ENR peer to dial queue"; "peer_id" => %peer_id);
             }
         }
     }

beacon_node/lighthouse_network/src/service/utils.rs

@@ -24,8 +24,6 @@ use types::{
 };
 
 pub const NETWORK_KEY_FILENAME: &str = "key";
-/// The maximum simultaneous libp2p connections per peer.
-pub const MAX_CONNECTIONS_PER_PEER: u32 = 1;
 /// The filename to store our local metadata.
 pub const METADATA_FILENAME: &str = "metadata";