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

Allow to have more than min_peers/2 outbound peers #2537

Merged
merged 2 commits into from
Feb 7, 2019

Conversation

hashmap
Copy link
Contributor

@hashmap hashmap commented Feb 6, 2019

Default value is 8, so we allow only 4 outbound peers, unfortunately if a node is behind NAT it can't get inbound peers and it stucks with 4 peers as maximum.
This PR allows a number of outbound peers to be between min_peers and max_peers/2. Node behind NAT will eventually have just min_peers number of peers.

Default value is 8, so we allow only 4 outbound peers, unfortunately if a node is behind NAT it can't get inbound peers and it stucks with 4 peers as maximum.
This PR allows a number of outbound peers to be between min_peers and max_peers/2. Node behind NAT will eventually have just min_peers number of peers.
@hashmap
Copy link
Contributor Author

hashmap commented Feb 6, 2019

Another option is to keep the second condition as peers.peer_outbound_count() >= p2p.config.peer_min_preferred_count() / 2, as I said a node behind NAT will eventually have min_peers anyway, but in this case we can save a few slots for inbound peers of a node with public ip.

@hashmap hashmap added this to the 1.0.2 milestone Feb 7, 2019
@hashmap hashmap added the bug label Feb 7, 2019
@antiochp
Copy link
Member

antiochp commented Feb 7, 2019

We do something similar to what you propose in monitor_peers (but looking at preferred counts in both cases) -

// We have enough peers, both total connected and outbound connected so we are good.
if peers.peer_count() >= config.peer_min_preferred_count()
&& peers.peer_outbound_count() >= config.peer_min_preferred_count() / 2
{
return;
}

Here we keep the "total peer count" below peer_min_preferred_count and the "total outbound peer count" below peer_min_preferred_count / 2.

I wonder if we should look at both "total peer count" and "total outbound peer count" as you suggest, in listen_for_addrs, but comparing to preferred count (and preferred / 2)?

i.e. We go try and connect to more outbound peers if -

  • we have less than peer_min_preferred_count / 2 outbound peers
  • or, we have less than peer_min_preferred_count total peers

That way we handle nodes behind NAT by just looking at total peers (we will never have inbound peers)
but for public nodes we default to looking at outbound peer counts.

I think it may make sense to keep the listen_for_addrs and monitor_peers peer counting logic consistent.

@antiochp
Copy link
Member

antiochp commented Feb 7, 2019

Good catch by the way - I don't think the NAT case was fully considered here when we split out default 8 slots into inbound/outbound.

@hashmap
Copy link
Contributor Author

hashmap commented Feb 7, 2019

I tested the latest version of this PR - after 20 mins a node behind NAT oscillates around 8 peers in total (all outbound), a node with a public IP has 4 outbounds (55 in the first seconds) and 100+ inbounds.

@hashmap
Copy link
Contributor Author

hashmap commented Feb 7, 2019

Ah, I pushed to the origin by mistake, removed the branch but Travis noticed it and now tries to build this removed branch too, please ignore push build

@antiochp antiochp self-requested a review February 7, 2019 13:36
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Yeah this looks good.

}

/// We have enough peers, both total connected and outbound connected
pub fn healthy_peers_mix(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

@ignopeverell ignopeverell merged commit a63cfa7 into mimblewimble:master Feb 7, 2019
@hashmap hashmap deleted the fix-nat-peer-number branch April 4, 2019 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants