Restrict Dials From Discovery #14052
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nisdas
added
Ready For Review
…bs/geth-sharding into addSubnetDialBackoff
Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>
nalepae
reviewed
May 29, 2024
| func (s *Service) wantedPeerDials() int { | ||
| maxPeers := int(s.cfg.MaxPeers) | ||
|
|
||
| activePeers := len(s.Peers().Active()) |
There was a problem hiding this comment.
Would'nt be safer to convert activePeers to uint instead converting s.cfg.MaxPeers to int?
There was a problem hiding this comment.
The end result would be the same as these are all preconfigured values at startup. Any edge cases we hit here would require a user wildly misconfiguring their node.
nalepae
reviewed
May 29, 2024
nalepae
reviewed
May 29, 2024
nalepae
reviewed
May 29, 2024
| // Do not excessively perform lookups if we constantly receive non-viable peers. | ||
| if lookupCounter > backOffCounter { | ||
| lookupCounter = 0 | ||
| runtime.Gosched() |
There was a problem hiding this comment.
Is this call still useful with latest versions of golang?
There was a problem hiding this comment.
why wouldn't it be useful with current versions of golang ?
nalepae
reviewed
May 29, 2024
nalepae
approved these changes
May 30, 2024
What type of PR is this?
Bug Fix
What does this PR do? Why is it needed?
There have been longstanding reports of windows users randomly losing peers over the years, this had been chalked down to an improperly configured firewall, timeserver or antivirus. Unfortunately over the years as the network has grown along with more intensive requirements in peer search for nodes, this has continued to happen more. It appears that a recent windows update has exacerbated this problem by quite a bit. This PR tries to fix this once and for all:
For discovery, we have an aggressive peer search algorithm. This was very useful in finding peers quickly for a recently booted node. However windows as an OS doesn't like the aggressive discovery search too much and randomly decides to block outgoing discovery packets from prysm. This PR makes it less aggressive, so it will take longer for nodes to find their desired number of peers
Add a flag to restrict the maximum amount of concurrent dials(
-max-concurrent-dials). In the event users continue running into this, they can run with this flag at a smaller value(5,10, etc). This should stop triggering the OS at the cost of discovery being a lot slower along with possible reduced validator performance.Which issues(s) does this PR fix?
Fixes #14025 #13936 #13431 #8144
Other notes for review