crypto: Add multisig to sui #7110

joyqvq · 2023-01-04T02:35:37Z

Multisig support in Sui: Now sui accepts Multisig signatures. [enum GenericSignature] now replaced all [enum Signature] for user signature verification logics.

Keytool command now supports 1) Generate multisig address 2) Combine single signatures to a multisig.

Design: #7187
PR: #7110

Step by step testing in localnet: https://gist.github.com/joyqvq/5febd8deb5cacc6f47afa1253224b7da

Main changes are in multisig.rs, signature.rs crypto.rs

Next step:

Typescript util to compute multisig address
Typescript util to combine partial sigs
Explorer rendering of multisig

vercel · 2023-01-04T02:35:42Z

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

4 Ignored Deployments

Name	Status	Updated
explorer	⬜️ Ignored (Inspect)	Feb 14, 2023 at 6:40PM (UTC)
explorer-storybook	⬜️ Ignored (Inspect)	Feb 14, 2023 at 6:40PM (UTC)
frenemies	⬜️ Ignored (Inspect)	Feb 14, 2023 at 6:40PM (UTC)
wallet-adapter	⬜️ Ignored (Inspect)	Feb 14, 2023 at 6:40PM (UTC)

kchalkias

Let's have a design doc for this first as there are many functionality decisions (ie weighted multisig)

joyqvq · 2023-01-06T10:06:46Z

#7187 linked design doc in this issue

LeviHHH · 2023-01-20T11:36:19Z

We should also consider supporting secp256r1 signature verification in the move library.

joyqvq · 2023-01-27T16:18:09Z

We should also consider supporting secp256r1 signature verification in the move library.

#7773

crates/sui-core/src/consensus_validator.rs

kchalkias · 2023-02-09T09:53:36Z

crates/sui-types/src/base_types.rs

@@ -258,6 +259,25 @@ impl From<&PublicKey> for SuiAddress {
    }
 }

+/// A Multisig address is the first 20 bytes of `threshold || pk_1 || weight_1 || ... || pk_n || weight_n`


... is the first 20 bytes of the hash of flag || threshold || pk_1....

crates/sui-types/src/base_types.rs

kchalkias · 2023-02-09T09:57:44Z

crates/sui-types/src/crypto.rs

