fix: handle rln-relay-message-limit #2867

darshankabariya · 2024-07-02T13:16:28Z

rymnc · 2024-07-02T13:17:14Z

waku/waku_rln_relay/rln_relay.nim

@@ -435,19 +434,20 @@ proc mount(
  (await groupManager.startGroupSync()).isOkOr:
    return err("could not start the group sync: " & $error)

+  if (conf.rlnRelayUserMessageLimit > 20):


it should be 100 now, as per parameters decided by the waku research team :)

Thanks for the review, @rymnc. I will updates in the next commit.

mm, why enforcing this limit? the user is free to choose whatever rlnRelayUserMessageLimit they want. TheWakuNetwork sets this to a max of 100, but it shouldn't be enforced in the code.

for example, someone may want to deploy their own cluster, with their own contract with rlnRelayUserMessageLimit=9999.

Hi @AlejandroCabeza, Alvaro makes a good point about why we are enforcing a limit for messages.

mm, why enforcing this limit? the user is free to choose whatever rlnRelayUserMessageLimit they want. TheWakuNetwork sets this to a max of 100, but it shouldn't be enforced in the code.

for example, someone may want to deploy their own cluster, with their own contract with rlnRelayUserMessageLimit=9999.

agree - this limit should probably be enforced in another place -
OnchainGroupManager: fetch the MAX_MESSAGE_LIMIT from the contract and compare with the user provided rlnRelayUserMessageLimit

Hi @rymnc, I tried fetching MAX_MESSAGE_LIMIT from the contract based on your suggestion, but it failed in many cases. Upon debugging, I discovered that it returns 0 because it hasn't been initialized yet. Now, I've found another approach: there's an attribute called userMessageLimit in base_groupmanager, which I believe is a better option. What do you think ?

Earlier, as you mentioned that the message limit is set 20 for static. Can we fetch the on-chain limit from the contract?

Can we fetch the on-chain limit from the contract?

that is possible by calling MAX_MESSAGE_LIMIT()

I discovered that it returns 0

please share a code snippet?

I've just updated the PR. plz take a look at.

cc @rymnc

The associated test case failed with this implementation because the MAX_MESSAGE_LIMIT() API is returning 0. This is likely due to it not being initialized, which is why it returns zero.

github-actions · 2024-07-02T13:21:01Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2867

Built from 45de3af

gabrielmer

Thanks so much! Added just a nitpick change

gabrielmer · 2024-07-02T13:38:11Z

waku/waku_rln_relay/rln_relay.nim

@@ -435,19 +434,20 @@ proc mount(
  (await groupManager.startGroupSync()).isOkOr:
    return err("could not start the group sync: " & $error)

+  if (conf.rlnRelayUserMessageLimit > 100):
+    return err("relay message limit (rln) cannot be exceed 100 ")


Suggested change

return err("relay message limit (rln) cannot be exceed 100 ")

return err("relay message limit (rln) cannot exceed 100")

gabrielmer · 2024-07-02T13:38:40Z

tests/waku_rln_relay/test_wakunode_rln_relay.nim

+    await node.mountRlnRelay(wakuRlnConfig)
+
+    check:
+      $error == "relay message limit (rln) cannot be exceed 100 "


Suggested change

$error == "relay message limit (rln) cannot be exceed 100 "

$error == "relay message limit (rln) cannot exceed 100"

rymnc · 2024-07-05T08:06:22Z

tests/waku_rln_relay/test_wakunode_rln_relay.nim

@@ -595,4 +595,4 @@ procSuite "WakuNode - RLN relay":
      await node.mountRlnRelay(wakuRlnConfig)
    except CatchableError as e:
      check e.msg ==
-        "failed to mount WakuRlnRelay: relay message limit (rln) cannot be exceed 100"
+        "failed to mount WakuRlnRelay: relay-message-limit can't be exceed then user-message-limit set by contract"


this file uses the static mode of operation, so there is no point checking the user message limit from the contract

This test should actually go in the tests/waku_rln_relay/test_rln_group_manager_onchain.nim file

rymnc · 2024-07-05T08:07:43Z

tests/waku_rln_relay/test_wakunode_rln_relay.nim

+
+    # mount rlnrelay in off-chain mode
+    let wakuRlnConfig = WakuRlnConfig(
+      rlnRelayDynamic: false,


notice that it is not dynamic, aka not onchain, hence we cannot get the user message limit from the contract. I would suggest to use a different mechanism for the static group manager

rymnc · 2024-07-05T13:10:02Z

tests/waku_rln_relay/test_rln_group_manager_static.nim

+
+  asyncTest "test rln-relay-message-limit":
+    let
+      nodeKey = generateSecp256k1Key()
+      node = newTestWakuNode(nodeKey, parseIpAddress("0.0.0.0"), Port(0))
+
+    await node.mountRelay(@[DefaultPubsubTopic])
+
+    # mount rlnrelay in off-chain mode
+    let wakuRlnConfig = WakuRlnConfig(
+      rlnRelayDynamic: false,
+      rlnRelayCredIndex: some(0.uint),
+      rlnRelayUserMessageLimit: 111,
+      rlnEpochSizeSec: 0,
+      rlnRelayTreePath: genTempPath("rln_tree", "wakunode_9"),
+    )
+
+    try:
+      await node.mountRlnRelay(wakuRlnConfig)
+    except CatchableError as e:
+      check e.msg ==
+        "failed to mount WakuRlnRelay: rln-relay-message-limit can't be exceed then MAX_MESSAGE_LIMIT set by contract"


this wont work, because you're testing the static group manager here (there is no contract, and therefore, no enforceable max msg limit)

rymnc · 2024-07-05T13:10:25Z

waku/waku_rln_relay/rln_relay.nim

+  if (conf.rlnRelayUserMessageLimit > groupManager.rlnRelayMaxMessageLimit):
+    return err(
+      "rln-relay-message-limit can't be exceed then MAX_MESSAGE_LIMIT set by contract"
+    )


this can be removed

rymnc · 2024-07-22T16:17:48Z

tests/waku_rln_relay/test_rln_group_manager_onchain.nim

@@ -775,7 +779,27 @@ suite "Onchain group manager":
      isReady == true

    await manager.stop()
+
+  asyncTest "rln-relay-max-message-limit testing":


i am not sure if this test belongs here since we are just testing the behaviour of the onchain group manager in this module
can you add a todo comment mentioning it should be moved later?

rymnc

LGTM after addressing my comment

github-actions bot assigned darshankabariya Jul 2, 2024

rymnc reviewed Jul 2, 2024

View reviewed changes

darshankabariya requested review from rymnc, Ivansete-status, gabrielmer and NagyZoltanPeter July 2, 2024 13:21

gabrielmer approved these changes Jul 2, 2024

View reviewed changes

darshankabariya force-pushed the bug_2822 branch from 4b0dd05 to f78854e Compare July 3, 2024 05:30

alrevuelta mentioned this pull request Jul 3, 2024

bug: RLN_RELAY_MSG_LIMIT handling #2822

Closed

darshankabariya force-pushed the bug_2822 branch from 5caf92e to 1f2c952 Compare July 5, 2024 04:44

rymnc reviewed Jul 5, 2024

View reviewed changes

darshankabariya closed this Jul 7, 2024

darshankabariya force-pushed the bug_2822 branch from 3a96b31 to c93f804 Compare July 7, 2024 18:51

darshankabariya reopened this Jul 7, 2024

darshankabariya force-pushed the bug_2822 branch from 5c6b03b to 9c59059 Compare July 9, 2024 11:33

darshankabariya closed this Jul 19, 2024

darshankabariya force-pushed the bug_2822 branch from 9c59059 to ad25f43 Compare July 19, 2024 07:38

darshankabariya reopened this Jul 19, 2024

darshankabariya added 2 commits July 19, 2024 19:12

fix: initial implementation of snforcing rln limit

15b74f6

fix: update according to resolve build error

2acba9d

darshankabariya force-pushed the bug_2822 branch 2 times, most recently from 81e2949 to 2acba9d Compare July 22, 2024 12:43

fix: update the contract

5c595a6

rymnc reviewed Jul 22, 2024

View reviewed changes

rymnc approved these changes Jul 22, 2024

View reviewed changes

darshankabariya merged commit 8d107b0 into master Jul 22, 2024
8 of 10 checks passed

darshankabariya deleted the bug_2822 branch July 22, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle rln-relay-message-limit #2867

fix: handle rln-relay-message-limit #2867

darshankabariya commented Jul 2, 2024

rymnc Jul 2, 2024

darshankabariya Jul 2, 2024

alrevuelta Jul 2, 2024

darshankabariya Jul 3, 2024

rymnc Jul 3, 2024

darshankabariya Jul 4, 2024

rymnc Jul 4, 2024

rymnc Jul 4, 2024

darshankabariya Jul 5, 2024

darshankabariya Jul 5, 2024

github-actions bot commented Jul 2, 2024 •

edited

Loading

gabrielmer left a comment

gabrielmer Jul 2, 2024

gabrielmer Jul 2, 2024

rymnc Jul 5, 2024

rymnc Jul 5, 2024

rymnc Jul 5, 2024

rymnc Jul 5, 2024

rymnc Jul 5, 2024

rymnc Jul 22, 2024

rymnc left a comment

	return err("relay message limit (rln) cannot be exceed 100 ")
	return err("relay message limit (rln) cannot exceed 100")

	$error == "relay message limit (rln) cannot be exceed 100 "
	$error == "relay message limit (rln) cannot exceed 100"

fix: handle rln-relay-message-limit #2867

fix: handle rln-relay-message-limit #2867

Conversation

darshankabariya commented Jul 2, 2024

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

github-actions bot commented Jul 2, 2024 • edited Loading

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

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

rymnc left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 2, 2024 •

edited

Loading