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: pruning excess in relay connections #2965

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

gabrielmer
Copy link
Contributor

Description

Pruning excess relay connections to respect our relay connection targets

Issue

closes #2929

Copy link

github-actions bot commented Aug 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2965

Built from 5be623b

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 it! 💯
As mentioned somewhere, we would need to check with DST and maybe manually deploy into waku.test or waku.sandbox and see how behaves the node.
The original purpose of the issue was to prevent relay occupying the whole libp2p available connections.

One possible option could be having 1/3 for in, 1/3 for out, and 1/3 for "others"

@gabrielmer
Copy link
Contributor Author

One possible option could be having 1/3 for in, 1/3 for out, and 1/3 for "others"

So we have max-relay-peers and max-connections parameters.

I understand that if we configure max-connections to be a bigger number than max-relay-peers then it's enough. Not sure if we need to change something? I think that it's just a matter on the config parameters that we use to run a node.

As mentioned somewhere, we would need to check with DST and maybe manually deploy into waku.test or waku.sandbox and see how behaves the node

I did test it in a normal node. What I was thinking is to merge it if tests pass, it will go through our regular regression testing and we will figure out if there's an issue. And if so, we open a specific issue for it and not leave it commented without any info. Although I think it shouldn't bring issues.

We can ask to go through explicit testing by DST before merging, but I think that in case this brings an issue, it will be thoroughly tested by all parties and we will know it before this actually gets released. Not sure if we should add this extra step or not.

LMK how you prefer to proceed :)

@gabrielmer
Copy link
Contributor Author

Oh nice, tests failed! So will start looking at that first :)

Turning the PR into draft

@gabrielmer gabrielmer marked this pull request as draft August 9, 2024 16:07
@gabrielmer gabrielmer marked this pull request as ready for review August 12, 2024 14:52
@gabrielmer
Copy link
Contributor Author

Well the CI was a false alarm, it failed on a non-related RLN test and then was never able to reproduce it. So it's ready for review :)

@AlbertoSoutullo ran a simulation with the image and there's no connectivity issues

@gabrielmer gabrielmer merged commit 54b5222 into master Aug 12, 2024
9 of 11 checks passed
@gabrielmer gabrielmer deleted the chore-properly-prunning-in-connections branch August 12, 2024 15:59
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.

chore: review max-relay-peers config attribute in wakunode2
3 participants