refactor: add share_decryption circuit [skip-line-limit]#1280
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:
📝 WalkthroughWalkthroughThis PR renames ciphertext and quotient polynomial parameters across threshold cryptography components (c_0→ct0, c_1→ct1, r_1→r1, r_2→r2), reduces bit-width configuration values by 1, introduces a new threshold share decryption circuit implementation with computation and codegen, updates DKG circuits to use the new committee structure, and refactors circuit aliases in the CLI. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant SampleGen as Sample Generation
participant Computation
participant Codegen
CLI->>SampleGen: generate_sample(preset, committee)
activate SampleGen
SampleGen->>SampleGen: Create threshold params
SampleGen->>SampleGen: Generate keys & encrypt
SampleGen->>SampleGen: Simulate secret/error shares
SampleGen->>SampleGen: Compute decryption share
SampleGen-->>CLI: ShareDecryptionCircuitInput
deactivate SampleGen
CLI->>Computation: compute(preset, input)
activate Computation
Computation->>Computation: Compute bounds (r1, r2)
Computation->>Computation: Compute bit-widths
Computation->>Computation: Build witness polynomials
Computation->>Computation: Per-modulus lifting & centering
Computation->>Computation: Aggregate results
Computation-->>CLI: ShareDecryptionComputationOutput
deactivate Computation
CLI->>Codegen: codegen(preset, input)
activate Codegen
Codegen->>Computation: Reuse computation phase
Codegen->>Codegen: Generate TOML artifact
Codegen->>Codegen: Generate configs.nr
Codegen-->>CLI: Artifacts
deactivate Codegen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/codegen.rs`:
- Line 7: The module-level doc comment in codegen.rs is incorrect (copy-pasted):
it reads "Code generation for the public-key BFV circuit"; update that top doc
comment to "Code generation for the share decryption circuit" so the module
description matches the actual purpose of the file (file:
crates/zk-helpers/src/circuits/threshold/share_decryption/codegen.rs, symbol:
module/codegen.rs doc comment).
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/computation.rs`:
- Around line 224-227: The inline comments for the cyclotomic polynomial
creation are reversed: cyclo[0] is the constant (x^0) term and cyclo[n as usize]
is the x^N term. Update the comments on the lines that set cyclo[0] and cyclo[n
as usize] in the cyclo vector (in the block creating cyclotomic polynomial using
BigInt) so they correctly state "constant (x^0) term" for cyclo[0] and "x^N
term" for cyclo[n as usize].
- Around line 7-10: The crate-level and struct-level doc comments incorrectly
reference the "public key generation circuit" and `PkGenerationCircuit`; update
all such occurrences to say "share decryption circuit" and use the correct type
name `ShareDecryptionCircuit`, including the top module docstring and the docs
for `Configs`, `Bounds`, `Bits`, and `Witness` so their descriptions and links
point to `ShareDecryptionCircuit` instead of `PkGenerationCircuit`.
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/mod.rs`:
- Around line 7-11: The module doc comment is copied from the
user-data-encryption circuit and incorrectly references "User data encryption
circuit", UserDataEncryptionCircuit, and UserDataEncryptionCircuitInput; update
the top-level module documentation to correctly describe this module as the
share decryption circuit and replace those references with
ShareDecryptionCircuit and ShareDecryptionCircuitInput (and any other mismatched
names) so the docs accurately describe the circuit provided in this file.
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/sample.rs`:
- Around line 7-10: The module doc is incorrect/copy-pasted from
user-data-encryption; update the top-level doc comment to describe "share
decryption sample generation" and reference the actual generator method
ShareDecryptionCircuitInput::generate_sample (instead of the nonexistent Sample
type) and mention that it produces example inputs (e.g., random key share(s) and
ciphertexts) used for codegen and tests; ensure the wording and examples align
with the types in this module (ShareDecryptionCircuitInput) and remove any
mention of BFV key pair or "user data encryption circuit".
🧹 Nitpick comments (1)
circuits/lib/src/core/threshold/share_decryption.nr (1)
114-131: Stale doc references:c_0/c_1/r_1/r_2inpayload()docstring.Lines 123–126 still reference the old field names (
c_0,c_1,r_1,r_2) when describing the serialization order. These should be updated toct0,ct1,r1,r2to match the renamed struct fields.The mathematical formula comments (e.g., line 34, line 243) are arguably fine since they describe the cryptographic equation rather than code identifiers.
cedoor
force-pushed
the
refactor/share-decryption
branch
from
d39cbb5 to
1ea78c2
Compare
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/core/threshold/share_decryption.nr (1)
34-34:⚠️ Potential issue | 🟡 MinorDoc comments still reference the old
c_0/c_1andr_1/r_2names.Several documentation blocks were not updated to match the renamed fields:
- Line 34: formula
d = c_0 + c_1 * s + e + r_2 * (X^N + 1) + r_1 * q_i- Lines 123–126: payload serialization order references
c_0,c_1,r_1,r_2- Line 187:
r_1andr_2- Line 223:
c_0/c_1,r_1/r_2- Lines 243–251:
c_0i,c_1i,r_1_i,r_2_iThese should be updated to
ct0/ct1andr1/r2to stay consistent with the code.Also applies to: 123-126, 187-187, 223-223, 240-251
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/computation.rs`:
- Around line 176-185: The r1_bounds computation in the r1_bounds.push call does
not match its comment: the numerator currently subtracts qi_bound (… -
&qi_bound.clone()) but the comment (r_1j upper bound) expects an addition of Be;
verify the intended formula and either change the expression in the
r1_bounds.push to use addition (e.g., (&qi_bound * (&qi_bound * &n +
BigInt::from(3)) + &Be_or_qi_bound) / &qi_bigint) or update the comment to
reflect the subtraction version; ensure you reference the actual variables used
here (r1_bounds, qi_bound, qi_bigint, n, Be/Be_var) so the code and comment are
consistent.
🧹 Nitpick comments (1)
crates/zk-helpers/src/circuits/threshold/share_decryption/computation.rs (1)
188-197:to_u128().unwrap()will panic if any bound exceeds 128 bits.For the current parameter sets this is likely safe, but it's a latent panic. Consider using
try_intowith a descriptive error via theCircuitsErrorspath, or at minimum document the assumption.
cedoor
changed the title
cedoor
force-pushed
the
refactor/share-decryption
branch
2 times, most recently
from
1d6bc02 to
f62b177
Compare
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 (4)
crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
134-160:⚠️ Potential issue | 🟡 MinorPre-existing bug:
test_generate_smudging_noise_sampleusesDkgInputType::SecretKeyinstead ofDkgInputType::SmudgingNoise.Line 141 passes
DkgInputType::SecretKey, so this test doesn't actually exercise the smudging noise path despite its name. This predates this PR but is worth fixing while you're touching this file.Proposed fix
let sample = ShareEncryptionCircuitInput::generate_sample( BfvPreset::InsecureThreshold512, committee.clone(), - DkgInputType::SecretKey, + DkgInputType::SmudgingNoise, sd.z, sd.lambda, );circuits/lib/src/core/threshold/share_decryption.nr (1)
114-155:⚠️ Potential issue | 🟡 MinorDoc comments still reference old field names
c_0/c_1andr_1/r_2.Lines 123–126, 187, and 223 still use the old naming (
c_0,c_1,r_1,r_2) when describing the serialization order and range-check targets. These should be updated toct0/ct1andr1/r2to match the renamed fields. The mathematical formula references (e.g., line 34, 243) are fine as-is since they follow cryptographic notation conventions.crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs (1)
174-174:⚠️ Potential issue | 🟠 Major
unwrap()ontry_decryptin non-test code will panic on decryption failure.
Witness::computereturnsResult<Self, CircuitsErrors>, so the decryption error should be propagated instead of panicking.Proposed fix
- let decrypted_pt = input.secret_key.try_decrypt(&party_cts[mod_idx]).unwrap(); + let decrypted_pt = input.secret_key.try_decrypt(&party_cts[mod_idx]).map_err(|e| { + CircuitsErrors::Other(format!( + "Failed to decrypt ciphertext at mod_idx {}: {:?}", + mod_idx, e + )) + })?;crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (1)
78-111:⚠️ Potential issue | 🟡 MinorDuplicate
.map_err()chains produce nested error messages.Both the
generate_smudging_errorcall (lines 80–91) and thegenerate_secret_shares_from_polycall (lines 100–111) chain two.map_err()transformations. The first.map_err()already converts the original library error intoCircuitsErrors::Sample. The second.map_err()then wraps thatCircuitsErrors::Sampleinto yet anotherCircuitsErrors::Sample, producing a nested message like"Failed to generate smudging error: Sample(\"Failed to generate smudging error: ...\")".Remove the duplicate
.map_err()on each call, keeping only one.Proposed fix
let esi_coeffs = trbfv .generate_smudging_error(sd.z as usize, sd.lambda as usize, &mut rng) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate smudging error: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate smudging error: {:?}", - e - )) })?; let esi_poly = share_manager.bigints_to_poly(&esi_coeffs).map_err(|e| { CircuitsErrors::Sample(format!( "Failed to convert error to poly: {:?}", e )) })?; let esi_sss_u64 = share_manager .generate_secret_shares_from_poly(esi_poly.clone(), &mut rng.clone()) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate error shares: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate error shares: {:?}", - e - )) })?;
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Line 258: The local variable committee is a non-Copy CiphernodesCommittee
value created from CiphernodesCommitteeSize::Small.values() and is currently
moved into whichever single match arm executes; to make this robust for future
changes, avoid relying on the single-arm move by either cloning committee each
time you need to use it (call .clone() where committee is moved into a match
arm) or change the match to borrow committee (e.g., match on a reference) so you
reuse the same instance without moving; locate the committee binding and any
match that consumes it and update uses to committee.clone() or to borrow
(&committee) as appropriate to ensure multiple arms can safely access it.
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/computation.rs`:
- Around line 310-335: The code computes pk_bit via
compute_pk_bit(&threshold_params) but then independently recomputes bit width
elsewhere; instead make the bits computed by Bits::compute authoritative: change
Witness::compute to accept the Bits instance (or add an overload) and use
bits.sk_bit when calling compute_aggregated_shares_commitment for
expected_sk_commitment and expected_e_sm_commitment (replace compute_pk_bit
usage), and add a unit test that constructs Bits::compute(...), calls
compute_pk_bit(...) and asserts bits.sk_bit == compute_pk_bit(...) to lock in
the equivalence if you still want the separate function.
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/sample.rs`:
- Line 43: Replace the hardcoded num_ciphertexts = 10 with the preset-derived z
value so smudging uses the configured parameter: locate the variable in
sample.rs (where num_ciphertexts is set) and use the preset/search_defaults
value (e.g., sd.z or preset.search_defaults().z) instead, and update the call
site that passes num_ciphertexts so it now receives that z value.
🧹 Nitpick comments (7)
crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs (1)
249-249: Weak assertion after prefix rename — substring match gives a false sense of coverage.
"SHARE_ENCRYPTION_CONFIGS"is a substring of"DKG_SHARE_ENCRYPTION_CONFIGS", so this passes by coincidence. If the prefix were ever changed to something that doesn't contain the old substring, this test would silently break. Consider asserting the full prefixed name.Proposed fix
- assert!(artifacts.configs.contains("SHARE_ENCRYPTION_CONFIGS")); + assert!(artifacts.configs.contains("DKG_SHARE_ENCRYPTION_CONFIGS"));crates/zk-helpers/src/circuits/threshold/share_decryption/sample.rs (2)
92-92: Redundant.take(num_parties).
party_secret_keysalready has exactlynum_partieselements (populated by the loop at lines 59–66). The.take()is a no-op.Proposed fix
- for party_sk in party_secret_keys.iter().take(num_parties) { + for party_sk in &party_secret_keys {
307-318:test_generate_template_repeatabledoesn't verify repeatability — only structural equality.The test name suggests deterministic output, but it only checks that the two independently-generated samples have the same number of limbs/components. Since both use
OsRng/thread_rng, they produce different values each time. Consider renaming totest_generate_template_structural_consistencyto avoid confusion.crates/zk-helpers/src/circuits/threshold/share_decryption/codegen.rs (2)
54-106: 22 positionalformat!args are fragile and hard to audit.The template and argument list are correct today, but any insertion or reorder risks silent misalignment. Consider using named format arguments for clarity.
Example (partial)
format!( r#"... pub global N: u32 = {n}; pub global L: u32 = {l}; pub global QIS: [Field; L] = [{qis}]; pub global {prefix}_BIT_CT: u32 = {bit_ct}; ..."#, n = configs.n, l = configs.l, qis = qis_str, prefix = prefix, bit_ct = configs.bits.ct_bit, // ... )
39-45:generate_tomltakesWitnessby value, while the DKG equivalent takes&Witness.Minor inconsistency with the DKG codegen at
crates/zk-helpers/src/circuits/dkg/share_decryption/codegen.rs:42which takes&Witness. SinceWitnessis only used to callto_json(), a reference would suffice and align the APIs.crates/zk-helpers/src/circuits/threshold/share_decryption/computation.rs (2)
130-151: All non-rbit widths are set tor2_bit— intentional?
ct_bit,sk_bit,e_sm_bit, andd_bitare all assignedr2_bit. This uses r2's bound as a conservative ceiling. If these polynomials can have tighter bounds in practice (e.g.,ct_bitviacompute_pk_bit), circuit constraint counts could be reduced. If this is intentional as a simplification, a brief comment would help future readers.
101-123: Redundantbuild_pair_for_preset+Bounds::compute+Bits::computecalls whenConfigsis used alongsideCircuitComputation::compute.
Configs::computeindependently callsbuild_pair_for_preset,Bounds::compute, andBits::compute. IfConfigsis ever used in the same flow asShareDecryptionCircuit::compute(which also callsBounds+Bits), these expensive operations run twice. Not a correctness issue, but worth noting for when the full pipeline is wired up — you may wantConfigsto accept pre-computedBounds/Bitsas input instead.
- cyclo[0] is constant (x^0) term, not x^N - cyclo[n] is x^N term, not x^0 Co-authored-by: Cursor <cursoragent@cursor.com>
- Update comment to reflect subtraction formula (qi_bound * (qi_bound * n + 3) - qi_bound) / qi_bigint - Document qi_bound, qi_bigint, n variables - Remove outdated BS/Be reference that expected addition Co-authored-by: Cursor <cursoragent@cursor.com>
- Replace hardcoded num_ciphertexts=10 with preset.search_defaults().z - Add search_defaults() resolution with error handling for presets without defaults - Smudging error generation now uses configured parameter per preset Co-authored-by: Cursor <cursoragent@cursor.com>
cedoor
force-pushed
the
refactor/share-decryption
branch
from
54b8655 to
f4cbb7a
Compare
2 participants
Summary by CodeRabbit
New Features
Improvements