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.

Sign up for GitHub

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

Jump to bottom

feat: limit relay connections below max conns #1813

Merged
merged 4 commits into from
Jul 4, 2023
Merged

feat: limit relay connections below max conns #1813

merged 4 commits into from
Jul 4, 2023

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Jun 21, 2023 •
edited
Loading

Description

  • Prunes incoming relay connections exceeding maxInRelayPeers
  • Does not attempt to connect to more than maxOutRelayPeers
  • This leaves maxConnections-maxInRelayPeers-maxOutRelayPeers slots for service peers
  • Amount of maxRelayPeers defaults to 80% of the maxConnections

image
ref

closes #1567

@alrevuelta alrevuelta mentioned this pull request Jun 22, 2023
Merged
@alrevuelta alrevuelta force-pushed the limit-relay-conns branch 2 times, most recently from 4e2f9cc to 864459f Compare June 30, 2023 07:27
@alrevuelta alrevuelta mentioned this pull request Jun 30, 2023
Closed
17 tasks
@alrevuelta alrevuelta marked this pull request as ready for review June 30, 2023 08:26
@alrevuelta alrevuelta requested review from Ivansete-status, jm-clius and SionoiS and removed request for Ivansete-status June 30, 2023 08:30
SionoiS
SionoiS approved these changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM expect the weird phrasing.

waku/v2/node/peer_manager/peer_manager.nim Outdated
var maxRelayPeersValue = 0
if maxRelayPeers.isSome():
if maxRelayPeers.get() > maxConnections:
error "Max number of relay peers can't be greater the max amount of connections",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
error "Max number of relay peers can't be greater the max amount of connections",
error "Max number of relay peers can't be greater than the max amount of connections",

phrasing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, all addressed c32672f

waku/v2/node/peer_manager/peer_manager.nim Outdated
error "Max number of relay peers can't be greater the max amount of connections",
maxConnections = maxConnections,
maxRelayPeers = maxRelayPeers.get()
raise newException(Defect, "Max number of relay peers can't be greater the max amount of connections")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise newException(Defect, "Max number of relay peers can't be greater the max amount of connections")
raise newException(Defect, "Max number of relay peers can't be greater than the max amount of connections")

phrasing

waku/v2/node/peer_manager/peer_manager.nim Outdated
raise newException(Defect, "Max number of relay peers can't be greater the max amount of connections")

if maxRelayPeers.get() == maxConnections:
warn "Max number of relay peers is equal to max amount of connections, peer wont be contribute to service peers",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
warn "Max number of relay peers is equal to max amount of connections, peer wont be contribute to service peers",
warn "Max number of relay peers is equal to max amount of connections, peer won't be contributing to service peers",

phrasing

jm-clius
jm-clius approved these changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@jm-clius jm-clius 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 this. Great to have a way to properly control number of relay vs service slots.

@jm-clius
Copy link
Contributor

jm-clius commented Jul 3, 2023

One thing I thought of after reviewing: it seems we only prune relay peers (which makes sense). However, nothing would therefore stop non-relay peers (i.e. client-only peers) from creating inbound connections occupying all slots? Or am I missing something?

@alrevuelta
Copy link
Contributor Author

One thing I thought of after reviewing: it seems we only prune relay peers (which makes sense). However, nothing would therefore stop non-relay peers (i.e. client-only peers) from creating inbound connections occupying all slots? Or am I missing something?

@jm-clius Indeed you are right. For this PR I was considering just relay protocol. Will followup with a PR addressing this.

@chaitanyaprem chaitanyaprem mentioned this pull request Jul 4, 2023
Closed
@alrevuelta alrevuelta merged commit 17b24cd into master Jul 4, 2023
@alrevuelta alrevuelta deleted the limit-relay-conns branch July 4, 2023 11:31
@chaitanyaprem chaitanyaprem mentioned this pull request Jul 6, 2023
Closed
7 tasks
@chaitanyaprem chaitanyaprem mentioned this pull request Aug 3, 2023
Merged
@chaitanyaprem chaitanyaprem mentioned this pull request Sep 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jm-clius jm-clius jm-clius approved these changes

@SionoiS SionoiS SionoiS approved these changes

@Ivansete-status Ivansete-status Awaiting requested review from Ivansete-status

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: add target amount of connections
3 participants
@alrevuelta @jm-clius @SionoiS