Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-1683] Fix handling of remote connection limit #1676

Merged
merged 4 commits into from
Jul 12, 2019
Merged

[PAN-1683] Fix handling of remote connection limit #1676

merged 4 commits into from
Jul 12, 2019

Conversation

mbaxter
Copy link
Contributor

@mbaxter mbaxter commented Jul 11, 2019

PR description

This PR fixes a few bugs in how remote connection limits are enforced:

  • Fix an order of operations error in the calculation of the fraction of remotely connected peers
  • Remove the branch that returns early if no peers are currently connected (its possible that 1 remote connection is too many)
  • Change disconnect reason to TOO_MANY_PEERS
  • Validate that fractionRemoteWireConnectionsAllowed is between 0.0 and 1.0 inclusive
    • A fraction of 0 is valid, meaning no untrusted remote connections should be allowed
  • Add additional tests
  • Ignore tests that are broken by this new functionality (for now). These limits will be reworked in a follow-up PR and these test will be re-enabled.

mbaxter added 2 commits July 11, 2019 18:04
@mbaxter
Fix bugs
59c7ad1 
Fix remote connection fraction calculation (order of operations bug),
remove early return, allow all remote connections to be prohibited,
change disconnect reason to TOO_MANY_PEERS.
@mbaxter
Fix comment
9d51fb1
@mbaxter mbaxter marked this pull request as ready for review July 11, 2019 22:22
shemnon
shemnon reviewed Jul 11, 2019
View reviewed changes
ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/rlpx/RlpxAgent.java
final double fractionRemoteConnections =
(double) remotelyInitiatedConnectionsCount + 1 / (double) maxPeers;
(double) (remotelyInitiatedConnectionsCount + 1) / (double) maxPeers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will maxPeers=0 cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good eye! however, turns out this actually works. this will evaluate to Infinity in that case and the comparison will return true.

@AbdelStark AbdelStark merged commit 002bb6a into PegaSysEng:master Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shemnon shemnon shemnon 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
@mbaxter @shemnon @AbdelStark