refactor: add new search module to fhe-params [skip-line-limit]#1216
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cedoor
force-pushed
the
refactor/fhe-params-search
branch
from
f7e58af to
65087d1
Compare
Contributor
📝 WalkthroughWalkthroughAdds a BFV parameter search subsystem to the fhe-params crate: a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "search_params (CLI)"
participant Search as "bfv_search"
participant Primes as "Prime_Selection"
participant Validate as "Validation"
participant Refine as "Refinement"
User->>CLI: invoke search (args)
CLI->>Search: bfv_search(config)
loop for each d (powers of two)
Search->>Primes: select_max_q_under_cap(limit_log2, prime_pool)
Primes-->>Search: candidate_primes
Search->>Validate: finalize_bfv_candidate(d, candidate_primes)
alt validation passes
Validate-->>Search: initial_result
Search->>Refine: refine_from_initial(d, candidate_primes)
Refine->>Primes: construct_qi_for_target_bits(d, bits)
Primes-->>Refine: refined_primes
Refine->>Validate: finalize_bfv_candidate(d, refined_primes)
Validate-->>Refine: refined_result
Refine-->>Search: optimized_result
else validation fails
Validate-->>Search: none
end
end
Search-->>CLI: best_result or error
CLI-->>User: print report / exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cedoor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/fhe-params/README.md`:
- Around line 128-137: The example is missing the build_pair_for_preset import
and doesn't show that the snippet must be inside a function returning Result to
use ?; add the import (e3_fhe_params::builder::build_pair_for_preset) alongside
BfvPreset and build_bfv_params_arc, and wrap the example in a function signature
that returns a Result (or add a comment stating “In a function returning
Result”) so calls to build_bfv_params_arc(...) and build_pair_for_preset(...)
can use the ? operator.
In `@crates/fhe-params/src/search/bfv.rs`:
- Around line 68-76: The qi_values() method in BfvSearchResult currently calls
expect on to_u64() which can panic if a prime ever exceeds u64::MAX; change
qi_values to return Result<Vec<u64>, _> (e.g., Result<Vec<u64>, BfvError> or
anyhow::Error) and replace the direct expect with fallible conversion (map each
selected_primes.iter().map(|p| p.value.to_u64().ok_or(...)) and
collect::<Result<_,_>>() ), returning an appropriate error when any prime is too
large; update callers of BfvSearchResult::qi_values accordingly to handle the
Result.
- Around line 828-834: The test currently asserts a tautology for the return
value of finalize_second_param; replace the useless assert with concrete checks:
call finalize_second_param(&config, d, small_primes, k_plain) and if you expect
a Some variant assert result.is_some() and validate its contents (e.g., check
the returned params contain expected modulus/primes/bit-length consistent with
k_plain, d and small_primes), otherwise assert result.is_none() for invalid
inputs; reference the symbols finalize_second_param, k_plain, d, small_primes
and result to locate and update the assertion to validate actual properties of
the returned parameter struct instead of using is_none() || is_some().
- Around line 693-704: The validation (let ok = lhs < delta;) and failure return
are currently guarded by bfv_search_config.verbose causing non-verbose runs to
skip it; move the comparison and the if !ok { return None; } out of the
verbose-only branch so the code always validates Eq1, and keep the println!
debug output inside the bfv_search_config.verbose block. Concretely: compute ok
using the existing let ok = lhs < delta; and immediately check if !ok return
None; then (only if bfv_search_config.verbose) run the println! block that shows
"2*B_C ... PASS/FAIL" and the "*** BFV-2nd FEASIBLE ..." message.
- Around line 316-407: The clamp to 61 for floor_r/ceil_r in
construct_qi_for_target_bits allows looking up a 61-bit bucket even though
build_prime_items filters out 61-bit primes; fix by clamping to the actual
available max bit-length from the buckets instead of hardcoding 61. Compute let
max_bit = by_bits_small.keys().max().cloned().unwrap_or(61) and use max_bit as
the upper clamp when computing floor_r and ceil_r (replace 61.0 with max_bit as
f64), ensuring subsequent by_bits_small.get(&floor_r) / get(&ceil_r) lookups
match the actual data.
🧹 Nitpick comments (7)
crates/fhe-params/README.md (1)
63-63: Convert bare URL to markdown link.Per markdownlint (MD034), bare URLs should be wrapped in angle brackets or formatted as proper markdown links for better readability and accessibility.
📝 Suggested fix
-- https://eprint.iacr.org/2024/1285.pdf (BFV security) +- [BFV security analysis](https://eprint.iacr.org/2024/1285.pdf)crates/fhe-params/src/search/errors.rs (2)
53-67: Consider preserving original error type instead of stringifying.The
Fromimplementations convertValidationErrorandSearchErrorto strings via.to_string(), which loses type information and prevents matching on specific error variants upstream. Consider using#[from]attribute or nesting the original error for better error handling.♻️ Alternative approach using #[from]
#[derive(Error, Debug)] pub enum BfvParamsError { - /// Validation errors for BFV parameters - #[error("Validation error: {message}")] - Validation { message: String }, + /// Validation errors for BFV parameters + #[error("Validation error: {0}")] + Validation(#[from] ValidationError), - /// Parameter search errors - #[error("Parameter search error: {message}")] - Search { message: String }, + /// Parameter search errors + #[error("Parameter search error: {0}")] + Search(#[from] SearchError), /// Generic error with context #[error("Error: {message}")] Generic { message: String }, } - -// Remove manual From implementations - #[from] handles this
69-81: BroadFrom<String>conversions are permissive.These conversions allow any string to become a
Genericerror, which can obscure error origins. This is a conscious design tradeoff for ergonomics, similar toanyhow. Just ensure call sites use specific error variants (likeValidationError::InvalidVotes) when the error type is known.crates/fhe-params/src/search/mod.rs (1)
1-11: Consider adding module-level documentation.The search module aggregates important BFV parameter search functionality but lacks a doc comment describing its purpose. Adding documentation would improve discoverability and align with the comprehensive README.
📝 Suggested documentation
// SPDX-License-Identifier: LGPL-3.0-only // // This file is provided WITHOUT ANY WARRANTY; // without even the implied warranty of MERCHANTABILITY // or FITNESS FOR A PARTICULAR PURPOSE. +//! BFV parameter search module. +//! +//! This module provides algorithms and utilities for finding optimal BFV parameters +//! that satisfy security constraints. It includes: +//! +//! - [`bfv`]: Core search algorithm and configuration types +//! - [`constants`]: NTT-friendly primes and search bounds +//! - [`errors`]: Error types for search operations +//! - [`prime`]: Prime item model and selection utilities +//! - [`utils`]: BigUint helpers for exact arithmetic + pub mod bfv; pub mod constants; pub mod errors; pub mod prime; pub mod utils;crates/fhe-params/src/search/utils.rs (1)
10-13: Consider returningResultinstead of panicking.
parse_hex_bigpanics on invalid input. While this is acceptable for parsing compile-time constants fromNTT_PRIMES_BY_BITS, consider returning aResultif this function might be exposed to untrusted input in the future.crates/fhe-params/src/search/prime.rs (1)
50-59: Consider documenting why specific bit-lengths are excluded.The exclusion of 61, 62, and 63-bit primes (and the different filters for first vs. second parameter sets) appears intentional for FHE compatibility reasons, but a brief comment explaining the rationale would improve maintainability.
📝 Suggested documentation
+/// Build a flat list of all primes with precomputed log2 and hex strings. +/// Excludes 61, 62, and 63-bit primes to ensure compatibility with typical +/// FHE library constraints on coefficient modulus sizes. pub fn build_prime_items() -> Vec<PrimeItem> { build_prime_items_with_filter(|bits| bits == 63 || bits == 62 || bits == 61) } +/// Build prime items for second parameter set. +/// Includes 62-bit primes but excludes 61 and 63-bit primes to allow +/// larger coefficient modulus while maintaining NTT compatibility. pub fn build_prime_items_for_second() -> Vec<PrimeItem> { build_prime_items_with_filter(|bits| bits == 63 || bits == 61) }crates/fhe-params/src/search/bfv.rs (1)
170-272: Edge case:finalize_bfv_candidatewith emptychosenvector.When
chosenis empty,product()returnsBigUint::one(), makingq_bfv = 1. This leads todelta = 1 / k_plain_eff = 0(integer division), and the Eq1 checklhs < deltawill always fail (returningNone). While the caller filters empty selections, adding an early return would make the invariant explicit.🛡️ Defensive early return
pub fn finalize_bfv_candidate( bfv_search_config: &BfvSearchConfig, d: u64, chosen: Vec<PrimeItem>, ) -> Option<BfvSearchResult> { + if chosen.is_empty() { + return None; + } let q_bfv = product(chosen.iter().map(|pi| pi.value.clone()));
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/fhe-params/README.md`:
- Around line 186-207: The example in README uses the `?` operator on fallible
calls (build_bfv_params_arc, decode_bfv_params, decode_bfv_params_arc) without
being inside a function that returns Result; wrap the snippet in a function
signature returning Result (e.g., fn example() -> Result<(), Box<dyn
std::error::Error>>) and return Ok(()) at the end so the `?` operator is valid,
keeping the existing calls to build_bfv_params_arc, encode_bfv_params,
decode_bfv_params, and decode_bfv_params_arc unchanged.
🧹 Nitpick comments (3)
crates/fhe-params/README.md (1)
63-63: Format bare URL as a Markdown link.The URL should be formatted as a proper Markdown link to satisfy documentation standards and linting rules.
-https://eprint.iacr.org/2024/1285.pdf (BFV security) +[BFV security paper](https://eprint.iacr.org/2024/1285.pdf)crates/fhe-params/src/search/bfv.rs (2)
177-186: Division by zero ifk_plain_effis zero.The public function
finalize_bfv_candidatecan panic at line 186 (&q_bfv / &k_big) if bothkandzin the config are zero, makingk_plain_eff = 0. Whilebfv_searchvalidatesz > 0, direct callers of this public API could trigger this.Consider adding a guard or documenting the precondition.
💡 Optional defensive check
pub fn finalize_bfv_candidate( bfv_search_config: &BfvSearchConfig, d: u64, chosen: Vec<PrimeItem>, ) -> Option<BfvSearchResult> { let q_bfv = product(chosen.iter().map(|pi| pi.value.clone())); // Compute plaintext space: max of user-defined k and z let k_plain_eff: u128 = bfv_search_config.k.max(bfv_search_config.z); + if k_plain_eff == 0 { + return None; + } // r_k(q) = q mod k let k_big = BigUint::from(k_plain_eff);
548-551: Consider computingmax_bitdynamically for consistency.Unlike
construct_qi_for_target_bitswhich now computesmax_bitfrom actual bucket keys (line 339), this function hardcodes 62. While correct for the currentbuild_prime_items_for_second()pool, computing it dynamically would be more resilient to future changes.💡 Optional consistency improvement
let target_f = target_bits as f64; - let s = target_bits.div_ceil(62).max(2) as usize; + let max_bit = by_bits_small.keys().max().cloned().unwrap_or(62); + let s = target_bits.div_ceil(max_bit as u64).max(2) as usize; let r_float = target_f / (s as f64); - let floor_r = r_float.floor().clamp(40.0, 62.0) as u8; - let ceil_r = r_float.ceil().clamp(40.0, 62.0) as u8; + let floor_r = r_float.floor().clamp(40.0, max_bit as f64) as u8; + let ceil_r = r_float.ceil().clamp(40.0, max_bit as f64) as u8;
0xjei
force-pushed
the
refactor/fhe-params-search
branch
from
0a3eabc to
ea53828
Compare
0xjei
force-pushed
the
refactor/fhe-params-search
branch
from
ea53828 to
86b2685
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/fhe-params/README.md`:
- Around line 146-157: The example uses the ? operator on
build_bfv_params_from_set_arc without a Result-returning context; wrap the
snippet in a function (e.g., fn example() -> Result<(), Box<dyn
std::error::Error>>) or main that returns Result and add an Ok(()) return so
BfvParamSet creation and let params = build_bfv_params_from_set_arc(¶m_set)?
compile correctly; reference the BfvParamSet struct and
build_bfv_params_from_set_arc function when making the change.
In `@crates/fhe-params/src/bin/search_params.rs`:
- Around line 153-156: The verbose banner printed when args.verbose is true is
inconsistent with docs: update the string in the println call inside the
args.verbose branch to read "== BFV parameter search (NTT-friendly primes 40..60
bits; 61-, 62- and 63-bit primes are excluded) ==" (or equivalent wording) so it
matches the README and the build_prime_items() documentation; ensure the message
references args.verbose and keep the wording consistent with
build_prime_items()'s exclusion of 61, 62 and 63-bit primes.
In `@crates/fhe-params/src/search/utils.rs`:
- Around line 26-44: The function log2_big produces wrong values for small
BigUint because adjust uses take*8 rather than the actual significant bit-length
of the extracted top u64; fix by computing the actual bit length of top (e.g.
top_bits = if top==0 {0} else {64 - top.leading_zeros() as usize} clipped to
take*8), normalize top by shifting out the extra high zero bits (shift = take*8
- top_bits), compute normalized = (top as f64) / 2f64.powi(shift as i32), then
return (bits - 1) as f64 + normalized.log2(); update log2_big to use
top_bits/shift and normalized value instead of the current (bits - adjust) +
frac formula.
🧹 Nitpick comments (3)
crates/fhe-params/README.md (1)
64-64: Convert bare URL to Markdown link.Per markdownlint MD034, bare URLs should be wrapped in angle brackets or converted to proper Markdown links for better accessibility and consistent formatting.
-The library implements security analysis from: - -- https://eprint.iacr.org/2024/1285.pdf (BFV security) +The library implements security analysis from: + +- [BFV security analysis](https://eprint.iacr.org/2024/1285.pdf)crates/fhe-params/src/search/bfv.rs (2)
170-272: Consider handling emptychosenvector explicitly.When
chosenis empty,product()returnsBigUint::one(), makingq_bfv = 1. This leads todelta = 1 / k = 0which causes the validation to fail (as expected), but an explicit early return with a more descriptive error would be clearer.The current behavior is safe but could be more explicit:
pub fn finalize_bfv_candidate( bfv_search_config: &BfvSearchConfig, d: u64, chosen: Vec<PrimeItem>, ) -> Option<BfvSearchResult> { + if chosen.is_empty() { + return None; + } let q_bfv = product(chosen.iter().map(|pi| pi.value.clone()));
527-610: Hardcoded 62-bit clamp is intentional but could be dynamic.Unlike
construct_qi_for_target_bits, this function uses hardcoded 62 for clamp bounds (lines 548, 550-551). This is intentional sincebuild_prime_items_for_second()includes 62-bit primes, but for consistency, consider derivingmax_bitdynamically as done in the primary function.
0xjei
approved these changes
Jan 28, 2026
- Move crypto_params crate into fhe-params as search module - Update all imports to use crate::search:: paths - Add required dependencies (num-traits, clap, anyhow) to fhe-params - Remove unused num-integer dependency - Update documentation references from crypto_params to search - Delete standalone crypto_params directory
- Remove unused BfvParamsError variants (Math, Config, PrimeSelection) - Remove unused ValidationError variants (InvalidParties, InvalidSecurity, InvalidErrorBound, General) - Remove unused SearchError variants (EquationValidation, PrimeSelection, General) - Remove unused helper functions (validation, search, math, config, prime_selection)
- Change product function to accept IntoIterator instead of slice - Use Iterator::fold for more idiomatic implementation - Update all call sites to pass iterators directly, avoiding Vec allocations
- Add tests for utils.rs: parse_hex_big, product, log2_big, approx_bits_from_log2, fmt_big_summary, big_shift_pow2 - Add tests for prime.rs: build_prime_items, build_prime_items_for_second, select_max_q_under_cap - Add tests for bfv.rs: qi_values extraction, input validation, candidate finalization, prime construction
- Remove PlaintextMode enum from constants.rs - Simplify k_plain_eff calculation to max(k, z) - Remove unused mode parameter from finalize_bfv_candidate
- Extract build_prime_items_with_filter to eliminate code duplication - Unify build_prime_items and build_prime_items_for_second using filter function - Remove unused q variable from select_max_q_under_cap (only qlog needed) - Remove unused num_traits::One import
- Add function-level docs for public APIs in bfv.rs - Document algorithm strategies and validation logic - Add struct documentation for PrimeItem - Document utility functions and constants - Note relationship between search results and hardcoded presets
- Replace crypto_params-specific README with fhe-params crate documentation - Add comprehensive documentation for all modules (presets, builder, search, encoding) - Include detailed search module documentation with security constraints and usage - Add usage examples for presets, custom parameter sets, parameter search, and ABI encoding - Fix terminology: change 'zkFHE' to 'FHE' in lib.rs and README
- Add missing build_pair_for_preset import in README example - Wrap README example in function returning Result for ? operator usage - Move Eq1 validation outside verbose block to always execute - Replace tautological test assertion with concrete property validations - Fix hardcoded 61-bit clamp to use actual max bit length from prime buckets
- Add search_params binary for interactive BFV parameter search - Refactor CLI code to reduce duplication with helper functions - Fix variance calculations to handle BigUint overflow - Add comprehensive CLI documentation to README - Include example for reproducing SecureThresholdBfv8192 preset
- Add stub src/bin/search_params.rs before first cargo build - Fixes CI build_ciphernode_image when e3-fhe-params declares [[bin]]
- Change prime range from 40..61 to 40..60 bits - State that 61-, 62- and 63-bit primes are excluded (matches build_prime_items())
cedoor
force-pushed
the
refactor/fhe-params-search
branch
from
74a2753 to
1b785f1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/fhe-params/README.md`:
- Around line 62-65: Replace the bare URL in the README line "The library
implements security analysis from:" with a proper markdown link (e.g., [BFV
security paper](https://eprint.iacr.org/2024/1285.pdf)) so the link is not a
bare URL and MD034 is satisfied; update that specific sentence in
crates/fhe-params/README.md where the URL appears.
- Around line 226-238: Replace the bolded example label "**Example: Reproducing
Production Preset**" with a proper Markdown heading to satisfy MD036 — e.g., add
a heading line like "### Example: Reproducing Production Preset" immediately
above the code block (removing the surrounding **...**), keeping the example
code and the reference to the production preset name SecureThresholdBfv8192
unchanged.
In `@crates/fhe-params/src/search/prime.rs`:
- Around line 61-85: The loop in select_max_q_under_cap currently iterates bit
lengths (40u8..=60u8). That omits 62‑bit primes; update the iteration to include
the actual max bit length (e.g., compute let max_bit =
*by_bits.keys().max().unwrap_or(&60) and iterate (40..=max_bit).rev()) or at
minimum extend the hardcoded range to 40u8..=62u8 so 62‑bit primes in by_bits
are considered; adjust the variable bb and the by_bits.get_mut(&bb) logic
accordingly.
🧹 Nitpick comments (1)
crates/fhe-params/src/search/bfv.rs (1)
442-466: Avoid rebuilding the second-prime pool each iteration.
Movebuild_prime_items_for_second()outside the loop to cut redundant allocations.♻️ Suggested change
- while d <= D_POW2_MAX { + let prime_items_second = build_prime_items_for_second(); + while d <= D_POW2_MAX { // Eq4: d ≥ 37.5*log2(q/B) + 75 => log2(q) ≤ log2(B) + (d-75)/37.5 let log2_q_limit = log2_b + ((d as f64) - 75.0) / 37.5; @@ - let prime_items_second = build_prime_items_for_second(); if let Some(res) = refine_second_param_at_d( bfv_search_config, d, &prime_items_second, log2_q_limit, k_second, ) { return Some(res); } d <<= 1; }
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.
This PR integrates the
crypto_paramscrate intofhe-paramsas asearchmodule, consolidating FHE parameter search functionality into a single crate.Changes
crypto_paramscrate intofhe-paramsas asearchmoduleproductfunction to accept iterators for better ergonomicsImpact
Closes #1202
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.