fix: refact `rln-relay` and post sync test by darshankabariya · Pull Request #3434 · logos-messaging/logos-delivery

darshankabariya · 2025-06-02T17:33:39Z

Description

This refactor was necessary because we’ve already transitioned to a non-synchronous approach, and all sync-related code is being deprecated.

Over time, GroupManager became bloated with tightly coupled and complex logic. This PR initiates a refactor to improve the clarity, maintainability, and future extensibility of the RLN relay protocol.

Changes

Restructured the GroupManager hierarchy and introduced offchainGroupManager to replace staticGroupManager.
Deprecated the static manager class in favor of the offchainManager interface. (As suggested by @jm-clius, both on-chain and off-chain trees may exist in the future; currently, OffchainManager is just an interface.)
Removed all zerokit ffi for the merkle tree along with its associated proof generation and verification wrappers.
Deleted all unit tests related to the old merkle tree implementation.
Update all effected with onchain anvail

Issue

Maintenance Y2025H2 #3483

github-actions · 2025-06-02T17:39:14Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3434

Built from c1dc003

Ivansete-status · 2025-06-04T14:30:30Z

Thanks so much for it @darshankabariya !
Always keen on simplify things.
Ping me when the tests are not disabled. If a test needs to be disabled, let's reason it.
Thanks again!

darshankabariya · 2025-08-06T09:10:30Z

Hi @Ivansete-status, @jm-clius, @stubbsta —
The PR is now ready for review.
A few test cases are taking longer than expected — I’m still looking into that.
However, all the core logic changes for improving the RLN relay protocol are complete.

cc @NagyZoltanPeter

jm-clius

Thanks! I've just eyeballed the changes for now - I think this is a good step towards simplification. So far one-ish question below.

jm-clius · 2025-08-07T14:48:12Z

waku/waku_rln_relay/group_manager/off_chain/group_manager.nim

I might just be missing it, but why do we need an OffchainGroupManager at all? We now have the base interface, which we can implement again when we re-add a real offchain implementation?

Yes, you got it right. Actually, I couldn’t find any use for the StaticGroupManager class, so I deprecated it along with its unit tests. As you noticed, I introduced the OffchainGroupManager interface—this idea came from your suggestion that we might support both offchain and onchain features in the future. I’ve left it unimplemented for now.

jm-clius · 2025-08-07T14:49:10Z

waku/waku_rln_relay/group_manager/off_chain.nim

@@ -0,0 +1,3 @@
+import off_chain/group_manager


Why have a barrel import for off_chain, but not for on_chain?

(Barrel imports are somewhat controversial, but for now that's beside the point :D )

There’s another file in the same directory, waku/waku_rln_relay/group_manager/on_chain.nim, similar to the offchain one. It looks a bit oversimplified — I’ll take a closer look.

cc @Ivansete-status @jm-clius

Ivansete-status

Thanks for it! 🙌
Some comments.
For every snippet code that you remove, kindly add a clear explanation as to why we can remove it, for future reference.
Thanks!

Ivansete-status · 2025-08-18T14:04:32Z

tests/node/test_wakunode_legacy_lightpush.nim


  suite "Lightpush attaching RLN proofs":
-    asyncTest "Message is published when RLN enabled":
+    xasyncTest "Message is published when RLN enabled":


Why that is commented if may I ask?

Unable testcase !!

Ivansete-status · 2025-08-18T14:06:11Z

tests/waku_rln_relay/rln/test_wrappers.nim


  suite "proofGen":
-    test "Valid zk proof":
+    xasyncTest "Valid zk proof":


Same for that one. Why skipped?

Ivansete-status · 2025-08-18T14:06:41Z

tests/waku_rln_relay/test_rln_group_manager_offchain.nim

+    credentials.add(generateCredentials(rlnInstance))
+  return credentials
+
+suite "Offchain group manager":


Is this test suite running any test ?
It seems this whole module can be removed.

Yes, I agree — we no longer need the off-chain testcase since there’s nothing left to test. but also there no harm at all, that why not delete.

Ivansete-status · 2025-08-18T14:11:15Z

waku/waku_rln_relay/group_manager/group_manager_base.nim

-    return err("proof verification failed: " & $proofVerifyRes.error())
-  return ok(proofVerifyRes.value())
+  ## Dummy implementation for verifyProof
+  return ok(true)


Why not the following?

Suggested change

return ok(true)

return err("veryfyProof is not implemented")

good idea !!

Ivansete-status · 2025-08-18T14:12:44Z

waku/waku_rln_relay/group_manager/off_chain/group_manager.nim

+template initializedGuard*(g: OffchainGroupManager): untyped =
+  discard


Why is everything discarded?

Thanks for pointing out, I’ve updated all functions to return an unimplemented error.

Ivansete-status

Some more comments, thanks ! 🙌

Ivansete-status · 2025-08-28T12:27:52Z

tests/waku_rln_relay/utils_onchain.nim

+    raise
+      newException(CatchableError, "Failed to deploy test token contract: " & $error)


Is it strictly needed the change? I would avoid explicitly raising exceptions, if possible

I updated it with the idea of unification. I noticed we mostly use exceptions in our code, so I replaced asserts with exceptions. But now i realize that is for is for binary not test. so assertion is better idea right ??

Ivansete-status · 2025-08-28T12:28:11Z

tests/waku_rln_relay/utils_onchain.nim

  let contractAddress = (await executeForgeContractDeployScripts(privateKey, acc, web3)).valueOr:
-    assert false, "Failed to deploy RLN contract: " & $error
-    return
+    raise newException(CatchableError, "Failed to deploy RLN contract: " & $error)


similar to above

Ivansete-status · 2025-08-28T12:53:28Z

tests/node/test_wakunode_legacy_lightpush.nim


+    anvilProc = runAnvil()
+    tempManager =
+      cast[ptr OnchainGroupManager](allocShared0(sizeof(OnchainGroupManager)))


Why the manual allocation is needed?

Actually, I need this onchainGroupManager to fetch real-time RLN metadata such as contractAddress and useMessageLimit. If we hardcode these values, then every time the default RLN metadata changes, we’ll also need to update the tests. but yes definatly it's finest way to fetch we also usg RlnConf default value but i feel over code.

Ivansete-status · 2025-08-28T12:56:56Z

tests/waku_rln_relay/rln/waku_rln_relay_utils.nim

-  let proof = rlnPeer.groupManager.generateProof(input, epoch, 0).valueOr:
+  # Fetch Merkle proof either when a new root was detected *or* when the cache is empty.
+  if rootUpdated or manager.merkleProofCache.len == 0:
+    let proofResult = waitFor manager.fetchMerkleProofElements()


Even though this is for testing purposes only, the waitFor should be used with care and only in the top logic layers. Better await

Ivansete-status · 2025-08-28T13:04:43Z

tests/waku_rln_relay/rln/waku_rln_relay_utils.nim

+): WakuRlnConfig =
+  let wakuRlnConfig = WakuRlnConfig(


Suggested change

): WakuRlnConfig =

let wakuRlnConfig = WakuRlnConfig(

): WakuRlnConfig =

## For testing purposes only

let wakuRlnConfig = WakuRlnConfig(

It’s already in the test file, so why do we need to mention it explicitly ?

Ivansete-status · 2025-08-28T13:07:25Z

tests/waku_rln_relay/test_rln_serde.nim


 suite "RLN Relay v2: serde":
-  test "toLeaf: converts a rateCommitment to a valid leaf":
+  xasyncTest "toLeaf: converts a rateCommitment to a valid leaf":


Suggested change

xasyncTest "toLeaf: converts a rateCommitment to a valid leaf":

asyncTest "toLeaf: converts a rateCommitment to a valid leaf":

same !! as below

Ivansete-status · 2025-08-28T13:07:38Z

tests/waku_rln_relay/test_rln_serde.nim

    check expectedLeaf == leafRes.value.inHex()

-  test "proofGen: generates a valid zk proof":
+  xasyncTest "proofGen: generates a valid zk proof":


Suggested change

xasyncTest "proofGen: generates a valid zk proof":

asyncTest "proofGen: generates a valid zk proof":

[tests/waku_rln_relay/test_rln_serde.nim] is no longer needed. It was testing proof generation in isolation, but after moving to the on-chain tree, proof generation requires running a node and registering membership. Since it’s no longer possible to generate proofs fully off-chain, this test case is obsolete. deleting whole file.

Ivansete-status · 2025-08-28T13:15:57Z

waku/waku_rln_relay/group_manager/on_chain/group_manager.nim

      if rootUpdated:
        if g.membershipIndex.isNone():
-          error "membershipIndex is not set; skipping proof update"
+          debug "membershipIndex is not set; skipping proof update"


Should not this be considered as an error?

It’s no longer required because earlier registering RLN membership was mandatory, but now users can run the code without membership. so removed it. thanks for pointing out, it;s help me think new direction.

Ivansete-status · 2025-08-28T13:23:35Z

waku/waku_rln_relay/rln_relay.nim

+proc getCurrentEpoch*(rlnPeer: WakuRLNRelay, t: float64 = -1.0): Epoch =
+  ## Returns the RLN epoch corresponding to `t` (Unix seconds).
+  ## If `t` is not supplied (or set to a negative value), the current time is used.
+  let timeToUse =
+    if t < 0.0:
+      epochTime()
+    else:
+      t
+  return rlnPeer.calcEpoch(timeToUse)


This can be simplified, considering that t is never being passed:

Suggested change

proc getCurrentEpoch*(rlnPeer: WakuRLNRelay, t: float64 = -1.0): Epoch =

## Returns the RLN epoch corresponding to `t` (Unix seconds).

## If `t` is not supplied (or set to a negative value), the current time is used.

let timeToUse =

if t < 0.0:

epochTime()

else:

t

return rlnPeer.calcEpoch(timeToUse)

proc getCurrentEpoch*(rlnPeer: WakuRLNRelay): Epoch =

## Returns the RLN epoch corresponding to current epochTimeme is used.

return rlnPeer.calcEpoch(epochTime())

Thanks for pointing that out. I had made the change for a specific test, but later realized that passing just one test isn’t a good reason to change the structure of this function. I forgot to revert it. finally remove that

darshankabariya · 2025-09-06T08:43:34Z

LGTM! I'd make sure we remove all the tests that are not needed and keep the needed ones. F.e., it seems we should not skip the following in `tests/waku_rln_relay/rln/test_wrappers.nim
And also, CI green :D

Hi @Ivansete-status , thank you for the approval. I’ve addressed all your suggestions and updated the changes accordingly. All CI checks are green now and tests have been updated as well. If you see this before merging the PR, kindly have a look.

waku/waku_rln_relay/group_manager/group_manager_base.nim

stubbsta

LGTM!

jm-clius

Thanks! I'm ready to approve once I understand why the OffchainGroupManager exists at all. :) Will be happy to have a call if I'm missing some context or my comments are unclear.

jm-clius · 2025-09-09T10:51:49Z

waku/waku_rln_relay/group_manager/off_chain/group_manager.nim

+# Placeholder for future OffchainGroupManager re-implementation
+type OffchainGroupManager* = ref object of GroupManager


Again, I don't understand why we have this non-implementation as placeholder. There is no real implementation and this is only referenced in tests (and in rln_relay.nim, where it probably shouldn't be included anymore, see my comment there). In other words, wherever you still use OffchainGroupManager in the tests it could just be GroupManager and we delete the files and folders for off_chain for now. Am I missing some important context?

thanks for review. main objective for offchainGM is to serve as a future perspective, so that when we revisit the offchain tree, we can leverage the currently unimplemented offchainGM functionality. But I understand your point — we can remove offchainGM for now as well, should i remove or not ??

jm-clius · 2025-09-09T10:53:52Z

waku/waku_rln_relay/rln_relay.nim

@@ -447,11 +413,11 @@ proc mount(

  if not conf.dynamic:


We don't support non-dynamic group managers anymore, from what I can see. There's no "OffchainGroupManager" - just a stub with that name that will raise exceptions for every function call. In other words, we can remove this completely and mark the config item as deprecated.

Yes!! we also need to remove that along with offchainGM.

jm-clius

Thanks! Two comments below, but no need to re-request review once addressed.

jm-clius · 2025-09-09T18:06:03Z

waku/waku_rln_relay/rln_relay.nim

  let rlnInstance = createRLNInstance(tree_path = conf.treePath).valueOr:
    return err("could not create RLN instance: " & $error)

-  if not conf.dynamic:


I think this config item must be marked in the external config as deprecated (and then removed within two releases). Presumably it will not break any existing deployments and the default is true in any case?

it’s a really good idea. I found that the default value true for dynamic is set only inside NetworkMonitor. we will handle in seperate PR.

jm-clius · 2025-09-09T18:06:35Z

apps/benchmarks/benchmarks.nim

  let credentials = toSeq(0 .. 1000).mapIt(membershipKeyGen(rlnIns).get())

-  let manager = StaticGroupManager(
+  let manager = OffchainGroupManager(


I think this reference does not exist anymore. Perhaps instantiate the base GroupManager?

Will update the benchmark accordingly.

github-actions bot assigned darshankabariya Jun 2, 2025

darshankabariya force-pushed the refact_groupmanager branch 2 times, most recently from c5860b8 to cc3cb7e Compare June 3, 2025 08:13

darshankabariya requested review from Ivansete-status, NagyZoltanPeter, gabrielmer and jm-clius June 3, 2025 08:39

darshankabariya changed the title ~~chore: refactor groupmanager~~ chore: refactor groupManager Jun 3, 2025

darshankabariya requested a review from SionoiS June 3, 2025 13:08

darshankabariya force-pushed the refact_groupmanager branch from 79657d1 to 57a17af Compare June 8, 2025 18:08

darshankabariya marked this pull request as draft June 16, 2025 06:17

darshankabariya force-pushed the refact_groupmanager branch from 57a17af to f300fe6 Compare July 24, 2025 18:43

darshankabariya changed the title ~~chore: refactor groupManager~~ chore: remove off-chain tree Jul 25, 2025

darshankabariya force-pushed the refact_groupmanager branch from 8453259 to 5f7bcc3 Compare July 25, 2025 09:31

darshankabariya force-pushed the refact_groupmanager branch from 691f1b9 to 426b327 Compare August 4, 2025 07:44

darshankabariya requested review from stubbsta and removed request for gabrielmer August 6, 2025 09:11

darshankabariya marked this pull request as ready for review August 6, 2025 09:11

jm-clius reviewed Aug 7, 2025

View reviewed changes

darshankabariya requested a review from jm-clius August 7, 2025 17:36

darshankabariya force-pushed the refact_groupmanager branch from 786bc6f to 49d66d5 Compare August 16, 2025 17:12

Ivansete-status reviewed Aug 18, 2025

View reviewed changes

darshankabariya force-pushed the refact_groupmanager branch from 1fbc1a4 to d75552f Compare August 25, 2025 17:44

darshankabariya changed the title ~~chore: remove off-chain tree~~ chore: refact rln-relay and post sync test Aug 26, 2025

darshankabariya changed the title ~~chore: refact rln-relay and post sync test~~ fix: refact rln-relay and post sync test Aug 26, 2025

Ivansete-status reviewed Aug 28, 2025

View reviewed changes

darshankabariya force-pushed the refact_groupmanager branch from 9b30f0a to 9eae858 Compare August 28, 2025 21:38

darshankabariya added 12 commits September 4, 2025 13:50

chore: update rlnConf1

67d9c93

chore: lint issue

8e08e60

chore: manage for wakunode_rln_relay

06260e7

chore: manage for wakunode_rln_relay

1ce8aaa

chore: manage for wakunode_lightpush

9936a6b

chore: update lagacy lightpush testcase

cf093e5

chore: update according to suggestion

7d4d942

chore: test compile issue

8e2d9d7

chore: update according to suggestion

0c03af2

chore: update according to suggestion

61754a1

chore: test compile issue

b1321af

chore: test compile issue

610492c

darshankabariya force-pushed the refact_groupmanager branch from 59c3591 to 610492c Compare September 4, 2025 08:26

darshankabariya added 3 commits September 4, 2025 16:55

chore: update test issue

435739f

chore: update test issue

7bff18a

chore: update test issue

88b5f62

stubbsta reviewed Sep 8, 2025

View reviewed changes

waku/waku_rln_relay/group_manager/group_manager_base.nim Show resolved Hide resolved

stubbsta approved these changes Sep 8, 2025

View reviewed changes

Ivansete-status mentioned this pull request Sep 8, 2025

Maintenance Y2025H2 #3483

Closed

jm-clius requested changes Sep 9, 2025

View reviewed changes

darshankabariya and others added 2 commits September 9, 2025 22:51

chore: remove offchain group manager

30961a8

Merge branch 'master' into refact_groupmanager

a6792d2

darshankabariya requested a review from jm-clius September 9, 2025 17:58

jm-clius approved these changes Sep 9, 2025

View reviewed changes

darshankabariya and others added 2 commits September 10, 2025 13:23

Merge branch 'master' into refact_groupmanager

5cb4ee2

chore: benchmark change to onchain

bac560f

darshankabariya merged commit 7538912 into master Sep 10, 2025
10 of 12 checks passed

darshankabariya deleted the refact_groupmanager branch September 10, 2025 10:48

Ivansete-status pushed a commit that referenced this pull request Jan 28, 2026

fix: refact rln-relay and post sync test (#3434)

9bba8b0

		template initializedGuard*(g: OffchainGroupManager): untyped =
		discard

		raise
		newException(CatchableError, "Failed to deploy test token contract: " & $error)

	xasyncTest "toLeaf: converts a rateCommitment to a valid leaf":
	asyncTest "toLeaf: converts a rateCommitment to a valid leaf":

	xasyncTest "proofGen: generates a valid zk proof":
	asyncTest "proofGen: generates a valid zk proof":

		# Placeholder for future OffchainGroupManager re-implementation
		type OffchainGroupManager* = ref object of GroupManager

Conversation

darshankabariya commented Jun 2, 2025 • edited by Ivansete-status Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue

Uh oh!

github-actions bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivansete-status commented Jun 4, 2025

Uh oh!

darshankabariya commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jm-clius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darshankabariya Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivansete-status left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivansete-status Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivansete-status left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darshankabariya Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darshankabariya Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

darshankabariya commented Jun 2, 2025 •

edited by Ivansete-status

Loading

github-actions bot commented Jun 2, 2025 •

edited

Loading

darshankabariya commented Aug 6, 2025 •

edited

Loading

darshankabariya Aug 7, 2025 •

edited

Loading

Ivansete-status Aug 18, 2025 •

edited

Loading

darshankabariya Sep 1, 2025 •

edited

Loading

darshankabariya Aug 28, 2025 •

edited

Loading

darshankabariya Aug 28, 2025 •

edited

Loading

darshankabariya Aug 28, 2025 •

edited

Loading

darshankabariya Sep 9, 2025 •

edited

Loading