Wasm bindings for user to participate in DKG and signing protocols #414

ameba23 · 2023-09-28T09:30:48Z

This adds wasm bindings to functions needed for client-side participation in the protocols

TODO

use gloo-net::websocket instead of tungstenite on wasm
use wasm_bindgen_futures::spawn_local instead of tokio::spawn on wasm
add wasm_bindgen to functions and make sure the types work
tests for running on wasm
address any issues with channels and maybe use a different channels implementation on wasm
makefile to package as a JS module with eg: wasm-pack

vercel · 2023-09-28T09:30:53Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
entropy-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 16, 2023 8:56am

crypto/protocol/src/user/wasm.rs

fjarri · 2023-10-06T23:02:11Z

Does that mean that we don't need separate WASM bindings in the Synedrion repo?

ameba23 · 2023-10-08T09:43:32Z

Does that mean that we don't need separate WASM bindings in the Synedrion repo?

@fjarri that was the idea. but i am starting to think this idea of making JS bindings to a function that involves a websocket client is too ambitious. it works in the browser but i think it is never going to work on all platforms. Unless maybe we use WASI.

but either way, we no longer need the make_key_shares function we have in synedrion-wasm as we no longer do centralized keygen.

* master: Write a Dockerfile that can build both `entropy` and `server`. (#430) Workflow for automated GitHub Release drafting/publishing. (#429) Update `.editorconfig` to match `.rustfmt.toml` settings (#427) Place `demo_offence` dispatchable behind root origin check (#426) Ensure correct validator order by using ValidatorInfo from chain rather than from user (#425) changed remaining references of --ws-external to --rpc-external Update README.md proactive refresh (#413)

ameba23 · 2023-11-14T10:19:11Z

.circleci/then.yml

+                  make build-nodejs
+                  cd nodejs-test
+                  yarn
+                  cd ../../..


Im not sure i've done this stuff in CI in the best way, but unless there is something really bad, i'd prefer to make suggest changes to it in a separate PR. Because this branch is constantly having conflicts and i want to get it merged.

HCastano

This looks cool! I'm still trying to figure out the context around this, so I haven't able to do a deep code review yet.

Okay so from what I can tell this PR is a couple steps towards #362, right?

And, correct me if I'm wrong here, but what it does is:

it enables parts of the server (just the protocol crate?) to be compiled to Wasm, which
allows the server to be bundled in an app with the Entropy SDK, which
lets a user connect directly to other TSS servers from the brower and participate in DKG themselves.

And how would you recommend to review this PR? Anywhere that should be looked at first or more deeply?

crypto/kvdb/Cargo.toml

HCastano · 2023-11-14T19:00:49Z

crypto/protocol/src/user/wasm.rs

+}
+
+/// Details of a validator intended for use on JS
+/// This differs from [crate::ValidatorInfo] only in that the fields must be private


We can always make the other struct's fields private as well and add a constructor or something to initialize it

Hmm yeah it would be nice to just have one type. Although currently this type is usually created by being deserialized from json in a signature request. Do you think that would stop working if we made the fields private? I wanna avoid making breaking changes where possible.

I want to say the field visibility shouldn't matter for deserializing, but I'm not 100% sure. We can file this as part of a general type-unification issue and deal with it later

crypto/protocol/nodejs-test/index.js

crypto/protocol/nodejs-test/package.json

crypto/protocol/src/protocol_transport/mod.rs

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

ameba23 · 2023-11-14T20:45:20Z

And, correct me if I'm wrong here, but what it does is:

* it enables parts of the `server` (just the `protocol` crate?) to be compiled to Wasm, which

* allows the server to be bundled in an app with the Entropy SDK, which

* lets a user connect directly to other TSS servers from the brower and participate in DKG themselves.

Yep exactly. The stuff in the protocol crate used to all be in server, and the idea was that rather than re-implement the protocol running logic for the client side and have to maintain it in two places, we have a common crate which is used on both the front and back end. In practice this has turned out to be quite tricky and i'm not sure it was the best decision.

And how would you recommend to review this PR? Anywhere that should be looked at first or more deeply?

idk, i think your comments are already useful and you understand pretty well whats going on here so just check for red flags

HCastano

Some smalls comments and questions but otherwise looks good! I'm happy to get this merged and continue improving on it iteratively.

In practice this has turned out to be quite tricky and i'm not sure it was the best decision.

Would you be able to elaborate a bit more on this? What other approach would you suggest?

HCastano · 2023-11-15T19:42:53Z

crypto/protocol/Cargo.toml

+]
+
+[lib]
+crate-type=["cdylib", "rlib"]


Why do we need to specify rlib here?

Without it, we can't use this crate as a dependency of server. cdylib will make it a dynamic library and if we want to also be able to compile it statically we need to make that explicit.

crypto/protocol/src/execute_protocol.rs

HCastano · 2023-11-15T19:51:33Z

crypto/protocol/src/protocol_transport/mod.rs

+                    // TODO if this fails, the ws connection has been dropped during the protocol
+                    // we should inform the chain of this.


I know it's not part of your PR, but maybe you know.

