waku-org / nwaku

Waku node and protocol.
Other
201 stars 53 forks source link

possible bug: [RLN] Mebership Index don't need to match #2742

Open AlejandroCabeza opened 5 months ago

AlejandroCabeza commented 5 months ago

Problem

When mounting OnChainGroupManager, the MembershipIndex specified in WakuRlnConfig doesn't need to match the MembershipIndex used on the addMembershipToKeystore. Also, as for the WakuRlnConfig indices, for some reason they must begin at 0 in incremental order. Otherwise it timeouts.

To reproduce

It's easier to understand by having a look at the test case tests/node/test_wakunode_relay_rln:482, named Valid contract. This test represents the happy path for sending an OnChain RLN message. In the line 516 two WakuRlnConfig instances are defined, one with MembershipIndex 0, and the other with MembershipIndex 1. In line 536, the indices are retrieved using onChainGroupManager.membershipIndex.get(); but this is irrelevant, as any pair of numbers will do.

Expected behavior

For a node to use a specific set of RLN credentials, their config should specify the same MembershipIndex as the one used to add those credentials to the keystore.

nwaku version/commit hash

AlejandroCabeza commented 5 months ago

Okay, with today's fresh mind I gathered a bit more information on these.

Matching indices

They're not required because after the node's RLN intialisation we're monkeypatching the node.groupManager.idCredentials. Reading the section below gives a better understanding on this.

Indices in Incremental Order

The credential indices assigned to each node's RLN config are not the tree's indices for those credentials, but the registration order of the patched credentials. Elaborating with an example:

  1. Let's say there are 3 available credentials, c0, c1, c2. You have instantiated them and registered them in that order.
  2. There are two configs that require one index each: configA(indexA) and configB(indexB). And let's arbitrarily say indexA is 0 and indexB is 2.
  3. After initialising two nodes, nodeA and nodeB (each with their corresponding config), you need to monkeypatch each node's groupManager's idCredentials to workaround an initialisation issue (happens during testing). To monkeypatch the credentials, you MUST use the credentials that correspond to the index specified in the respective config, but regarding the registration order of those credentials (not the index those credentials were saved in in the keystore).
    • Given c0 was registered first (akin to index 0), c1 second (akin to index 1), and c2 third (akin to index2). Then for configA, who we said had index=0, you'd need to use c0; and for configB, who we said had index=2, you need to use c2. That is:
    • nodeA.groupManager.idCredentials = c0
    • nodeB.groupManager.idCredentials = c2 If you don't make these match, the RLN message will be rejected.

I'm still not sure as to why this works this way. As part of the test setup, when you add the credentials to the keystore (calling addMembershipCredentialsToKeystore) you need to specify the index you want to save them in. Even if you're monkeypatching the credentials later, if you save the credentials in the keystore with indices 100 and 101; I'd expect the nodes to fail booting if you pass them any other indices, as that'd mean it failed fetching the credentials from the keystore.

AlejandroCabeza commented 5 months ago

A node that didn't register credentials (this test case works this way, it uses a different manager to register credentials beforehand), should be able to fetch the corresponding credentials with the keystore path, keystore password and membership index. In this case, after the nodes boot, the idCredentials are none where they should be populated (as the parameters are seemingly okay). This would explain the behaviour described above.

rymnc commented 5 months ago

will investigate this week