-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: wasm #38
feat: wasm #38
Conversation
Maybe it can be of help here to have a look to how we computed the circuit builder before switching to the current witness generator logic from semaphore-rs: Lines 66 to 74 in 9ca9c22
We had performance issues at that time, but those were mainly due to not compiling in release mode. So I expect that even that was a valid and efficient solution, although we never actually tested it. If this helps sorting out the wasm problem and there we measure no big regression in circuit performances, we can move back to that implementation. |
rln/Cargo.toml
Outdated
ark-relations = { version = "0.3.0", default-features = false, features = [ "std" ] } | ||
ark-serialize = { version = "0.3.0", default-features = false } | ||
ark-circom = { git = "https://github.com/gakonst/ark-circom", features = ["circom-2"] } | ||
wasmer = { version = "2.0" } | ||
ark-circom = { git = "https://github.com/vacp2p/ark-circom", branch = "wasm", features = ["circom-2"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fork I created so (most of) this dependency can be run in wasm
rln/www/index.js
Outdated
|
||
rln.init_panic_hook(); | ||
|
||
function _base64ToArrayBuffer(base64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper function is for converting the circom, zkey and vk files from base64 to an Uint8Array
rln/src/protocol.rs
Outdated
fn calculate_witness_element<E: ark_ec::PairingEngine>( | ||
witness: Vec<BigInt>, | ||
sanity_check: bool, | ||
) -> Result<Vec<E::Fr>> { | ||
use ark_ff::{FpParameters, PrimeField}; | ||
let modulus = <<E::Fr as PrimeField>::Params as FpParameters>::MODULUS; | ||
|
||
// convert it to field elements | ||
use num_traits::Signed; | ||
let witness = witness | ||
.into_iter() | ||
.map(|w| { | ||
let w = if w.sign() == num_bigint::Sign::Minus { | ||
// Need to negate the witness element if negative | ||
modulus.into() - w.abs().to_biguint().unwrap() | ||
} else { | ||
w.to_biguint().unwrap() | ||
}; | ||
E::Fr::from(w) | ||
}) | ||
.collect::<Vec<_>>(); | ||
|
||
Ok(witness) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was extracted from ark-circom, It's main difference is that it will receive the witness instead of calculating it inside the function.
@s1fr0 , @staheri14 I'm currently having trouble with
I'll try cloning arkworks-rs/groth16 and adding some logs to that function to try to find out what's going on. |
rln/www/index.js
Outdated
const msgLen = Buffer.allocUnsafe(8); | ||
msgLen.writeUIntLE(uint8Msg.length, 0, 8); | ||
|
||
// Converting index to LE bytes | ||
const memIndexBytes = Buffer.allocUnsafe(8) | ||
memIndexBytes.writeUIntLE(memIndex, 0, 8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fryorcraken I had to use a buffer here because i need the function writeUintLE
rln/www/index.js
Outdated
const verifKeyUint8Array = _base64ToArrayBuffer(files.verification_key); | ||
const zkeyUint8Array = _base64ToArrayBuffer(files.zkey); | ||
const circomUint8Array = _base64ToArrayBuffer(files.rln_wasm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a dev be able to specify the depth and the zkey and verification key to use? or should these be embedded in the code?
fcabd96
to
ddf0f8c
Compare
Good news! I managed to generate proofs now! I had to make a change in vacp2p/ark-circom@dcde9ba so the
However, the proof generation is very slow. It takes ~9s. Perhaps there's a way to enable threads on wasm? |
I just noticed that if I don't open the devtools console immediatly, the proof generations is faster, i.e:
~2.5s 🤔 |
@richard-ramos this is great! So as we suspected, it was Regarding performance: does the delay happen only for the first proof generated or it takes 2.5s for all of them? When we struggled with the performance issue, those were affecting mainly the circom builder/witness calculator initialization (i.e., read the circuit wasm and load it) while proof generation and verification remained relatively fast. In other words we had a big delay ~60s (but in debug mode) for generating the first proof (since the witness generator had to be load first), while successive proofs using the same RLN object were generated fast (since the witness generator was ready). If that's the case we might able to manage it by init the witness generator while intializing the node. Can you replicate execution of |
e107eaa
to
924ea58
Compare
I think the code is now ready for a review. Please do not focus on the javascript code, as it is done only to see interop between js and rust: it is going to be removed from the repository, and is kept there so PR reviewers can see/test the wasm output. |
I will close the PR as I learned today that compiling zerokit to wasm is not needed. |
@richard-ramos First of all, amazing work on getting WASM support working! I think there was some miscommunication here. The first/easiest thing to do here was to use existing JS RLN libraries, like zk-kit and other things from PSE. This seems like a riskier and harder path, but if it works that's great. Sanaz and Franck have some more context from PM call(s) about this too. Regardless of if this is this the main path we choose or not we should definitely try to get it merged, so re-opening this. Will have a look at the PR a bit later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying this out, having some problems getting it to run.
Compile zerokit for wasm32-unknown-unknown:
cd rln
wasm-pack build --release
I assume this should be rln-wasm
? When I run in rln
I get:
error: the wasm32-unknown-unknown target is not supported by default, you may need to enable the "js" feature. For more information see: https://docs.rs/getrandom/#webassembly-support
But when I run in rln-wasm
the wasm-pack
command succeeds.
npm install
also works. However, when I run npm start
I get:
ERROR in ../pkg/rln_wasm_bg.wasm
Import "__wbg_BigInt_1fab4952b6c4a499" from "./rln_wasm_bg.js" with Non-JS-compatible Func Sigurature (i64 as parameter) can only be used for direct wasm to wasm dependencies
@ ../pkg/rln_wasm.js
@ ./index.js
@ ./bootstrap.js
ERROR in ../pkg/rln_wasm_bg.wasm
Import "__wbg_BigInt_d0c7d465bfa30d3b" from "./rln_wasm_bg.js" with Non-JS-compatible Func Sigurature (i64 as parameter) can only be used for direct wasm to wasm dependencies
@ ../pkg/rln_wasm.js
@ ./index.js
@ ./bootstrap.js
ℹ 「wdm」: Failed to compile.
Any idea what could be wrong?
Env:
# Running on MBP M1
node --version: v18.9.0
wasm-pack --version: wasm-pack 0.10.3
rustc --version: rustc 1.63.0 (4b91a6ea7 2022-08-08)
npm --version: 8.19.1
Few general comments:
|
} | ||
|
||
false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tests to this module? Super basic one
rln-wasm/www/README.md
Outdated
@@ -0,0 +1,20 @@ | |||
# RLN WASM EXAMPLE APP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see there's a README here already! I missed it because I expected it to be in the rln-wasm dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example app will also be removed, and will exist in https://github.com/waku-org/js-rln/
:)
ark-relations = { version = "0.3.0", default-features = false, features = [ "std" ] } | ||
ark-serialize = { version = "0.3.0", default-features = false } | ||
ark-circom = { git = "https://github.com/gakonst/ark-circom", features = ["circom-2"] } | ||
wasmer = { version = "2.0" } | ||
ark-circom = { git = "https://github.com/vacp2p/ark-circom", branch = "wasm", default-features = false, features = ["circom-2"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we upstream this as a PR to ark-circom too? Are all things in arkworks-rs/circom-compat#4 supported?
I think even if just partial support for our case it'd be a welcome PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see in vacp2p/ark-circom@0e58714 that it requires disabling parallels
. I wonder if it is possible to get the feature flag to be disabled when wasm
flag is enabled? Perhaps in a conditional package list or something, WDYT?
Also any idea why parallels
doesn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we upstream this as a PR to ark-circom too?
I will create a PR for ark-circom, that way we don't have to maintain a separate fork!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any idea why
parallels
doesn't work?
While running with that feature enabled, the following error was generated:
The global thread pool has not been initialized.: ThreadPoolBuildError { kind: IOError(Error { kind: Unsupported, message: "operation not supported on this platform"
I was looking at https://github.com/GoogleChromeLabs/wasm-bindgen-rayon , which should enable concurrency using js web workers, but I feel like the effort of adding multithreading to zerokit-wasm should be done in a separate PR.
Please try again with the latest commit. I updated some js dependency versions, so it's necessary to do a |
Works now! I see the proof verification in the console :) |
I added a couple of tests, and removed the example app since it's supposed to live in js-rln anyways. The only task pending is creating PRs for ark-circom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Feel free to merge, can do ark-circom upstream update in separate PR
``` | ||
cd rln-wasm | ||
wasm-pack build --release | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a pointer to how https://github.com/waku-org/js-rln/? Maybe this belongs more in that repo, but looking at README not obvious to me how to run something similar to the example app that existed here before
How to compile zerokit for wasm and see example code:
build-essential
package if using ubuntu.wasm32-unknown-unknown
: