Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit the number of inbound peer connections #2961

Merged
merged 9 commits into from
Oct 28, 2021
Merged

Limit the number of inbound peer connections #2961

merged 9 commits into from
Oct 28, 2021

Conversation

teor2345
Copy link
Contributor

Motivation

Zebra has no limit on the number of inbound peer connections it will accept.

Solution

  • Limit open inbound connections based on the config
  • Log inbound connection errors at debug level
  • Test inbound connection limits

Closes #1851.

Review

@jvff was reviewing PR #2944, which this PR is based on.

This PR is on the critical path for Sprint 21.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@teor2345 teor2345 added P-High C-security Category: Security issues I-unbounded-growth Zebra keeps using resources, without any limit A-network Area: Network protocol updates or fixes labels Oct 27, 2021
@teor2345 teor2345 requested a review from jvff October 27, 2021 03:50
@teor2345 teor2345 self-assigned this Oct 27, 2021
@teor2345 teor2345 linked an issue Oct 27, 2021 that may be closed by this pull request
Security: Limit the number of inbound peer connections in the listener task #1851
Closed
@teor2345 teor2345 mentioned this pull request Oct 27, 2021
Merged
2 tasks
@teor2345 teor2345 changed the title Limit inbound peers Limit the number of inbound peer connections Oct 27, 2021
jvff
jvff previously approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

We can merge this as is, but I think there is some opportunities to improve the tests. I added a few comments and suggestions.

We might want to do a larger refactor of them later to try to reduce the repeated code, maybe use a property test and/or a semaphore to always count active connections 🤔. Right now it would be extra work that might delay the PR :/

zebra-network/src/peer_set/initialize.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 mentioned this pull request Oct 27, 2021
Merged
3 tasks
Base automatically changed from limit-outbound-peers to main October 27, 2021 21:28
@teor2345
Copy link
Contributor Author

To fix the slow test from #2944 (comment) , I decreased its configured peer limit in b50e322, which automatically decreases the expected number of peers.

@teor2345 teor2345 enabled auto-merge (squash) October 28, 2021 00:40
@teor2345 teor2345 requested a review from jvff October 28, 2021 00:40
@teor2345
Copy link
Contributor Author

We might want to do a larger refactor of them later to try to reduce the repeated code, maybe use a property test and/or a semaphore to always count active connections 🤔.

I just realised I didn't reply to this.

I already did some refactors to reduce repeated code - I'd be happy to move more code into the setup methods in a future PR.

@teor2345 teor2345 merged commit f26a60b into main Oct 28, 2021
@teor2345 teor2345 deleted the limit-inbound-peers branch October 28, 2021 01:49
mpguerra added a commit that referenced this pull request May 19, 2023
@mpguerra
#1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixe…
9b67e09 
…d by #2969
mergify bot pushed a commit that referenced this pull request May 23, 2023
@mpguerra @teor2345
Delete outdated TODOs refering to closed issues (#6732)
ec2e9ca 
* ZIPs were updated to remove ambiguity, this was tracked in #1267.

* #2105 was fixed by #3039 and #2379 was closed by #3069

* #2230 was a duplicate of #2231 which was closed by #2511

* #3235 was obsoleted by #2156 which was fixed by #3505

* #1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixed by #2969

* We migrated to Rust 2021 edition in Jan 2022 with #3332

* #1631 was closed as not needed

* #338 was fixed by #3040 and #1162 was fixed by #3067

* #2079 was fixed by #2445

* #4794 was fixed by #6122

* #1678 stopped being an issue

* #3151 was fixed by #3934

* #3204 was closed as not needed

* #1213 was fixed by #4586

* #1774 was closed as not needed

* #4633 was closed as not needed

* Clarify behaviour of difficulty spacing

Co-authored-by: teor <teor@riseup.net>

* Update comment to reflect implemented behaviour

Co-authored-by: teor <teor@riseup.net>

* Update comment to reflect implemented behaviour when retrying block downloads

Co-authored-by: teor <teor@riseup.net>

* Update `TODO` to remove closed issue and clarify when we might want to fix

Co-authored-by: teor <teor@riseup.net>

* Update `TODO` to remove closed issue and clarify what we might want to change in future

Co-authored-by: teor <teor@riseup.net>

* Clarify benefits of how we do block verification

Co-authored-by: teor <teor@riseup.net>

* Fix rustfmt errors

---------

Co-authored-by: teor <teor@riseup.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvff jvff jvff approved these changes

Assignees

@teor2345 teor2345

Labels
A-network Area: Network protocol updates or fixes C-security Category: Security issues I-unbounded-growth Zebra keeps using resources, without any limit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Security: Limit the number of inbound peer connections in the listener task
2 participants
@teor2345 @jvff