feat(rln): proof generation and verification with on-chain merkle proof/root #2749
Conversation
size-limit report 📦
|
adklempner
force-pushed
the
feat/rln-proof-gen-verification
branch
from
c6b6d03 to
a15b352
Compare
fix: update tests fix: store merkle proof/root as constant, remove use of RPC in proof gen/verification test
adklempner
force-pushed
the
feat/rln-proof-gen-verification
branch
from
a15b352 to
73a2c1d
Compare
adklempner
changed the title
| public toString(): string { | ||
| return JSON.stringify(this.data); | ||
| // Custom replacer function to handle BigInt serialization | ||
| const bigIntReplacer = (_key: string, value: unknown): unknown => { |
There was a problem hiding this comment.
nit: remove this duplication as there is literally the same function below
| expect(merkleProof).to.be.an("array"); | ||
| expect(merkleProof).to.have.lengthOf(MERKLE_TREE_DEPTH); // RLN uses fixed depth merkle tree | ||
|
|
||
| merkleProof.forEach((element, i) => { |
There was a problem hiding this comment.
nit: for loop would be better here
| */ | ||
| public static async create(): Promise<RLNInstance> { | ||
| try { | ||
| await initUtils(); |
There was a problem hiding this comment.
question: how fast does it load?
There was a problem hiding this comment.
Pull request overview
This PR implements stateless RLN proof generation and verification using on-chain Merkle proofs, eliminating the need to maintain a local Merkle tree. The implementation updates the zerokit WASM dependencies and adds comprehensive functionality for generating and verifying RLN proofs directly in the browser using smart contract-provided roots and proofs.
Key Changes:
- Added
generateRLNProof()andverifyRLNProof()methods to the Zerokit class for proof generation and verification - Implemented Merkle tree utilities for root reconstruction and path direction extraction from on-chain proofs
- Updated hash utilities to use new zerokit-rln-wasm-utils package with simplified APIs
- Fixed BigInt serialization in Keystore to handle JSON conversion properly
Reviewed changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rln/src/zerokit.ts | Adds core proof generation/verification methods with witness serialization and external nullifier calculation |
| packages/rln/src/utils/merkle.ts | New Merkle tree utilities for root reconstruction, rate commitment calculation, and path direction extraction |
| packages/rln/src/utils/hash.ts | Updates hash functions to use new zerokit-rln-wasm-utils package |
| packages/rln/src/utils/bytes.ts | Adds fromBigInt() method for converting BigInt to byte arrays with configurable endianness |
| packages/rln/src/utils/epoch.ts | Simplifies epoch encoding using BytesUtils.writeUIntLE |
| packages/rln/src/proof.spec.ts | Integration test validating end-to-end proof generation and verification with real keystore data |
| packages/rln/src/zerokit.browser.spec.ts | Browser test for seeded identity credential generation consistency |
| packages/rln/src/utils/test_keystore.ts | Static test data with membership keystore and on-chain Merkle proof/root |
| packages/rln/src/rln.ts | Initializes new zerokit-rln-wasm-utils package |
| packages/rln/src/keystore/keystore.ts | Fixes BigInt JSON serialization with custom replacer function |
| packages/rln/karma.conf.cjs | Adds WASM file configuration for zerokit-rln-wasm-utils |
| packages/rln/package.json | Adds zerokit-rln-wasm-utils dependency, removes unused @waku/interfaces |
| packages/rln/.mocharc.cjs | Minor formatting fix (trailing newline) |
| package-lock.json | Updates dependencies including viem, @wagmi/cli, and adds zerokit-rln-wasm-utils |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function extractPathDirectionsFromProof( | ||
| proof: readonly bigint[], | ||
| leafValue: bigint, | ||
| expectedRoot: bigint, | ||
| maxIndex: bigint = (1n << BigInt(MERKLE_TREE_DEPTH)) - 1n | ||
| ): number[] | null { |
There was a problem hiding this comment.
The documentation mentions that the function can return null, but doesn't explain what a null return value means for the caller or what they should do when this happens. Consider adding guidance such as: "Returns null if no valid index can be found that produces the expected root, which may indicate an invalid proof or incorrect parameters."
| function bigIntToBytes32(value: bigint): Uint8Array { | ||
| const bytes = new Uint8Array(32); | ||
| let temp = value; | ||
|
|
||
| for (let i = 0; i < 32; i++) { | ||
| bytes[i] = Number(temp & 0xffn); | ||
| temp >>= 8n; | ||
| } | ||
|
|
||
| return bytes; | ||
| } |
There was a problem hiding this comment.
Missing validation for value parameter. The function should validate that the BigInt value fits within 32 bytes (i.e., value < 2n ** 256n). If a larger value is provided, the function will silently truncate it by only taking the lower 32 bytes, which could lead to incorrect results without any indication of error.
| public static fromBigInt( | ||
| value: bigint, | ||
| byteLength: number, | ||
| outputEndianness: "big" | "little" = "little" | ||
| ): Uint8Array { | ||
| if (value < 0n) { | ||
| throw new Error("Cannot convert negative BigInt to bytes"); | ||
| } | ||
|
|
||
| if (value === 0n) { | ||
| return new Uint8Array(byteLength); | ||
| } | ||
|
|
||
| const result = new Uint8Array(byteLength); | ||
| let workingValue = value; | ||
|
|
||
| // Extract bytes in big-endian order | ||
| for (let i = byteLength - 1; i >= 0; i--) { | ||
| result[i] = Number(workingValue & 0xffn); | ||
| workingValue = workingValue >> 8n; | ||
| } | ||
|
|
||
| // If we need little-endian output, reverse the array | ||
| if (outputEndianness === "little") { | ||
| result.reverse(); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Missing validation to ensure the BigInt value fits within the specified byteLength. If value requires more than byteLength bytes to represent, the function will silently truncate the higher-order bytes, leading to data loss without any error indication. Consider adding a check: if (value >= (1n << BigInt(byteLength * 8))) throw new Error("Value too large for specified byte length");
| Number(membershipIndex), | ||
| new Date(), | ||
| credential.identity.IDSecretHash, | ||
| merkleProof.map((proof) => BytesUtils.fromBigInt(proof, 32, "little")), |
There was a problem hiding this comment.
Variable name shadowing: the parameter name proof in the map callback shadows the outer variable merkleProof being mapped. This is confusing and could lead to errors. Consider renaming to: merkleProof.map((element) => BytesUtils.fromBigInt(element, 32, "little"))
| merkleProof.map((proof) => BytesUtils.fromBigInt(proof, 32, "little")), | |
| merkleProof.map((element) => BytesUtils.fromBigInt(element, 32, "little")), |
| leafValue: bigint, | ||
| expectedRoot: bigint, | ||
| maxIndex: bigint = (1n << BigInt(MERKLE_TREE_DEPTH)) - 1n | ||
| ): bigint | null { |
There was a problem hiding this comment.
Missing validation for maxIndex parameter. The function should validate that maxIndex >= 0 to prevent unexpected behavior with negative values. Although BigInt comparisons with negative values work, it's better to be explicit about the expected input range.
| ): bigint | null { | |
| ): bigint | null { | |
| if (maxIndex < 0n) { | |
| throw new Error(`maxIndex must be non-negative, got ${maxIndex}`); | |
| } |
| epoch: Uint8Array, | ||
| rateLimit: number, | ||
| messageId: number // number of message sent by the user in this epoch | ||
| ): Promise<Uint8Array> { |
There was a problem hiding this comment.
Missing validation for x parameter. The function should validate that x.length === 32 since it represents a SHA-256 hash output. Incorrect length could lead to malformed witness data.
| ): Promise<Uint8Array> { | |
| ): Promise<Uint8Array> { | |
| if (x.length !== 32) { | |
| throw new Error("x must be a 32-byte SHA-256 hash"); | |
| } |
| const proofElementIndexes = extractPathDirectionsFromProof( | ||
| merkleProof, | ||
| rateCommitment, | ||
| merkleRoot |
There was a problem hiding this comment.
The function call to extractPathDirectionsFromProof does not pass the known membershipIndex as maxIndex parameter, which means it could potentially iterate through up to 2^20 (1,048,576) indices if the proof doesn't match early on. This could cause performance issues and test timeouts.
Consider passing the known index or a reasonable upper bound: extractPathDirectionsFromProof(merkleProof, rateCommitment, merkleRoot, membershipIndex + 100n) to limit the search space.
| merkleRoot | |
| merkleRoot, | |
| membershipIndex + 100n |
| epoch: Uint8Array, | ||
| rateLimit: number, | ||
| messageId: number // number of message sent by the user in this epoch | ||
| ): Promise<Uint8Array> { |
There was a problem hiding this comment.
Missing validation for epoch parameter. The function should validate that epoch.length === 32 to ensure it represents a properly formatted epoch value. Incorrect length could lead to invalid external nullifier calculation.
| ): Promise<Uint8Array> { | |
| ): Promise<Uint8Array> { | |
| if (!(epoch instanceof Uint8Array) || epoch.length !== 32) { | |
| throw new Error("epoch must be a 32-byte Uint8Array"); | |
| } |
| if (epoch.length !== 32) throw new Error("invalid epoch"); | ||
| if (idSecretHash.length !== 32) throw new Error("invalid id secret hash"); |
There was a problem hiding this comment.
Error messages "invalid epoch" and "invalid id secret hash" are not descriptive enough. They should specify the expected length. Consider: "Epoch must be 32 bytes, got ${epoch.length}" and "ID secret hash must be 32 bytes, got ${idSecretHash.length}"
| if (epoch.length !== 32) throw new Error("invalid epoch"); | |
| if (idSecretHash.length !== 32) throw new Error("invalid id secret hash"); | |
| if (epoch.length !== 32) throw new Error(`Epoch must be 32 bytes, got ${epoch.length}`); | |
| if (idSecretHash.length !== 32) throw new Error(`ID secret hash must be 32 bytes, got ${idSecretHash.length}`); |
| function extractIndexFromProof( | ||
| proof: readonly bigint[], | ||
| leafValue: bigint, | ||
| expectedRoot: bigint, | ||
| maxIndex: bigint = (1n << BigInt(MERKLE_TREE_DEPTH)) - 1n | ||
| ): bigint | null { | ||
| // Try different indices to see which one produces the expected root | ||
| for (let index = 0n; index <= maxIndex; index++) { | ||
| try { | ||
| const reconstructedRoot = reconstructMerkleRoot(proof, index, leafValue); | ||
| if (reconstructedRoot === expectedRoot) { | ||
| return index; | ||
| } | ||
| } catch (error) { | ||
| // Continue trying other indices if reconstruction fails | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Performance issue: extractIndexFromProof has O(2^20) time complexity by default, trying over 1 million indices sequentially. This is extremely inefficient and could cause significant delays or timeouts.
The function attempts to brute-force find the correct index by trying every possible value from 0 to 2^20-1 (1,048,576 iterations). In the test, this is avoided by passing the known membershipIndex, but the default behavior is problematic.
Consider either:
- Making
maxIndexa required parameter rather than optional with a large default - Adding a reasonable upper bound (e.g., 10,000) for when the index is unknown
- Documenting that this function should only be used when the approximate index range is known
Problem / Description
The RLN contracts and zerokit libraries have been updated to work in a stateless manner, where maintaining a local merkle tree of memberships is no longer necessary; instead, the smart contracts provide a function for getting the latest root of the on-chain tree along with a proof for a given identity commitment in that tree.
The RLN package needs to be updated to use this functionality for generating and verifying RLN proofs in the browser.
Solution
Notes
Checklist