feat: merkle tree persistence #630
Conversation
Jenkins BuildsClick to see older builds (58)
|
|
🤔 For some reason, when a tree config is passed in https://github.com/waku-org/go-waku/pull/630/files#diff-f9f1366bf48973e91af5a769c566b2662c33177e31388325c7f9307b4e7cca55R72 RLN refuses to initialize. Tomorrow I'll figure out what's going on here so I can properly test interop between nwaku and go-waku |
richard-ramos
force-pushed
the
tree-path
branch
from
f61b410 to
19a2f8e
Compare
|
The issue was that I was using the same database for all nodes in the test units, so RLN was complaining about not being able to acquire locks on the DB. It's now fixed :) |
richard-ramos
force-pushed
the
tree-path
branch
from
19a2f8e to
4de8870
Compare
richard-ramos
force-pushed
the
rln/atomics
branch
from
b96c5b0 to
2e2a76f
Compare
richard-ramos
force-pushed
the
tree-path
branch
from
dd4be9f to
4de8870
Compare
richard-ramos
force-pushed
the
rln/atomics
branch
from
2e2a76f to
2b71b1b
Compare
richard-ramos
force-pushed
the
tree-path
branch
from
4de8870 to
9d7eef8
Compare
| @@ -49,15 +49,34 @@ type WakuRLNRelay struct { | |||
| log *zap.Logger | |||
| } | |||
|
|
|||
| const rlnDefaultDepth = 20 | |||
| const rlnDefaultTreePath = "rln_tree.db" | |||
There was a problem hiding this comment.
Maybe we should have default path prefixed with ./
| "errors" | ||
| ) | ||
|
|
||
| type RLNMetadata struct { |
There was a problem hiding this comment.
Need to add comments for all the exported types.
|
|
||
| // TODO: should we track indexes to identify missing? | ||
| startIndex := rln.MembershipIndex(uint(oldestIndexInBlock.Int64())) | ||
| err := gm.rln.InsertMembers(startIndex, idCommitments) |
There was a problem hiding this comment.
Does the RLN API take care of a scenario where few members are already inserted and not throw error? If not, this has to be handled at this level.
e.g: While inserting members, let's say member insertion succeeded whereas before metadata got updated, node went down. Next time when node comes up, it will try to insert some set of members which are already inserted and probably a new set if they are part of a later block. In this case, atomicOp should not fail. If it fails, then we may have to handle it at this layer(identify if members are already present and not insert them again).
There was a problem hiding this comment.
Does the RLN API take care of a scenario where few members are already inserted and not throw error? I
The RLN API handles it by acting as an upsert, overwriting whatever is stored at some index in the merkle tree, so atomicOp should not fail as it will just replace the same members at the same position with the same values
| @@ -112,7 +112,17 @@ func (gm *DynamicGroupManager) HandleGroupUpdates(ctx context.Context, handler R | |||
| } | |||
|
|
|||
| func (gm *DynamicGroupManager) loadOldEvents(ctx context.Context, rlnContract *contracts.RLN, handler RegistrationEventHandler) error { | |||
| events, err := gm.getEvents(ctx, 0, nil) | |||
|
|
|||
There was a problem hiding this comment.
General Improvement: Does this require an archive node access(since we are querying event logs from past)?
If so, archive node operations are costly and running archive node itself is as well. Maybe we should think of an alternative way to get merkleTree changes (either by Store protocol or some other decentralized storage service).
There was a problem hiding this comment.
It should be possible to retrieve events with a full node because these type of nodes contain all transaction receipts/logs. A full node can still be costly so I agree that we'll need to come up with a solution for obtaining the merkle tree changes, instead of relying only on the event logs (although for the time being, it should be fine with using infura/pocket)
- resume onchain sync from persisted tree db - close eth client and db connection appropriately - pass in the path to the tree db
richard-ramos
force-pushed
the
tree-path
branch
4 times, most recently
from
849845b to
7bafa30
Compare
richard-ramos
force-pushed
the
tree-path
branch
from
7bafa30 to
94ed1d8
Compare
|
@chaitanyaprem I updated this PR with fixes for code review observations, most code climate issues, and also fixed some interop issues I found while attempting to get messages going back and forth between nwaku's chat2 and go-waku's |
| @@ -30,7 +31,7 @@ func rlnFlags() []cli.Flag { | |||
| }, | |||
| &cli.StringFlag{ | |||
| Name: "rln-relay-content-topic", | |||
| Value: "/toy-chat/2/luzhou/proto", | |||
| Value: protocol.NewContentTopic("toy-chat", 3, "mingde", "proto").String(), | |||
There was a problem hiding this comment.
Does it make sense to have a default value for content-topic? Rather we should throw an error if content-topic is not mentioned.
We can always inlcude in docs which content-topic can be used if someone wants to just test.
There was a problem hiding this comment.
Note that we plan to remove rln pubsub topics and content topics, so that rln is applied in all the subscribed ones if enabled.
See 1. from waku-org/nwaku#1906
and https://discord.com/channels/1110799176264056863/1111540684575477821/1141378004841410640
There was a problem hiding this comment.
For this PR let's keep it with the hardcoded default content topic to match nwaku's flag: https://github.com/waku-org/nwaku/blob/master/apps/wakunode2/external_config.nim#L165
Description
Persist Merkle Tree in sled db
This is part of Milestone: #605
Changes