feat: enhance polynomial - #26
Conversation
…pport for univariate and multivariate polynomials - Introduce `Polynomial` type with an extended API for enhanced usability. - Add univariate polynomial implementation under `poly/univariate.rs` with advanced operations like composition, division with remainder, and interpolation. - Add multivariate polynomial implementation under `poly/multivariate.rs` with sparse representation and support for variable-specific operations. - Migrate all references in tests and examples from `DensePolynomial` to `Polynomial`. - Remove `DensePolynomial` and its supporting code.
- Implement `to_le_bytes()` and `to_be_bytes()` for `U1024` with unit tests. - Add comprehensive roundtrip and value-specific tests for byte conversions. - Bump crate version to `1.1.0` and update changelog.
WalkthroughThe PR refactors polynomial support by converting DensePolynomial from a concrete type into a generic Polynomial type, introduces a new MultivariatePolynomial for sparse multivariate polynomials, adds extensive univariate polynomial operations (interpolation, division, derivatives, composition), adds byte conversion methods to U1024, and includes comprehensive integration tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Key areas requiring careful attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/polynomial_integration_test.rs (1)
180-197: Consider removing or renaming this test.The test name
test_dense_polynomial_aliasand comment "Polynomial should be identical to Polynomial" appear to be leftover artifacts from theDensePolynomialtoPolynomialmigration. The test now trivially compares two instances of the same type, which doesn't provide meaningful coverage.Consider either:
- Removing this test entirely, or
- If
DensePolynomialis still available as a type alias, rename and update the test to verify that alias relationshipsrc/poly/univariate.rs (2)
144-155: Minor: Redundant zero check afteris_zero()guard.Lines 148-151 handle the
Nonecase fromself.degree(), but this only occurs whenself.is_zero(), which is already handled at lines 144-146. The check is harmless but redundant.🔎 Simplified version
if self.is_zero() { return (Self::zero(), Self::zero()); } - let self_deg = match self.degree() { - Some(d) => d, - None => return (Self::zero(), Self::zero()), - }; + let self_deg = self.degree().unwrap(); // Safe: self is not zero let divisor_deg = match divisor.degree() { Some(d) => d, None => panic!("Division by zero polynomial"), };
119-127: Performance: Derivative uses O(n²) repeated addition instead of O(n) scalar multiplication.The loop multiplies each coefficient by its index using repeated addition, which is O(i) per term, resulting in O(n²) total. Use scalar multiplication instead:
let mut result = Vec::with_capacity(self.coeffs.len() - 1); for (i, coeff) in self.coeffs.iter().enumerate().skip(1) { - // multiply by i - let mut acc = FieldElement::zero(); - for _ in 0..i { - acc = acc + *coeff; - } - result.push(acc); + let i_elem = FieldElement::<C>::new(U1024::from_u64(i as u64)); + result.push(*coeff * i_elem); }src/poly/multivariate.rs (3)
125-138: Performance: Inefficient zero-term removal after addition.After adding coefficients, the code searches through all keys to find the zero entry (O(n)), but we already know which key was modified. The key was moved into
entry(), making it unavailable. Consider cloning the key beforehand or using theEntryAPI more directly.🔎 Proposed optimization using Entry API
pub fn add_term(&mut self, exponents: Exponent, coeff: FieldElement<C>) { assert_eq!( exponents.len(), self.num_vars, "Exponent vector length must match num_vars" ); if coeff.is_zero() { return; } - let entry = self.terms.entry(exponents).or_insert(FieldElement::zero()); - *entry = *entry + coeff; - - // Remove if resulting coefficient is zero - if entry.is_zero() { - let exp = self - .terms - .keys() - .find(|k| self.terms[*k].is_zero()) - .cloned(); - if let Some(e) = exp { - self.terms.remove(&e); - } - } + use std::collections::btree_map::Entry; + match self.terms.entry(exponents) { + Entry::Occupied(mut e) => { + let new_val = *e.get() + coeff; + if new_val.is_zero() { + e.remove(); + } else { + *e.get_mut() = new_val; + } + } + Entry::Vacant(e) => { + e.insert(coeff); + } + } }
161-167: Performance: Linear exponentiation instead of binary exponentiation.The power computation uses repeated multiplication (O(exp)), which could be slow for high-degree terms. Consider using binary exponentiation (square-and-multiply) for O(log exp) complexity.
🔎 Proposed optimization with binary exponentiation
for (i, &exp) in exponents.iter().enumerate() { - // Compute point[i]^exp - let mut power = FieldElement::one(); - for _ in 0..exp { - power = power * point[i]; - } + // Compute point[i]^exp using binary exponentiation + let mut power = FieldElement::one(); + let mut base = point[i]; + let mut e = exp; + while e > 0 { + if e & 1 == 1 { + power = power * base; + } + base = base * base; + e >>= 1; + } term_value = term_value * power; }
189-201: Same exponentiation performance issue as inevaluate.The power computation at lines 192-195 uses repeated multiplication. Apply the same binary exponentiation optimization as suggested for
evaluate.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)benches/poly_bench.rs(1 hunks)examples/poly_display.rs(1 hunks)src/big_int/u1024.rs(1 hunks)src/lib.rs(1 hunks)src/poly/dense.rs(0 hunks)src/poly/mod.rs(1 hunks)src/poly/multivariate.rs(1 hunks)src/poly/univariate.rs(1 hunks)tests/integration_test.rs(2 hunks)tests/polynomial_display_test.rs(1 hunks)tests/polynomial_integration_test.rs(1 hunks)tests/polynomial_large_coeff_test.rs(5 hunks)tests/polynomial_test.rs(10 hunks)tests/u1024_test.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/poly/dense.rs
🧰 Additional context used
🧬 Code graph analysis (11)
tests/u1024_test.rs (1)
src/big_int/u1024.rs (2)
from_le_bytes(460-468)from_be_bytes(485-495)
tests/integration_test.rs (1)
src/poly/dense.rs (5)
C(11-80)C(82-104)DensePolynomial(7-9)C(106-127)C(135-218)
tests/polynomial_large_coeff_test.rs (2)
src/poly/multivariate.rs (1)
new(34-39)src/poly/univariate.rs (1)
new(27-31)
examples/poly_display.rs (1)
src/poly/univariate.rs (1)
new(27-31)
tests/polynomial_integration_test.rs (2)
src/poly/multivariate.rs (11)
new(34-39)is_zero(87-89)zero(42-44)one(47-51)constant(54-60)evaluate(150-173)monomial(73-84)neg(282-288)variable(63-70)degree_in(103-110)from_univariate(225-233)src/poly/univariate.rs (11)
new(27-31)degree(66-72)is_zero(75-77)leading_coefficient(80-82)zero(34-36)one(39-43)x(51-55)constant(46-48)evaluate(96-106)monomial(58-62)neg(344-346)
src/big_int/u1024.rs (2)
tests/property_tests.rs (1)
u1024_to_biguint(6-12)src/big_int/mod.rs (1)
u1024(2-2)
tests/polynomial_test.rs (2)
src/poly/univariate.rs (2)
new(27-31)zero(34-36)src/poly/dense.rs (1)
C(11-80)
tests/polynomial_display_test.rs (1)
src/poly/univariate.rs (1)
new(27-31)
src/poly/univariate.rs (3)
src/poly/ntt.rs (2)
ntt(17-50)intt(53-70)tests/polynomial_test.rs (1)
test_polynomial_zero(76-79)tests/polynomial_integration_test.rs (2)
test_polynomial_derivative(102-110)test_polynomial_division(67-82)
benches/poly_bench.rs (1)
src/poly/univariate.rs (2)
one(39-43)new(27-31)
src/poly/multivariate.rs (3)
src/poly/univariate.rs (5)
constant(46-48)monomial(58-62)degree(66-72)mul(352-372)x(51-55)tests/polynomial_integration_test.rs (1)
test_multivariate_basic(204-214)src/field/element.rs (1)
to_u1024(146-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Coverage
🔇 Additional comments (52)
Cargo.toml (2)
3-3: LGTM! Version bump is appropriate.The minor version bump from 1.0.0 to 1.1.0 correctly follows semantic versioning for new features with backward compatibility.
4-4: Invalid Rust edition "2024".Rust does not have a "2024" edition. Valid editions are "2015", "2018", and "2021". This will cause compilation failures.
🔎 Proposed fix
-edition = "2024" +edition = "2021"Likely an incorrect or invalid review comment.
CHANGELOG.md (1)
8-48: LGTM! Comprehensive and well-structured changelog.The changelog clearly documents all new features, changes, and improvements. It follows the Keep a Changelog format and provides good context for users upgrading to 1.1.0.
tests/integration_test.rs (1)
3-3: LGTM! Consistent API update.The import and constructor usage have been correctly updated from
DensePolynomialtoPolynomial, aligning with the public API changes.Also applies to: 83-83
tests/polynomial_large_coeff_test.rs (1)
1-1: LGTM! Consistent API migration.All references to
DensePolynomialhave been correctly updated toPolynomialthroughout the test file.Also applies to: 6-6, 14-14, 26-26, 44-44, 53-53, 65-65
benches/poly_bench.rs (1)
2-2: LGTM! Benchmark updated correctly.The benchmark code has been correctly updated to use the new
PolynomialAPI.Also applies to: 9-9
src/big_int/u1024.rs (2)
497-518: LGTM! Well-implemented little-endian conversion.The
to_le_bytesmethod correctly converts U1024 to a 128-byte little-endian array. Documentation and examples are clear.
520-543: Byte order consistency verified in big-endian conversion.The
to_be_bytesandfrom_be_bytesimplementations are correctly synchronized. Both methods handle limb reversal and byte-level endianness consistently:from_be_bytesreads bytes with the first byte as most significant and reconstructs limbs in big-endian order, whileto_be_bytesreverses limb order and applies big-endian byte serialization per limb. Round-trip tests confirm the implementations are consistent.tests/polynomial_test.rs (1)
1-1: LGTM! Comprehensive API migration in tests.All test cases have been correctly updated to use the new
PolynomialAPI instead ofDensePolynomial. The migration is complete and consistent throughout the file.Also applies to: 5-6, 19-20, 28-29, 42-43, 52-52, 67-67, 77-77, 84-84, 92-93, 112-113, 127-128, 141-141, 152-152
tests/u1024_test.rs (4)
225-250: LGTM! Comprehensive round-trip test for little-endian conversion.The test validates that
to_le_bytesandfrom_le_bytesare inverse operations, ensuring correct serialization/deserialization.
252-277: LGTM! Comprehensive round-trip test for big-endian conversion.The test validates that
to_be_bytesandfrom_be_bytesare inverse operations, ensuring correct serialization/deserialization.
279-291: LGTM! Detailed value verification for little-endian conversion.The test checks specific byte positions and verifies that unused bytes are zero, providing good coverage of the implementation details.
293-306: LGTM! Detailed value verification for big-endian conversion.The test correctly verifies that big-endian bytes appear at the end of the 128-byte array (positions 120-127) and that leading bytes are zero, confirming the expected big-endian layout.
src/lib.rs (1)
14-14: LGTM!The updated re-exports cleanly expose the new
PolynomialandMultivariatePolynomialtypes while maintaining the existingntt::*exports. This aligns well with the API refactoring fromDensePolynomialto the new generic polynomial types.examples/poly_display.rs (1)
1-29: LGTM!The example file is cleanly updated to use the new
Polynomialtype. The demonstrations cover various polynomial forms (quadratic, sparse, monomial, linear, constant) and multiplication, providing good coverage of the API.tests/polynomial_integration_test.rs (18)
10-18: LGTM!The basic operations test correctly verifies degree, zero check, and leading coefficient for the polynomial
3x^2 + 2x + 1.
20-37: LGTM!Good coverage of all constructor variants with appropriate assertions.
39-64: LGTM!Arithmetic tests cover addition, subtraction, multiplication, and negation with correct expected values.
66-82: LGTM!Division test correctly verifies that
(x² - 1) / (x - 1) = x + 1with zero remainder.
84-99: LGTM!Evaluation tests correctly verify single-point and batch evaluation.
101-110: LGTM!Derivative test correctly verifies that
d/dx(1 + 2x + 3x²) = 2 + 6x.
112-124: LGTM!Zerofier test correctly validates that the polynomial vanishes at all specified roots.
126-139: LGTM!Interpolation test correctly verifies Lagrange interpolation through the specified points.
141-155: LGTM!Scale and shift operations are correctly tested.
157-170: LGTM!NTT multiplication correctness test is well-structured, comparing fast and naive multiplication results.
172-178: LGTM!Display format test verifies the expected mathematical notation.
203-231: LGTM!Multivariate basic operations and constructor tests are well-structured with appropriate assertions.
233-253: LGTM!Multivariate arithmetic tests correctly verify addition, multiplication, and negation.
255-267: LGTM!Multivariate evaluation test correctly computes
p(2, 3) = 12.
269-286: LGTM!Partial evaluation test correctly verifies dimension reduction and subsequent evaluation.
288-300: LGTM!Degree tracking test correctly verifies per-variable and total degree computation.
302-361: LGTM!Remaining multivariate tests (scale, univariate conversion, equality, display) are well-structured.
367-383: LGTM!Cross-type consistency test effectively validates that univariate and multivariate representations yield identical evaluation results.
src/poly/mod.rs (1)
1-15: LGTM!The module structure is well-organized with clear documentation. The re-exports provide a clean public API surface for
Polynomial,MultivariatePolynomial, and NTT utilities.tests/polynomial_display_test.rs (2)
3-24: LGTM!The display format tests provide good coverage of various polynomial forms including standard, sparse, linear, and zero polynomials.
31-32: Verify Debug format consistency across test files.This test expects the Debug format to start with
"Polynomial(", but the relevant code snippet fromtests/polynomial_test.rs(lines 150-155) shows a test expecting"Poly"prefix. Ensure all test files agree on the expected Debug format.#!/bin/bash # Check Debug format expectations across test files rg -n 'starts_with.*Poly' tests/src/poly/univariate.rs (11)
1-22: LGTM!The module documentation and struct definition are clear. Storing coefficients in ascending degree order with public access (
pub coeffs) is a reasonable design choice for a ZK polynomial library.
24-62: LGTM!Constructors are well-designed with consistent handling of zero coefficients through the
trimmethod.
64-93: LGTM!Accessor methods are correctly implemented. The
trimfunction properly maintains the invariant that coefficients have no trailing zeros.
95-111: LGTM!Evaluation uses Horner's method correctly, providing optimal O(n) complexity.
176-228: LGTM!NTT-based multiplication is correctly implemented with proper zero handling and power-of-two padding. The
shiftmethod correctly implements multiplication by x^n.
230-285: LGTM!The zerofier and Lagrange interpolation implementations are correct. The O(n²) complexity of interpolation is expected for this straightforward approach.
287-304: LGTM!The composition uses Horner's method correctly. The cloning of
otherin each iteration is necessary given theMultrait takes ownership.
349-373: LGTM!The
Mulimplementation intelligently switches between O(n²) schoolbook multiplication for small polynomials and NTT-based O(n log n) multiplication for larger ones. The threshold of 64 total coefficients is a reasonable heuristic.
391-409: LGTM!The equality comparison via
to_u1024()is a valid approach. TheDebugimplementation delegates toDisplayfor useful output.
411-477: LGTM!The
Displayimplementation provides readable polynomial formatting with proper handling of coefficients (decimal for small values, hex for large), degree notation, and edge cases.
479-519: LGTM!The unit tests cover essential functionality. More comprehensive integration tests exist separately per the PR context.
src/poly/multivariate.rs (5)
1-30: LGTM!The sparse representation using
BTreeMap<Exponent, FieldElement>is appropriate for multivariate polynomials where many terms are typically zero. UsingBTreeMapoverHashMapprovides deterministic ordering for display and comparison.
32-84: LGTM!Constructors are well-designed with proper validation of variable indices and exponent vector lengths.
224-250: LGTM!The conversion methods between univariate and multivariate polynomials are correct. Edge cases like zero polynomials are handled properly through the respective constructors' trimming behavior.
252-318: LGTM!Arithmetic trait implementations are correct with proper validation of matching variable counts.
411-479: LGTM!Unit tests provide good coverage of basic functionality including construction, evaluation, and arithmetic operations.
| let coeff_str = if coeff_u1024.0[1..].iter().all(|&x| x == 0) { | ||
| format!("{}", coeff_u1024.0[0]) | ||
| } else { | ||
| format!("0x{:x}", coeff_u1024.0[0]) | ||
| }; |
There was a problem hiding this comment.
Bug: Large coefficient display truncates to first limb only.
When a coefficient requires more than 64 bits, only the lowest limb is displayed (line 376). This differs from the univariate Display implementation which correctly formats all limbs. Large coefficients will appear incorrectly truncated.
🔎 Proposed fix to match univariate formatting
let coeff_str = if coeff_u1024.0[1..].iter().all(|&x| x == 0) {
format!("{}", coeff_u1024.0[0])
} else {
- format!("0x{:x}", coeff_u1024.0[0])
+ let mut hex_str = String::from("0x");
+ let mut started = false;
+ for &limb in coeff_u1024.0.iter().rev() {
+ if !started && limb == 0 {
+ continue;
+ }
+ if started {
+ hex_str.push_str(&format!("{:016x}", limb));
+ } else {
+ hex_str.push_str(&format!("{:x}", limb));
+ started = true;
+ }
+ }
+ hex_str
};🤖 Prompt for AI Agents
In src/poly/multivariate.rs around lines 373 to 377, the coefficient display
only prints the lowest 64-bit limb when higher limbs are non‑zero, truncating
large coefficients; update the branch to format the entire 1024‑bit coefficient
rather than just coeff_u1024.0[0]. Concretely, when all high limbs are zero keep
the plain decimal formatting of the whole coefficient, otherwise produce a full
hexadecimal representation of the entire coeff_u1024 value (delegate to the
type's Display/hex formatting or convert the full value to a hex string) so
every limb is included.
1 participant
Summary by CodeRabbit
Release Notes v1.1.0
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.