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.

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

fix(net): Reduce maximum number of connections per IP to 1 #6993

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 18, 2023

Motivation

After running PR #6980 over the weekend, I started getting these logs:

2023-06-18T22:11:23.779916Z INFO crawl: zebra_network::peer_set::set: duplicate IP addresses in peer set duplicate_connections=13 duplicated_peers=6 peers=46

This means that 15% of the peer set (7/46) is redundant connections, which seems like a peer set takeover risk.

I'm running 3 nodes locally on 2 IPs, so it's possible that 4 of those connections are from my local setup. But that would still be 11% redundant connections (5/46) from external nodes.

I suggest we aim for 5-10% redundant connections at most.

In my setup:

  • 1 peer per IP would be 0% redundant connections. (Unless a peer has multiple IP addresses, which we can't detect.)
  • 2 peers per IP would be 9% redundant connections from external nodes (4/46), or 13% from all nodes (6/46).

I don't mind whether we choose 1 or 2, but 3 peers per IP seems to allow too many redundant connections.

Specifications

zcashd's limit is one connection per IP address:
https://github.com/zcash/zcash/blob/fa3827656df8f97fb4700b22022cc581fbda61fa/src/net.cpp#L1663

Solution

Limit Zebra to 1 connection per IP address.

Related fixes:

  • Fix tests that require more than 1 connection per IP

Review

This should go in before the next release.

@arya2 wrote the original PR #6980.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added P-Medium ⚡ C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes I-remote-trigger Remote nodes can make Zebra do something bad labels Jun 18, 2023
@teor2345 teor2345 requested a review from a team as a code owner June 18, 2023 22:36
@teor2345 teor2345 self-assigned this Jun 18, 2023
@teor2345 teor2345 requested review from arya2 and removed request for a team June 18, 2023 22:36
@github-actions github-actions bot added C-bug Category: This is a bug labels Jun 18, 2023
Base automatically changed from limit-peers-per-ip to main June 19, 2023 00:53
@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #6993 (85fda39) into main (73ce8fb) will decrease coverage by 0.18%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6993      +/-   ##
==========================================
- Coverage   77.55%   77.38%   -0.18%     
==========================================
  Files         310      310              
  Lines       41582    41582              
==========================================
- Hits        32248    32177      -71     
- Misses       9334     9405      +71     

mergify bot added a commit that referenced this pull request Jun 19, 2023
@mergify mergify bot merged commit 231f5be into main Jun 19, 2023
@mergify mergify bot deleted the reduce-conns-per-ip branch June 19, 2023 18:18
@teor2345 teor2345 changed the title fix(net): Reduce maximum number of connections per IP fix(net): Reduce maximum number of connections per IP to 1 Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-remote-trigger Remote nodes can make Zebra do something bad
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants