Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #594 +/- ##
==========================================
+ Coverage 96.72% 96.78% +0.05%
==========================================
Files 224 228 +4
Lines 29867 30080 +213
==========================================
+ Hits 28890 29113 +223
+ Misses 977 967 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implementation SummaryChanges
Deviations from Plan
Open Questions
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new MultivariateQuadratic (MQ) problem model — a system of quadratic polynomial equations over a finite field F_q — to the problem reductions library. The MQ problem is fundamental in post-quantum cryptography and is NP-complete over F_2. The implementation follows the existing codebase patterns for model registration, CLI integration, and testing.
Changes:
- New
MultivariateQuadraticandQuadraticPolytypes with evaluate logic,Problem/SatisfactionProblemtrait implementations, schema registration, and variant declaration. - CLI integration: dispatch for load/serialize, alias registration (
MQ), and explicit bail for CLI creation (complex input structure). - Comprehensive unit tests covering F_2 and F_3 fields, brute-force solving, serialization, edge cases (empty equations, contradictory systems).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/models/algebraic/multivariate_quadratic.rs |
Core model: QuadraticPoly, MultivariateQuadratic, Problem/SatisfactionProblem impls, schema registration, variants |
src/models/algebraic/mod.rs |
Module declaration and re-export of new types |
src/models/mod.rs |
Re-export MultivariateQuadratic and QuadraticPoly from top-level models |
problemreductions-cli/src/dispatch.rs |
CLI dispatch for loading and serializing MQ instances |
problemreductions-cli/src/problem_name.rs |
Alias MQ → MultivariateQuadratic |
problemreductions-cli/src/commands/create.rs |
Bail for CLI creation (complex input) |
docs/src/reductions/problem_schemas.json |
Schema entry for documentation |
src/unit_tests/models/algebraic/multivariate_quadratic.rs |
Unit tests for the new model |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn new(field_size: usize, num_variables: usize, equations: Vec<QuadraticPoly>) -> Self { | ||
| Self { | ||
| field_size, | ||
| num_variables, | ||
| equations, | ||
| } |
There was a problem hiding this comment.
The constructor does not validate that field_size >= 2. A field_size of 0 will cause a division-by-zero panic in QuadraticPoly::evaluate (via % q), and field_size of 1 produces a trivial/degenerate field. Other model constructors in this codebase validate their inputs (e.g., Knapsack::new asserts matching lengths). Consider adding assert!(field_size >= 2, "field_size must be at least 2") here.
| let mut result = self.constant % q; | ||
| for &((j, k), coeff) in &self.quadratic_terms { | ||
| result = | ||
| (result + coeff % q * (config[j] as u64 % q) % q * (config[k] as u64 % q) % q) % q; | ||
| } | ||
| for &(j, coeff) in &self.linear_terms { | ||
| result = (result + coeff % q * (config[j] as u64 % q) % q) % q; | ||
| } | ||
| result |
There was a problem hiding this comment.
The modular arithmetic here is incorrect due to operator precedence. In Rust, * and % have the same precedence and are left-associative, so coeff % q * (config[j] as u64 % q) % q * (config[k] as u64 % q) % q parses as:
((((coeff % q) * (config[j] as u64 % q)) % q) * (config[k] as u64 % q)) % q
While this happens to produce the correct mathematical result, the addition step is problematic: result + <quadratic_term_value> is computed before the final % q, but result could be up to q-1 and the quadratic term value up to q-1, so their sum could be up to 2*(q-1), which is fine for u64. However, the expression is extremely fragile and hard to reason about.
More importantly, the expression coeff % q * (config[j] as u64 % q) could overflow u64 when q is large (e.g., close to u32::MAX or larger), since both operands can be up to q-1 and their product up to (q-1)^2. Consider using explicit parentheses and intermediate variables for clarity and to make overflow behavior obvious. For example, compute each multiplication followed by a modulus operation with explicit grouping.
| let mut result = self.constant % q; | |
| for &((j, k), coeff) in &self.quadratic_terms { | |
| result = | |
| (result + coeff % q * (config[j] as u64 % q) % q * (config[k] as u64 % q) % q) % q; | |
| } | |
| for &(j, coeff) in &self.linear_terms { | |
| result = (result + coeff % q * (config[j] as u64 % q) % q) % q; | |
| } | |
| result | |
| let q128 = q as u128; | |
| let mut result = (self.constant % q) as u128; | |
| for &((j, k), coeff) in &self.quadratic_terms { | |
| let coeff_mod = (coeff % q) as u128; | |
| let xj = (config[j] as u64 % q) as u128; | |
| let xk = (config[k] as u64 % q) as u128; | |
| let term = (((coeff_mod * xj) % q128) * xk) % q128; | |
| result = (result + term) % q128; | |
| } | |
| for &(j, coeff) in &self.linear_terms { | |
| let coeff_mod = (coeff % q) as u128; | |
| let xj = (config[j] as u64 % q) as u128; | |
| let term = (coeff_mod * xj) % q128; | |
| result = (result + term) % q128; | |
| } | |
| (result % q128) as u64 |
Address Copilot review comments: - Add assert!(field_size >= 2) in MultivariateQuadratic::new() to prevent division-by-zero panic from field_size=0 - Use u128 intermediate arithmetic in QuadraticPoly::evaluate() to prevent u64 overflow when field_size is large Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Serde's derive(Deserialize) bypassed the field_size >= 2 assertion in MultivariateQuadratic::new(), allowing invalid instances to be loaded from JSON. Use #[serde(try_from)] with a raw helper struct to enforce the invariant on both construction and deserialization paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implementation SummaryChanges
Deviations from Plan
Open Questions
|
- Add display-name and problem-def entry in docs/paper/reductions.typ - Add MQ to CLI help table (via reduction only) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implementation Summary (completion pass)Changes Added
Deviations from Plan
Open Questions
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Pipeline Report
Agentic Test Highlights
Known Limitations (expected for [Model] PR)
🤖 Generated by review-pipeline |
- Remove `field_size` field from struct; hardcode F_2 - Change QuadraticPoly: quadratic_terms to Vec<(usize, usize)>, linear_terms to Vec<usize>, constant to bool - Simplify evaluate() to use XOR/AND (mod 2 arithmetic) - Rename getter num_vars() to num_variables(); remove field_size() - Update dims() to return vec![2; n] (hardcoded F_2) - Remove F_3 test; add constant-equation test - Update declare_variants! complexity to "1.6181^num_variables" - Update problem_schemas.json and paper definition - Remove serde TryFrom validation (no longer needed without field_size) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes #129