Skip to content
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

Support for wasm32-unknown-unknown target? #138

Closed
tmpfs opened this issue Sep 7, 2021 · 25 comments
Closed

Support for wasm32-unknown-unknown target? #138

tmpfs opened this issue Sep 7, 2021 · 25 comments

Comments

@tmpfs
Copy link
Contributor

tmpfs commented Sep 7, 2021

I have been investigating using multi-party ECDSA in WASM and wanted to know if there are plans to support the wasm32-unknown-unknown target so we could use this library from the browser?

I have explored your emerald-city library and have used it to put together a very simple demo of using WASM (via wasm-pack) but would prefer to use something that is ready for production and has been audited, like this library or threshold-signatures.

All my attempts to compile for wasm32-unknown-unknown have failed so far and when I tried with multi-party-ecdsa I get an error (that is now quite familiar!):

error[E0046]: not all trait items implemented, missing: `encode`
    --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-serialize-0.3.24/src/serialize.rs:1358:1
     |
853  |     fn encode<S: Encoder>(&self, s: &mut S) -> Result<(), S::Error>;
     |     ---------------------------------------------------------------- `encode` from trait
...
1358 | impl Encodable for path::Path {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `encode` in implementation

error[E0046]: not all trait items implemented, missing: `decode`
    --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-serialize-0.3.24/src/serialize.rs:1382:1
     |
904  |     fn decode<D: Decoder>(d: &mut D) -> Result<Self, D::Error>;
     |     ----------------------------------------------------------- `decode` from trait
...
1382 | impl Decodable for path::PathBuf {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `decode` in implementation

Which is due to the deprecated rustc-serialize library being used by rust-crypto. If we look at the code there is no implementation for the unknown target OS.

We can't fix rustc-serialize so the solution I would imagine is to remove the dependency on rust-crypto which I believe is now deprecated in favour of RustCrypto.

Would it be possible to merge the experimental work in emerald-city so we can build this library for WASM?

Thanks 🙏

@omershlo
Copy link
Contributor

omershlo commented Sep 7, 2021

Hi ! We plan to remove rust-crypto .
meanwhile : see if this helps : #137

@survived
Copy link

survived commented Sep 7, 2021

rust-crypto dependency bothers us a lot, so we're removing it: ZenGo-X/curv#137. Though it's one of minor challenges, a lot of other things block us from being wasm-compatible right now, e.g. we likely need pure Rust implementation of secp256k1, see #137 mentioned above.

@tmpfs
Copy link
Contributor Author

tmpfs commented Sep 8, 2021

Thanks @omershlo and @survived, I am excited to see those changes land!

a lot of other things block us from being wasm-compatible right now

It would be really useful to keep track of these to get an idea when we might be able to use multi-party-ecdsa in the browser, would you be able to list the other blocking issue(s) please?

@elichai
Copy link
Contributor

elichai commented Sep 12, 2021

@survived rust-secp256k1 should work on wasm, we even try to test it in the CI: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/.github/workflows/rust.yml#L32 (I say try because there are dozens of different flavors of wasm interpreters )

@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 4, 2021

Hey,

I noticed that ZenGo-X/curv#137 has landed in curv@0.8 and wanted to upgrade the dependency here but it looks like centipede and bulletproof would need to be upgraded in lock step.

What is the recommended process to test out updating the curv library? Anything I can do to help please let me know 🙏

@survived
Copy link

survived commented Oct 4, 2021

Hi @tmpfs, as you correctly observed, multi-party-ecdsa depends on several crates that depend on curv as well, so they need to be upgraded first. Usually I update multi-party-ecdsa and its deps in this order: paillier, zk-paillier, bulletproof, centipede, and then multi-party-ecdsa. Updating deps is like warming up, because they have fewer curv usage (and generally have smaller codebase) 🙂 . Any help with upgrading crates is really appreciated ❤️

@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 28, 2021

@survived, heads up that I just got this repository compiling and we move past the rust-crypto problem but it looks like we need to enable the js feature flag for getrandom:

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
   --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.3/src/lib.rs:219:9
    |
219 | /         compile_error!("the wasm32-unknown-unknown target is not supported by \
220 | |                         default, you may need to enable the \"js\" feature. \
221 | |                         For more information see: \
222 | |                         https://docs.rs/getrandom/#webassembly-support");
    | |_________________________________________________________________________^

error[E0433]: failed to resolve: use of undeclared crate or module `imp`
   --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.3/src/lib.rs:246:5
    |
246 |     imp::getrandom_inner(dest)
    |     ^^^ use of undeclared crate or module `imp`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0433`.
error: could not compile `getrandom`

I will return to this problem once #144 is ready for review 👍

@elichai
Copy link
Contributor

elichai commented Oct 28, 2021

@tmpfs Small note, if you want to enable a feature in a transitive dependency you can add getrandom = {version = "0.2", features = ["js"]} in your downstream app/crate and that will enable the js feature transitively even if you don't use getrandom directly.

@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 29, 2021

Thanks @elichai, setting the feature flag for the transitive dependency fixes that issue and I move on to the next error:

error[E0432]: unresolved imports `libc::c_char`, `libc::c_int`, `libc::c_long`, `libc::c_ulong`, `libc::c_void`, `libc::c_double`, `libc::size_t`
 --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/rust-gmp-kzen-0.5.1/src/mpz.rs:1:12
  |
1 | use libc::{c_char, c_int, c_long, c_ulong, c_void, c_double, size_t};
  |            ^^^^^^  ^^^^^  ^^^^^^  ^^^^^^^  ^^^^^^  ^^^^^^^^  ^^^^^^ no `size_t` in the root
  |            |       |      |       |        |       |
  |            |       |      |       |        |       no `c_double` in the root
  |            |       |      |       |        no `c_void` in the root
  |            |       |      |       no `c_ulong` in the root
  |            |       |      no `c_long` in the root
  |            |       no `c_int` in the root
  |            no `c_char` in the root

So I figured I needed to enable the num-bigint feature for curv to use the pure Rust implementation so I added:

curv-kzen = {version = "0.9", features = ["num-bigint"], default-features = false}

But that doesn't help. Any ideas?

@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 29, 2021

Interestingly, when I try compiling for the wasm32-wasi target I get this error:

error: You can choose only one bigint implementation. See crate features.
  --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/curv-kzen-0.9.0/src/arithmetic/mod.rs:26:1
   |
26 | compile_error!("You can choose only one bigint implementation. See crate features.");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0252]: the name `BigInt` is defined multiple times
  --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/curv-kzen-0.9.0/src/arithmetic/mod.rs:37:9
   |
31 | pub use big_gmp::BigInt;
   |         --------------- previous import of the type `BigInt` here
...
37 | pub use big_native::BigInt;
   |         ^^^^^^^^^^^^^^^^^^ `BigInt` reimported here
   |
   = note: `BigInt` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
   |
37 | pub use big_native::BigInt as OtherBigInt;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

As I have disabled default-features and configured for num-bigint I wonder why this conflict is happening, does default-features not apply for transitive dependencies?

@survived
Copy link

@tmpfs Note that if you have other dependencies that depend on curv (eg. multi-party-ecdsa), then you need to turn off default features for them too, ie: multi-party-ecdsa = { git = "...", tag = "...", default-features = false }.

The reason why you see this message is that some of your dependencies enforces curv/rust-gmp-kzen feature. You may use cargo tree tool to find out which one

@tmpfs
Copy link
Contributor Author

tmpfs commented Nov 2, 2021

Thanks for the tip @survived, it seems like all I needed was to set default-features for the multi-party-ecdsa dependency to get it to compile.

Using these dependencies:

[dependencies]
multi-party-ecdsa = {path = "../../git/multi-party-ecdsa", default-features = false}
getrandom = {version = "0.2", features = ["js"]}
curv-kzen = {version = "0.9", features = ["num-bigint"], default-features = false}

Running cargo check --target wasm32-unknown-unknown is working 👍

@tmpfs
Copy link
Contributor Author

tmpfs commented Nov 26, 2021

@survived, having some issues getting entropy in WASM, I wonder if you have any ideas.

First I hit this error calling Keys::create() in the WASM module:

panicked at 'could not initialize thread_rng: All entropy sources failed (permanently unavailable); cause: OsRng: support for wasm32 requires emscripten, stdweb or wasm-bindgen (permanently unavailable)', /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.6.5/src/rngs/thread.rs:80:17

So I added the rand dependency with the wasm-bindgen feature (note that the wasm-bindgen feature no longer exists in rand@0.8):

rand = { version="0.6.5", features = ["wasm-bindgen"] }

Which yields:

panicked at 'Error: getrandom: this target is not supported', /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/rand_core-0.5.1/src/os.rs:63:13

Any pointers on how I should set up the RNG for WASM?

Full Cargo.toml is here.

@tmpfs
Copy link
Contributor Author

tmpfs commented Nov 29, 2021

I have been looking into the rand errors a bit and it appears to be something to do with the transitive dependency chain configuration but I am not sure exactly where yet. I did a quick test depending upon rand@0.8 with getrandom using the js feature and with rand@0.6.5 with the wasm-bindgen feature and they both work fine compiling to WASM as direct dependencies.

When I look at the rand dependency for multi-party-ecdsa there are 4 versions:

  rand:0.3.23
  rand:0.4.6
  rand:0.6.5
  rand:0.7.3

I noticed that the 0.6.5 version is required by curv (as rand_legacy) because secp256k1 depends on rand ^0.6. Which means that upgrading rand is not going to be feasible. Is there some cargo-fu I am missing that would get us past this error?

@tmpfs
Copy link
Contributor Author

tmpfs commented Nov 29, 2021

I managed to get it working by downgrading getrandom from:

getrandom = {version = "0.2", features = ["js"]}

To 0.1.16 with the wasm-bindgen feature:

getrandom = {version = "0.1.16", features = ["wasm-bindgen"]}

Phew!

@tmpfs
Copy link
Contributor Author

tmpfs commented Dec 4, 2021

@survived and anyone else following this thread I found an interesting problem. For context, I am trying to port the gg18 example to WASM using a websocket server backend, the code is here.

I got to the point of verifying correctness of round 2 answers during key generation and hit this error:

panicked at 'The global thread pool has not been initialized.: ThreadPoolBuildError { kind: IOError(Error { kind: Unsupported, message: "operation not supported on this platform" }) }', /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.9.1/src/registry.rs:170:10

Using cargo tree I noticed that the crates centipede, kzen-paillier and zk-paillier use rayon so I figured I would use wasm-bindgen-rayon to enable threads and rayon support in webassembly.

Once I managed to integrate with wasm-bindgen-rayon (which involved an upgrade to webpack@5 to get it to work!) I hit a new error:

TypeError: Crypto.getRandomValues: Argument 1 can't be a SharedArrayBuffer or an ArrayBufferView backed by a SharedArrayBuffer

Which leads to this issue about Crypto.getRandomValues() not supporting SharedArrayBuffer.

So I think it is fair to say that adding threads to webassembly leads to the call to Crypto.getRandomValues() being backed by a SharedArrayBuffer which makes sense as that is what would be used for communication between threads.

I think that supporting SharedArrayBuffer in Crypto.getRandomValues() is a non-starter as it would be possible for other threads to modify the buffer before it has been filled leading to a data race.

Would it be feasible to introduce a feature flag for those crates that disabled use of rayon? Is this practical?

Maybe we could have a multi-threaded feature that is enabled by default and then in webassembly builds we set default-features to false for those dependencies?

Happy to do the work to land this if you think it is a good approach 🙏

@tmpfs
Copy link
Contributor Author

tmpfs commented Dec 6, 2021

So this gets a bit strange, I have learnt that the multi-threaded SharedArrayBuffer issue was fixed in getrandom by forcing usage of an Uint8Array for the call to Crypto.getRandomValues(), see this PR and was backported to getrandom@0.1 here.

So I tried to reproduce this using rand@0.6.5, rayon and wasm-bindgen-rayon and got the same error, the code is in the rayon-0.6.5 branch.

Which ultimately led me to find out that rand@0.6.5 does not use getrandom which is why that fix is not present. The code that is actually executing is here: https://github.com/rust-random/rand/blob/0.6.5/rand_os/src/wasm32_bindgen.rs.

And because secp256k1 depends upon rand ^0.6 we have a bit of a problem.

@survived, some guidance on the best way forward here would be useful. Should we try to get secp256k1 to upgrade to rand@0.8 so we can lose the rand_legacy usage in curv?

@tmpfs
Copy link
Contributor Author

tmpfs commented Dec 7, 2021

Looks like we need to wait for this PR (related issue) to land and then we should be good.

I also wanted to update pairing-plus to use the latest version so we don't have 3 different versions of rand in the dependency tree, hopefully this PR will get merged too.

Once they land we can update curv to use rand@0.8 and then I think we should be able to move past this error 🙏

@survived
Copy link

survived commented Dec 7, 2021

I'm waiting for secp256k1 library update too that will bump rand dependency 🙂 This will happen eventually.

Regarding pairing-plus issue, I believe we can eliminate this one by slightly modifying curv crate. We could (and should) make every curve support optional, so users could turn off particular curve implementation if it's not relevant for them (and even causes problems). In your case, you only need secp256k1 curve. What it requires is to simply add secp256k1-curve, seck256r1-curve, [...] features that would enable required dependencies

(pairing-plus library provides implementation of BLS curve and nothing more)

@tmpfs
Copy link
Contributor Author

tmpfs commented Dec 8, 2021

Regarding pairing-plus issue, I believe we can eliminate this one by slightly modifying curv crate. We could (and should) make every curve support optional, so users could turn off particular curve implementation if it's not relevant for them (and even causes problems). In your case, you only need secp256k1 curve. What it requires is to simply add secp256k1-curve, seck256r1-curve, [...] features that would enable required dependencies

I like the sound of this, makes a lot of sense. Happy to help out if needed!

For anyone that wants to try building for wasm32-unknown-unknown now before we get the rand updates here is a little hack to get past this particular problem:

// Temporary hack for getRandomValues() error
const getRandomValues = crypto.getRandomValues;
crypto.getRandomValues = function(buffer) {
  const array = new Uint8Array(buffer);
  const value = getRandomValues.call(crypto, array);
  buffer.set(value);
  return buffer;
}

@aon
Copy link

aon commented Dec 23, 2022

Hi! I was just about to go down this path, and wanted to ask. @tmpfs were you able to compile for wasm? Were those conflicts solved or did you have to keep on using the hacked you mentioned?

Thanks!

@tmpfs
Copy link
Contributor Author

tmpfs commented Dec 24, 2022

@aon, yes it compiles, the hacks are still necessary due to the use of rayon/threads in some of the dependencies.

Ideally, multi-threaded operation should be configurable, see #154.

@aon
Copy link

aon commented Dec 24, 2022

@tmpfs great! Well I'll try to make it work following this thread. If you happen to have somewhere a repo where this is implemented that'd be awesome. Thanks for the help!

@tmpfs
Copy link
Contributor Author

tmpfs commented Dec 25, 2022

@aon
Copy link

aon commented Dec 26, 2022

@tmpfs Thanks!

@tmpfs tmpfs closed this as completed Dec 27, 2022
luca992 added a commit to luca992/curv that referenced this issue Aug 2, 2023
luca992 added a commit to luca992/multi-party-ecdsa that referenced this issue Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants