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

lower bound peer limit #4200

Merged
merged 31 commits into from
Aug 9, 2022
Merged

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Aug 1, 2022

Added (experimental) CLI option for a lower bound for p2p peers. Similar to Teku's https://docs.teku.consensys.net/en/latest/Reference/CLI/CLI-Syntax/#p2p-peer-lower-bound

So we actively try to find more peers up to --Xp2p-peer-lower-bound, but still allow incoming connections until we hit --p2p-peer-upper-bound AKA --max-peers, only then we refuse incoming connections.

  • Note currently this PR changes the default for max-peers - happy to change this back once the plumbing for deploying canary nodes supports the "lower-bound" param
  • Timer refresh for the peer table tableRefreshIntervalMs is 30 minutes

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

Filter94 and others added 11 commits July 27, 2022 10:18
…ledger#4170)

Signed-off-by: Roman Vaseev <4833306+Filter94@users.noreply.github.com>

Co-authored-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Aug 1, 2022
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
@@ -228,7 +228,7 @@ public void callingVersionDisplayBesuInfoVersion() {
public void callingBesuCommandWithoutOptionsMustSyncWithDefaultValues() throws Exception {
parseCommand();

final int maxPeers = 25;
final int maxPeers = 100;
Copy link
Contributor

@garyschulte garyschulte Aug 2, 2022

Choose a reason for hiding this comment

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

💪

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

Looks good on first run through, some minor nitpicks

Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, a couple of log level suggestions.
The main point is to verify if and how the increased number of peers impacts the perfomance on small machines.

Fun fact:
playing with the configuration of remote-connections-max-percentage, is possible to launch Besu in a way that it never reaches the max peers upped limit, for example is lower limit is 50, upper limit is 100, and remote-connections-max-percentage is 10, the max number of connected peers should be 60

Comment on lines 60 to 61
int DEFAULT_MAX_PEERS = 100;
int DEFAULT_P2P_PEER_LOWER_BOUND = 64;
Copy link
Contributor

Choose a reason for hiding this comment

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

since this change max peers to 4 times the actual max, it could impact memory, cpu and lock contention, especially on resource constrained nodes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I want to collect some metrics on this. maybe 25/50 would be a more sensible default.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be very cautious about raising the peer limit. From what I've seen besu works really well when it has 25 peers, it just tends to struggling to fill it's peer quota at times which is when it then struggles. Experimenting with different values by overriding them in config is probably better than changing the defaults in code before we've checked what impact it will have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I've changed the defaults to min = max = 25 which is equivalent to current behavior.

Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
@macfarla
Copy link
Contributor Author

macfarla commented Aug 9, 2022

Nodes running on mainnet with min=25 max=35 and two control nodes (22.7.0) with max=25 and max=35. All nodes get to min/max number of peers within ~17 minutes and peer numbers remain stable
Screen Shot 2022-08-09 at 2 05 36 pm

@macfarla
Copy link
Contributor Author

macfarla commented Aug 9, 2022

Nodes running on goerli with min=max=25
nodes get to max peers within 1hr and remain stable

Screen Shot 2022-08-08 at 3 39 41 pm

@macfarla
Copy link
Contributor Author

macfarla commented Aug 9, 2022

nodes running SNAP sync on goerli - peering behavior much more variable.

Screen Shot 2022-08-08 at 3 28 04 pm

@macfarla macfarla merged commit 37e0e04 into hyperledger:main Aug 9, 2022
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Aug 10, 2022
@macfarla macfarla deleted the target-peer-limit branch August 22, 2022 23:24
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* introducing a lower bound on number of peers
* added lower bound CLI option

Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Signed-off-by: Roman Vaseev <4833306+Filter94@users.noreply.github.com>
Co-authored-by: Roman Vaseev <4833306+Filter94@users.noreply.github.com>
Co-authored-by: Justin Florentine <justin+github@florentine.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mainnet peering TeamRevenant GH issues worked on by Revenant Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants