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

Limit number of incoming connections - #391

Merged
gavofyork merged 9 commits into
masterfrom
sp-eclipse-flooding
Jul 27, 2018
Merged

Limit number of incoming connections#391
gavofyork merged 9 commits into
masterfrom
sp-eclipse-flooding

Conversation

@sorpaas

@sorpaassorpaas commented Jul 20, 2018

Copy link
Copy Markdown
Contributor

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.

@sorpaassorpaas added the A0-please_review Pull request needs code review. label Jul 20, 2018
tomaka
tomaka suggested changes Jul 20, 2018
View reviewed changes
Comment threadsubstrate/network-libp2p/src/network_state.rs Outdated
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

@tomakatomakaJul 20, 2018

Copy link
Copy Markdown
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

tomaka commented Jul 20, 2018

Copy link
Copy Markdown
Contributor

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

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

sorpaas commented Jul 20, 2018

Copy link
Copy Markdown
ContributorAuthor

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).

@sorpaassorpaas 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
Comment threadsubstrate/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;

@tomakatomakaJul 20, 2018

Copy link
Copy Markdown
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.

Comment threadsubstrate/network-libp2p/src/traits.rs Outdated
min_peers: 25,
max_peers: 50,
max_incoming_peers_factor: 3,
incoming_peers_factor: 3,

@tomakatomakaJul 20, 2018

Copy link
Copy Markdown
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.

@sorpaassorpaasJul 20, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So 2, probably?

@sorpaassorpaasJul 20, 2018

Copy link
Copy Markdown
ContributorAuthor

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

gavofyork commented Jul 24, 2018

Copy link
Copy Markdown
Member

@tomaka better?

@tomaka

tomaka commented Jul 24, 2018

Copy link
Copy Markdown
Contributor

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

@sorpaas

sorpaas commented Jul 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@sorpaas

sorpaas commented Jul 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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

sorpaas commented Jul 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@gavofyorkgavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jul 27, 2018
@gavofyork
gavofyork merged commit 0d99cb2 into masterJul 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@tomakatomakatomaka approved these changes
Reviewers whose approvals may not affect merge requirements

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