-
Notifications
You must be signed in to change notification settings - Fork 66
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
base: master
Are you sure you want to change the base?
Conversation
You can find the image built from this PR at
Built from 35c0f5b |
b65110f
to
511e8b3
Compare
8b4fbf1
to
fcb814e
Compare
There was a problem hiding this 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?
for i in 0 ..< rateCommitments.len: | ||
let index = startIndex + MembershipIndex(i) | ||
await g.fetchMerkleProof(stuint(index, 256)) | ||
|
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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*( |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
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 ) |
e2f5997
to
d1dee0d
Compare
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:
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. 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. :) |
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). |
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/ |
22c1c6b
to
79f48fb
Compare
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. |
d8bff6a
to
aa3c8f5
Compare
7ac7b50
to
7be304f
Compare
5c7f74b
to
965f4bf
Compare
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.
Changes
close #2924