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

Fix/plonk constraints #186

Merged
merged 30 commits into from
Dec 2, 2021
Merged

Fix/plonk constraints #186

merged 30 commits into from
Dec 2, 2021

Conversation

ThomasPiellard
Copy link
Collaborator

@ThomasPiellard ThomasPiellard commented Nov 30, 2021

Summary

This PR changes the way PLONK circuits are compiled. It is now faster and every cases are covered, contrary to the previous attempt at reducing the number of constraints where some cases where ignored, resulting in blowing up the number of constraints in some "big" circuits (like scalar multiplication, pairing...).

Description

The previous solution relied on factoring linear expression in Groth16 to not split them twice in PLONK. Here it is a different and simpler approach. The API has now a method that returns the backendID. If the backendID is PLONK, then the ADD and SUB methods add a new variable and the linear expression is really recorded as a constraint (it is not in GROTH16). With that, when a linear expression is reused later in the circuit, then it is the variable equal to the sum that is used, so the linear expression is split only once. Several methods in the API needed to be refactored to avoid bugs or to decrease the number of constraints, and to be consistant with the splitting algo (splitting from r1cs to sparse_r1cs).

Internal changes

Some refactoring on the constraint system has been done, to put the right fields to the right place:

  • A Variable is a linear combination of Wires, and correspond to Variable as in a normal program
  • A Variable has a field IsBoolean (since Boolean constraint is on the Variable level)
  • A Variable no longer has a Visibility (Visibility is on the Wire level)
  • A Variable no longer has an ID (the ID is on the wire level, and used only for the routing of the wires)

Internal changes:

  • API has a method returning the backendID
  • ADD,SUB and AssertIsBoolean are implemented differently according to the backend
  • API: Select, Or, Xor have been written to ease up the conversion to sparse_r1cs
  • in a constraintSystem, public, secret are now just a slice of string corresponding to the names
  • in a constraintSystem, internal is now just a counter (int)
  • in cs.go: variables, inputs struct have been removed
  • in cs_to_r1cs_sparse: split method is simplified (no need to cover the list of sub linear expressions)
  • in cs_to_r1cs_sparse: r1cToSparseR1C method just switches over a finite number of possibilities
  • in cs_to_r1cs_sparse: all the logic to factorize linear expressions has been removed
  • VariableID --> WireID since ID is on the Wire level (a Variable does not have an ID)
  • Variable struct is now defined in compiled/variable.go
  • struct Variable now contains a *bool to monitor the Variables which are boolean constrained (NOTE: we can restore the map to track Variables which are boolean constrained, for that we need to put an ID field in Variable, but it should be explicit that this field is only to track boolean constraints, and not to track the ID of wires, which are completely different)

Status

  • TestCircuitStatistics fails due to a negligible delta (both + and -)
  • Remaining tests pass

Ex perfs:

  • compiling scalarMulG1 PLONK: 305765908 ns/op, 7928 constraints [develop] vs 10602651 ns/op, 7543 constraints[this branch]
  • compiling pairing PLONK: 1762081141 ns/op, 324432 constraints [develop] vs 67556893 ns/op, 64059 constraints [this branch]

Copy link
Collaborator

@gbotrel gbotrel left a comment

Choose a reason for hiding this comment

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

looks good, couple of comment in the review.

  • Also, refactoring error "compiled.Variable" appears multiple time in comments where "variable" was fine :)
  • dealing better with marking boolean constrain variables (ie Variable == LinearExpression) would remove also some of these changes (everywhere v.LinExp appears)
  • minor conflicts with develop to fix before merging

test/engine.go Show resolved Hide resolved
internal/backend/bls12-381/cs/r1cs.go Outdated Show resolved Hide resolved
integration_test.go Outdated Show resolved Hide resolved
frontend/cs_api.go Outdated Show resolved Hide resolved
frontend/cs_api.go Outdated Show resolved Hide resolved
internal/backend/compiled/variable.go Outdated Show resolved Hide resolved
type LinearExpression []Term

// Variable represent a linear expression of wires
type Variable struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

here I would just merge the compiled.Variable and compiled.LinearExpression. Ie define:

type Variable []Term

Managing boolean constrained variables with a pointer seems a bit risky to me; while it may not bug with our current API, cloning a Variable, adding terms to the linear expressions, boolean constraining it will incorrectly mark the first variable as boolean constrained.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can restore the map to track boolean constrained variables, however we still need to store an ID field in Variable, right?

frontend/cs_api_test.go Outdated Show resolved Hide resolved
@gbotrel gbotrel merged commit 0843573 into develop Dec 2, 2021
@gbotrel gbotrel deleted the fix/plonk_constraints branch December 2, 2021 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants