feat: add share-decryption circuit gen and refactoring [skip-line-limit]#1269
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates share-decryption bit-message constants into a single Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI/zk_cli
participant Sample as SampleGen
participant Compute as CircuitComputation
participant Codegen as CircuitCodegen
participant Artifacts as ArtifactsStore
CLI->>Sample: generate_sample(preset, committee, dkg_input_type)
Sample-->>CLI: ShareDecryptionCircuitInput
CLI->>Compute: compute(preset, input)
Compute->>Compute: Configs::compute / Bits::compute / Bounds::compute / Witness::compute
Compute-->>CLI: ShareDecryptionOutput
CLI->>Codegen: codegen(preset, input)
Codegen->>Codegen: generate_toml(witness), generate_configs(preset, configs)
Codegen-->>Artifacts: Artifacts { toml, configs }
Artifacts-->>CLI: Ready for prover
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Line 167: The current call
input.secret_key.try_decrypt(&party_cts[mod_idx]).unwrap() will panic on
failure; replace unwrap() with the ? operator to propagate the decryption error
(e.g., let decrypted_pt = input.secret_key.try_decrypt(&party_cts[mod_idx])?;),
update the enclosing function's signature to return a Result type (propagating
the try_decrypt error type or mapping it into your crate's error enum), and
update any callers to handle the returned Result accordingly so decryption
failures are handled instead of panicking.
- Around line 150-189: In compute (fn compute) avoid panicking by replacing the
.unwrap() on input.secret_key.try_decrypt with error propagation: call
try_decrypt(...).map_err(|e| CircuitsErrors::Sample(e.to_string()))? so
decryption failures return Err instead of panic; keep building
expected_commitments and decrypted_shares and return Ok(Witness {
expected_commitments, decrypted_shares }) as before.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs`:
- Around line 174-176: The assertion incorrectly compares
sample.sum_ciphertexts.len() to committee.threshold; instead assert it equals
the number of CRT moduli used to build it (i.e.,
threshold_params.moduli().len()). Update both tests
(test_generate_secret_key_sample and test_generate_smudging_noise_sample) to
replace the second assert_eq! so it checks sample.sum_ciphertexts.len() ==
threshold_params.moduli().len(), referencing sample.sum_ciphertexts,
threshold_params.moduli(), and committee.threshold to locate the code.
🧹 Nitpick comments (5)
crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (2)
61-115: Inner loop generates the sameshare_rowfor every TRBFV basis — intentional?The modulus loop (line 65) iterates
threshold_params.moduli().len()times, but theshare_rowcomputed inside (e.g.,sk_sss_u64[0].row(0)at line 78) doesn't vary with the loop index. Every ciphertext within a single party encrypts the same plaintext data, just with different encryption randomness.For a test sample generator this is probably fine, but the comment on line 130 ("The message should be the same for all TRBFV bases") is a consequence of this decision, not an inherent property of the scheme. A brief comment at line 65 noting that the same share is reused across bases for simplicity would help future readers.
94-96:rng.clone()produces a duplicate RNG state.Cloning the RNG at line 96 means
generate_secret_shares_from_polyreceives a copy with the same state. If the originalrngis used afterward in the same iteration, both could produce identical sequences. This is acceptable in test sample code per project conventions (learnings confirm RNG reuse in tests is fine), but a brief// clone is fine for test samplescomment would prevent future reviewers from flagging it again.crates/zk-helpers/src/bin/zk_cli.rs (1)
334-353: New generation branch looks correct and consistent.The match arm mirrors the
ShareEncryptionCircuitpattern (lines 288–313), correctly fetchingsearch_defaults, generating a sample, and constructingShareDecryptionCircuitInputwith the right fields.As a minor note, the
search_defaults/ sample-generation / codegen pattern is now duplicated across three arms (ShareComputation,ShareEncryption,ShareDecryption). If more circuits follow, consider extracting a shared helper. Not blocking for this PR.crates/zk-helpers/src/circuits/dkg/share_decryption/codegen.rs (1)
51-72:dkg_counterpart().unwrap()can panic for non-DKG presets on apubfunction.
generate_configsis public, but the two.unwrap()calls on line 67–68 will panic if a caller passes a preset without a DKG counterpart. The CLI validates this upstream, but a standalone caller of this API won't.Consider returning a
Resultor, at minimum, extracting the counterpart once and usingexpectwith a clear message.Proposed fix
-pub fn generate_configs(preset: BfvPreset, configs: &Configs) -> CodegenConfigs { +pub fn generate_configs(preset: BfvPreset, configs: &Configs) -> Result<CodegenConfigs, CircuitsErrors> { let prefix = <ShareDecryptionCircuit as Circuit>::PREFIX; + let dkg = preset + .dkg_counterpart() + .ok_or_else(|| CircuitsErrors::Sample("preset has no DKG counterpart".into()))?; - format!( + Ok(format!( r#"pub global N: u32 = {}; pub global L: u32 = {}; ... pub global {}_BIT_MSG: u32 = {}; "#, - preset.dkg_counterpart().unwrap().metadata().degree, - preset.dkg_counterpart().unwrap().metadata().num_moduli, + dkg.metadata().degree, + dkg.metadata().num_moduli, prefix, configs.bits.msg_bit, - ) + )) }This would also require updating the call site in
codegen()(line 32) to propagate the error.crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs (1)
99-117:Configs::computecallsBounds::compute(preset, &input)— the extra&is redundant.
inputis already&ShareDecryptionCircuitInput, so&inputintroduces an extra indirection layer that is resolved via deref coercion. Passinginputdirectly is clearer.Proposed fix
- let bounds = Bounds::compute(preset, &input)?; + let bounds = Bounds::compute(preset, input)?;
0xjei
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/circuits/crt.rs`:
- Around line 42-57: In reconstruct_crt_coefficients: stop silently falling back
to BigInt::from(1u64) when mod_inverse returns None and avoid unguarded
crt.limb(0) access; instead, validate that crt has at least one limb (return Err
or panic with a clear message if empty) and replace the unwrap_or_else on
mod_inverse with a failure that surfaces the problem (e.g., use expect or
propagate a Result) including context (i, qi, q_i) so missing inverses
(non-coprime or duplicate moduli) fail loudly and can be diagnosed; keep the
rest of the CRT loop logic the same.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs`:
- Around line 140-142: The comment above the decryption call is misleading: in
this sample generator each TRBFV basis encrypts an independent plaintext (the
inner loop creates a fresh secret per (party, modulus)), so the messages are not
the same across bases; update the comment at the
`dkg_secret_key.try_decrypt(&sum_ciphertexts[0]).unwrap()` line to state that
only the aggregate plaintext for basis 0 is being decrypted/stored and that
other bases contain unrelated plaintexts (or, if intended, change the code to
decrypt each entry in `sum_ciphertexts` instead). Ensure you reference
`sum_ciphertexts` and `dkg_secret_key.try_decrypt` in the updated comment to
make the behavior clear.
🧹 Nitpick comments (11)
crates/zk-helpers/src/circuits/ring.rs (1)
33-87: Potential underflow in degree assertions when polynomial length is zero.The pattern
(poly.coefficients().len() as u64) - 1(Lines 50, 58, 66, 72, 76) will wrap around tou64::MAXif the polynomial has zero coefficients, causing a confusing panic message instead of a meaningful error. While this scenario is not expected with valid FHE inputs, a guard or a descriptive message would help debugging if it ever occurs.Example defensive check (optional)
- let num_coeffs = xi.sub(xi_hat).coefficients().to_vec(); - assert_eq!((num_coeffs.len() as u64) - 1, 2 * (n - 1)); + let num_coeffs = xi.sub(xi_hat).coefficients().to_vec(); + assert!( + !num_coeffs.is_empty(), + "decompose_residue: xi - xi_hat produced an empty polynomial" + ); + assert_eq!((num_coeffs.len() as u64) - 1, 2 * (n - 1));crates/zk-helpers/src/circuits/parity.rs (1)
44-65: Noir constant dimensions are hardcoded — ensure they stay in sync with the circuit template.The dimension string
[[[Field; N_PARTIES + 1]; N_PARTIES - T]; L_THRESHOLD]on Line 62 is embedded as a literal. If the Noir circuit template changes these constant names (e.g.,L_THRESHOLD→L), this will silently produce invalid Noir code. Consider extracting these dimension names as constants or parameters if the names are expected to evolve.This isn't blocking since the same approach is used by the
generate_configsfunction incodegen.rs.crates/zk-helpers/src/bin/zk_cli.rs (1)
208-211: Stale comment: "Only share-computation has a witness-type choice" no longer accurate.The comment on line 208 states that only share-computation has a witness-type choice, but
has_witness_typenow also includesShareEncryptionCircuitandShareDecryptionCircuit.✏️ Suggested comment fix
- // Only share-computation has a witness-type choice (secret-key vs smudging-noise). pk always uses secret key. + // share-computation, share-encryption, and share-decryption have a witness-type choice (secret-key vs smudging-noise).crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs (1)
207-207: Minor inconsistency:circuit_input_from_sampleis a free function here, but an associated method on other sample types.
ShareEncryptionCircuitusescircuit_input_from_sample(&sample, preset)as a free function, whilePkSample::circuit_input_from_sample(...)andUserDataEncryptionSample::circuit_input_from_sample(...)are associated methods. This isn't a bug, but aligning the API surface would improve discoverability.Also applies to: 227-227
crates/zk-helpers/src/circuits/threshold/user_data_encryption/sample.rs (1)
29-64: Non-deterministic encryption insidecircuit_input_from_sample.Each call to
circuit_input_from_sampleperforms a freshtry_encrypt_extendedwiththread_rng()(line 41), producing different ciphertexts and randomness each time. This is fine for test/codegen use, but it differs from theShareEncryptionSamplepattern where the ciphertext is pre-stored in the sample.If deterministic replay matters for debugging, consider storing the ciphertext and randomness in
UserDataEncryptionSample(likeShareEncryptionSampledoes) and converting them here. Otherwise, the current approach is acceptable for its purpose.crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
113-142: Inconsistent API placement: free function vs. associated method.
circuit_input_from_sampleis a free function here, whereasShareComputationSample,UserDataEncryptionSample, andPkSample(seecrates/zk-helpers/src/circuits/dkg/pk/sample.rslines 29–41) all define it as an associated method on their respective sample types. Consider moving this intoimpl ShareEncryptionSamplefor discoverability and consistency.Suggested restructure
-/// Builds [`ShareEncryptionCircuitInput`] from a sample by converting FHE polynomials to CRT form -/// (same pattern as [`crate::threshold::pk_generation::PkGenerationCircuitInput::generate_sample`]). -pub fn circuit_input_from_sample( - sample: &ShareEncryptionSample, - preset: BfvPreset, -) -> Result<ShareEncryptionCircuitInput, CircuitsErrors> { +impl ShareEncryptionSample { + // ... existing generate() ... + + /// Builds [`ShareEncryptionCircuitInput`] from a sample by converting FHE polynomials to CRT form. + pub fn circuit_input_from_sample( + sample: &ShareEncryptionSample, + preset: BfvPreset, + ) -> Result<ShareEncryptionCircuitInput, CircuitsErrors> { + // ... body unchanged ... + } +}crates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rs (1)
311-341: Multiple.to_u128().unwrap()calls on bounds could panic for large parameter sets.All bound values are converted via
.to_u128().unwrap(). If the cryptographic parameters ever produce bounds exceedingu128::MAX, these will panic. For current insecure/test presets this is fine, but production presets with larger moduli could be a concern.Consider propagating errors instead of unwrapping, or at minimum adding a comment noting the u128 assumption.
crates/zk-helpers/src/circuits/dkg/share_decryption/codegen.rs (2)
54-72: Duplicate.unwrap()onpreset.dkg_counterpart()— extract and propagate error.Lines 67 and 68 each call
preset.dkg_counterpart().unwrap()independently. If the preset has no DKG counterpart, this panics in codegen. Extract the result once and convertNoneto an error sincegenerate_configscould returnResult.Alternatively, if the caller guarantees a valid preset, at least deduplicate:
Proposed fix: deduplicate and fail gracefully
pub fn generate_configs(preset: BfvPreset, configs: &Configs) -> CodegenConfigs { let prefix = <ShareDecryptionCircuit as Circuit>::PREFIX; + let dkg_meta = preset.dkg_counterpart().expect("preset must have a DKG counterpart").metadata(); format!( r#"pub global N: u32 = {}; pub global L: u32 = {}; /************************************ ------------------------------------- share_decryption_sk (CIRCUIT 4a - BFV DECRYPTION SK) share_decryption_e_sm (CIRCUIT 4b - BFV DECRYPTION E_SM) ------------------------------------- ************************************/ pub global {}_BIT_MSG: u32 = {}; "#, - preset.dkg_counterpart().unwrap().metadata().degree, - preset.dkg_counterpart().unwrap().metadata().num_moduli, + dkg_meta.degree, + dkg_meta.num_moduli, prefix, configs.bits.msg_bit, ) }
42-48: Nit: simplifymap_errclosure to method reference.Minor style fix
let json = witness .to_json() - .map_err(|e| CircuitsErrors::SerdeJson(e))?; + .map_err(CircuitsErrors::SerdeJson)?;crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs (1)
344-579: Witness computation is structurally near-identical touser_data_encryption.The limb-wise parallel CRT processing, e0 reconstruction, decompose_residue calls, and CrtPolynomial aggregation in this file closely mirror the same logic in
threshold/user_data_encryption/computation.rs(lines 350–554 there). The key differences are:
- Uses
messagefield instead ofk1- Different commitment functions (
compute_dkg_pk_commitment+compute_share_encryption_commitment_from_messagevscompute_pk_aggregation_commitment+compute_ciphertext_commitment)- Uses DKG params instead of threshold params
Given these circuits have genuinely different semantics, the duplication is understandable. Consider extracting the shared CRT-limb-processing core into a helper if a third circuit needs the same pattern.
crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs (1)
94-117: Inconsistent error mapping betweenConfigs::computeandWitness::compute.Line 101 maps
build_pair_for_preseterrors toZkHelpersUtilsError::ParseBound, while line 152 maps the same call toCircuitsErrors::Sample. Using a consistent error mapping pattern within the same module would improve clarity.
|
@cedoor ready |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
135-160:⚠️ Potential issue | 🔴 CriticalBug:
test_generate_smudging_noise_sampletests the wrongDkgInputType.Line 141 passes
DkgInputType::SecretKeyinstead ofDkgInputType::SmudgingNoise. This is a copy-paste error — the smudging noise code path is completely untested.🐛 Proposed fix
let sample = ShareEncryptionCircuitInput::generate_sample( BfvPreset::InsecureThreshold512, CiphernodesCommitteeSize::Small, - DkgInputType::SecretKey, + DkgInputType::SmudgingNoise, sd.z, sd.lambda, );
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Line 268: preset.search_defaults().unwrap() can panic in production; replace
the unwrap with graceful handling by checking the result of
preset.search_defaults() (e.g., match or if let) and emit a clear user-facing
error and non-zero exit when no search defaults are present. Locate the call
that binds sd from preset.search_defaults() in zk_cli.rs (the sd variable) and
change it to handle the None/Err case: print/log a descriptive message including
which preset failed and exit with a non-zero status (or return a Result::Err) so
the CLI doesn't panic.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Around line 164-179: The loop over mod_idx silently skips when threshold_l >
party_cts.len(), causing party_commitments and party_shares to be shorter than
expected; update the code in the block that builds
party_commitments/party_shares (the loop using threshold_l, party_cts,
input.secret_key.try_decrypt, compute_share_encryption_commitment_from_message)
to explicitly enforce the invariant by either (a) checking upfront that
party_cts.len() >= threshold_l and returning an Err or panicking with a clear
message, or (b) replacing the conditional inside the loop with an explicit error
when mod_idx >= party_cts.len(); ensure the function returns a proper Result or
panics consistently so downstream consumers always see exactly threshold_l
entries per party.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs`:
- Around line 97-112: The computed but unused variable sum_ciphertexts should
either be removed and the module docstring updated, or actually stored in the
ShareDecryptionCircuitInput returned by build (i.e., add a sum_ciphertexts:
Vec<Ciphertext> field to the ShareDecryptionCircuitInput struct and populate it
when constructing the value in the function that builds the input); update any
struct constructors/serializers and call sites to handle the new field, and
update the module doc (the docstring that promises "sum ciphertexts" and
"message") to match the chosen behavior.
- Around line 132-136: The assertion for secret key degree uses
DEFAULT_BFV_PRESET but the test actually builds parameters for a specific preset
(e.g., BfvPreset::InsecureThreshold512); update the test to derive the expected
degree from the actual preset used to build the DKG parameters (call
build_pair_for_preset(...) with the explicit preset where the committee and
sample are created) and replace the DEFAULT_BFV_PRESET.metadata().degree
reference in the assertion comparing sample.secret_key.coeffs.len() with the
preset.metadata().degree so the test uses the same preset as the generated
secret key.
🧹 Nitpick comments (7)
crates/zk-helpers/src/circuits/dkg/share_computation/utils.rs (1)
44-45: Nit: simplify themap_errclosure.The closure
|e| CircuitsErrors::Sample(e)can be replaced with a method reference.♻️ Suggested simplification
- .map_err(|e| CircuitsErrors::Sample(e))?; + .map_err(CircuitsErrors::Sample)?;crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
82-87: Nit:_ctprefix is misleading since the value is used.The underscore prefix in
_ctconventionally signals an unused binding, but it's assigned tosample.ciphertexton line 87. Consider renaming toct.♻️ Suggested rename
- let (_ct, u_rns, e0_rns, e1_rns) = + let (ct, u_rns, e0_rns, e1_rns) = dkg_public_key.try_encrypt_extended(&pt, &mut rng).unwrap(); ShareEncryptionCircuitInput { plaintext: pt, - ciphertext: _ct, + ciphertext: ct,crates/zk-helpers/src/circuits/dkg/share_computation/sample.rs (1)
33-33: Nit:search_defaults()is computed unconditionally but only used in theSmudgingNoisebranch.Consider moving
preset.search_defaults().unwrap()inside theSmudgingNoisearm to avoid unnecessary computation in theSecretKeypath.crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs (1)
349-358:pk1isis assignedaby move afterais cloned — consider a brief comment.
pk1is: areuses the moveda(which equals pk1 in BFV). The clone on line 350 (a: a.clone()) goes to theafield while the original is moved topk1is. This is correct and avoids a redundant clone, but the semantic equivalence ofpk1isandamight surprise a future reader.crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (1)
74-76:rng.clone()is a no-op forThreadRng.
ThreadRngdelegates to the thread-local PRNG; cloning it produces another handle to the same underlying state. The.clone()on line 76 is misleading — it doesn't create an independent RNG. Just pass&mut rngdirectly.Suggested fix
let esi_sss_u64 = share_manager - .generate_secret_shares_from_poly(esi_poly.clone(), &mut rng.clone()) + .generate_secret_shares_from_poly(esi_poly.clone(), &mut rng)crates/zk-helpers/src/ring.rs (1)
33-88: Consider documenting preconditions more explicitly fordecompose_residue.The function has strict mathematical preconditions on its inputs (e.g.,
ximust have degreen-1,xi_hatmust have degree2*(n-1),cyclomust be the cyclotomic polynomial of degreen). Violations cause panics at various assertion points, which is documented at a high level, but callers would benefit from knowing exactly what input shapes are expected.The assertions themselves are valuable as they verify the mathematical invariants at each step — this is appropriate for ZK witness computation where silent corruption would be far worse than a panic.
crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs (1)
99-101: Inconsistent error handling inConfigs::compute—use directCircuitsErrors::Samplefor consistency withWitness::compute.Both
Configs::compute(line 99) andWitness::compute(line 152) handle errors frombuild_pair_for_preset(preset)with the same trait return type (CircuitsErrors), but they map errors differently:
Configs::computemaps throughZkHelpersUtilsError::ParseBound(indirect, via From impl)Witness::computeusesCircuitsErrors::Sample(direct)The
ParseBoundvariant is semantically misleading here—the error isn't about parsing bounds, it's about building parameters. Change to match the pattern established byWitness::compute:Proposed fix
- let (_, dkg_params) = build_pair_for_preset(preset) - .map_err(|e| crate::utils::ZkHelpersUtilsError::ParseBound(e.to_string()))?; + let (_, dkg_params) = build_pair_for_preset(preset) + .map_err(|e| CircuitsErrors::Sample(e.to_string()))?;
2 participants
Summary by CodeRabbit
New Features
Refactor