refactor: improve c7 circuit computation [skip-line-limit]#1315
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors circuit configurations by systematically renaming DKG share encryption constants, introducing new modulus parameters, removing ZKP modulus-based reduction operations, refactoring data structures from raw BigInt vectors to polynomial abstractions, and updating benchmark reports with new execution metrics. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
0xjei
force-pushed
the
ref/circuits-comp
branch
from
6aa2188 to
2403461
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs (1)
83-98:⚠️ Potential issue | 🟡 MinorStale doc comments reference ZKP modulus reduction that was removed.
Lines 84–86 state coefficients are "reduced to the ZKP field modulus for serialization," and lines 90/92 reference
[0, zkp_modulus). The ZKP modulus reduction has been removed in this refactor, so these comments are now inaccurate. Similarly, the module-level doc (lines 10–11) mentions normalization "to the ZKP field modulus."Consider updating the doc comments to reflect that values are now in
[0, q_j)per modulus without further ZKP reduction.crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs (1)
529-546:⚠️ Potential issue | 🟠 Major
e0_mod_qis not reduced by ZKP modulus, unlike inuser_data_encryption.In
user_data_encryption/computation.rs(lines 544-546),e0_mod_q.reduce(&zkp_modulus)is explicitly called before storinge0in the output, with a comment explaining the reduction is necessary so coefficients fit in the proof system field. Here,e0_mod_qis stored directly (line 540) with centered coefficients that can be as large as ±Q/2 (~2^100+ for insecure, ~2^200+ for secure). Add the missingreduce()call to matchuser_data_encryptionor clarify whyshare_encryptionintentionally omits it.crates/zk-prover/src/circuits/utils.rs (1)
49-64:⚠️ Potential issue | 🔴 CriticalNegative coefficients serialized as JSON numbers will fail to parse.
FieldElement::try_from_strin Noir v1.0.0-beta.15 does not support negative decimal strings. Whenbigint_to_json_valueemits a negative coefficient as a JSON number (e.g.,-5for values fitting in i64), the parsing code converts it to the string"-5"and passes it totry_from_str, which returnsNonesince it only recognizes hex strings (0x...) or unsigned decimals. This causes any negative coefficient in the range[-i64::MAX, -1]to fail with aSerializationError.Fix: Either parse the magnitude and negate in field arithmetic (strip the
-, parse, then negate), or emit negative coefficients as strings frombigint_to_json_valueto bypass theas_i64()path entirely.
🧹 Nitpick comments (6)
examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs (1)
74-83: Remove now-unnecessarymutqualifiers.With the ZKP modulus reduction block gone,
prev_ct0,prev_ct1,sum_ct0,sum_ct1,r0, andr1are never mutated. The compiler will emitunused_mutwarnings.♻️ Suggested diff
- let [mut prev_ct0, mut prev_ct1, mut ct0, mut ct1, mut sum_ct0, mut sum_ct1] = + let [prev_ct0, prev_ct1, ct0, ct1, sum_ct0, sum_ct1] = crt_polynomials; ... - let mut r0 = Self::compute_quotient(&sum_ct0, &ct0, &prev_ct0, &moduli) + let r0 = Self::compute_quotient(&sum_ct0, &ct0, &prev_ct0, &moduli) .with_context(|| "Failed to compute r0 quotient")?; - let mut r1 = Self::compute_quotient(&sum_ct1, &ct1, &prev_ct1, &moduli) + let r1 = Self::compute_quotient(&sum_ct1, &ct1, &prev_ct1, &moduli) .with_context(|| "Failed to compute r1 quotient")?;examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
160-196: Consider extracting the repeated parsing into a helper.The ct0is and ct1is parsing blocks (lines 160-177 and 179-196) are nearly identical. A small helper like
fn parse_crt_arrays(input: &JsValue, name: &str) -> Result<Vec<Vec<BigInt>>, JsValue>would reduce duplication and ensure both paths stay in sync.♻️ Sketch of extracted helper
fn parse_crt_arrays(input: &JsValue, name: &str) -> Result<Vec<Vec<BigInt>>, JsValue> { let outer = js_sys::Array::from(input); let mut result: Vec<Vec<BigInt>> = Vec::new(); for i in 0..outer.length() { let inner = outer .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.length() { let s = js_value_to_bigint_string(inner.get(j)) .ok_or_else(|| JsValue::from_str("Expected string, number, or BigInt 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) }circuits/benchmarks/scripts/run_benchmarks.sh (1)
216-222: Raw JSON deletion is unconditional — consider gating it behind--clean.The raw benchmark data is removed after every run regardless of flags. If someone needs to debug or re-process raw results, they'd have to re-run the full benchmark. Since the script already has a
--cleanflag for artifact cleanup, it would be more consistent to gate this removal behind the same flag (or a separate--keep-rawopt-out).That said, if the intent is that raw data is strictly ephemeral and only the Markdown report matters, this is fine as-is.
crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs (2)
137-163: Minor inconsistency:q_mod_tis not reduced by ZKP modulus here, but is inshare_encryption.In this file (line 147),
q_mod_tis stored ascenter(q_mod_t_uint, t)without the ZKP modulus reduction. Incrates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs(lines 159–169), the equivalent field still appliesreduce(&q_mod_t, &p)before storing. Both produce the same value in practice since|q_mod_t| ≤ t/2which is tiny relative to the ZKP modulus, but it's worth aligning the two for consistency.
634-646: Test doesn't assertq_mod_troundtrip.The JSON roundtrip test checks
n,l,moduli,bits, andboundsbut omits the newq_mod_tfield. Consider adding an assertion for completeness.Suggested addition
assert_eq!(decoded.n, constants.n); assert_eq!(decoded.l, constants.l); assert_eq!(decoded.moduli, constants.moduli); + assert_eq!(decoded.q_mod_t, constants.q_mod_t); assert_eq!(decoded.bits, constants.bits); assert_eq!(decoded.bounds, constants.bounds);circuits/benchmarks/results_secure/report.md (1)
69-70: Minor: Quoted numeric values in detail tables but not in summary tables.Values like
"2949141"and"11539441"have surrounding quotes in the Circuit Details section, while the summary tables use unquoted numbers. This is likely a benchmark script formatting artifact — consider normalizing the output in the script for consistency.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor