Feat/better browser support #169

CPerezz · 2024-10-15T15:56:46Z

This PR resolves #155

The current approach, allows to:

Re-use the same traits to fit-in the Circom browser folding.
Allows to receive the Circom-generated witness from the browser and load them into the circuit in order to fold via prove_step.
Implements a mechanism that allows to keep having the same exact APIs (Exept for the addition of load_witness trait fn for FCircuit.

Current benchmarks situate the folding cost within the browser in arround 3.9s. Which is definitely not good. But better than the current status.

An example on how to use this can actually be seen at https://github.com/CPerezz/wasm-sonobe-integration

Since the witness is generated in the browser we need a way to store it such that it can flow from `prove_step` into `generate_constraints` due to the API restrictions.

This commit introduces CircomFCircuitBrowser, a structure that facilitates folding of Circom-generated circuits in the browser via WASM. It loads R1CS data, handles witness inputs, and generates constraints for the folding process. Resolves: #155

CPerezz · 2024-10-15T21:22:35Z

I just noticed an important thing. Unsure if it's related specifically to that. But I've tried a ton of configs and can't really tell any other cause.

Originally, with this PR I was getting ~4s prove_step calls.
When my battery dropped to <25%, i got a performance regression of +5x. I was at ~27s per fold.

I'm not sure if this is expected. But it's definitely a pain point. I know having low battery doesn't help with performance. But wasn't expecting to get such a pitfall..

Does anyone know if that's a thing?

dmpierre

I have a few questions, notably regarding formatting and the step_native method

dmpierre · 2024-10-16T11:52:27Z

folding-schemes/Cargo.toml

It seems like the formatting has some strange behaviour? Line 53 has 146 characters, while wrapping happens at line 78 at line 27.

I agree. Unsure why this happens TBH. Let me try to fix it.

Hope 30a78d3 looks better

Tried to solve it and makes the parser unhappy...

I don't think I can do much with this..

related: #169 (comment)

folding-schemes/src/frontend/circom/browser.rs

arnaucube

Nice! I like the load_witness method so that it does not affect the other frontends interfaces.

Since the file browser.rs does not contain any tests, I think that is good that we ensure that things work well. In the https://github.com/CPerezz/wasm-sonobe-integration example, it is folding once, but the first fold usually is not representative since is just the 'base case' (which has less logic than the following iteration steps, and also takes less time).
Could it be updated to fold >3 iterations? so that we can check that everything works fine when folding the output of the previous fold.

folding-schemes/src/frontend/circom/browser.rs

arnaucube · 2024-10-16T08:02:39Z

folding-schemes/src/frontend/circom/browser.rs

+        // Should we keep these?
+        assert_eq!(z_i.len(), self.state_len());
+        assert_eq!(external_inputs.len(), self.external_inputs_len());


maybe this can be changed into an 'if' that returns an error instead of panicking? so that the user of the lib does not have an uncontrolled crash of the run but an error that they can decide how to manage.

Or at least under a test flag (#[cfg(test)]) as in the original Circom frontend folding-schemes/src/frontend/circom/mod.rs#L60

The problem that I want to avoid is having errors. Errors are really hard to optimize for whereas panics aren't.

Also, this assert should only trigger when you're testing. In prod, you should always have a way to ensure the lengths are correct. (Notice that your'e not going to change the circuit for which you're generating witness nor anything similar.

If honest, I'd entirely remove the panics and errors and simply do panic="abort in the profile.
But, for now I left the panics so that debugging is easier.

Or at least under a test flag (#[cfg(test)]) as in the original Circom frontend folding-schemes/src/frontend/circom/mod.rs#L60

Notice that under a testing flag, this will never be triggered. As you will need to compile the lib for testing to then compile also the tests within the WASM binary.

with the test flag it will still be triggered when running the tests, no? (previous to going into wasm in the browser)

The point of those checks is not to panic the runtime of the program that uses sonobe, but to make the dev aware that the lenghts don't match.
With the assert_eq the app breaks without the dev being able to recover.

Sure, should we add an error then? Or simply ignore?

I think that we can do it as in the lines linked in this comment: #169 (comment)

folding-schemes/Cargo.toml

arnaucube · 2024-10-16T08:06:54Z

folding-schemes/Cargo.toml

-ark-poly = { version = "^0.4.0", default-features = false, features = ["parallel"] }
-ark-std = { version = "^0.4.0", default-features = false, features = ["parallel"] }
-ark-crypto-primitives = { version = "^0.4.0", default-features = false, features = ["r1cs", "sponge", "crh", "parallel"] }
+ark-ec = { version = "^0.4.0", default-features = false, features = [


One question, most of the changes in this file are formatting (mostly converting one-lines into multi-lines). Is there some standard that we could use, and if so could we apply it in the CI (and in our machines too) like we do with the rustfmt?
(the split into lines make it harder to read tho)

Also it seems arbitrary as @dmpierre pointed in the comment #169 (comment)
(some shorter lines get split, while longer ones keep as a oneline)

VSCode did that for some reason I ignore..

Happy to bring this to the previous status or use whatever tool is suggested bu you guys.

folding-schemes/src/folding/nova/circuits.rs

folding-schemes/src/folding/nova/mod.rs

folding-schemes/src/frontend/circom/browser.rs

CPerezz · 2024-10-18T09:25:04Z

Could it be updated to fold >3 iterations? so that we can check that everything works fine when folding the output of the previous fold.

I wanted to wait for the 0xPARC folks to code the full thing to actually merge this. As I don't want to use my example as a test of correctness.

But I can update it if desired.

@arnaucube

As @arnaucube correctly suggested to me, the `wasm` feature can be omitted as the `wasmer` loader does not require it. Therefore, feature set, CI and code has been updated accordingly. Also, the browser feature has been renamed to `circom-browser`.

CPerezz added 3 commits October 15, 2024 17:32

feat: Enable load_witness functionallity within FCircuit

39d4ee3

Since the witness is generated in the browser we need a way to store it such that it can flow from `prove_step` into `generate_constraints` due to the API restrictions.

chore: Add "browser" feature and leave CircomFCircuitBrowser under it

67a066d

CPerezz requested review from arnaucube and dmpierre October 15, 2024 15:56

dmpierre reviewed Oct 16, 2024

View reviewed changes

chore: Address review leftovers

9bcf84c

CPerezz force-pushed the feat/better_browser_support branch from 30a78d3 to 9bcf84c Compare October 16, 2024 14:26

arnaucube reviewed Oct 16, 2024

View reviewed changes

arnaucube added wasm/browser frontend labels Oct 16, 2024

chore: Ammend review comments

206984c

arnaucube mentioned this pull request Oct 17, 2024

Move the experimental frontends into a separate crate, so that when not using them they don't take several minutes to compile (and indirect dependencies). #168

Merged

fix: leftover parallel feature code

0cf1aae

update: Remove wasm feature

47d908a

As @arnaucube correctly suggested to me, the `wasm` feature can be omitted as the `wasmer` loader does not require it. Therefore, feature set, CI and code has been updated accordingly. Also, the browser feature has been renamed to `circom-browser`.

CPerezz force-pushed the feat/better_browser_support branch from 7ec21d6 to 47d908a Compare October 18, 2024 09:33

CPerezz added 6 commits October 18, 2024 11:34

fix: Exclude constant 1 from zi_1 returned in browser::step_native

5c67849

fix: Avoid whitespaces within features passed to CLI commands

29ba23e

fix: Clippy warns

291082e

fix: CI feature typo

d2d543f

Merge branch 'main' into feat/better_browser_support

9946b86

fix: Update CI package to test for WASM build to frontends

63f599e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/better browser support #169

Feat/better browser support #169

CPerezz commented Oct 15, 2024

CPerezz commented Oct 15, 2024

dmpierre left a comment

dmpierre Oct 16, 2024

CPerezz Oct 16, 2024

CPerezz Oct 16, 2024

CPerezz Oct 16, 2024

arnaucube Oct 16, 2024

arnaucube left a comment

arnaucube Oct 16, 2024

CPerezz Oct 16, 2024

CPerezz Oct 16, 2024

arnaucube Oct 16, 2024

CPerezz Oct 18, 2024

arnaucube Oct 18, 2024

arnaucube Oct 16, 2024

arnaucube Oct 16, 2024

CPerezz Oct 16, 2024

CPerezz commented Oct 18, 2024

Feat/better browser support #169

Are you sure you want to change the base?

Feat/better browser support #169

Conversation

CPerezz commented Oct 15, 2024

CPerezz commented Oct 15, 2024

dmpierre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaucube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz commented Oct 18, 2024