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

Refactor Arith trait #162

Merged

Conversation

winderica
Copy link
Contributor

@winderica winderica commented Sep 22, 2024

As the second part of #145, this PR improves the design of the Arith trait with a pair of generic witness and committed instance. The Dummy trait is also introduced, providing a unified interface for the dummy method.

Specifically, unlike the previous Arith that can only handle z (a vector of field elements) when checking satisfiability, now Arith and ArithGadget allow for taking the witness w and instance u as input, where w and u are bounded by generic types W and U:

  • When W and U are vectors of field elements, the behavior becomes the same as the previous one.
  • When W and U are Nova or ProtoGalaxy instances, Arith and ArithGadget are able to replace the RelaxedR1CS and RelaxedR1CSGadget.
  • We can further specify W and U are CCS instances, thereby replacing {CCCS, LCCCS}::check_relation and LCCCSCheckerGadget.

This is useful because in #145, we will build a generic decider circuit that essentially runs IVC.Verify (including ArithGadget::enforce_relation and commitment verification) in-circuit without knowing which types of folding scheme, witness, and committed instances are being used.

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.

I doubt a bit about the Dummy impls. And also about the RUNNING flag.

The rest seems a really good refactor! Thanks for it!

impl<F: PrimeField> Arith<F> for CCS<F> {
fn eval_relation(&self, z: &[F]) -> Result<Vec<F>, Error> {
impl<F: PrimeField> CCS<F> {
pub fn eval_core(&self, z: &[F]) -> Result<Vec<F>, Error> {
Copy link
Member

Choose a reason for hiding this comment

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

May I ask why we changed the name to eval_core or at least what core means within this context?
I think if we make the fn pub would be nice to have some minimal docs where we relate the core thing.

Copy link
Contributor Author

@winderica winderica Sep 23, 2024

Choose a reason for hiding this comment

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

What about eval_z (or eval_at_z)? Will also add the docs!

Copy link
Member

Choose a reason for hiding this comment

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

Both work for me. Definitely better than core.

folding-schemes/src/arith/ccs.rs Show resolved Hide resolved
folding-schemes/src/folding/hypernova/mod.rs Show resolved Hide resolved
folding-schemes/src/folding/nova/mod.rs Show resolved Hide resolved
folding-schemes/src/folding/nova/mod.rs Show resolved Hide resolved
folding-schemes/src/folding/protogalaxy/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@arnaucube arnaucube left a comment

Choose a reason for hiding this comment

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

Very nice abstractions! Just left a couple of small suggestions regarding extending the comments to make it faster to understand the abstractions to the reader.

LGTM!

folding-schemes/src/folding/nova/traits.rs Show resolved Hide resolved
folding-schemes/src/arith/mod.rs Outdated Show resolved Hide resolved
@CPerezz
Copy link
Member

CPerezz commented Sep 25, 2024

@winderica can we merge this and start merging the cascade of PRs that depends on this one??

@arnaucube
Copy link
Collaborator

woah sudden rush @CPerezz hehe. Well, I approved but left a couple of suggestions, and the other PRs will need reviews too. For this one it's missing your approval :P
I think that we can keep with the normal flow of review+approval+updates and then merging.

@winderica
Copy link
Contributor Author

Sorry for the delay! The reviews should be addressed now :)

@arnaucube
Copy link
Collaborator

ahh 😳 no worries, there has been no delay 🙏

@CPerezz
Copy link
Member

CPerezz commented Sep 25, 2024

Are @arnaucube 's suggestions addressed or discussed? Ping once it's done and we can merge! :)

@arnaucube
Copy link
Collaborator

Are @arnaucube 's suggestions addressed or discussed? Ping once it's done and we can merge! :)

I understand this as an approve (?) (although the approval check is missing)

Regarding my suggestions, they are (greatly) addressed in the last commits, yes ^^
Then assuming that everything is fine from your side I think we're good to go!

@arnaucube arnaucube added this pull request to the merge queue Sep 25, 2024
Merged via the queue into privacy-scaling-explorations:main with commit f1d8241 Sep 25, 2024
8 checks passed
@arnaucube arnaucube added this to the Stabilize lib milestone milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants