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

Update init.go to increase max outbound peers for validator nodes #464

Merged
merged 4 commits into from
Oct 2, 2021

Conversation

bfritton
Copy link
Contributor

@bfritton bfritton commented Sep 8, 2021

Change to max outbound peers for validator node as requested by Valardragon and proposed by FONDSMATIVE and dff | mp20

Change to max outbound peers for validator node as requested by Valardragon and proposed by FONDSMATIVE and dff | mp20
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM! Just going to leave open for a day or so, to check if any other folks would like to opine. I think this is long term too high, but probably useful short term.

My reasoning is that with high numbers of peers being saturated on the peerlist, increasing the number of outbound connections you try makes sense. And when the p2p network reforms each epoch, you retry connecting to more peers.

(Granted Tendermint p2p is complex, and I may be misunderstanding this!)

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2021

Codecov Report

Merging #464 (cba87c5) into main (0929265) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #464   +/-   ##
=======================================
  Coverage   20.20%   20.20%           
=======================================
  Files         163      163           
  Lines       23358    23358           
=======================================
  Hits         4720     4720           
  Misses      17878    17878           
  Partials      760      760           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0929265...cba87c5. Read the comment docs.

@faddat
Copy link
Member

faddat commented Sep 22, 2021

I'd say "do not merge".

I recently made a PR on Tendermint itself and the key to my logic was changing the ratio:

I went from previous default (10 outbound and 40 inbound)

to

new default (20 outbound and 160 inbound)

the goal is to have peers gettin all promiscuous. In the p2p layer, we want them connecting, lots.

@ValarDragon
Copy link
Member

Should we be updating the defaults here to those settings Jacob?

@faddat
Copy link
Member

faddat commented Oct 2, 2021

Yes, I should have made a PR to his branch.

I'll do that now.

@faddat
Copy link
Member

faddat commented Oct 2, 2021

40 and 320 actually seem more appropriate for us.

The network has always seemed kind of choked.

faddat added 2 commits October 2, 2021 18:15
Updated update to inbound and outbound connections to keep an 8:1 ratio of inbound slots to outbound.
@faddat
Copy link
Member

faddat commented Oct 2, 2021

@bfritton I'm sorry I slowed this down!

Have been super busy lately. Surely, we needed to change these settings.

❤️

pseudo-go-fmt
@ValarDragon ValarDragon merged commit 1339f3c into osmosis-labs:main Oct 2, 2021
ValarDragon pushed a commit that referenced this pull request Oct 11, 2021
* Update init.go

Change to max outbound peers for validator node as requested by Valardragon and proposed by FONDSMATIVE and dff | mp20

* Update init.go

Updated update to inbound and outbound connections to keep an 8:1 ratio of inbound slots to outbound.

* Update init.go

pseudo-go-fmt

Co-authored-by: Jacob Gadikian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants