refactor: general zk-helpers refactoring#1283
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughMultiple crates updated to propagate fallible sample generation: many Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
cedoor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
68-81:⚠️ Potential issue | 🟠 MajorDuplicate
.map_err()produces nested, garbled error messages.There are two chained
.map_err()calls on the result ofgenerate_smudging_error. The first (lines 70–75) converts the original error toCircuitsErrors::Sample. The second (lines 76–81) then re-wraps thatCircuitsErrorsinside anotherCircuitsErrors::Sample, producing a message like"Failed to generate smudging error: Sample(\"Failed to generate smudging error: ...\")".Remove the duplicate and use
?directly after the first mapping.🐛 Proposed fix
let esi_coeffs = trbfv .generate_smudging_error(num_ciphertexts as usize, 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 - )) })?;crates/zk-helpers/src/circuits/dkg/share_computation/sample.rs (1)
88-114:⚠️ Potential issue | 🔴 Critical
SmudgingNoisebranch still panics via.unwrap()instead of propagating errors.The entire purpose of this refactor is to make
generate_samplereturnResult, yet theSmudgingNoisebranch uses.unwrap()on four fallible operations (lines 97, 98, 104, 112). This means the function will panic instead of returningErrif any of these fail — completely defeating the error-propagation goal. TheSecretKeybranch (lines 52–86) correctly usesmap_err(...)?throughout.🐛 Proposed fix — propagate errors consistently
DkgInputType::SmudgingNoise => { let esi_coeffs = trbfv .generate_smudging_error(committee.n as usize, sd.lambda as usize, &mut rng) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate smudging error: {:?}", e )) - }) - .unwrap(); - let esi_poly = share_manager.bigints_to_poly(&esi_coeffs).unwrap(); + })?; + 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(), rng) .map_err(|e| { CircuitsErrors::Sample(format!("Failed to generate error shares: {:?}", e)) - }) - .unwrap(); + })?; let secret_sss: SecretShares = esi_sss_u64 .into_iter() .map(|arr| arr.mapv(BigInt::from)) .collect(); let mut secret_crt = CrtPolynomial::from_mod_q_polynomial(&esi_coeffs, threshold_params.moduli()); - secret_crt.center(threshold_params.moduli()).unwrap(); + secret_crt.center(threshold_params.moduli()).map_err(|e| { + CircuitsErrors::Sample(format!("Failed to center secret CRT: {:?}", e)) + })?; (secret_crt, secret_sss) }
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs`:
- Around line 85-92: The call to
share_manager.generate_secret_shares_from_poly(esi_poly.clone(), &mut
rng.clone()) is mapped twice with .map_err(), which re-wraps an
already-converted error; remove the duplicate mapping so the error is converted
to CircuitsErrors::Sample only once. Locate the expression assigning esi_sss_u64
and keep a single .map_err(|e| CircuitsErrors::Sample(format!("Failed to
generate error shares: {:?}", e))) (or adjust to propagate the original error
type if intended), ensuring generate_secret_shares_from_poly is only followed by
one map_err transformation.
🧹 Nitpick comments (3)
crates/zk-helpers/src/utils.rs (1)
176-178:unwrap()will panic on an emptymodulislice.Since
compute_max_modulusis now apubAPI, callers outsidecompute_modulus_bitcould pass an empty slice. Consider returning anOption<u64>or documenting the panic precondition explicitly (e.g.,# Panics).Option A: Add a `# Panics` doc section
/// Computes the maximum modulus from a vector of moduli. /// /// # Arguments /// * `moduli` - Vector of moduli /// /// # Returns /// The maximum modulus +/// +/// # Panics +/// Panics if `moduli` is empty. pub fn compute_max_modulus(moduli: &[u64]) -> u64 { moduli.iter().copied().max().unwrap() }crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/sample.rs (1)
148-155: Redundant doublemap_err— inner already maps toCircuitsErrors.The inner
map_err(Line 148) already converts theShapeErrorintoCircuitsErrors::Sample. The outermap_err(Line 153) then wraps theCircuitsErrorsDebug output into anotherCircuitsErrors::Sample, producing nested messages like"Failed to collect sk_sss_collected: Sample(\"sk_sss_collected shape: ...\")". The outermap_errcan be removed sincecollect::<Result<Vec<_>, _>>()?will propagate the inner error directly.Suggested simplification
.collect::<Result<Vec<_>, _>>() - .map_err(|e| { - CircuitsErrors::Sample(format!("Failed to collect sk_sss_collected: {:?}", e)) - })?; + ?;Same applies to the
es_sss_collectedblock at Lines 167–174.crates/zk-helpers/src/bin/zk_cli.rs (1)
328-332: Inconsistent use ofcommitteevariable.Line 262 already defines
let committee = CiphernodesCommitteeSize::Small.values();, but this branch re-creates the same value inline. All other branches use thecommitteelocal. Consider using it here too for consistency.♻️ Suggested fix
name if name == <DecryptedSharesAggregationCircuit as Circuit>::NAME => { let sample = DecryptedSharesAggregationCircuitInput::generate_sample( preset, - CiphernodesCommitteeSize::Small.values(), + committee, )?;
cedoor
force-pushed
the
refactoring/zk-helpers
branch
from
f5a3be4 to
83f4c70
Compare
- Remove redundant .map_err() on generate_secret_shares_from_poly result - Error now converted to CircuitsErrors::Sample only once Co-authored-by: Cursor <cursoragent@cursor.com>
cedoor
force-pushed
the
refactoring/zk-helpers
branch
from
f0efb78 to
0f57efa
Compare
2 participants
Summary by CodeRabbit