chore: check vote is zero for masking#938
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdded a coefficient-zero check and expanded ciphertext-addition initialization in the Noir circuit; implemented Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Circuit as main.nr
participant Utils as utils.nr
Client->>Circuit: submit inputs (k1, merkle_proof, signature, slot_address, prev_cts, sums, ...)
Circuit->>Circuit: verify signature & merkle -> set is_greco_valid
alt signature & merkle valid
Circuit->>Circuit: assert(is_greco_valid)
Circuit->>Circuit: initialize ct_add (prev_ct*/sum_*)
Circuit->>Circuit: compute & assert is_ct_add_valid
Circuit->>Utils: check_coefficient_values(k1, q_mod_t)
Utils-->>Circuit: ok
else invalid signature/merkle/root/address
Circuit->>Utils: check_coefficient_zero(k1)
Utils-->>Circuit: ok
Circuit->>Circuit: assert(is_ct_add_valid)
end
Circuit-->>Client: constraints satisfied / proof produced
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ctrlc03
force-pushed
the
feat/check-vote-is-zero-noir
branch
from
8d63743 to
bba84ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
26-29: Consider parameterizing the message for production use.The comment describes this as a "mock message," but it's used in production code (
generateMaskVote). If this function is intended for production use, consider:
- Making the message include the actual round number dynamically
- Or clarifying that
generateMaskVoteis test-onlyCurrently, the message hardcodes "round 0" which may not match the actual round being masked.
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
243-244: Hardcoded Merkle proof values suggest test-only usage.The Merkle proof indices and siblings are hardcoded to arrays of zeros, which would fail verification in production. Combined with the hardcoded private key (see separate comment), this strongly suggests
generateMaskVoteis intended for testing only.Consider:
- Moving this function to test utilities, OR
- Accepting real Merkle proof parameters if this needs to work in production
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/CRISP/circuits/src/main.nr(4 hunks)examples/CRISP/circuits/src/utils.nr(4 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(7 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/main.nrexamples/CRISP/circuits/src/utils.nr
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-08-27T14:02:25.412Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T14:02:25.412Z
Learning: In CRISP circuits using Greco library, k1 represents a binary value encoded as a polynomial where only one specific coefficient (k1.coefficients[2048 - 1]) carries the actual binary information and needs the constraint `assert(0 == b * (qmt - b))`. Other coefficients in the polynomial are not significant and don't require the binary constraint.
Applied to files:
examples/CRISP/circuits/src/utils.nr
🧬 Code graph analysis (4)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
MESSAGE(11-11)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (6)
testAddress(32-32)merkleProof(33-33)VOTE(14-14)MESSAGE(11-11)votingPowerLeaf(31-31)LEAVES(16-27)examples/CRISP/packages/crisp-sdk/src/vote.ts (5)
generateMaskVote(211-250)encodeVote(64-90)encryptVoteAndGenerateCRISPInputs(163-199)generateProof(252-260)verifyProof(262-266)examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/utils.ts (2)
hashLeaf(18-20)generateMerkleProof(39-79)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
generateMerkleProof(39-79)
🪛 Gitleaks (8.28.0)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[high] 232-232: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: crisp_e2e
- GitHub Check: rust_integration
🔇 Additional comments (9)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
216-216: LGTM! Parameter addition is well-integrated.The
slotAddressfield is properly added as a required parameter and is consistently used across the SDK functions and tests.examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
65-65: Good consistency improvement.Using
proof.indexinstead of the localindexvariable improves consistency since all other values are derived from theproofobject. Both values should be identical, but this makes the code more cohesive.examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
32-33: LGTM! Improved test maintainability.Extracting
testAddressas a constant and reusing it inmerkleProofgeneration makes the test data more maintainable and reduces duplication.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
141-141: LGTM! Consistent parameter usage.The
slotAddressparameter is correctly passed through to match the updated function signature.
172-172: LGTM! Consistent parameter usage.The
testAddressparameter is correctly passed togenerateMaskVotematching the updated function signature.
201-227: LGTM! Test now uses real cryptographic values.The test has been properly updated to use a real signer and dynamically generated Merkle proof, which provides better test coverage than placeholder values.
examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
10-10: LGTM! Required import for message constant.The MESSAGE constant import is needed for the signing flow in
generateMaskVote.
15-15: LGTM! Required import for account generation.The
privateKeyToAccountimport is needed for the signing flow, though the usage requires attention (see separate comment on lines 231-235).
163-198: LGTM! Function signature properly extended.The function now correctly accepts and propagates
slotAddress,merkleData,message,signature, andbalanceparameters, properly populating the CRISP circuit inputs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
204-210: Extract hardcoded private key to a constant.The Hardhat default private key is duplicated here and in the
generateMaskVotesource function (line 222 of vote.ts). Extract it to a shared test constant to improve maintainability and avoid duplication.In
tests/constants.ts, add:export const HARDHAT_DEFAULT_PRIVATE_KEY = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"Then update this test:
- // hardhat default private key - const privateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" - const account = privateKeyToAccount(privateKey) + const account = privateKeyToAccount(HARDHAT_DEFAULT_PRIVATE_KEY)Note: The same constant should also replace the hardcoded key in
src/vote.ts:222.examples/CRISP/circuits/src/utils.nr (2)
89-115: Test coverage could include cancellation edge case.The tests cover the basic pass and fail scenarios but don't verify the edge case where coefficients might cancel out to sum to zero while being individually non-zero. While this may be difficult to construct in Noir depending on Field literal handling, consider documenting why the sum check is sufficient (e.g., due to range constraints elsewhere) or adding a test comment explaining the assumption.
34-59: Consider extracting shared constants to reduce duplication.The constants
HALF_LARGEST_MINIMUM_DEGREE,HALF_D,START_INDEX_Y, andSTART_INDEX_Nare duplicated betweencheck_coefficient_valuesandcheck_coefficient_zero. While this duplication is minor, extracting them as module-level constants could improve maintainability:const HALF_LARGEST_MINIMUM_DEGREE: u32 = 28; fn compute_start_indices<let D: u32>() -> (u32, u32) { let HALF_D = D / 2; let START_INDEX_Y = HALF_D - HALF_LARGEST_MINIMUM_DEGREE; let START_INDEX_N = D - HALF_LARGEST_MINIMUM_DEGREE; (START_INDEX_Y, START_INDEX_N) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/CRISP/circuits/src/main.nr(3 hunks)examples/CRISP/circuits/src/utils.nr(2 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/CRISP/circuits/src/main.nr
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/utils.nr
🧬 Code graph analysis (1)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (6)
testAddress(32-32)merkleProof(33-33)VOTE(14-14)MESSAGE(11-11)votingPowerLeaf(31-31)LEAVES(16-27)examples/CRISP/packages/crisp-sdk/src/vote.ts (5)
generateMaskVote(211-250)encodeVote(64-90)encryptVoteAndGenerateCRISPInputs(163-199)generateProof(252-260)verifyProof(262-266)examples/CRISP/packages/crisp-sdk/src/utils.ts (2)
hashLeaf(18-20)generateMerkleProof(39-79)
🪛 Gitleaks (8.28.0)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
[high] 205-205: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
20-23: LGTM!The new imports are appropriately used for dynamic Merkle proof generation and signing logic in the updated tests.
141-141: LGTM!The
slotAddressparameter is correctly provided to match the updated function signature.
172-172: LGTM!The
testAddressparameter is correctly passed togenerateMaskVotematching the updated function signature.
201-201: Good improvement on timeout reduction.The timeout has been reduced from 1000000ms to 100000ms, addressing the previous review feedback.
examples/CRISP/circuits/src/utils.nr (1)
34-59: Sum check is mathematically safe given the binary constraint applied first.The concern about field cancellation does not apply here. Before
check_coefficient_zeroruns,check_coefficient_valuesconstrains all relevant coefficients to either 0 or q_mod_t viaassert(0 == coeff * (q_mod_t - coeff)). Given this constraint and assuming q_mod_t and the field prime are coprime (standard for BFV parameters), the sum check correctly verifies that all constrained coefficients are zero.Minor: Extract duplicated constant definitions.
Both
check_coefficient_valuesandcheck_coefficient_zeroduplicate HALF_LARGEST_MINIMUM_DEGREE, HALF_D, START_INDEX_Y, and START_INDEX_N. Consider extracting these to a shared constant or helper function to reduce duplication.
Note from learnings: Retrieved context from PR 648 indicates that binary constraints on k1 should apply only to specific coefficients (e.g., k1.coefficients[2048-1]), not to all coefficients in the ranges. Verify whether the current range-based approach in
check_coefficient_valuesaligns with the intended circuit design.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
229-245: Add negative test case to verify zero-vote validation per PR objective.This test covers the happy path (masking with zero vote passes). However, the PR objective (#937) is to "Add validation to ensure that the vote used for masking is equal to 0." To properly verify this validation works, add a negative test that attempts to create a mask vote with a non-zero coefficient and confirms it's rejected.
Add a test like:
it('should fail proof generation when masking vote is non-zero', { timeout: 180000 }, async () => { // Create a non-zero vote for masking (invalid scenario) const invalidVote = { yes: 1n, no: 0n } const encodedVote = encodeVote(invalidVote, VotingMode.GOVERNANCE, 1n) const zkInputsGenerator: ZKInputsGenerator = new ZKInputsGenerator( DEFAULT_BFV_PARAMS.degree, DEFAULT_BFV_PARAMS.plaintextModulus, DEFAULT_BFV_PARAMS.moduli, ) const vote = BigInt64Array.from(encodedVote.map(BigInt)) const encryptedVote = zkInputsGenerator.encryptVote(publicKey, vote) const maskVote = await generateMaskVote(publicKey, encryptedVote, DEFAULT_BFV_PARAMS, merkleProof.proof.root, testAddress) // Should fail during proof generation due to circuit validation await expect(generateProof(maskVote)).rejects.toThrow() })Note: This assumes the circuit enforces the zero-vote constraint. If validation is only in SDK, adjust accordingly.
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
231-235: CRITICAL: Hardcoded private key remains unresolved.This critical security issue was previously flagged but remains in the code. The hardcoded Hardhat development key in production code creates severe risks:
- Anyone can sign messages as this account
- Masking signatures can be forged by any attacker
- Defeats the purpose of signature verification
While the learnings indicate this key is acceptable in template README files for local testing, this is production code in
src/vote.ts.The previous review suggested three remediation options. Please implement one of them:
Option 1 (Recommended): Move to test utilities
// Move to tests/utils.ts or tests/helpers.ts export const generateMaskVoteForTesting = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, slotAddress: string, ): Promise<CRISPCircuitInputs> => { // Implementation with hardcoded key acceptable here }Option 2: Accept signer parameter
export const generateMaskVote = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, + signer: PrivateKeyAccount, bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, slotAddress: string, ): Promise<CRISPCircuitInputs> => { // ... - const privateKey = '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' - const account = privateKeyToAccount(privateKey) - const signature = await account.signMessage({ message: MESSAGE }) + const signature = await signer.signMessage({ message: MESSAGE }) // ... }
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
201-227: Timeout improved but could be further optimized.The timeout has been reduced from 1000000ms to 180000ms (3 minutes), which is better. However, past reviews suggested 20-100 seconds. The test logic with signer-derived data looks correct.
Consider reducing the timeout further if proof generation completes faster:
- it('should generate a proof for a voter and verify it', { timeout: 180000 }, async () => { + it('should generate a proof for a voter and verify it', { timeout: 100000 }, async () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts(6 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/CRISP/packages/crisp-sdk/tests/constants.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/vote.ts
🧬 Code graph analysis (2)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (6)
testAddress(32-32)merkleProof(33-33)VOTE(14-14)MESSAGE(11-11)votingPowerLeaf(31-31)LEAVES(16-27)examples/CRISP/packages/crisp-sdk/src/vote.ts (5)
generateMaskVote(211-250)encodeVote(64-90)encryptVoteAndGenerateCRISPInputs(163-199)generateProof(252-260)verifyProof(262-266)examples/CRISP/packages/crisp-sdk/src/utils.ts (2)
hashLeaf(18-20)generateMerkleProof(39-79)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
🪛 Gitleaks (8.28.0)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[high] 232-232: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: test_contracts
- GitHub Check: rust_unit
🔇 Additional comments (6)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
20-23: LGTM: Import changes properly reflect new test dependencies.The imports have been correctly updated to include Merkle proof utilities and account signing from viem, which are necessary for the signer-derived test data flow.
141-141: LGTM: slotAddress parameter correctly added.The
slotAddressparameter is properly passed to match the updated function signature.
172-172: LGTM: slotAddress parameter correctly added.The
testAddressparameter is properly passed to match the updatedgenerateMaskVotesignature.examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
163-199: LGTM! Parameter addition and usage is correct.The
slotAddressparameter is properly accepted and used to populate theslot_addressfield in the returned CRISP circuit inputs.
218-221: LGTM! Correctly implements zero vote for masking.The mask vote is properly initialized with
yes: 0nandno: 0n, which aligns with the PR objective to validate that the masked vote is equal to 0.
243-246: Verify placeholder Merkle proof values are valid for mask votes.The function uses placeholder zeros for
merkle_proof_indicesandmerkle_proof_siblingswhile using the actualmerkleRootparameter. This mixed approach may cause issues:
- If the circuit validates Merkle proofs even for mask votes (zero votes), these placeholder values will fail validation
- The
merkle_proof_lengthis set to '1', but both arrays have 20 elementsPlease verify whether the circuit skips Merkle proof validation for mask votes or if valid proof data is required. If validation is required, consider accepting Merkle proof data as parameters:
export const generateMaskVote = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, merkleProofIndices: string[], merkleProofSiblings: string[], slotAddress: string, ): Promise<CRISPCircuitInputs> => { // ... return { ...crispInputs, // ... merkle_proof_indices: merkleProofIndices, merkle_proof_siblings: merkleProofSiblings, merkle_proof_length: merkleProofIndices.length.toString(), merkle_root: merkleRoot.toString(), // ... } }
ctrlc03
force-pushed
the
feat/check-vote-is-zero-noir
branch
from
57ff1de to
37337aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
examples/CRISP/circuits/src/main.nr (1)
106-129: Assert the coefficient checks so the circuit actually enforces themBoth
check_coefficient_values(line 117) andcheck_coefficient_zero(line 122) returnbool, but their results are dropped. In Noir, boolean expressions only become constraints when youassertthem. Without assertions:
- A prover can choose any
k1polynomial with invalid coefficients or non-zero vote- The functions will return
false, but the circuit will still pass- This defeats the PR objective (#937) to validate the masked vote is zero
This is a critical security issue that regresses both the existing binary check and the new zero-vote validation.
Apply this diff to enforce the constraints:
// Verify the correct coefficient values. - check_coefficient_values(k1, params.crypto_params().q_mod_t); + assert(check_coefficient_values(k1, params.crypto_params().q_mod_t)); (ct0is, ct1is) } else { // check if vote == 0. - check_coefficient_zero(k1); + assert(check_coefficient_zero(k1));examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
231-235: CRITICAL: Hardcoded private key remains in production code.The hardcoded Hardhat development private key is still present in production source code (
src/vote.ts). This is a critical security vulnerability as anyone can forge signatures using this key, undermining the entire signature verification mechanism.While the learning notes indicate this key is acceptable in template README files and documentation,
src/vote.tsis production source code, not documentation.Recommended solutions:
If
generateMaskVoteis test-only: Move it totests/utils.tsor similar test helper file.If it must remain in production: Accept a signer parameter:
export const generateMaskVote = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, + signer: PrivateKeyAccount, bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, slotAddress: string, ): Promise<CRISPCircuitInputs> => { // ... - const privateKey = '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' - const account = privateKeyToAccount(privateKey) - const signature = await account.signMessage({ message: MESSAGE }) + const signature = await signer.signMessage({ message: MESSAGE }) - const { hashed_message, pub_key_x, pub_key_y, signature: extractedSignature } = await extractSignature(MESSAGE, signature) + const { hashed_message, pub_key_x, pub_key_y, signature: extractedSignature } = await extractSignature(MESSAGE, signature) // ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/CRISP/circuits/src/main.nr(3 hunks)examples/CRISP/circuits/src/utils.nr(2 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(6 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/CRISP/circuits/src/utils.nr
- examples/CRISP/packages/crisp-sdk/src/types.ts
- examples/CRISP/packages/crisp-sdk/src/utils.ts
- examples/CRISP/packages/crisp-sdk/src/constants.ts
- examples/CRISP/packages/crisp-sdk/tests/constants.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/vote.ts
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.tsexamples/CRISP/circuits/src/main.nr
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2025-08-27T14:02:25.412Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T14:02:25.412Z
Learning: In CRISP circuits using Greco library, k1 represents a binary value encoded as a polynomial where only one specific coefficient (k1.coefficients[2048 - 1]) carries the actual binary information and needs the constraint `assert(0 == b * (qmt - b))`. Other coefficients in the polynomial are not significant and don't require the binary constraint.
Applied to files:
examples/CRISP/circuits/src/main.nr
🧬 Code graph analysis (2)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (6)
testAddress(32-32)merkleProof(33-33)VOTE(14-14)MESSAGE(11-11)votingPowerLeaf(31-31)LEAVES(16-27)examples/CRISP/packages/crisp-sdk/src/vote.ts (5)
generateMaskVote(211-250)encodeVote(64-90)encryptVoteAndGenerateCRISPInputs(163-199)generateProof(252-260)verifyProof(262-266)examples/CRISP/packages/crisp-sdk/src/utils.ts (2)
hashLeaf(18-20)generateMerkleProof(39-79)
🪛 Gitleaks (8.28.0)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[high] 232-232: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
[high] 205-205: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test_contracts
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: build_e3_support_risc0
- GitHub Check: rust_unit
- GitHub Check: rust_integration
🔇 Additional comments (6)
examples/CRISP/circuits/src/main.nr (1)
17-17: LGTM!The import of
check_coefficient_zerois necessary for the new validation logic in the else branch.examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
10-10: LGTM: Imports support new cryptographic functionality.The new imports (
MESSAGEandprivateKeyToAccount) are necessary for the signer-derived data flow ingenerateMaskVote.Also applies to: 15-15
156-198: LGTM: Signature expansion supports real cryptographic data flow.The function now accepts and properly integrates merkle proof data, signature components, balance, and slot address into the circuit inputs, aligning with the PR's objective to validate masked votes with real cryptographic parameters.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
20-20: LGTM: Updated imports support new test requirements.The imports now include merkle proof utilities, test constants, and account management functions needed for the enhanced cryptographic data flow.
Also applies to: 22-23
141-141: LGTM: Tests updated to pass required slotAddress parameter.The test calls correctly provide the
slotAddressparameter to align with the updated function signatures.Also applies to: 172-172
201-227: LGTM: Test properly exercises voter proof with real cryptographic data.The timeout has been appropriately reduced to 180000ms (from the excessive 1000000ms in earlier versions). The use of the hardcoded Hardhat development private key in test code is acceptable for local testing purposes per established learnings.
The test now properly exercises the full cryptographic flow with signer-derived merkle proofs and signatures.
cedoor
force-pushed
the
feat/check-vote-is-zero-noir
branch
from
37337aa to
325f13f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
examples/CRISP/circuits/src/main.nr (2)
117-117: Assert the coefficient value check to enforce the constraint.The function
check_coefficient_valuesreturns a boolean, but the result is not asserted. In Noir, boolean values only become enforced constraints when wrapped withassert(). Without this, a prover can provide invalid coefficient values and the circuit will accept them.Apply this diff to enforce the constraint:
- check_coefficient_values(k1, params.crypto_params().q_mod_t); + assert(check_coefficient_values(k1, params.crypto_params().q_mod_t));
121-122: Assert the zero-vote check to enforce the constraint.The function
check_coefficient_zeroreturns a boolean, but the result is not asserted. In Noir, boolean values only become enforced constraints when wrapped withassert(). Without this assertion, a prover can submit a non-zero vote in the masking case (ineligible voter) and the circuit will accept it. This defeats the entire objective of PR #937.Apply this diff to enforce the constraint:
- // check if vote == 0. - check_coefficient_zero(k1); + // check if vote == 0. + assert(check_coefficient_zero(k1));examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
231-235: CRITICAL: Hardcoded private key in production code.A hardcoded private key (Hardhat's default development account) is embedded in production source code. This is a severe security vulnerability:
- Compromised signatures: Anyone can sign messages as this account, defeating signature verification
- Production exposure: Unlike test utilities, this exported function may be called in production
- Integrity risk: Masking signatures can be forged by any attacker
The function also uses placeholder Merkle proof values (all zeros), suggesting it's intended for testing only.
Recommended fix: Move this function to test utilities or refactor to accept a signer parameter:
Option 1 (preferred): Move to test utilities
Move
generateMaskVotetotests/utils.tsor similar:// In tests/utils.ts or tests/helpers.ts export const generateMaskVoteForTesting = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, slotAddress: string, ): Promise<CRISPCircuitInputs> => { // ... implementation (hardcoded key acceptable in test utilities) }Option 2: Accept signer parameter
import type { PrivateKeyAccount } from 'viem/accounts' export const generateMaskVote = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, signer: PrivateKeyAccount, // Pass signer from caller bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, slotAddress: string, ): Promise<CRISPCircuitInputs> => { // ... const signature = await signer.signMessage({ message: MESSAGE }) // ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/CRISP/circuits/src/main.nr(3 hunks)examples/CRISP/circuits/src/utils.nr(2 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(6 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/packages/crisp-sdk/src/types.ts
- examples/CRISP/circuits/src/utils.nr
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.tsexamples/CRISP/circuits/src/main.nr
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2025-08-27T14:02:25.412Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T14:02:25.412Z
Learning: In CRISP circuits using Greco library, k1 represents a binary value encoded as a polynomial where only one specific coefficient (k1.coefficients[2048 - 1]) carries the actual binary information and needs the constraint `assert(0 == b * (qmt - b))`. Other coefficients in the polynomial are not significant and don't require the binary constraint.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/vote.ts
🧬 Code graph analysis (4)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
MESSAGE(11-11)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
generateMerkleProof(39-79)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (6)
testAddress(32-32)merkleProof(33-33)VOTE(14-14)MESSAGE(11-11)votingPowerLeaf(31-31)LEAVES(16-27)examples/CRISP/packages/crisp-sdk/src/vote.ts (5)
generateMaskVote(211-250)encodeVote(64-90)encryptVoteAndGenerateCRISPInputs(163-199)generateProof(252-260)verifyProof(262-266)examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/utils.ts (2)
hashLeaf(18-20)generateMerkleProof(39-79)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
🪛 Gitleaks (8.28.0)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[high] 232-232: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: build_e3_support_risc0
🔇 Additional comments (11)
examples/CRISP/circuits/src/main.nr (3)
17-17: LGTM: Import added for the new zero-check function.The import is correctly added to support the new coefficient zero validation.
106-108: LGTM: Greco validation is now unconditionally enforced.Moving the assertion outside the conditional logic ensures that the ciphertext encryption is always validated, which is the correct behavior.
127-129: LGTM: Ciphertext addition validation is correctly enforced.The assertion on
is_ct_add_validcorrectly enforces the constraint in the else branch when outputting the sum of ciphertexts.examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
65-65: LGTM: Usingproof.indeximproves clarity.The change from
indextoproof.indexis appropriate since the indices should be derived from the proof object's index rather than the input parameter. This makes the code more self-documenting and ensures consistency with the proof structure.examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
26-29: LGTM: Mock message constant added for masking.The MESSAGE constant is clearly documented as a mock value for masking signatures. This constant is used in the
generateMaskVotefunction for test/development purposes.examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
32-33: LGTM: Good refactor extracting test address.Extracting the hardcoded address into a named
testAddressconstant improves test maintainability and enables reuse across test files.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
20-23: LGTM: Enhanced test utilities imported.The new imports (
generateMerkleProof,hashLeaf,privateKeyToAccount) enable tests to construct signer-derived data and Merkle proofs directly, improving test coverage and realism.
201-227: LGTM: Voter test enhanced with signer-derived data.The test now constructs realistic signer-derived data (private key, account, signature, leaf, merkle proof) and passes
slotAddressthrough the voting flow. The 180-second timeout is reasonable for proof generation/verification.examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
10-10: LGTM: MESSAGE constant imported for masking.The MESSAGE import supports the masking signature workflow in
generateMaskVote.
163-198: LGTM: slotAddress parameter added for enhanced circuit inputs.The addition of the
slotAddressparameter enables the circuit to validate voter slot addresses. The parameter is properly threaded through to the circuit inputs and documented in the JSDoc.
243-244: Placeholder Merkle proof values suggest test-only usage.The function generates placeholder Merkle proof arrays (all zeros, length 20) which are not valid for production use. This reinforces that
generateMaskVoteshould be moved to test utilities or refactored to accept real Merkle proof data.
2 participants
fix #937
Summary by CodeRabbit
New Features
Bug Fixes
Tests