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

Add Constraint and Selector types #606

Merged
merged 1 commit into from
Oct 8, 2021
Merged

Conversation

vlopes11
Copy link
Contributor

@vlopes11 vlopes11 commented Oct 7, 2021

Constraint will introduce a fail-safe type that can be used to
describe a circuit. It will replace any composer argument that takes the
circuit description/selector polynomials.

Selector will allow the user to extract specific coefficients from the
constraint representation.

Resolves #608

@vlopes11 vlopes11 added the team:Core Low Level Core Development Team (Rust) label Oct 7, 2021
@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #606 (a287df1) into add_lookups (481b119) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head a287df1 differs from pull request most recent head b217133. Consider uploading reports for the commit b217133 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           add_lookups     #606      +/-   ##
===============================================
+ Coverage        97.76%   97.80%   +0.04%     
===============================================
  Files               52       53       +1     
  Lines             4076     4105      +29     
===============================================
+ Hits              3985     4015      +30     
+ Misses              91       90       -1     
Impacted Files Coverage Δ
src/constraint_system/logic.rs 96.15% <ø> (ø)
src/circuit.rs 93.61% <100.00%> (+0.13%) ⬆️
src/constraint_system/arithmetic.rs 100.00% <100.00%> (+1.08%) ⬆️
src/constraint_system/boolean.rs 100.00% <100.00%> (ø)
src/constraint_system/composer.rs 97.65% <100.00%> (+0.23%) ⬆️
src/constraint_system/constraint.rs 100.00% <100.00%> (ø)
...nt_system/ecc/curve_addition/variable_base_gate.rs 100.00% <100.00%> (ø)
src/constraint_system/ecc/scalar_mul/fixed_base.rs 99.17% <100.00%> (ø)
src/constraint_system/helper.rs 100.00% <100.00%> (ø)
src/permutation.rs 99.05% <100.00%> (+<0.01%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 481b119...b217133. Read the comment docs.

@vlopes11 vlopes11 requested review from ZER0 and ureeves October 8, 2021 07:03
@vlopes11 vlopes11 changed the title Add Selector for gate API polynomials Add Constraint and Selector types Oct 8, 2021
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Love the simplification in appending gates.

Copy link
Contributor

@ZER0 ZER0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits, plus we should add the documentation also to internal methods, not only public ones.

src/constraint_system/constraint.rs Outdated Show resolved Hide resolved
self
}

fn copy_public_selectors(mut self, rhs: &Self) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is only for internal use, but I still don't like it much. Also, if it's just used internally, it might be very well be an associated function instead of a method, since it looks like to me we always use in conjunction with Self::default(), e.g.:

   pub(crate) fn range(s: &Self) -> Self {
        Self::default()
            .copy_public_selectors(s)
            .set(Selector::Range, 1)
    }

So what about having something like:

   pub(crate) fn range(s: &Self) -> Self {
        Self::from_public_selectors(s)
            .set(Selector::Range, 1)
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would conflict with the idea of creating the arithmetic constraint exclusively from Self::arithmetic.

`Constraint` will introduce a fail-safe type that can be used to
describe a circuit. It will replace any composer argument that takes the
circuit description/selector polynomials.

Resolves #608
@vlopes11 vlopes11 merged commit 7782406 into add_lookups Oct 8, 2021
@vlopes11 vlopes11 deleted the vlopes11/selector branch October 8, 2021 13:17
@ZER0 ZER0 linked an issue Oct 8, 2021 that may be closed by this pull request

use dusk_bls12_381::BlsScalar;

/// Index the coefficients in a polynomial description of the circuit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// Selectors use to address a coefficient inside a [Constraint]


// Compute the inverse
let inv_x_denom =
composer.evaluate_witness(&x_denominator).invert().unwrap();
let inv_x_denom = composer.append_witness(inv_x_denom);

let constraint = Constraint::new().mul(1).constant(-BlsScalar::one());
Copy link
Contributor

@ZER0 ZER0 Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x = (1, a, b)
y = (1, c)

enum Wire {
   Mult(BlsScalar, Witness, Witness),
   Left(BlsScalar, Witness),
}

Constraint::new()
          .set(Wire::Mult(x, a, b))
          .set(Wire::Left(y, c))
          .constant(z);

qM * a * b + qL * c + z = 0

Constraint::new().mul(x, a, b).left(y, c).constant(-BlsScalar::one());

vlopes11 added a commit that referenced this pull request Oct 18, 2021
`Constraint` will introduce a fail-safe type that can be used to
describe a circuit. It will replace any composer argument that takes the
circuit description/selector polynomials.

Resolves #608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:Core Low Level Core Development Team (Rust)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Constraint to describe the circuit
3 participants