Adapt Frontend/Circom for FCircuit Trait

[yugocabrio]

Work in progress, I have to write test codes and clean up all. I'll provide an update in detail once finished. #55

[yugocabrio]

I've been refining the Circom integration and encountered an issue:

• CircomWrapper: I've removed the convert_to_folding_schemes_r1cs and circom_to_folding_schemes_r1cs_and_z because we don't have to convert the R1CS which is based on ark-circom into folding-schemes' one.
• Circom file: I've updated taht input and output variables are handled as vectors.
• FCircuit implementation for CircomtoFCircuit, which includes a circom_wrapper along with its corresponding test functions.

However, I face an issue with the R1CS in ark-circom being defined over a pairing, which doesn't align with our use of Pallas::Fr. This discrepancy leads to incompatibilities so I'm considering the need to shift from pairing to primefield in the R1CS definition of ark-circom. However, I'm uncertain if this is the optimal approach or not. Could you provide your insights on this matter?

[arnaucube]

Oh right! I was unaware that circom-compat (old ark-circom) repo uses the Pairing trait for the R1CS related stuff (where it only should need the Fr), and furthermore, it seems that it has some hardcoded values for the BN254 only.
I've migrated it to use PrimeField instead of Pairing in this fork: /~https://github.com/arnaucube/circom-compat and disabled some specific checks related to the BN254.
Could you check if with that fork it works when using the Pallas curve?

Hopefully we can get into a state of the fork where it makes sense to open a PR into the original arkworks/circom-compat repo so other people in the community can benefit from it too, so if you find something that does not work in the fork and you find how to fix it feel free to update it.

[yugocabrio]

@arnaucube
Thank you for the code adjustments! I am using it right away.

[yugocabrio]

@arnaucube
I would like to ask about the generate_step_constraints function. In the test function of FCircuit for the arkworks circuit, I think that the constant values are subjected to constraints using cs: ConstraintSystemRef, but since there's no enforce method, it seems like function F merely returns just the calculation results. However, does this essentially act as a constraint?

In my current implementation of generate_step_constraints for theCustomtoFCircuit, the approach is to extract CircomCircuit { r1cs, witness } and call the generate_constraints that contains the enforce_constraint to guarantee the R1CS from ark-circom. Is this the right overall strategy?

[arnaucube]

In principle it is generating the constraints under the hood.
The R1CS matrices can be printed to see an example, by adding

        cs.finalize();
        let cs = cs.into_inner().unwrap();
        dbg!(crate::ccs::r1cs::extract_r1cs(&cs));

right after src/frontend/mod.rs#L164 , it will print the expected A,B,C matrices for the CubicCircuit (x^3 + x + 5 = y).

Regarding the approach to extract the circom constraints, yes it makes sense!

[yugocabrio]

Thanks. Augmented circuit wraps F circuit constraints with ark-circom, like forming two layers.
However, if I try the current code with the test_circom_step_constraints test function, that test for the generate_step_constraints that contains circom_circuit.generate_constraints(cs.clone()) in the CircomtoFCircuit implementation, I get an error Constraint trace requires enabling ConstraintLayer.

I have confirmed that circom_circuit.generate_constraints(cs.clone()) by itself(Not constraint wrap structure) works fine with test_extract_r1cs_and_witness.

Thus, I am having trouble with the ark-circom handling process when there are two constraint layers. I apologize for the inconvenience, but could you please help me how to handle this error? @arnaucube

[arnaucube]

Hi ^^
The error seems to be that the constraints are not satisfied, I think that the Constraint trace requires enabling ConstraintLayer is more an informative print.

The issue seems to be that the inputs are allocated twice, one in the arkworks FpVar::new_witness and another in the self.calculate_witness(inputs)?; call inside extract_r1cs_and_witness.

I've made some temporary changes in a fork from your branch that make the test_circom_step_constraints pass: /~https://github.com/arnaucube/folding-schemes-CircomFrontend/commit/3d05ef5ea4c2b14841a8e9b0c8a5bf489e63ed88 (more details in the commit message).
(The reason to remove the { public [ivc_input] } from the circom circuit is that the FCircuit is 'internal' so it has no 'public' inputs)

You will see that there are hardcoded values, this is to keep the commit minimal and simple but that works. A next step should be to iterate those changes and make them dependent on the inputs instead of having the hardcoded values in the function.

[yugocabrio]

Hello. I worked on the generalization to get rid of the hardcoded values in the generate_step_constraints function for Circorm implementation. But I still face the same error agaein. the z_i's argument type in the generate_step_constraints is Vec<FpVar<F>> . So I think I need to assign it with FpVar::<Fr>::new_input considering the Wrapper test circuit(augmented circuit) in the test_circom_step_constraints test function. However, again I face the error Constraint trace requires enabling ConstraintLayer. If I use FpVar::<Fr>::new_constant here, it passes, but since z_i is mutable, FpVar::<Fr>::new_input would be appropriate.
To be honest, this issue is difficult for me. I am sorry, but could you please take over?

[arnaucube]

Hi ^^
I think that you did a very good job and you're very close to have the full thing working! I've done some changes in this fork from your branch: /~https://github.com/arnaucube/folding-schemes-CircomFrontend
also rebasing to last main branch changes (porting your commits over the new main adapting it, since there were some conflicts) so we're up to date to the last main version and can use also the self.state_len() method.

One of the changes has been adding an option to the circom-compat fork to allow to specify that the inputs have been already allocated, which is what was causing the circuit satisfiability fail (allocating more than once the inputs).

One think that is pending to do is to separate the witness extraction from extract_r1cs_and_witness, so we avoid loading the r1cs each time that step_native is called. It could be a separate method extract_witness, and then the already existing method extract_r1cs_and_witness internally calls the new extract_witness. If you're up to do it would be great!

If you're up to, I think that you could continue from this fork of your fork, and add commits on top, finishing the changes, and then we should be ready for merging to the main repo :)

[yugocabrio]

Thank you so much for helping me a lot! Then I continue to work.

[yugocabrio]

Could you please move on to the review?
There are errors in spell check, but they are all notations in the code and are not actually a problem. Can I ignore them? Or should I add them to typos.toml?

[arnaucube]

Looks good! Regarding the 'typos' errors, yes, don't worry about it, it looks like the last release of typos added lot of 'false positives'
eg. crate-ci/typos#963, crate-ci/typos#967, crate-ci/typos#970.
It seems that now is fixed: crate-ci/typos#975

[dmpierre]

Solid work, thanks for this! Added a few comments.

[arnaucube]

LGTM! Thanks a lot for this contribution @yugocabrio !! 😊
Just left a small nit

[yugocabrio]

I've applied your feedback. Thank you very much for your assistance!!! @arnaucube and @dmpierre