zkmopro / mopro

Making client-side proving on mobile simple.
https://zkmopro.org
Apache License 2.0
118 stars 33 forks source link

Improve native witness generation performance with circom-witness-rs #32

Open oskarth opened 10 months ago

oskarth commented 10 months ago

Problem

When running RSA circuit we get:

Initializing library with arkzkey
Time to deserialize proving key: 7.937347541s
Time to deserialize matrices: 28.784083ms
Initializing arkzkey took: 8.00s
Generating proof 2
Witness generation took: 4.73s
Loading arkzkey took: 125.00ns
proof generation took: 1.14s

That is, witness generation takes 80% of time.

Notes

We want to try to integrate this as it looks promising: https://github.com/philsippl/circom-witness-rs

Since it is experimental we want to keep it under a feature flag.

We want to do this because it is low hanging fruit and we know for sure that it'll have any impact if it works.

Also getting rid of WASM will have many other benefits.

Acceptance criteria

Experimental support for native witness generator should get this down x10 faster, without wasm.

Ideally we also try this for larger circuits.

oskarth commented 10 months ago

There's also this https://github.com/0xPolygonID/witnesscalc but I do believe the circom-witness-rs should be faster and integrate nicer with current stack. Advantage of that one is that it is more mature.

oskarth commented 8 months ago

One thing that isn't clear to me is how much of an impact we expect this to have for new Anon-Aadhaar circuits.

@Meyanis95 can you provide some numbers of how long witness generation takes for you on desktop (using WASM?) for old version vs new version? Number of constraints is up by x10, but is this also true for witness generation time?

Meyanis95 commented 8 months ago

Sure, you can assign it to me. Just want to make sure, is the development environment expected is the Mopro repo?

oskarth commented 8 months ago

Just want to make sure, is the development environment expected is the Mopro repo?

I think this is easiest for this 1-3w sprint to figure out MoPerf. But once that's done we should do it outside if the repo.

oskarth commented 7 months ago

@vivianjeng can you update status of this in this issue for transparency? I pinged Phil but no response so think we'll have to figure this out on our own.

vivianjeng commented 7 months ago

errors

questions

unknown issues

oskarth commented 7 months ago

1) Were you able to run the example project? E.g. https://github.com/philsippl/semaphore-witness-example or https://github.com/worldcoin/semaphore-rs

These should work and give some idea of how to test the code.

2) > I tried to implement Fr_neg, Fr_inv, Fr_div,… - any idea why we'd need these but semaphore doesn't? Seems a bit odd to me.

3) Questions: No idea, would have to look into more deeply

vivianjeng commented 7 months ago
  1. yes I tried the semaphore circuits and it works
  2. I tried to compile keccak, AnonAadhaar circuits and found that we need these operations haven't been implemented but semaphore circuits didn't throw these errors
vivianjeng commented 6 months ago

create unsolved issues in circom-witness-rs repo https://github.com/philsippl/circom-witness-rs/issues/12 https://github.com/philsippl/circom-witness-rs/issues/15 https://github.com/philsippl/circom-witness-rs/issues/16 we need 15 and 16 to complete AA circuit witness generation

oskarth commented 6 months ago

Perfect, thank you! I guess it is a slightly bigger task than what we expected initially.

Is the current integration useful as is? For example for some circuits like keccak256. Would it make sense to integrate it and keep experimental flag off? So we don't lose that progress and can try when upstream issues are resolved.

0xturboblitz commented 6 months ago

Hi, I tried using it, implemented some of the missing ops here, and have had the three exact same errors as Vivian. Didn't dig deeper except for this one, which seems to be related to the fact that Fr_toInt tries to cast field elements as u64. This fails in my case when M - 1 is passed (the prime used minus one), which doesn't fit.

vivianjeng commented 1 day ago

https://github.com/iden3/circom-witnesscalc