refactor: fhe-params and circuits rename [skip-line-limit]#1244
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedToo many files! This PR contains 211 files, which is 61 over the limit of 150. You can disable this status message by setting the
✨ 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 |
0xjei
requested review from
cedoor,
ctrlc03 and
hmzakhalid
and removed request for
cedoor
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/fhe-params/README.md (1)
228-228:⚠️ Potential issue | 🟡 MinorInconsistent preset name in documentation.
Line 228 still references the old naming convention
SecureThresholdBfv8192while the rest of the documentation has been updated to useSecureThreshold8192.📝 Suggested fix
-The production preset `SecureThresholdBfv8192` can be reproduced using: +The production preset `SecureThreshold8192` can be reproduced using:crates/zk-helpers/src/commitments.rs (1)
547-592:⚠️ Potential issue | 🔴 CriticalTests use old naming conventions that no longer exist.
The test functions reference renamed constants and functions that will cause compilation failures:
- Line 576:
DS_SPMshould beDS_SHARE_ENCRYPTION- Line 578:
compute_spm_commitment_from_sharesshould becompute_share_encryption_commitment_from_shares- Line 587:
compute_pk_trbfv_challengeshould becompute_threshold_pk_challenge- Line 588:
compute_bfv_enc_challengeshould becompute_share_encryption_challenge🐛 Proposed fix for test renames
fn compute_spm_commitment_from_shares_matches_manual_payload() { // ... test setup unchanged ... let input_size = payload.len() as u32; let io_pattern = [0x80000000 | input_size, 1]; - let expected = field_to_bigint(compute_commitments(payload, DS_SPM, io_pattern)[0]); + let expected = field_to_bigint(compute_commitments(payload, DS_SHARE_ENCRYPTION, io_pattern)[0]); - let actual = compute_spm_commitment_from_shares(&y, party_idx, mod_idx); + let actual = compute_share_encryption_commitment_from_shares(&y, party_idx, mod_idx); assert_eq!(actual, expected); } #[test] fn challenge_lengths_match_expected_output() { let payload = vec![Field::from(1u64), Field::from(2u64)]; let l = 3; - let pk_trbfv = compute_pk_trbfv_challenge(payload.clone(), l); - let bfv_enc = compute_bfv_enc_challenge(payload, l); + let pk_challenge = compute_threshold_pk_challenge(payload.clone(), l); + let share_enc = compute_share_encryption_challenge(payload, l); - assert_eq!(pk_trbfv.len(), 2 * l); - assert_eq!(bfv_enc.len(), 2 * l); + assert_eq!(pk_challenge.len(), 2 * l); + assert_eq!(share_enc.len(), 2 * l); }circuits/lib/src/core/threshold/share_decryption.nr (1)
158-175:⚠️ Potential issue | 🟡 MinorMethod rename from
verifytoexecuteand comment updates needed.Line 159-162 comments still reference old variable names ("s commitment", "e commitment"). Should be updated to "sk commitment" and "e_sm commitment".
📝 Proposed fix
pub fn execute(self) { - // Step 1: Verify s commitment matches expected + // Step 1: Verify sk commitment matches expected self.verify_agg_sk_commitment(); - // Step 2: Verify e commitment matches expected + // Step 2: Verify e_sm commitment matches expected self.verify_agg_e_sm_commitment();
🤖 Fix all issues with AI agents
In `@circuits/lib/Nargo.toml`:
- Line 10: The dependency line for the bignum crate in Nargo.toml references a
non-existent tag v0.0.3; update the bignum entry (the line starting with bignum
= { tag = "v0.0.3", git = "https://github.com/gnosisguild/noir-bignum" }) to
point to a valid release or branch—either change tag to a real tag (e.g.,
"v0.2.1" or a later valid tag) or switch to a correct repository/branch that
actually contains v0.0.3; ensure the tag/branch you choose exists in the repo
before committing.
In `@circuits/lib/src/configs/insecure/dkg.nr`:
- Around line 7-162: The module defines inconsistent modulus parameters: global
L and QIS (L=1, QIS) are used by share_encryption configs while pk and
share_computation use L_THRESHOLD/QIS_THRESHOLD (L_THRESHOLD=2, QIS_THRESHOLD)
via PARITY_MATRIX and ShareComputationConfigs; unify them by making the DKG-wide
modulus parameters consistent. Fix: replace usages of local L and QIS in
share_encryption config declarations (SHARE_ENCRYPTION_CONFIGS_SK,
SHARE_ENCRYPTION_CONFIGS_E_SM, and any arrays like SHARE_ENCRYPTION_K0IS,
PK_BOUNDS, R2_BOUNDS, P2_BOUNDS) to use L_THRESHOLD and QIS_THRESHOLD (or
vice-versa choose the single canonical symbol) and update their types from
ShareEncryptionConfigs<L> to ShareEncryptionConfigs<L_THRESHOLD> and array
lengths to match L_THRESHOLD; ensure PARITY_MATRIX,
ShareComputationConfigs::new(QIS_THRESHOLD) and ShareEncryptionConfigs::new(...)
all consume the same QIS_THRESHOLD/QIS value to eliminate mismatch.
In `@circuits/lib/src/configs/insecure/threshold.nr`:
- Around line 98-99: Update the stale comments referencing "greco" and "GRECO"
to the new name "user_data_encryption": change the header comment above the
USER_DATA_ENCRYPTION_CONFIGS constant (currently "greco - configs") and the
section header "share_decryption (GRECO)" to use "user_data_encryption" (e.g.,
"user_data_encryption - configs" and "share_decryption (user_data_encryption)")
so comments match the constant USER_DATA_ENCRYPTION_CONFIGS and the renamed
component.
In `@circuits/lib/src/core/dkg/pk.nr`:
- Around line 12-16: The docstring for pk0 and pk1 incorrectly labels them as
"public input"; update the comments for the fields pk0: [Polynomial<N>; L] and
pk1: [Polynomial<N>; L] to state they are private inputs (e.g., "private input"
or "committed/private polynomial components") so it accurately reflects that
these Polynomial<N> arrays are committed and must remain hidden.
In `@circuits/lib/src/core/threshold/share_decryption.nr`:
- Around line 96-112: The docstrings for verify_agg_sk_commitment and
verify_agg_e_sm_commitment are stale: update the comment above
verify_agg_sk_commitment to say it verifies that sk hashes to
expected_sk_commitment, and update the comment above verify_agg_e_sm_commitment
to say it verifies that e_sm hashes to expected_e_sm_commitment so the text
matches the parameters and expected_* fields used in
compute_aggregated_shares_commitment::<N, L, BIT_SK>(self.sk) and
compute_aggregated_shares_commitment::<N, L, BIT_E_SM>(self.e_sm) respectively.
In `@circuits/lib/src/lib.nr`:
- Line 13: Update the crate-level documentation comment that still reads
"production" to use the new terminology "secure" so it matches the PR
objectives; locate the doc line that currently says "levels (insecure,
production) and different circuit variants (DKG, Threshold)" and change
"production" to "secure" so it reads "levels (insecure, secure) and different
circuit variants (DKG, Threshold)".
In `@circuits/lib/src/math/commitments.nr`:
- Around line 68-74: The domain separator constant DS_CLG_PK_GENERATION
currently contains bytes for "CLGPK_GENERATION" (underscore after K) but its
name/comment indicate "CLG_PK_GENERATION" (underscore after G); fix by making
the byte array encode the intended ASCII string "CLG_PK_GENERATION" (then
zero-pad to 64 bytes) so the bytes, the pub global DS_CLG_PK_GENERATION
declaration, and the comment all match; alternatively, if the current bytes are
intended, update the comment/name to "CLGPK_GENERATION" for consistency—ensure
whichever choice you make is applied to the DS_CLG_PK_GENERATION declaration and
its comment.
In `@crates/fhe-params/src/presets.rs`:
- Around line 334-357: The From<BfvPreset> impl for SecureDkg8192 currently
selects secure_8192::dkg::PLAINTEXT_MODULUS and secure_8192::dkg::MODULI but the
BFV pair expects the BFV-specific constants; update the SecureDkg8192 branch in
the From<BfvPreset> implementation to use
secure_8192::dkg::BFV_PLAINTEXT_MODULUS and secure_8192::dkg::BFV_MODULI
(instead of PLAINTEXT_MODULUS and MODULI) so SecureDkg8192 construction matches
the values asserted in build_pair and the test.
In `@crates/support/host/src/bin/profile_risc0.rs`:
- Around line 18-20: The comment above the preset is outdated: replace the
misleading "Use InsecureThresholdBfv512 parameter set" text with a brief comment
that reflects the actual symbol DEFAULT_BFV_PRESET being used (e.g., "Use
DEFAULT_BFV_PRESET parameter set"), so that the code around param_set and the
call to build_bfv_params_from_set_arc(param_set) accurately documents the preset
in use.
In `@examples/CRISP/crates/crisp-constants/Cargo.toml`:
- Around line 11-12: Add the missing workspace dependency declaration for the
e3-config crate in the workspace [workspace.dependencies] section of Cargo.toml
so Cargo can resolve it; specifically, declare e3-config = { workspace = true }
alongside e3-fhe-params to match its usage
(e3_config::bfv_config::DEFAULT_BFV_PRESET) in src/lib.rs.
🧹 Nitpick comments (4)
crates/config/src/lib.rs (1)
7-19: Minor visibility inconsistency withapp_config.The
bfv_configmodule is declared aspub mod(line 8) whileapp_configuses a privatemod(line 7). Both are then re-exported withpub use *. This works but creates inconsistent patterns:
app_config: private module, items accessible only via re-exportbfv_config: public module, items accessible via bothconfig::bfv_config::*andconfig::*Consider aligning with
app_config's pattern for consistency:Suggested change
mod app_config; -pub mod bfv_config; +mod bfv_config;templates/default/program/src/lib.rs (1)
29-30: LGTM! Consider using the helper function for cleaner code.The migration to
DEFAULT_BFV_PRESETis correct. However, there's a convenience functionbuild_bfv_params_from_set_arc(as seen incrates/fhe-params/src/builder.rs) that could simplify the parameter construction.♻️ Optional: Use build_bfv_params_from_set_arc for cleaner code
use e3_config::bfv_config::DEFAULT_BFV_PRESET; -use e3_fhe_params::{build_bfv_params_arc, encode_bfv_params, BfvParamSet}; +use e3_fhe_params::{build_bfv_params_from_set_arc, encode_bfv_params, BfvParamSet}; ... let params_set: BfvParamSet = DEFAULT_BFV_PRESET.into(); - let params = build_bfv_params_arc( - params_set.degree, - params_set.plaintext_modulus, - ¶ms_set.moduli, - params_set.error1_variance, - ); + let params = build_bfv_params_from_set_arc(params_set);Also applies to: 41-47
circuits/lib/src/configs/secure/dkg.nr (1)
117-132: Consider documenting why SK and E_SM share encryption configs are identical.Both
SHARE_ENCRYPTION_CONFIGS_SKandSHARE_ENCRYPTION_CONFIGS_E_SMuse the exact same parameters. While this may be intentional (same encryption scheme for different shares), a brief comment explaining this design choice would improve maintainability.Also applies to: 141-156
circuits/lib/src/configs/secure/threshold.nr (1)
106-106: Minor: Comment inconsistency.Other sections use the pattern
{circuit_name} - configs(e.g., "pk_generation - configs"), but this says "greco - configs". Consider updating to "user_data_encryption - configs" for consistency.-// greco - configs +// user_data_encryption - configs
Contributor
Author
|
CRISP is okay if it fails since zkfhe-generator commitments are not up-to-date |
This was
linked to
issues
Jan 31, 2026
cedoor
approved these changes
Feb 2, 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.
hello, sorry for this PR since this will be moving lots of things but it's more than needed. Breaking changes addressed:
crates/fhe-paramspvssterminology also from crates in favour of zk)noir-bignumin order to rename the oldEnclave_TRBFVand make them Threshold and DKGcircuits/lib/configs/default(this will avoid having duplicated bin for insecure & secure)N_PUBLIC_INPUTSinstead of awaiting the generator directly in Noir.error1_variance& remove stale constants from dkg params.Summary by CodeRabbit
New Features
DEFAULT_BFV_PRESETconstant.Refactor
defaultandsecurevariants.✏️ Tip: You can customize this high-level summary in your review settings.