Refactor away from PairingEngine + misc type changes #61

ghost · 2021-12-16T00:25:22Z

Summary of changes:

Refactor anything that is only dependent on the scalar field to not depend on PairingEngine (unfortunately this means there's a lot of E::Fr -> F changes in the diff)
Move dependence on TEModelParams to other places and not Circuit/StandardComposer. In practice this means (I think) the implementation of the fixed-base ECC gadget is determined later, and the variable-base ECC gadget depends on the params but not the rest of the circuit.
Suggested change to remove the TranscriptWrapper type
Minor change to Permutation generic types (can be reversed)
Some other minor tweaks to types (can be reversed). I was mostly just experimenting to see what might be slightly cleaner.

In summary this PR is not really ready to be merged, but open to input and feedback.

bhgomes · 2021-12-16T00:53:26Z

Thanks for the PR! For the type parameters, can you use a where clause instead of declaring the type constraints inline at the declaration site, it’s easier to read/change this way and keeps it consistent with the rest of the library.

bhgomes · 2021-12-16T00:54:06Z

Also, if you think it's not ready for review you can mark it as draft so we know it's not ready from the PR list.

CPerezz · 2021-12-16T08:22:58Z

Hey @joebebel this looks really promising! Willing to see it completed!

Also, as @bhgomes suggested, if you mark it as Draft for now while the PR is not ready would be nice! :)

CPerezz · 2021-12-16T17:48:27Z

@joebebel
Since FFtField does not imply PrimeField, how do we plan to ensure that the field is prime and also extract lots of the associated trait methods info for them?

Also, going this way will probably imply gate-featured verification & prover for IPA and KZG verifications. As now the verification always uses pairings and you don't want to enforce the PairingEngine.

ghost · 2021-12-28T11:57:37Z

Since FFtField does not imply PrimeField, how do we plan to ensure that the field is prime and also extract lots of the associated trait methods info for them?

we should just bind to FftField when we need those interfaces, and PrimeField when we need that. For practical purposes, I don't think any circuit will be over a non prime field. There is some theoretical possibility where non-fft fields might be desirable, but that seems like an extra complication for now.

Also, going this way will probably imply gate-featured verification & prover for IPA and KZG verifications. As now the verification always uses pairings and you don't want to enforce the PairingEngine.

As long as we're generic over the field and the PolynomialCommitment I think this should work. But it requires some moving of the Rust generics.

ghost · 2021-12-31T11:12:58Z

OK, this is getting closer. There are some problems:

For some reason, in SonicKZG and MarlinKZG the open operation on multiple polynomials is not as efficient as the random linear combination before. So more powers of tau in the setup are required. This regression is unfortunate, might require some further attention. I had to increase the setup size to get the tests to pass.
Rust type errors when using the ? operator, as it is unable to convert ark_poly_commit errors to ark_plonk errors implicitly. I tried several ways, I can't figure out how to get it to work, because the ark_poly_commit error is an associated type of PolynomialCommitment it's difficult to debug. For now I just .unwrap() instead, but this isn't a great long term solution.
I added back TEModelParameters as a generic for StandardComposer and other circuit related structs. It would be ideal to use ModelParameters instead (so that short weierstrass inner curves can be used) but after spending some hours trying to figure this out, it's difficult to do in a fully generic way.

I think other than the regression in the efficiency of the polynomial commitment and the error handling, it's ready to review for merge.

bhgomes · 2021-12-31T17:18:13Z

Rust type errors when using the ? operator, as it is unable to convert ark_poly_commit errors to ark_plonk errors implicitly. I tried several ways, I can't figure out how to get it to work, because the ark_poly_commit error is an associated type of PolynomialCommitment it's difficult to debug. For now I just .unwrap() instead, but this isn't a great long term solution.

Whenever an error type is an associated type of a trait, you cannot write a From impl since it would actually be generic over every type (since the compiler cannot prove that only some types can inhabit the associated type). So instead, I usually use this pattern:

let value = fn_returning_ark_poly_commit_error(x, y, z).map_err(ark_plonk::Error::CustomVariant)?;

It would be nice if the error could be converted with an Into<ark_plonk::Error> impl to remove the map_err but as I said above, this is not possible generally (again, this is only because the error we want to map is an associated type, otherwise this does work).

I added back TEModelParameters as a generic for StandardComposer and other circuit related structs. It would be ideal to use ModelParameters instead (so that short weierstrass inner curves can be used) but after spending some hours trying to figure this out, it's difficult to do in a fully generic way.

I think that the inner curve abstractions in arkworks are lacking here, and in general, there should be a trait that defines the addition and scalar multiplication laws for the curves directly, and the StandardComposer just queries that trait for implementing the circuit. Then, when constructing the circuit, you specify which instance of this trait you want, and the circuit is populated with the correct constraints. The arkworks abstractions are too low level to be leveraged to do this, since they are more about specifying the kind of curve (and performing the direct native computation), than specifying the algorithm itself at compile-time.

CPerezz

Just went over the surface and highlighted a few possible leftovers.

Will go in depth as soon as all is cleared! :)

benches/plonk.rs

CPerezz · 2022-01-05T11:57:36Z

plonk-core/src/constraint_system/logic.rs

        // assert_eq!(
        //     self.variables[&a].into_repr()
-        //         & (E::Fr::from(2u64).pow(&[(num_bits) as u64, 0, 0, 0])
-        //             - E::Fr::one())
+        //         & (F::from(2u64).pow(&[(num_bits) as u64, 0, 0, 0])
+        //             - F::one())
        //         .into_repr(),
        //     self.variables[&self.w_l[self.n - 1]]
        // );
        // assert_eq!(
        //     self.variables[&b]
-        //         & (E::Fr::from(2u64).pow(&[(num_bits) as u64, 0, 0, 0])
-        //             - E::Fr::one()),
+        //         & (F::from(2u64).pow(&[(num_bits) as u64, 0, 0, 0])
+        //             - F::one()),
        //     self.variables[&self.w_r[self.n - 1]]
        // );


We should file an issue for this.
(Not relevant to the PR)

CPerezz · 2022-01-05T11:58:31Z

plonk-core/src/proof_system/widget/ecc/fixed_base_scalar_mul.rs

+/*
+impl<F, P> GateConstraint<F> for FixedBaseScalarMul<F, P>
+where
+    F: Field,
+    P: SWModelParameters<BaseField = F>,
+{
+}*/


We need a better way of abstracting the embedded curve type

plonk-core/src/proof_system/widget/mod.rs

plonk-core/src/circuit.rs

Pratyush · 2022-01-05T21:37:08Z

For some reason, in SonicKZG and MarlinKZG the open operation on multiple polynomials is not as efficient as the random linear combination before. So more powers of tau in the setup are required. This regression is unfortunate, might require some further attention. I had to increase the setup size to get the tests to pass.

This should not be the case unless you're using hiding and/or strict degree bounds (which plonk doesn't need, AFAIK). I can hop on a call to help debug. btw you can turn on the print-trace feature on ark-poly-commit; that should give you a sense of why opening requires a larger SRS.

ghost · 2022-01-06T08:38:29Z

This should not be the case unless you're using hiding and/or strict degree bounds (which plonk doesn't need, AFAIK). I can hop on a call to help debug. btw you can turn on the print-trace feature on ark-poly-commit; that should give you a sense of why opening requires a larger SRS.

I don't know what I changed to fix it, but now it functions correctly. It wasn't a problem with the aggregation, though; the error was TooManyCoefficients { num_coefficients: 80, num_powers: 23 }. In this case the largest degree of the aggregated polynomials, t_4_poly was correctly 80, but only a degree 23 setup was generated. Strangely, t_4_poly was degree 23 in the previous proof in the same test. So somehow the CRS was not correctly sized.

LukePearson1 · 2022-01-06T09:33:48Z

When this ready for a review, even if still in draft, could you let us know and I'll give a first round? 😄 @joebebel

ghost · 2022-01-06T11:15:19Z

I believe this branch is now non-inferior (passes all tests, etc) to the main branch, so it is "ready for review".

There is one main deficiency, which is that it performs two PC::check operations (whereas the original effectively just did one). So slightly worse performance.

Thanks @Pratyush, I figured out what you meant about the trait bounding and it seems to work well. Also thanks @bhgomes, the error handling is better now.

davidnevadoc

What an amazing PR!

Just highlighted a couple of nits but overall I love the improvements introduced. Great job! :)

plonk-core/src/constraint_system/boolean.rs

plonk-core/src/circuit.rs

plonk-core/src/constraint_system/arithmetic.rs

CPerezz

Some nits and questions.

Aside from that, this is a brilliant work @joebebel.
Congratulations and big thanks for this huge improvement! 🥇

plonk-core/src/error.rs

plonk-core/src/proof_system/preprocess.rs

plonk-core/src/proof_system/proof.rs

plonk-core/src/proof_system/prover.rs

plonk-core/src/test.rs

plonk-core/src/transcript.rs

plonk-core/src/proof_system/preprocess.rs

CPerezz

A couple unused things in proof.rs
Once fixed the CI should pass.

Once it passes, feel free to merge!
Awesome job with this PR @joebebel !!!!!

ghost · 2022-01-11T10:19:55Z

OK, ready to merge

refactor

1cb1d06

ghost changed the title ~~Refactor away from PairingEngine + misc type changes~~ Draft: Refactor away from PairingEngine + misc type changes Dec 16, 2021

CPerezz marked this pull request as draft December 22, 2021 08:24

CPerezz mentioned this pull request Dec 22, 2021

Revert "Move to Workspace" #69

Merged

joe bebel added 7 commits December 29, 2021 17:23

use ark-poly-commit

f386832

fix bench

639c5c5

cleanups

9b2ddef

merged upstream

