chore: capping mechanism for relay and service connections #3184

darshankabariya · 2024-11-28T06:26:26Z

This PR prevents nodes from becoming isolated from the network by implementing a capping mechanism for service and relay peers. This ensures there is always room available for relay peers at any given time.

close #3163

github-actions · 2024-11-28T06:31:41Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3184

Built from a379b02

SionoiS

LGTM

waku/node/peer_manager/peer_manager.nim

gabrielmer

Looks great, thanks so much!

Added just a couple questions/comments

gabrielmer · 2024-12-10T11:29:11Z

waku/node/peer_manager/peer_manager.nim

@@ -993,6 +999,24 @@ proc new*(
    # Leave by default 20% of connections for service peers
    maxRelayPeersValue = maxConnections - (maxConnections div 5)

+  var maxServicePeersValue = 0
+  var d_low = 4


I think we shouldn't hardcode this and take it directly from WakuRelay

gabrielmer · 2024-12-10T11:31:40Z

waku/node/peer_manager/peer_manager.nim

+      # relay has higher priority then service peers
+  else:
+    maxServicePeersValue =
+      max(maxConnections - maxRelayPeersValue, maxConnections div 5)


what's the logic of this line? Why the maxConnections div 5?

My logic is to reserve 20% for service peers if the user does not provide maxService, as we discuss in nwaku pm we need to change whole logic

Ivansete-status

The PR doesn't compile snif snif :s
Re-thinking about this, I suggest doing the following:
We need to keep maxConnections config param because this needs to be passed to the nim-libp2p-Switch object.
On the other hand, I would only add an additional parameter, named protoConnWeights , a.k.a. proto-conn-weights, with the following logic:

If proto-conn-weights is not passed, then we apply the internal limits of 70% for relay, and the other 30% is evenly distributed for other protocols.

If proto-conn-weights=relay:80;services:20, we apply the internal limits of 80% for relay, and the other 20% is evenly distributed for other protocols.

We need to make sure the relay limit is bigger than DHigh ( we can create a
public Constant in waku_relay/protocol.nim )

nwaku/waku/waku_relay/protocol.nim

Line 73 in 09f05fe

dHigh = 8,

wdyt @NagyZoltanPeter , @gabrielmer , @darshankabariya ?

Ivansete-status · 2024-11-29T09:49:47Z

waku/node/peer_manager/peer_manager.nim

@@ -993,6 +995,20 @@ proc new*(
    # Leave by default 20% of connections for service peers
    maxRelayPeersValue = maxConnections - (maxConnections div 5)

+  var maxServicePeersValue = 0
+  var d_low = 4


We will need to use the d_low value extracted from waku_relay. Perhaps defining a const in waku_relay and use that value

darshankabariya · 2024-12-12T21:38:49Z

The PR doesn't compile snif snif

My bad, I pushed the code without a local build. I’ll update this PR until we finalize it.

darshankabariya · 2024-12-12T21:56:00Z

The PR doesn't compile snif snif :s Re-thinking about this, I suggest doing the following: We need to keep maxConnections config param because this needs to be passed to the nim-libp2p-Switch object. On the other hand, I would only add an additional parameter, named protoConnWeights , a.k.a. proto-conn-weights, with the following logic:

If proto-conn-weights is not passed, then we apply the internal limits of 70% for relay, and the other 30% is evenly distributed for other protocols.

If proto-conn-weights=relay:80;services:20, we apply the internal limits of 80% for relay, and the other 20% is evenly distributed for other protocols.

We need to make sure the relay limit is bigger than DHigh ( we can create a public Constant in waku_relay/protocol.nim )

nwaku/waku/waku_relay/protocol.nim

Line 73 in 09f05fe

dHigh = 8,

wdyt @NagyZoltanPeter , @gabrielmer , @darshankabariya ?

At the moment, we have parameters like maxConnections, maxRelayPeers, maxServicePeers, and another one called protoConnWeight. This can make things more confusing, as users need to ensure that maxConnections is never greater than the sum of maxRelayPeers and maxServicePeers. Additionally, aligning proto-conn-weights with maxRelayPeers and maxServicePeers adds another layer of complexity.

Instead, I believe users generally don’t think in terms of absolute numbers for services and relays. From a psychological perspective, they might find it more intuitive to think in percentages—e.g., 80% for services, 20% for relays.

My suggestion is to simplify this structure. We should keep maxConnections bcz ( represent hardware capacity ? right ) , which users are familiar with, and deprecate maxRelayPeers and maxServicePeers. Instead, we can ask users to specify the percentage they want to allocate for relays and services. This feels more straightforward and user-friendly. maxConnection, relayWeight and serviceWeight will be deadly combination. the user to easily convey their priorities.

If users don’t provide this percentage, we could default to a ratio, as Ivan suggested. I believe this would simplify the experience while maintaining flexibility.

@gabrielmer @NagyZoltanPeter @Ivansete-status @SionoiS

SionoiS · 2024-12-13T12:39:30Z

I like your idea! IMO maxConnection and relayToServiceRatio (float) would be simpler.

I feel we still need hardcoded minimum still so that you can't, by mistake brick your node.

gabrielmer · 2024-12-13T14:39:08Z

Same opinion here, having 2 params, a maximum and a ratio to know how many to allocate for each should make things simple yet customizable :)

darshankabariya · 2024-12-14T23:23:44Z

I like your idea! IMO maxConnection and relayToServiceRatio (float) would be simpler.

I feel we still need hardcoded minimum still so that you can't, by mistake brick your node.

Same opinion here, having 2 params, a maximum and a ratio to know how many to allocate for each should make things simple yet customizable :)

Thanks @gabrielmer @SionoiS for reply ! i also like suggestion.

If this approach looks good to you, I can begin the implementation. Just to confirm, we're only discussing the deprecation of the external configuration parameter, correct? I believe we still need to maxRelayPeer and maxServicePeer within the peerManager.

Regarding your thoughts on the ratio, I have an idea: instead of using a float, we could represent it as a string, like 0.33 becoming 40:60 or 0.43 as 30:70. it's just need calculator 😂for some ratio, Let me know what you guys think, and I’ll implement accordingly.

gabrielmer · 2024-12-16T09:36:54Z

If this approach looks good to you, I can begin the implementation. Just to confirm, we're only discussing the deprecation of the external configuration parameter, correct? I believe we still need to maxRelayPeer and maxServicePeer within the peerManager.

Regarding your thoughts on the ratio, I have an idea: instead of using a float, we could represent it as a string, like 0.33 becoming 40:60 or 0.43 as 30:70. it's just need calculator 😂for some ratio, Let me know what you guys think, and I’ll implement accordingly.

Yes! That would only be for the external configuration, we would then translate it into maxRelayPeers and maxServicePeers when configuring the Peer Manager

Regarding the format, I'm pretty neutral about it - no strong opinions. Would like to hear what others think :))

SionoiS · 2024-12-16T12:29:39Z

Regarding your thoughts on the ratio, I have an idea: instead of using a float, we could represent it as a string, like 0.33 becoming 40:60 or 0.43 as 30:70. it's just need calculator 😂for some ratio, Let me know what you guys think, and I’ll implement accordingly.

If that's simpler go for it! No strong opinion here either.

darshankabariya · 2025-01-02T10:23:59Z

Just leaving a comment to inform, this PR is ready to be merged. Thank you!

Ivansete-status

Good work! I think we are getting even closer ;P
Kindly review my comments and let me know if something isn't clear or I'm missing anything
Thanks again 🙌 !

Ivansete-status · 2025-01-02T16:49:00Z

waku/common/utils/parse_size_units.nim

+
+    ok((float(relayRatio / total), float(serviceRatio / total)))
+  except Exception:
+    err("Invalid number format in ratio")


Suggested change

err("Invalid number format in ratio")

return err("Invalid number format in ratio: " & getCurrentExceptionMsg())

Ivansete-status · 2025-01-02T16:50:17Z

waku/common/utils/parse_size_units.nim

+    if total == 0:
+      return err("Total ratio cannot be zero")
+
+    ok((float(relayRatio / total), float(serviceRatio / total)))


nitpick comment: I think is better to always use the return keyword

Suggested change

ok((float(relayRatio / total), float(serviceRatio / total)))

return ok((float(relayRatio / total), float(serviceRatio / total)))

Ivansete-status · 2025-01-02T16:52:01Z

waku/common/utils/parse_size_units.nim

@@ -49,3 +49,24 @@ proc parseCorrectMsgSize*(input: string): uint64 =
  let ret = parseMsgSize(input).valueOr:
    return 0
  return ret
+
+proc parseRelayServiceRatio*(ratio: string): Result[(float, float), string] =


We need to have a comment to explain the parameter and the returning values

Ivansete-status · 2025-01-02T16:52:39Z

waku/common/utils/parse_size_units.nim

+  try:
+    let elements = ratio.split(":")
+    if elements.len != 2:
+      return err("Invalid format for relay ratio expected relay:service")


Suggested change

return err("Invalid format for relay ratio expected relay:service")

return err("Invalid format for relay ratio expected relay:service, found: " & ratio)

Ivansete-status · 2025-01-02T16:53:10Z

waku/common/utils/parse_size_units.nim

+      serviceRatio = parseInt(elements[1])
+
+    if relayRatio < 0 or serviceRatio < 0:
+      return err("Relay and service ratios must be non-negative integers")


Suggested change

return err("Relay and service ratios must be non-negative integers")

return err("Relay and service ratios must be non-negative integers. Found this: " & ratio)

Ivansete-status · 2025-01-02T17:02:41Z

waku/factory/builder.nim

+  let relayServiceRatio = parseRelayServiceRatio(relayServiceRatio)
+  if relayServiceRatio.isErr:
+    warn "Invalid relay service ratio", ratio = relayServiceRatio
+    return
+
+  let (relayRatio, serviceRatio) = relayServiceRatio.get


Suggested change

let relayServiceRatio = parseRelayServiceRatio(relayServiceRatio)

if relayServiceRatio.isErr:

warn "Invalid relay service ratio", ratio = relayServiceRatio

return

let (relayRatio, serviceRatio) = relayServiceRatio.get

let (relayRatio, serviceRatio) = parseRelayServiceRatio(relayServiceRatio).valueOr:

error "Invalid relay service ratio", ratio = relayServiceRatio, error = error

return ## we should stop the execution in this point but for further PR's

Ivansete-status · 2025-01-02T17:04:45Z

waku/node/peer_manager/peer_manager.nim

+  var relayPeers = int(ceil(float(maxConnections) * relayRatio))
+  var servicePeers = int(floor(float(maxConnections) * serviceRatio))
+
+  if maxRelayPeers.isSome() or maxServicePeers.isSome():


Maybe not needed this if statement?

Ivansete-status · 2025-01-02T17:05:48Z

waku/node/peer_manager/peer_manager.nim

+  if relayPeers + servicePeers > maxConnections:
+    warn "Max number of connections can't be greater than maxRelayPeers + maxServicePeers",
+      maxConnections = maxConnections,
+      maxRelayPeers = relayPeers,
+      maxServicePeers = servicePeers


This block shouldn't be needed because we should validate that on startup

Ivansete-status · 2025-01-02T17:15:37Z

waku/node/peer_manager/peer_manager.nim

+      maxRelayPeers = relayPeers,
+      maxServicePeers = servicePeers
+
+  let minRelayPeers = 8


The value is correct nowadays:)
However, instead of using this hard-coded value, we should extract this value from the dHigh configured in WakuRelay.

Then, we should have something like:

Suggested change

let minRelayPeers = 8

let minRelayPeers = WakuRelay.getDHigh()

And, in waku_relay/protocol.nim, we should create the following simple proc that exposes the dHigh configured value:

proc getDHigh*(T: type WakuRelay): int = return GossipsubParameters.dHigh

Ivansete-status · 2025-01-02T17:17:23Z

waku/node/peer_manager/peer_manager.nim

+  if relayPeers < minRelayPeers:
+    let newMaxConnections = int(float(minRelayPeers) / relayRatio)
+    warn "fullfill the minimum criteria you need to increase the max number of connections",
+      newMaxConnections = newMaxConnections


I think this validation should be made earlier and, in case not configured properly, immediately stop the execution.

I think this validation should be made earlier and, in case not configured properly, immediately stop the execution.

Thanks for all the comments! I've tried to address all of them all but instead of replying to each one individually (I’m just afraid it will scatter our discussion.), bcz I agree with most of your comments and have updated the PR accordingly.

However, regarding this suggestion:

I couldn't find a way to validate it earlier. While it's possible to add the validation in the parse function, doing so would require the maxConnection value within that function, which could overcomplicate the logic and violate the single responsibility principle.

Thank you very much !

github-actions bot assigned darshankabariya Nov 28, 2024

darshankabariya changed the title ~~chore: Capping mechanism for relay and service Connections~~ chore: capping mechanism for relay and service connections Nov 28, 2024

darshankabariya marked this pull request as draft December 3, 2024 10:45

darshankabariya force-pushed the capping_mechanism branch from 2e420e4 to fa97a4f Compare December 4, 2024 11:57

darshankabariya requested review from gabrielmer, Ivansete-status, NagyZoltanPeter and SionoiS December 9, 2024 14:25

darshankabariya marked this pull request as ready for review December 9, 2024 15:00

SionoiS approved these changes Dec 9, 2024

View reviewed changes

waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved

waku/node/peer_manager/peer_manager.nim Show resolved Hide resolved

gabrielmer reviewed Dec 10, 2024

View reviewed changes

Ivansete-status reviewed Dec 12, 2024

View reviewed changes

darshankabariya removed the request for review from NagyZoltanPeter December 12, 2024 21:37

darshankabariya marked this pull request as draft December 12, 2024 21:37

darshankabariya closed this Dec 15, 2024

darshankabariya force-pushed the capping_mechanism branch from 02fd08c to ab0c1d4 Compare December 15, 2024 16:29

darshankabariya reopened this Dec 15, 2024

darshankabariya marked this pull request as ready for review December 16, 2024 08:15

darshankabariya closed this Dec 16, 2024

darshankabariya force-pushed the capping_mechanism branch from d5a12ef to 049fbea Compare December 16, 2024 19:19

darshankabariya reopened this Dec 16, 2024

darshankabariya requested a review from Ivansete-status January 1, 2025 07:17

darshankabariya requested review from gabrielmer and SionoiS January 1, 2025 07:17

Ivansete-status reviewed Jan 2, 2025

View reviewed changes

darshankabariya force-pushed the capping_mechanism branch from eb617de to 36f4473 Compare January 3, 2025 06:13

darshankabariya added 13 commits January 3, 2025 19:56

chore: testing

f60ddd1

chore: working condition

f4eb8af

chore: update network monitor

fd69c61

chore: lint issue

138e7bb

chore: string update

1c1db6c

chore: better exception

2ecb337

chore: better exception

a93f837

chore: better exception

802b516

chore: lint issue

de39b7a

chore: update according to suggestion

3bc8887

chore: update according to function change

76f5045

chore: update according reviews !

6e84050

chore: update according reviews !

dbf8a0a

darshankabariya force-pushed the capping_mechanism branch from a414c70 to dbf8a0a Compare January 3, 2025 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: capping mechanism for relay and service connections #3184

chore: capping mechanism for relay and service connections #3184

darshankabariya commented Nov 28, 2024

github-actions bot commented Nov 28, 2024 •

edited

Loading

SionoiS left a comment

gabrielmer left a comment

gabrielmer Dec 10, 2024

gabrielmer Dec 10, 2024

darshankabariya Dec 11, 2024

Ivansete-status left a comment •

edited

Loading

Ivansete-status Nov 29, 2024

darshankabariya commented Dec 12, 2024

darshankabariya commented Dec 12, 2024 •

edited

Loading

SionoiS commented Dec 13, 2024

gabrielmer commented Dec 13, 2024

darshankabariya commented Dec 14, 2024 •

edited

Loading

gabrielmer commented Dec 16, 2024

SionoiS commented Dec 16, 2024

darshankabariya commented Jan 2, 2025

Ivansete-status left a comment

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

Ivansete-status Jan 2, 2025

darshankabariya Jan 3, 2025 •

edited

Loading

	err("Invalid number format in ratio")
	return err("Invalid number format in ratio: " & getCurrentExceptionMsg())

	ok((float(relayRatio / total), float(serviceRatio / total)))
	return ok((float(relayRatio / total), float(serviceRatio / total)))

	return err("Invalid format for relay ratio expected relay:service")
	return err("Invalid format for relay ratio expected relay:service, found: " & ratio)

	return err("Relay and service ratios must be non-negative integers")
	return err("Relay and service ratios must be non-negative integers. Found this: " & ratio)

	let minRelayPeers = 8
	let minRelayPeers = WakuRelay.getDHigh()

chore: capping mechanism for relay and service connections #3184

Are you sure you want to change the base?

chore: capping mechanism for relay and service connections #3184

Conversation

darshankabariya commented Nov 28, 2024

github-actions bot commented Nov 28, 2024 • edited Loading

SionoiS left a comment

Choose a reason for hiding this comment

gabrielmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darshankabariya commented Dec 12, 2024

darshankabariya commented Dec 12, 2024 • edited Loading

SionoiS commented Dec 13, 2024

gabrielmer commented Dec 13, 2024

darshankabariya commented Dec 14, 2024 • edited Loading

gabrielmer commented Dec 16, 2024

SionoiS commented Dec 16, 2024

darshankabariya commented Jan 2, 2025

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darshankabariya Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 28, 2024 •

edited

Loading

Ivansete-status left a comment •

edited

Loading

darshankabariya commented Dec 12, 2024 •

edited

Loading

darshankabariya commented Dec 14, 2024 •

edited

Loading

darshankabariya Jan 3, 2025 •

edited

Loading