refactor: make CRS deterministic [skip-line-limit]#1347
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Closed
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:
📝 WalkthroughWalkthroughCentralizes CRP creation to deterministic default-seed constructors, removes the Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Benchmarks CLI / Scripts
participant Runtime as FHE Runtime
participant FHEParams as crates/fhe-params
participant ZKHelpers as zk-helpers (codegen/computation)
participant Noir as Generated Noir Circuit
CLI->>Runtime: request PK-generation / benchmarking (no seed)
Runtime->>FHEParams: decode BFV params → request deterministic CRP
FHEParams-->>Runtime: CommonRandomPoly (deterministic)
Runtime->>ZKHelpers: provide params + CRP (no `a` field)
ZKHelpers->>Noir: inject CRP matrix const into generated code
Noir-->>ZKHelpers: circuit computes CRT-polynomial internally
ZKHelpers-->>CLI: sample/benchmark output (raw data preserved)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
crates/fhe-params/Cargo.toml (2)
24-25:fhe-traitsin[dev-dependencies]is redundant.
fhe-traitsis already listed in[dependencies](line 12), making it available in tests automatically. The[dev-dependencies]entry is unnecessary.Proposed fix
-[dev-dependencies] -fhe-traits = { workspace = true } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe-params/Cargo.toml` around lines 24 - 25, Remove the redundant fhe-traits entry from the [dev-dependencies] table in Cargo.toml: keep fhe-traits only under [dependencies] (where it already appears) and delete the line `fhe-traits = { workspace = true }` from the [dev-dependencies] section so tests use the dependency from [dependencies].
22-22: Userand_chacha = { workspace = true }to maintain consistency with other crates.The workspace root defines
rand_chacha = "=0.3.1". Other crates likecrates/fhe/Cargo.tomluserand_chacha = { workspace = true }. Hardcoding the version here risks version drift.Proposed fix
-rand_chacha = "0.3.1" +rand_chacha = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe-params/Cargo.toml` at line 22, The Cargo.toml currently pins rand_chacha to a literal version (rand_chacha) which risks version drift; change that dependency to use the workspace-provided rand_chacha (the same pattern used in the other crates) so it inherits the workspace version constraint rather than hardcoding "0.3.1". Update the dependency declaration for rand_chacha in the crate's Cargo.toml to reference the workspace entry instead of a fixed version.crates/trbfv/tests/integration.rs (1)
53-53: Remove commented-out code.- // let crp = ArcBytes::from_bytes(crp_raw.to_bytes());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trbfv/tests/integration.rs` at line 53, Remove the leftover commented-out line that instantiates ArcBytes from crp_raw (the line containing "// let crp = ArcBytes::from_bytes(crp_raw.to_bytes());"); simply delete this commented code so the test file no longer contains dead/commented-out instantiation of crp using ArcBytes/from_bytes/to_bytes.crates/fhe-params/src/crp.rs (2)
25-27: Simplify the default seed expression;ChaCha8Rngis not the RNG that generates the CRP.
<ChaCha8Rng as SeedableRng>::Seed::default()evaluates unconditionally to[0u8; 32].ChaCha8Rngis only referenced for its associatedSeedtype; the actual polynomial generation is handled internally byCommonRandomPoly::new_deterministic. Keeping this import suggests ChaCha8 is used as the CRP's RNG, which is inaccurate and adds an unnecessary dependency onrand_chachain this module.♻️ Proposed fix
-use rand::SeedableRng; -use rand_chacha::ChaCha8Rng; +/// Fixed seed used for deterministic CRP generation. +const DEFAULT_CRP_SEED: [u8; 32] = [0u8; 32]; pub fn create_deterministic_crp_from_default_seed(params: &Arc<BfvParameters>) -> CommonRandomPoly { - create_deterministic_crp_from_seed(params, <ChaCha8Rng as SeedableRng>::Seed::default()) + create_deterministic_crp_from_seed(params, DEFAULT_CRP_SEED) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe-params/src/crp.rs` around lines 25 - 27, The call in create_deterministic_crp_from_default_seed unnecessarily references ChaCha8Rng for its Seed; replace the explicit <ChaCha8Rng as SeedableRng>::Seed::default() with a generic default seed (e.g., Default::default() or the literal [0u8; 32]) when calling create_deterministic_crp_from_seed, and remove any rand_chacha/ChaCha8Rng import from this module so the function signatures remain create_deterministic_crp_from_default_seed(params: &Arc<BfvParameters>) -> CommonRandomPoly and delegate to create_deterministic_crp_from_seed without implying ChaCha8 is used.
15-22: Remove spurious#[allow(dead_code)]and unnecessary double-borrow.Two independent nits in this function:
#[allow(dead_code)]is redundant on apub fnthat is also re-exported from the crate root — the compiler never raisesdead_codefor public items. The attribute is misleading (implies the function could be deleted).paramsis already&Arc<BfvParameters>; writing¶mscreates&&Arc<BfvParameters>and silently relies on deref coercion. Passingparamsdirectly is more precise and avoids aclippy::needless_borrowwarning.♻️ Proposed fix
-#[allow(dead_code)] /// Creates a Common Random Polynomial for the given BFV parameters and seed. pub fn create_deterministic_crp_from_seed( params: &Arc<BfvParameters>, seed: [u8; 32], ) -> CommonRandomPoly { - CommonRandomPoly::new_deterministic(¶ms, seed).unwrap() + CommonRandomPoly::new_deterministic(params, seed).unwrap() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe-params/src/crp.rs` around lines 15 - 22, Remove the misleading #[allow(dead_code)] attribute from the pub fn create_deterministic_crp_from_seed and stop double-borrowing params: pass params (type &Arc<BfvParameters>) directly to CommonRandomPoly::new_deterministic instead of ¶ms to avoid creating a &&Arc and a clippy::needless_borrow; keep the existing unwrap() behavior and the call to CommonRandomPoly::new_deterministic as the return.crates/zk-helpers/src/circuits/threshold/pk_generation/codegen.rs (1)
57-59: Consider replacingunwrap()with?for consistency.Line 57 uses
preset.build_pair().unwrap()while the function now returnsResult. Ifbuild_pair()can fail, this would panic instead of propagating the error. Since the function signature already supports error propagation, consider usingmap_err+?for consistency.♻️ Suggested fix
- let (threshold_params, _) = preset.build_pair().unwrap(); + let (threshold_params, _) = preset.build_pair().map_err(|e| CircuitsErrors::Other(e.to_string()))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/pk_generation/codegen.rs` around lines 57 - 59, The code currently calls preset.build_pair().unwrap(), which will panic on error; change it to propagate the error instead by replacing the unwrap with a fallible call (e.g., use preset.build_pair()? or preset.build_pair().map_err(|e| /* convert e */)? if the error type needs conversion) so that let (threshold_params, _) = preset.build_pair().unwrap(); becomes a non-panicking binding and the function can return the error to the caller before calling crp_matrix_constant_string(&threshold_params)?.circuits/benchmarks/scripts/run_benchmarks.sh (1)
138-139: Tab-delimited parsing is fragile if the script is reformatted.The
%%\t*and#*\tparameter expansions rely on literal tab characters between%%/#*and the*. If an editor or formatter replaces tabs with spaces, this will silently break. Consider adding a comment or using a more robust delimiter.♻️ Alternative using IFS-based splitting
- c="${line%% *}" - modes="${line#* }" + # Split on tab (jq output uses \t as separator) + IFS=$'\t' read -r c modes <<< "$line"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/run_benchmarks.sh` around lines 138 - 139, The current parsing using the parameter expansions c="${line%% *}" and modes="${line#* }" depends on a literal tab character and will break if tabs are replaced by spaces; update the parsing to use a robust IFS-based split (set IFS to a tab and use read -r to split into c and modes) or switch to an explicit non-whitespace delimiter and split on that, and add a short comment near the parsing explaining the chosen delimiter so future formatting won't silently break.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fhe-params/src/crp.rs`:
- Around line 77-79: The assignment seed2[0] = 2 is redundant because seed2 is
initialized as [2u8; 32]; remove the stray seed2[0] = 2 statement to avoid
confusion and keep the test clear—locate the seed1 and seed2 declarations
(variables named seed1 and seed2 in crp.rs) and delete the unnecessary seed2[0]
= 2 line.
In `@crates/fhe/src/runtime.rs`:
- Around line 50-55: The code in from_encoded uses
decode_bfv_params_arc(bytes).expect(...) which will panic instead of returning
an Err; replace the expect call with proper error propagation (e.g., let params
= decode_bfv_params_arc(bytes)?;) or, if decode_bfv_params_arc returns a
different error type, use map_err to convert it into the crate's Result error
(e.g., decode_bfv_params_arc(bytes).map_err(|e| <convert e>)?); update the
from_encoded function to remove expect and propagate the decode error so
Fhe::new is only called on success.
---
Nitpick comments:
In `@circuits/benchmarks/scripts/run_benchmarks.sh`:
- Around line 138-139: The current parsing using the parameter expansions
c="${line%% *}" and modes="${line#* }" depends on a literal tab character and
will break if tabs are replaced by spaces; update the parsing to use a robust
IFS-based split (set IFS to a tab and use read -r to split into c and modes) or
switch to an explicit non-whitespace delimiter and split on that, and add a
short comment near the parsing explaining the chosen delimiter so future
formatting won't silently break.
In `@crates/fhe-params/Cargo.toml`:
- Around line 24-25: Remove the redundant fhe-traits entry from the
[dev-dependencies] table in Cargo.toml: keep fhe-traits only under
[dependencies] (where it already appears) and delete the line `fhe-traits = {
workspace = true }` from the [dev-dependencies] section so tests use the
dependency from [dependencies].
- Line 22: The Cargo.toml currently pins rand_chacha to a literal version
(rand_chacha) which risks version drift; change that dependency to use the
workspace-provided rand_chacha (the same pattern used in the other crates) so it
inherits the workspace version constraint rather than hardcoding "0.3.1". Update
the dependency declaration for rand_chacha in the crate's Cargo.toml to
reference the workspace entry instead of a fixed version.
In `@crates/fhe-params/src/crp.rs`:
- Around line 25-27: The call in create_deterministic_crp_from_default_seed
unnecessarily references ChaCha8Rng for its Seed; replace the explicit
<ChaCha8Rng as SeedableRng>::Seed::default() with a generic default seed (e.g.,
Default::default() or the literal [0u8; 32]) when calling
create_deterministic_crp_from_seed, and remove any rand_chacha/ChaCha8Rng import
from this module so the function signatures remain
create_deterministic_crp_from_default_seed(params: &Arc<BfvParameters>) ->
CommonRandomPoly and delegate to create_deterministic_crp_from_seed without
implying ChaCha8 is used.
- Around line 15-22: Remove the misleading #[allow(dead_code)] attribute from
the pub fn create_deterministic_crp_from_seed and stop double-borrowing params:
pass params (type &Arc<BfvParameters>) directly to
CommonRandomPoly::new_deterministic instead of ¶ms to avoid creating a &&Arc
and a clippy::needless_borrow; keep the existing unwrap() behavior and the call
to CommonRandomPoly::new_deterministic as the return.
In `@crates/trbfv/tests/integration.rs`:
- Line 53: Remove the leftover commented-out line that instantiates ArcBytes
from crp_raw (the line containing "// let crp =
ArcBytes::from_bytes(crp_raw.to_bytes());"); simply delete this commented code
so the test file no longer contains dead/commented-out instantiation of crp
using ArcBytes/from_bytes/to_bytes.
In `@crates/zk-helpers/src/circuits/threshold/pk_generation/codegen.rs`:
- Around line 57-59: The code currently calls preset.build_pair().unwrap(),
which will panic on error; change it to propagate the error instead by replacing
the unwrap with a fallible call (e.g., use preset.build_pair()? or
preset.build_pair().map_err(|e| /* convert e */)? if the error type needs
conversion) so that let (threshold_params, _) = preset.build_pair().unwrap();
becomes a non-panicking binding and the function can return the error to the
caller before calling crp_matrix_constant_string(&threshold_params)?.
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 (1)
circuits/benchmarks/scripts/run_benchmarks.sh (1)
126-126:⚠️ Potential issue | 🟡 MinorDisplayed circuit count may be misleading after mode filtering.
$CIRCUITSis computed before mode filtering, soecho $CIRCUITS | wc -wcan overcount. For example, in insecure modeconfigis included in$CIRCUITSbut gets skipped later. Consider printing this summary afterRUN_CIRCUITSis finalized, or usingRUN_CIRCUITShere instead, to avoid confusing the user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/run_benchmarks.sh` at line 126, The printed circuit count uses CIRCUITS which is set before mode filtering and can overcount; update the script in run_benchmarks.sh to print the count after RUN_CIRCUITS is finalized (or replace CIRCUITS with RUN_CIRCUITS) so the summary reflects the actual circuits that will run, e.g., use echo "$RUN_CIRCUITS" | wc -w (with trimming) or equivalent when emitting "Circuits: ..." to avoid misleading output.
🧹 Nitpick comments (6)
crates/fhe-params/Cargo.toml (1)
24-25:fhe-traitsin[dev-dependencies]is redundant.
fhe-traitsis already an unconditional entry in[dependencies](line 12), which means it is automatically available to tests and benchmarks. Repeating it under[dev-dependencies]has no effect and adds noise.♻️ Proposed fix
-[dev-dependencies] -fhe-traits = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe-params/Cargo.toml` around lines 24 - 25, The `fhe-traits` entry under `[dev-dependencies]` is redundant because `fhe-traits` is already declared unconditionally in `[dependencies]`; remove the `fhe-traits = { workspace = true }` line from the `[dev-dependencies]` section so the crate is only listed once under `[dependencies]`, leaving the existing `fhe-traits` dependency declaration intact.circuits/benchmarks/scripts/run_benchmarks.sh (1)
135-160: Tab-delimited parsing is fragile — preferIFS=$'\t' read.Lines 138–139 split on a literal tab embedded in the parameter-expansion pattern. If an editor or linter ever converts that tab to spaces, the split silently breaks and every circuit name would carry its modes string (or vice versa), causing all circuits to run unfiltered.
Using
IFS=$'\t' read -r c modesmakes the delimiter explicit and editor-proof:♻️ Suggested simplification
-while IFS= read -r line; do - [ -z "$line" ] && continue - c="${line%% *}" - modes="${line#* }" +while IFS=$'\t' read -r c modes; do + [ -z "$c" ] && continueThis removes the need for the manual
%%/#splitting entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/run_benchmarks.sh` around lines 135 - 160, The loop that parses CIRCUIT_MODES uses parameter-expansion with a literal tab which is fragile; replace the manual splitting (lines using c="${line%% *}" and modes="${line#* }") by reading each line with an explicit tab IFS (use IFS=$'\t' read -r c modes) inside the while loop that consumes CIRCUIT_MODES. Update the while IFS= read -r line; do ... done <<< "$CIRCUIT_MODES" construct to instead use a while IFS=$'\t' read -r c modes; do and adapt subsequent logic to use the already-populated c and modes variables (preserve checks against CIRCUT_FILTER, MODE, and appending to RUN_CIRCUITS).circuits/benchmarks/config.json (1)
3-14: Consider declaring mode restrictions for all mode-gated circuits in config.json.The
configcircuit now has explicit"modes": ["secure"], butdecrypted_shares_aggregation_mod(insecure-only) anddecrypted_shares_aggregation_bn(secure-only) still rely on hardcoded checks inrun_benchmarks.sh(lines 151–158). Moving their mode restrictions into config.json too would make this file the single source of truth and let you drop the special-caseifblocks in the script.{ "name": "threshold/decrypted_shares_aggregation_bn", "modes": ["secure"] }, { "name": "threshold/decrypted_shares_aggregation_mod", "modes": ["insecure"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/config.json` around lines 3 - 14, Add explicit mode restrictions for the two mode-gated circuits by updating circuits/benchmarks/config.json to include entries for "threshold/decrypted_shares_aggregation_bn" with "modes": ["secure"] and "threshold/decrypted_shares_aggregation_mod" with "modes": ["insecure"], so the config becomes the single source of truth; after that remove the special-case hardcoded checks in run_benchmarks.sh (the if blocks handling decrypted_shares_aggregation_bn/mod) so benchmark selection uses the config.json modes uniformly.crates/zk-helpers/src/circuits/threshold/share_decryption/sample.rs (1)
311-321: Test nametest_generate_template_repeatableis misleading.The test only checks structural equality (limb counts / array lengths), not that two calls produce identical values. Because
OsRngandthread_rngare still used for SK and error generation, the actual polynomial values differ across calls. Either rename the test totest_generate_template_consistent_structureor seed all RNGs to truly verify repeatability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/share_decryption/sample.rs` around lines 311 - 321, Rename the misleading test or make its inputs deterministic: either rename test_generate_template_repeatable to test_generate_template_consistent_structure to reflect it only asserts structural equality, or modify the test (and/or ShareDecryptionCircuitData::generate_sample) to seed all RNGs used (replace OsRng/thread_rng usage or provide a SeedableRng) so that two calls to ShareDecryptionCircuitData::generate_sample(PRESET, committee) produce identical values; reference the test name test_generate_template_repeatable, the generator function ShareDecryptionCircuitData::generate_sample, and RNG usages OsRng/thread_rng (and the committee source CiphernodesCommitteeSize::Small) when making the change.crates/fhe-params/src/crp.rs (1)
21-21: Redundant double-ref:¶mswhereparamsis already&Arc<BfvParameters>.
paramsis already a reference;¶mscreates&&Arc<BfvParameters>. While auto-deref resolves this, it's cleaner to passparamsdirectly.♻️ Proposed fix
- CommonRandomPoly::new_deterministic(¶ms, seed).unwrap() + CommonRandomPoly::new_deterministic(params, seed).unwrap()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe-params/src/crp.rs` at line 21, The call to CommonRandomPoly::new_deterministic currently passes ¶ms which yields a &&Arc<BfvParameters>; change the argument to pass params directly so the signature receives the expected &Arc<BfvParameters> (i.e., replace CommonRandomPoly::new_deterministic(¶ms, seed).unwrap() with a call that passes params, keeping seed the same) to remove the redundant double-reference while preserving unwrap() behavior.crates/zk-helpers/src/circuits/threshold/pk_generation/utils.rs (1)
28-53: Minor formatting nit in generated Noir code.The closing
];will be on the same line as the last polynomial entry rather than on its own line. This is cosmetic but could affect readability of generated code.♻️ Optional: place closing bracket on its own line
Ok(format!( - "pub global CRP: [Polynomial<N>; L] = [\n {}];", - limb_strings.join(",\n ") + "pub global CRP: [Polynomial<N>; L] = [\n {}\n];", + limb_strings.join(",\n ") ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/pk_generation/utils.rs` around lines 28 - 53, The generated Noir code places the closing "];" on the same line as the last polynomial; update crp_matrix_constant_string to emit the closing bracket on its own line by adjusting the final format string (used with limb_strings.join) to append a newline before the closing "];" (ensure indentation matches the existing "\n " prefix so the final output ends with "\n];" on its own line); modify the format invocation that constructs the returned string in crp_matrix_constant_string accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fhe-params/Cargo.toml`:
- Line 22: In Cargo.toml update the rand_chacha dependency line to delegate
versioning to the workspace (replace the hard-coded "0.3.1" with a
workspace-managed entry such as rand_chacha = { workspace = true }) so the crate
uses the root workspace version, and remove the duplicate fhe-traits entry from
[dev-dependencies] since fhe-traits is already declared under [dependencies] and
is available to tests automatically; target the rand_chacha line and the
fhe-traits entry in the Cargo.toml to make these edits.
In `@crates/fhe-params/src/crp.rs`:
- Around line 15-22: Remove the unnecessary #[allow(dead_code)] and change
create_deterministic_crp_from_seed to return a Result instead of unwrapping:
update the signature from returning CommonRandomPoly to returning
Result<CommonRandomPoly, E> (use the concrete error type returned by
CommonRandomPoly::new_deterministic or a suitable boxed error), then replace the
.unwrap() with returning the Result from
CommonRandomPoly::new_deterministic(¶ms, seed). Finally, update callers
(including the one currently invoking this function) to handle the Result
(propagate ? or map errors as appropriate).
---
Outside diff comments:
In `@circuits/benchmarks/scripts/run_benchmarks.sh`:
- Line 126: The printed circuit count uses CIRCUITS which is set before mode
filtering and can overcount; update the script in run_benchmarks.sh to print the
count after RUN_CIRCUITS is finalized (or replace CIRCUITS with RUN_CIRCUITS) so
the summary reflects the actual circuits that will run, e.g., use echo
"$RUN_CIRCUITS" | wc -w (with trimming) or equivalent when emitting "Circuits:
..." to avoid misleading output.
---
Duplicate comments:
In `@crates/fhe/src/runtime.rs`:
- Around line 50-55: The from_encoded function currently calls
decode_bfv_params_arc(bytes).expect(...), which will panic instead of returning
an Err; change this to propagate the decoding error into the Result by using ?
(or map_err to convert the underlying error) when calling decode_bfv_params_arc
in from_encoded so invalid bytes return Err to the caller; keep the creation of
crp via create_deterministic_crp_from_default_seed(¶ms) and return
Ok(Fhe::new(params, crp, rng)) as before.
---
Nitpick comments:
In `@circuits/benchmarks/config.json`:
- Around line 3-14: Add explicit mode restrictions for the two mode-gated
circuits by updating circuits/benchmarks/config.json to include entries for
"threshold/decrypted_shares_aggregation_bn" with "modes": ["secure"] and
"threshold/decrypted_shares_aggregation_mod" with "modes": ["insecure"], so the
config becomes the single source of truth; after that remove the special-case
hardcoded checks in run_benchmarks.sh (the if blocks handling
decrypted_shares_aggregation_bn/mod) so benchmark selection uses the config.json
modes uniformly.
In `@circuits/benchmarks/scripts/run_benchmarks.sh`:
- Around line 135-160: The loop that parses CIRCUIT_MODES uses
parameter-expansion with a literal tab which is fragile; replace the manual
splitting (lines using c="${line%% *}" and modes="${line#* }") by reading each
line with an explicit tab IFS (use IFS=$'\t' read -r c modes) inside the while
loop that consumes CIRCUIT_MODES. Update the while IFS= read -r line; do ...
done <<< "$CIRCUIT_MODES" construct to instead use a while IFS=$'\t' read -r c
modes; do and adapt subsequent logic to use the already-populated c and modes
variables (preserve checks against CIRCUT_FILTER, MODE, and appending to
RUN_CIRCUITS).
In `@crates/fhe-params/Cargo.toml`:
- Around line 24-25: The `fhe-traits` entry under `[dev-dependencies]` is
redundant because `fhe-traits` is already declared unconditionally in
`[dependencies]`; remove the `fhe-traits = { workspace = true }` line from the
`[dev-dependencies]` section so the crate is only listed once under
`[dependencies]`, leaving the existing `fhe-traits` dependency declaration
intact.
In `@crates/fhe-params/src/crp.rs`:
- Line 21: The call to CommonRandomPoly::new_deterministic currently passes
¶ms which yields a &&Arc<BfvParameters>; change the argument to pass params
directly so the signature receives the expected &Arc<BfvParameters> (i.e.,
replace CommonRandomPoly::new_deterministic(¶ms, seed).unwrap() with a call
that passes params, keeping seed the same) to remove the redundant
double-reference while preserving unwrap() behavior.
In `@crates/zk-helpers/src/circuits/threshold/pk_generation/utils.rs`:
- Around line 28-53: The generated Noir code places the closing "];" on the same
line as the last polynomial; update crp_matrix_constant_string to emit the
closing bracket on its own line by adjusting the final format string (used with
limb_strings.join) to append a newline before the closing "];" (ensure
indentation matches the existing "\n " prefix so the final output ends with
"\n];" on its own line); modify the format invocation that constructs the
returned string in crp_matrix_constant_string accordingly.
In `@crates/zk-helpers/src/circuits/threshold/share_decryption/sample.rs`:
- Around line 311-321: Rename the misleading test or make its inputs
deterministic: either rename test_generate_template_repeatable to
test_generate_template_consistent_structure to reflect it only asserts
structural equality, or modify the test (and/or
ShareDecryptionCircuitData::generate_sample) to seed all RNGs used (replace
OsRng/thread_rng usage or provide a SeedableRng) so that two calls to
ShareDecryptionCircuitData::generate_sample(PRESET, committee) produce identical
values; reference the test name test_generate_template_repeatable, the generator
function ShareDecryptionCircuitData::generate_sample, and RNG usages
OsRng/thread_rng (and the committee source CiphernodesCommitteeSize::Small) when
making the change.
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 (1)
circuits/bin/threshold/pk_generation/src/main.nr (1)
7-33:⚠️ Potential issue | 🔴 CriticalThe fixture file
crates/zk-prover/tests/fixtures/pk_generation.jsonmust be regenerated to match the updated circuit interface.Your review comment correctly identifies that removing
aas a public input constitutes a breaking circuit interface change. However, it overlooks a critical issue: the fixture file itself is now out of sync with the circuit code.The problem:
- Current circuit (
main.nr): 7 parameters withouta- Fixture file (
pk_generation.json): 8 parameters withaas public input (outdated ABI)Why this matters:
Integration tests incrates/tests/tests/integration.rsdepend on the fixture file to initialize the test backend. An outdated fixture ABI will cause test failures. Additionally, any off-chain infrastructure that parses this fixture will receive incorrect public input layout information.Required action:
Regenerate the fixture file by runningnargo compilein the circuit directory. The build scriptscripts/build-circuits.tsorchestrates this process. After regeneration, verify thatcrates/zk-prover/tests/fixtures/pk_generation.jsonreflects the new 7-parameter interface withouta.Include this regeneration step alongside the tooling updates you've already identified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/threshold/pk_generation/src/main.nr` around lines 7 - 33, The test fixture pk_generation.json is out of sync with the updated circuit main function (which now takes eek, sk, e_sm, r1is, r2is, pk0is, pk1is and no longer exposes a), so regenerate the fixture by running the circuit build (run `nargo compile` in the circuit directory or invoke the existing scripts/build-circuits.ts orchestration) to produce a new crates/zk-prover/tests/fixtures/pk_generation.json that reflects the 7-parameter ABI; after regeneration, verify the fixture no longer lists `a` as a public input and that integration tests (which read this fixture) pass.
🧹 Nitpick comments (4)
crates/fhe/src/lib.rs (1)
8-11:pub use runtime::*creates an open-ended public API surface.Any new public item added to
runtimewill be automatically re-exported. Consider switching to explicit re-exports once theruntimemodule stabilizes, to avoid unintended API growth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe/src/lib.rs` around lines 8 - 11, The current line pub use runtime::* re-exports every public item from the runtime module, which can unintentionally expand the crate's public API; replace this wildcard re-export with explicit re-exports of the stable symbols (e.g., list each function/type/struct/enum you intend to expose from runtime such as RuntimeType, init_runtime, RuntimeError, etc.) by exporting them by name instead of using pub use runtime::*, keeping the module private or internal until its API stabilizes and adding new names to the explicit list only when you intentionally want them public.crates/fhe-params/src/lib.rs (1)
11-21:unwrap()increate_deterministic_crp_from_seed(crp.rs lines 15–20) introduces a panic path in a public infallible function.
CommonRandomPoly::new_deterministic(...).unwrap()will panic on bad parameters. The function signature returnsCommonRandomPolywith noResult, so callers have no way to handle the error. This matches the establishedbuild_bfv_paramspanic pattern in this codebase (noted as a temporary solution), but as a new public API it's worth wrapping inResultor at minimum adding a debug-assert thatparamsis non-degenerate.Based on learnings, the team is aware of this pattern and plans to move to
Result-based error handling in a future batch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fhe-params/src/lib.rs` around lines 11 - 21, create_deterministic_crp_from_seed currently calls CommonRandomPoly::new_deterministic(...).unwrap(), exposing a panic in a public infallible API; change create_deterministic_crp_from_seed (in crp.rs) to return Result<CommonRandomPoly, E> (or a crate error type) and propagate the error from CommonRandomPoly::new_deterministic instead of unwrapping so callers can handle parameter failures; if you must keep an infallible signature short-term, replace unwrap() with a debug_assert! that validates the incoming params are non-degenerate and add a clear expect() message, but the preferred fix is to change the function signature to return a Result and propagate the underlying error.crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs (1)
248-339:ais shadowed at line 313; rename the assembled variable for clarity.The outer
a(line 248,CrtPolynomialfromdeterministic_crp_crt_polynomial) is used as the izip source. After the parallel computation, a newlet mut a = CrtPolynomial::new(vec![])at line 313 shadows it, and the result is assigned topk1is(line 339). The shadowing is correct but makes it non-obvious whichais in scope at each point.♻️ Suggested rename
- let mut a = CrtPolynomial::new(vec![]); + let mut crp_assembled = CrtPolynomial::new(vec![]); ... for (_i, r2i, r1i, pk0_sharei, ai, e_smi) in results { r2.add_limb(r2i); r1.add_limb(r1i); pk0_share.add_limb(pk0_sharei); - a.add_limb(ai); + crp_assembled.add_limb(ai); e_sm.add_limb(e_smi); } Ok(Inputs { ... - pk1is: a, + pk1is: crp_assembled, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs` around lines 248 - 339, The outer variable a returned from deterministic_crp_crt_polynomial is shadowed by a new let mut a = CrtPolynomial::new(...); rename the assembled CrtPolynomial to a distinct name (e.g., assembled_a or a_crt) to avoid confusion: update the declaration (let mut a -> let mut assembled_a), update where you call assembled_a.add_limb(ai) inside the loop, and assign assembled_a to Inputs.pk1is instead of the shadowing a; leave the izip source variable and its usage (the original a from deterministic_crp_crt_polynomial) unchanged.crates/zk-helpers/src/circuits/threshold/pk_generation/utils.rs (1)
50-53: Nit: closing];lands on the last element's line.The generated output will look like:
pub global CRP: [Polynomial<N>; L] = [ Polynomial::new([...]), Polynomial::new([...])];Consider adding a newline before the closing
];for consistency with typical Noir formatting.Optional formatting tweak
Ok(format!( - "pub global CRP: [Polynomial<N>; L] = [\n {}];", - limb_strings.join(",\n ") + "pub global CRP: [Polynomial<N>; L] = [\n {}\n];", + limb_strings.join(",\n "), ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/pk_generation/utils.rs` around lines 50 - 53, The generated Noir global CRP string ends the closing "];" on the same line as the last Polynomial element, so update the format in utils.rs that produces the string (the Ok(format!(...)) that uses limb_strings.join(",\n ")) to put the closing bracket on its own line (e.g. ensure the format appends "\n];" or otherwise adds a newline before the "];") so output looks like "[\n ..., \n];" for consistent Noir formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/zk-helpers/src/circuits/threshold/pk_generation/codegen.rs`:
- Line 57: The code calls preset.build_pair().unwrap() inside generate_configs
which panics instead of returning an error; change this to propagate the error
by replacing unwrap() with a call that maps PresetError into CircuitsErrors
(e.g., use preset.build_pair().map_err(|e| CircuitsErrors::from(e))? or
.map_err(|e| CircuitsErrors::Other(e.to_string()))?) so build_pair() returns
through generate_configs' Result; update imports if needed to convert
PresetError to CircuitsErrors and keep the existing ?-style error flow
consistent with the later call.
---
Outside diff comments:
In `@circuits/bin/threshold/pk_generation/src/main.nr`:
- Around line 7-33: The test fixture pk_generation.json is out of sync with the
updated circuit main function (which now takes eek, sk, e_sm, r1is, r2is, pk0is,
pk1is and no longer exposes a), so regenerate the fixture by running the circuit
build (run `nargo compile` in the circuit directory or invoke the existing
scripts/build-circuits.ts orchestration) to produce a new
crates/zk-prover/tests/fixtures/pk_generation.json that reflects the 7-parameter
ABI; after regeneration, verify the fixture no longer lists `a` as a public
input and that integration tests (which read this fixture) pass.
---
Duplicate comments:
In `@crates/fhe-params/Cargo.toml`:
- Around line 24-25: The dependency entry for "fhe-traits" under the
[dev-dependencies] table is redundant because "fhe-traits" is already declared
under [dependencies]; remove the duplicate "fhe-traits" line from the
[dev-dependencies] section in Cargo.toml so tests/benches use the existing
dependency declaration (look for the "fhe-traits" key in Cargo.toml and delete
the entry under [dev-dependencies]).
In `@crates/fhe-params/src/crp.rs`:
- Around line 1-27: The code currently uses unwrap() in
create_deterministic_crp_from_seed (calling CommonRandomPoly::new_deterministic)
which can panic; change create_deterministic_crp_from_seed to return a
Result<CommonRandomPoly, E> (propagate the exact error type returned by
new_deterministic) and use the ? operator instead of unwrap(), and update
create_deterministic_crp_from_default_seed to also return Result and call the
first function; if these public helpers may be unused in tests, restore
#[allow(dead_code)] on the affected functions to silence lints.
---
Nitpick comments:
In `@crates/fhe-params/src/lib.rs`:
- Around line 11-21: create_deterministic_crp_from_seed currently calls
CommonRandomPoly::new_deterministic(...).unwrap(), exposing a panic in a public
infallible API; change create_deterministic_crp_from_seed (in crp.rs) to return
Result<CommonRandomPoly, E> (or a crate error type) and propagate the error from
CommonRandomPoly::new_deterministic instead of unwrapping so callers can handle
parameter failures; if you must keep an infallible signature short-term, replace
unwrap() with a debug_assert! that validates the incoming params are
non-degenerate and add a clear expect() message, but the preferred fix is to
change the function signature to return a Result and propagate the underlying
error.
In `@crates/fhe/src/lib.rs`:
- Around line 8-11: The current line pub use runtime::* re-exports every public
item from the runtime module, which can unintentionally expand the crate's
public API; replace this wildcard re-export with explicit re-exports of the
stable symbols (e.g., list each function/type/struct/enum you intend to expose
from runtime such as RuntimeType, init_runtime, RuntimeError, etc.) by exporting
them by name instead of using pub use runtime::*, keeping the module private or
internal until its API stabilizes and adding new names to the explicit list only
when you intentionally want them public.
In `@crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs`:
- Around line 248-339: The outer variable a returned from
deterministic_crp_crt_polynomial is shadowed by a new let mut a =
CrtPolynomial::new(...); rename the assembled CrtPolynomial to a distinct name
(e.g., assembled_a or a_crt) to avoid confusion: update the declaration (let mut
a -> let mut assembled_a), update where you call assembled_a.add_limb(ai) inside
the loop, and assign assembled_a to Inputs.pk1is instead of the shadowing a;
leave the izip source variable and its usage (the original a from
deterministic_crp_crt_polynomial) unchanged.
In `@crates/zk-helpers/src/circuits/threshold/pk_generation/utils.rs`:
- Around line 50-53: The generated Noir global CRP string ends the closing "];"
on the same line as the last Polynomial element, so update the format in
utils.rs that produces the string (the Ok(format!(...)) that uses
limb_strings.join(",\n ")) to put the closing bracket on its own line (e.g.
ensure the format appends "\n];" or otherwise adds a newline before the "];") so
output looks like "[\n ..., \n];" for consistent Noir formatting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/test-helpers/src/lib.rs (1)
89-89: Unnecessary.clone()oncrp_bytes.
CommonRandomPoly::deserializetakes&[u8];&crp_bytescoerces directly without cloning.♻️ Proposed fix
- let crpoly = CommonRandomPoly::deserialize(&crp_bytes.clone(), ¶ms)?; + let crpoly = CommonRandomPoly::deserialize(&crp_bytes, ¶ms)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/test-helpers/src/lib.rs` at line 89, The call to CommonRandomPoly::deserialize is cloning crp_bytes unnecessarily; change the argument from &crp_bytes.clone() to a borrowed slice like &crp_bytes or crp_bytes.as_slice() so you pass a &[u8] directly to CommonRandomPoly::deserialize and avoid the pointless allocation.circuits/benchmarks/scripts/run_benchmarks.sh (2)
135-135: Optional: replace embedded literal-tab parsing withIFS=$'\t' read.The jq expression emits
\tas a separator (line 135), and lines 138–139 parse it back using${line%% *}/${line#* }with literal tab characters embedded in the source. This works correctly today, but is silently broken if any editor auto-converts tabs to spaces.♻️ Proposed refactor
while IFS= read -r line; do [ -z "$line" ] && continue - c="${line%% *}" - modes="${line#* }" + IFS=$'\t' read -r c modes <<< "$line"Also applies to: 138-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/run_benchmarks.sh` at line 135, CIRCUIT_MODES is built with a tab separator but the downstream parsing uses literal-tab parameter expansion (`${line%% *}` / `${line#* }`) which breaks if editors convert tabs; change the parsing to use IFS=$'\t' read -r to split each line (e.g., in the loop that iterates CIRCUIT_MODES) so you explicitly split into variables like path and modes without embedding literal tab characters—replace the `${line%%...}`/`${line#...}` usage with a read-based split using IFS=$'\t' and handle empty/missing modes as before.
151-158: Migrate hard-coded circuit mode checks toconfig.jsonfor consistency.The script at lines 151–158 enforces mode restrictions for
threshold/decrypted_shares_aggregation_mod(insecure-only) andthreshold/decrypted_shares_aggregation_bn(secure-only) via hard-codedifblocks. However, the script already reads mode restrictions fromconfig.jsonfor other circuits (e.g.,"config"has"modes": ["secure"]). These two circuits should be defined as objects inconfig.jsonto unify the approach:{ "name": "threshold/decrypted_shares_aggregation_mod", "modes": ["insecure"] }, { "name": "threshold/decrypted_shares_aggregation_bn", "modes": ["secure"] }Once added, lines 151–158 become dead code and can be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/run_benchmarks.sh` around lines 151 - 158, The two hard-coded mode checks in run_benchmarks.sh (the if blocks that check MODE and the variable c for "threshold/decrypted_shares_aggregation_mod" and "threshold/decrypted_shares_aggregation_bn") should be removed and the mode restrictions migrated to config.json: add objects { "name": "threshold/decrypted_shares_aggregation_mod", "modes": ["insecure"] } and { "name": "threshold/decrypted_shares_aggregation_bn", "modes": ["secure"] } to the existing circuits list so the existing config-based gating logic handles them; after updating config.json, delete the conditional blocks that reference MODE and c in run_benchmarks.sh to avoid duplicate/dead checks.crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs (1)
310-339: Optional: eliminate redundant CRP re-accumulation and variable shadowing.The outer
aat line 248 (CrtPolynomial) is fully equivalent to the re-accumulated inneraat line 313–330: the closure passes each limb through unmodified (a.clone(), line 304), results are sorted to restore original order (line 308), and the accumulatedCrtPolynomialis bit-for-bit identical to the original. This creates three nested shadows ofa(lines 248, 277, 313).Simplify by removing
a/aifrom the parallel pipeline and using the outeradirectly forpk1is:♻️ Proposed refactor
let mut results: Vec<( usize, Polynomial, Polynomial, Polynomial, - Polynomial, Polynomial, )> = izip!( moduli.clone(), data.pk0_share.limbs.clone(), - a.limbs.clone(), data.eek.limbs.clone(), data.e_sm.limbs.clone(), data.sk.limbs.clone(), ) .enumerate() .par_bridge() - .map(|(i, (qi, mut pk0_share, a, mut eek, mut e_sm, mut sk))| { + .map(|(i, (qi, mut pk0_share, mut eek, mut e_sm, mut sk))| { + let a_limb = a.limbs[i].clone(); // borrow outer `a` pk0_share.reverse(); pk0_share.center(&qi); ... let pk0_share_hat = { - let mut exp = a.neg(); + let mut exp = a_limb.neg(); ... }; ... - (i, r2, r1, pk0_share.clone(), a.clone(), e_sm.clone()) + (i, r2, r1, pk0_share.clone(), e_sm.clone()) }) .collect(); results.sort_by_key(|(i, _, _, _, _)| *i); let mut r2 = CrtPolynomial::new(vec![]); let mut r1 = CrtPolynomial::new(vec![]); let mut pk0_share = CrtPolynomial::new(vec![]); - let mut a = CrtPolynomial::new(vec![]); // shadowed name removed let mut e_sm = CrtPolynomial::new(vec![]); ... - for (_i, r2i, r1i, pk0_sharei, ai, e_smi) in results { + for (_i, r2i, r1i, pk0_sharei, e_smi) in results { r2.add_limb(r2i); r1.add_limb(r1i); pk0_share.add_limb(pk0_sharei); - a.add_limb(ai); e_sm.add_limb(e_smi); } Ok(Inputs { ... - pk1is: a, + pk1is: a, // outer `a` from line 248, used directly })Note: the
par_bridge()+ index capture pattern would need the outerato beArc-wrapped or the limb accessed via index before entering the closure. The simplest alternative is to keep the indexiin scope and index intoa.limbs[i]after sorting, rather than threading it through the parallel map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs` around lines 310 - 339, The re-accumulation and shadowing of the CrtPolynomial named `a` is redundant: stop producing/passing the `ai` limb through the parallel pipeline and remove the inner `a`/`ai` variable and its `CrtPolynomial::new` + `add_limb` calls; instead preserve the outer `a` and, after you collect and sort `results`, build `pk1is` by indexing into the outer `a.limbs` in the original order (or wrap the outer `a` in an Arc if you need shared access inside the parallel closure), then remove `ai` from the tuple returned by the closure and from the loop that currently calls `a.add_limb(ai)` so `pk1is` is assigned from the original `a` when constructing `Inputs`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@circuits/benchmarks/scripts/run_benchmarks.sh`:
- Line 135: The jq expression that builds CIRCUIT_MODES must detect an explicit
empty "modes": [] and emit a sentinel so downstream logic can warn; change the
jq pipeline in the CIRCUIT_MODES assignment (the line producing
"\($path)\t\(.)") to branch on (.modes | length == 0) and emit a special marker
(e.g. "EMPTY_MODES") instead of producing an empty string, and then update the
shell handling where $modes is checked (the guard using [ -n "$modes" ] around
the mode-restriction block) to detect that sentinel: print a warning if $modes
== "EMPTY_MODES" and then treat it as unrestricted by clearing $modes (or skip
execution if you prefer), otherwise proceed with the existing gating logic. This
touches the CIRCUIT_MODES assignment and the mode-check block that references
$modes.
---
Duplicate comments:
In `@crates/fhe-params/Cargo.toml`:
- Around line 24-25: Remove the redundant dev-dependency entry for the crate by
deleting the fhe-traits line under the [dev-dependencies] table in Cargo.toml;
keep the existing fhe-traits entry under [dependencies] (so
tests/benches/examples still get it) and ensure there are no other duplicate
fhe-traits declarations elsewhere in this manifest.
In `@crates/fhe-params/src/crp.rs`:
- Around line 16-21: The function create_deterministic_crp_from_seed currently
calls CommonRandomPoly::new_deterministic(...).unwrap() which causes a panic on
failure; change create_deterministic_crp_from_seed to return a
Result<CommonRandomPoly, E> (use the appropriate error type returned by
CommonRandomPoly::new_deterministic) and propagate the error instead of
unwrapping, and update create_deterministic_crp_from_default_seed to call the
new fallible create_deterministic_crp_from_seed and propagate or map the error
as needed so no public API panics.
In `@crates/fhe/src/runtime.rs`:
- Around line 50-55: from_encoded currently calls
decode_bfv_params_arc(...).expect(...), which will panic instead of returning an
error; change it to propagate the decoding error through the Result return
(e.g., replace the expect call with an expression that returns Err on failure
using the ? operator or map_err to convert the decode error into the function's
Result error type), so decode_bfv_params_arc(bytes) -> params is handled without
panicking before calling create_deterministic_crp_from_default_seed and
Fhe::new.
In `@crates/zk-helpers/src/circuits/threshold/pk_generation/codegen.rs`:
- Line 57: Replace the pre-existing unwrap on preset.build_pair() with error
propagation consistent with generate_configs' Result return: call
preset.build_pair().map_err(|e| CircuitsErrors::Something(e))? (or the
appropriate CircuitsErrors variant) so the failure is propagated instead of
panicking; update the binding `let (threshold_params, _) =
preset.build_pair().unwrap();` to use map_err + ? and keep the rest of
generate_configs unchanged.
---
Nitpick comments:
In `@circuits/benchmarks/scripts/run_benchmarks.sh`:
- Line 135: CIRCUIT_MODES is built with a tab separator but the downstream
parsing uses literal-tab parameter expansion (`${line%% *}` / `${line#* }`)
which breaks if editors convert tabs; change the parsing to use IFS=$'\t' read
-r to split each line (e.g., in the loop that iterates CIRCUIT_MODES) so you
explicitly split into variables like path and modes without embedding literal
tab characters—replace the `${line%%...}`/`${line#...}` usage with a read-based
split using IFS=$'\t' and handle empty/missing modes as before.
- Around line 151-158: The two hard-coded mode checks in run_benchmarks.sh (the
if blocks that check MODE and the variable c for
"threshold/decrypted_shares_aggregation_mod" and
"threshold/decrypted_shares_aggregation_bn") should be removed and the mode
restrictions migrated to config.json: add objects { "name":
"threshold/decrypted_shares_aggregation_mod", "modes": ["insecure"] } and {
"name": "threshold/decrypted_shares_aggregation_bn", "modes": ["secure"] } to
the existing circuits list so the existing config-based gating logic handles
them; after updating config.json, delete the conditional blocks that reference
MODE and c in run_benchmarks.sh to avoid duplicate/dead checks.
In `@crates/test-helpers/src/lib.rs`:
- Line 89: The call to CommonRandomPoly::deserialize is cloning crp_bytes
unnecessarily; change the argument from &crp_bytes.clone() to a borrowed slice
like &crp_bytes or crp_bytes.as_slice() so you pass a &[u8] directly to
CommonRandomPoly::deserialize and avoid the pointless allocation.
In `@crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs`:
- Around line 310-339: The re-accumulation and shadowing of the CrtPolynomial
named `a` is redundant: stop producing/passing the `ai` limb through the
parallel pipeline and remove the inner `a`/`ai` variable and its
`CrtPolynomial::new` + `add_limb` calls; instead preserve the outer `a` and,
after you collect and sort `results`, build `pk1is` by indexing into the outer
`a.limbs` in the original order (or wrap the outer `a` in an Arc if you need
shared access inside the parallel closure), then remove `ai` from the tuple
returned by the closure and from the loop that currently calls `a.add_limb(ai)`
so `pk1is` is assigned from the original `a` when constructing `Inputs`.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/fhe-params/Cargo.toml`:
- Around line 12-23: The crate exports deterministic CRP APIs
(create_deterministic_crp_from_default_seed) that require RNG at runtime, so
move rand and rand_chacha from dev-only to normal dependencies: ensure the
Cargo.toml lists rand = { workspace = true } and rand_chacha = { workspace =
true } under [dependencies] (not under [dev-dependencies]), leaving fhe-traits
only in [dev-dependencies]; update the manifest accordingly so the runtime RNG
crates are available to consumers of the library.
cedoor
previously approved these changes
Feb 19, 2026
Contributor
Author
|
waiting for #1319 to avoid fixture updates |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ctrlc03
approved these changes
Feb 20, 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
4 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.
crptofhe-paramsa(CRP) to configs (throughpk_generationupdate)Summary by CodeRabbit
New Features
Behavior Changes
Breaking Changes
Documentation