+        // Unwrap below is safe because the bytes are directly read from the signature bytes from [enum Signature].
+        match self.scheme() {
+            SignatureScheme::ED25519 => Ok(CompressedSignature::Ed25519(
+                Ed25519Signature::from_bytes(bytes).unwrap(),


are these unwrap always successful?

yeah i added a line above at 697, bc here we always use the signature_bytes()

edit: unwrap removed

crates/sui-types/src/multisig.rs

kchalkias · 2023-02-09T10:09:01Z

crates/sui-types/src/multisig.rs

+            || pks.len() != weights.len()
+            || pks.len() > MAX_SIGNER_IN_MULTISIG
+        {
+            return Err(SuiError::InvalidSignature {


do we do these checks even when deserializing?

good point. added to from_bytes to validate the multisig itself and its pk

kchalkias · 2023-02-09T10:10:21Z

crates/sui-types/src/multisig.rs

+        threshold: ThresholdUnit,
+    ) -> Result<Self, SuiError> {
+        if pks.is_empty()
+            || weights.is_empty()


I believe we should even disallow any zero weights, no multisig key should carry any weight = 0 pk.

good point, added tests

crates/sui-types/src/sui_serde.rs

benr-ml · 2023-02-09T15:41:34Z

crates/sui-types/src/multisig.rs

+
+/// The struct that contains the public key used for authenticating a multisig.
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
+pub struct MultiPublicKey {


nit, and obviously very subjective - I view "MultiSig" as the scheme name, like ECDSA or EdDSA, and so MultiSigPublicKey sounds clearer to me here.

benr-ml · 2023-02-09T15:42:42Z

crates/sui-types/src/multisig.rs

+/// This way multisig (and future Authenticators) can implement its own `verify`.
+#[enum_dispatch(AuthenticatorTrait)]
+#[derive(Debug, Clone, PartialEq, Eq, JsonSchema, Hash)]
+pub enum GenericSignature {


nit, and also very subjective - maybe enum Signature { MultiSig, SingleSig }?

agreed on both naming. renaming Signature -> SingleSig is quite invasive, will do this in a later PR.

benr-ml · 2023-02-09T16:00:43Z

crates/sui-types/src/multisig.rs

+        let mut weight_sum = 0;
+        let message = bcs::to_bytes(&value).expect("Message serialization should not fail");
+
+        for (sig, i) in self.sigs.iter().zip(&self.bitmap) {


Does it assume here that the sigs are sorted according to their public keys?

also, we depend here on bitmap to check that we don't have 2 signatures for the same public key, right?

yeah sigs are assumed sorted

yeah roaring bitmap prevents inserting the same value twice.

however, it is technically possible if the multisig pubkey itself is constructed with two same pks (with different different index in bitmap ). i dont know if there is a good reason someone would do that but didnt check to fail for this.

yeah as the user controls their multisig pubKey, there is no good incentive to have a double pubKey. Maybe an edge case is if a user wanted to double their max weight for some peculiar auth structure, but it's fine to omit that check.

joyqvq · 2023-02-14T16:11:00Z

@kchalkias @benr-ml addressed most of the comments PTAL

kchalkias

Great job Joy!

kchalkias · 2023-02-14T16:22:23Z

crates/sui-types/src/multisig.rs

+        let mut weight_sum = 0;
+        let message = bcs::to_bytes(&value).expect("Message serialization should not fail");
+
+        for (sig, i) in self.sigs.iter().zip(&self.bitmap) {


yeah as the user controls their multisig pubKey, there is no good incentive to have a double pubKey. Maybe an edge case is if a user wanted to double their max weight for some peculiar auth structure, but it's fine to omit that check.

kchalkias · 2023-02-14T16:31:06Z

crates/sui-types/src/multisig.rs

+                error: "Invalid address".to_string(),
+            });
+        }
+        let mut weight_sum = 0;


I'd specifically add the type here (u16) just for readability. I understand we have weights being u8 atm and we aggregate them into a u16, but in any case I'd also add a test where all keys have max weight (just to capture any future update where for some reason we allow 256+ pubKeys)

kchalkias reviewed Jan 4, 2023

View reviewed changes

joyqvq force-pushed the multisig branch from 547fa7c to 201620a Compare January 6, 2023 04:53

joyqvq mentioned this pull request Jan 6, 2023

Support Multisig in Sui #7187

Closed

joyqvq force-pushed the multisig branch 4 times, most recently from 00d8556 to 4ff57d7 Compare January 11, 2023 09:48

joyqvq marked this pull request as ready for review January 11, 2023 10:34

joyqvq force-pushed the multisig branch from 4ff57d7 to 8ad22a5 Compare January 11, 2023 10:34

joyqvq requested review from patrickkuo and benr-ml as code owners January 11, 2023 10:34

joyqvq force-pushed the multisig branch from 8ad22a5 to 4bfbabf Compare January 11, 2023 15:22

joyqvq requested a review from kchalkias January 11, 2023 15:35

joyqvq force-pushed the multisig branch 2 times, most recently from 4c14cbd to e8ca0e3 Compare January 12, 2023 10:04

joyqvq force-pushed the multisig branch 2 times, most recently from 67d4a17 to 8cbb4a2 Compare January 30, 2023 22:54

joyqvq force-pushed the multisig branch from 8cbb4a2 to 62a7734 Compare February 9, 2023 01:06

kchalkias reviewed Feb 9, 2023

View reviewed changes

benr-ml reviewed Feb 9, 2023

View reviewed changes

crates/sui-types/src/sui_serde.rs Show resolved Hide resolved

benr-ml reviewed Feb 9, 2023

View reviewed changes

joyqvq force-pushed the multisig branch 2 times, most recently from 8036589 to 44c51ba Compare February 14, 2023 16:09

joyqvq requested a review from kchalkias February 14, 2023 16:11

joyqvq requested a review from benr-ml February 14, 2023 16:11

joyqvq force-pushed the multisig branch from 44c51ba to ab94960 Compare February 14, 2023 16:23

kchalkias approved these changes Feb 14, 2023

View reviewed changes

crypto: Add multisig as an authenticator

0358216

joyqvq force-pushed the multisig branch from ab94960 to 0358216 Compare February 14, 2023 18:39

joyqvq enabled auto-merge (squash) February 14, 2023 18:39

joyqvq merged commit 818406c into main Feb 14, 2023

joyqvq deleted the multisig branch February 14, 2023 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: Add multisig to sui #7110

crypto: Add multisig to sui #7110

joyqvq commented Jan 4, 2023 •

edited

Loading

vercel bot commented Jan 4, 2023 •

edited

Loading

kchalkias left a comment

joyqvq commented Jan 6, 2023

LeviHHH commented Jan 20, 2023

joyqvq commented Jan 27, 2023

kchalkias Feb 9, 2023

kchalkias Feb 9, 2023

joyqvq Feb 10, 2023 •

edited

Loading

joyqvq Feb 14, 2023

kchalkias Feb 9, 2023

joyqvq Feb 14, 2023

kchalkias Feb 9, 2023

joyqvq Feb 14, 2023

benr-ml Feb 9, 2023

benr-ml Feb 9, 2023

joyqvq Feb 10, 2023 •

edited

Loading

benr-ml Feb 9, 2023

benr-ml Feb 9, 2023

joyqvq Feb 14, 2023

joyqvq Feb 14, 2023 •

edited

Loading

kchalkias Feb 14, 2023

joyqvq commented Feb 14, 2023

kchalkias left a comment

kchalkias Feb 14, 2023

kchalkias Feb 14, 2023

crypto: Add multisig to sui #7110

crypto: Add multisig to sui #7110

Conversation

joyqvq commented Jan 4, 2023 • edited Loading

vercel bot commented Jan 4, 2023 • edited Loading

kchalkias left a comment

Choose a reason for hiding this comment

joyqvq commented Jan 6, 2023

LeviHHH commented Jan 20, 2023

joyqvq commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyqvq Feb 10, 2023 • edited Loading

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

joyqvq Feb 10, 2023 • edited Loading

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

joyqvq Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyqvq commented Feb 14, 2023

kchalkias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyqvq commented Jan 4, 2023 •

edited

Loading

vercel bot commented Jan 4, 2023 •

edited

Loading

joyqvq Feb 10, 2023 •

edited

Loading

joyqvq Feb 10, 2023 •

edited

Loading

joyqvq Feb 14, 2023 •

edited

Loading