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

Commitment scheme abstraction #77

Closed
wants to merge 49 commits into from
Closed

Commitment scheme abstraction #77

wants to merge 49 commits into from

Conversation

kilic
Copy link

@kilic kilic commented Jun 2, 2022

Having pse/halo2curves which reimplements pse/pairing with upstream traits and also reexports zcash/pasta_curves it is now possible to keep original commitment scheme rather than replacing it.

Also since we won't have to deal with mimicking traits in pse/pasta_curves at pse/pairing there must be less diffs in this fork and won't require any maintenance effort regarding those traits.

This PR introduces an abstraction technique for commitment schemes. It keeps the original IPA and adds two KZG variants and allows circuit developer to specify a commitment scheme for a specific circuit without conditional compilation.

Regarding the changes at constraint system side most of them are for boundary definitions of functions which mostly specifies the commitment scheme. I don't think this abstraction change would make its way to the upstream. However it can be a discussion base for a working group if idea is found useful somehow.

For further rebasing and maintenance concerns:

mod poly:

30 files changed, 3365 insertions(+), 1411 deletions(-)

This is admittedly a huge diff. However current replacement approach change for �mod poly is also not small.

Integration for halo2_proofs except mod poly:

35 files changed, 623 insertions(+), 578 deletions(-)

There are some formatting/folding related differences that should reduce sense of this diff. There is also a ~100 liner block diff w/o an actual change. I think it is for an unrecognized line shift.

Integration for halo2_gadgets:

37 files changed, 117 insertions(+), 88 deletions(-)

Which looks ok.

@CPerezz CPerezz self-requested a review June 6, 2022 09:15
@CPerezz
Copy link
Member

CPerezz commented Jun 6, 2022

@kilic will take a look into this.

I'll try to see if there's a way on which we can do the same work you did here but resulting in less conflicts when merging upstream changes.

I'll need a day or two as I'll parallelize this with my keccak work :)

Copy link

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

Finished KZG part and abstraction on CommitmentScheme looks really great! Nice work!

Will review IPA next.

halo2_gadgets/benches/sha256.rs Show resolved Hide resolved
halo2_proofs/src/plonk/prover.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/kzg/multiopen/gwc.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/kzg/multiopen/shplonk.rs Outdated Show resolved Hide resolved
halo2_proofs/tests/plonk_api.rs Outdated Show resolved Hide resolved
halo2_proofs/tests/plonk_api.rs Show resolved Hide resolved
halo2_proofs/src/poly/query.rs Outdated Show resolved Hide resolved
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Missing gwc/verifier, multiopen and shplonk.

Will do them later today!

halo2_proofs/src/poly/strategy.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/ipa/commitment.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/ipa/strategy.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/ipa/strategy.rs Show resolved Hide resolved
halo2_proofs/src/poly/kzg/commitment.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/kzg/commitment.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/kzg/commitment.rs Outdated Show resolved Hide resolved

#[cfg(test)]
mod test {
// TODO: duplicate is in IPA side
Copy link
Member

Choose a reason for hiding this comment

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

I belive this is already done. So let's remove the comment.

Copy link
Author

@kilic kilic Jun 8, 2022

Choose a reason for hiding this comment

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

Actually contrary. It supposed to mean make these tests generic and use it across schemes. So still a TODO :)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm that's weird. I saw all the tests under ipa.rs. But maybe weren't the same ones.

So if the sentence meant something else then my bad.

If we want to abstract the tests over commitment scheme, here is a suggestion on how we do it at ZKGarage: https://github.com/ZK-Garage/plonk/blob/79dffa1bacbe73ab42e2d7e48194efe5c0070bd6/plonk-core/src/test.rs#L84

halo2_proofs/src/poly/kzg/multiopen/gwc.rs Outdated Show resolved Hide resolved
@kilic kilic force-pushed the abstraction branch 4 times, most recently from d13862e to 04ffc45 Compare June 10, 2022 10:25
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM.

Just a nit/suggestion.

halo2_proofs/src/poly/kzg/commitment.rs Show resolved Hide resolved
rust-toolchain Outdated Show resolved Hide resolved
@CPerezz
Copy link
Member

CPerezz commented Jun 16, 2022

Hey @kilic why did you rebase upstream head here??? I thought this was being done in another PR.

See: #64

@kilic
Copy link
Author

kilic commented Jun 16, 2022

@CPerezz

I thought In the end this PR should be on top of recent upstream and also under other changes that we introduce? It was already started 3 or 5 commits behind of the current upstream head.

@CPerezz
Copy link
Member

CPerezz commented Jun 16, 2022

@kilic makes sense to me. But we should ping @noctrlz since I think is working on it.

[book] Add `U` to `BCMS` comparison table
str4d and others added 7 commits June 21, 2022 15:04
Add `BatchVerifier::finalize_and_return_rng`
This means we only need to `Debug`-format the `PinnedVerificationKey`
once on construction, instead of once per proof.
This means we only compute the degree in a verification context during
construction, instead of twice per proof in the permutation argument.
Cache values in `VerifyingKey` that can be computed on construction
daira and others added 4 commits June 23, 2022 14:15
…ests.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
`BatchVerifier` now manages the entire batch verification process.
Individual proofs are verified on a threadpool, and the resulting MSMs
are then batch-checked as before. The addition of parallelism here
couples with zcash#608 to make parallelism less fine-grained and
reduce the overhead of multi-threading.
@CPerezz
Copy link
Member

CPerezz commented Jan 16, 2023

I belive this can be closed right @kilic @han0110 ??

@kilic kilic closed this Jan 17, 2023
mratsim pushed a commit to taikoxyz/halo2 that referenced this pull request Jun 26, 2023
jonathanpwang pushed a commit to axiom-crypto/halo2 that referenced this pull request Oct 26, 2023
jonathanpwang added a commit to axiom-crypto/halo2 that referenced this pull request Nov 18, 2023
…17)

* Resolve Prover optimization: memory reduction privacy-scaling-explorations#77 (#6)

* Resolve taikoxyz/zkevm-circuits#77

* Please Clippy

* fix: recursive FFT for lengths not `2^k, 2^extended_k`

Also fixed some issue when multicore feature is not on.

* fix: evaluation (cherry-pick
https://github.com/scroll-tech/halo2-gpu/blob/a3019b047ad7d7119103f4d9df3fd33ce1429f95/halo2_proofs/src/plonk/evaluation.rs)

* fix: `g_to_lagrange` uses inverse fft

* Bump version since this is a breaking change to pk

* chore: update halo2curves version

---------

Co-authored-by: einar-taiko <126954546+einar-taiko@users.noreply.github.com>
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.

7 participants