refactor: update search algo to latest approach [skip-line-limit]#1600
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe BFV parameter search is overhauled: the ChangesBFV Parameter Search Overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
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/src/search/bfv.rs (1)
743-758:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate candidates in ascending
qbits, not just the smallest one.
bestis chosen beforefinalize_second_param; if the smallest candidate fails correctness/margin, this returnsNoneeven when a slightly larger tried selection would pass. Sort byqbitsand return the first finalized candidate.Proposed candidate validation order
- // Prefer the smallest qualifying primes (minimise prime size), matching the - // old behaviour of taking the smallest valid primes in the smallest bucket. - let mut best: Option<(f64, Vec<PrimeItem>)> = None; - for sel in tried { + // Prefer the smallest passing selection. + let mut candidates: Vec<(f64, Vec<PrimeItem>)> = Vec::new(); + for sel in tried { let q = product(sel.iter().map(|pi| pi.value.clone())); let qbits = log2_big(&q); - if let Some((best_qbits, _)) = &best { - if qbits < *best_qbits { - best = Some((qbits, sel)); - } - } else { - best = Some((qbits, sel)); - } + candidates.push((qbits, sel)); } - if let Some((_, sel)) = best { - return finalize_second_param(bfv_search_config, d, sel.clone(), k_plain); + candidates.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap_or(std::cmp::Ordering::Equal)); + + for (_, sel) in candidates { + if let Some(res) = finalize_second_param(bfv_search_config, d, sel, k_plain) { + return Some(res); + } } None }🤖 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 743 - 758, The current code selects only the single candidate with the smallest qbits value and immediately calls finalize_second_param on it, returning None if that candidate fails validation even though larger candidates in the tried set might pass. Instead, sort the tried selections by their computed qbits values in ascending order, then iterate through them sequentially, calling finalize_second_param on each candidate and returning the first one that succeeds (does not return None). If all candidates fail validation, then return None.crates/fhe-params/src/bin/search_params.rs (2)
21-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the CLI help range to match the new prime pool.
Line 23 still advertises
40..63 bits, but this refactor’s prime builder now targets the narrowed50..60-bit pool. The--helptext should not describe an unavailable search range.Proposed fix
#[command( version, - about = "Search BFV params with NTT-friendly CRT primes (40..63 bits)" + about = "Search BFV params with NTT-friendly CRT primes (50..60 bits)" )]🤖 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/bin/search_params.rs` around lines 21 - 24, The `about` field in the `#[command(...)]` macro on line 23 advertises a search range of `40..63 bits`, but the refactored prime builder now targets a narrowed `50..60` bit pool. Update the help text string in the `about` attribute to accurately reflect the new range of `50..60 bits` instead of the outdated `40..63 bits` range.
69-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep both Uniform variance helpers on the same formula.
The new big-int path uses
B²/3, butvariance_uniform_strstill reportsB*(B+1)/3, so the final output can show inconsistent variance formulas forBvsB_Enc.Proposed fix
fn variance_uniform_str(b: u128) -> String { let b_big = BigUint::from(b); - let var = (&b_big * (b + 1)) / 3u32; - var.to_str_radix(10) + variance_uniform_big_str(&b_big) } fn variance_uniform_big_str(b: &BigUint) -> String { // Variance for Uniform(-B..B): Var = B^2 / 3🤖 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/bin/search_params.rs` around lines 69 - 78, The two variance computation functions variance_uniform_str and variance_uniform_big_str are using different formulas. The variance_uniform_big_str function correctly uses B²/3 for uniform variance, but variance_uniform_str uses B*(B+1)/3 instead. Update variance_uniform_str to use the same formula as variance_uniform_big_str by changing the variance calculation from (b_big * (b + 1)) / 3u32 to (b_big * b_big) / 3u32 to ensure both functions report consistent variance formulas.
🧹 Nitpick comments (3)
crates/fhe-params/src/search/prime.rs (1)
100-109: ⚡ Quick winAssert the new first-set bit range in the regression test.
build_prime_items()now promises only50..=60bit primes, but the test still only rejects61/62/63. Add the lower-bound assertion so the new correctness-margin contract cannot regress silently.Proposed test tightening
- // Verify no 61, 62, or 63-bit primes are included + // Verify first-set primes are restricted to 50..=60 bits. for item in &items { - assert_ne!(item.bitlen, 61); - assert_ne!(item.bitlen, 62); - assert_ne!(item.bitlen, 63); + assert!( + (50..=60).contains(&item.bitlen), + "unexpected first-set prime bit length: {}", + item.bitlen + ); }🤖 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/prime.rs` around lines 100 - 109, The regression test test_build_prime_items() currently only verifies the upper bounds by asserting that bitlen is not 61, 62, or 63, but does not validate the lower bound. Since build_prime_items() now promises only 50..=60 bit primes, add assertions to the test loop to verify that each item has a bitlen greater than or equal to 50. This ensures the lower-bound contract cannot regress silently.crates/fhe-params/src/search/bfv.rs (1)
96-104: ⚡ Quick winRefresh the public
bfv_searchdocs for the fixed-d bucket scan.The docs still describe degree iteration, Eq4 max-
qselection, and refinement, but the implementation now fixesd = RING_DIMand scans bit buckets. That stale contract is misleading for callers tuning parameters.🤖 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 96 - 104, Update the documentation comment for the `bfv_search` function to accurately reflect the current implementation. Replace the description that mentions iterating through polynomial degrees d and Eq4 max-q selection with documentation that correctly describes the fixed-d bucket scan approach. The docs should clearly state that the function now fixes d to RING_DIM and scans through bit buckets to find optimal parameters, rather than the old degree iteration and refinement process described in the current comment block.crates/fhe-params/src/search/utils.rs (1)
88-96: ⚡ Quick winCover byte-boundary cases for
log2_big.The new approximation drives margin checks, but the test only checks positivity/order. Add exact power-of-two and >8-byte cases so future changes don’t skew
q/margin decisions near bucket boundaries.Proposed regression cases
assert!(log2_big(&BigUint::from(256u64)) > 0.0); assert!(log2_big(&BigUint::from(1024u64)) > 0.0); assert!(log2_big(&BigUint::from(1024u64)) > log2_big(&BigUint::from(256u64))); + + assert_eq!(log2_big(&(BigUint::one() << 64u32)), 64.0); + let x = (BigUint::one() << 80u32) + BigUint::from(123u64); + assert!((log2_big(&x) - 80.0).abs() < 1e-12);🤖 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/utils.rs` around lines 88 - 96, The test_log2_big function only validates basic behavior (positivity and ordering) but lacks coverage for exact power-of-two cases and large byte-boundary scenarios. Expand the test by adding assertions for specific power-of-two values (such as 2^8, 2^16, 2^32, 2^64) to verify precise calculations at key boundaries, and include test cases with large BigUint values that exceed 8 bytes to ensure the approximation remains accurate for oversized inputs. This ensures future modifications to the log2_big function do not inadvertently affect margin calculations and bucket boundary decisions.
🤖 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/fhe-params/src/bin/search_params.rs`:
- Around line 234-237: The error message in the bfv_search error handler on line
236 suggests users "increase d", but the CLI no longer exposes ring dimension
(d) as a tunable parameter. Remove the "increase d" suggestion from the
eprintln! statement and keep only the parameters that are actually exposed in
the CLI for users to adjust.
- Around line 52-54: The `min_margin` field in the CLI argument parser accepts
any f64 value without validation, but it is used to gate candidate acceptance in
the search logic. Add validation after the argument is parsed to reject negative
and non-finite values (such as infinity and NaN) before passing `min_margin`
into `BfvSearchConfig`. Implement this check before the search is executed to
ensure only valid margin values reach the search logic that depends on it.
In `@crates/fhe-params/src/search/bfv.rs`:
- Around line 126-128: The code in bfv.rs computes `log2_q_limit` as a security
limit based on correctness margins, but currently only logs this value without
enforcing it as a filter. You need to add validation logic at all locations
where `log2_q_limit` is computed (at lines 126-128, 181-197, 499-511, and
587-601) to reject any candidate where the logarithm of q exceeds the computed
security limit. This means before a candidate is returned or accepted, check
that its log2(q) value does not exceed log2_q_limit, and skip or reject the
candidate if this security constraint is violated.
- Around line 567-601: The current implementation takes only the smallest valid
primes using `.take(num_primes)` and if the `finalize_second_param()` call
returns None, it immediately continues to the next bucket without trying larger
valid primes from the same bucket. To fix this, wrap the section from where
`sel` is created through the `finalize_second_param()` call in a loop that
iterates through different selections of primes from the `valid` vector. Instead
of just taking the first `num_primes`, try multiple consecutive selections of
`num_primes` primes starting from different offsets in the sorted `valid`
vector, so that larger candidates from the same bucket are tested before
abandoning the bucket entirely.
- Around line 58-59: Add input validation for the min_margin field in
search_params.rs before BfvSearchConfig construction (lines 204–213). Insert a
validation check that ensures min_margin.is_finite() && min_margin >= 0.0,
following the same validation pattern already used for z and k parameters in the
preceding lines (187–202). This prevents non-finite and negative min_margin
values from bypassing the margin gate comparisons that occur later at lines 314
and 802.
In `@crates/fhe-params/src/search/constants.rs`:
- Around line 8-465: The NTT_PRIMES_BY_BITS constant contains 51 composite
(non-prime) entries that will cause silent failures in parameter generation,
with high concentrations in the 40–48 bit ranges (40–50% composite rates) and
all entries being valid primes in the 49–62 bit ranges. Replace all composite
entries in the tuples for bit-lengths 40–48 with validated prime numbers that
satisfy the NTT-compatibility requirement (p % 16384 == 1) and their declared
bit-length, ensuring all entries in each bit-length group are actual primes
before merge.
---
Outside diff comments:
In `@crates/fhe-params/src/bin/search_params.rs`:
- Around line 21-24: The `about` field in the `#[command(...)]` macro on line 23
advertises a search range of `40..63 bits`, but the refactored prime builder now
targets a narrowed `50..60` bit pool. Update the help text string in the `about`
attribute to accurately reflect the new range of `50..60 bits` instead of the
outdated `40..63 bits` range.
- Around line 69-78: The two variance computation functions variance_uniform_str
and variance_uniform_big_str are using different formulas. The
variance_uniform_big_str function correctly uses B²/3 for uniform variance, but
variance_uniform_str uses B*(B+1)/3 instead. Update variance_uniform_str to use
the same formula as variance_uniform_big_str by changing the variance
calculation from (b_big * (b + 1)) / 3u32 to (b_big * b_big) / 3u32 to ensure
both functions report consistent variance formulas.
In `@crates/fhe-params/src/search/bfv.rs`:
- Around line 743-758: The current code selects only the single candidate with
the smallest qbits value and immediately calls finalize_second_param on it,
returning None if that candidate fails validation even though larger candidates
in the tried set might pass. Instead, sort the tried selections by their
computed qbits values in ascending order, then iterate through them
sequentially, calling finalize_second_param on each candidate and returning the
first one that succeeds (does not return None). If all candidates fail
validation, then return None.
---
Nitpick comments:
In `@crates/fhe-params/src/search/bfv.rs`:
- Around line 96-104: Update the documentation comment for the `bfv_search`
function to accurately reflect the current implementation. Replace the
description that mentions iterating through polynomial degrees d and Eq4 max-q
selection with documentation that correctly describes the fixed-d bucket scan
approach. The docs should clearly state that the function now fixes d to
RING_DIM and scans through bit buckets to find optimal parameters, rather than
the old degree iteration and refinement process described in the current comment
block.
In `@crates/fhe-params/src/search/prime.rs`:
- Around line 100-109: The regression test test_build_prime_items() currently
only verifies the upper bounds by asserting that bitlen is not 61, 62, or 63,
but does not validate the lower bound. Since build_prime_items() now promises
only 50..=60 bit primes, add assertions to the test loop to verify that each
item has a bitlen greater than or equal to 50. This ensures the lower-bound
contract cannot regress silently.
In `@crates/fhe-params/src/search/utils.rs`:
- Around line 88-96: The test_log2_big function only validates basic behavior
(positivity and ordering) but lacks coverage for exact power-of-two cases and
large byte-boundary scenarios. Expand the test by adding assertions for specific
power-of-two values (such as 2^8, 2^16, 2^32, 2^64) to verify precise
calculations at key boundaries, and include test cases with large BigUint values
that exceed 8 bytes to ensure the approximation remains accurate for oversized
inputs. This ensures future modifications to the log2_big function do not
inadvertently affect margin calculations and bucket boundary decisions.
🪄 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: 1321bba5-4625-434a-93a5-22a9febd774c
📒 Files selected for processing (5)
crates/fhe-params/src/bin/search_params.rscrates/fhe-params/src/search/bfv.rscrates/fhe-params/src/search/constants.rscrates/fhe-params/src/search/prime.rscrates/fhe-params/src/search/utils.rs
zahrajavar
previously approved these changes
Jun 15, 2026
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 (3)
crates/fhe-params/src/search/constants.rs (1)
7-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the table doc to match variable-length buckets.
Line 7 still says “6 per size”, but the new
&[&str]buckets are variable-length and several buckets contain far more than six entries.Proposed wording update
-/// NTT-friendly primes by bit-length (40..63), 6 per size +/// NTT-friendly primes by bit-length (40..63), variable count per size. pub const NTT_PRIMES_BY_BITS: &[(u8, &[&str])] = &[🤖 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/constants.rs` around lines 7 - 8, Update the documentation comment for the NTT_PRIMES_BY_BITS constant to accurately reflect that the buckets contain variable-length entries rather than a fixed count of 6 per size. Replace the phrase "6 per size" with language that describes the variable-length nature of the buckets in the data structure.crates/fhe-params/src/search/bfv.rs (2)
785-800:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChoose the smallest passing second-set candidate, not just the smallest tried candidate.
The current code picks the lowest
qbitsfromtriedand finalizes only that one. If the smallest candidate fails the gap, correctness, or margin checks, a larger tried candidate can still be valid but is never tested.Proposed candidate selection fix
- // Prefer the smallest qualifying primes (minimise prime size), matching the - // old behaviour of taking the smallest valid primes in the smallest bucket. - let mut best: Option<(f64, Vec<PrimeItem>)> = None; + // Prefer the smallest candidate that actually passes validation. + let mut best: Option<(f64, BfvSearchResult)> = None; for sel in tried { let q = product(sel.iter().map(|pi| pi.value.clone())); let qbits = log2_big(&q); - if let Some((best_qbits, _)) = &best { - if qbits < *best_qbits { - best = Some((qbits, sel)); + if let Some(res) = finalize_second_param(bfv_search_config, d, sel, k_plain) { + if let Some((best_qbits, _)) = &best { + if qbits < *best_qbits { + best = Some((qbits, res)); + } + } else { + best = Some((qbits, res)); } - } else { - best = Some((qbits, sel)); } } - if let Some((_, sel)) = best { - return finalize_second_param(bfv_search_config, d, sel.clone(), k_plain); - } - None + best.map(|(_, res)| res)🤖 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 785 - 800, The current code in the loop around line 785-800 selects the single candidate with the smallest qbits from the `tried` set and immediately returns after calling finalize_second_param on it. If that smallest candidate fails internal validation checks within finalize_second_param (such as gap, correctness, or margin checks), no fallback to larger candidates occurs. Instead of picking one best candidate and returning immediately, sort the candidates in `tried` by qbits in ascending order, then iterate through them in that order, calling finalize_second_param on each candidate until one succeeds and returns a valid result. Only if all candidates fail should the function return without a solution. This ensures the smallest valid candidate is selected rather than just the smallest tried candidate.
430-459:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid duplicating a CRT prime when floor and ceil buckets are the same.
When
floor_r == ceil_r, the mixed selection loop takes from the same bucket twice, so candidates like[p0, p0]can be pushed intotried. The finalizers do not reject duplicateqi, so this can produce an invalid CRT modulus.Proposed shape for both constructors
- for k in 0..=s { - let take_ceil = k; - let take_floor = s - k; - let mut sel: Vec<PrimeItem> = Vec::new(); - if take_floor > 0 { - if let Some(b) = by_bits_small.get(&floor_r) { - if b.len() < take_floor { - continue; - } - sel.extend(b.iter().take(take_floor).cloned()); - } else { - continue; - } - } - if take_ceil > 0 { - if let Some(b) = by_bits_small.get(&ceil_r) { - if b.len() < take_ceil { - continue; - } - sel.extend(b.iter().take(take_ceil).cloned()); - } else { - continue; - } - } - if sel.len() == s { - tried.push(sel); - } + if floor_r == ceil_r { + if let Some(b) = by_bits_small.get(&floor_r) { + if b.len() >= s { + tried.push(b.iter().take(s).cloned().collect()); + } + } + } else { + for k in 0..=s { + let take_ceil = k; + let take_floor = s - k; + let mut sel: Vec<PrimeItem> = Vec::new(); + if take_floor > 0 { + if let Some(b) = by_bits_small.get(&floor_r) { + if b.len() < take_floor { + continue; + } + sel.extend(b.iter().take(take_floor).cloned()); + } else { + continue; + } + } + if take_ceil > 0 { + if let Some(b) = by_bits_small.get(&ceil_r) { + if b.len() < take_ceil { + continue; + } + sel.extend(b.iter().take(take_ceil).cloned()); + } else { + continue; + } + } + if sel.len() == s { + tried.push(sel); + } + } }Also applies to: 745-772
🤖 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 430 - 459, The loop that builds candidate selections by mixing floor and ceil buckets can produce invalid CRT moduli with duplicate primes when floor_r equals ceil_r, because both the take_floor and take_ceil selections pull from the same bucket. Add a check at the beginning of the loop (before the if statements for take_floor and take_ceil) to skip the iteration when floor_r == ceil_r. This same fix must also be applied to the analogous loop structure in the second location (the parallel code that handles the identical selection logic).
🤖 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/fhe-params/src/search/bfv.rs`:
- Around line 147-152: The prime pools used in the BFV search are validated only
for specific ring dimensions but the search can now return larger dimensions
like 16384 or 32768. Filter the first-set and second-set prime pools based on
the current ring dimension value d before selecting candidates, ensuring that
each prime p satisfies the NTT constraint p % (2 * d) == 1 for the requested
dimension. Additionally, update the table validator in constants.rs to validate
that all primes in the pools meet this constraint for every dimension that
bfv_search may return, not just the base dimension.
- Around line 186-210: The current implementation skips the entire bucket with a
`continue` statement when the largest `num_primes` primes selection exceeds the
`log2_q_limit` security cap. However, smaller windows within the same bucket may
still satisfy both the security limit and the correctness lower bound. Instead
of immediately continuing to the next bucket when `q_bits > log2_q_limit`,
introduce a loop that progressively tries smaller windows (fewer primes) from
the same bucket by reducing the window size, retesting the q_bits constraint
against `log2_q_limit`, and checking the lower bound constraint against
`min_log2_q` for each smaller window before eventually moving to the next bucket
if no valid selection is found.
---
Outside diff comments:
In `@crates/fhe-params/src/search/bfv.rs`:
- Around line 785-800: The current code in the loop around line 785-800 selects
the single candidate with the smallest qbits from the `tried` set and
immediately returns after calling finalize_second_param on it. If that smallest
candidate fails internal validation checks within finalize_second_param (such as
gap, correctness, or margin checks), no fallback to larger candidates occurs.
Instead of picking one best candidate and returning immediately, sort the
candidates in `tried` by qbits in ascending order, then iterate through them in
that order, calling finalize_second_param on each candidate until one succeeds
and returns a valid result. Only if all candidates fail should the function
return without a solution. This ensures the smallest valid candidate is selected
rather than just the smallest tried candidate.
- Around line 430-459: The loop that builds candidate selections by mixing floor
and ceil buckets can produce invalid CRT moduli with duplicate primes when
floor_r equals ceil_r, because both the take_floor and take_ceil selections pull
from the same bucket. Add a check at the beginning of the loop (before the if
statements for take_floor and take_ceil) to skip the iteration when floor_r ==
ceil_r. This same fix must also be applied to the analogous loop structure in
the second location (the parallel code that handles the identical selection
logic).
In `@crates/fhe-params/src/search/constants.rs`:
- Around line 7-8: Update the documentation comment for the NTT_PRIMES_BY_BITS
constant to accurately reflect that the buckets contain variable-length entries
rather than a fixed count of 6 per size. Replace the phrase "6 per size" with
language that describes the variable-length nature of the buckets in the data
structure.
🪄 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: 2a5af242-13c6-41d6-9868-3981002b9134
📒 Files selected for processing (3)
crates/fhe-params/src/bin/search_params.rscrates/fhe-params/src/search/bfv.rscrates/fhe-params/src/search/constants.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/fhe-params/src/bin/search_params.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/fhe-params/src/search/prime.rs (1)
51-56: ⚡ Quick winLock the new 50-bit lower bound in tests.
build_prime_items()now enforces 50..=60 bits, buttest_build_prime_itemsonly asserts exclusion of 61/62/63. Please add an explicitbitlen >= 50assertion so this contract can’t regress silently.Suggested test tightening
@@ for item in &items { + assert!(item.bitlen >= 50); + assert!(item.bitlen <= 60); assert_ne!(item.bitlen, 61); assert_ne!(item.bitlen, 62); assert_ne!(item.bitlen, 63); }🤖 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/prime.rs` around lines 51 - 56, The test_build_prime_items test function only verifies that primes with 61, 62, and 63 bits are excluded, but does not verify the new 50-bit lower bound enforced by build_prime_items(). Add an explicit assertion in test_build_prime_items to verify that all returned prime items have a bitlen of at least 50, ensuring the lower bound contract cannot regress silently in future changes.
🤖 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/fhe-params/src/search/prime.rs`:
- Around line 70-72: The is_ntt_friendly function lacks input validation and can
panic when d is zero (causing modulo by zero) or miscompute when d is extremely
large (due to unchecked overflow in 2 * d). Add explicit input validation at the
start of the is_ntt_friendly function to check that d is greater than zero and
that 2 * d does not overflow a u64. Return a safe default or handle invalid
cases explicitly to prevent panics and silent errors, making the function's
behavior safe and predictable for all inputs.
---
Nitpick comments:
In `@crates/fhe-params/src/search/prime.rs`:
- Around line 51-56: The test_build_prime_items test function only verifies that
primes with 61, 62, and 63 bits are excluded, but does not verify the new 50-bit
lower bound enforced by build_prime_items(). Add an explicit assertion in
test_build_prime_items to verify that all returned prime items have a bitlen of
at least 50, ensuring the lower bound contract cannot regress silently in future
changes.
🪄 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: 097b7c35-9694-48d2-ab15-569d1ca93b79
📒 Files selected for processing (5)
crates/fhe-params/src/bin/search_params.rscrates/fhe-params/src/search/bfv.rscrates/fhe-params/src/search/constants.rscrates/fhe-params/src/search/prime.rscrates/fhe-params/src/search/utils.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/fhe-params/src/search/utils.rs
- crates/fhe-params/src/search/constants.rs
- crates/fhe-params/src/search/bfv.rs
- crates/fhe-params/src/bin/search_params.rs
ctrlc03
approved these changes
Jun 16, 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.
mirrors latest in https://github.com/gnosisguild/enclave-research/tree/3-moduli
Summary by CodeRabbit
Release Notes
New Features
--min-marginCLI argument to control parameter acceptance margin threshold (default: 1.0)Improvements