e1933f3

fixes

6f6de8a

fixes

b923262

ipa tests

a7cc73e

joe bebel added 2 commits January 5, 2022 00:44

merge

9e57211

error and aggregate

dc22df8

CPerezz reviewed Jan 5, 2022

View reviewed changes

Pratyush reviewed Jan 5, 2022

View reviewed changes

plonk-core/src/circuit.rs Outdated Show resolved Hide resolved

joe bebel added 5 commits January 5, 2022 17:42

cleanup

48dc7ca

fix doctest

e7bdc57

fix bench

27db0bd

setup bounds

de45372

trim bound

686e932

Use ark_poly_commit aggregation

3d0826a

ghost changed the title ~~Draft: Refactor away from PairingEngine + misc type changes~~ Refactor away from PairingEngine + misc type changes Jan 6, 2022

serde test

1c8566c

derive with trait bounds

3099bea

ghost marked this pull request as ready for review January 6, 2022 11:15

joe bebel added 10 commits January 6, 2022 03:18

nightly fmt, remove ruffini

80c21e0

fix ci checks

44b65d9

fix ci checks

192c98d

fix ci checks

6f3b7b3

fix ci checks

c1afa1e

fix ci checks

ed7145e

verifierkey

7b1c4ad

merge latest

433e918

rustdoc

a8272d4

rustdoc

9635c30

CPerezz requested a review from davidnevadoc January 7, 2022 11:59

davidnevadoc approved these changes Jan 10, 2022

View reviewed changes

plonk-core/src/constraint_system/boolean.rs Outdated Show resolved Hide resolved

plonk-core/src/circuit.rs Show resolved Hide resolved

plonk-core/src/constraint_system/arithmetic.rs Outdated Show resolved Hide resolved

CPerezz mentioned this pull request Jan 10, 2022

Find workarround for bitshifting ops in logic gate assertion. #85

Open

CPerezz requested changes Jan 10, 2022

View reviewed changes

joe bebel added 2 commits January 11, 2022 00:48

requsted changes

d88924e

requsted changes

294b4f1

CPerezz mentioned this pull request Jan 11, 2022

Error handling for InvalidEvalDomain improvement #86

Open

CPerezz approved these changes Jan 11, 2022

View reviewed changes

requsted changes

a24a872

LukePearson1 requested review from LukePearson1 and removed request for LukePearson1 January 11, 2022 11:32

CPerezz merged commit 632afc7 into ZK-Garage:master Jan 11, 2022

markulf mentioned this pull request Jan 30, 2022

Support Weierstrass inner curves in StandardComposer #108

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor away from PairingEngine + misc type changes #61

Refactor away from PairingEngine + misc type changes #61

ghost commented Dec 16, 2021

bhgomes commented Dec 16, 2021

bhgomes commented Dec 16, 2021

CPerezz commented Dec 16, 2021

CPerezz commented Dec 16, 2021

ghost commented Dec 28, 2021

ghost commented Dec 31, 2021

bhgomes commented Dec 31, 2021 •

edited

Loading

CPerezz left a comment •

edited by markulf

Loading

CPerezz Jan 5, 2022

CPerezz Jan 5, 2022

ghost Jan 5, 2022

Pratyush commented Jan 5, 2022 •

edited

Loading

ghost commented Jan 6, 2022

LukePearson1 commented Jan 6, 2022

ghost commented Jan 6, 2022

davidnevadoc left a comment •

edited

Loading

CPerezz left a comment

CPerezz left a comment

ghost commented Jan 11, 2022

Refactor away from PairingEngine + misc type changes #61

Refactor away from PairingEngine + misc type changes #61

Conversation

ghost commented Dec 16, 2021

bhgomes commented Dec 16, 2021

bhgomes commented Dec 16, 2021

CPerezz commented Dec 16, 2021

CPerezz commented Dec 16, 2021

ghost commented Dec 28, 2021

ghost commented Dec 31, 2021

bhgomes commented Dec 31, 2021 • edited Loading

CPerezz left a comment • edited by markulf Loading

Choose a reason for hiding this comment

CPerezz Jan 5, 2022

Choose a reason for hiding this comment

CPerezz Jan 5, 2022

Choose a reason for hiding this comment

ghost Jan 5, 2022

Choose a reason for hiding this comment

Pratyush commented Jan 5, 2022 • edited Loading

ghost commented Jan 6, 2022

LukePearson1 commented Jan 6, 2022

ghost commented Jan 6, 2022

davidnevadoc left a comment • edited Loading

Choose a reason for hiding this comment

CPerezz left a comment

Choose a reason for hiding this comment

CPerezz left a comment

Choose a reason for hiding this comment

ghost commented Jan 11, 2022

bhgomes commented Dec 31, 2021 •

edited

Loading

CPerezz left a comment •

edited by markulf

Loading

Pratyush commented Jan 5, 2022 •

edited

Loading

davidnevadoc left a comment •

edited

Loading