refactor: use commitments for ciphertext in crisp [skip-line-limit]#1156
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds BFV→Greco conversion and a 32-byte ciphertext commitment API, integrates commitment computation into Merkle leaf hashing and ZK input generation, updates CRISP circuits to use/verify commitments, and propagates encryptedVote and commitment fields through SDK, WASM, contracts, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as SDK / Client
participant ZKGen as ZKInputs Generator
participant BFV as BFV Helpers
participant Circuit as CRISP Circuit
participant Verifier as Smart Contract Verifier
SDK->>ZKGen: generate_inputs(vote_bytes, prev_ct_bytes?)
ZKGen->>BFV: compute_ct_commitment(ciphertext_bytes, params)
BFV->>BFV: bfv_ciphertext_to_greco(ct)
BFV->>BFV: compute_poly_commitment(...)
BFV-->>ZKGen: ct_commitment (32-byte)
ZKGen-->>SDK: (ciphertext_bytes, circuit_inputs + ct_commitment, encryptedVote)
SDK->>Circuit: submit circuit_inputs (includes ct_commitment, prev_ct_commitment)
Circuit->>Circuit: generate_commitment(ct0is, ct1is)
Circuit->>Circuit: Fiat-Shamir challenge using commitments
Circuit-->>SDK: returnValue = ct_commitment (Field)
SDK->>Verifier: submitProof(proof, publicInputs..., encryptedVote)
Verifier->>Verifier: verify proof using public inputs (includes commitments)
Verifier-->>SDK: tx success / state update (store ct_commitment)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
cedoor
force-pushed
the
refactor/ct-commitment
branch
from
7da7ac2 to
c4194f2
Compare
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
cedoor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/CRISP/circuits/src/ciphertext_addition.nr (1)
198-217: Use commitment-based Fiat-Shamir with explicit security assumptions.The approach is sound under the Random Oracle Model assuming: (1) the Poseidon2 hash with SAFE API provides a binding commitment scheme (collision-resistant over BN254), (2) commitments are computed from full polynomial data before challenge generation (which they are), and (3) the underlying sigma-protocol has special soundness. The distinct domain separators ("Greco" and "CiphertextAddition") and proper SAFE sponge structure support these requirements. This reduces the challenge input from N*L polynomial coefficients to 3 commitments, improving circuit efficiency without compromising soundness.
Document the security assumptions explicitly: specify that the proof holds in the classical Random Oracle Model (not post-quantum QROM), and confirm that Poseidon2 parameters for BN254 match the target security level.
🤖 Fix all issues with AI agents
In @crates/bfv-helpers/src/client.rs:
- Around line 190-198: compute_ct_commitment is missing a length check before
padding: validate that commitment_bigint.to_bytes_be().1 (the local bytes) has
length <= 32 and return an Err (e.g., anyhow!("Commitment too large")) if not,
before allocating padded_bytes and computing start_idx; mirror the same
bytes.len() <= 32 check used in compute_pk_commitment so start_idx = 32 -
bytes.len() cannot underflow and the subsequent padded_bytes -> ciphertext_hash
conversion remains safe.
🧹 Nitpick comments (9)
examples/CRISP/program/src/lib.rs (1)
17-18: Consider propagating deserialization errors instead of panicking.Using
.unwrap()onCiphertext::from_byteswill panic if the ciphertext bytes are malformed or corrupted. While this aligns with the current temporary approach for BFV utilities (based on learnings), consider whether this example program should demonstrate more robust error handling for production use cases.If intentional for simplicity in the example, this is acceptable.
examples/CRISP/crates/zk-inputs/src/serialization.rs (1)
429-441: Consider adding test assertion forprev_ct_commitment.The existing test verifies required fields but doesn't explicitly check for the newly added
prev_ct_commitmentfield in the serialized JSON output.🔧 Suggested addition
assert!(parsed.get("pk_commitment").is_some()); + assert!(parsed.get("prev_ct_commitment").is_some()); } }crates/bfv-helpers/src/client.rs (2)
172-201: Consider adding a unit test forcompute_ct_commitment.The existing tests cover
bfv_encryptandbfv_verifiable_encryptbut there's no test for the newcompute_ct_commitmentfunction. Adding a test would validate the commitment computation and catch regressions.Do you want me to generate a unit test for
compute_ct_commitment?
186-186: Add a clarifying comment explaining whypk_boundsis used for ciphertext commitment bit width.The code uses
bounds.pk_bounds[0]to calculatebit_ctfor ciphertext commitment, which may appear inconsistent with naming. However,GrecoBoundsprovides unified system-level bounds rather than separate bounds for ciphertext. Thepk_boundsfield is intentionally shared for both public key and ciphertext commitments as part of Greco's overall bounds computation. Consider adding an inline comment (e.g.,// GrecoBounds uses unified bounds for both pk and ct commitments) to clarify this design choice for future readers.crates/compute-provider/src/merkle_tree_builder.rs (2)
38-51: Avoid cloningmodulion each iteration.The
moduli.clone()inside the loop creates a newVec<u64>allocation for every data item. Sincecompute_ct_commitmenttakes ownership of the moduli vector, consider restructuring to avoid this cost.♻️ Suggested optimization
If
compute_ct_commitmentcannot be changed to accept a slice, consider whether the repeated cloning is acceptable for the expected data sizes. Otherwise, the inner function signature could be adjusted to accept&[u64]instead ofVec<u64>.
45-50: Consider propagating errors instead of panicking.Using
.expect()loses the underlying error context fromcompute_ct_commitment. If this function is called in contexts where recovery is possible, consider changing the signature to returnResultand propagating the error.♻️ Suggested improvement
- pub fn compute_leaf_hashes(&mut self, data: &[(Vec<u8>, u64)], params_bytes: &[u8]) { + pub fn compute_leaf_hashes(&mut self, data: &[(Vec<u8>, u64)], params_bytes: &[u8]) -> Result<(), anyhow::Error> { let params = decode_bfv_params(params_bytes); // ... for item in data { let commitment = - compute_ct_commitment(item.0.clone(), degree, plaintext_modulus, moduli.clone()) - .expect("Failed to compute ciphertext commitment"); + compute_ct_commitment(item.0.clone(), degree, plaintext_modulus, moduli.clone())?; // ... } + Ok(()) }examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
130-183: Consider extracting duplicated array parsing logic.The parsing logic for
ct0is(lines 138-156) andct1is(lines 159-177) is nearly identical. Extracting to a helper function would reduce duplication and improve maintainability.♻️ Suggested helper function
fn parse_bigint_array(array: JsValue, name: &str) -> Result<Vec<Vec<BigInt>>, JsValue> { let js_array: js_sys::Array = js_sys::Array::from(&array); let mut result: Vec<Vec<BigInt>> = Vec::new(); for i in 0..js_array.length() { let inner_array = js_array .get(i) .dyn_into::<js_sys::Array>() .map_err(|_| JsValue::from_str(&format!("Expected array of arrays for {}", name)))?; let mut coefficients: Vec<BigInt> = Vec::new(); for j in 0..inner_array.length() { let s = inner_array .get(j) .as_string() .ok_or_else(|| JsValue::from_str("Expected string in inner array"))?; let bigint = s .parse::<BigInt>() .map_err(|e| JsValue::from_str(&format!("Failed to parse BigInt: {}", e)))?; coefficients.push(bigint); } result.push(coefficients); } Ok(result) }Then usage becomes:
let ct0is_vec = parse_bigint_array(ct0is, "ct0is")?; let ct1is_vec = parse_bigint_array(ct1is, "ct1is")?;examples/CRISP/circuits/src/hash.nr (1)
56-74: Unused generic parameterL.The generic parameter
Lingenerate_hash<let L: u32>is declared but not used in the function body. It appears to be carried over from the caller but serves no purpose here.♻️ Suggested fix
-pub fn generate_hash<let L: u32>( +pub fn generate_hash( inputs: Vec<Field>, domain_separator: [u8; 64], squeeze_count: u32, ) -> Vec<Field> {And update the call site:
- let hash = generate_hash::<L>(inputs, domain_separator, 1); + let hash = generate_hash(inputs, domain_separator, 1);examples/CRISP/crates/zk-inputs/src/lib.rs (1)
208-232: Clarify the semantic difference betweengenerate_inputsandgenerate_inputs_for_update.Both functions compute ciphertext addition inputs, but:
generate_inputssetsprev_ct_commitment = 0and returnsct(new vote)generate_inputs_for_updateuses the computed commitment and returnssum_ctThe comment on line 218 helps, but consider adding documentation to clarify when each function should be used (first vote vs. updating an existing slot).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.locktemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
crates/bfv-helpers/src/client.rscrates/bfv-helpers/src/utils/greco.rscrates/compute-provider/Cargo.tomlcrates/compute-provider/src/compute_manager.rscrates/compute-provider/src/merkle_tree_builder.rsexamples/CRISP/circuits/src/ciphertext_addition.nrexamples/CRISP/circuits/src/hash.nrexamples/CRISP/circuits/src/main.nrexamples/CRISP/crates/evm_helpers/src/lib.rsexamples/CRISP/crates/zk-inputs-wasm/Cargo.tomlexamples/CRISP/crates/zk-inputs-wasm/src/lib.rsexamples/CRISP/crates/zk-inputs/Cargo.tomlexamples/CRISP/crates/zk-inputs/src/ciphertext_addition.rsexamples/CRISP/crates/zk-inputs/src/lib.rsexamples/CRISP/crates/zk-inputs/src/serialization.rsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/sdk.tsexamples/CRISP/packages/crisp-sdk/src/types.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.tsexamples/CRISP/program/src/lib.rsexamples/CRISP/server/src/server/indexer.rs
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/compute-provider/Cargo.tomlexamples/CRISP/crates/zk-inputs-wasm/Cargo.toml
📚 Learning: 2025-12-23T17:18:32.343Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1142
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:141-147
Timestamp: 2025-12-23T17:18:32.343Z
Learning: When verifying Noir circuit proofs in Solidity, the public inputs array passed to the verifier must include both the circuit's public inputs AND public outputs. For example, if a Noir circuit has 3 public inputs and returns 2064 public output fields (-> pub [Field; 2064]), the verification array must contain all 2067 values (3 + 2064).
Applied to files:
examples/CRISP/packages/crisp-sdk/src/types.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.tsexamples/CRISP/circuits/src/main.nrexamples/CRISP/packages/crisp-sdk/src/vote.ts
📚 Learning: 2025-12-19T11:35:51.645Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 1109
File: examples/CRISP/server/src/server/routes/state.rs:33-37
Timestamp: 2025-12-19T11:35:51.645Z
Learning: In the CRISP voting system (examples/CRISP), ciphertext data and slot states are public and stored on-chain in the CRISPProgram smart contract. The server endpoints `/previous-ciphertext` and `/is-slot-empty` expose public blockchain data that can already be accessed by anyone through the contract's external view functions (getSlotIndex, isSlotEmptyByAddress) or by reading contract state directly. Authentication is not needed for these endpoints since they only expose data that is already publicly available on the blockchain.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/sdk.tsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/program/src/lib.rsexamples/CRISP/server/src/server/indexer.rsexamples/CRISP/crates/evm_helpers/src/lib.rsexamples/CRISP/crates/zk-inputs/src/lib.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
examples/CRISP/program/src/lib.rscrates/compute-provider/src/merkle_tree_builder.rscrates/bfv-helpers/src/client.rs
📚 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/program/src/lib.rsexamples/CRISP/crates/zk-inputs/src/lib.rscrates/bfv-helpers/src/utils/greco.rsexamples/CRISP/circuits/src/ciphertext_addition.nr
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
examples/CRISP/crates/zk-inputs/Cargo.tomlexamples/CRISP/crates/zk-inputs-wasm/Cargo.toml
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
examples/CRISP/server/src/server/indexer.rs
📚 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:
crates/bfv-helpers/src/client.rsexamples/CRISP/circuits/src/main.nrexamples/CRISP/crates/zk-inputs/src/lib.rscrates/bfv-helpers/src/utils/greco.rsexamples/CRISP/circuits/src/ciphertext_addition.nr
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol
📚 Learning: 2025-12-23T17:18:22.421Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1142
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:141-147
Timestamp: 2025-12-23T17:18:22.421Z
Learning: For Solidity contracts that verify Noir circuit proofs, the verifier's public inputs array must include both the circuit's public inputs and the public outputs. Ensure the verification input array length equals pub_inputs + pub_outputs. Example: a Noir circuit with 3 public inputs and 2064 public outputs should pass an array of length 2067 (3 + 2064). Apply this consistently across all contract verifier implementations and adjust tests accordingly.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
examples/CRISP/crates/evm_helpers/src/lib.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
examples/CRISP/crates/zk-inputs/src/lib.rscrates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2025-11-07T16:17:58.988Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 972
File: circuits/crates/libs/greco/src/lib.nr:192-192
Timestamp: 2025-11-07T16:17:58.988Z
Learning: In the Greco library (circuits/crates/libs/greco/src/lib.nr), the e1is array should satisfy the constraint `e1 mod qi == e1is[i]` for each CRT basis i, where qi are the CRT moduli. This constraint ensures the relationship between the global error polynomial e1 and its per-basis representations e1is[i].
Applied to files:
examples/CRISP/circuits/src/ciphertext_addition.nr
🧬 Code graph analysis (6)
examples/CRISP/program/src/lib.rs (2)
crates/bfv-helpers/src/lib.rs (1)
decode_bfv_params_arc(431-433)crates/trbfv/src/trbfv_config.rs (1)
params(34-36)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (2)
crates/bfv-helpers/src/lib.rs (1)
from(100-142)crates/bfv-helpers/src/client.rs (1)
compute_ct_commitment(172-201)
crates/compute-provider/src/merkle_tree_builder.rs (3)
crates/bfv-helpers/src/client.rs (1)
compute_ct_commitment(172-201)crates/bfv-helpers/src/lib.rs (1)
decode_bfv_params(347-411)crates/trbfv/src/trbfv_config.rs (1)
params(34-36)
crates/bfv-helpers/src/client.rs (1)
crates/bfv-helpers/src/utils/greco.rs (2)
bfv_ciphertext_to_greco(61-105)bfv_public_key_to_greco(118-168)
crates/compute-provider/src/compute_manager.rs (1)
crates/trbfv/src/trbfv_config.rs (1)
params(34-36)
examples/CRISP/crates/zk-inputs/src/lib.rs (1)
crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(278-300)
⏰ 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). (9)
- GitHub Check: build_crisp_sdk
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: build_ciphernode_image
- GitHub Check: build_enclave_cli
- GitHub Check: Build & Push Image
🔇 Additional comments (52)
examples/CRISP/crates/zk-inputs-wasm/Cargo.toml (1)
18-19: LGTM!The dependency additions are appropriate for the ciphertext commitment feature:
serde_json.workspace = truefollows workspace conventions for version management.num-bigint = "0.4.6"matches the version in the parentzk-inputscrate, maintaining consistency.examples/CRISP/crates/zk-inputs/Cargo.toml (1)
28-29: LGTM. Theark-bn254andark-ffversion0.5additions correctly align withgreco's dependencies.The
zkfhe-grecopackage (aliased asgreco) already requiresark-bn254 0.5.0andark-ff 0.5.0, and the PR additions match these versions exactly. The matching minor version across arkworks ecosystem crates ensures compatibility for the BN254 curve operations needed for ciphertext commitment hashing. No version conflicts exist.examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
97-97: Index shift aligns with the new commitment-based public inputs structure.The change from
[0]to[1]is correct—pk_commitment(the committee public key commitment) is now at index 1 afterprev_ct_commitmentwas added at index 0. Consider adding a brief comment to clarify this index corresponds topk_commitment, improving maintainability if the input order changes again.examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (2)
17-129: Verification key G1 points updated for new circuit.The verification key points have been regenerated to match the refactored circuit with ciphertext commitments. The
publicInputsSizeat Line 17 correctly matches theNUMBER_OF_PUBLIC_INPUTSconstant. These values are typically auto-generated from circuit compilation.
10-11: Verify NUMBER_OF_PUBLIC_INPUTS decomposition matches circuit public inputs + outputs after Noir compilation.The reduction from 2068 to 22 public inputs is correct for the ciphertext commitment refactor. The circuit defines 5 public inputs (prev_ct_commitment, pk_commitment, merkle_root, slot_address, is_first_vote) plus 1 public output, totaling 6 logical fields. The Solidity implementation (CRISPProgram.sol lines 136-141) correctly passes 6 bytes32 values matching these 6 circuit fields. The verifier constant of 22 field elements represents the decomposed count after Noir/Honk compilation. Confirm this decomposition is accurate by checking the compiled circuit metadata or build output.
examples/CRISP/program/src/lib.rs (1)
7-23: LGTM! Clean simplification of the ciphertext processing logic.The refactor from multi-step Greco decoding to direct
Ciphertext::from_bytesdeserialization streamlines the code while maintaining the same accumulation behavior. The use ofdecode_bfv_params_arcproperly leverages the shared BFV parameter utilities.examples/CRISP/packages/crisp-sdk/src/types.ts (2)
128-128: LGTM!The
prev_ct_commitmentfield addition toCircuitInputscorrectly reflects the circuit's need for ciphertext commitment verification.
168-177: LGTM!The new
ProofDatatype cleanly encapsulates proof-related data, and the simplifiedreturnValue: bigintinExecuteCircuitResultaligns with the updated circuit return pathway.examples/CRISP/server/src/server/indexer.rs (1)
392-400: LGTM!The transition from
event.votetoevent.encryptedVotecorrectly aligns with the updated event signature. The logging safely handles potentially short data with8.min(event.encryptedVote.len()).examples/CRISP/crates/zk-inputs/src/serialization.rs (2)
19-44: LGTM!The
prev_ct_commitmentfield is correctly added to theZKInputsstruct, maintaining consistency with the circuit input requirements.
112-114: LGTM!The
prev_ct_commitmentis correctly populated from the source struct and converted to string format for JSON serialization.crates/compute-provider/Cargo.toml (1)
21-21: LGTM!The
e3-bfv-helpersworkspace dependency correctly enables the compute-provider crate to utilize the BFV helper utilities for ciphertext commitment computation.examples/CRISP/packages/crisp-sdk/src/sdk.ts (1)
10-10: LGTM!Clean consolidation of type imports from the local types module. The
ProofDatatype aligns with the return types already declared ongenerateMaskVoteProofandgenerateVoteProof.examples/CRISP/crates/evm_helpers/src/lib.rs (2)
35-35: LGTM!The rename from
votetoencryptedVoteimproves clarity and aligns with the corresponding Solidity contract event signature. Event parameter names don't affect the ABI selector, so this is a non-breaking documentation improvement.
141-145: LGTM!Formatting improvement for readability. No semantic changes.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
10-18: LGTM!Import of
computeCommitmentaligns with the new commitment-based verification approach being tested.
117-130: LGTM!Good test refactor. The destructuring of
crispInputsand verification that circuit output matches the computed commitment provides clear validation of the new commitment-based output.
138-153: LGTM!Correctly uses
sum_ct0isandsum_ct1iswhen a previous ciphertext exists, validating that the circuit outputs the commitment of the homomorphically added ciphertexts.
161-175: LGTM!Correctly uses
ct0isandct1iswhen no previous ciphertext exists, verifying the commitment of the fresh ciphertext.crates/compute-provider/src/compute_manager.rs (2)
62-65: LGTM!The updated
compute_leaf_hashescall correctly passes FHE parameters required for the new commitment-based leaf hash computation.
93-93: LGTM!Parallel path correctly passes
params.as_slice()to match the updatedcompute_leaf_hashessignature, maintaining consistency with the sequential path.crates/bfv-helpers/src/client.rs (2)
8-17: LGTM!Imports are properly added to support the new
compute_ct_commitmentfunction and updatedcompute_pk_commitment.
150-150: LGTM!Aligned with the imported
compute_poly_commitmentfunction.crates/bfv-helpers/src/utils/greco.rs (2)
61-105: LGTM! Well-structured BFV ciphertext to Greco conversion.The implementation correctly mirrors
bfv_public_key_to_grecoand properly:
- Clones and converts polynomials to power basis representation
- Extracts coefficients per modulus and degree
- Applies Greco conversion (centering, reversing, ZKP modulus reduction)
The function is well-documented and follows the established pattern in this module.
268-355: LGTM! Comprehensive test coverage.The test correctly validates the new function against the established
GrecoVectors::compute().standard_form()reference implementation, ensuring coefficient-level equality.examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (2)
65-83: LGTM! Clean composite return structure.The change from returning just JSON to a composite object with
encryptedVote(Uint8Array) andinputs(parsed JSON) provides a better API for JavaScript consumers who need both the ciphertext bytes and the structured inputs.
260-277: LGTM! Tests properly validate new return structure.The tests correctly verify that the result object contains both
encryptedVote(as a non-empty Uint8Array) andinputs(with expected JSON fields).examples/CRISP/circuits/src/hash.nr (1)
10-42: LGTM! Well-documented commitment generation.The implementation correctly:
- Flattens polynomial coefficients from both arrays
- Uses "Greco" as the domain separator (properly hex-encoded)
- Generates a single-element hash and returns it as the commitment
The TODO comment (line 12) indicates this is a temporary implementation to be replaced with PVSS circuits, which is good documentation of technical debt.
examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs (4)
27-36: LGTM! Clean addition of commitment field to inputs struct.The new
prev_ct_commitmentfield is properly integrated into theCiphertextAdditionInputsstruct, following the existing pattern of polynomial coefficient storage.
235-237: LGTM! Commitment computed after parallel aggregation.The placement of
compute_poly_commitmentafter the parallel results are merged intoresis correct, ensuring the commitment is computed over the complete coefficient vectors.
244-255: LGTM! Consistent standard form conversion.The
prev_ct_commitmentis correctly reduced modulozkp_modulusin the same manner as other coefficient vectors, maintaining consistency in the standard form representation.
286-291: LGTM! Appropriate test helper for bit width calculation.The helper correctly encapsulates the
GrecoBoundscomputation and bit width derivation, making tests cleaner and more maintainable.examples/CRISP/crates/zk-inputs/src/lib.rs (3)
77-83: LGTM! Clear return type documentation.The updated return type
Result<(Vec<u8>, String)>with the docstring clarifying it returns "the sum ciphertext bytes and JSON string" makes the API contract explicit.
270-287: LGTM! Clean commitment computation API.The method correctly:
- Derives bit width from
GrecoBoundsusing the generator's BFV parameters- Delegates to
compute_poly_commitmentfor the actual computation- Returns
Result<BigInt>for proper error propagation
319-326: LGTM! Tests properly validate new return structure.The test correctly destructures the tuple and validates both the ciphertext bytes (non-empty) and JSON output (contains expected fields).
examples/CRISP/circuits/src/main.nr (4)
50-52: LGTM! New public inputs for commitment-based verification.The addition of
prev_ct_commitment: pub Fieldas a public input allows the circuit to verify that the prover used the correct previous ciphertext without exposing the full polynomial coefficients on-chain.
165-180: LGTM! Commitment generation with clear documentation.The comments (lines 165-177) clearly explain:
- Purpose of each commitment
- Why
prev_ct_commitmentverification only happens for mask votes on non-first slots- How commitments integrate with the Fiat-Shamir transform
The three commitments are generated consistently using
generate_commitment<512, 2, 36>.
216-232: LGTM! Correct branching logic for commitment verification and returns.The verification logic correctly handles all three cases:
- Actual vote: Returns
ct_commitment(new vote replaces slot)- Mask vote (first): Returns
ct_commitment(zero vote, no previous to verify)- Mask vote (update): Verifies
prev_ct_commitment == _prev_ct_commitmentto prevent tampering, then returnssum_ct_commitmentThe
prev_ct_commitmentcheck (line 227) is correctly placed only in the mask-update path where the prover could potentially cheat by using a different previous ciphertext.
88-88: Excellent improvement: Returning commitment instead of full ciphertext.Changing the return type from ciphertext polynomial arrays to a single
Fieldcommitment dramatically reduces the on-chain verification cost. Based on the retrieved learnings about Noir circuit verification, this reduces the public outputs from potentially thousands of field elements to just one, significantly lowering gas costs for on-chain proof verification.examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (4)
13-13: LGTM!Import cleanup is appropriate -
PoseidonT3is no longer needed since the contract now uses direct commitment insertion into the Merkle tree instead of hashing vote data.
59-59: LGTM!Event signature update aligns with the new commitment-based flow. The parameter rename from
votetoencryptedVoteaccurately reflects the emitted data type.
131-148: LGTM! Public inputs ordering matches circuit expectations.The restructured public inputs array correctly implements the commitment-based verification:
- Index 0:
previousEncryptedVoteCommitment- Index 1:
committeePublicKey- Index 2:
merkleRoot- Index 3:
slotAddress- Index 4:
is_first_voteflag (derived from commitment being zero)- Index 5:
encryptedVoteCommitmentThe boolean-to-uint256 conversion at index 4 is correct for Noir circuit compatibility.
222-242: LGTM! Vote processing logic correctly handles commitment flow.The refactored
_processVotefunction:
- Returns
previousEncryptedVoteCommitmentinstead of a boolean flag, enabling the circuit to verify the previous state- For first votes: correctly sets
previousEncryptedVoteCommitmenttobytes32(0)- For re-votes: retrieves the stored commitment from
votes.elements[voteIndex]- Inserts/updates the Merkle tree with the commitment directly (no intermediate hashing)
The type casts between
bytes32anduint256are safe for commitment storage.examples/CRISP/packages/crisp-sdk/src/vote.ts (6)
8-17: LGTM!Import updates correctly bring in the new types needed for the commitment-based flow:
ProofDataandPolynomialfor the updated function signatures.
148-162: LGTM! Testing utility for commitment computation.The
computeCommitmentfunction appropriately wraps the zkInputsGenerator's commitment computation, exposing it for testing purposes. The coefficient extraction and BigInt conversion are correct.
170-212: LGTM! Circuit inputs generation correctly propagates encrypted vote.The updated
generateCircuitInputsfunction:
- Returns both
crispInputsandencryptedVotefrom the zkInputsGenerator result- Handles both initial vote and update paths consistently
- Properly extracts
encryptedVotefrom the generator result for both code paths
248-276: LGTM! Vote proof generation correctly bundles encrypted vote.The
generateVoteProoffunction properly:
- Destructures
{ crispInputs, encryptedVote }fromgenerateCircuitInputs- Spreads the proof result and adds
encryptedVoteto return a completeProofDataobject
318-327: Indices correctly aligned with contract and circuit structure.The function extracts
slotAddressfrompublicInputs[3]andencryptedVoteCommitmentfrompublicInputs[5], which correctly map to the contract'snoirPublicInputsarray. The circuit defines 5 public inputs (prev_ct_commitment, pk_commitment, merkle_root, slot_address, is_first_vote) and returns 1 public output; the contract's verification array properly includes all 6 values, with the output at index 5. Index alignment is correct.
219-225: ThereturnValuetype coercion is correct and necessary.The Noir
execute()method returnsreturnValueas a string (hex format), not asbigint. The explicit castas \0x${string}`correctly identifies this format, and theBigInt()conversion transforms it to thebiginttype required byExecuteCircuitResult`. The test cases confirm this assumption is correct and the conversion works reliably.examples/CRISP/circuits/src/ciphertext_addition.nr (3)
42-68: LGTM! Struct redesign for commitment-based verification.The renamed fields (
ct0is/ct1isinstead ofzero_ct0is/zero_ct1is) and new commitment fields (ct_commitment,prev_ct_commitment,sum_ct_commitment) align with the PR's goal of using commitments for on-chain verification. The struct cleanly separates the full polynomial data (for local verification) from the commitment hashes (for on-chain binding).
70-107: LGTM!Constructor correctly initializes all new commitment fields alongside the polynomial data.
245-289: LGTM! Verification logic correctly implements the addition constraint.The
verify_evaluationsfunction properly checks:
sum_ct0i = ct0i + prev_ct0i + r0i * q_ifor all CRT basessum_ct1i = ct1i + prev_ct1i + r1i * q_ifor all CRT basesThe linear combination approach with random weights (Schwartz-Zippel) provides soundness guarantees. The updated comments accurately reflect the renamed variables.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @examples/CRISP/packages/crisp-sdk/tests/vote.test.ts:
- Around line 36-40: The mock response in mockGetPreviousCiphertextResponse
returns a Uint8Array but the server serializes ciphertext as a JSON array of
numbers; update the json() return value to return Array.from(previousCiphertext)
(i.e., a plain JS array of numbers) so the mock matches the actual serialized
shape used by the code that consumes ciphertext.
🧹 Nitpick comments (2)
crates/bfv-helpers/src/client.rs (2)
172-208: Consider extracting shared commitment-to-bytes conversion logic.The byte conversion and padding logic (lines 190-207) is duplicated verbatim from
compute_pk_commitment(lines 152-169). This could be extracted into a helper function to reduce duplication and centralize the 32-byte constraint validation.♻️ Proposed helper extraction
// Helper function to convert commitment BigInt to 32-byte array fn commitment_to_bytes(commitment_bigint: num_bigint::BigInt) -> Result<[u8; 32]> { let bytes = commitment_bigint.to_bytes_be().1; if bytes.len() > 32 { return Err(anyhow!( "Commitment must be at most 32 bytes, got {}", bytes.len() )); } let mut padded_bytes = vec![0u8; 32]; let start_idx = 32 - bytes.len(); padded_bytes[start_idx..].copy_from_slice(&bytes); padded_bytes .try_into() .map_err(|_| anyhow!("Failed to convert padded bytes to array")) }Then both functions can use:
- let bytes = commitment_bigint.to_bytes_be().1; - - if bytes.len() > 32 { - return Err(anyhow!( - "Commitment must be at most 32 bytes, got {}", - bytes.len() - )); - } - - let mut padded_bytes = vec![0u8; 32]; - let start_idx = 32 - bytes.len(); - padded_bytes[start_idx..].copy_from_slice(&bytes); - - let ciphertext_hash: [u8; 32] = padded_bytes - .try_into() - .map_err(|_| anyhow!("Failed to convert padded bytes to array"))?; - - Ok(ciphertext_hash) + commitment_to_bytes(commitment_bigint)
210-335: Missing unit tests for commitment functions.The test module covers
bfv_encryptandbfv_verifiable_encrypt, but there are no tests forcompute_pk_commitmentor the newcompute_ct_commitment. Consider adding tests to verify:
- Correct commitment computation for valid inputs
- Proper error handling for invalid/malformed ciphertext bytes
- Deterministic output (same input produces same commitment)
Would you like me to generate unit tests for
compute_ct_commitment?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
templates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/bfv-helpers/src/client.rsexamples/CRISP/packages/crisp-sdk/tests/fixtures/previous-ciphertext.jsonexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 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:
crates/bfv-helpers/src/client.rs
📚 Learning: 2024-10-22T03:42:14.057Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/context.rs:94-97
Timestamp: 2024-10-22T03:42:14.057Z
Learning: In `packages/ciphernode/router/src/context.rs`, avoid adding complexity for batching checkpoint operations in code; rely on the database's batching capabilities instead.
Applied to files:
crates/bfv-helpers/src/client.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/bfv-helpers/src/client.rs
📚 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:
crates/bfv-helpers/src/client.rs
📚 Learning: 2025-12-19T11:35:51.645Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 1109
File: examples/CRISP/server/src/server/routes/state.rs:33-37
Timestamp: 2025-12-19T11:35:51.645Z
Learning: In the CRISP voting system (examples/CRISP), ciphertext data and slot states are public and stored on-chain in the CRISPProgram smart contract. The server endpoints `/previous-ciphertext` and `/is-slot-empty` expose public blockchain data that can already be accessed by anyone through the contract's external view functions (getSlotIndex, isSlotEmptyByAddress) or by reading contract state directly. Authentication is not needed for these endpoints since they only expose data that is already publicly available on the blockchain.
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
📚 Learning: 2025-12-23T17:18:32.343Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1142
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:141-147
Timestamp: 2025-12-23T17:18:32.343Z
Learning: When verifying Noir circuit proofs in Solidity, the public inputs array passed to the verifier must include both the circuit's public inputs AND public outputs. For example, if a Noir circuit has 3 public inputs and returns 2064 public output fields (-> pub [Field; 2064]), the verification array must contain all 2067 values (3 + 2064).
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
🧬 Code graph analysis (2)
crates/bfv-helpers/src/client.rs (1)
crates/bfv-helpers/src/utils/greco.rs (2)
bfv_ciphertext_to_greco(61-105)bfv_public_key_to_greco(118-168)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
examples/CRISP/packages/crisp-sdk/src/sdk.ts (1)
CrispSDK(15-69)examples/CRISP/packages/crisp-sdk/src/index.ts (2)
CrispSDK(20-20)encryptVote(17-17)examples/CRISP/packages/crisp-sdk/src/vote.ts (4)
encryptVote(134-138)generateCircuitInputs(170-212)computeCommitment(155-162)executeCircuit(219-225)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
ZERO_VOTE(40-40)
⏰ 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_enclave_cli
- GitHub Check: test_net
- GitHub Check: build_e3_support_dev
- GitHub Check: build_crisp_sdk
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_integration
- GitHub Check: Build & Push Image
🔇 Additional comments (9)
crates/bfv-helpers/src/client.rs (3)
8-17: LGTM!The import changes correctly bring in the necessary types and functions to support the new ciphertext commitment functionality.
150-150: LGTM!Good refactor to use the shared
compute_poly_commitmentfunction, which aligns with the broader commitment workflow and reduces code duplication across the codebase.
185-186: Variable naming is misleading but usage appears consistent with codebase pattern.Line 186 uses
bounds.pk_bounds[0]to computebit_ct, which is semantically confusing sincepk_boundssuggests a public-key-specific bound. However, this pattern is used consistently throughout the codebase for ciphertext commitments (also at lines 112, 147 in client.rs and multiple places in examples/CRISP). Noct_boundsfield exists as an alternative. The naming suggestspk_boundsmay represent a general coefficient bound applicable to both public keys and ciphertexts, though this should be clarified or the variable should be renamed to reflect its broader semantic purpose.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (6)
10-19: LGTM!The updated imports correctly bring in
computeCommitmentandencryptVoteneeded for the commitment-based verification flow.
32-32: LGTM!The
previousCiphertextis now correctly generated viaencryptVote(ZERO_VOTE, publicKey), producing a valid encrypted ciphertext (Uint8Array) for test scenarios requiring a previous vote.Also applies to: 65-65
112-132: LGTM!The test correctly validates the commitment-based circuit output for a regular vote. Using
ct0is/ct1isfor the commitment is appropriate since there's no previous ciphertext being added.
134-155: LGTM!The test correctly uses
sum_ct0is/sum_ct1isfor computing the commitment when there's a previous ciphertext, validating that the circuit outputs the commitment of the ciphertext addition result.
157-177: LGTM!The test correctly validates the commitment-based output for a mask vote without previous ciphertext, using
ct0is/ct1isas expected.
226-246: LGTM!The SDK integration test for mask votes with previous ciphertext correctly chains the mock responses and validates the full proof generation and verification pipeline.
3 participants
Refactors CRISP to use ciphertext commitments instead of directly handling ciphertexts.
Main Changes
Breaking Changes
Closes #1084
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.