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

Limit number of incoming connections #391

Merged
merged 9 commits into from
Jul 27, 2018
Merged

Limit number of incoming connections #391

merged 9 commits into from
Jul 27, 2018

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented Jul 20, 2018

Fix #331

Add max_incoming_peers, which is calculated from config max_peers / max_incoming_peers_factor. Limit the number of incoming peers to be below this number.

@sorpaas sorpaas added the A0-please_review Pull request needs code review. label Jul 20, 2018
tomaka
tomaka suggested changes Jul 20, 2018
View reviewed changes
substrate/network-libp2p/src/network_state.rs Outdated
@@ -548,7 +551,8 @@ impl NetworkState {
let node_is_reserved = self.reserved_peers.read().contains(&infos.id);
if !node_is_reserved {
if self.reserved_only.load(atomic::Ordering::Relaxed) ||
num_open_connections >= self.max_peers
num_open_connections.total >= self.max_peers ||
num_open_connections.incoming >= self.max_incoming_peers
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is also reached when we dial. You should use endpoint to determine whether we are dialing or listening.

@tomaka
Copy link
Contributor

tomaka commented Jul 20, 2018

We already limit the number of outgoing connections to min_peers, so I feel like with this code we would never reach max_peers.

@tomaka tomaka added A4-gotissues and removed A0-please_review Pull request needs code review. labels Jul 20, 2018
@sorpaas sorpaas added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A4-gotissues labels Jul 20, 2018
@sorpaas
Copy link
Member Author

sorpaas commented Jul 20, 2018

Tried a fix for the "never reaching max_peers problem" by maintaining outgoing connections to be at least 1 - 1 / incoming_peers_factor. So we limit the number of outgoing connections by:

  • min_peers, if we have less than min_peers.
  • We don't try additional connections once reaching min_peers (just like now), but try to maintain the portion 1 : (incoming_peers_factor - 1).

@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 20, 2018
tomaka
tomaka suggested changes Jul 20, 2018
View reviewed changes
substrate/network-libp2p/src/network_state.rs Outdated
/// Returns the number of new outgoing custom connections to peers to
/// open. This takes into account the number of active peers.
pub fn should_open_outgoing_custom_connections(&self) -> u32 {
use std::cmp::max;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be at top of the file.

substrate/network-libp2p/src/traits.rs Outdated
@@ -190,7 +190,7 @@ impl NetworkConfiguration {
use_secret: None,
min_peers: 25,
max_peers: 50,
max_incoming_peers_factor: 3,
incoming_peers_factor: 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like 3 is too high, especially for bootstrap nodes which probably want 90% of incoming connections.

Copy link
Member Author

Choose a reason for hiding this comment

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

So 2, probably?

Copy link
Member Author

Choose a reason for hiding this comment

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

And just for information, was choosing 3 because it comes from the eclipse attack paper.

@gavofyork
Copy link
Member

@tomaka better?

@tomaka
Copy link
Contributor

tomaka commented Jul 24, 2018

This PR is in conflict with #412
We can merge it, but all the changes will be reverted in order to do #412

@sorpaas
Copy link
Member Author

sorpaas commented Jul 25, 2018

@tomaka Tried to change this to implement #412's peer counting.

@sorpaas
Copy link
Member Author

sorpaas commented Jul 25, 2018

And just wonder, what are the reasons that we use u32 instead of usize for some of the config parameters? I think it doesn't actually save memory usage?

@sorpaas
Copy link
Member Author

sorpaas commented Jul 25, 2018

Ah I see, from my understanding, looks like we need it for some cases in wasm.

@gavofyork gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jul 27, 2018
@gavofyork gavofyork merged commit 0d99cb2 into master Jul 27, 2018
@gavofyork gavofyork deleted the sp-eclipse-flooding branch July 27, 2018 14:13
dvdplm added a commit that referenced this pull request Jul 30, 2018
@dvdplm
Merge branch 'master' into refactor/substrate-state-machine-generic
f7c30f4 
* master: (86 commits)
  Make contract a separate runtime module (#345)
  Version bump (#450)
  DB-based blockchain data cache for light nodes (#251)
  Update libp2p again (#445)
  Update version on git head change (#444)
  Fix the public key of bootnode 3 (#441)
  Update libp2p (#442)
  Switch to the master branch of libp2p (#427)
  Export ws port 9944 and add doc (#440)
  Iterate over overlay to decide which keys to purge (#436)
  Exit signal gets its own trait (#433)
  Add docker image (#375)
  Reset peers.json if the content is not loadable (#405)
  Limit number of incoming connections (#391)
  Fix memory leaks in libp2p (#432)
  Do not queue empty blocks set for import (#431)
  5 random fixes (#1) (#435)
  Chore: fix typo (#434)
  Prevent building invalid blocks (#430)
  Better logging for public key mismatch (#429)
  ...
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
@rphmeier @gavofyork
pay out slashes to the treasury (paritytech#391)
5e1f3f1
liuchengxu added a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
@liuchengxu @toxotguo
Dive into spot (paritytech#391)
bcb1f39 
* Refactored Spot and Test
liuchengxu added a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
@liuchengxu
Prepare 2.0 genesis (paritytech#391)
25321fd
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomaka tomaka tomaka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sorpaas @tomaka @gavofyork