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

improve checking for p2p connection limits (revised) #2993

Merged
merged 8 commits into from
Aug 21, 2019

Conversation

antiochp
Copy link
Member

Replacement PR for #2985 which we reverted in 17dddee

Tweaked the limit in terms of how many peer connections we attempt regardless of our desired outbound connections.

The key point here is the vast majority of our peer addresses in the db are not publicly accessible.
So if we only try a random 8 then we stand a good chance of failing to connect to any new ones.

This is particularly important during the initial seed step on node startup - we want to try all the seeds retrieved via dns, not just 8 of them. If these 8 fail for any reason then we end up stuck with our node not making any further attempts.

@antiochp antiochp requested a review from hashmap August 21, 2019 15:18
@antiochp antiochp self-assigned this Aug 21, 2019
@j01tz
Copy link
Member

j01tz commented Aug 21, 2019

My issue cleared up locally when I reverted fb9b06e still doing some more testing

@antiochp
Copy link
Member Author

We were originally taking max peers (125) each time off the address queue and attempting to connect to them.

for addr in addrs.into_iter().take(p2p.config.peer_max_count() as usize) {

#2985 modified this to only take max outbound peers (8) each time.

for addr in addrs
	.into_iter()
	.take(p2p.config.peer_max_outbound_count() as usize)

So every 30s we were only attempting to connect to 8 new peers, with a high possibility that all or most of those were not publicly accessible.

This is why initial seeding sometimes failed completely and why it seems to be slower to get new peer connections.

This revised PR relaxes this rule and simply takes a max of 128 new peer addresses each loop, regardless of the inbound/outbound limits.

let max_outbound_attempts = 128;
for addr in addrs.into_iter().take(max_outbound_attempts) {

Testing seems to indicate this is significantly faster at filling up the default 8 outbound slots as we have a reasonable chance of finding publicly accessible nodes every loop now.

We also have a far more reliable initial seeding as we allow our node to attempt to connect to all the seeds up front.

@antiochp
Copy link
Member Author

antiochp commented Aug 21, 2019

This config seems to be pretty stable behind a firewall, with my node looking (and finding a couple of new ones) for publicly accessible peers every 30s -

peer_max_inbound_count = 0
peer_max_outbound_count = 64
peer_min_preferred_outbound_count = 32

Takes a few minutes but its churning through the full set of peers in the db, marking large numbers as defunct -

20190821 19:50:46.094 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 
  29 connected (26 most_work). all 4389 = 1584 healthy + 0 banned + 2805 defunct

Got to our preferred 32 outbound eventually, 3702 defunct along the way -

20190821 19:54:26.899 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 
  32 connected (29 most_work). all 4610 = 908 healthy + 0 banned + 3702 defunct

And restarting the node connects successfully to at least 32 outbound connections quickly, within a few seconds -

20190821 19:55:57.635 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 
  34 connected (31 most_work). all 4636 = 932 healthy + 0 banned + 3704 defunct

@antiochp antiochp merged commit 5bf813e into mimblewimble:master Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants