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
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3075049
Fixed desired gas limit setting in the reference tests service (#4170)
Filter94 Jul 26, 2022
9854bb3
first draft of introducing a lower bound on number of peers
macfarla Jul 28, 2022
7b11607
added lower bound CLI option
macfarla Jul 28, 2022
59a7fea
change default values back to current defaults
macfarla Jul 29, 2022
a94a19f
Merge branch 'main' of github.com:hyperledger/besu into target-peer-l…
macfarla Jul 31, 2022
3c1003f
hidden CLI option
macfarla Jul 31, 2022
40bd6ff
formatting
macfarla Jul 31, 2022
8a1da6e
add test
macfarla Aug 1, 2022
e426552
test
macfarla Aug 1, 2022
9ce3374
added rlpx test
macfarla Aug 1, 2022
2fa135c
changelog
macfarla Aug 1, 2022
64b97ed
changed defaults
macfarla Aug 1, 2022
fa6e752
changed defaults
macfarla Aug 1, 2022
f95996f
log
macfarla Aug 2, 2022
764fb35
merge and add test
macfarla Aug 3, 2022
0daf9b3
add validateOptions and some tests
macfarla Aug 3, 2022
85b4b43
formatting
macfarla Aug 3, 2022
a6e0bc7
more tests; set min = max if in doubt
macfarla Aug 3, 2022
fc41df2
Merge branch 'main' into target-peer-limit
macfarla Aug 3, 2022
efc7b26
merge
macfarla Aug 4, 2022
73107bc
logging
macfarla Aug 4, 2022
892d596
Merge branch 'main' of github.com:hyperledger/besu into target-peer-l…
macfarla Aug 4, 2022
cd868af
Merge branch 'target-peer-limit' of github.com:macfarla/besu into tar…
macfarla Aug 4, 2022
20d1880
defaults minPeers/maxPeers 25/50
macfarla Aug 4, 2022
104ac95
defaults minPeers/maxPeers 25/30
macfarla Aug 4, 2022
2d4e5ee
Merge branch 'main' into target-peer-limit
macfarla Aug 5, 2022
ca216c1
changed defaults to 25/25
macfarla Aug 5, 2022
2747c9c
Merge branch 'main' of github.com:hyperledger/besu into target-peer-l…
macfarla Aug 5, 2022
effec82
Merge branch 'target-peer-limit' of github.com:macfarla/besu into tar…
macfarla Aug 5, 2022
de5eaab
Merge branch 'main' into target-peer-limit
macfarla Aug 8, 2022
c110861
Merge branch 'main' into target-peer-limit
macfarla Aug 9, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog


## 22.7.1

### Additions and Improvements
- Experimental CLI option for --Xp2p-peer-lower-bound [#4200](https://github.com/hyperledger/besu/pull/4200)

## 22.7.0

### Additions and Improvements
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ public void startNode(final BesuNode node) {
.build();

final int maxPeers = 25;
final int minPeers = 25;

builder
.synchronizerConfiguration(new SynchronizerConfiguration.Builder().build())
Expand Down Expand Up @@ -198,6 +199,7 @@ public void startNode(final BesuNode node) {
.p2pAdvertisedHost(node.getHostName())
.p2pListenPort(0)
.maxPeers(maxPeers)
.minPeers(minPeers)
.networkingConfiguration(node.getNetworkingConfiguration())
.jsonRpcConfiguration(node.jsonRpcConfiguration())
.webSocketConfiguration(node.webSocketConfiguration())
Expand Down
13 changes: 12 additions & 1 deletion besu/src/main/java/org/hyperledger/besu/RunnerBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ public class RunnerBuilder {
private String natManagerServiceName;
private boolean natMethodFallbackEnabled;
private int maxPeers;
private int minPeers;
private boolean limitRemoteWireConnectionsEnabled = false;
private float fractionRemoteConnectionsAllowed;
private EthNetworkConfig ethNetworkConfig;
Expand Down Expand Up @@ -444,7 +445,8 @@ public Runner build() {
RlpxConfiguration.create()
.setBindHost(p2pListenInterface)
.setBindPort(p2pListenPort)
.setMaxPeers(maxPeers)
.setPeerUpperBound(maxPeers)
.setPeerLowerBound(minPeers)
.setSupportedProtocols(subProtocols)
.setClientId(BesuInfo.nodeName(identityString))
.setLimitRemoteWireConnectionsEnabled(limitRemoteWireConnectionsEnabled)
Expand Down Expand Up @@ -1157,4 +1159,13 @@ private Optional<MetricsService> createMetricsService(
final Vertx vertx, final MetricsConfiguration configuration) {
return MetricsService.create(vertx, configuration, metricsSystem);
}

public int getMinPeers() {
return minPeers;
}

public RunnerBuilder minPeers(final int minPeers) {
this.minPeers = minPeers;
return this;
}
}
22 changes: 21 additions & 1 deletion besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -424,11 +424,13 @@ static class P2PDiscoveryOptionGroup {
private final Integer p2pPort = EnodeURLImpl.DEFAULT_LISTENING_PORT;

@Option(
names = {"--max-peers"},
names = {"--max-peers", "--p2p-peer-upper-bound"},
paramLabel = MANDATORY_INTEGER_FORMAT_HELP,
description = "Maximum P2P connections that can be established (default: ${DEFAULT-VALUE})")
private final Integer maxPeers = DEFAULT_MAX_PEERS;

private int minPeers;

@Option(
names = {"--remote-connections-limit-enabled"},
description =
Expand Down Expand Up @@ -1602,6 +1604,7 @@ private Runner buildRunner() {
p2PDiscoveryOptionGroup.peerDiscoveryEnabled,
ethNetworkConfig,
p2PDiscoveryOptionGroup.maxPeers,
p2PDiscoveryOptionGroup.minPeers,
p2PDiscoveryOptionGroup.p2pHost,
p2PDiscoveryOptionGroup.p2pInterface,
p2PDiscoveryOptionGroup.p2pPort,
Expand Down Expand Up @@ -1724,6 +1727,7 @@ private void validateOptions() {
validateNatParams();
validateNetStatsParams();
validateDnsOptionsParams();
ensureValidPeerBoundParams();
validateRpcOptionsParams();
p2pTLSConfigOptions.checkP2PTLSOptionsDependencies(logger, commandLine);
pkiBlockCreationOptions.checkPkiBlockCreationOptionsDependencies(logger, commandLine);
Expand Down Expand Up @@ -1796,6 +1800,20 @@ private void validateDnsOptionsParams() {
}
}

private void ensureValidPeerBoundParams() {
final int min = unstableNetworkingOptions.toDomainObject().getRlpx().getPeerLowerBound();
final int max = p2PDiscoveryOptionGroup.maxPeers;
if (min > max) {
logger.warn("`--Xp2p-peer-lower-bound` " + min + " must not exceed --max-peers " + max);
macfarla marked this conversation as resolved.
Show resolved Hide resolved
// modify the --X lower-bound value if it's not valid, we don't want unstable defaults
// breaking things
unstableNetworkingOptions.toDomainObject().getRlpx().setPeerLowerBound(max);
p2PDiscoveryOptionGroup.minPeers = max;
} else {
p2PDiscoveryOptionGroup.minPeers = min;
}
}

public void validateRpcOptionsParams() {
Predicate<String> configuredApis =
apiName ->
Expand Down Expand Up @@ -2774,6 +2792,7 @@ private Runner synchronize(
final boolean peerDiscoveryEnabled,
final EthNetworkConfig ethNetworkConfig,
final int maxPeers,
final int minPeers,
final String p2pAdvertisedHost,
final String p2pListenInterface,
final int p2pListenPort,
Expand Down Expand Up @@ -2808,6 +2827,7 @@ private Runner synchronize(
.p2pListenInterface(p2pListenInterface)
.p2pListenPort(p2pListenPort)
.maxPeers(maxPeers)
.minPeers(minPeers)
.limitRemoteWireConnectionsEnabled(
p2PDiscoveryOptionGroup.isLimitRemoteWireConnectionsEnabled)
.fractionRemoteConnectionsAllowed(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ public interface DefaultCommandValues {
NatMethod DEFAULT_NAT_METHOD = NatMethod.AUTO;
JwtAlgorithm DEFAULT_JWT_ALGORITHM = JwtAlgorithm.RS256;
int FAST_SYNC_MIN_PEER_COUNT = 5;
int DEFAULT_MAX_PEERS = 25;
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.

int DEFAULT_HTTP_MAX_CONNECTIONS = 80;
int DEFAULT_WS_MAX_CONNECTIONS = 80;
int DEFAULT_WS_MAX_FRAME_SIZE = 1024 * 1024;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.hyperledger.besu.cli.options.unstable;

import org.hyperledger.besu.cli.DefaultCommandValues;
import org.hyperledger.besu.cli.options.CLIOptions;
import org.hyperledger.besu.cli.options.OptionParser;
import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration;
Expand All @@ -31,6 +32,7 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
"--Xp2p-check-maintained-connections-frequency";
private final String DNS_DISCOVERY_SERVER_OVERRIDE_FLAG = "--Xp2p-dns-discovery-server";
private final String DISCOVERY_PROTOCOL_V5_ENABLED = "--Xv5-discovery-enabled";
private final String P2P_PEER_LOWER_BOUND_FLAG = "--Xp2p-peer-lower-bound";

@CommandLine.Option(
names = INITIATE_CONNECTIONS_FREQUENCY_FLAG,
Expand Down Expand Up @@ -66,6 +68,13 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
description = "Whether to enable P2P Discovery Protocol v5 (default: ${DEFAULT-VALUE})")
private final Boolean isPeerDiscoveryV5Enabled = false;

@CommandLine.Option(
hidden = true,
names = {P2P_PEER_LOWER_BOUND_FLAG},
description =
"Lower bound on the target number of P2P connections (default: ${DEFAULT-VALUE})")
private final Integer peerLowerBound = DefaultCommandValues.DEFAULT_P2P_PEER_LOWER_BOUND;

private NetworkingOptions() {}

public static NetworkingOptions create() {
Expand All @@ -90,6 +99,7 @@ public NetworkingConfiguration toDomainObject() {
config.setInitiateConnectionsFrequency(initiateConnectionsFrequencySec);
config.setDnsDiscoveryServerOverride(dnsDiscoveryServerOverride);
config.getDiscovery().setDiscoveryV5Enabled(isPeerDiscoveryV5Enabled);
config.getRlpx().setPeerLowerBound(peerLowerBound);
return config;
}

Expand Down
45 changes: 43 additions & 2 deletions besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.

💪


final ArgumentCaptor<EthNetworkConfig> ethNetworkArg =
ArgumentCaptor.forClass(EthNetworkConfig.class);
Expand Down Expand Up @@ -865,7 +865,7 @@ public void noOverrideDefaultValuesIfKeyIsNotPresentInConfigFile() throws IOExce
MAINNET_DISCOVERY_URL));
verify(mockRunnerBuilder).p2pAdvertisedHost(eq("127.0.0.1"));
verify(mockRunnerBuilder).p2pListenPort(eq(30303));
verify(mockRunnerBuilder).maxPeers(eq(25));
verify(mockRunnerBuilder).maxPeers(eq(100));
verify(mockRunnerBuilder).limitRemoteWireConnectionsEnabled(eq(true));
verify(mockRunnerBuilder).fractionRemoteConnectionsAllowed(eq(0.6f));
verify(mockRunnerBuilder).jsonRpcConfiguration(eq(jsonRpcConfiguration));
Expand Down Expand Up @@ -1542,6 +1542,47 @@ public void maxpeersOptionMustBeUsed() {
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void p2pPeerUpperBound_without_p2pPeerLowerBound_shouldSetLowerBoundEqualToUpperBound() {

final int maxPeers = 23;
parseCommand("--p2p-peer-upper-bound", String.valueOf(maxPeers));

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();

verify(mockRunnerBuilder).maxPeers(intArgumentCaptor.capture());
assertThat(intArgumentCaptor.getValue()).isEqualTo(maxPeers);

verify(mockRunnerBuilder).minPeers(intArgumentCaptor.capture());
assertThat(intArgumentCaptor.getValue()).isEqualTo(maxPeers);

verify(mockRunnerBuilder).build();
}

@Test
public void maxpeersSet_p2pPeerLowerBoundSet() {

final int maxPeers = 123;
final int minPeers = 66;
parseCommand(
"--max-peers",
String.valueOf(maxPeers),
"--Xp2p-peer-lower-bound",
String.valueOf(minPeers));

verify(mockRunnerBuilder).maxPeers(intArgumentCaptor.capture());
assertThat(intArgumentCaptor.getValue()).isEqualTo(maxPeers);

verify(mockRunnerBuilder).minPeers(intArgumentCaptor.capture());
assertThat(intArgumentCaptor.getValue()).isEqualTo(minPeers);

verify(mockRunnerBuilder).build();

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void remoteConnectionsPercentageOptionMustBeUsed() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ public void initMocks() throws Exception {
when(mockRunnerBuilder.p2pListenInterface(anyString())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.permissioningConfiguration(any())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.maxPeers(anyInt())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.minPeers(anyInt())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.limitRemoteWireConnectionsEnabled(anyBoolean()))
.thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.fractionRemoteConnectionsAllowed(anyFloat()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
import static org.hyperledger.besu.cli.DefaultCommandValues.DEFAULT_P2P_PEER_LOWER_BOUND;

import org.hyperledger.besu.cli.options.unstable.NetworkingOptions;
import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration;
Expand Down Expand Up @@ -125,6 +126,32 @@ public void checkDiscoveryV5Enabled_isNotSet() {
assertThat(commandOutput.toString(UTF_8)).isEmpty();
}

@Test
public void checkP2pPeerLowerBound_isSet() {
final int lowerBound = 13;
final TestBesuCommand cmd = parseCommand("--Xp2p-peer-lower-bound", String.valueOf(lowerBound));

final NetworkingOptions options = cmd.getNetworkingOptions();
final NetworkingConfiguration networkingConfig = options.toDomainObject();
assertThat(networkingConfig.getRlpx().getPeerLowerBound()).isEqualTo(lowerBound);

assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
assertThat(commandOutput.toString(UTF_8)).isEmpty();
}

@Test
public void checkP2pPeerLowerBound_isNotSet() {
final TestBesuCommand cmd = parseCommand();

final NetworkingOptions options = cmd.getNetworkingOptions();
final NetworkingConfiguration networkingConfig = options.toDomainObject();
assertThat(networkingConfig.getRlpx().getPeerLowerBound())
.isEqualTo(DEFAULT_P2P_PEER_LOWER_BOUND);

assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
assertThat(commandOutput.toString(UTF_8)).isEmpty();
}

@Override
NetworkingConfiguration createDefaultDomainObject() {
return NetworkingConfiguration.create();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ public void processMessage(final Capability cap, final Message message) {

if (this.mergePeerFilter.isPresent()) {
if (this.mergePeerFilter.get().disconnectIfGossipingBlocks(message, ethPeer)) {
LOG.info("Post-merge disconnect: peer still gossiping blocks {}", ethPeer);
macfarla marked this conversation as resolved.
Show resolved Hide resolved
handleDisconnect(ethPeer.getConnection(), DisconnectReason.SUBPROTOCOL_TRIGGERED, false);
return;
}
Expand Down Expand Up @@ -389,6 +390,7 @@ private void handleStatusMessage(final EthPeer peer, final MessageData data) {
peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
} else if (mergePeerFilter.isPresent()
&& mergePeerFilter.get().disconnectIfPoW(status, peer)) {
LOG.info("Post-merge disconnect: peer still PoW {}", peer);
macfarla marked this conversation as resolved.
Show resolved Hide resolved
handleDisconnect(peer.getConnection(), DisconnectReason.SUBPROTOCOL_TRIGGERED, false);
} else {
LOG.debug("Received status message from {}: {}", peer, status);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public class RlpxConfiguration {
private String clientId = "TestClient/1.0.0";
private String bindHost = NetworkUtility.INADDR_ANY;
private int bindPort = 30303;
private int maxPeers = 25;
private int peerUpperBound = 100;
private int peerLowerBound = 64;
private boolean limitRemoteWireConnectionsEnabled = false;
private float fractionRemoteWireConnectionsAllowed = DEFAULT_FRACTION_REMOTE_CONNECTIONS_ALLOWED;
private List<SubProtocol> supportedProtocols = Collections.emptyList();
Expand Down Expand Up @@ -70,13 +71,13 @@ public RlpxConfiguration setBindPort(final int bindPort) {
return this;
}

public RlpxConfiguration setMaxPeers(final int peers) {
maxPeers = peers;
public RlpxConfiguration setPeerUpperBound(final int peers) {
peerUpperBound = peers;
return this;
}

public int getMaxPeers() {
return maxPeers;
public int getPeerUpperBound() {
return peerUpperBound;
}

public String getClientId() {
Expand Down Expand Up @@ -105,10 +106,10 @@ public RlpxConfiguration setFractionRemoteWireConnectionsAllowed(

public int getMaxRemotelyInitiatedConnections() {
if (!limitRemoteWireConnectionsEnabled) {
return maxPeers;
return peerUpperBound;
}

return (int) Math.floor(maxPeers * fractionRemoteWireConnectionsAllowed);
return (int) Math.floor(peerUpperBound * fractionRemoteWireConnectionsAllowed);
}

@Override
Expand Down Expand Up @@ -136,4 +137,13 @@ public String toString() {
sb.append('}');
return sb.toString();
}

public RlpxConfiguration setPeerLowerBound(final int peers) {
peerLowerBound = peers;
return this;
}

public int getPeerLowerBound() {
return peerLowerBound;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,10 @@ public class DefaultP2PNetwork implements P2PNetwork {
this.nodeId = nodeKey.getPublicKey().getEncodedBytes();
this.peerPermissions = peerPermissions;

final int maxPeers = config.getRlpx().getMaxPeers();
peerDiscoveryAgent.addPeerRequirement(() -> rlpxAgent.getConnectionCount() >= maxPeers);
// set the requirement here that the number of peers be greater than the lower bound
final int peerLowerBound = config.getRlpx().getPeerLowerBound();
LOG.info("setting peerLowerBound {}", peerLowerBound);
macfarla marked this conversation as resolved.
Show resolved Hide resolved
peerDiscoveryAgent.addPeerRequirement(() -> rlpxAgent.getConnectionCount() >= peerLowerBound);
subscribeDisconnect(reputationManager);
}

Expand Down
Loading