Fix peer selection so inbound connections cannot occupy all peer slots and prevent required outbound connections#10846
Conversation
| () -> | ||
| targetPeerCountRange.getMinimumRandomlySelectedPeerCount() | ||
| + peerSubnetSubscriptions.getSubscribersRequired()); | ||
| return peersBeingDropped.stream().toList(); |
There was a problem hiding this comment.
return List.copyOf(peersBeingDropped); ?
tbenr
left a comment
tbenr
left a comment
There was a problem hiding this comment.
some additional comments. Not sure i can approve yet :)
|
@tbenr addressed |
There was a problem hiding this comment.
two more feedbacks:
--Xp2p-minimum-randomly-selected-peer-count should at least not allow inconsistent values compared to upperbound.
Additionally: do we want to allow the edgecase where all connections should be outbound (essentially aloways disconnecting inbounds?) if not we may want to enforce some reasonable values.. but i don't have a strong feeling here.
The remote-cap calculation currently drops remotely initiated peers even when total peer count is well below the lower bound. For example with upper=35, minRandom=25, and subnetRequired=1, the remote limit is 9, so a node with only 18 peers and 10 remotely initiated peers still disconnects. Could we make remote-cap drops conditional on being at/near the upper bound, or otherwise account for current headroom, so inbound peers are not dropped while the node is already below target?
|
|
I've updated check along with my suggestion 8e63ff0 |
tbenr
left a comment
tbenr
left a comment
There was a problem hiding this comment.
LGTM. just a simplification option
| public int getRemotelyInitiatedPeersToDrop( | ||
| final int currentRemotelyInitiatedPeerCount, final int totalOutboundRequirement) { | ||
| final int currentRemotelyInitiatedPeerCount, | ||
| final int totalOutboundRequirement, | ||
| final int currentTotalPeerCount) { | ||
| final int remotelyInitiatedPeerLimit = Math.max(0, upperBound - totalOutboundRequirement); | ||
| return Math.max(0, currentRemotelyInitiatedPeerCount - remotelyInitiatedPeerLimit); | ||
| final int remotelyInitiatedPeersAboveLimit = | ||
| Math.max(0, currentRemotelyInitiatedPeerCount - remotelyInitiatedPeerLimit); | ||
| final int peersAboveLowerBound = Math.max(0, currentTotalPeerCount - lowerBound); | ||
| return Math.min(remotelyInitiatedPeersAboveLimit, peersAboveLowerBound); | ||
| } |
There was a problem hiding this comment.
this blows a my mind a bit.
an alternative:
public int getRemotelyInitiatedPeersToDrop(
final int currentRemotelyInitiatedPeerCount,
final int totalOutboundRequirement,
final int currentTotalPeerCount) {
final int remotelyInitiatedPeerLimit = Math.max(0, upperBound - totalOutboundRequirement);
final int maxPeersToDropWithoutGoingBelowLowerBound =
Math.max(0, currentTotalPeerCount - lowerBound);
return Math.clamp(
currentRemotelyInitiatedPeerCount - remotelyInitiatedPeerLimit,
0,
maxPeersToDropWithoutGoingBelowLowerBound);
}
2 participants
PR Description
minRandomlySelectedPeers+subnetSubscribersRequired.Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Note
Medium Risk
Changes live P2P connect/disconnect policy for the beacon node, which can affect peer counts and gossip reach; behavior is covered by new unit tests but impacts production networking.
Overview
Fixes Eth2 peer selection so inbound connections cannot fill the peer budget and block required outbound dials (minimum randomly selected peers plus attestation subnet subscribers).
Connect: Score-based capacity now subtracts
randomlySelectedPeersToAdd(not total random pool size), and the random pool count used for targets includes only locally initiated random peers.Disconnect: Computes a remote-peer cap from
upperBound - (minRandomPeers + subnet requirement), drops remotely initiated peers first (by score), then fills remaining over-cap drops via the usual path (demote locally initiated random peers to score-based, then lowest scores). Inbound random peers are not protected like outbound random peers.Config: Default minimum random peer count rises from 20% to 30% of the peer lower bound;
minRandomlySelectedPeersis validated againstmaxPeers.Reviewed by Cursor Bugbot for commit 41bb431. Bugbot is set up for automated code reviews on this repo. Configure here.