Skip to content

feat: deprecate sync / local merkle tree #3312

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

Draft
wants to merge 81 commits into
base: master
Choose a base branch
from

Conversation

darshankabariya
Copy link
Contributor

@darshankabariya darshankabariya commented Mar 2, 2025

This PR deprecates the sync strategy by removing the need for each node to maintain a local Merkle tree. Previously, every node independently managed a Merkle tree and used it to generate Merkle proofs for RLN proof generation. However, constructing a local Merkle tree required syncing a large amount of blockchain data, which was highly time-consuming.

With the latest RLN contract update, we can now fetch Merkle proofs directly from the smart contract, eliminating the need for local Merkle tree maintenance.

Screenshot 2025-03-21 at 12 19 33 PM

Changes

  • Deprecated the syncing and Merkle tree maintenance code from onchainGroupManager.
  • Introduced a new group manager that fetches Merkle proofs directly from the smart contract and uses them for RLN proof generation.
  • disable effected unittest according

close #2924

Copy link

github-actions bot commented Mar 3, 2025

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3312

Built from 35c0f5b

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.

Thanks! The bulk of the generateProof logic makes sense to me (at least on a high level). I do however have a question below about the caching of merkleProofsByIndex and the continuing handling of events to maintain the Merkle tree. Even if we don't deprecate the tree yet, I'm not sure I understand why we maintain a merkleProofsByIndex (for state we're about to deprecate). It could well be to speed up proof generation (not having to wait for on-chain proof for our own index), but wouldn't a regular timed task be more efficient than processing all change events?
Happy to discuss if I'm missing something or if there's a good reason to maintain some caching. :)
Then it seems to me that what's outstanding to deprecate the tree sync would be to implement validateRoot and verifyProof on the group manager?

Comment on lines 451 to 447
for i in 0 ..< rateCommitments.len:
let index = startIndex + MembershipIndex(i)
await g.fetchMerkleProof(stuint(index, 256))

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because I'm not well-acquainted with RLN code, but I'm struggling to understand why we need to keep handling tree events and caching merkle proofs here. Wouldn't the idea of the changes be to stop handling these events at all? Where are these cached proofs currently used? Afaics the base methods of verifyProof and validateRoot still needs to be implemented with the "on-chain root and proof" approach, which would obviate needing this offline state. Or am I missing something obvious? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if this efficiently targets merkleProofsByIndex, we can use it in generateProof.

From my understanding, this event notifies all remote subscribers whenever there’s an update in the smart contract. Is there a better place where I can move this code?

Copy link
Contributor

@jm-clius jm-clius Mar 13, 2025

Choose a reason for hiding this comment

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

There are some questions here about when to cache the proof, but in principle I agree that caching the proof on tree change event could be a useful strategy, However, the main issue here is that we do not need to cache merkle proofs for every member - we only need our own merkle proof (associated with our own, single membership) to generate proofs. Worst case scenario is to lookup the merkle proof every time we want to generate a proof (i.e. every time we publish a message). Of course caching this would be very helpful, but we don't need the merkle proofs for any other member.

@@ -226,6 +239,93 @@ method withdrawBatch*(
): Future[void] {.async: (raises: [Exception]).} =
initializedGuard(g)

method generateProof*(
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion, but why not simply create a new group manager type for this new no-sync on-chain group manager (or rename and move the old one to something like SyncedGroupManager which can remain unused for now)?

Copy link
Contributor Author

@darshankabariya darshankabariya Mar 11, 2025

Choose a reason for hiding this comment

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

Interesting idea. I agree with you it’s better to isolate the code, at least temporarily, so it doesn’t affect unit tests or other parts of the code. After review, we can use the new group manager for unit testing and then finally replace replace it with the on-chain group manager. is that you want think right ? for now i just put generateProof Function on new groupmanager and cache and handling event i still pul in onchaingroupmanager.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm thinking is that there may eventually be interest in a group manager that does indeed sync the entire tree (as a form of further self-sovereignty). If we keep the current on-chain group manager as-is and simply default to a new group manager (without tree syncing), we leave this option open. No strong opinion, though. :) My thought was simply that it could be simpler to implement in this way - fully implement a new group manager from scratch and only switch to this new manager once it's fully tested and mature, rather than trying to adapt the existing one.

@darshankabariya
Copy link
Contributor Author

darshankabariya commented Mar 11, 2025

I do however have a question below about the caching of merkleProofsByIndex and the continuing handling of events to maintain the Merkle tree. Even if we don't deprecate the tree yet, I'm not sure I understand why we maintain a merkleProofsByIndex (for state we're about to deprecate). It could well be to speed up proof generation (not having to wait for on-chain proof for our own index), but wouldn't a regular timed task be more efficient than processing all change events?

Hey @jm-clius, I use caching to speed up proof generation. Without it, let say the node would call the smart contract 80 times per epoch, which is inefficient. With caching, we can instantly use Merkle proofs and update the index through event handling. The trade-off is that updates only trigger when new members join, but I don’t think 80 members join every epoch. Let me know if I’m missing something.

Then it seems to me that what's outstanding to deprecate the tree sync would be to implement validateRoot and verifyProof on the group manager?

Thanks for suggestion ! now it's more clear.

One question: Do we use the index of other members in the group manager cache because light nodes rely on the relay node to generate proofs for them right ? If that’s the case, the trade-off is clear—if only 5 light nodes connect to a relay node, it would need to generate around 400 proofs (or just 80 if I’m mistaken) ( apx :- 400 smart contract call per epoch )

@jm-clius
Copy link
Contributor

I use caching to speed up proof generation. Without it, let say the node would call the smart contract 80 times per epoch, which is inefficient.

Do you mean if we generate up to 80 messages per epoch? It makes sense to me to cache the proof to avoid having to call the SC for every generated messages. My point is that I don't see why you need to maintain this for every member (or maintain the entire tree). Afaiu you only need the merkle proof for your own membership index.

In other words, all we need from the SC:

  • your own merkle proof, whenever you generate a message with a proof attached
  • the current root of the tree, whenever you validate a received proof

In other words, there is no need to maintain a proof by index. Perhaps it's an acceptable strategy for now to refresh this single merkle proof whenever there is a change event, but the state can be much simpler.
To validate received messages, you would need the latest root. This can also be cached, I think, whenever there's a tree change, but this would still be the only necessary state to validate.

I haven't looked at it yet, but we can use this comparable go-waku implementation as reference: https://github.com/alrevuelta/go-waku-light

That said, I might be missing something here, so if there's any confusion, let's set up a call or find some expert eyes to help us think. :)

@jm-clius
Copy link
Contributor

Do we use the index of other members in the group manager cache because light nodes rely on the relay node to generate proofs for them right ?

No, light/edge nodes in this model will generate their own proofs, by simply getting the merkle proof from the tree (exactly as the "full" nwaku node will do). This is essentially stateless, if you don't mind doing the merkle proof SC call every time you generate a message (which could be low rate for most use cases). The RLNaaS model you refer to will have a full node generate a proof on behalf of lightpush clients using its own (i.e. the full node's) membership. In other words, from an RLN perspective it will be exactly the same as if the full node generated the message itself. It's not possible to generate a proof on behalf of another node as you need access to that node's secret (the merkle proof is not enough).

@jm-clius
Copy link
Contributor

This is also a useful article about what state is necessary for which operations in RLN: https://blog.waku.org/2024-07-10-rln-v2-stateless-light-clients/

@darshankabariya darshankabariya force-pushed the deprecate_sync_strategy branch from 22c1c6b to 79f48fb Compare March 13, 2025 21:14
@darshankabariya
Copy link
Contributor Author

These three comments have cleared up a lot of my doubts. I now clearly understand the responsibilities of each group manager and how they differ from one another.

Currently, I’m building parallel implementations of the static and on-chain group managers. This new group manager will directly use the root and Merkle proof from the smart contract, eliminating the need for Merkle tree maintenance. As a result, we no longer have to deal with extensive syncing and event-related tasks.

I’m moving in that direction, and once it’s ready, I’ll ask for another review. Sounds cool, right? After getting an expert review, we can deprecate the current on-chain manager.

@darshankabariya darshankabariya force-pushed the deprecate_sync_strategy branch from 7ac7b50 to 7be304f Compare April 17, 2025 11:04
@darshankabariya darshankabariya force-pushed the deprecate_sync_strategy branch from 5c7f74b to 965f4bf Compare April 17, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore(rln-relay): deprecate tree sync strategy
6 participants