feat(p2p): grow MaxPeers with # of subnets #1923
Conversation
|
Adding here this data @nkryuchkov collected about the committees per operator distribution in mainnet network, this should us as a reference for deciding on the parameters of this change: |
* wip * subnets instead of committees * fix
moshe-blox
changed the title
|
added new log for topic peers distribution: {"topic peers distribution","min":2,"median":5,"max":7,"dead_subnets":0,"unhealthy_subnets":1} |
cli/operator/node.go
Outdated
| start := time.Now() | ||
| myValidators := nodeStorage.ValidatorStore().OperatorValidators(operatorData.ID) | ||
| mySubnets := make(records.Subnets, networkcommons.SubnetsCount) | ||
| myActiveSubnets := 0 |
There was a problem hiding this comment.
Can just replace myActiveSubnets with len(mySubnets) ?
cli/operator/node.go
Outdated
| myActiveSubnets++ | ||
| } | ||
| } | ||
| idealMaxPeers := min(baseMaxPeers+peersPerSubnet*myActiveSubnets, maxPeersLimit) |
There was a problem hiding this comment.
Shouldn't idealMaxPeers be instead something like this ?
idealMaxPeers := min(max(baseMaxPeers, peersPerSubnet*myActiveSubnets), maxPeersLimit)If we have 52 active subnets baseMaxPeers+peersPerSubnet*myActiveSubnets would be = 60 + 52*3 which is way too high, no ?
There was a problem hiding this comment.
i tested with only 16 subnets and 60 peers is not enough (but 80-90 is), so instead we keep the minimum of 60 + add more for every subnet
There was a problem hiding this comment.
i tested with only 16 subnets and 60 peers is not enough (but 80-90 is), so instead we keep the minimum of 60 + add more for every subnet
ah, I see
| } | ||
| } | ||
|
|
||
| func (n *p2pNetwork) reportTopicPeers(logger *zap.Logger, name string) { | ||
| func (n *p2pNetwork) reportTopicPeers(logger *zap.Logger, name string) (peerCount int) { |
There was a problem hiding this comment.
It would probably be simpler to get rid of this func completely (inline it) because currently we have this measuring/reporting/logging code partially here, partially there
cli/operator/node.go
Outdated
| if cfg.P2pNetworkConfig.MaxPeers < idealMaxPeers { | ||
| logger.Warn("increasing MaxPeers to match the operator's subscribed subnets", | ||
| zap.Int("old_max_peers", cfg.P2pNetworkConfig.MaxPeers), | ||
| zap.Int("new_max_peers", idealMaxPeers), | ||
| zap.Int("subscribed_subnets", myActiveSubnets), | ||
| zap.Duration("took", time.Since(start)), | ||
| ) | ||
| cfg.P2pNetworkConfig.MaxPeers = idealMaxPeers | ||
| } |
There was a problem hiding this comment.
Is there a need to even have cfg.P2pNetworkConfig.MaxPeers value now if we are gonna overwrite it in most cases anyway ? Perhaps we want to remove it to keep it simple (otherwise later on we/somebody might wrongly think the MaxPeers value he sets in config file is the value that's gonna be used - which is not gonna be the case).
There was a problem hiding this comment.
this is a temporary solution and we wouldnt want to educate everyone about a new configuration change only to take it down very soon after
There was a problem hiding this comment.
although we could potentially rename it to TargetPeers and just leave it as is even if we end up reverting this PR
There was a problem hiding this comment.
@iurii-ssv i made some changes to make it more clear that MaxPeers is currently not the max
We have noticed public operators with a lot of subnets having hard time to fill peers into all their subnets, having connectivity issues at the end making them miss duties
Since some of them may have configured max peers to the current default value (60), changing the default config value is not enough. Therefore, we need some workaround like this PR
However, we'll need to come up with a more elegant solution later
This PR is meant to be an alternative (simpler) temporary solution in case we decide not to proceed with #1917 and #1919 right now