Feat/circom prover

[KimiWu123]

close #299

[cloudflare-workers-and-pages]

Deploying mopro with    Cloudflare Pages

Latest commit:	0201e3d
Status:	⚡️  Build in progress...

View logs

[chancehudson]

Could i get a concrete rationale for creating a new package in the monorepo instead of breaking the ark logic into a separate file instead? This adds a layer of complexity and as such i'd expect concrete upside. As it stands i don't see the upside and am against adding a new package.

[chancehudson]

I think this macro is a bad abstraction. Why do we want to hide which adapter is being used to create the witness function? e.g. why can't we use rust_witness and witnesscalc_adapter directly? Adding this adds a layer of complexity, what is the justification?

[KimiWu123]

removed in 265abb6

[chancehudson]

This thread is a noop. This generate_witness implementation creates a background thread, then blocks the main thread waiting for the background thread (e.g. immediately joins the thread below).

[KimiWu123]

Nice catch! I forgot to handle this. fixed in b6d39a6

[chancehudson]

ah i see, the intention is to publish the package on crates.io?

[KimiWu123]

Could i get a concrete rationale for creating a new package in the monorepo instead of breaking the ark logic into a separate file instead? This adds a layer of complexity and as such i'd expect concrete upside. As it stands i don't see the upside and am against adding a new package.

I think Vivian's intention is

We can maintain our own circom-prover for cross-platform solution, and we can publish our packages.
In the future, we can encourage ZK applications (e.g. Semaphore) to use the crate circom-prover instead of circom-compat for cross-platform proving.

, want to address

We have several adapters (now or in the future). We need to easily switch between adapters

and also to simply the dependencies of mopro, here.

At the beginning, Vivian proposed to have a separated repo to address above issues. However, I suggested to put the logic inside mopro repo and have another crate. The reason why I suggest that is because it'll be getting harder and harder to maintain if we separate our code in so many different repos. And it's also hard for newcomers to get whole pictures of mopro.

I would love to hear your feedback on how to fix #299 and make the codebase clean! 😄 Moving to circom-prover repo works for me as well.

[vivianjeng]

@chancehudson Yes
to be clarified
It will be circom-prover (#299) in the future
(will be here https://github.com/zkmopro/circom-prover)
But for current stage, it would be convenient to put in monorepo like this

and I think the rapidsnark can also be in part of circom-prover
like we can set different features to enable different adapter
e.g. features = ["ark-works"] or features = ["rapidsnark"]
how do you think? related to this PR #305

and I think the rapidsnark-adpater can also be a crate like witnesscalc-adpater
I think they are quite independent to mopro or circom-prover

[vivianjeng]

I think the more ideal implementation is to have like

witness/
   witnesscalc.rs
   rust_witness.rs
   mod.rs

and we enable each by

#[cfg(feature = "witnesscalc")]
mod witnesscalc;
#[cfg(feature = "rust-witness")]
mod rust_witness;

so we don't have to set many feature flags
and what if people set two different witness generators at the same time?
e.g. rustwitness and witnesscalc

it looks like it will execute the same operation twice?

            #[cfg(feature = "witnesscalc")]
            WitnessFn::WitnessCalc(wit_fn) => wit_fn(bigint_inputs, _dat_path.as_str()),
            #[cfg(feature = "rustwitness")]
            WitnessFn::RustWitness(wit_fn) => wit_fn(bigint_inputs),

I actually prefer the way if we can enable both features at the same time
(though it might not make sense)
but we need to design the functions
like generate_witness_rust_witness, generate_witness_witnesscalc
and init the prover like
let circom_prover = CircomProver(wtns_fn, proof_fn);

or we need to check if one feature is enabled, and another should not be executed

[KimiWu123]

I think the more ideal implementation is to have like

witness/
   witnesscalc.rs
   rust_witness.rs
   mod.rs

and we enable each by

#[cfg(feature = "witnesscalc")]
mod witnesscalc;
#[cfg(feature = "rust-witness")]
mod rust_witness;

It's a good idea, but I realized there is no many code in witnesscalc.rs or rust_witness.rs. It seems a little over design for me and that's why I kept two different witness generation functions in the same file.

so we don't have to set many feature flags and what if people set two different witness generators at the same time? e.g. rustwitness and witnesscalc

it looks like it will execute the same operation twice?

            #[cfg(feature = "witnesscalc")]
            WitnessFn::WitnessCalc(wit_fn) => wit_fn(bigint_inputs, _dat_path.as_str()),
            #[cfg(feature = "rustwitness")]
            WitnessFn::RustWitness(wit_fn) => wit_fn(bigint_inputs),

No, it won't execute the same operation twice. wit_fn is passed from outside, it's the first argument of generate_witness. Callers can call which witness generation function as they want. For example, if both features were set, the code would be

             WitnessFn::WitnessCalc(wit_fn) => wit_fn(bigint_inputs, _dat_path.as_str()),
             WitnessFn::RustWitness(wit_fn) => wit_fn(bigint_inputs),

Just like a normal match case.

[KimiWu123]

let circom_prover = CircomProver(wtns_fn, proof_fn);

I was considering this design as well, but it seems not really necessary to do in this way. I think current design is more lightweight, people can call any functions as they want, not have to init a CircomProver first. It better to support all the options for witness and proving in this kind of desgin, for example,

let a = CircomProver(rust_witness, arkworks);
let b = CircomProver(witness_calc, arkworks);

which means it better not to use cfg(feature) to control the output of CircomProver, supports all features at a time. Otherwise, it'll be strange that I can use rust_witness in this release/package but can't in another release. Not sure if it's clear enough.

The only benefit I can image is that we don't need to pass witness/witness_thread around, just a single prove can handle everything.

[vivianjeng]

LGTM
Just some lines can be removed