feat: add ciphertext addition to circuit#912
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
cedoor
changed the title
WalkthroughAdds a BFV ciphertext-addition verifier and wires it into the CRISP circuit; flattens ciphertext-addition inputs into explicit per-field arrays across Rust serialization and the TypeScript SDK; updates tests; and adds a local Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Circuit
participant CTAdd as CiphertextAddition
participant Sponge as Fiat‑Shamir Sponge
participant Constraints as Constraint Verifier
Main->>CTAdd: CiphertextAddition::new(...) → ct_add
Main->>CTAdd: ct_add.verify_correct_ciphertext_addition()
CTAdd->>CTAdd: check_range_bounds()
CTAdd->>CTAdd: payload()
CTAdd->>Sponge: generate_challenge(domain_sep, payload)
Sponge-->>CTAdd: gamma
CTAdd->>Constraints: check_ciphertext_addition_constraints(gamma)
loop per CRT basis i
Constraints->>Constraints: eval polynomials at gamma
Constraints->>Constraints: assert sum0i = zero0i + prev0i + r0i * qi
Constraints->>Constraints: assert sum1i = zero1i + prev1i + r1i * qi
end
Constraints-->>CTAdd: constraints satisfied
CTAdd-->>Main: verification complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
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 |
cedoor
force-pushed
the
feat/noir-ct-add
branch
from
c58751c to
f2cfbfd
Compare
cedoor
force-pushed
the
feat/noir-ct-add
branch
from
2b2cbf3 to
2aab57e
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: 5
🧹 Nitpick comments (5)
examples/CRISP/circuits/src/ciphertext_addition.nr (2)
17-27: Naming: “zero_ct” is misleading; use “new_ct” or “addend_ct*”.**These are the fresh ciphertext polynomials (the addend), not an encryption of zero. Consider renaming for clarity:
new_ct0is/new_ct1isoradd_ct0is/add_ct1is.
74-89: Confirm randomness flatten bit‑width for signed coefficients.
flatten::<_, _, 1>(..., r0is/r1is)assumes 1‑bit representation. If coefficients can be −1,0,1, ensureflattenhandles signed values correctly; otherwise use 2 bits or sign‑aware serialization.examples/CRISP/circuits/src/main.nr (1)
92-102: Constructor arg order is easy to swap by mistake; keep it labeled in comments or group by section.Consider inline comments to prevent accidental arg misordering (ct0is/ct1is vs prev/sum/r).
- let ct_add: CiphertextAddition<2048, 1, 54, 54, 54> = CiphertextAddition::new( - params.crypto_params(), - ct0is, - ct1is, - prev_ct0is, - prev_ct1is, - sum_ct0is, - sum_ct1is, - sum_r0is, - sum_r1is, - ); + let ct_add: CiphertextAddition<2048, 1, 54, 54, 54> = CiphertextAddition::new( + params.crypto_params(), + /* new/addend ct */ ct0is, /* ct1 */ ct1is, + /* previous ct */ prev_ct0is, /* ct1 */ prev_ct1is, + /* sum ct */ sum_ct0is, /* ct1 */ sum_ct1is, + /* quotient r */ sum_r0is, /* r1 */ sum_r1is, + );examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
133-139: Strengthen assertions for new fields (shape, not just type).Use
Array.isArrayand assert expected length (L=1) to catch schema regressions.- expect(crispInputs.prev_ct0is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.prev_ct0is)).toBe(true) + expect(crispInputs.prev_ct0is).toHaveLength(1) - expect(crispInputs.prev_ct1is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.prev_ct1is)).toBe(true) + expect(crispInputs.prev_ct1is).toHaveLength(1) - expect(crispInputs.sum_ct0is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_ct0is)).toBe(true) + expect(crispInputs.sum_ct0is).toHaveLength(1) - expect(crispInputs.sum_ct1is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_ct1is)).toBe(true) + expect(crispInputs.sum_ct1is).toHaveLength(1) - expect(crispInputs.sum_r0is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_r0is)).toBe(true) + expect(crispInputs.sum_r0is).toHaveLength(1) - expect(crispInputs.sum_r1is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_r1is)).toBe(true) + expect(crispInputs.sum_r1is).toHaveLength(1)
158-164: Repeat stronger checks after augmentation.Same as above; validate array lengths to ensure augmentation preserves shape.
- expect(crispInputs.prev_ct0is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.prev_ct0is)).toBe(true) + expect(crispInputs.prev_ct0is).toHaveLength(1) - expect(crispInputs.prev_ct1is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.prev_ct1is)).toBe(true) + expect(crispInputs.prev_ct1is).toHaveLength(1) - expect(crispInputs.sum_ct0is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_ct0is)).toBe(true) + expect(crispInputs.sum_ct0is).toHaveLength(1) - expect(crispInputs.sum_ct1is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_ct1is)).toBe(true) + expect(crispInputs.sum_ct1is).toHaveLength(1) - expect(crispInputs.sum_r0is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_r0is)).toBe(true) + expect(crispInputs.sum_r0is).toHaveLength(1) - expect(crispInputs.sum_r1is).toBeInstanceOf(Array) + expect(Array.isArray(crispInputs.sum_r1is)).toBe(true) + expect(crispInputs.sum_r1is).toHaveLength(1)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/CRISP/circuits/Nargo.toml(1 hunks)examples/CRISP/circuits/src/ciphertext_addition.nr(1 hunks)examples/CRISP/circuits/src/main.nr(5 hunks)examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs(0 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(2 hunks)examples/CRISP/crates/zk-inputs/src/serialization.rs(9 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(2 hunks)examples/CRISP/packages/crisp-zk-inputs/.gitignore(0 hunks)
💤 Files with no reviewable changes (2)
- examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs
- examples/CRISP/packages/crisp-zk-inputs/.gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
examples/CRISP/crates/zk-inputs/src/lib.rs (1)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
create_vote_vector(145-147)
⏰ 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: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: test_contracts
🔇 Additional comments (12)
examples/CRISP/circuits/src/main.nr (3)
10-12: Good modularization.Exposing
ciphertext_additionand importingCiphertextAdditionlooks clean.
20-27: Input visibility split is sensible; verify consistency with JSON schema.
prev_ct*are public;sum_ct*andsum_r*are private witnesses. Ensure frontend serialization matches this (nopubrequired forsum_*).
104-107: Order of checks makes sense; call remains valid after &self change.
greco.verify_correct_ciphertext_encryption();thenct_add.verify_correct_ciphertext_addition();is a good sequencing.After changing
CiphertextAdditionmethods to&self, no call‑site changes are needed. Please re-run synth/compile.examples/CRISP/crates/zk-inputs/src/lib.rs (1)
371-373: Good randomness check; minor nit.Using a fresh vector via
create_vote_vector()keeps the non‑determinism test meaningful. LGTM.examples/CRISP/circuits/src/ciphertext_addition.nr (1)
139-160: Generate challenges without consuming self; consider squeezing L not 2L.Current code consumes
selfand squeezes2*Lbut only usesL. Reduce toLor use both.- fn generate_challenge(self) -> Vec<Field> { - let inputs = self.payload(); + fn generate_challenge(&self) -> Vec<Field> { + let inputs = self.payload(); // Domain separator unchanged... - // IO Pattern: ABSORB(input_size), SQUEEZE(2*L) + // IO Pattern: ABSORB(input_size), SQUEEZE(L) let input_size = inputs.len(); - let io_pattern = [0x80000000 | input_size, 0x00000000 | (2 * L)]; + let io_pattern = [0x80000000 | input_size, 0x00000000 | L]; let mut sponge = SafeSponge::start(io_pattern, domain_separator); sponge.absorb(inputs); - let gammas = sponge.squeeze(); + let gammas = sponge.squeeze(); sponge.finish(); gammas }Confirm
SafeSponge’s IO pattern unit is “number of field elements” and thatsqueeze()returns exactlyLelements with this pattern. Ifsqueeze()always returns the full pattern, this is safe; otherwise use the 2L variant and consume both sets.examples/CRISP/circuits/Nargo.toml (1)
11-11: Dependency addition verified and approved.The
safecrate exists at the declared pathpackages/circuits/crates/libs/safe/with a valid Nargo.toml manifest. TheSafeSpongestruct is properly exported from the crate and correctly used inciphertext_addition.nr. The relative path resolution is correct.examples/CRISP/packages/crisp-sdk/src/types.ts (2)
136-142: Well-structured ciphertext addition fields.The flattened structure with inline fields is clear and well-organized. The section comment helps with readability, and the naming convention (prev_* for inputs, sum_* for outputs) makes the data flow intuitive.
143-169: Excellent interface organization.The section comments significantly improve code readability by clearly delineating different functional areas (Greco, ECDSA, Merkle Tree, Balance). This makes the interface easier to navigate and understand.
examples/CRISP/crates/zk-inputs/src/serialization.rs (4)
21-26: Correct struct field definitions.The flattened fields are properly typed as
Vec<serde_json::Value>and match the TypeScript interface naming exactly. The placement at the beginning of the struct provides good logical grouping.
88-142: Correct serialization implementation.The field mapping logic is sound:
- Each vector is properly transformed to JSON objects with
coefficientsarrays- The renaming from
r0is/r1istosum_r0is/sum_r1iscorrectly applies thesum_prefix for consistency with other sum ciphertext fields- The pattern is consistent across all six fields and matches the TypeScript
Polynomialinterface structure
340-345: Test assertions updated correctly.The test properly verifies that all six ciphertext addition fields are present and have the expected structure, confirming the serialization works as intended.
382-387: JSON serialization test updated correctly.The test verifies that all six ciphertext addition fields are present in the serialized JSON output, ensuring the end-to-end serialization pipeline works correctly.
cedoor
force-pushed
the
feat/noir-ct-add
branch
from
79bd666 to
2530de6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
examples/CRISP/circuits/src/ciphertext_addition.nr (5)
62-91: Do not consume self in payload; borrow instead.Taking self moves all fields. Make the receiver &self and pass borrowed arrays to flatten.
Apply:
- fn payload(self) -> Vec<Field> { + fn payload(&self) -> Vec<Field> { let mut inputs = Vec::new(); - inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, self.zero_ct0is); - inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, self.zero_ct1is); - inputs = flatten::<_, _, BIT_PREV_CT>(inputs, self.prev_ct0is); - inputs = flatten::<_, _, BIT_PREV_CT>(inputs, self.prev_ct1is); - inputs = flatten::<_, _, BIT_SUM_CT>(inputs, self.sum_ct0is); - inputs = flatten::<_, _, BIT_SUM_CT>(inputs, self.sum_ct1is); - inputs = flatten::<_, _, 1>(inputs, self.r0is); - inputs = flatten::<_, _, 1>(inputs, self.r1is); + // NOTE: flatten must accept borrows; if it doesn’t, add a by-ref overload or clone the arrays locally. + inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, &self.zero_ct0is); + inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, &self.zero_ct1is); + inputs = flatten::<_, _, BIT_PREV_CT>(inputs, &self.prev_ct0is); + inputs = flatten::<_, _, BIT_PREV_CT>(inputs, &self.prev_ct1is); + inputs = flatten::<_, _, BIT_SUM_CT>(inputs, &self.sum_ct0is); + inputs = flatten::<_, _, BIT_SUM_CT>(inputs, &self.sum_ct1is); + inputs = flatten::<_, _, 1>(inputs, &self.r0is); + inputs = flatten::<_, _, 1>(inputs, &self.r1is); inputs }If flatten cannot borrow, introduce
flatten_refin polynomial or minimallyclone()the arrays here.
111-120: verify_correct_ciphertext_addition moves self then reuses it. Switch to &self.Current sequencing will fail due to use-after-move.
- pub fn verify_correct_ciphertext_addition(self) { + pub fn verify_correct_ciphertext_addition(&self) { self.check_range_bounds(); let gammas = self.generate_challenge(); self.check_ciphertext_addition_constraints(gammas); }
122-130: Borrow in check_range_bounds; keep semantics.Receiver should be &self; still assert {-1,0,1}.
- fn check_range_bounds(self) { + fn check_range_bounds(&self) { for i in 0..L { self.r0is[i].range_check_1bound::<1>(1); self.r1is[i].range_check_1bound::<1>(1); } }
139-160: Borrow in generate_challenge and fix squeeze count vs usage.You squeeze 2*L but only read L later.
- fn generate_challenge(self) -> Vec<Field> { - let inputs = self.payload(); + fn generate_challenge(&self) -> Vec<Field> { + let inputs = self.payload(); let input_size = inputs.len(); - let io_pattern = [0x80000000 | input_size, 0x00000000 | (2 * L)]; + // One gamma per CRT basis. + let io_pattern = [0x80000000 | input_size, 0x00000000 | L]; let mut sponge = SafeSponge::start(io_pattern, domain_separator); sponge.absorb(inputs); let gammas = sponge.squeeze(); sponge.finish(); gammas }If you intended two challenges/basis, keep
2*Land index both halves below.
166-187: Fix gamma indexing and borrow receiver in constraint checks.
gammas.get(i)returns Option and mismatched type; also method should borrow.- fn check_ciphertext_addition_constraints(self, gammas: Vec<Field>) { + fn check_ciphertext_addition_constraints(&self, gammas: Vec<Field>) { for i in 0..L { - let gamma_i = gammas.get(i); + let gamma_i = gammas[i as usize]; let sum0 = self.sum_ct0is[i].eval(gamma_i); let sum1 = self.sum_ct1is[i].eval(gamma_i); let ct0 = self.zero_ct0is[i].eval(gamma_i); let ct1 = self.zero_ct1is[i].eval(gamma_i); let old0 = self.prev_ct0is[i].eval(gamma_i); let old1 = self.prev_ct1is[i].eval(gamma_i); let q_i = self.crypto_params.qis[i]; let radd0 = self.r0is[i].eval(gamma_i); let radd1 = self.r1is[i].eval(gamma_i); assert(sum0 == ct0 + old0 + radd0 * q_i); assert(sum1 == ct1 + old1 + radd1 * q_i); } }#!/bin/bash # Verify no remaining self-by-value receivers or get()-based indexing rg -n "fn\s+\w+\(self" examples/CRISP/circuits/src/ciphertext_addition.nr rg -n "squeeze\(.+2\s*\*\s*L" examples/CRISP/circuits/src/ciphertext_addition.nr rg -n "gammas\.get\(" examples/CRISP/circuits/src/ciphertext_addition.nr
🧹 Nitpick comments (2)
examples/CRISP/crates/zk-inputs/src/serialization.rs (2)
88-143: Reduce repetition with a small helper to map vectors to JSON.The six near-identical blocks can be DRY’d.
+fn map_poly_vec(vecs: &[Vec<BigInt>]) -> Vec<serde_json::Value> { + vecs.iter() + .map(|v| serde_json::json!({ "coefficients": to_string_1d_vec(v) })) + .collect() +} @@ - prev_ct0is: ciphertext_addition_inputs_standard - .prev_ct0is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + prev_ct0is: map_poly_vec(&ciphertext_addition_inputs_standard.prev_ct0is), @@ - prev_ct1is: ciphertext_addition_inputs_standard - .prev_ct1is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + prev_ct1is: map_poly_vec(&ciphertext_addition_inputs_standard.prev_ct1is), @@ - sum_ct0is: ciphertext_addition_inputs_standard - .sum_ct0is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_ct0is: map_poly_vec(&ciphertext_addition_inputs_standard.sum_ct0is), @@ - sum_ct1is: ciphertext_addition_inputs_standard - .sum_ct1is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_ct1is: map_poly_vec(&ciphertext_addition_inputs_standard.sum_ct1is), @@ - sum_r0is: ciphertext_addition_inputs_standard - .r0is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_r0is: map_poly_vec(&ciphertext_addition_inputs_standard.r0is), @@ - sum_r1is: ciphertext_addition_inputs_standard - .r1is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_r1is: map_poly_vec(&ciphertext_addition_inputs_standard.r1is),
340-346: Tests: good coverage for new ct-add vectors.Basic length checks ensure presence; consider adding a spot-check of coefficient string values.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/CRISP/circuits/Nargo.toml(1 hunks)examples/CRISP/circuits/src/ciphertext_addition.nr(1 hunks)examples/CRISP/circuits/src/main.nr(5 hunks)examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs(0 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(2 hunks)examples/CRISP/crates/zk-inputs/src/serialization.rs(9 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(2 hunks)examples/CRISP/packages/crisp-zk-inputs/.gitignore(0 hunks)
💤 Files with no reviewable changes (2)
- examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs
- examples/CRISP/packages/crisp-zk-inputs/.gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/crates/zk-inputs/src/lib.rs
- examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
⏰ 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: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: rust_integration
🔇 Additional comments (8)
examples/CRISP/circuits/Nargo.toml (1)
11-11: No issues found—dependency path is correct and properly used.The
safecrate at../../../packages/circuits/crates/libs/safeexists and is properly structured withlib.nrfor library crates. The dependency is actively imported in the circuit implementation atexamples/CRISP/circuits/src/ciphertext_addition.nr:7withuse safe::SafeSponge;. The relative path follows standard Noir conventions for workspace local dependencies.examples/CRISP/packages/crisp-sdk/src/types.ts (2)
136-143: LGTM: flattened ciphertext-addition inputs at top level.Names align with main.nr (prev_ct*, sum_ct*, sum_r*). No issues.
168-169: No action required.The concern in the review comment is based on incorrect assumptions. The Field type is defined as string, so there is no mismatch between the circuit's expectations and the SDK's implementation. Additionally, balance is consistently serialized as a decimal string via
.toString()across all call sites (vote.ts, utils.ts, and tests), ensuring uniform encoding throughout the codebase.examples/CRISP/circuits/src/main.nr (3)
92-102: Constructor arg order matches struct fields.crypto_params, zero_ct*, prev_ct*, sum_ct*, r* are wired in correct order.
104-107: Review comment contains incorrect assertion about method signature.The verification reveals that
verify_correct_ciphertext_additiontakesselfby value (line 111 of ciphertext_addition.nr), not&selfby reference as the comment claims. The call at line 105 of main.nr moves thect_addvalue rather than borrowing it.Likely an incorrect or invalid review comment.
20-27: Verified: All L=1 array declarations are consistent across producers.All input ciphertexts (ct0is, ct1is), previous ciphertexts (prev_ct0is, prev_ct1is), and witness arrays (sum_ct0is, sum_ct1is, sum_r0is, sum_r1is) are declared as [Polynomial<2048>; 1], matching the generic parameter L=1 in CiphertextAddition<2048, 1, 54, 54, 54> at line 92. Visibility matches intent: prev_* public, sum_* private.
examples/CRISP/crates/zk-inputs/src/serialization.rs (2)
21-27: LGTM: expose per-field ct-add vectors in ZKInputs.Matches SDK and circuit expectations.
382-387: JSON keys assertions verified and correct.The assertions at lines 382-387 properly validate that the JSON object contains all required keys. These field names align exactly with:
- The
ZKInputsstruct fields (lines 21-26)- The TypeScript type definitions in
types.ts(lines 137-142)No issues identified.
cedoor
force-pushed
the
feat/noir-ct-add
branch
from
585afbe to
edac80b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
164-166: Merkle proof types should be numeric and bit‑typed; avoid strings.Circuit expects
u32and[u1; 20]. UsenumberandArray<0|1>to prevent string conversions and invalid values. This was flagged previously.merkle_root: string - merkle_proof_length: string - merkle_proof_indices: string[] + merkle_proof_length: number + merkle_proof_indices: Array<0 | 1> merkle_proof_siblings: string[]Find and remove
.toString()casts caused by string types:#!/bin/bash rg -nP --type=ts --type=tsx -C2 '(merkle_proof_length|merkle_proof_indices).*toString\(|:.*string\[\]|:.*string\b' examples/CRISP/packages/crisp-sdkexamples/CRISP/circuits/src/ciphertext_addition.nr (4)
62-91: Don’t consume self in payload; borrow&self(otherwise later uses are after‑move).
fn payload(self)moves the struct; subsequent calls onselfare invalid.- fn payload(self) -> Vec<Field> { + fn payload(&self) -> Vec<Field> { let mut inputs = Vec::new(); - inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, self.zero_ct0is); - inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, self.zero_ct1is); - inputs = flatten::<_, _, BIT_PREV_CT>(inputs, self.prev_ct0is); - inputs = flatten::<_, _, BIT_PREV_CT>(inputs, self.prev_ct1is); - inputs = flatten::<_, _, BIT_SUM_CT>(inputs, self.sum_ct0is); - inputs = flatten::<_, _, BIT_SUM_CT>(inputs, self.sum_ct1is); - inputs = flatten::<_, _, 1>(inputs, self.r0is); - inputs = flatten::<_, _, 1>(inputs, self.r1is); + // Use flatten variant that borrows (or update `flatten` to accept borrows). + inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, self.zero_ct0is); + inputs = flatten::<_, _, BIT_ZERO_CT>(inputs, self.zero_ct1is); + inputs = flatten::<_, _, BIT_PREV_CT>(inputs, self.prev_ct0is); + inputs = flatten::<_, _, BIT_PREV_CT>(inputs, self.prev_ct1is); + inputs = flatten::<_, _, BIT_SUM_CT>(inputs, self.sum_ct0is); + inputs = flatten::<_, _, BIT_SUM_CT>(inputs, self.sum_ct1is); + inputs = flatten::<_, _, 1>(inputs, self.r0is); + inputs = flatten::<_, _, 1>(inputs, self.r1is); inputs }Please confirm
polynomial::flattensupports borrowing; if not, add aflatten_refor similar to avoid moving fields.#!/bin/bash # Check flatten signature rg -nPU '(?s)fn\s+flatten\s*\(' packages/circuits/crates/libs/polynomial/src/lib.nr examples/CRISP -S -C3
111-130: All verification methods should take&self.Avoid moving the instance; borrow instead.
- pub fn verify_correct_ciphertext_addition(self) { + pub fn verify_correct_ciphertext_addition(&self) { self.check_range_bounds(); let gammas = self.generate_challenge(); self.check_ciphertext_addition_constraints(gammas); } @@ - fn check_range_bounds(self) { + fn check_range_bounds(&self) { for i in 0..L { self.r0is[i].range_check_1bound::<1>(1); self.r1is[i].range_check_1bound::<1>(1); } }
139-160: Sponge squeeze count mismatch; borrow&selfand squeezeLif only one gamma per basis is used.Current code squeezes
2*Lbut consumes one gamma per i.- fn generate_challenge(self) -> Vec<Field> { - let inputs = self.payload(); + fn generate_challenge(&self) -> Vec<Field> { + let inputs = self.payload(); @@ - // IO Pattern: ABSORB(input_size), SQUEEZE(2*L) + // IO Pattern: ABSORB(input_size), SQUEEZE(L) let input_size = inputs.len(); - let io_pattern = [0x80000000 | input_size, 0x00000000 | (2 * L)]; + let io_pattern = [0x80000000 | input_size, 0x00000000 | L];If you intended two challenges per CRT basis, keep
2*Land index withgammas[L + i]below instead.
166-187: Fix gamma access and borrow receiver in constraints.Use direct indexing and
&self.gammas.get(i)is the wrong type/semantics here.- fn check_ciphertext_addition_constraints(self, gammas: Vec<Field>) { + fn check_ciphertext_addition_constraints(&self, gammas: Vec<Field>) { for i in 0..L { - let gamma_i = gammas.get(i); + let gamma_i = gammas[i as usize]; let sum0 = self.sum_ct0is[i].eval(gamma_i); let sum1 = self.sum_ct1is[i].eval(gamma_i); let ct0 = self.zero_ct0is[i].eval(gamma_i); let ct1 = self.zero_ct1is[i].eval(gamma_i); let old0 = self.prev_ct0is[i].eval(gamma_i); let old1 = self.prev_ct1is[i].eval(gamma_i); let q_i = self.crypto_params.qis[i]; let radd0 = self.r0is[i].eval(gamma_i); let radd1 = self.r1is[i].eval(gamma_i); assert(sum0 == ct0 + old0 + radd0 * q_i); assert(sum1 == ct1 + old1 + radd1 * q_i); } }
🧹 Nitpick comments (6)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
135-143: Field naming OK; consider documenting L=NCRT alignment for consumers.Add a brief comment that each Polynomial[] length equals CRT bases (currently L=1) to guide SDK users.
examples/CRISP/crates/zk-inputs/src/serialization.rs (2)
88-143: Migration to per‑field vectors looks correct; reduce repetition with a helper.Same mapping repeated 6×; extract a helper for clarity and fewer mistakes.
+fn polys_to_json(vecs: &[Vec<BigInt>]) -> Vec<serde_json::Value> { + vecs.iter() + .map(|v| serde_json::json!({ "coefficients": to_string_1d_vec(v) })) + .collect() +} @@ - ZKInputs { - prev_ct0is: ciphertext_addition_inputs_standard - .prev_ct0is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + ZKInputs { + prev_ct0is: polys_to_json(&ciphertext_addition_inputs_standard.prev_ct0is), @@ - prev_ct1is: ciphertext_addition_inputs_standard - .prev_ct1is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + prev_ct1is: polys_to_json(&ciphertext_addition_inputs_standard.prev_ct1is), @@ - sum_ct0is: ciphertext_addition_inputs_standard - .sum_ct0is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_ct0is: polys_to_json(&ciphertext_addition_inputs_standard.sum_ct0is), @@ - sum_ct1is: ciphertext_addition_inputs_standard - .sum_ct1is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_ct1is: polys_to_json(&ciphertext_addition_inputs_standard.sum_ct1is), @@ - sum_r0is: ciphertext_addition_inputs_standard - .r0is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_r0is: polys_to_json(&ciphertext_addition_inputs_standard.r0is), @@ - sum_r1is: ciphertext_addition_inputs_standard - .r1is - .iter() - .map(|v| { - serde_json::json!({ - "coefficients": to_string_1d_vec(v) - }) - }) - .collect(), + sum_r1is: polys_to_json(&ciphertext_addition_inputs_standard.r1is),
324-392: Tests cover shape; add one coefficient value assertion to ensure to_string mapping.Light sanity check that one coefficient serializes as expected.
- assert!(parsed.get("prev_ct0is").is_some()); + assert!(parsed.get("prev_ct0is").is_some()); + // spot check + assert_eq!( + parsed["prev_ct0is"][0]["coefficients"][0].as_str().unwrap(), + "1" + );examples/CRISP/circuits/src/ciphertext_addition.nr (1)
14-17: Naming: considercurr_ct*instead ofzero_ct*for clarity.If these are the “new” ciphertext terms, rename to reduce ambiguity; otherwise document “zero ciphertext” explicitly.
Also applies to: 34-38
examples/CRISP/circuits/src/main.nr (1)
93-103: Argument order toCiphertextAddition::newlooks right; verify “zero_ct” semantics.*You pass
ct0is/ct1isas zero/new ciphertexts. Confirm they represent the intended term insum = new_ct + prev_ct + r*q. If yes, consider renaming in theCiphertextAdditionstruct for clarity.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
141-146: Add structure/length assertions for ct-addition arrays.Type checks are fine; also assert L and element shape to catch regressions. Both locations (141–146 and 171–176) test identical array properties and should be updated consistently.
expect(crispInputs.sum_r1is).toBeInstanceOf(Array) + // L = 1 in current circuit + ;[crispInputs.prev_ct0is, crispInputs.prev_ct1is, crispInputs.sum_ct0is, crispInputs.sum_ct1is, crispInputs.sum_r0is, crispInputs.sum_r1is] + .forEach(arr => { + expect(arr.length).toBe(1) + expect(arr[0]).toHaveProperty('coefficients') + expect(Array.isArray(arr[0].coefficients)).toBe(true) + })Apply the same assertion pattern at both test locations (141–146 and 171–176).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/CRISP/circuits/Nargo.toml(1 hunks)examples/CRISP/circuits/src/ciphertext_addition.nr(1 hunks)examples/CRISP/circuits/src/main.nr(5 hunks)examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs(0 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(2 hunks)examples/CRISP/crates/zk-inputs/src/serialization.rs(9 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(2 hunks)examples/CRISP/packages/crisp-zk-inputs/.gitignore(0 hunks)
💤 Files with no reviewable changes (2)
- examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs
- examples/CRISP/packages/crisp-zk-inputs/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/CRISP/crates/zk-inputs/src/lib.rs
⏰ 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: build_sdk
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
🔇 Additional comments (3)
examples/CRISP/circuits/Nargo.toml (1)
11-11: Thesafedependency is correctly configured and actively used in the circuit.Verification confirms the library exists at the specified path, is properly configured as a Noir library with
Nargo.toml, and is actively imported inexamples/CRISP/circuits/src/ciphertext_addition.nr(line 7:use safe::SafeSponge;). The dependency follows the established pattern for local path dependencies and is correctly ordered.examples/CRISP/circuits/src/main.nr (2)
21-26: Input visibility: confirmprev_ct*should be public.Making previous ciphertext polynomials
pubexposes them as public inputs. Confirm this aligns with protocol privacy goals.
105-109: LGTM on verification flow.Greco verification precedes CT‑add; final coefficient check remains. No issues.
YounesTal1
left a comment
YounesTal1
left a comment
There was a problem hiding this comment.
The ciphertext_addition.nr looks good. I have left one comment
4 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Chores