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

Limit incoming connections. #8060

Merged
kirushik merged 2 commits into
masterfrom
net-ingress-fix
Mar 9, 2018
Merged

Limit incoming connections. #8060
kirushik merged 2 commits into
masterfrom
net-ingress-fix

Conversation

@arkpar
Copy link
Copy Markdown
Collaborator

@arkpar arkpar commented Mar 5, 2018 •
edited
Loading

This limits incoming connections to max(max_peers - min_peers, min_peers / 2)
effectively reserving at least min_peers / 2 connections to be outbound.

Needs careful review and testing on a machine that can accept connections.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 5, 2018
@5chdn 5chdn added the B0-patch label Mar 5, 2018
@5chdn 5chdn added this to the 1.11 milestone Mar 5, 2018
kirushik
kirushik reviewed Mar 5, 2018
View reviewed changes
Comment thread util/network-devp2p/src/host.rs Outdated
started += 1;
}
debug!(target: "network", "Connecting peers: {} sessions, {} pending, {} started", self.session_count(), self.handshake_count(), started);
debug!(target: "network", "Connecting peers: {} sessions, {} pending, {} started", egress_count + ingress_count, self.handshake_count(), started);
Copy link
Copy Markdown
Collaborator

@kirushik kirushik Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why not calling a variable handshake_count? Calling a method initiates another iteration of session_count().

kirushik
kirushik approved these changes Mar 5, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kirushik kirushik left a comment

Choose a reason for hiding this comment

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

Looks good (and requires some thorough real-world testing, which I didn't do), except for that minor effectiveness issue.

@kirushik kirushik added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 5, 2018
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Mar 5, 2018
@kirushik kirushik added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 5, 2018
@kirushik
Copy link
Copy Markdown
Collaborator

kirushik commented Mar 5, 2018

Please don't forget to battle-test this on nightly before backporting to other release channels (which, I think, we should do eventually).

andresilva
andresilva approved these changes Mar 6, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Comment thread util/network-devp2p/src/host.rs
(info.config.min_peers as usize, max_peers as usize, info.config.non_reserved_mode == NonReservedPeerMode::Deny, info.id().clone())
};

max_peers = max(max_peers, min_peers);
Copy link
Copy Markdown
Contributor

@andresilva andresilva Mar 6, 2018

Choose a reason for hiding this comment

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

Why is this needed? (assuming we validate in the config that max_peers >= min_peers)

Copy link
Copy Markdown
Collaborator Author

@arkpar arkpar Mar 7, 2018

Choose a reason for hiding this comment

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

Just another sanity check, so that this code does not rely on config.

@andresilva
Copy link
Copy Markdown
Contributor

andresilva commented Mar 6, 2018

I'll test this tomorrow once I have a proper internet connection.

@andresilva
Copy link
Copy Markdown
Contributor

andresilva commented Mar 7, 2018

This is working properly for me.

@kirushik kirushik merged commit e810601 into master Mar 9, 2018
@ddorgan
Copy link
Copy Markdown
Collaborator

ddorgan commented Mar 9, 2018

@arkpar @andresilva I installed master on a boot node and only had about 11 peers after 30 minutes. When I reverted to 1.8.4 it got 18 peers straight away.

Is there logs or debug info I can provide to you on this?

BTW the node is running with these options:

parity --chain kovan --log-file parity.log --auto-update=all --no-dapps --no-ui --max-peers 500 --snapshot-peers 1200 --pruning-history 1200   --jsonrpc-apis web3,eth,parity,parity_set,net,traces,rpc --no-warp --allow-ips public

@tomusdrw tomusdrw deleted the net-ingress-fix branch March 16, 2018 12:45
tomusdrw pushed a commit that referenced this pull request Mar 16, 2018
@arkpar @tomusdrw
Limit incoming connections. (#8060)
638f5d1 
* Limit ingress connections
* Optimized handshakes logging
@tomusdrw tomusdrw mentioned this pull request Mar 16, 2018
Merged
9 tasks
andresilva pushed a commit that referenced this pull request Mar 16, 2018
@arkpar @andresilva
Limit incoming connections. (#8060)
ba457a9 
* Limit ingress connections
* Optimized handshakes logging
@andresilva andresilva mentioned this pull request Mar 16, 2018
Merged
14 tasks
debris pushed a commit that referenced this pull request Mar 19, 2018
@tomusdrw @debris
[stable] Backports (#8133)
feef2f8 
* updater: apply exponential backoff after download failure (#8059)

* updater: apply exponential backoff after download failure

* updater: reset backoff on new release

* Limit incoming connections.  (#8060)

* Limit ingress connections
* Optimized handshakes logging

* Max code size on Kovan (#8067)

* Enable code size limit on kovan

* Fix formatting.

* add some dos protection (#8084)

* more dos protection (#8104)

* Const time comparison (#8113)

* Use `subtle::slices_equal` for constant time comparison.

Also update the existing version of subtle in `ethcrypto` from
0.1 to 0.5

* Test specifically for InvalidPassword error.

* revert removing blooms (#8066)

* Revert "fix traces, removed bloomchain crate, closes #7228, closes #7167"

This reverts commit 1bf6203.

* Revert "fixed broken logs (#7934)"

This reverts commit f8a2e53.

* fixed broken logs

* bring back old lock order

* remove migration v13

* revert CURRENT_VERSION to 12 in migration.rs

* Fix compilation.

* Check one step deeper if we're on release track branches

* add missing pr

* Fix blooms?

* Fix tests compiilation.

* Fix size.
andresilva pushed a commit that referenced this pull request Mar 19, 2018
@arkpar @andresilva
Limit incoming connections. (#8060)
2b73a64 
* Limit ingress connections
* Optimized handshakes logging
tomusdrw pushed a commit that referenced this pull request Mar 19, 2018
@andresilva @tomusdrw
[Beta] Backports (#8136)
610f6f1 
* Support parity protocol. (#8035)

* updater: apply exponential backoff after download failure (#8059)

* updater: apply exponential backoff after download failure

* updater: reset backoff on new release

* Max code size on Kovan (#8067)

* Enable code size limit on kovan

* Fix formatting.

* Limit incoming connections.  (#8060)

* Limit ingress connections
* Optimized handshakes logging

* WASM libraries bump (#7970)

* update wasmi, parity-wasm, wasm-utils to latest version

* Update to new wasmi & error handling

* also utilize new stack limiter

* fix typo

* replace dependency url

* Cargo.lock update

* add some dos protection (#8084)

* revert removing blooms (#8066)

* Revert "fix traces, removed bloomchain crate, closes #7228, closes #7167"

This reverts commit 1bf6203.

* Revert "fixed broken logs (#7934)"

This reverts commit f8a2e53.

* fixed broken logs

* bring back old lock order

* remove migration v13

* revert CURRENT_VERSION to 12 in migration.rs

* more dos protection (#8104)

* Const time comparison (#8113)

* Use `subtle::slices_equal` for constant time comparison.

Also update the existing version of subtle in `ethcrypto` from
0.1 to 0.5

* Test specifically for InvalidPassword error.

* fix trace filter returning returning unrelated reward calls, closes #8070 (#8098)

* network: init discovery using healthy nodes (#8061)

* network: init discovery using healthy nodes

* network: fix style grumble

* network: fix typo

* Postpone Kovan hard fork (#8137)

* ethcore: postpone Kovan hard fork

* util: update version fork metadata

* Disable UI by default. (#8105)

* dapps: update parity-ui dependencies (#8160)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@andresilva andresilva andresilva approved these changes

@kirushik kirushik kirushik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

1.11 Prosperity

Development

Successfully merging this pull request may close these issues.

5 participants

@arkpar @kirushik @andresilva @ddorgan @5chdn