chore: increase max vote allowed#1103
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughThe PR increases HALF_LARGEST_MINIMUM_DEGREE from 28 to 50 in CRISP circuit utilities and the SDK, updates MAXIMUM_VOTE_VALUE accordingly, adjusts test vectors, and replaces the verifier key data (VK_HASH and verification key G1 points) in the CRISP verifier contract. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
29-29: Good security documentation, but unrelated to PR scope.The @notice about signature reuse is a valuable security reminder. However, it's unrelated to the main PR objective of increasing the maximum vote value. Consider whether this should be in a separate commit for cleaner history.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/CRISP/circuits/src/utils.nr(4 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/constants.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/src/constants.ts (1)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
MAXIMUM_VOTE_VALUE(9-9)
⏰ 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). (10)
- GitHub Check: crisp_unit
- GitHub Check: build_e3_support_dev
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: integration_prebuild
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: build_sdk
🔇 Additional comments (6)
examples/CRISP/circuits/src/utils.nr (4)
61-61: Constant update consistent with check_coefficient_values_with_balance.The same HALF_LARGEST_MINIMUM_DEGREE increase ensures both functions validate the same coefficient window.
92-92: Test coefficient array correctly expanded.The array now has 100 elements (50 per half) with padding zeros added to accommodate the new window size. The test validates that binary
111(value 7) is within the balance of 1000.Also applies to: 95-95
168-168: Test array correctly reflects expanded window boundaries.The array expansion maintains test validity by properly positioning the non-zero coefficient at index 50 (YES section boundary).
Also applies to: 171-171
15-16: The 50-bit vote window expansion is mathematically sound and safely fits within u64 storage.The change correctly supports vote values up to 2^50 (1,125,899,906,842,624) for both yes and no outcomes, well within the u64 bounds (2^64 - 1). The polynomial coefficients are properly validated with the assertion
0 == coeff * (q_mod_t - coeff), ensuring only binary values (0 or q_mod_t) are accepted.No action needed on BFV compatibility—the BFV encryption degree parameter (512, 1024, 2048) is independent of this 50-bit vote window used for coefficient validation. However, note that existing proofs generated with the previous vote encoding will not be compatible with this change, as the bit positions and validation windows have shifted.
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
18-18: Constant increase drives maximum vote value expansion.The change correctly updates HALF_LARGEST_MINIMUM_DEGREE from 28 to 50, which drives the maximum vote value from ~268 million (2^28 - 1) to ~1.125 quadrillion (2^50 - 1). The minimum BFV degree correspondingly increases from 56 to 100 to support the larger range. Verify that all usages of these constants and the BFV implementation support the expanded degree and vote value parameters.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
75-96: Test data correctly expanded to match new HALF_LARGEST_MINIMUM_DEGREE value of 50.The tally array in the
decodeTallytest contains 97 zero entries, maintaining the expected test assertions ofyes=22nandno=1n. The expansion preserves the decoded values and aligns with the constant change from 28 to 50 (delta of 22 entries).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (1)
14-129: Add an automated "VK points are on-curve" check to prevent bad key material merges.The
Honk.VerificationKeystruct loaded inloadVerificationKey()(lines 14–129) contains hardcoded G1Point coordinates with no on-curve validation. While avalidateOnCurve()function exists in this file (line 1827) and is used to validate proof points, it is not applied to the VK literals themselves. A single typo in any coordinate would only be caught as an on-chain revert during verification—a late signal. Adding a compile-time or deploy-time check to validate all VK points against the curve equationy² ≡ x³ + 3 (mod Q)would catch transcription errors earlier and prevent corrupted key material from being merged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol(2 hunks)
⏰ 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). (10)
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: crisp_unit
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: build_ciphernode_image
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
2 participants
We need to be able to support larger values due to token supplies, unless we want to lose precision by scaling down.
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.