-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
p2p: peer store and dialing changes #8737
p2p: peer store and dialing changes #8737
Conversation
Do we need the |
internal/p2p/peermanager.go
Outdated
// sort peers who our most recent dialing attempt was | ||
// successful ahead of peers with recent dialing | ||
// failures | ||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Score already incorporates this logic by subtracting one for every failed dial. Peers who have previously failed will naturally be scored lower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it doesn't increase for successful dials, and "I just dialed you and succeeded after failing for a while" is a problem.
I also think that given that the small range of possible scores (before this) we end up not really capturing (meaningfully) "this peer is real"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to increase for successful dials when we reset the counter after being successful. That should be enough to sufficiently offset your score
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is commented out and we can revisit it later.
Totally unused, removed. |
// MaxOutgoingConnections specifies how many outgoing | ||
// connections. It must be lower than MaxConnected. If it is | ||
// 0, then all connections can be outgoing. | ||
MaxOutgoingConnections uint16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think that does make sense.
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
// peer. | ||
|
||
// nolint:gosec // G404: Use of weak random number generator | ||
if numAddresses <= int(limit) || rand.Intn(totalScore+1) <= scores[peer.ID]+1 || rand.Intn((idx+1)*10) <= idx+1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This coin flip is:
Pick a random number between 0 and totalScore +1
If this value is less than the score of the peer, select the peer.
I think we'll hit maxAttempts
a lot doing it this way since most peers will have small scores. Either we need maxAttempts
to grow with the limit
or just switch to the simpler coinflip logic before. I'm fine with the simpler coinflip since I'd prefer this logic to be as dead simple as possible.
Perhaps my suggestion was too complex for now. The risk that this adds is frequently gossiping too few peers and slowing down PEX. I'd really prefer to keep any and all risks from complexity as low as possible if it means definitely shipping an improvement. To that end, I'm ultimately fine with the initial:
pick top limit * 2
shuffle
reslice to be of size limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we add 1 to both sides, (which we have to do to avoid passing 0 to rand.Intn) the +1 isn't notable.
The extra 10% of flip, I think helps reduce the max attempts case. Anyway, I've made it == to limit, which means we'd have to get < 1 address per iteration. (and the addedLastIteration check will let us continue past that as long as we make progress in each iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do 2 * limit? I think it's quite likely we'll get less than 1 per iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure!
(cherry picked from commit 9e5b137)
Closes: #8768
This needs some testing or validation, and maybe to be split into a
few batches, but I wanted to collect a number of changes that we'd
been discussing into one branch to facilitate testing and conversation.
The high level:
Questions: