From fad77555ad4d822d5819a49a2bb9772c9fab004e Mon Sep 17 00:00:00 2001 From: chriseth Date: Sat, 20 Sep 2025 19:58:29 +0200 Subject: [PATCH] Reduce visibility. (#3295) This PR changes `pub(crate)` visibility of the fields of `GroupedExpression` back to `non-pub` as it was before the extraction of the solving routines. The solving routines do not really need write access to those fields and read-access is obtained using `components()`. --- .../src/algebraic_constraint/solve.rs | 81 ++++++++++--------- constraint-solver/src/constraint_system.rs | 5 +- constraint-solver/src/grouped_expression.rs | 17 ++-- .../src/solver/quadratic_equivalences.rs | 5 +- 4 files changed, 62 insertions(+), 46 deletions(-) diff --git a/constraint-solver/src/algebraic_constraint/solve.rs b/constraint-solver/src/algebraic_constraint/solve.rs index 1c7ae7ccd..cf4a91b8b 100644 --- a/constraint-solver/src/algebraic_constraint/solve.rs +++ b/constraint-solver/src/algebraic_constraint/solve.rs @@ -83,26 +83,27 @@ where /// `variable` does not appear in the quadratic component and /// has a coefficient which is known to be not zero. /// + /// If the constraint has the form `A + k * x = 0` where `A` does not + /// contain the variable `x` and `k` is a non-zero runtime constant, + /// it returns `A * (-k^(-1))`. + /// /// Returns the resulting solved grouped expression. pub fn try_solve_for(&self, variable: &V) -> Option> { - let expression = self.expression; + let coefficient = self + .expression + .coefficient_of_variable_in_affine_part(variable)?; + if !coefficient.is_known_nonzero() { + return None; + } - if expression - .quadratic - .iter() - .flat_map(|(l, r)| [l, r]) - .flat_map(|c| c.referenced_unknown_variables()) - .contains(variable) - { - // The variable is in the quadratic component, we cannot solve for it. + let subtracted = self.expression.clone() + - GroupedExpression::from_unknown_variable(variable.clone()) * coefficient.clone(); + if subtracted.referenced_unknown_variables().contains(variable) { + // There is another occurrence of the variable in the quadratic component, + // we cannot solve for it. return None; } - if !expression.linear.get(variable)?.is_known_nonzero() { - return None; - } - let mut result = self.expression.clone(); - let coefficient = result.linear.remove(variable)?; - Some(result * (-T::one().field_div(&coefficient))) + Some(subtracted * (-T::one().field_div(coefficient))) } /// Algebraically transforms the constraint such that `self = 0` is equivalent @@ -128,10 +129,14 @@ where let normalization_factor = expr .referenced_unknown_variables() .find_map(|var| { - let coeff = expression.coefficient_of_variable(var)?; + let coeff = expression.coefficient_of_variable_in_affine_part(var)?; // We can only divide if we know the coefficient is non-zero. if coeff.is_known_nonzero() { - Some(expr.coefficient_of_variable(var).unwrap().field_div(coeff)) + Some( + expr.coefficient_of_variable_in_affine_part(var) + .unwrap() + .field_div(coeff), + ) } else { None } @@ -159,10 +164,10 @@ where &self, range_constraints: &impl RangeConstraintProvider, ) -> Result, Error> { - let expression = self.expression; + let (_, mut linear, constant) = self.expression.components(); - Ok(if expression.linear.len() == 1 { - let (var, coeff) = expression.linear.iter().next().unwrap(); + Ok(if linear.clone().count() == 1 { + let (var, coeff) = linear.next().unwrap(); // Solve "coeff * X + self.constant = 0" by division. assert!( !coeff.is_known_zero(), @@ -170,16 +175,16 @@ where ); if coeff.is_known_nonzero() { // In this case, we can always compute a solution. - let value = expression.constant.field_div(&-coeff.clone()); + let value = constant.field_div(&-coeff.clone()); ProcessResult::complete(vec![assignment_if_satisfies_range_constraints( var.clone(), value, range_constraints, )?]) - } else if expression.constant.is_known_nonzero() { + } else if constant.is_known_nonzero() { // If the offset is not zero, then the coefficient must be non-zero, // otherwise the constraint is violated. - let value = expression.constant.field_div(&-coeff.clone()); + let value = constant.field_div(&-coeff.clone()); ProcessResult::complete(vec![ Assertion::assert_is_nonzero(coeff.clone()), assignment_if_satisfies_range_constraints( @@ -208,13 +213,12 @@ where &self, range_constraints: &impl RangeConstraintProvider, ) -> Vec> { - let expression = self.expression; // Solve for each of the variables in the linear component and // compute the range constraints. - assert!(!expression.is_quadratic()); - expression - .linear - .iter() + assert!(!self.expression.is_quadratic()); + self.expression + .components() + .1 .filter_map(|(var, _)| { let rc = self.try_solve_for(var)?.range_constraint(range_constraints); Some((var, rc)) @@ -852,7 +856,7 @@ mod tests { expr.substitute_by_unknown(&"x", &subst); assert!(!expr.is_quadratic()); - assert_eq!(expr.linear.len(), 3); + assert_eq!(expr.components().1.count(), 3); assert_eq!(expr.to_string(), "a + b + y"); } @@ -874,14 +878,17 @@ mod tests { ); // Structural validation assert_eq!(quadratic.len(), 2); - assert_eq!(quadratic[0].0.to_string(), "(a) * (b) + 1"); - assert_eq!(quadratic[0].0.quadratic[0].0.to_string(), "a"); - assert_eq!(quadratic[0].0.quadratic[0].1.to_string(), "b"); - assert!(quadratic[0].0.linear.is_empty()); - assert_eq!( - quadratic[0].0.constant.try_to_number(), - Some(GoldilocksField::from(1)), - ); + { + assert_eq!(quadratic[0].0.to_string(), "(a) * (b) + 1"); + let (inner_quadratic, inner_linear, inner_constant) = quadratic[0].0.components(); + assert_eq!(inner_quadratic[0].0.to_string(), "a"); + assert_eq!(inner_quadratic[0].1.to_string(), "b"); + assert!(inner_linear.count() == 0); + assert_eq!( + inner_constant.try_to_number(), + Some(GoldilocksField::from(1)), + ); + } assert_eq!(quadratic[0].1.to_string(), "w"); assert_eq!(quadratic[1].0.to_string(), "a"); assert_eq!(quadratic[1].1.to_string(), "b"); diff --git a/constraint-solver/src/constraint_system.rs b/constraint-solver/src/constraint_system.rs index a974ef9a3..bb6a0b162 100644 --- a/constraint-solver/src/constraint_system.rs +++ b/constraint-solver/src/constraint_system.rs @@ -189,7 +189,10 @@ impl< // `k * var + e` is in range rc <=> // `var` is in range `(rc - RC[e]) / k` = `rc / k + RC[-e / k]` // If we solve `expr` for `var`, we get `-e / k`. - let k = expr.coefficient_of_variable(var).unwrap().try_to_number()?; + let k = expr + .coefficient_of_variable_in_affine_part(var) + .unwrap() + .try_to_number()?; let expr = AlgebraicConstraint::assert_zero(expr).try_solve_for(var)?; let rc = rc .multiple(T::FieldType::from(1) / k) diff --git a/constraint-solver/src/grouped_expression.rs b/constraint-solver/src/grouped_expression.rs index e7c290777..470b2db3d 100644 --- a/constraint-solver/src/grouped_expression.rs +++ b/constraint-solver/src/grouped_expression.rs @@ -39,12 +39,12 @@ pub struct GroupedExpression { /// Quadratic terms of the form `a * X * Y`, where `a` is a (symbolically) /// known value and `X` and `Y` are grouped expressions that /// have at least one unknown. - pub(crate) quadratic: Vec<(Self, Self)>, + quadratic: Vec<(Self, Self)>, /// Linear terms of the form `a * X`, where `a` is a (symbolically) known /// value and `X` is an unknown variable. - pub(crate) linear: BTreeMap, + linear: BTreeMap, /// Constant term, a (symbolically) known value. - pub(crate) constant: T, + constant: T, } impl, V> GroupedExpression { @@ -236,10 +236,13 @@ impl GroupedExpression { .unwrap() } - /// Returns the coefficient of the variable `variable` if this is an affine expression. - /// Panics if the expression is quadratic. - pub fn coefficient_of_variable<'a>(&'a self, var: &V) -> Option<&'a T> { - assert!(!self.is_quadratic()); + /// Returns the coefficient of the variable `variable` in the affine part of this + /// expression. + /// If the expression is affine, this is the actual coefficient of the variable + /// in the expression. Otherwise, the quadratic part of the expression could + /// also contain the variable and thus the actual coefficient might be different + /// (even zero). + pub fn coefficient_of_variable_in_affine_part<'a>(&'a self, var: &V) -> Option<&'a T> { self.linear.get(var) } diff --git a/constraint-solver/src/solver/quadratic_equivalences.rs b/constraint-solver/src/solver/quadratic_equivalences.rs index 8a5ab0e7e..062276798 100644 --- a/constraint-solver/src/solver/quadratic_equivalences.rs +++ b/constraint-solver/src/solver/quadratic_equivalences.rs @@ -128,7 +128,10 @@ impl QuadraticEqualityCandidate< /// Returns an equivalent candidate that is normalized /// such that `var` has a coefficient of `1`. fn normalized_for_var(&self, var: &V) -> Self { - let coefficient = self.expr.coefficient_of_variable(var).unwrap(); + let coefficient = self + .expr + .coefficient_of_variable_in_affine_part(var) + .unwrap(); // self represents // `(coeff * var + X) * (coeff * var + X + offset) = 0`