Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: loosen trait bounds on impls depending on AcirField #5115

Merged
merged 4 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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> 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 @@
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 @@
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 @@
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 Expand Up @@ -366,7 +302,7 @@
use acir_field::{AcirField, FieldElement};

#[test]
fn add_mul_smoketest() {

Check warning on line 305 in acvm-repo/acir/src/native_types/expression/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (smoketest)
let a = Expression {
mul_terms: vec![(FieldElement::from(2u128), Witness(1), Witness(2))],
..Default::default()
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
Loading