feat: add share-encryption circuit gen [skip-line-limit]#1267
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new share-encryption DKG circuit (circuit, computation, codegen, sample), integrates it into the CLI, unifies/renames SHARE_ENCRYPTION config symbols and adds many bound constants, updates share-encryption bit-widths, and adds compute_msg_bit utility. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Handler
participant Sample as Sample Generator
participant Compute as Circuit Computation
participant Codegen as Code Generator
participant Artifacts as Output Artifacts
CLI->>Sample: generate(preset, committee, dkg_input_type)
Sample->>Sample: create DKG keys, plaintext, ciphertext, RNS
Sample-->>CLI: ShareEncryptionSample
CLI->>Compute: compute(preset, ShareEncryptionSample)
Compute->>Compute: derive Bounds & Bits
Compute->>Compute: build Witness (CRT limbs, polynomials, commitments)
Compute-->>CLI: ShareEncryptionOutput
CLI->>Codegen: codegen(preset, ShareEncryptionCircuitInput)
Codegen->>Codegen: generate_toml(witness)
Codegen->>Codegen: generate_configs(preset, configs)
Codegen-->>Artifacts: Prover.toml + configs.nr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/circuit.rs`:
- Around line 22-28: The circuit declares the wrong parameter type: in the impl
for ShareEncryptionCircuit the const SUPPORTED_PARAMETER is set to
ParameterType::THRESHOLD but the Noir configs for this circuit use DKG-specific
parameters; update the constant SUPPORTED_PARAMETER in the
ShareEncryptionCircuit impl to ParameterType::DKG so the circuit metadata
matches the actual L/QIS parameter set used by the DKG configs.
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs`:
- Around line 249-267: The test_toml_generation_and_structure creates a sample
with BfvPreset::InsecureThreshold512 via
prepare_share_encryption_sample_for_test but then calls
ShareEncryptionCircuit.codegen using DEFAULT_BFV_PRESET causing a preset
mismatch; fix by calling codegen with the same preset used to create the sample
(pass BfvPreset::InsecureThreshold512 or obtain the preset from the sample) so
ShareEncryptionCircuit::codegen receives matching BFV parameters instead of
DEFAULT_BFV_PRESET.
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs`:
- Around line 940-956: The test test_bound_and_bits_computation_consistency
mixes presets: it creates the sample with BfvPreset::InsecureThreshold512 but
calls Bounds::compute and Bits::compute with DEFAULT_BFV_PRESET; update the test
so the same preset is used for sample creation and subsequent
computations/assertions (e.g., introduce a local let preset =
BfvPreset::InsecureThreshold512 and pass preset to
prepare_share_encryption_sample_for_test, Bounds::compute and Bits::compute, and
use that preset for any related expectations) to ensure consistent preset usage
across prepare_share_encryption_sample_for_test, Bounds::compute, Bits::compute
and the assertions.
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs`:
- Around line 135-161: The test uses BfvPreset::InsecureThreshold512 to build
the sample but asserts lengths against DEFAULT_BFV_PRESET.metadata().degree,
which can mismatch; update the assertions to use the same preset used to build
the sample by capturing the preset (or its metadata()) used in
prepare_share_encryption_sample_for_test and replace
DEFAULT_BFV_PRESET.metadata().degree with that preset.metadata().degree (or
compute expected_degree from BfvPreset::InsecureThreshold512 directly) so
assert_eq! checks reference the exact same preset; locate uses in
test_generate_secret_key_sample and adjust all four degree-based assertions
accordingly.
🧹 Nitpick comments (7)
crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
36-109: Minor: Variable naming inconsistency.The variable
_cton line 101 uses an underscore prefix, which conventionally indicates an unused variable, but it's actually used in the returned struct. Consider renaming toctfor clarity.✏️ Suggested fix
- 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(); ShareEncryptionSample { plaintext: pt, - ciphertext: _ct, + ciphertext: ct, public_key: dkg_public_key,crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs (2)
64-218: Consider extracting format arguments into a struct for maintainability.The
format!macro has 50+ positional arguments, making it difficult to verify correctness and maintain. A typo or off-by-one in the argument list would cause subtle bugs.Consider grouping related values into a helper struct or using named arguments:
💡 Alternative approach sketch
// Example: use a struct to organize template data struct ConfigsTemplateData<'a> { n: usize, l: usize, qis_str: &'a str, prefix: &'a str, bits: &'a Bits, // ... etc } impl ConfigsTemplateData<'_> { fn render(&self) -> String { // Use a templating library like `askama` or `minijinja` // Or at minimum, build the string in sections } }This is optional given the PR scope, but worth noting for future maintainability.
126-141: Minor formatting inconsistency.Line 140 has a closing parenthesis directly after a comma without a newline, unlike the
_CONFIGS_SKblock above (line 117). This creates visual inconsistency in the generated Noir code.✏️ Suggested fix
{}_R1_UP_BOUNDS, {}_R2_BOUNDS, {}_P1_BOUNDS, {}_P2_BOUNDS, - {}_MSG_BOUND,); + {}_MSG_BOUND, +); "#,crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs (4)
326-356: Potential panic on large bound values.The
to_u128().unwrap()calls (lines 329, 334, 337, 341, 345, 349, 353) will panic if any bound value exceedsu128::MAX. While this is unlikely with current BFV parameters, consider adding explicit error handling or documenting the assumption.✏️ Suggested defensive approach
Ok(Bounds { pk_bounds: pk_bounds .iter() - .map(|b| BigUint::from(b.to_u128().unwrap())) + .map(|b| { + BigUint::try_from(b.clone()) + .expect("pk_bound exceeds BigUint capacity") + }) .collect(),Or return an error instead of panicking:
.map(|b| b.to_biguint().ok_or_else(|| CircuitsErrors::Other("bound overflow".into()))) .collect::<Result<Vec<_>, _>>()?
518-543: Inconsistent error handling inside unsafe blocks.The code uses
.ok_or_else(...).unwrap()which is verbose and still panics. Either propagate the error properly or useexpect()directly for clearer intent.✏️ Suggested fix
let e0i: Vec<BigInt> = unsafe { qi.center_vec_vt( e0_coeffs .as_slice() - .ok_or_else(|| "Cannot center coefficients.".to_string()) - .unwrap(), + .expect("e0_coeffs must be contiguous"), ) .iter() .rev() .map(|&x| BigInt::from(x)) .collect() };
665-667: O(n²) leading zero removal.Using
remove(0)in a loop is O(n) per removal, making the overall complexity O(n²). For large polynomials, consider usingdrainor finding the first non-zero index and slicing.✏️ Suggested fix
- while !ct0i_calculated.is_empty() && ct0i_calculated[0].is_zero() { - ct0i_calculated.remove(0); - } + let first_nonzero = ct0i_calculated.iter().position(|x| !x.is_zero()).unwrap_or(ct0i_calculated.len()); + ct0i_calculated.drain(..first_nonzero);The same pattern appears at lines 761-763.
784-786: Unnecessary clone before sort.
resultsis already an ownedVec, so.clone()before sorting is unnecessary.✏️ Suggested fix
- let mut results = results.clone(); + let mut results = results; results.sort_by_key(|(i, ..)| *i);
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
circuits/lib/src/configs/insecure/dkg.nr (1)
103-118:⚠️ Potential issue | 🟡 MinorRemove unused
SHARE_ENCRYPTION_CONFIGS_SKconstant.The
SHARE_ENCRYPTION_CONFIGS_SKdeclaration at lines 103–118 is dead code—it is never referenced elsewhere in the codebase. The circuit files (sk_share_encryptionande_sm_share_encryption) import the unifiedSHARE_ENCRYPTION_CONFIGSinstead, making the_SKvariant obsolete. Remove it to complete the unification.
2 participants
Summary by CodeRabbit
New Features
Chores
Tests