Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR restructures the circuits module from a single Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
|
tested both secure & insecure, will update insecure params in a subsequent PR to check if things break or not |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/README.md`:
- Around line 30-32: The README incorrectly states that `--toml` "Skip[s]
writing Prover.toml"; update docs or the flag to match code: either rename the
CLI flag to `--no-toml` (change occurrences of args.toml usage in zk_cli.rs and
related help text) if the intent is to skip writing Prover.toml, or change the
README line to state that `--toml` enables writing Prover.toml (reflecting the
code path where let toml = if !args.toml { None } else { Some(&artifacts.toml)
}; and the file `Prover.toml` / `artifacts.toml` is written when args.toml is
true).
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Around line 202-223: The boolean branch for determining witness_str is
inverted: change the if condition on args.toml in the block that builds
dkg_input_type so that when configs-only (args.toml == false) you use
args.witness.unwrap_or("secret-key") and when generating Prover.toml (args.toml
== true) you require args.witness and return an error if missing; update the
inline comments accordingly; keep the rest of the flow (call parse_input_type,
match DkgInputTypeArg to DkgInputType) unchanged so functions/idents like
args.witness, witness_str, parse_input_type, DkgInputTypeArg, DkgInputType and
circuit_meta.dkg_input_type() are used as before.
- Around line 140-142: The CLI flag `toml` has inverted semantics: its docstring
says "Skip generating Prover.toml" but passing `--toml` currently enables
generation (checked via `args.toml` in the Prover.toml generation code), so
rename the flag to a positively named option like `write_toml` or
`with_prover_toml` (update the #[arg(...)] attribute name and help text to
"Write/Generate Prover.toml") and set default_value to "false"; alternatively,
if you prefer a skip-style flag, rename to `skip_toml` and invert the default
and runtime checks. Update all references to the CLI field (`toml` /
`args.toml`) used in the Prover.toml generation logic to the new field name and
adjust the help string to clearly match the behavior.
- Around line 83-88: The print branch logic is inverted/confusing because the
boolean toml_only (set from args.toml) doesn't match the messages; update the
code so the boolean name and branches are consistent: rename toml_only to
write_prover_toml (or similar) and invert the condition/printed messages
accordingly so when write_prover_toml is true you print both "configs.nr" and
"Prover.toml", and when false you print only "configs.nr only (--toml:
Prover.toml skipped)"; adjust any other uses (e.g., where toml_only is read at
lines ~276-280) to the new name to keep behavior unchanged but semantics clear.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/circuit.rs`:
- Around line 17-24: The const DKG_INPUT_TYPE on ShareComputationCircuit is
incorrectly hard-coded with a const match that always yields
Some(DkgInputType::SecretKey) while the circuit actually accepts runtime-varying
inputs via the input.dkg_input_type field and dispatches on both SecretKey and
SmudgingNoise in share_computation::computation.rs; update
ShareComputationCircuit by either removing the DKG_INPUT_TYPE constant or
setting it to None so the metadata correctly indicates multi-type runtime inputs
(modify the declaration of DKG_INPUT_TYPE in the impl for
ShareComputationCircuit accordingly).
In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 144-200: generate_configs currently hardcodes
CiphernodesCommitteeSize::Small when building the parity matrix which produces
incorrect parity for other committee sizes; update generate_configs to accept
the committee size (or explicit n and threshold) as an argument and use that
value when calling parity_matrix_constant_string instead of
CiphernodesCommitteeSize::Small — replace the hardcoded committee =
CiphernodesCommitteeSize::Small.values() with the passed-in committee (or
construct committee from the provided n/threshold), call
parity_matrix_constant_string(&threshold_params, committee.n,
committee.threshold), and update all callers of generate_configs to supply the
appropriate committee size (or n/threshold).
🧹 Nitpick comments (3)
circuits/lib/src/configs/secure/dkg.nr (1)
79-81: Minor: Comment mismatch with constant name.Line 79 comment reads
verify_shares - configsbut the constant defined isSHARE_COMPUTATION_E_SM_CONFIGS. Consider aligning the comment with the constant name for consistency with the rest of the file.📝 Suggested fix
-// verify_shares - configs +// share_computation_e_sm - configs pub global SHARE_COMPUTATION_E_SM_CONFIGS: ShareComputationConfigs<L_THRESHOLD> =crates/zk-helpers/src/bin/zk_cli.rs (1)
241-242: Potential panic:unwrap()onsearch_defaults()for non-threshold presets.
preset.search_defaults()returnsNonefor DKG presets. Whilefrom_security_config_namecurrently only returns threshold presets, this is fragile. Consider adding a guard or using a fallback:🛡️ Defensive handling
+ let search_defaults = preset + .search_defaults() + .ok_or_else(|| anyhow!("preset {} has no search defaults", preset.name()))?; let sample = Sample::generate( &threshold_params, &dkg_params, Some(dkg_input_type.clone()), CiphernodesCommitteeSize::Small, - preset.search_defaults().unwrap().z, - preset.search_defaults().unwrap().lambda, + search_defaults.z, + search_defaults.lambda, )?;crates/zk-helpers/src/circuits/sample.rs (1)
164-183: Avoid panicking on presets without search defaults.
search_defaults().unwrap()will panic if a preset lacks defaults; returning a structured error makes this public helper safer.♻️ Suggested change
- let num_ciphertexts = threshold_preset.search_defaults().unwrap().z; - let lambda = threshold_preset.search_defaults().unwrap().lambda; + let defaults = threshold_preset + .search_defaults() + .ok_or_else(|| CircuitsErrors::Sample("Missing search defaults for preset".to_string()))?; + let num_ciphertexts = defaults.z; + let lambda = defaults.lambda;
There was a problem hiding this comment.
Actionable comments posted: 5
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/mod.rs (1)
11-12:⚠️ Potential issue | 🟡 MinorStale documentation references
pk_bfv.The module doc still mentions "pk_bfv submodule" but the module has been restructured to
dkg::pk.📝 Suggested fix
-//! public-key BFV commitment circuit. +//! The `dkg` submodule implements the DKG public-key BFV commitment and share-computation circuits.
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/README.md`:
- Line 30: Rewrite the README line describing `--witness` to remove the
ambiguous "(optional if `--toml`)" and clearly state when the flag is required:
explain that for the `share-computation` command `--witness <type>` must be
provided when writing a Prover.toml (i.e., when `--toml` is enabled) and that it
is not required for configs-only generation; reference the `--witness` flag, the
`share-computation` command, `Prover.toml`, and the `--toml` option in the
sentence so readers know exactly when to include the flag.
- Around line 20-22: The README claim "Configs only (no Prover.toml)" is
incorrect because zk_cli.rs uses args.toml to set toml = Some(&artifacts.toml)
which writes Prover.toml; either remove the --toml flag from the two example
commands so they truly produce configs-only, or change the header/comment to
state that these examples will also generate Prover.toml (mentioning args.toml /
artifacts.toml behavior in zk_cli.rs to clarify why).
In `@crates/zk-helpers/src/circuits/dkg/share_computation/circuit.rs`:
- Line 20: The SUPPORTED_PARAMETER constant currently advertises
ParameterType::DKG but the share-computation path uses threshold-derived
bounds/moduli; change the constant SUPPORTED_PARAMETER from ParameterType::DKG
to ParameterType::THRESHOLD (update the declaration named SUPPORTED_PARAMETER)
and ensure any CLI/registry validation that checks circuit parameter type is
updated to accept THRESHOLD for this circuit; verify consistency with the
threshold logic in share_computation/computation.rs (where bounds/moduli are
derived) so the circuit advertises the parameter set it actually uses.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs`:
- Around line 102-126: The impl of Computation for Configs is populating n, l,
and moduli from the DKG params (second element) but Bits::compute and
Bounds::compute use the threshold params (first element); change the code in the
compute method to use the threshold parameter set returned by
build_pair_for_preset (the first tuple element, e.g. rename to threshold_params)
when deriving Configs { n, l, moduli } so that n = threshold_params.degree(),
moduli = threshold_params.moduli().to_vec(), and l = moduli.len(), keeping the
same error mapping from build_pair_for_preset and leaving
Bits::compute/Bounds::compute calls unchanged.
In `@crates/zk-helpers/src/circuits/sample.rs`:
- Around line 172-173: Replace the two unwrap() calls on
threshold_preset.search_defaults() with a single fallible binding and propagate
the error using ?; for example, get let defaults =
threshold_preset.search_defaults()?; then set let num_ciphertexts = defaults.z;
let lambda = defaults.lambda; (this avoids panics by using the function's Result
return to propagate the error).
🧹 Nitpick comments (6)
crates/zk-helpers/src/ciphernodes_committee.rs (1)
39-41: Pre-existing:MediumandLargevariants will panic at runtime.This is outside the scope of your current changes, but note that calling
.values()onMediumorLargewill hitunreachable!(). The TODO on line 41 acknowledges this. Consider returning aResultor implementing actual values before these variants are used elsewhere.circuits/lib/src/configs/insecure/dkg.nr (1)
51-52: Confirm reduced share/secret bit sizes remain sufficient.These drops (36/1) are tight; please re-derive the required bit lengths from the share/secret bounds to ensure the range checks still hold. A brief derivation note would help future maintenance.
circuits/lib/src/configs/secure/dkg.nr (2)
63-64: Re‑verify secure share/secret bit widths after rename.With 53/1 bits, please confirm these still cover the secure parameter ranges and circuit expectations. If this is formula‑derived, a short note would reduce future ambiguity.
77-79: Check 192‑bit e_sm secret bound and comment intent.192 is a large bound; please confirm it matches the secure e_sm noise limits. Also ensure the “verify_shares” label reflects the current circuit naming to avoid confusion.
crates/zk-helpers/src/circuits/dkg/share_computation/mod.rs (1)
9-14: Consider re-exporting codegen helpers for API consistency.The
pk/mod.rsmodule re-exportsgenerate_configs,generate_toml, andTomlJsonfrom its codegen module. Theshare_computation/codegen.rsexposes the same public functions and struct, butshare_computation/mod.rsdoes not currently re-export them. Addpub use codegen::{generate_configs, generate_toml, TomlJson};to maintain consistent API patterns across the DKG circuit modules.crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
203-293: Test coverage is solid but only usesCiphernodesCommitteeSize::Small.The test validates TOML structure, artifact writing, and config content thoroughly. However, it only tests with
CiphernodesCommitteeSize::Small, which masks the hardcoded committee size bug ingenerate_configs.Consider adding a test with a different committee size once the hardcoded value is fixed.
There was a problem hiding this comment.
Actionable comments posted: 3
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/pk/computation.rs (1)
91-108:⚠️ Potential issue | 🟠 MajorUse threshold params (not DKG params) for PK bounds/witness.
build_pair_for_presetreturns(params_threshold, params_dkg)with threshold parameters as the first element. The PK circuit is intended to align with threshold parameters (L_THRESHOLD / QIS_THRESHOLD), but the current code discards the first element and uses the second (DKG params) for moduli, degree, and bounds calculations. This causes desync between the configuration and the actual parameter set.🐛 Suggested fix
- let (_, dkg_params) = + let (threshold_params, _) = build_pair_for_preset(preset).map_err(|e| CircuitsErrors::Sample(e.to_string()))?; - let moduli = dkg_params.moduli().to_vec(); + let moduli = threshold_params.moduli().to_vec(); let bounds = Bounds::compute(preset, &())?; let bits = Bits::compute(preset, &bounds)?; Ok(Configs { - n: dkg_params.degree(), + n: threshold_params.degree(), l: moduli.len(), moduli, bits, bounds, })- let (_, dkg_params) = + let (threshold_params, _) = build_pair_for_preset(preset).map_err(|e| CircuitsErrors::Sample(e.to_string()))?; - for &qi in dkg_params.moduli() { + for &qi in threshold_params.moduli() {- let (_, dkg_params) = + let (threshold_params, _) = build_pair_for_preset(preset).map_err(|e| CircuitsErrors::Sample(e.to_string()))?; - let moduli = dkg_params.moduli(); + let moduli = threshold_params.moduli();Affects lines 91–108, 132–147, 160–176.
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 7-8: The module-level doc comment at the top of codegen.rs
incorrectly describes the file as "public-key BFV circuit"; update that module
doc to accurately describe this file as implementing share-computation code
generation (e.g., "Code generation for the share-computation BFV circuit:
Prover.toml and configs.nr") so the documentation matches the implemented
functionality in this module.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs`:
- Around line 160-168: The compute function currently calls
preset.search_defaults().unwrap(), which can panic for presets missing defaults;
update compute to check preset.search_defaults() and return a
CircuitsErrors::Sample (or another appropriate error) when None is returned
instead of unwrapping. Locate compute and the calls to preset.search_defaults()
(and build_pair_for_preset) and replace the unwraps with pattern matching or
.ok_or_else(...) to propagate a clear error (e.g.,
CircuitsErrors::Sample("missing search defaults".to_string())) so missing
defaults are handled safely.
In `@crates/zk-helpers/src/circuits/mod.rs`:
- Around line 28-31: The module-level documentation still references the removed
`pk_bfv`; update the top comment to reference `dkg::pk` instead. Edit the module
doc comment above the `pub mod dkg;`/re-exports so any mention of `pk_bfv` is
replaced with `dkg::pk` and ensure the text reflects the current public API
(e.g., `generate_configs`, `generate_toml`, `TomlJson`, `Bits`, `Bounds`,
`PkComputationOutput`, `Witness`, `PkCircuit`).
🧹 Nitpick comments (2)
crates/zk-helpers/src/ciphernodes_committee.rs (1)
30-52:MediumandLargevariants will panic at runtime.The
unreachable!()macro on line 39 will cause a runtime panic if any code attempts to useCiphernodesCommitteeSize::MediumorCiphernodesCommitteeSize::Large. While the TODO at line 41 indicates this is known, consider either:
- Implementing the remaining variants now (even with placeholder values).
- Removing the unused enum variants until they're needed to prevent accidental misuse.
Would you like me to open an issue to track implementing the Medium and Large committee sizes?
circuits/lib/src/configs/secure/dkg.nr (1)
79-81: Comment label appears inconsistent with the config it describes.Line 79 says
// verify_shares - configsbut the config declaration on lines 80-81 isSHARE_COMPUTATION_E_SM_CONFIGS. Consider updating the comment to match:📝 Suggested comment fix
-// verify_shares - configs +// share_computation_e_sm - configs pub global SHARE_COMPUTATION_E_SM_CONFIGS: ShareComputationConfigs<L_THRESHOLD> = ShareComputationConfigs::new(QIS_THRESHOLD);
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
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/pk/computation.rs (1)
91-108:⚠️ Potential issue | 🟠 MajorUse threshold params (not DKG params) for pk computations.
Configs::compute,Bounds::compute, andWitness::computeare deriving moduli/bounds from the DKG parameter set (second element ofbuild_pair_for_preset). The pk circuit should use the threshold parameter set to stay consistent withL_THRESHOLD/QIS_THRESHOLDexpectations; otherwise bounds and witness values can diverge from the Noir constants.🔧 Suggested fix
- let (_, dkg_params) = + let (threshold_params, _) = build_pair_for_preset(preset).map_err(|e| CircuitsErrors::Sample(e.to_string()))?; - let moduli = dkg_params.moduli().to_vec(); + let moduli = threshold_params.moduli().to_vec(); let bounds = Bounds::compute(preset, &())?; let bits = Bits::compute(preset, &bounds)?; Ok(Configs { - n: dkg_params.degree(), + n: threshold_params.degree(), l: moduli.len(), moduli, bits, bounds, })- let (_, dkg_params) = + let (threshold_params, _) = build_pair_for_preset(preset).map_err(|e| CircuitsErrors::Sample(e.to_string()))?; let mut pk_bound_max = BigUint::from(0u32); - for &qi in dkg_params.moduli() { + for &qi in threshold_params.moduli() { let qi_bound: BigUint = (&BigUint::from(qi) - 1u32) / 2u32; if qi_bound > pk_bound_max { pk_bound_max = qi_bound; } }- let (_, dkg_params) = + let (threshold_params, _) = build_pair_for_preset(preset).map_err(|e| CircuitsErrors::Sample(e.to_string()))?; - let moduli = dkg_params.moduli(); + let moduli = threshold_params.moduli();Based on learnings: In the circuits/lib/src/configs/insecure/dkg.nr file, DKG and Threshold circuits intentionally use different parameter sets: pk and share_computation circuits use L_THRESHOLD and QIS_THRESHOLD because they generate components for threshold operations, while share_encryption circuits use the DKG-specific L and QIS parameters for encrypting shares. This separation is by design.
Also applies to: 132-151, 160-176
🧹 Nitpick comments (3)
crates/zk-helpers/src/circuits/dkg/pk/sample.rs (1)
27-43: Consider documenting why_threshold_paramsis accepted but unused.The
_threshold_paramsparameter maintains API consistency withShareComputationSample::generate, but a brief doc comment would clarify this intent for future maintainers.📝 Optional: Add clarifying doc comment
impl PkSample { - /// Generates sample data for the pk circuit. + /// Generates sample data for the pk circuit. + /// + /// Note: `_threshold_params` is unused but accepted for API consistency with + /// `ShareComputationSample::generate`. pub fn generate( _threshold_params: &Arc<BfvParameters>,crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
163-210: Consider extracting the configs template to a separate constant.The large format string with many placeholders is difficult to read and maintain. Extracting it to a named constant or using a templating approach could improve readability.
crates/zk-helpers/src/circuits/dkg/share_computation/sample.rs (1)
61-82: Consider deferring TRBFV creation to the SmudgingNoise branch.The
TRBFVinstance is created unconditionally at line 61-62 but only used in theSmudgingNoisebranch (line 119). Moving this into the match arm would avoid unnecessary computation for theSecretKeypath.♻️ Optional: Defer TRBFV creation
- let trbfv = TRBFV::new(committee.n, committee.threshold, threshold_params.clone()) - .map_err(|e| CircuitsErrors::Sample(format!("Failed to create TRBFV: {:?}", e)))?; let mut share_manager = ShareManager::new(committee.n, committee.threshold, threshold_params.clone()); // Parity check matrix per modulus... // ... let (secret, secret_sss) = match dkg_input_type { DkgInputType::SecretKey => { // ... unchanged } DkgInputType::SmudgingNoise => { + let trbfv = TRBFV::new(committee.n, committee.threshold, threshold_params.clone()) + .map_err(|e| CircuitsErrors::Sample(format!("Failed to create TRBFV: {:?}", e)))?; let esi_coeffs = trbfv .generate_smudging_error(num_ciphertexts as usize, lambda as usize, &mut rng)
2 participants
this PR moves C2
share_computation(for bothskande_sm) - prev.verify_shares_trbfv- inside the new circuits helpers.Summary by CodeRabbit
New Features
Documentation
Chores