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

Circom wrapper's helper function #26

Merged

Conversation

yugocabrio
Copy link
Contributor

Hello, I conducted the issue #16 (Circom Wrapper). It would be best to write frontend_trait at first, but I made this helper function in advance.

What?

frontend/circom/mod.rs is the main code in this PR. The file works for below;

  1. Extract R1CS constraints from Circom's .r1cs files.
  2. Convert the BigInt constraints to Bn254's ScalarField constraints.
  3. Convert R1CS constraints suited for the Bn254 pairing curve to a format suitable for folding-schemes's R1CS.
  4. Calculate the witness from Circom's .wasm file and inputs.
  5. Convert R1CS constraints and witness from the Circom format to the folding-schemes's R1CS and z format.

Changes

  • Added convert_constraints_bigint_to_scalar function that maps BigInt constraints to Bn254's ScalarField constraints.
  • Implemented extract_constraints_from_r1cs to parse constraints from Circom's .r1cs files.
  • Added convert_to_folding_schemes_r1cs to adapt the constraints for folding-schemes's R1CS.
  • Implemented calculate_witness for witness computation using Circom's .wasm file.
  • Added circom_to_folding_schemes_r1cs_and_z to convert from Circom's R1CS and witness to folding-schemes formats.

How?

In #16, I said that I would use nova-scotia, but ark-circom(circom-compat) is based on arkworks so it is more compatible to folding-schemes.
So I adopted the ark-circom instead of nova-scotia, and copy&pasted their codes in circom/r1cs_reader.rs and circom/witness/circom.rs, memory.rs, mod.rs, witness_calculator.rs because I couldn't import ark-circom crate due to arkworks dependencies problem.
ark-circom has Apache-2.0 and MIT license, but it would be better to write credit for the library in readme.

Testing?

The test_circom_to_folding_conversion function showcases the entire process: from extracting constraints from an R1CS file, through conversion to folding-schemes's R1CS and z, to checking the equality of A•z◎B•z=C•z.
I confirmed folding-schemes's R1CS and z from Circom's .r1cs and witness.wasm satisfy the check_relation function in src/ccs.

Anything Else?

To Do

  • Currently, in the convert_to_folding_schemes_r1cs function, the variable l in R1CS is derived from the length of the first row in the a_matrix:
    let l = a_matrix.first().map(|vec| vec.len()).unwrap_or(0);
    This does not accurately represent the size of public inputs and outputs. In other words, everything is public i/o. Moving forward, we need to modify this to ensure l captures the correct size of public inputs and outputs."
  • design the frontend_trait and fit this circom wrapper function to it.
  • mention the credit to ark-circom in readme.

Thank you for your review. I appreciate it and look forward to your feedback!

@arnaucube
Copy link
Collaborator

So I adopted the ark-circom instead of nova-scotia, and copy&pasted their codes in circom/r1cs_reader.rs and circom/witness/circom.rs, memory.rs, mod.rs, witness_calculator.rs because I couldn't import ark-circom crate due to arkworks dependencies problem.

Which were the dependencies problems that you found with https://github.com/arkworks-rs/circom-compat ?
I'm afraid that copy pasting part of it's code into the folding-schemes repo will bring extra mainteinance overhead, while circom-compat/ark-circom seems to be well maintained.
Ideally we would use its methods by importing it as a dependency, that's why I'm curious on which issues you found when trying to use it as a dependency.

@yugocabrio
Copy link
Contributor Author

yugocabrio commented Sep 19, 2023

@arnaucube
Thank you for the feedback. I concur with your observations. Consequently, I identified the optimal dependencies that facilitate the utilization of circom-compat (a.k.a ark-circom) as an external library.

I updated ark-ec from "0.4.2" to "^0.4.0" and ark-serialize from "0.4.2" to "^0.4.0". Additionally, I incorporated ark-bn254 and ark-circom, and subsequently, the cargo test passed successfully.

I recognize the need to eliminate the copy-pasted code of ark-circom from the previous pull request and to utilize the ark-circom crate instead. I'm going to re-submit a revised Pull request within the next two days.

@yugocabrio
Copy link
Contributor Author

Successfully imported ark-circom as an external library and removed duplicated code. Please review the changes.

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! Thank you very much for contribuiting ^^

For task #16 there is a dependancy on #15, but while we don't have #15 (frontend trait) defined, this PR seems a good advancement of the methods that will be used once the frontend trait is defined.

I don't have much time now (currently OOO), but leaving some comments as a first high level pass.

Cargo.toml Outdated Show resolved Hide resolved
src/frontend/circom/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.

Tagging @han0110 and @CPerezz for a review

@arnaucube arnaucube self-requested a review October 9, 2023 10:01
src/frontend/circom/mod.rs Outdated Show resolved Hide resolved
src/frontend/circom/mod.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.

Could we avoid adding .wasm and .r1cs files for testing and generate them on the fly? Would that take a lot of time and slow down the tests that much?

Asking as otherwise, sync the repo will take time. And if we can save it would be cool. But not at the cost of tests taking forever.
In any case, worth exploring the alternatives.

Also, as for the frontend, could we have some struct/trait for which we implement this public functions? I'd rather have a unit struct that has nothing but parses all that stuff so that I don't need to call 3 functions rather than this.

@yugocabrio
Copy link
Contributor Author

@han0110 Thanks for the feedback.

Could we avoid adding .wasm and .r1cs files for testing and generate them on the fly? Would that take a lot of time and slow down the tests that much?

Certainly, I believe it's possible by using a sh file. In fact, nova-scotia allows us to generate .wasm and .r1cs files from .circom directly within a sh file, as seen (here). I'll work on implementing this shortly.

Also, as for the frontend, could we have some struct/trait for which we implement this public functions? I'd rather have a unit struct that has nothing but parses all that stuff so that I don't need to call 3 functions rather than this.

Thank you for the suggestion. I see the merit in your approach and will proceed with creating such a struct or trait.

@yugocabrio
Copy link
Contributor Author

yugocabrio commented Oct 11, 2023

I've addressed and refined every point in our code based on your invaluable feedback. Thank you for your guidance. Here's a concise summary of the modifications:

I removed the redundant generic F, now using E::ScalarField directly in the proper functions, making the code cleaner. And then, the unnecessary convert_constraints_bigint_to_scalar function has been removed.

Instead of adding .wasm and .r1cs files directly, we now generate them on the fly through the sh file during tests, ensuring a lightweight repository. So I added .wasm and .r1cs paths in .gitignore.

I integrated several related functions into the CircomWrapper struct. And extract_r1cs_and_z is only needed to convert circom into folding-schemes finally.

let circom_wrapper = CircomWrapper::<Bn254>::new(r1cs_filepath, wasm_filepath);

let (r1cs, z) = circom_wrapper
    .extract_r1cs_and_z(&inputs)
    .expect("Error processing input");

I would be most grateful if you could review the revised code again. @arnaucube

Copy link
Collaborator

@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.

LGTM

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.

Fixed conflicts.

LGTM! Thanks for this work @yugonsan

@yugocabrio
Copy link
Contributor Author

yugocabrio commented Oct 16, 2023

I have fixed some issues based on the error in GitHub actions. I have edited the yaml file to install Circom in GitHub Actions, and I would be happy to run GitHub Actions again.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@arnaucube arnaucube self-requested a review October 16, 2023 15:52
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 good job! The interface looks very nice now^^
I'm excited to start folding Circom circuits soon™

@arnaucube arnaucube added this pull request to the merge queue Oct 17, 2023
Merged via the queue into privacy-scaling-explorations:main with commit 7656c6b Oct 17, 2023
3 checks passed
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.

4 participants