search

waku-org / js-rln

Browser library providing the cryptographic functions for Waku RLN Relay https://rfc.vac.dev/spec/17/
Apache License 2.0
5 stars 1 forks source link

fix(rln): convert id commitments to uint256 from LE on register #37

Closed s1fr0 closed 1 year ago

s1fr0 commented 2 years ago

Problem

Zerokit generates and serializes id keys and the corresponding id commitments in little endian. The membership smart contract, instead, expects id commitments to be uint256.

This means that when registering an id commitment generated from zerokit, js-rln should ensure that the value registered corresponds to the uint256 conversion from the id commitment in little endian.

This is not the case at the moment, where no endianess conversion is implemented and commitments are registered "as they are" to the contract. This has the consequences that proofs might result invalid when verified from other implementations.

Example

Zerokit generates

Key: b20ea77f8d11182df896021c9f1c6f82e21694a44ecf1b030461a5b4fc581817
Commitment: b69a4ee2e32152c4b4bf3b677d160b9d19d9ab043a281dd39cca84a6eadafa11

both visualized in their little-endian representation. The value that should be registered to the smart contract is

Commitment: 0x11fadaeaa684ca9cd31d283a04abd9199d0b167d673bbfb4c45221e3e24e9ab6

and not 0xb69a4ee2e32152c4b4bf3b677d160b9d19d9ab043a281dd39cca84a6eadafa11 as happens now.

Acceptance criteria

staheri14 commented 2 years ago

A few questions: Is this happening just in the js-rln? Does the inconsistency happen at the Merkle tree level? Does it require update on the wire format? is the e.g., Merkle tree root exchanged as part of RateLimitProof also encoded in LE?

Fix endianess conversion Where should this fix happen? in js-rln?

s1fr0 commented 2 years ago

Is this happening just in the js-rln?

Observed/tested only there so far.

Does the inconsistency happen at the Merkle tree level?

No.

Does it require update on the wire format? is the e.g., Merkle tree root exchanged as part of RateLimitProof also encoded in LE?

Not sure, I would say this depends how it is implemented. According to logs, it seems that rln-js proofs are correctly verified by chat2 users except for the root validation check which depends on this issue.

Where should this fix happen? in js-rln?

Yes, the issue is open in js-rln.

staheri14 commented 2 years ago

So,

  1. The contract should hold the unit256 representation of the id commitments with LE encoding. That is id commitment should be registered to the contract in LE format.
  2. The Merkle tree roots should be also in LE format.
  3. And the zkSNARK proofs should be generated using the id commitment and id key in LE format.

What is the exact part that js-rln is doing inconsistent with the above?

s1fr0 commented 2 years ago
  1. The contract should hold the unit256 representation of the id commitments with LE encoding. That is id commitment should be registered to the contract in LE format.

No, the id commitment should be registered as its corresponding uint256 (eventually converted from LE or any other encoding you may use).

  1. The Merkle tree roots should be also in LE format.
  2. And the zkSNARK proofs should be generated using the id commitment and id key in LE format.

Correct, but all this is managed by zerokit and is transparent to js-rln. When something is printed "outside" zerokit to make it human-readable you can print the values as uint256 (see related PRs: https://github.com/status-im/nwaku/pull/1256, https://github.com/status-im/nwaku/pull/1259)

js-rln sends the LE encoding directly to the contract instead of converting it first to uit256. The issue contains an example that should clarify what happens.

fryorcraken commented 1 year ago

@s1fr0 If js-rln registers the wrong value in the contract, how come proofs generated by js-rln are accepted by nwaku when the creds registered in the contract are incorrect?

s1fr0 commented 1 year ago

@s1fr0 If js-rln registers the wrong value in the contract, how come proofs generated by js-rln are accepted by nwaku when the creds registered in the contract are incorrect?

@fryorcraken EXPERIMENTAL features are disabled by default on test fleet (this implies that all messages will be relayed, but you should receive a local warning in chat2 if messages are spam/invalid).

I deployed master with experimental features enabled and messages:

Note that messages generated in js-rln are marked as verified, but verification is done with the "Verifying proof without roots" routine. Before also nwaku used such routine, and that's why you were able to exchange messages even if roots differ, but now root verification is enabled against the last 5 root values.

I believe the LE encoding is the main responsible here, and I hope is the only one.