refactor: porting of greco circuit (aka greco) [skip-line-limit]#1260
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a new UserDataEncryption circuit (circuit, computation, codegen, sample, utils), migrates CRISP GRECO-based input flows to the new circuit and preset-driven APIs, removes greco-helpers and related workspace deps, adapts codegen/computation traits, updates clients/SDK/Noir configs, and introduces BFV↔Greco conversion and commitments. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
participant User as User/Caller
end
rect rgba(200,255,200,0.5)
participant Circuit as UserDataEncryptionCircuit
participant Computation as ComputationPipeline
participant Codegen as Codegen
participant Artifacts as ArtifactsStore
end
User->>Circuit: codegen(preset, input)
Circuit->>Computation: compute(preset, input)
Computation->>Computation: derive Configs → Bounds → Bits → Witness
Computation-->>Circuit: witness, bits, configs
Circuit->>Codegen: generate_toml(witness)
Codegen->>Codegen: format CodegenToml
Circuit->>Codegen: generate_configs(preset, configs)
Codegen->>Codegen: format CodegenConfigs
Codegen-->>Artifacts: Artifacts{toml, configs}
Artifacts-->>User: return artifacts
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
🧪 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
changed the title
cedoor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@examples/CRISP/crates/zk-inputs/src/lib.rs`:
- Around line 115-117: Replace redundant calculate_bit_width calls and re-use
the precomputed ct_bit from the circuit output: wherever code sets bit_ct from
calculate_bit_width(&bounds.pk_bounds[0].to_string()) (the local variable bit_ct
and the bounds = UserDataEncryptionBounds::compute(...) usage), instead read
user_data_encryption_computation_output.bits.ct_bit and assign that to bit_ct;
do the same replacement for the second occurrence that duplicates this logic.
For the third spot (the place currently creating Bits from a string), instead
compute a Bits value consistent with the circuit by constructing it from
user_data_encryption_computation_output.bits (e.g., derive the Bits instance
from ct_bit or the circuit bits structure) so the runtime matches the
circuit-determined widths; finally remove the now-unused
UserDataEncryptionBounds::compute call and any related temporary variables
(bounds, calculate_bit_width usage) to clean up dead code.
🧹 Nitpick comments (5)
crates/zk-helpers/src/utils.rs (1)
189-202: Consider adding unit tests forpolynomial_to_toml_json.The function is well-implemented, but unlike
crt_polynomial_to_toml_json, there are no tests covering this new utility. Consider adding a test case to verify the JSON structure matches expectations, especially since the output format differs (individual{ "coefficient": ... }objects vs.{ "coefficients": [...] }arrays).crates/zk-helpers/src/circuits/threshold/user_data_encryption/circuit.rs (1)
22-25: Consider derivingDebugforUserDataEncryptionCircuitInput.
UserDataEncryptionCircuitderivesDebug, butUserDataEncryptionCircuitInputdoes not. Adding#[derive(Debug)](ifPublicKeyandPlaintextsupport it) would improve consistency and aid debugging.crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs (2)
782-784: Unnecessary clone before sorting.
resultsis already an ownedVec, so the clone is redundant. You can sort it in place directly.-let mut results = results.clone(); +let mut results = results; results.sort_by_key(|(i, ..)| *i);
865-865: Redundant field name syntax.The
k1: k1uses redundant field initialization syntax.-k1: k1, +k1,examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs (1)
182-193: Align test bit_ct with the circuit bit width.The test helper derives
bit_ctfrompk_bounds[0]. To keep tests aligned with the circuit configs, preferBits::ct_bitfrom the user-data-encryption computation.♻️ Suggested refactor
@@ - let bounds = Bounds::compute(params, &()).unwrap(); - calculate_bit_width(&bounds.pk_bounds[0].to_string()).unwrap() + let bounds = Bounds::compute(params, &()).unwrap(); + let bits = e3_zk_helpers::threshold::Bits::compute(params, &bounds).unwrap(); + bits.ct_bit🧹 Cleanup (if no longer used)
-use e3_zk_helpers::utils::calculate_bit_width;
cedoor
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs`:
- Around line 151-159: The loop that computes k0qi and pushes into k0is uses
k0qi.to_u64().unwrap_or(0), which silently tolerates conversion failures;
replace the unwrap_or(0) with a checked conversion that returns an error instead
(e.g., k0qi.to_u64().ok_or_else(||
CircuitsErrors::Fhe(fhe::Error::MathError(fhe_math::Error::Default("Failed to
convert k0qi to u64".into()))))? ) and propagate the error with ? so failures in
converting k0qi (computed in the ctx.moduli_operators() loop) do not become 0s;
update the push to push the successful u64 into k0is (referencing variables k0qi
and k0is in computation.rs).
- Around line 566-567: Replace the panic-prone `.unwrap()` on the modular
inverse call so failures are propagated instead of panicking: change the code
that computes `koqi_u64 = qi.inv(qi.neg(t.modulus())).unwrap()` to handle the
`Option`/`Result` return from `qi.inv` (e.g., map/and_then or `match`) and
propagate an error (return a `Result`) up from the enclosing closure/function,
then construct `k0qi` only on success; update the enclosing closure/function
signature to return `Result<..., E>` and convert callers accordingly so errors
from `qi.inv(...)` are consistently handled instead of unwrapping.
- Around line 519-544: The closures building e0i and e0i_from_poly call
.as_slice().ok_or_else(...).unwrap() inside what is run in parallel, which can
panic and lose context; change these closures (where e0i and e0i_from_poly are
constructed using qi.center_vec_vt, e0_coeffs and e0_poly_coeffs) to return
Result<Vec<BigInt>, String> (or a suitable error type) instead of unwrapping,
propagate any as_slice() or center_vec_vt errors out of the closure, and then
collect the parallel results with collect::<Result<_,_>>() (or equivalent) so
callers can handle/report errors instead of panicking. Ensure you reference
qi.center_vec_vt, e0_coeffs, e0_poly_coeffs, e0i and e0i_from_poly when applying
the change.
- Around line 258-263: The code currently uses to_u128().unwrap() when computing
e0_bound (see e0_bound, params.get_error1_variance, and uniform_bound), which
can panic if uniform_bound > u128::MAX; change this to a safe, non-panicking
conversion and propagate the failure: replace the unwrap with a fallible
conversion (e.g., try_from or to_u128().ok_or(...)) and return or propagate an
error from the surrounding function instead of panicking, ensuring e0_bound
computation handles the Result/Option and keeps cbd_bound as the fallback
branch.
- Around line 324-354: The Bounds construction currently calls
.to_u128().unwrap() in multiple places (pk_bounds, k1_low_bound, k1_up_bound,
r1_low_bounds, r1_up_bounds, r2_bounds, p1_bounds, p2_bounds) which can panic on
overflow; update these conversions to use fallible conversions (e.g.
.to_u128().ok_or(...) or TryInto with .map_err(...)) and propagate a Result from
the enclosing function instead of panicking so overflows return a clear error;
locate the Bounds return in computation.rs (the Bounds { ... } block) and change
each map/assignment that uses .to_u128().unwrap() to return a mapped error on
failure and ensure the surrounding function signature and callers handle the
Result error accordingly.
🧹 Nitpick comments (2)
crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs (2)
276-276: Remove unused variable_num_moduli.The variable is declared but never used.
♻️ Suggested fix
- let _num_moduli = ctx.moduli().len();
868-868: Use field shorthand for cleaner code.
k1: k1can be simplified tok1.♻️ Suggested fix
- k1: k1, + k1,
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs`:
- Around line 382-385: The current code uses .to_bigint().unwrap() on e0_mod_q
which can panic; change the conversion to return a Result and propagate errors
instead: replace the map+collect into something like e0_mod_q.iter().map(|c|
c.to_bigint().ok_or_else(|| CircuitsErrors::Other(format!("failed converting
BigUint to BigInt: {:?}", c)))).collect::<Result<Vec<BigInt>,
CircuitsErrors>>()? so e0_bigints is produced without unwrap, then pass that
Vec<BigInt> into Poly::from_bigints(&e0_bigints, &ctx) as before and propagate
any errors from Poly::from_bigints using the existing map_err(...)?. Reference
symbols: e0_mod_q, e0_bigints, Poly::from_bigints, CircuitsErrors::Other.
In `@crates/zk-helpers/src/utils.rs`:
- Around line 195-207: The function polynomial_to_toml_json currently emits one
object per coefficient ("coefficient": ...), but consumers expect the schema
with a single object containing a "coefficients" array like
crt_polynomial_to_toml_json; change polynomial_to_toml_json to produce a single
serde_json::Value with key "coefficients" whose value is an array of the
coefficient strings (e.g., collect polynomial.coefficients().iter().map(|c|
c.to_string()) into a Vec and wrap it in serde_json::json!({ "coefficients": vec
})). Ensure the function still returns Vec<serde_json::Value> (e.g., return a
single-element Vec containing that object) and keep using the
Polynomial::coefficients() accessor.
🧹 Nitpick comments (2)
crates/zk-helpers/src/circuits/threshold/user_data_encryption/sample.rs (1)
25-37: Avoidunwrap()in a public generator; return a Result instead.This is a public API and will panic if encoding fails. Prefer returning a
Resultand letting callers decide how to handle the error.♻️ Suggested change (update call sites accordingly)
- pub fn generate(params: &Arc<BfvParameters>) -> Self { + pub fn generate(params: &Arc<BfvParameters>) -> Result<Self, fhe::Error> { let mut rng = thread_rng(); let secret_key = SecretKey::random(¶ms, &mut rng); let public_key = PublicKey::new(&secret_key, &mut rng); - let plaintext = Plaintext::try_encode(&[1u64], Encoding::poly(), ¶ms).unwrap(); + let plaintext = Plaintext::try_encode(&[1u64], Encoding::poly(), ¶ms)?; - Self { + Ok(Self { public_key, plaintext, - } + }) }- let sample = Sample::generate(¶ms); + let sample = Sample::generate(¶ms).expect("Failed to generate sample");crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs (1)
867-867: Minor: redundant bindingk1: k1.The field initialization can use shorthand syntax.
✨ Suggested fix
- k1: k1, + k1,
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs`:
- Line 392: The current conversion q_mod_t = (ctx.modulus() %
t.modulus()).to_u64().unwrap() can panic; replace the unwrap with a safe
conversion that returns a Result and propagate the error instead of panicking
(e.g., call to_u64().ok_or(...)? or try_from and use ?), following the file's
existing error-handling pattern; update the code around q_mod_t, referencing
ctx.modulus(), t.modulus(), and to_u64, so the function returns an error instead
of panicking when the modulus exceeds u64::MAX.
In `@crates/zk-helpers/src/circuits/threshold/user_data_encryption/utils.rs`:
- Around line 93-99: compute_pk_bit currently assumes params.moduli()[0] is the
largest modulus which can undercompute the bit width and cause truncated
commitments; update compute_pk_bit to first check that params.moduli() is
non-empty (return a ZkHelpersUtilsError on empty), iterate through all moduli to
find the maximum modulus, convert that max modulus into a BigInt, compute bound
= (max_modulus - 1) / 2 and pass that to calculate_bit_width; reference the
compute_pk_bit function and mirror the max-modulus selection pattern used in the
pk_bfv computation (crates/zk-helpers/src/circuits/pk_bfv/computation.rs) when
locating where to change the logic.
🧹 Nitpick comments (6)
crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs (6)
196-218: Consider using iterator.max()for cleaner code.The manual loops to find maximum bit widths could be simplified using iterator methods.
♻️ Optional refactor using iterator pattern
// For r1, use the maximum of all low and up bounds - let mut r1_bit = 0; - for bound in input.r1_low_bounds.iter().chain(input.r1_up_bounds.iter()) { - r1_bit = r1_bit.max(calculate_bit_width(BigInt::from(bound.clone()))); - } + let r1_bit = input + .r1_low_bounds + .iter() + .chain(input.r1_up_bounds.iter()) + .map(|b| calculate_bit_width(BigInt::from(b.clone()))) + .max() + .unwrap_or(0); // For r2, use the maximum of all bounds - let mut r2_bit = 0; - for bound in &input.r2_bounds { - r2_bit = r2_bit.max(calculate_bit_width(BigInt::from(bound.clone()))); - } + let r2_bit = input + .r2_bounds + .iter() + .map(|b| calculate_bit_width(BigInt::from(b.clone()))) + .max() + .unwrap_or(0); // For p1, use the maximum of all bounds - let mut p1_bit = 0; - for bound in &input.p1_bounds { - p1_bit = p1_bit.max(calculate_bit_width(BigInt::from(bound.clone()))); - } + let p1_bit = input + .p1_bounds + .iter() + .map(|b| calculate_bit_width(BigInt::from(b.clone()))) + .max() + .unwrap_or(0); // For p2, use the maximum of all bounds - let mut p2_bit = 0; - for bound in &input.p2_bounds { - p2_bit = p2_bit.max(calculate_bit_width(BigInt::from(bound.clone()))); - } + let p2_bit = input + .p2_bounds + .iter() + .map(|b| calculate_bit_width(BigInt::from(b.clone()))) + .max() + .unwrap_or(0);
546-559: Assertions in production code may cause silent failures in parallel context.Multiple
assert!andassert_eq!calls throughout the parallel closure (lines 546, 559, 577, 582, 586, 596, 602, 616-617, 628-629, etc.) will panic on failure. In a parallel context, these panics can be difficult to diagnose. Consider converting critical assertions toResultreturns, or documenting that these are invariant checks that indicate bugs rather than invalid inputs.
665-667: Consider more efficient leading zero removal.Using
remove(0)in a loop is O(n²) worst case. For polynomials with many leading zeros, this could be inefficient.♻️ Optional: More efficient approach
- while !ct0i_calculated.is_empty() && ct0i_calculated[0].is_zero() { - ct0i_calculated.remove(0); - } + let leading_zeros = ct0i_calculated.iter().take_while(|x| x.is_zero()).count(); + ct0i_calculated.drain(..leading_zeros);
784-786: Unnecessary clone before sorting.The
resultsvariable is already owned, so the clone is unnecessary.♻️ Remove unnecessary clone
// Sort by modulus index so CRT limbs are in order - let mut results = results.clone(); + let mut results = results; results.sort_by_key(|(i, ..)| *i);
867-867: Use field init shorthand.When the field name matches the variable name, shorthand syntax is preferred.
♻️ Use shorthand
- k1: k1, + k1,
902-911: Test uses hardcoded expected value that may break if parameters change.The assertion
assert_eq!(bounds.pk_bounds[0], BigUint::from(34359701504u64))will fail ifDEFAULT_BFV_PRESETis modified. Consider documenting why this specific value is expected or deriving it from the parameters.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs`:
- Around line 183-187: In compute (fn compute) the pk_bit and ct_bit are
computed from input.pk_bounds[0], which is incorrect when pk_bounds are not
ordered; change the logic to find the maximum modulus in input.pk_bounds (e.g.,
iterate input.pk_bounds to select the largest BigInt), then call
calculate_bit_width on that maximum to derive pk_bit and ct_bit (keep ct_bit
equal to the pk max as before); update references to pk_bounds[0] to use the
computed max value so pk_bit/ct_bit correctly reflect the largest pk bound.
🧹 Nitpick comments (1)
crates/zk-helpers/src/circuits/pk_bfv/computation.rs (1)
189-198: Test name may be misleading after refactor.The test
test_bound_and_bits_computation_consistencyno longer verifies consistency between bounds and bits—it now independently checks each. The assertionbits.pk_bit == expected_bitsis tautological since both usecompute_pk_bit. Consider either:
- Renaming to
test_bound_and_bits_computationto reflect the decoupled nature, or- Adding an assertion that
pk_bitis sufficient to representpk_bound(e.g.,pk_bound.bits() <= pk_bit as u64).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs`:
- Around line 177-181: The docstring for the method to_json on the
CiphertextAddition witness incorrectly states it returns a "JSON string" while
the signature returns a serde_json::Value; update the doc comment for to_json to
say it returns a serde_json::Value (a JSON value/object representation) and
clarify that callers get a JSON value rather than a serialized string so they
can call to_string()/serde_json::to_string if they need a string.
In `@examples/CRISP/crates/zk-inputs/src/lib.rs`:
- Around line 95-101: The code calls UserDataEncryptionCircuit::compute with the
hard-coded DEFAULT_BFV_PRESET which can mismatch generators built with custom
params (see ZKInputsGenerator::from_set and ZKInputsGenerator::new); update the
calls (including the other occurrence around lines 159-165) to pass the
generator's actual BFV preset/params (self.bfv_params or the stored preset) into
UserDataEncryptionCircuit::compute instead of DEFAULT_BFV_PRESET so the computed
ciphertext/witness aligns with the generator's parameters.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs`:
- Line 189: The code uses let max_pk_bound =
input.pk_bounds.iter().max().unwrap(); which will panic if input.pk_bounds is
empty; replace this unwrap with an explicit check of input.pk_bounds.is_empty()
and handle the empty-case by returning a proper error (or Option) instead of
panicking—e.g., return Err(...) or propagate a descriptive error (create/return
a domain-specific error like EmptyPkBounds) from the surrounding function;
ensure all call sites handle the Result/Option and remove the unwrap on
max_pk_bound.
🧹 Nitpick comments (2)
crates/zk-helpers/src/circuits/dkg/pk/computation.rs (1)
194-203: Test verifies internal consistency but not cross-type consistency.The test checks that
bits.pk_bit == compute_pk_bit(&dkg_params), but this doesn't validate thatpk_bitis actually sufficient to representpk_bound. Consider adding an assertion that verifies the bit width can accommodate the bound:// Verify pk_bit is sufficient to represent pk_bound assert!( bounds.pk_bound < BigUint::from(1u32) << bits.pk_bit, "pk_bit {} insufficient to represent pk_bound {}", bits.pk_bit, bounds.pk_bound );This would catch any mismatch between
Bounds::compute(which uses max modulus) andcompute_pk_bit(which may not).crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs (1)
882-882: Redundant field initialization syntax.The field
k1: k1can be simplified using shorthand syntax.🛠️ Suggested fix
- k1: k1, + k1,
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/bfv-client/src/client.rs`:
- Around line 104-118: The code currently calls
UserDataEncryptionWitness::compute with the hardcoded DEFAULT_BFV_PRESET while
earlier code builds params from degree, plaintext_modulus, and moduli, causing a
mismatch; fix by deriving/constructing the same preset from those params (or
accept a preset identifier) and pass that derived preset into
UserDataEncryptionWitness::compute instead of DEFAULT_BFV_PRESET so the preset
used to deserialize the public key and encode the plaintext matches the preset
used to compute the witness (ensure consistency between params, the public key
deserialization, plaintext encoding, and the call to
UserDataEncryptionWitness::compute).
In `@packages/enclave-sdk/src/greco.ts`:
- Line 53: The current call uses a blind cast "circuitInputs as any" when
calling noir.execute, which disables type safety; replace it by importing and
using the proper InputMap type from `@noir-lang/noir_js` (or upgrade the package
if that version doesn't export InputMap) and cast/declare circuitInputs as
InputMap before calling noir.execute (referencing noir.execute and circuitInputs
in packages/enclave-sdk/src/greco.ts); if InputMap is unavailable in the beta,
either upgrade to a release that exports InputMap or create a local typed alias
matching InputMap shape and use that type for circuitInputs and the noir.execute
call.
🧹 Nitpick comments (2)
crates/wasm/src/lib.rs (1)
45-55: Consider adding detailed documentation for consistency.Other wasm-exported functions in this file (e.g.,
bfv_encrypt_number,compute_pk_commitment) include comprehensive docstrings with Arguments, Returns, and Panics sections. This new function has only a single-line comment.📝 Suggested documentation enhancement
-/// Generate a public key from JavaScript. +/// Generate a BFV public key. +/// +/// # Arguments +/// +/// * `degree` - Polynomial degree for BFV parameters +/// * `plaintext_modulus` - Plaintext modulus for BFV parameters +/// * `moduli` - Moduli for BFV parameters +/// +/// # Returns +/// +/// Returns a `Result<Vec<u8>, JsValue>` containing the serialized public key bytes. +/// +/// # Panics +/// +/// Panics if key generation fails #[wasm_bindgen] pub fn generate_public_key(crates/bfv-client/src/client.rs (1)
108-108: Redundant field name syntax.Clippy will flag
plaintext: plaintextas a redundant field name.♻️ Minor fix
let witness = UserDataEncryptionWitness::compute( DEFAULT_BFV_PRESET, &UserDataEncryptionCircuitInput { public_key: pk, - plaintext: plaintext, + plaintext, }, )?;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re #1259
Closes #1227
Summary by CodeRabbit
New Features
Refactor
Chores