Skip to content

Commit

Permalink
chore: loosen trait bounds on impls depending on AcirField (#5115)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

This PR is some code-organisation after #5114, rather than having a
single big impl with an `AcirField` bound we can break these down into
smaller impls with looser bounds.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench authored Jun 5, 2024
1 parent 6d3e732 commit b8ba9e6
Show file tree
Hide file tree
Showing 11 changed files with 682 additions and 695 deletions.
4 changes: 2 additions & 2 deletions acvm-repo/acir/src/circuit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ impl FromStr for OpcodeLocation {
}
}

impl<F: AcirField> Circuit<F> {
impl<F> Circuit<F> {
pub fn num_vars(&self) -> u32 {
self.current_witness_index + 1
}
Expand Down Expand Up @@ -425,7 +425,7 @@ mod tests {
};
let program = Program { functions: vec![circuit], unconstrained_functions: Vec::new() };

fn read_write<F: AcirField + Serialize + for<'a> Deserialize<'a>>(
fn read_write<F: Serialize + for<'a> Deserialize<'a>>(
program: Program<F>,
) -> (Program<F>, Program<F>) {
let bytes = Program::serialize_program(&program);
Expand Down
130 changes: 33 additions & 97 deletions acvm-repo/acir/src/native_types/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,29 +42,12 @@ impl<F: AcirField> std::fmt::Display for Expression<F> {
}
}

impl<F: AcirField> Expression<F> {
// TODO: possibly remove, and move to noir repo.
pub const fn can_defer_constraint(&self) -> bool {
false
}

impl<F> Expression<F> {
/// Returns the number of multiplication terms
pub fn num_mul_terms(&self) -> usize {
self.mul_terms.len()
}

pub fn from_field(q_c: F) -> Self {
Self { q_c, ..Default::default() }
}

pub fn one() -> Self {
Self::from_field(F::one())
}

pub fn zero() -> Self {
Self::default()
}

/// Adds a new linear term to the `Expression`.
pub fn push_addition_term(&mut self, coefficient: F, variable: Witness) {
self.linear_combinations.push((coefficient, variable));
Expand All @@ -85,6 +68,19 @@ impl<F: AcirField> Expression<F> {
self.mul_terms.is_empty() && self.linear_combinations.is_empty()
}

/// Returns a `FieldElement` if the expression represents a constant polynomial.
/// Otherwise returns `None`.
///
/// Examples:
/// - f(x,y) = x would return `None`
/// - f(x,y) = x + 6 would return `None`
/// - f(x,y) = 2*y + 6 would return `None`
/// - f(x,y) = x + y would return `None`
/// - f(x,y) = 5 would return `FieldElement(5)`
pub fn to_const(&self) -> Option<&F> {
self.is_const().then_some(&self.q_c)
}

/// Returns `true` if highest degree term in the expression is one or less.
///
/// - `mul_term` in an expression contains degree-2 terms
Expand Down Expand Up @@ -122,21 +118,29 @@ impl<F: AcirField> Expression<F> {
self.is_linear() && self.linear_combinations.len() == 1
}

/// Sorts opcode in a deterministic order
/// XXX: We can probably make this more efficient by sorting on each phase. We only care if it is deterministic
pub fn sort(&mut self) {
self.mul_terms.sort_by(|a, b| a.1.cmp(&b.1).then(a.2.cmp(&b.2)));
self.linear_combinations.sort_by(|a, b| a.1.cmp(&b.1));
}
}

impl<F: AcirField> Expression<F> {
pub fn from_field(q_c: F) -> Self {
Self { q_c, ..Default::default() }
}

pub fn zero() -> Self {
Self::default()
}

pub fn is_zero(&self) -> bool {
*self == Self::zero()
}

/// Returns a `FieldElement` if the expression represents a constant polynomial.
/// Otherwise returns `None`.
///
/// Examples:
/// - f(x,y) = x would return `None`
/// - f(x,y) = x + 6 would return `None`
/// - f(x,y) = 2*y + 6 would return `None`
/// - f(x,y) = x + y would return `None`
/// - f(x,y) = 5 would return `FieldElement(5)`
pub fn to_const(&self) -> Option<F> {
self.is_const().then_some(self.q_c)
pub fn one() -> Self {
Self::from_field(F::one())
}

/// Returns a `Witness` if the `Expression` can be represented as a degree-1
Expand All @@ -161,74 +165,6 @@ impl<F: AcirField> Expression<F> {
None
}

/// Sorts opcode in a deterministic order
/// XXX: We can probably make this more efficient by sorting on each phase. We only care if it is deterministic
pub fn sort(&mut self) {
self.mul_terms.sort_by(|a, b| a.1.cmp(&b.1).then(a.2.cmp(&b.2)));
self.linear_combinations.sort_by(|a, b| a.1.cmp(&b.1));
}

/// Checks if this expression can fit into one arithmetic identity
/// TODO: This needs to be reworded, arithmetic identity only makes sense in the context
/// TODO of PLONK, whereas we want expressions to be generic.
/// TODO: We just need to reword it to say exactly what its doing and
/// TODO then reference the fact that this is what plonk will accept.
/// TODO alternatively, we can define arithmetic identity in the context of expressions
/// TODO and then reference that.
pub fn fits_in_one_identity(&self, width: usize) -> bool {
// A Polynomial with more than one mul term cannot fit into one opcode
if self.mul_terms.len() > 1 {
return false;
};
// A Polynomial with more terms than fan-in cannot fit within a single opcode
if self.linear_combinations.len() > width {
return false;
}

// A polynomial with no mul term and a fan-in that fits inside of the width can fit into a single opcode
if self.mul_terms.is_empty() {
return true;
}

// A polynomial with width-2 fan-in terms and a single non-zero mul term can fit into one opcode
// Example: Axy + Dz . Notice, that the mul term places a constraint on the first two terms, but not the last term
// XXX: This would change if our arithmetic polynomial equation was changed to Axyz for example, but for now it is not.
if self.linear_combinations.len() <= (width - 2) {
return true;
}

// We now know that we have a single mul term. We also know that the mul term must match up with two other terms
// A polynomial whose mul terms are non zero which do not match up with two terms in the fan-in cannot fit into one opcode
// An example of this is: Axy + Bx + Cy + ...
// Notice how the bivariate monomial xy has two univariate monomials with their respective coefficients
// XXX: note that if x or y is zero, then we could apply a further optimization, but this would be done in another algorithm.
// It would be the same as when we have zero coefficients - Can only work if wire is constrained to be zero publicly
let mul_term = &self.mul_terms[0];

// The coefficient should be non-zero, as this method is ran after the compiler removes all zero coefficient terms
assert_ne!(mul_term.0, F::zero());

let mut found_x = false;
let mut found_y = false;

for term in self.linear_combinations.iter() {
let witness = &term.1;
let x = &mul_term.1;
let y = &mul_term.2;
if witness == x {
found_x = true;
};
if witness == y {
found_y = true;
};
if found_x & found_y {
break;
}
}

found_x & found_y
}

/// Returns `self + k*b`
pub fn add_mul(&self, k: F, b: &Self) -> Self {
if k.is_zero() {
Expand Down
8 changes: 3 additions & 5 deletions acvm-repo/acir/src/native_types/expression/ordering.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use acir_field::AcirField;

use crate::native_types::Witness;
use std::cmp::Ordering;

Expand All @@ -8,7 +6,7 @@ use super::Expression;
// TODO: It's undecided whether `Expression` should implement `Ord/PartialOrd`.
// This is currently used in ACVM in the compiler.

impl<F: AcirField> Ord for Expression<F> {
impl<F: Ord> Ord for Expression<F> {
fn cmp(&self, other: &Self) -> Ordering {
let mut i1 = self.get_max_idx();
let mut i2 = other.get_max_idx();
Expand All @@ -25,7 +23,7 @@ impl<F: AcirField> Ord for Expression<F> {
}
}

impl<F: AcirField> PartialOrd for Expression<F> {
impl<F: Ord> PartialOrd for Expression<F> {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
Expand All @@ -37,7 +35,7 @@ struct WitnessIdx {
second_term: bool,
}

impl<F: AcirField> Expression<F> {
impl<F: Ord> Expression<F> {
fn get_max_idx(&self) -> WitnessIdx {
WitnessIdx {
linear: self.linear_combinations.len(),
Expand Down
4 changes: 0 additions & 4 deletions acvm-repo/acir/src/native_types/witness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ impl Witness {
// This is safe as long as the architecture is 32bits minimum
self.0 as usize
}

pub const fn can_defer_constraint(&self) -> bool {
true
}
}

impl From<u32> for Witness {
Expand Down
Loading

0 comments on commit b8ba9e6

Please sign in to comment.