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

chore: stop connecting to out peers until target is reached #2823

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

gabrielmer
Copy link
Contributor

Description

We're currently attempting establish more out connections than our target. We are trying to connect 10 peers at a time, and we still dial 10 even if our target is less than 10.

When investigating #2780, I noticed that the issue started right after we tried to connect to more nodes than we could.

Making a small fix so we stick more precisely to our targets.

Changes

  • only dial to the peers necessary to reach outRelayPeersTarget

Copy link

github-actions bot commented Jun 18, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2823-rln-v2

Built from 9a53969

Copy link

github-actions bot commented Jun 18, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2823-rln-v1

Built from 9a53969

@Ivansete-status
Copy link
Collaborator

Thanks for that @gabrielmer !
However, there might be something that I don't get 100% because I was assuming that we already achieve that goal from the following line:

let numPeersToConnect = min(numPendingConnReqs, MaxParallelDials)

@gabrielmer
Copy link
Contributor Author

Thanks for that @gabrielmer ! However, there might be something that I don't get 100% because I was assuming that we already achieve that goal from the following line:

let numPeersToConnect = min(numPendingConnReqs, MaxParallelDials)

So before the fix that I added, numPendingConnReqs is initialized to outsideBackoffPeers.len, which is all the peers that we have saved and can connect to. But this number does not take into account our target, so if we have more possible peers than our target, we can currently overshoot.

That's why I'm fixing numPendingConnReqs to match exactly whatever is left for our target.

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the clarification! 💯 🥳

Thanks to this enhancement, I understand that the precondition controlled in the next point:

if outRelayPeers.len >= pm.outRelayPeersTarget:

...still remains at the end of the connectToRelayPeers proc. In other words, we make sure that the this condition is met: outRelayPeers.len < pm.outRelayPeersTarget

Great catch 🙌 !

@gabrielmer
Copy link
Contributor Author

Thanks to this enhancement, I understand that the precondition controlled in the next point:

if outRelayPeers.len >= pm.outRelayPeersTarget:

...still remains at the end of the connectToRelayPeers proc. In other words, we make sure that the this condition is met: outRelayPeers.len < pm.outRelayPeersTarget

Exactly! This was still controlled but it wasn't precise, we would overshoot most of the times and could dial up to 9 more peers than the target

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks ok.

@gabrielmer gabrielmer merged commit 41bc582 into master Jun 20, 2024
14 of 15 checks passed
@gabrielmer gabrielmer deleted the chore-establish-out-connections-until-target branch June 20, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants