Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Pause Kademlia if too many connections #4828

Merged
merged 4 commits into from
Feb 11, 2020
Merged

Pause Kademlia if too many connections #4828

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b32e29
Pause Kademlia if too many connections
tomaka Feb 4, 2020
7785f4f
Fix test
tomaka Feb 4, 2020
a303a5f
Update client/network/src/discovery.rs
tomaka Feb 5, 2020
9cd7a15
Change the limit
tomaka Feb 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion client/network/src/behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> Behaviour<B, S, H> {
known_addresses: Vec<(PeerId, Multiaddr)>,
enable_mdns: bool,
allow_private_ipv4: bool,
discovery_only_if_under_num: u64,
) -> Self {
Behaviour {
substrate,
Expand All @@ -73,7 +74,8 @@ impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> Behaviour<B, S, H> {
local_public_key,
known_addresses,
enable_mdns,
allow_private_ipv4
allow_private_ipv4,
discovery_only_if_under_num,
).await,
events: Vec::new(),
}
Expand Down
26 changes: 21 additions & 5 deletions client/network/src/discovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ pub struct DiscoveryBehaviour<TSubstream> {
/// If false, `addresses_of_peer` won't return any private IPv4 address, except for the ones
/// stored in `user_defined`.
allow_private_ipv4: bool,
/// Number of active connections over which we interrupt the discovery process.
discovery_only_if_under_num: u64,
}

impl<TSubstream> DiscoveryBehaviour<TSubstream> {
Expand All @@ -98,6 +100,7 @@ impl<TSubstream> DiscoveryBehaviour<TSubstream> {
user_defined: Vec<(PeerId, Multiaddr)>,
enable_mdns: bool,
allow_private_ipv4: bool,
discovery_only_if_under_num: u64,
) -> Self {
if enable_mdns {
#[cfg(target_os = "unknown")]
Expand All @@ -120,6 +123,7 @@ impl<TSubstream> DiscoveryBehaviour<TSubstream> {
local_peer_id: local_public_key.into_peer_id(),
num_connections: 0,
allow_private_ipv4,
discovery_only_if_under_num,
#[cfg(not(target_os = "unknown"))]
mdns: if enable_mdns {
match Mdns::new() {
Expand Down Expand Up @@ -302,11 +306,19 @@ where

// Poll the stream that fires when we need to start a random Kademlia query.
while let Poll::Ready(_) = self.next_kad_random_query.poll_unpin(cx) {
let random_peer_id = PeerId::random();
debug!(target: "sub-libp2p", "Libp2p <= Starting random Kademlia request for \
{:?}", random_peer_id);
if self.num_connections < self.discovery_only_if_under_num {
let random_peer_id = PeerId::random();
debug!(target: "sub-libp2p", "Libp2p <= Starting random Kademlia request for \
{:?}", random_peer_id);

self.kademlia.get_closest_peers(random_peer_id);
self.kademlia.get_closest_peers(random_peer_id);
} else {
debug!(
target: "sub-libp2p",
"Kademlia paused due to high number of connections ({})",
self.num_connections
);
}

// Schedule the next random query with exponentially increasing delay,
// capped at 60 seconds.
Expand Down Expand Up @@ -406,6 +418,10 @@ where
NetworkBehaviourAction::GenerateEvent(event) => {
match event {
MdnsEvent::Discovered(list) => {
if self.num_connections >= self.discovery_only_if_under_num {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with this code so might be a stupid question. If we already discovered the node why not keep it? Kademlia and sub protocol account for the peer limits separately right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't really matter for mDNS.
I applied the change to mDNS as well because the variable name is about stopping discovery altogether, and not just Kademlia.

continue;
}

self.discoveries.extend(list.into_iter().map(|(peer_id, _)| peer_id));
if let Some(peer_id) = self.discoveries.pop_front() {
let ev = DiscoveryOut::Discovered(peer_id);
Expand Down Expand Up @@ -473,7 +489,7 @@ mod tests {
let user_defined = user_defined.clone();
let keypair_public = keypair.public();
async move {
DiscoveryBehaviour::new(keypair_public, user_defined, false, true).await
DiscoveryBehaviour::new(keypair_public, user_defined, false, true, 50).await
}
});
let mut swarm = Swarm::new(transport, behaviour, keypair.public().into_peer_id());
Expand Down
1 change: 1 addition & 0 deletions client/network/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> NetworkWorker
TransportConfig::MemoryOnly => false,
TransportConfig::Normal { allow_private_ipv4, .. } => allow_private_ipv4,
},
u64::from(params.network_config.out_peers) * 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a a fixed number of additional discovery connections, rather than a x2. Once we reach the target number of peers, it is fine for discovery to be less aggressive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it for out_peers + 15. The 15 is a bit magic, but I think this option is a bit too specific to warrant a field in the configuration.

));
let (transport, bandwidth) = {
let (config_mem, config_wasm) = match params.network_config.transport {
Expand Down