What would "informing the chain" look like in this case? I'm also wondering if the chain should even care in this case since it's mostly a TSS <-> TSS problem right?

ya but we should be noting like hey one party failed, this is a griefing issue and if a specific validator keeps getting reported for this then should be removed from the validating pool and potentially slashed

crypto/protocol/src/protocol_transport/mod.rs

HCastano · 2023-11-15T20:08:13Z

crypto/protocol/src/user/wasm.rs

+#[cfg_attr(feature = "wasm", wasm_bindgen)]
+pub async fn run_signing_protocol(
+    key_share: String,
+    sig_uid: String,


Doing this as a follow up sounds fine to me

HCastano · 2023-11-15T20:13:27Z

crypto/protocol/src/user/wasm.rs

+}
+
+/// Details of a validator intended for use on JS
+/// This differs from [crate::ValidatorInfo] only in that the fields must be private


I want to say the field visibility shouldn't matter for deserializing, but I'm not 100% sure. We can file this as part of a general type-unification issue and deal with it later

HCastano · 2023-11-15T20:18:15Z

crypto/server/src/user/tests.rs

+
+/// For testing running the protocol on wasm, spawn a process running nodejs and pass
+/// the protocol runnning parameters as JSON as a command line argument
+pub async fn spawn_user_participates_in_signing_protocol(


What do you think about moving some of these tests out to an integration test folder instead? Seems strange to me that unit tests would be spawning a NodeJS process for example.

Although you did say that eventually those should be run as part of the SDK test suite, right?

crypto/server/src/user/tests.rs

JesseAbram · 2023-11-15T20:25:53Z

crypto/kvdb/Cargo.toml

@@ -22,7 +22,7 @@ zeroize         ={ version="1.4", features=["zeroize_derive"], default-features=
 rpassword       ={ version="5.0", default-features=false }
 scrypt          ={ version="0.11.0", default-features=false, features=["std"] }
 chacha20poly1305={ version="0.9", features=["alloc"], default-features=false }
-synedrion       ={ git="ssh://git@github.com/entropyxyz/synedrion.git", tag="v0.0.9" }
+synedrion       ={ git="ssh://git@github.com/entropyxyz/synedrion.git", branch="fix-32bit" }


Personally I don't love going to branches, ok for now to get this in but please make an issue to fix and fix when you can

JesseAbram · 2023-11-15T20:28:22Z

crypto/protocol/src/execute_protocol.rs

@@ -173,7 +175,9 @@ pub async fn execute_dkg(
 ) -> Result<KeyShare<KeyParams>, ProtocolExecutionErr> {
    let party_ids: Vec<PartyId> =
        threshold_accounts.clone().into_iter().map(PartyId::new).collect();
+    // TODO #499 this will panic if we give an out of bounds my_idx


is this not handled with the bad key share?

Yeah i think you are right, i don't think theres currently any way this function could get called with an bad value for my_idx

JesseAbram · 2023-11-15T20:28:36Z

crypto/protocol/src/execute_protocol.rs

@@ -261,6 +265,7 @@ pub async fn execute_proactive_refresh(
 ) -> Result<KeyShare<KeyParams>, ProtocolExecutionErr> {
    let party_ids: Vec<PartyId> =
        threshold_accounts.clone().into_iter().map(PartyId::new).collect();
+    // TODO #499 this will panic if we give an out of bounds my_idx


JesseAbram · 2023-11-15T20:30:31Z

crypto/protocol/src/protocol_transport/mod.rs

+                    // TODO if this fails, the ws connection has been dropped during the protocol
+                    // we should inform the chain of this.


ya but we should be noting like hey one party failed, this is a griefing issue and if a specific validator keeps getting reported for this then should be removed from the validating pool and potentially slashed

JesseAbram · 2023-11-15T20:35:44Z

crypto/protocol/src/user/wasm.rs

+#[cfg_attr(feature = "wasm", wasm_bindgen)]
+pub async fn run_signing_protocol(
+    key_share: String,
+    sig_uid: String,


https://github.com/paritytech/ss58-registry/tree/main also I think this lib would work but ya im cool with making an issue and having someone follow up

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

ameba23 · 2023-11-16T08:37:29Z

In practice this has turned out to be quite tricky and i'm not sure it was the best decision.

Would you be able to elaborate a bit more on this? What other approach would you suggest?

The problem is that this contains networking code (websockets), which i expect is going to cause problems on some platforms. I would have liked to have this crate only deal with protocol-level stuff, (handshaking, encryption, and synedrion protocol execution) and leave the networking stuff out of it. But that would mean having a wasm interface with some sort of channels or streams (nodejs streams?) which i don't know how to do.

Adds a missing `CHANGELOG` entry for #414.

ameba23 added 3 commits September 28, 2023 11:13

Dependencies for wasm

6af9bcf

User functions for wasm

07e9d84

Refactor

9f29044

ameba23 marked this pull request as draft September 28, 2023 09:31

vercel bot deployed to Preview September 28, 2023 09:53 View deployment

Add wasm-bindgen fn to user participates dkg fn

0fe5658

vercel bot deployed to Preview September 29, 2023 10:06 View deployment

Attempt test for nodejs

26785c0

vercel bot deployed to Preview October 3, 2023 09:20 View deployment

Add makefile

6eb6032

vercel bot deployed to Preview October 3, 2023 11:17 View deployment

Try using wasm-bindgen-derive

8397ca6

vercel bot deployed to Preview October 5, 2023 21:54 View deployment

fjarri reviewed Oct 6, 2023

View reviewed changes

crypto/protocol/src/user/wasm.rs Show resolved Hide resolved

Nodejs test

6e94a05

vercel bot deployed to Preview October 9, 2023 09:36 View deployment

Add extern crate alloc

8ebd491

vercel bot deployed to Preview October 13, 2023 18:01 View deployment

ameba23 added 2 commits October 14, 2023 08:48

Put extern crate alloc at top level lib.rs

64fa127

Fmt

126ab7f

vercel bot deployed to Preview October 14, 2023 07:57 View deployment

Fix types for wasm run_dkg fn and add fn to run signing protocol from JS

d386865

vercel bot deployed to Preview October 18, 2023 10:27 View deployment

vercel bot deployed to Preview October 18, 2023 11:11 View deployment

WIP Update nodejs test

40086fc

vercel bot deployed to Preview October 18, 2023 12:21 View deployment

vercel bot deployed to Preview November 14, 2023 10:14 View deployment

ameba23 added 2 commits November 14, 2023 11:15

Update server tests for changed ValidatorInfo type

f39babd

Rm comment

e8ad6b4

ameba23 commented Nov 14, 2023

View reviewed changes

ameba23 marked this pull request as ready for review November 14, 2023 10:44

ameba23 requested review from HCastano and JesseAbram November 14, 2023 10:46

vercel bot deployed to Preview November 14, 2023 11:05 View deployment

HCastano reviewed Nov 14, 2023

View reviewed changes

Change package name of nodejs test module

fdd9fb4

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

vercel bot deployed to Preview November 14, 2023 20:25 View deployment

HCastano approved these changes Nov 15, 2023

View reviewed changes

JesseAbram approved these changes Nov 15, 2023

View reviewed changes

ameba23 and others added 5 commits November 16, 2023 08:49

Rename argument to open_ws_connection fn

5eb0354

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

Rename argument to open_ws_connection fn

36b7361

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

Update doc comments following review

e5f6c21

Spawn functions in tests should not return Option

7a47208

Variable name, comments following review

e796e4c

ameba23 mentioned this pull request Nov 16, 2023

entropy-protocol's execute_dkg and execute_proactive_refresh functions will panic if we give an out of bounds my_idx #499

Closed

vercel bot deployed to Preview November 16, 2023 08:56 View deployment

ameba23 merged commit 3f87f9b into master Nov 16, 2023
10 checks passed

ameba23 deleted the bindings-user-participates branch November 16, 2023 10:07

HCastano added a commit that referenced this pull request Nov 27, 2023

Adds a missing CHANGELOG entry for #414.

a348cee

HCastano added a commit that referenced this pull request Nov 28, 2023

Add missing CHANGELOG entry for #414 (#527)

f8169ec

Adds a missing `CHANGELOG` entry for #414.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wasm bindings for user to participate in DKG and signing protocols #414

Wasm bindings for user to participate in DKG and signing protocols #414

ameba23 commented Sep 28, 2023 •

edited

vercel bot commented Sep 28, 2023 •

edited

fjarri commented Oct 6, 2023

ameba23 commented Oct 8, 2023

ameba23 Nov 14, 2023

HCastano left a comment

HCastano Nov 14, 2023

ameba23 Nov 14, 2023

HCastano Nov 15, 2023

ameba23 Nov 16, 2023

ameba23 commented Nov 14, 2023 •

edited

HCastano left a comment

HCastano Nov 15, 2023

ameba23 Nov 16, 2023

HCastano Nov 15, 2023

JesseAbram Nov 15, 2023

HCastano Nov 15, 2023

HCastano Nov 15, 2023

HCastano Nov 15, 2023

ameba23 Nov 16, 2023

JesseAbram Nov 15, 2023

ameba23 Nov 16, 2023

JesseAbram Nov 15, 2023

ameba23 Nov 16, 2023

JesseAbram Nov 15, 2023

JesseAbram Nov 15, 2023

JesseAbram Nov 15, 2023

ameba23 commented Nov 16, 2023

		// TODO if this fails, the ws connection has been dropped during the protocol
		// we should inform the chain of this.

Wasm bindings for user to participate in DKG and signing protocols #414

Wasm bindings for user to participate in DKG and signing protocols #414

Conversation

ameba23 commented Sep 28, 2023 • edited

vercel bot commented Sep 28, 2023 • edited

fjarri commented Oct 6, 2023

ameba23 commented Oct 8, 2023

Choose a reason for hiding this comment

HCastano 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

ameba23 commented Nov 14, 2023 • edited

HCastano 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

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

ameba23 commented Nov 16, 2023

ameba23 commented Sep 28, 2023 •

edited

vercel bot commented Sep 28, 2023 •

edited

ameba23 commented Nov 14, 2023 •

edited