feat: add decrypted-shares-aggregation [skip-line-limit]#1273
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a new threshold decrypted shares aggregation circuit with complete computation, codegen, and sampling infrastructure. It concurrently refactors CRT and ring utilities into a unified math module and updates existing circuits to consume these centralized helpers. Configuration parameters are adjusted for threshold operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Sample as Sample Generator
participant Computation as Computation Pipeline
participant Codegen as Code Generator
participant Output as Artifacts
User->>CLI: Request DecryptedSharesAggregationCircuit codegen
CLI->>Sample: generate_sample(preset, committee)
Sample->>Sample: Setup TRBFV context
Sample->>Sample: Generate key/decryption shares
Sample->>Sample: Encrypt synthetic message
Sample->>Sample: Recover via threshold decryption
Sample-->>CLI: DecryptedSharesAggregationCircuitInput
CLI->>Computation: Bounds::compute(preset)
Computation-->>CLI: Bounds
CLI->>Computation: Bits::compute(bounds)
Computation-->>CLI: Bits
CLI->>Computation: Configs::compute(preset, input)
Computation->>Computation: Compute q, q_mod_t, moduli
Computation-->>CLI: Configs
CLI->>Computation: Witness::compute(input, configs)
Computation->>Computation: Lagrange-at-zero recovery
Computation->>Computation: CRT reconstruction
Computation->>Computation: standard_form normalization
Computation-->>CLI: Witness
CLI->>Codegen: codegen(preset, input)
Codegen->>Codegen: generate_toml(witness)
Codegen->>Codegen: generate_configs(preset, configs)
Codegen-->>Output: Artifacts {toml, configs}
Output-->>User: Generated code artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 10
🤖 Fix all issues with AI agents
In `@circuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.toml`:
- Line 2: The recursive_aggregation workspace still references the old package
name "decrypted_shares_aggregation"; update the member entry in
circuits/bin/recursive_aggregation/wrapper/threshold/Nargo.toml to match the
renamed package ("decrypted_shares_aggregation_mod") or alternatively rename the
threshold package back to the original name so both workspaces use the same
member string; ensure the member value exactly matches the name =
"decrypted_shares_aggregation_mod" declared in
circuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.toml.
In `@circuits/lib/src/configs/insecure/threshold.nr`:
- Line 17: The constant Q_INVERSE_MOD_T in threshold.nr is wrong (57) for
Q_MOD_T = 3 under PLAINTEXT_MODULUS = 100; change the value of Q_INVERSE_MOD_T
to 67 so that Q_MOD_T * Q_INVERSE_MOD_T ≡ 1 (mod PLAINTEXT_MODULUS). Update the
declaration pub global Q_INVERSE_MOD_T: Field = 67; and keep Q_MOD_T and
PLAINTEXT_MODULUS as references to ensure the modular inverse is correct for the
core decryption code in decrypted_shares_aggregation (where Q_INVERSE_MOD_T is
used).
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Around line 215-218: The inline comment above the has_witness_type condition
is stale and only mentions share-computation; update or replace it to reflect
that the witness-type choice applies to three circuits (ShareComputationCircuit,
ShareEncryptionCircuit, and ShareDecryptionCircuit). Locate the condition that
sets has_witness_type (using circuit_meta.name() ==
ShareComputationCircuit::NAME || circuit_meta.name() ==
ShareEncryptionCircuit::NAME || circuit_meta.name() ==
ShareDecryptionCircuit::NAME) and change the comment to accurately describe
these three circuits (or remove the comment if redundant).
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Around line 226-257: The tests generate samples with
BfvPreset::InsecureThreshold512 but call Bounds::compute, Bits::compute and
Configs::compute with DEFAULT_BFV_PRESET, which can produce inconsistent
CRT/moduli-derived values; update each test (e.g., in
test_bound_and_bits_computation_consistency and test_constants_json_roundtrip)
to pass the same preset used for ShareDecryptionCircuitInput::generate_sample
into the compute calls (use the BfvPreset variable or replace DEFAULT_BFV_PRESET
with BfvPreset::InsecureThreshold512) so that
ShareDecryptionCircuitInput::generate_sample, Bounds::compute, Bits::compute and
Configs::compute all use an identical preset.
- Around line 165-179: In Witness::compute replace the .unwrap() on
input.secret_key.try_decrypt(...) with proper error propagation (return a
CircuitsErrors variant) so decryption failures bubble up instead of panicking;
use the try_decrypt result (e.g., match or ? operator) and include context
mentioning the failing party/mod_idx. Also add an explicit check for missing
ciphertexts (when mod_idx >= party_cts.len()) and return a descriptive error
rather than silently skipping — update handling around party_cts,
party_commitments, and party_shares so ragged/missing entries cause a clear
CircuitsErrors indicating which party/mod_idx/threshold_l failed.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs`:
- Around line 97-112: The variable sum_ciphertexts is computed via homomorphic
addition but never used; either remove the dead computation loop or extend the
ShareDecryptionCircuitInput to carry it. Fix options: (A) delete the
sum_ciphertexts declaration and the for-loop that builds it so only
honest_ciphertexts and dkg_secret_key are returned, or (B) add a field
sum_ciphertexts: Vec<Ciphertext> to the ShareDecryptionCircuitInput struct and
include sum_ciphertexts in the returned ShareDecryptionCircuitInput {
honest_ciphertexts, secret_key: dkg_secret_key, sum_ciphertexts } so downstream
code can consume it; update any constructors/usages of
ShareDecryptionCircuitInput accordingly.
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs`:
- Around line 135-144: The test test_generate_smudging_noise_sample is passing
DkgInputType::SecretKey instead of exercising the SmudgingNoise branch; update
the call to ShareEncryptionCircuitInput::generate_sample in that test to pass
DkgInputType::SmudgingNoise (replace SecretKey with SmudgingNoise) so the
SmudgingNoise path is actually tested and no longer duplicates the previous
SecretKey test.
In
`@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs`:
- Around line 399-415: The assertion assert_eq!(out.bounds.delta,
out.bounds.delta) is tautological; replace it by computing the expected delta
independently (e.g. via Configs::compute or the circuit's bounds helper) and
compare that to out.bounds.delta inside test_full_computation_with_sample;
specifically, obtain the expected value (for example let expected =
Configs::compute(preset, &()).unwrap().bounds.delta or call the circuit bounds
computation used elsewhere) and assert_eq!(out.bounds.delta, expected) so the
test verifies the correct delta value rather than comparing the value to itself.
In
`@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/utils.rs`:
- Around line 28-36: lagrange_recover_at_zero lacks a length check between
party_ids and shares which can cause an out-of-bounds panic when indexing
shares[i]; add a precondition at the start of the function to verify
party_ids.len() == shares.len() and return an appropriate CircuitsErrors
(consistent with errors used in crt_reconstruct) if they differ, ensuring no
further processing occurs on mismatched slices.
In `@crates/zk-helpers/src/utils.rs`:
- Around line 217-229: The doc comment for bigint_2d_to_json_values is incorrect
(says "1D") and the function duplicates logic present in
bigint_1d_to_json_values; update the doc comment to state it converts a 2D
vector of BigInt to nested JSON string values and refactor the implementation to
call the existing bigint_1d_to_json_values for each inner Vec<BigInt> (similar
to how bigint_3d_to_json_values delegates) to remove duplication and keep
behavior consistent across bigint_1d_to_json_values, bigint_2d_to_json_values,
and bigint_3d_to_json_values.
🧹 Nitpick comments (13)
circuits/lib/src/configs/insecure/dkg.nr (1)
103-135: Pre-existing:SHARE_ENCRYPTION_CONFIGS_SKandSHARE_ENCRYPTION_CONFIGSare identical.Both config instances (Lines 103–118 and 120–135) are constructed with exactly the same parameters. If the consolidation pattern applied to
SHARE_DECRYPTION_BIT_MSGis any indication, these could be unified in a follow-up.Cargo.toml (1)
154-154: Pinnum-integerto"=0.1.46"for consistency with othernum-*crates.The sibling crates
num("=0.4.3"),num-bigint("=0.4.6"), andnum-traits("=0.2.19") all use exact version pinning, butnum-integeruses a semver range"0.1"instead. While this is unlikely to cause issues in practice, pinning to the current stable version"=0.1.46"would be consistent with the workspace convention for thenum-*family.crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/circuit.rs (1)
24-35: Consider documenting the length invariant betweend_share_polysandreconstructing_parties.Both vectors must have exactly
threshold + 1elements, and their indices correspond 1-to-1. This invariant is upheld bygenerate_sampleand presumably byWitness::compute, but a doc comment on the struct would help future consumers.crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/sample.rs (1)
152-159: Simplify public key aggregation by removing redundant Vec collection and re-iteration.The code clones values with
.map(|p| p.pk_share.clone()), collects them into a Vec, then iterates and clones again with.iter().cloned()before calling.aggregate(). This is redundant—.aggregate()works directly on iterators, as shown in the same module atpk_aggregation/sample.rs:60.♻️ Suggested simplification
let public_key: PublicKey = parties .iter() .map(|p| p.pk_share.clone()) - .collect::<Vec<_>>() - .iter() - .cloned() .aggregate() .unwrap();crates/zk-helpers/src/circuits/dkg/share_computation/utils.rs (1)
39-42: Consider importingArcinstead of using the fully qualified path.
std::sync::Arcin the function signature is unusual when auseimport is cleaner.Optional: import Arc
+use std::sync::Arc; use crate::utils::bigint_to_field; use crate::CircuitsErrors; ... -pub fn parity_matrix_constant_string( - threshold_params: &std::sync::Arc<fhe::bfv::BfvParameters>, +pub fn parity_matrix_constant_string( + threshold_params: &Arc<fhe::bfv::BfvParameters>,crates/zk-helpers/src/circuits/dkg/share_decryption/codegen.rs (2)
66-68:dkg_counterpart().unwrap()is called twice — assign to a local variable.Both Line 67 and Line 68 call
preset.dkg_counterpart().unwrap()independently. Extract once for clarity.Optional cleanup
+ let dkg_metadata = preset.dkg_counterpart().unwrap().metadata(); 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, )
42-48: Redundant closure inmap_err.
map_err(|e| CircuitsErrors::SerdeJson(e))can be simplified using a method reference.Optional
let json = witness .to_json() - .map_err(|e| CircuitsErrors::SerdeJson(e))?; + .map_err(CircuitsErrors::SerdeJson)?;crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/utils.rs (1)
51-54: Consider explicit parentheses for clarity in modular arithmetic expressions.These lines rely on Rust's left-to-right
*/%precedence to produce the correct modular result. While mathematically correct, the dense chaining is easy to misread during review. Adding parentheses around the intended groupings would make the intent self-documenting.Suggested clarification
- lambda_i = (&lambda_i * &num % &m * &den_inv % &m + &m) % &m; + lambda_i = (((&lambda_i * &num) % &m * &den_inv) % &m + &m) % &m;- secret = (&secret + &y_i_pos * &lambda_i % &m + &m) % &m; + secret = (&secret + ((&y_i_pos * &lambda_i) % &m) + &m) % &m;crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs (1)
131-153:threshold: 0default is a footgun — callers must remember to override it.
Configs::computesetsthreshold: 0with a comment that the caller should set it. If any codegen or witness path consumesConfigs.thresholdwithout overriding, the circuit would silently operate with an invalid threshold. Consider either:
- Accepting threshold as part of
Input(e.g., extendInputfrom()to a struct containing threshold), or- Making
thresholdanOption<usize>so the circuit panics or errors explicitly if it's unset.crates/zk-helpers/src/math.rs (2)
143-198:decompose_residuecontains manyassert!calls — acceptable for crypto verification but note the u64 underflow potential.The ~10 assertions serve as mathematical invariant checks ensuring the residue decomposition identity
xi_hat + r1·qi + r2·cyclo = xiholds. In a crypto library this fail-fast approach is reasonable since a violated invariant means a soundness bug.However, expressions like
(num_coeffs.len() as u64) - 1(lines 160, 168, 176, 182, 186) will underflow if the polynomial has 0 coefficients since the arithmetic is onu64. While this should never happen with valid inputs, a guard or usingchecked_subwould make it robust against edge cases in future refactors.
200-210: Consider expanding unit tests for the math module.Only
compute_q_producthas a direct unit test. Functions likecompute_q_inverse_mod_t,compute_t_inv_mod_q,compute_k0is, andcyclotomic_polynomialare exercised indirectly through circuit tests, but dedicated unit tests with edge cases (e.g., coprimality failure, single-modulus input, n=1) would catch regressions earlier.crates/zk-helpers/src/bin/zk_cli.rs (2)
271-283: Prefer?over.unwrap()onsearch_defaults()for consistency.Line 272 uses
.unwrap()which will panic ifsearch_defaults()returnsNone. Every other fallible call in this match block propagates errors with?. Converting this to a descriptive error keeps the CLI from panicking with a confusing backtrace.Proposed fix
- let sd = preset.search_defaults().unwrap(); + let sd = preset.search_defaults().ok_or_else(|| { + anyhow!("no search defaults available for preset {:?}", preset) + })?;
237-239: Minor: stale comment on the else branch.Line 238 says "pk circuit: always secret key" but this branch now applies to all circuits without a witness-type choice (pk, user-data-encryption, pk-generation, pk-aggregation, decrypted-shares-aggregation), not just
pk.Proposed fix
} else { - // pk circuit: always secret key (no smudging noise). + // Circuits without a witness-type choice default to secret key. DkgInputType::SecretKey };
0xjei
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 156: The dependency version for num-integer in Cargo.toml is using a
flexible range ("0.1"); change it to an exact pinned patch release (e.g.,
=0.1.44) to match the workspace convention used by other third-party
dependencies so builds are reproducible; update the num-integer entry to use the
exact version string and save the manifest.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs`:
- Around line 224-231: The loop that builds shares uses indexed access into
d_share_polys for indices 0..=threshold but never checks that
d_share_polys.len() >= threshold + 1, which can panic; before the for coeff_idx
in 0..degree loop (or at start of the function), validate that
d_share_polys.len() >= (threshold + 1) and if not return an Err (or propagate a
descriptive error) or otherwise handle the short input; alternatively change the
inner indexing to safe access (e.g., get) and map a missing entry to an early
error with context mentioning d_share_polys and threshold so the function does
not panic at runtime.
🧹 Nitpick comments (8)
crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
82-87: Misleading_ctprefix — variable is actually used.In Rust, the
_prefix conventionally signals an intentionally unused binding. Since_ctis assigned tociphertexton line 87, consider renaming it toct.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(); ShareEncryptionCircuitInput { plaintext: pt, - ciphertext: _ct, + ciphertext: ct, public_key: dkg_public_key,crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/utils.rs (2)
49-61: Dense operator-chained expressions are hard to audit for correctness.Lines 58 and 61 rely on
*and%having equal precedence with left-to-right associativity to be correct. This works, but a reader has to mentally parse(&lambda_i * &num % &m * &den_inv % &m + &m) % &mto verify no misgrouping. Breaking into named intermediates would make the intent obvious and reduce the chance of a future edit introducing a precedence bug.Also,
BigInt::from(x_i)on line 52 is re-created on every inner-loop iteration — it can be hoisted above thefor jloop.Suggested clarification
+ let x_i_b = BigInt::from(x_i); let mut lambda_i = BigInt::from(1); for (j, &x_j) in party_ids.iter().enumerate() { if i != j { - let x_i_b = BigInt::from(x_i); let x_j_b = BigInt::from(x_j); - let num = BigInt::from(0) - &x_j_b; + let num = -&x_j_b; let den = &x_i_b - &x_j_b; let den_inv = crate::math::mod_inverse_bigint(&den, &m) .ok_or_else(|| CircuitsErrors::Other("lagrange: den not invertible".into()))?; - lambda_i = (&lambda_i * &num % &m * &den_inv % &m + &m) % &m; + lambda_i = ((&lambda_i * &num % &m) * &den_inv % &m + &m) % &m; } } - secret = (&secret + &y_i_pos * &lambda_i % &m + &m) % &m; + let term = (&y_i_pos * &lambda_i) % &m; + secret = (&secret + &term + &m) % &m;
107-128: Consider adding error-path tests.The happy-path coverage is solid. A couple of negative tests (e.g., mismatched lengths returning
Err, or a non-coprime modulus triggering the "not invertible" path) would give confidence that the error handling works as intended.crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs (2)
140-141:threshold: 0default is misleading and error-prone.
Configsexposes athresholdfield, butConfigs::computealways sets it to0. Downstream consumers that readconfigs.thresholdwill silently get the wrong value. Consider either removing the field fromConfigs(sinceWitness::computereads it from the input instead) or populating it properly. A sentinel0for a threshold is indistinguishable from a valid single-party threshold.
160-163:build_pair_for_presetis called twice — once insideConfigs::computeand again here.
Witness::computealready callsConfigs::compute(preset, &())at line 161 (which internally callsbuild_pair_for_preset), then immediately callsbuild_pair_for_presetagain at line 162. Consider either exposing the params throughConfigsor extracting the shared call to avoid the redundant work.crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs (1)
94-118: Redundantbuild_pair_for_presetcall insideConfigs::compute.
Configs::computecallsbuild_pair_for_preset, then separately callsBounds::computeandBits::compute— both of which callbuild_pair_for_presetagain internally. This means the preset is parsed three times for a singleConfigs::computecall. Not a correctness issue, but worth noting for a future clean-up.crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (1)
111-141: Tests are minimal but cover both DkgInputType variants — acceptable for sample validation.Consider adding an assertion on the inner dimension (number of ciphertexts per party equals
threshold_params.moduli().len()) to catch structural regressions.Suggested additional assertion
assert_eq!(sample.honest_ciphertexts.len(), committee.n); + let (threshold_params, _) = build_pair_for_preset(BfvPreset::InsecureThreshold512).unwrap(); + for party_cts in &sample.honest_ciphertexts { + assert_eq!(party_cts.len(), threshold_params.moduli().len()); + } assert_eq!( sample.secret_key.coeffs.len(), BfvPreset::InsecureThreshold512.metadata().degree );crates/zk-helpers/src/bin/zk_cli.rs (1)
252-327: Match arms follow the established pattern — consider a table-driven dispatch to reduce boilerplate.Each circuit arm follows the same
generate_sample → codegenpattern with slight parameter variations. This works well today with 8 circuits, but as more circuits are added the match block will keep growing. A trait-based or registry-driven dispatch (e.g., registering a factory closure alongside eachCircuit) could reduce this in the future.Not urgent for this PR — just a note for when the circuit count grows further.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Line 280: The call to preset.search_defaults().unwrap() can panic if a preset
lacks search defaults; replace the unwrap with proper handling in the
surrounding CLI flow (e.g., match or if let): check preset.search_defaults()
and, if None, print a clear error message (include preset identifier) and exit
with a non-zero code or return an Err from the CLI main handler; if Some(sd)
proceed. Update the code around the variable sd (where preset.search_defaults()
is used) to use the matched value instead of unwrapping so missing defaults are
handled gracefully.
🧹 Nitpick comments (4)
crates/zk-helpers/src/math.rs (2)
143-198:decompose_residuepanics on invalid inputs instead of returningResult.This function has 7
assert!/assert_eq!calls and 3.unwrap()calls, all of which will panic with unhelpful messages if inputs are invalid. Every other function in this module returnsResultorOptionfor fallible operations, making this an inconsistency.If this is intentional (invariant checks during witness generation that should never fail), consider using
debug_assert!/debug_assert_eq!so the checks are elided in release builds, or at minimum add messages to the assertions to aid debugging:- assert_eq!(xi, &xi_hat_mod_rqi); + debug_assert_eq!(xi, &xi_hat_mod_rqi, "xi != xi_hat reduced mod R_qi");
200-209: Minimal test coverage for a module with 11 public functions.Only
compute_q_producthas a unit test. Functions likecompute_q_inverse_mod_t,compute_t_inv_mod_q,mod_inverse_bigint,cyclotomic_polynomial, and the CRT helpers would benefit from basic correctness tests — especially the edge cases (non-coprime inputs, single-element moduli, etc.).Would you like me to generate a set of unit tests covering the remaining public functions?
crates/zk-helpers/src/bin/zk_cli.rs (1)
327-335: Use the existingcommitteevariable instead of re-computing it.Line 330 calls
CiphernodesCommitteeSize::Small.values()directly, but the same value is already computed and stored incommitteeon Line 262. All other match arms usecommittee—this one should too for consistency.Proposed fix
name if name == <DecryptedSharesAggregationCircuit as Circuit>::NAME => { let sample = DecryptedSharesAggregationCircuitInput::generate_sample( preset, - CiphernodesCommitteeSize::Small.values(), + committee, ); let circuit = DecryptedSharesAggregationCircuit; circuit.codegen(preset, &sample)? }crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs (1)
140-152:threshold: 0is a dead field inConfigs.
Configs::computehardcodesthresholdto0(Line 141), andgenerate_configsincodegen.rsnever reads it. If the field isn't needed in the config fragment, consider removing it fromConfigsto avoid confusion. If it's intended for future use, the comment should state that explicitly.
2 participants
requires #1269 first
Summary by CodeRabbit
Release Notes
New Features
Chores