refactor: polynomial conversions [skip-line-limit]#1231
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a CRT-based polynomial type ( Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as SDK / Frontend
participant WASM as zk-inputs-wasm
participant Generator as ZK Generator
participant Commit as zk-helpers::commitments
participant Poly as e3_polynomial::CrtPolynomial
SDK->>WASM: computeCiphertextCommitment(ct0is, ct1is)
WASM->>Poly: CrtPolynomial::from_bigint_vectors(ct0is)
WASM->>Poly: CrtPolynomial::from_bigint_vectors(ct1is)
WASM->>Generator: generator.compute_commitment(&ct0, &ct1)
Generator->>Commit: compute_ciphertext_commitment(&ct0, &ct1, bit_ct)
Commit-->>Generator: BigInt (commitment)
Generator-->>WASM: commitment
WASM-->>SDK: return commitment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
cedoor
force-pushed
the
refactor/polynomial-conversions
branch
from
4a9b9e4 to
d7b0a38
Compare
cedoor
force-pushed
the
refactor/polynomial-conversions
branch
from
9a4c65f to
5dc1462
Compare
cedoor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
crates/polynomial/Cargo.toml (1)
12-20:⚠️ Potential issue | 🟡 MinorRemove unused
ndarraydependency from Cargo.toml.Verification shows
fhe_mathandbincodeare properly used, butndarrayhas no usage in the crate and should be removed to avoid dependency bloat.
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 157: Remove the unused "rand" feature from the num-bigint dependency
entry in Cargo.toml by deleting the features = ["rand"] clause so the dependency
becomes num-bigint = { version = "=0.4.6" }; verify no code uses num-bigint's
RandBigInt APIs and that existing random logic continues using the rand crate
directly (e.g., rand::thread_rng(), ChaCha20Rng); then run cargo build/cargo
update to ensure the workspace dependency graph updates and no member (like
crates/compute-provider) inherits the unnecessary feature.
In `@examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs`:
- Around line 143-145: The code currently discards the remainder returned from
diff.div(&qi) (bound to _remainder) when computing (q_poly, _remainder); change
this to capture the remainder, verify it is zero (or all-zero polynomial) and
return an error (or panic/eyre) including index i if it is non-zero so the
division is exact; update the call site around diff.div(&qi) and the surrounding
logic that relies on q_poly to handle the validation failure.
🧹 Nitpick comments (4)
crates/polynomial/src/utils.rs (1)
29-60: Consider guarding or documentingmodulus != 0inreduce.
% 0will panic; since this is now public, an explicit precondition helps.🔧 Optional guard
pub fn reduce(x: &BigInt, modulus: &BigInt) -> BigInt { + debug_assert!(!modulus.is_zero(), "modulus must be non-zero"); let mut r = x % modulus; if r < BigInt::zero() { r += modulus; } r }crates/polynomial/src/crt_polynomial.rs (2)
64-66: Incomplete documentation section.The doc comment has an empty "Coefficient order" section header with no content following it. Either add documentation explaining the coefficient order semantics or remove the empty section.
📝 Suggested fix
- /// # Coefficient order - /// - + /// # Coefficient order + /// + /// The resulting polynomial retains the coefficient order from fhe-math's PowerBasis + /// representation (ascending degree: c_0, c_1, …, c_n). Call `reverse()` if descending + /// order is needed for Horner evaluation.
177-186: Documentation references non-existent method.The doc comment references
from_limb_coefficientsas the inverse operation, but the actual constructor isfrom_bigint_vectors.📝 Suggested fix
/// Returns limb coefficient vectors (one `Vec<BigInt>` per modulus). /// /// Use when you need a raw CRT representation for serialization, hashing, - /// or APIs that expect `&[Vec<BigInt>]`. The inverse of [`from_limb_coefficients`](Self::from_limb_coefficients). + /// or APIs that expect `&[Vec<BigInt>]`. The inverse of [`from_bigint_vectors`](Self::from_bigint_vectors). pub fn to_limb_coefficients(&self) -> Vec<Vec<BigInt>> {examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs (1)
119-120: Stale documentation parameter.The doc comment documents a parameter
n(polynomial degree) that doesn't exist in the actual function signature.📝 Suggested fix
/// * `sum` - CRT polynomial of the sum ciphertext /// * `a` - CRT polynomial of the first ciphertext /// * `b` - CRT polynomial of the second ciphertext - /// * `n` - polynomial degree (number of coefficients per limb) /// * `moduli` - moduli for each CRT limb
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/polynomial/Cargo.toml`:
- Around line 10-16: Update the polynomial crate's Cargo.toml to remove unused
dependency bloat: in the num-bigint dependency entry drop the unused "rand" and
"serde" features (leave num-bigint in workspace but only with needed features),
and remove the ndarray dependency entirely; keep fhe-math as-is since
crt_polynomial.rs uses it. Ensure the resulting Cargo.toml no longer lists
ndarray and that num-bigint's features only include those actually used by the
polynomial crate.
In `@crates/polynomial/src/crt_polynomial.rs`:
- Around line 55-77: Update the from_fhe_polynomial docstring to specify that
coefficients().outer_iter() yields coefficients in ascending degree order (c_0,
c_1, ...), that if p is in NTT it is converted to PowerBasis (as already done
via p.change_representation(Representation::PowerBasis)), and callers who expect
descending-degree ordering must apply .reverse() to the resulting limbs (see the
ciphertext_addition.rs example). Also mention that the returned Self { limbs }
preserves this ascending ordering so downstream code (e.g.,
Polynomial::from_u64_vector) should treat limb 0 as the constant term.
🧹 Nitpick comments (1)
crates/polynomial/src/crt_polynomial.rs (1)
155-170: Consider adding a safe accessor to avoid panics in public API.Optional enhancement: add
get_limbreturningOption<&Polynomial>for callers that prefer bounds-checked access.💡 Possible addition
impl CrtPolynomial { + pub fn get_limb(&self, i: usize) -> Option<&Polynomial> { + self.limbs.get(i) + } }
- Reword comment for first-in-slot votes and prev_ct_commitment - Add IMPORTANT prefix so the requirement is not missed Co-authored-by: Cursor <cursoragent@cursor.com>
- Capture remainder from diff.div(&qi) instead of discarding it - Verify remainder is zero (all-zero polynomial) after division - Return eyre error including modulus index i when remainder is non-zero Co-authored-by: Cursor <cursoragent@cursor.com>
…ding_coefficients - Drop from_ascending_coefficients and to_ascending_coefficients; use new() + reverse() instead - Point new() doc to reverse() for ordering - Remove tests and benchmark that relied on removed APIs - Update README to mention reverse() for ordering Co-authored-by: Cursor <cursoragent@cursor.com>
cedoor
force-pushed
the
refactor/polynomial-conversions
branch
from
a82208f to
0dac129
Compare
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)
crates/polynomial/src/polynomial.rs (1)
371-400:⚠️ Potential issue | 🟡 MinorDocs claim in-place mutation but methods return new polynomials.
scalar_mulandreduce_by_cyclotomicdo not mutateself; the comments are misleading.📝 Suggested doc fix
- /// Mutates the polynomial in place. + /// Returns a new polynomial with each coefficient scaled. @@ - /// Mutates the polynomial in place. + /// Returns the reduced polynomial (does not mutate `self`).
🤖 Fix all issues with AI agents
In `@examples/CRISP/crates/zk-inputs/src/lib.rs`:
- Around line 127-143: CiphertextAdditionInputs::compute is being called with
bit_pk (public-key bounds) but it expects bit_ct (ciphertext bounds) used for
packing; this leads to wrong commitment calculations via
compute_ciphertext_commitment. Fix by computing the correct ciphertext bit width
(bit_ct: u32) from the ciphertext bounds and pass that bit_ct into
CiphertextAdditionInputs::compute (replace bit_pk with bit_ct) at all call sites
where CiphertextAdditionInputs::compute is invoked, and ensure any subsequent
calls to compute_ciphertext_commitment also use the same bit_ct value.
🧹 Nitpick comments (5)
crates/polynomial/src/utils.rs (2)
54-67:reduce_and_centerduplicates centering logic fromcenter.The centering logic (lines 58-64) is identical to the
centerfunction (lines 28-34). Consider refactoring to reusecenter:♻️ Proposed refactor to eliminate duplication
pub fn reduce_and_center(x: &BigInt, modulus: &BigInt, half_modulus: &BigInt) -> BigInt { - let mut r = reduce(x, modulus); - - // Adjust the remainder if it is greater than half_modulus. - if (modulus % BigInt::from(2)) == BigInt::from(1) { - if r > *half_modulus { - r -= modulus; - } - } else if r >= *half_modulus { - r -= modulus; - } - - r + let r = reduce(x, modulus); + center(&r, modulus) }Note: The
half_modulusparameter becomes unused and could be removed in a follow-up if callers are updated.
118-130: Unnecessary clone inreduce_in_ring.Line 123 clones
coefficientsonly to immediately wrap it in aPolynomial. Sincereduce_by_cyclotomicreturns a newPolynomial, the clone could be avoided by constructing directly:♻️ Proposed refactor to avoid clone
pub fn reduce_in_ring( coefficients: &mut Vec<BigInt>, cyclo: &[BigInt], modulus: &BigInt, ) -> Result<(), PolynomialError> { - let coeffs = coefficients.clone(); - let poly = Polynomial::new(coeffs); + let poly = Polynomial::new(std::mem::take(coefficients)); let reduced = poly.reduce_by_cyclotomic(cyclo)?; - *coefficients = reduced.coefficients; + *coefficients = reduced.into_coefficients(); // or similar accessor reduce_coefficients_mut(coefficients, modulus); center_coefficients_mut(coefficients, modulus); Ok(()) }This requires an
into_coefficients()method onPolynomial. Alternatively, keep the current approach if the clone overhead is negligible for typical polynomial sizes.crates/polynomial/src/crt_polynomial.rs (2)
146-150: Consider removing unnecessary borrow inreduce_uniform.Line 148 uses
&modulusbutmodulusis already a reference. The extra&is harmless but unnecessary.♻️ Proposed fix
pub fn reduce_uniform(&mut self, modulus: &BigInt) { for limb in &mut self.limbs { - limb.reduce(&modulus); + limb.reduce(modulus); } }
166-168:limb()panics on out-of-bounds; consider returningOptionor documenting panics prominently.The method panics if
i >= self.limbs.len(). While this is documented, a safer alternative would be to returnOption<&Polynomial>or provide a separateget()method. However, if panic semantics are intentional for internal use, the current documentation is acceptable.examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
184-188: Validate limb/degree shape before buildingCrtPolynomial.
This avoids silently producing commitments from malformed JS input.🧩 Suggested input validation
+ let params = self.generator.get_bfv_params(); + let expected_limbs = params.moduli().len(); + let expected_degree = params.degree(); + + if ct0is_vec.len() != expected_limbs || ct1is_vec.len() != expected_limbs { + return Err(JsValue::from_str("ct0is/ct1is limb count mismatch")); + } + if ct0is_vec.iter().any(|limb| limb.len() != expected_degree) + || ct1is_vec.iter().any(|limb| limb.len() != expected_degree) + { + return Err(JsValue::from_str( + "ct0is/ct1is coefficient length mismatch", + )); + }
2 participants
Closes #1203
Summary by CodeRabbit
New Features
Refactor
Chores