fix: lock fhe.rs and revs to tags [skip-line-limit]#1596
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
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 pins FHE Git dependencies to tag ChangesWorkspace dependency pinning and support updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/support/Cargo.toml`:
- Line 46: The Cargo.toml pins rand to 0.9.2 but the template still calls
thread_rng(...) which is incompatible with rand 0.9; update the template to use
the rand 0.9 API (replace thread_rng() usage with rand::thread_rng()
import/usage or use a local RNG instance via rand::rngs::ThreadRng and the Rng
trait) in templates/default/program/src/lib.rs so the template compiles with
rand = "=0.9.2" and ensure any uses of Rng::gen*/gen_range* are updated to the
0.9 method signatures if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04ee0196-9a61-4f8b-828d-284a3e50853c
⛔ Files ignored due to path filters (2)
crates/support/Cargo.lockis excluded by!**/*.lockcrates/support/methods/guest/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/support/Cargo.tomlcrates/support/contracts/ImageID.solcrates/support/host/Cargo.tomlcrates/support/host/src/bin/profile_risc0.rscrates/support/methods/guest/Cargo.tomlcrates/support/program/Cargo.toml
✅ Files skipped from review due to trivial changes (2)
- crates/support/program/Cargo.toml
- crates/support/contracts/ImageID.sol
0xjei
changed the title
Contributor
Author
|
we might need to make a change into fhe.rs as well so might end up releasing a new version - hold on for a bit here |
Contributor
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 (4)
crates/support/host/src/bin/profile_risc0.rs (1)
19-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix inconsistent
BFV_PRESETtoken handling to prevent silent insecure fallback.
BFV_PRESETparsing mixes old/new names: e.g., Line 20 documentsSECURE_DKG_BFV_8192, but Line 24 only acceptsSECURE_DKG_8192. That documented value falls into the unknown branch (Line 27) and silently downgrades toInsecureThreshold512(Line 30), which is a wrong runtime outcome.Proposed fix
- // INSECURE_THRESHOLD_BFV_512 | INSECURE_DKG_BFV_512 | - // SECURE_THRESHOLD_BFV_8192 | SECURE_DKG_BFV_8192 + // INSECURE_THRESHOLD_512 | INSECURE_DKG_512 | + // SECURE_THRESHOLD_8192 | SECURE_DKG_8192 let param_set: BfvPreset = match std::env::var("BFV_PRESET").ok().as_deref() { + Some("INSECURE_THRESHOLD_512") => BfvPreset::InsecureThreshold512, Some("INSECURE_DKG_512") => BfvPreset::InsecureDkg512, - Some("SECURE_THRESHOLD_BFV_8192") => BfvPreset::SecureThreshold8192, + Some("SECURE_THRESHOLD_8192") => BfvPreset::SecureThreshold8192, Some("SECURE_DKG_8192") => BfvPreset::SecureDkg8192,Also applies to: 27-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/support/host/src/bin/profile_risc0.rs` around lines 19 - 24, The BFV_PRESET parsing mixes legacy/new token names causing documented values like "SECURE_DKG_BFV_8192" to fall through to the default insecure preset; update the match in param_set creation to accept the legacy/_BFV_ variants as aliases (e.g., add Some("SECURE_DKG_BFV_8192") => BfvPreset::SecureDkg8192, Some("INSECURE_DKG_BFV_512") => BfvPreset::InsecureDkg512, Some("INSECURE_THRESHOLD_BFV_512") => BfvPreset::InsecureThreshold512, etc.) so each documented token maps to the correct BfvPreset variant (referencing param_set and the BfvPreset enum), and consider replacing the silent default with an explicit error or a logged warning if an unknown value is provided.crates/zk-helpers/src/circuits/dkg/share_computation/sample.rs (1)
91-114:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
.unwrap()with?for consistent error propagation.The
SmudgingNoisebranch contains multiple.unwrap()calls (lines 99, 100, 106, 114) that will panic instead of propagating errors properly. The pattern.map_err(...).unwrap()defeats the purpose of error mapping—.map_err()transforms the error type but doesn't handle it, and then.unwrap()panics if an error occurs.🔧 Proposed fix
let esi_coeffs = trbfv .generate_smudging_error(committee.n, lambda, &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(), &mut 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| array2_u64_to_bigint(&arr)) .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)) + })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/dkg/share_computation/sample.rs` around lines 91 - 114, The SmudgingNoise branch uses several .unwrap() calls that should instead propagate errors; replace the unwraps on trbfv.generate_smudging_error(...) , share_manager.bigints_to_poly(...), share_manager.generate_secret_shares_from_poly(...), and CrtPolynomial::from_mod_q_polynomial(...).center(...) with the ? operator and map their errors into CircuitsErrors::Sample (e.g., .map_err(|e| CircuitsErrors::Sample(format!(...)))? ), and ensure the conversion of esi_sss_u64 into SecretShares stays compatible with the function's Result return type so the propagated errors compile.crates/test-helpers/src/usecase_helpers.rs (1)
51-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDerive the test lambda from the same preset family instead of hard-coding
Insecure(40).This helper builds its BFV params from
InsecureDkg512but sends an unrelated literal lambda in the request. That splits one fixture path across two security sources and can make helper-generated shares diverge from the runtime preset contract.♻️ Suggested change
- let bfv_params = BfvParamSet::from(BfvPreset::InsecureDkg512).build_arc(); + let threshold_preset = BfvPreset::InsecureThreshold512; + let dkg_preset = threshold_preset + .dkg_counterpart() + .expect("threshold preset should have a DKG counterpart"); + let bfv_params = BfvParamSet::from(dkg_preset).build_arc(); @@ - lambda: LambdaConfig::Insecure(40), + lambda: threshold_preset.lambda_config(),Also applies to: 75-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/test-helpers/src/usecase_helpers.rs` around lines 51 - 52, The helper currently builds BFV params from BfvPreset::InsecureDkg512 (via BfvParamSet::from(BfvPreset::InsecureDkg512).build_arc()) but then hard-codes a different lambda (Insecure(40)), splitting fixture sources; change the code so the test lambda is derived from the same preset family used to build bfv_params instead of the literal Insecure(40) — i.e., obtain the security lambda/value from the BfvPreset/BfvParamSet used to construct bfv_params (or compute it from bfv_params) and use that derived value when forming the request lambda (replace the hard-coded lambda at the site around lines 75-79 with the derived value).crates/trbfv/src/calculate_decryption_share.rs (1)
115-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
es_poly_sumbefore modulo indexing.
let es_idx = index % es_poly_sum.len();panics when the request carries zero smudging polynomials. This worker should reject that shape with a regular error instead of crashing the compute thread.Suggested fix
let params = req.trbfv_config.params(); let sk_poly_sum = req.sk_poly_sum; let es_poly_sum = req.es_poly_sum; + + if es_poly_sum.is_empty() { + bail!("es_poly_sum must contain at least one smudging polynomial"); + } info!("Calculating d_share_poly..."); let d_share_poly = req .ciphertexts .into_iter()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/trbfv/src/calculate_decryption_share.rs` around lines 115 - 127, The code uses es_poly_sum.len() without checking for zero which causes a panic at let es_idx = index % es_poly_sum.len(); — add a guard before iterating req.ciphertexts (or at start of the function that computes d_share_poly) to check if es_poly_sum.is_empty() and return a proper Err with a clear message (e.g., "no smudging polynomials provided") instead of allowing modulo-by-zero; adjust the surrounding flow that builds d_share_poly (the mapping closure that references es_idx and ShareManager::new) to assume es_poly_sum.len() > 0 after this guard so no further changes to the closure are needed.
🧹 Nitpick comments (4)
crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (1)
89-102: ⚡ Quick winRemove duplicate error handling.
Lines 90-96 and 97-102 contain identical
.map_err()calls. Only one is needed to convert the error intoCircuitsErrors::Sample.♻️ Proposed fix
let esi_coeffs = trbfv .generate_smudging_error(sd.z as usize, lambda, &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 - )) })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs` around lines 89 - 102, The call to trbfv.generate_smudging_error used to produce esi_coeffs has duplicate .map_err(...) conversions to CircuitsErrors::Sample; remove the redundant .map_err(...) so there is a single map_err that maps the error to CircuitsErrors::Sample and then apply the ? to propagate the result (i.e., keep the call to trbfv.generate_smudging_error(...).map_err(|e| CircuitsErrors::Sample(format!("Failed to generate smudging error: {:?}", e)))? and delete the extra identical map_err).crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
73-86: ⚡ Quick winRemove duplicate error handling.
Lines 74-80 and 81-86 contain identical
.map_err()calls. Only one is needed.♻️ Proposed fix
let esi_coeffs = trbfv .generate_smudging_error(num_ciphertexts as usize, lambda, &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 - )) })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs` around lines 73 - 86, The code calls trbfv.generate_smudging_error(...).map_err(...) twice producing duplicate error handling; remove the redundant .map_err(...) so the call to generate_smudging_error is followed by a single .map_err(|e| CircuitsErrors::Sample(format!("Failed to generate smudging error: {:?}", e)))?; keep the existing variable name esi_coeffs and the same error message/enum (CircuitsErrors::Sample) and ensure the final ? operator remains to propagate the mapped error.crates/fhe-params/src/search/bfv.rs (1)
197-202: ⚡ Quick winLock the revised
b_freshalgebra behind a numeric regression test.The current tests only check whether a candidate passes or fails; they do not pin the exact
b_fresh/b_c/b_sm_minvalues for a fixed config. Since this line changes the acceptance threshold for the search, a deterministic assertion here would make future algebra drift immediately visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fhe-params/src/search/bfv.rs` around lines 197 - 202, Add a numeric regression test that pins the exact computed b_fresh (and optionally b_c and b_sm_min) for a fixed BFV search config: write a unit test that constructs the same config used in search, reproduces the computation using term_d_b_b_chi_n, benc_min and b_fresh from the bfv search code, and assert equality against a hard-coded BigUint literal (or hex string) so any future algebraic drift fails the test; ensure the test uses deterministic inputs and documents the expected value in a comment.crates/fhe-params/src/presets.rs (1)
420-439: ⚡ Quick winAdd direct tests for the new preset-to-lambda mapping.
lambda_config()/lambda()are now the only translation layer between preset security tiers and runtime smudging security, but the test module never asserts the secure/insecure mapping or that each preset still constructs the expected variant. A small table-driven test here would catch constant drift before it reaches runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fhe-params/src/presets.rs` around lines 420 - 439, Add a unit test in the presets module that table-drives over every BfvPreset variant (use the existing preset registry or enumerate the variants) and for each preset verifies two things: 1) preset.lambda_config() matches the preset.metadata().security (i.e., equals LambdaConfig::Secure(meta.lambda) when metadata().security == SecurityTier::SECURE and LambdaConfig::Insecure(meta.lambda) otherwise), and 2) preset.lambda().is_ok() (or otherwise behaves as expected) to ensure the preset still constructs a runtime Lambda without unnoticed drift; reference the lambda_config, lambda, SecurityTier, LambdaConfig, Lambda, PresetError, and BfvPreset symbols when locating code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/trbfv/src/gen_pk_share_and_sk_sss.rs`:
- Around line 34-36: GenPkShareAndSkSssRequest.lambda currently uses the new
enum LambdaConfig (with variants Secure(usize)/Insecure(usize)) which breaks
deserialization of legacy integer-only payloads; fix by making LambdaConfig
accept both legacy bare integers and the new enum form during deserialization
(e.g. annotate LambdaConfig with #[serde(untagged)] or implement custom
Deserialize for LambdaConfig that maps a bare usize to the appropriate variant
and still supports the Secure/Insecure tagged form), keep the existing
LambdaConfig::into_lambda helper unchanged so callers (and
GenPkShareAndSkSssRequest.lambda) continue to use it as before.
---
Outside diff comments:
In `@crates/support/host/src/bin/profile_risc0.rs`:
- Around line 19-24: The BFV_PRESET parsing mixes legacy/new token names causing
documented values like "SECURE_DKG_BFV_8192" to fall through to the default
insecure preset; update the match in param_set creation to accept the
legacy/_BFV_ variants as aliases (e.g., add Some("SECURE_DKG_BFV_8192") =>
BfvPreset::SecureDkg8192, Some("INSECURE_DKG_BFV_512") =>
BfvPreset::InsecureDkg512, Some("INSECURE_THRESHOLD_BFV_512") =>
BfvPreset::InsecureThreshold512, etc.) so each documented token maps to the
correct BfvPreset variant (referencing param_set and the BfvPreset enum), and
consider replacing the silent default with an explicit error or a logged warning
if an unknown value is provided.
In `@crates/test-helpers/src/usecase_helpers.rs`:
- Around line 51-52: The helper currently builds BFV params from
BfvPreset::InsecureDkg512 (via
BfvParamSet::from(BfvPreset::InsecureDkg512).build_arc()) but then hard-codes a
different lambda (Insecure(40)), splitting fixture sources; change the code so
the test lambda is derived from the same preset family used to build bfv_params
instead of the literal Insecure(40) — i.e., obtain the security lambda/value
from the BfvPreset/BfvParamSet used to construct bfv_params (or compute it from
bfv_params) and use that derived value when forming the request lambda (replace
the hard-coded lambda at the site around lines 75-79 with the derived value).
In `@crates/trbfv/src/calculate_decryption_share.rs`:
- Around line 115-127: The code uses es_poly_sum.len() without checking for zero
which causes a panic at let es_idx = index % es_poly_sum.len(); — add a guard
before iterating req.ciphertexts (or at start of the function that computes
d_share_poly) to check if es_poly_sum.is_empty() and return a proper Err with a
clear message (e.g., "no smudging polynomials provided") instead of allowing
modulo-by-zero; adjust the surrounding flow that builds d_share_poly (the
mapping closure that references es_idx and ShareManager::new) to assume
es_poly_sum.len() > 0 after this guard so no further changes to the closure are
needed.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/sample.rs`:
- Around line 91-114: The SmudgingNoise branch uses several .unwrap() calls that
should instead propagate errors; replace the unwraps on
trbfv.generate_smudging_error(...) , share_manager.bigints_to_poly(...),
share_manager.generate_secret_shares_from_poly(...), and
CrtPolynomial::from_mod_q_polynomial(...).center(...) with the ? operator and
map their errors into CircuitsErrors::Sample (e.g., .map_err(|e|
CircuitsErrors::Sample(format!(...)))? ), and ensure the conversion of
esi_sss_u64 into SecretShares stays compatible with the function's Result return
type so the propagated errors compile.
---
Nitpick comments:
In `@crates/fhe-params/src/presets.rs`:
- Around line 420-439: Add a unit test in the presets module that table-drives
over every BfvPreset variant (use the existing preset registry or enumerate the
variants) and for each preset verifies two things: 1) preset.lambda_config()
matches the preset.metadata().security (i.e., equals
LambdaConfig::Secure(meta.lambda) when metadata().security ==
SecurityTier::SECURE and LambdaConfig::Insecure(meta.lambda) otherwise), and 2)
preset.lambda().is_ok() (or otherwise behaves as expected) to ensure the preset
still constructs a runtime Lambda without unnoticed drift; reference the
lambda_config, lambda, SecurityTier, LambdaConfig, Lambda, PresetError, and
BfvPreset symbols when locating code to change.
In `@crates/fhe-params/src/search/bfv.rs`:
- Around line 197-202: Add a numeric regression test that pins the exact
computed b_fresh (and optionally b_c and b_sm_min) for a fixed BFV search
config: write a unit test that constructs the same config used in search,
reproduces the computation using term_d_b_b_chi_n, benc_min and b_fresh from the
bfv search code, and assert equality against a hard-coded BigUint literal (or
hex string) so any future algebraic drift fails the test; ensure the test uses
deterministic inputs and documents the expected value in a comment.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs`:
- Around line 89-102: The call to trbfv.generate_smudging_error used to produce
esi_coeffs has duplicate .map_err(...) conversions to CircuitsErrors::Sample;
remove the redundant .map_err(...) so there is a single map_err that maps the
error to CircuitsErrors::Sample and then apply the ? to propagate the result
(i.e., keep the call to trbfv.generate_smudging_error(...).map_err(|e|
CircuitsErrors::Sample(format!("Failed to generate smudging error: {:?}", e)))?
and delete the extra identical map_err).
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs`:
- Around line 73-86: The code calls
trbfv.generate_smudging_error(...).map_err(...) twice producing duplicate error
handling; remove the redundant .map_err(...) so the call to
generate_smudging_error is followed by a single .map_err(|e|
CircuitsErrors::Sample(format!("Failed to generate smudging error: {:?}", e)))?;
keep the existing variable name esi_coeffs and the same error message/enum
(CircuitsErrors::Sample) and ensure the final ? operator remains to propagate
the mapped error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7815f54-1a22-4fcf-990a-2c9549e0bb67
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockcrates/support/Cargo.lockis excluded by!**/*.lockcrates/support/methods/guest/Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.locktemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
Cargo.tomlcrates/aggregator/src/actors/threshold_plaintext_aggregator.rscrates/evm/src/domain/interfold_events.rscrates/fhe-params/src/lib.rscrates/fhe-params/src/presets.rscrates/fhe-params/src/search/bfv.rscrates/keyshare/src/actors/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/support/Cargo.tomlcrates/support/host/Cargo.tomlcrates/support/host/src/bin/profile_risc0.rscrates/support/methods/guest/Cargo.tomlcrates/support/program/Cargo.tomlcrates/test-helpers/src/usecase_helpers.rscrates/tests/tests/integration.rscrates/trbfv/src/calculate_decryption_key.rscrates/trbfv/src/calculate_decryption_share.rscrates/trbfv/src/calculate_threshold_decryption.rscrates/trbfv/src/gen_esi_sss.rscrates/trbfv/src/gen_pk_share_and_sk_sss.rscrates/trbfv/src/helpers.rscrates/trbfv/src/trbfv_request.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/circuits/dkg/share_computation/computation.rscrates/zk-helpers/src/circuits/dkg/share_computation/sample.rscrates/zk-helpers/src/circuits/dkg/share_decryption/sample.rscrates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rscrates/zk-helpers/src/circuits/dkg/share_encryption/computation.rscrates/zk-helpers/src/circuits/dkg/share_encryption/sample.rscrates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/sample.rscrates/zk-helpers/src/circuits/threshold/pk_generation/computation.rscrates/zk-helpers/src/circuits/threshold/pk_generation/sample.rscrates/zk-helpers/src/circuits/threshold/share_decryption/sample.rscrates/zk-prover/tests/common/node_fold_witness.rscrates/zk-prover/tests/fold_accumulators_e2e_tests.rscrates/zk-prover/tests/local_e2e_tests.rsexamples/CRISP/Cargo.tomltemplates/default/Cargo.toml
💤 Files with no reviewable changes (5)
- crates/zk-helpers/src/bin/zk_cli.rs
- crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs
- crates/zk-prover/tests/local_e2e_tests.rs
- crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs
- crates/zk-prover/tests/fold_accumulators_e2e_tests.rs
✅ Files skipped from review due to trivial changes (2)
- crates/fhe-params/src/lib.rs
- crates/aggregator/src/actors/threshold_plaintext_aggregator.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/default/Cargo.toml
- Cargo.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/trbfv/src/calculate_threshold_decryption.rs (1)
76-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject ragged decryption-share batches before transposing.
transpose()assumes every party supplied the same number of shares. A longer later row will panic onresult[col_idx], and a shorter row carries a malformed(threshold_shares, reconstructing_parties)pair intodecrypt_from_shares. Validatevec_of_bytes.len() == ciphertexts.len()for every party before buildingd_share_polys.💡 Suggested fix
for (party_id, vec_of_bytes) in ordered_polys { + ensure!( + vec_of_bytes.len() == ciphertexts.len(), + "party {} provided {} shares for {} ciphertexts", + party_id, + vec_of_bytes.len(), + ciphertexts.len() + ); + let polys: Vec<Poly<PowerBasis>> = vec_of_bytes .iter() .map(|bytes| try_poly_pb_from_bytes(bytes, ¶ms)) .collect::<Result<Vec<_>>>()?;Also applies to: 168-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/trbfv/src/calculate_threshold_decryption.rs` around lines 76 - 87, Before calling transpose() ensure every party supplied the expected number of shares: inside the loop that iterates ordered_polys (the block that builds d_share_polys, uses try_poly_pb_from_bytes, pushes into d_share_polys and reconstructing_parties), check that vec_of_bytes.len() == ciphertexts.len() and return an error (or propagate) if not; do the same validation at the other analogous site that builds d_share_polys (the second batch handling around the later use that leads into decrypt_from_shares). This prevents transpose() from panicking on uneven rows and avoids passing malformed (threshold_shares, reconstructing_parties) into decrypt_from_shares.crates/trbfv/src/calculate_decryption_share.rs (1)
120-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against empty
es_poly_sumbefore modulo indexing.Line 126 computes
index % es_poly_sum.len(). Ifes_poly_sumis empty, this panics and aborts request handling. Add an explicit precondition check and return an error instead of panicking.Suggested fix
pub fn calculate_decryption_share( cipher: &Cipher, req: CalculateDecryptionShareRequest, ) -> Result<CalculateDecryptionShareResponse> { @@ let sk_poly_sum = req.sk_poly_sum; let es_poly_sum = req.es_poly_sum; + anyhow::ensure!( + !es_poly_sum.is_empty(), + "es_poly_sum must contain at least one polynomial" + ); @@ .map(|(index, ciphertext)| { let share_manager = ShareManager::new(num_ciphernodes, threshold, params.clone())?; info!("Create decryption share for ct index {}...", index);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/trbfv/src/calculate_decryption_share.rs` around lines 120 - 127, The closure that computes es_idx uses index % es_poly_sum.len() and will panic if es_poly_sum is empty; add an explicit check for es_poly_sum.is_empty() before the modulo in the map closure (the code creating ShareManager and computing es_idx) and return an Err with a descriptive error (propagated to the caller) instead of performing the modulo; ensure the closure's result types remain consistent (or convert to a Result if needed) so ShareManager::new(...) and subsequent processing are skipped when es_poly_sum is empty.
🧹 Nitpick comments (2)
crates/aggregator/src/actors/threshold_plaintext_aggregator.rs (1)
648-657: ⚡ Quick winLog the outer failure message alongside the structured threshold detail.
When
failure.thresholdis present, this branch only logsthreshold.message. That drops the higher-level context captured infailure.message(for example, which operation wrapped the fhe.rs error). Logging both keeps the new structured fields without losing the outer diagnostic.💡 Suggested fix
match &failure.threshold { Some(threshold) => warn!( e3_id = %self.e3_id, kind = ?threshold.kind, party_id = ?threshold.party_id, - "threshold decryption failed with structured error: {}", - threshold.message, + error = %failure.message, + detail = %threshold.message, + "threshold decryption failed with structured error", ), None => warn!( e3_id = %self.e3_id, "threshold decryption failed: {}", failure.message,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aggregator/src/actors/threshold_plaintext_aggregator.rs` around lines 648 - 657, The warn! call inside the ComputeRequestErrorKind::TrBFV arm only logs threshold.message and loses the outer context; update the logging in the branch that handles failure.threshold (from trbfv_err.failure()) to include failure.message alongside threshold.message (e.g., add failure.message as a field or in the formatted string) while keeping existing structured fields like e3_id, threshold.kind, threshold.party_id and threshold.message; modify the warn! invocation used in threshold decryption failure to log both messages.crates/fhe-params/src/search/bfv.rs (1)
197-202: ⚡ Quick winAdd a regression test for the revised noise-budget arithmetic.
This PR changes
b_fresh, but the module tests still only assert coarse success/failure properties. Please pinb_fresh/b_cfor at least one known config so future edits here do not silently retune the preset search.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fhe-params/src/search/bfv.rs` around lines 197 - 202, Add a regression test that pins the computed b_fresh (and optionally b_c) for a known bfv_search_config to a hard-coded BigUint so future arithmetic changes are caught: create a unit test (e.g. test_bfresh_regression) that constructs the same bfv_search_config used in presets, computes term_d_b_b_chi_n and b_fresh using the existing expressions (using benc_min, term_d_b_b_chi_n, and bfv_search_config fields), and assert_eq! the result to a literal BigUint constant (precomputed for that config); if your module exposes b_c, also assert it against a fixed value. Ensure the test uses the exact config values from the preset so the expected literal matches the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/support/Cargo.toml`:
- Around line 41-47: The git dependencies fhe, fhe-traits, and fhe-util in the
support crate are unpinned and causing lockfile drift; update the support
crate's Cargo.toml entries for fhe, fhe-traits, and fhe-util to pin them to the
exact same tag or commit (e.g., add tag = "v0.2.2" or rev = "<commit-hash>" to
each dependency so all three resolve identically), ensure Cargo.lock is
committed after that change, add publish = false to the support-related crate
manifests that are not meant to be published (so downstream consumers can't pull
an unpinned copy), and update CI to run cargo commands with --locked or --frozen
to prevent implicit lockfile updates.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/sample.rs`:
- Around line 88-90: The smudging-noise branch still calls unwrap() (at the
sites you noted) which can panic instead of returning CircuitsErrors::Sample;
replace each unwrap() with a fallible conversion that maps the error into
CircuitsErrors::Sample (e.g., use .map_err(|e|
CircuitsErrors::Sample(e.to_string()))? for Result-returning calls or
.ok_or_else(|| CircuitsErrors::Sample("...".into()))? for Option-to-Result
conversions) so the function consistently returns
Err(CircuitsErrors::Sample(...)) instead of panicking; apply this change to
every unwrap in the smudging-noise branch so error propagation matches how
preset.lambda() is already handled.
---
Outside diff comments:
In `@crates/trbfv/src/calculate_decryption_share.rs`:
- Around line 120-127: The closure that computes es_idx uses index %
es_poly_sum.len() and will panic if es_poly_sum is empty; add an explicit check
for es_poly_sum.is_empty() before the modulo in the map closure (the code
creating ShareManager and computing es_idx) and return an Err with a descriptive
error (propagated to the caller) instead of performing the modulo; ensure the
closure's result types remain consistent (or convert to a Result if needed) so
ShareManager::new(...) and subsequent processing are skipped when es_poly_sum is
empty.
In `@crates/trbfv/src/calculate_threshold_decryption.rs`:
- Around line 76-87: Before calling transpose() ensure every party supplied the
expected number of shares: inside the loop that iterates ordered_polys (the
block that builds d_share_polys, uses try_poly_pb_from_bytes, pushes into
d_share_polys and reconstructing_parties), check that vec_of_bytes.len() ==
ciphertexts.len() and return an error (or propagate) if not; do the same
validation at the other analogous site that builds d_share_polys (the second
batch handling around the later use that leads into decrypt_from_shares). This
prevents transpose() from panicking on uneven rows and avoids passing malformed
(threshold_shares, reconstructing_parties) into decrypt_from_shares.
---
Nitpick comments:
In `@crates/aggregator/src/actors/threshold_plaintext_aggregator.rs`:
- Around line 648-657: The warn! call inside the ComputeRequestErrorKind::TrBFV
arm only logs threshold.message and loses the outer context; update the logging
in the branch that handles failure.threshold (from trbfv_err.failure()) to
include failure.message alongside threshold.message (e.g., add failure.message
as a field or in the formatted string) while keeping existing structured fields
like e3_id, threshold.kind, threshold.party_id and threshold.message; modify the
warn! invocation used in threshold decryption failure to log both messages.
In `@crates/fhe-params/src/search/bfv.rs`:
- Around line 197-202: Add a regression test that pins the computed b_fresh (and
optionally b_c) for a known bfv_search_config to a hard-coded BigUint so future
arithmetic changes are caught: create a unit test (e.g. test_bfresh_regression)
that constructs the same bfv_search_config used in presets, computes
term_d_b_b_chi_n and b_fresh using the existing expressions (using benc_min,
term_d_b_b_chi_n, and bfv_search_config fields), and assert_eq! the result to a
literal BigUint constant (precomputed for that config); if your module exposes
b_c, also assert it against a fixed value. Ensure the test uses the exact config
values from the preset so the expected literal matches the current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0decf14-6d01-4b5e-af62-2bc5a0e35c98
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockcrates/support/Cargo.lockis excluded by!**/*.lockcrates/support/methods/guest/Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.locktemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
Cargo.tomlcircuits/benchmarks/results_insecure_minimum/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_minimum/integration_summary.jsoncircuits/benchmarks/results_insecure_minimum/report.mdcircuits/benchmarks/results_secure_micro/benchmark_run_meta.jsoncircuits/benchmarks/results_secure_micro/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_micro/integration_summary.jsoncircuits/benchmarks/results_secure_micro/report.mdcircuits/lib/src/configs/insecure/threshold.nrcircuits/lib/src/configs/secure/threshold.nrcrates/aggregator/src/actors/threshold_plaintext_aggregator.rscrates/evm/src/domain/interfold_events.rscrates/fhe-params/src/lib.rscrates/fhe-params/src/presets.rscrates/fhe-params/src/search/bfv.rscrates/keyshare/src/actors/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/support/Cargo.tomlcrates/support/host/Cargo.tomlcrates/support/host/src/bin/profile_risc0.rscrates/support/methods/guest/Cargo.tomlcrates/support/program/Cargo.tomlcrates/test-helpers/src/usecase_helpers.rscrates/tests/tests/integration.rscrates/trbfv/src/calculate_decryption_key.rscrates/trbfv/src/calculate_decryption_share.rscrates/trbfv/src/calculate_threshold_decryption.rscrates/trbfv/src/gen_esi_sss.rscrates/trbfv/src/gen_pk_share_and_sk_sss.rscrates/trbfv/src/helpers.rscrates/trbfv/src/trbfv_request.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/circuits/dkg/share_computation/computation.rscrates/zk-helpers/src/circuits/dkg/share_computation/sample.rscrates/zk-helpers/src/circuits/dkg/share_decryption/sample.rscrates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rscrates/zk-helpers/src/circuits/dkg/share_encryption/computation.rscrates/zk-helpers/src/circuits/dkg/share_encryption/sample.rscrates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/sample.rscrates/zk-helpers/src/circuits/threshold/pk_generation/computation.rscrates/zk-helpers/src/circuits/threshold/pk_generation/sample.rscrates/zk-helpers/src/circuits/threshold/share_decryption/sample.rscrates/zk-prover/tests/common/node_fold_witness.rscrates/zk-prover/tests/fold_accumulators_e2e_tests.rscrates/zk-prover/tests/local_e2e_tests.rsexamples/CRISP/Cargo.tomlexamples/CRISP/interfold.config.yamlexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonpackages/interfold-contracts/test/fixtures/bfv_vk_binding/folded_artifacts.jsonscripts/build-circuits.tstemplates/default/Cargo.toml
💤 Files with no reviewable changes (5)
- crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs
- crates/zk-prover/tests/fold_accumulators_e2e_tests.rs
- crates/zk-prover/tests/local_e2e_tests.rs
- crates/zk-helpers/src/bin/zk_cli.rs
- crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs
✅ Files skipped from review due to trivial changes (8)
- crates/support/methods/guest/Cargo.toml
- crates/support/program/Cargo.toml
- circuits/benchmarks/results_secure_micro/benchmark_run_meta.json
- crates/fhe-params/src/lib.rs
- circuits/benchmarks/results_insecure_minimum/report.md
- circuits/benchmarks/results_secure_micro/report.md
- circuits/benchmarks/results_secure_micro/crisp_verify_gas.json
- circuits/benchmarks/results_secure_micro/integration_summary.json
🚧 Files skipped from review as they are similar to previous changes (3)
- templates/default/Cargo.toml
- Cargo.toml
- crates/support/host/src/bin/profile_risc0.rs
hmzakhalid
pushed a commit
that referenced
this pull request
Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit