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

feat: allow using custom R1CS to QAP reductions #34

Merged
merged 14 commits into from
Aug 10, 2021

Conversation

gakonst
Copy link
Contributor

@gakonst gakonst commented Jul 20, 2021

Description

As discussed, it is potentially useful to allow customizing the R1CS to QAP conversion, for cases such as Circom's where they skip some of the FFTs.

Implementation

This PR abstracts the R1CS to QAP to a trait and exposes methods for providing implementors of the trait. It maintains backwards compat.

Comment on lines 44 to 46
pub(crate) struct R1CStoQAP;
pub struct R1CStoQAP;

impl R1CStoQAP {
pub trait QAPCalculator {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I would rename the trait to R1CSToQAP, and change the struct name to something more specific; maybe LibsnarkReduction, and the new circom one would be CircomReduction?

Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

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

LGTM modulo the naming change!

@Pratyush
Copy link
Member

Actually another suggestion: maybe we can parameterize the Groth16 struct itself by a new generic type parameter bounded by the R1CSToQap trait, and set the default to the old version? Do you think that would be cleaner?

@gakonst
Copy link
Contributor Author

gakonst commented Aug 9, 2021

Addressed your comments!

Actually another suggestion: maybe we can parameterize the Groth16 struct itself by a new generic type parameter bounded by the R1CSToQap trait, and set the default to the old version? Do you think that would be cleaner?

I think we should be considering a larger refactor (which I've started here) so as to allow the prover to cache intermediate steps, which should also help with having a cleaner abstraction around the QAP type

src/r1cs_to_qap.rs Outdated Show resolved Hide resolved
@Pratyush
Copy link
Member

Also, for the documentation, should we add the details of the reduction as described in libsnark: https://github.com/scipr-lab/libsnark/blob/2af440246fa2c3d0b1b0a425fb6abd8cc8b9c54d/libsnark/reductions/r1cs_to_qap/r1cs_to_qap.tcc

@Pratyush
Copy link
Member

I updated the API names slightly @gakonst, but this is good to merge otherwise. Any comments?

@Pratyush Pratyush changed the title feat: allow using custom R1CS to QAP conversions feat: allow using custom R1CS to QAP reductions Aug 10, 2021
@Pratyush Pratyush merged commit d86285f into arkworks-rs:master Aug 10, 2021
@gakonst gakonst deleted the feat/customizable-r1cs-to-qap branch August 10, 2021 23:29
@Pratyush Pratyush mentioned this pull request Dec 5, 2022
4 tasks
@Pratyush Pratyush mentioned this pull request Feb 3, 2023
6 tasks
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.

3 participants