multisig brainstorming #118

pro-wh · 2021-05-01T00:09:51Z

due to scope changes, this will fix #120

codecov · 2021-05-01T00:10:44Z

Codecov Report

Merging #118 (f3606a1) into main (0a5add6) will increase coverage by 1.12%.
The diff coverage is 78.09%.

❗ Current head f3606a1 differs from pull request most recent head 9388aa1. Consider uploading reports for the commit 9388aa1 to get more accurate results

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   68.77%   69.90%   +1.12%     
==========================================
  Files          51       53       +2     
  Lines        2729     2931     +202     
==========================================
+ Hits         1877     2049     +172     
- Misses        833      862      +29     
- Partials       19       20       +1

Impacted Files	Coverage Δ
runtime-sdk/src/dispatcher.rs	`0.00% <0.00%> (ø)`
runtime-sdk/src/module.rs	`34.04% <0.00%> (-1.52%)`	⬇️
tests/runtimes/simple-consensus/src/lib.rs	`0.00% <0.00%> (ø)`
tests/runtimes/simple-keyvalue/src/lib.rs	`0.00% <0.00%> (ø)`
runtime-sdk/src/types/transaction.rs	`10.00% <15.00%> (+6.00%)`	⬆️
runtime-sdk/src/modules/consensus/mod.rs	`75.36% <50.00%> (ø)`
client-sdk/go/types/transaction.go	`64.51% <62.50%> (+3.80%)`	⬆️
runtime-sdk/src/modules/core/mod.rs	`63.33% <92.30%> (+4.84%)`	⬆️
client-sdk/go/types/crypto.go	`70.00% <93.33%> (+35.00%)`	⬆️
runtime-sdk/src/crypto/multisig/mod.rs	`93.93% <93.93%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a5add6...9388aa1. Read the comment docs.

runtime-sdk/src/modules/accounts/types.rs

pro-wh · 2021-05-04T00:07:31Z

update: multisig is moved out to an alternative at the transaction structure level, where SignerInfo.public_key is now an enum allowing you to choose between PublicKey and a multisig config. Likewise, UnverifiedTransaction.1 T is now an enum allowing you to choose between Signature and a multisig signature set. we're 🤏 this close to putting "multisig v0" as a signing algorithm next to ed25519 and secp256k1, but not quite. in this state, we can at least use PublicKey and Signature inside the multisig structures without getting all cyclic.

this all won't build yet, as I haven't updated the surrounding code

pro-wh · 2021-05-04T23:57:05Z

update: spreading the changes through the rest of the rust part of the sdk, added a verification routine:

check single-key vs multisig consistency -> zip -> extract present signatures for multisig -> bail if weight is below threshold -> flatten public key and signature lists -> call batch verify

pro-wh · 2021-05-05T23:42:35Z

update: adding sanity checker for multisig configurations, based on the oasis-core version. need to add calls to it.

runtime-sdk/src/crypto/multisig/mod.rs

kostko

Don't forget to add #[serde(deny_unknown_fields)] to all the serializable things.

pro-wh · 2021-05-12T18:49:04Z

yup, gotta add that, gotta add renames, gotta add doc comments

pro-wh · 2021-05-13T00:12:02Z

opening this for review at this stage. here's the current idea for a data model.

some feedback is in already:

need doc comments
need some conventional attributes on rust structures
need multisig config size limit

we need some discussion on how multisig conceptually fits in to the existing multiple-accounts-i.e.-nonces being able to sign.

in this draft it's not anticipated that a principal that has a key should distinguish between signing any specific SignerInfo slot or any specific entry inside a multisig proof.

btw, the terminology: 'proof,' as introduced in this PR refers to a generalization of (i) a signature satisfying the authentication requirement of a single-public-key account or a (ii) set of signatures satisfying the authentication requirement of a multisig configuration. also 'config' and 'solo.' let me know your thoughts on this too.

and a little weirdness with the encoding of unverified transactions: it's focused on lists that match up one-to-one with the various signer info of the tx, but this results in needing duplicate signatures if the same key would be used in solo and/or multiple multisig proofs. maybe we could flatten and dedup these signatures, if we prefer it that way. it would be more work on the verification side to match them up, maybe. let me know your thoughts on this too

runtime-sdk/src/types/transaction.rs

kostko

we need some discussion on how multisig conceptually fits in to the existing multiple-accounts-i.e.-nonces being able to sign.

I guess the use case would be that one account pays for fees and another account authorizes some action?

btw, the terminology: 'proof,' as introduced in this PR refers to a generalization of (i) a signature satisfying the authentication requirement of a single-public-key account or a (ii) set of signatures satisfying the authentication requirement of a multisig configuration. also 'config' and 'solo.' let me know your thoughts on this too.

Yeah auth proof and multisig config seem fine. Added a comment regarding "solo".

and a little weirdness with the encoding of unverified transactions: it's focused on lists that match up one-to-one with the various signer info of the tx, but this results in needing duplicate signatures if the same key would be used in solo and/or multiple multisig proofs. maybe we could flatten and dedup these signatures, if we prefer it that way. it would be more work on the verification side to match them up, maybe. let me know your thoughts on this too

I would rather keep it simple and not dedup.

client-sdk/go/modules/consensusaccounts/consensus_accounts.go

runtime-sdk/src/types/transaction.rs

pro-wh · 2021-05-14T23:31:57Z

adding some limits for number of authinfo slots in tx and number of signer slots in multisig config

Co-authored-by: Jernej Kos <[email protected]>

pro-wh · 2021-05-18T23:54:40Z

The new spec naming looks good to me. Also the signature limits are nice, would be nice to also charge gas per signature slot.

tracking in #141

client-sdk/go/types/address.go

client-sdk/go/types/crypto.go

kostko · 2021-05-19T10:44:45Z

client-sdk/go/types/transaction.go

-		if !si.PublicKey.Equal(pk) {
-			continue
-		}
+		switch {


Oh I see, so now you sign all the slots that correspond to the signer.

yeah 🤷 the typescript client is different in style

runtime-sdk/src/crypto/multisig/mod.rs

runtime-sdk/src/modules/core/mod.rs

client-sdk/go/types/crypto.go

Yawning · 2021-05-19T14:06:02Z

client-sdk/go/types/crypto.go

+
+// Batch checks that enough signers have signed and returns vectors of public keys and signatures
+// for batch verification of those signatures. This internally calls `Verify`.
+func (mc *MultisigConfig) Batch(signatureSet [][]byte) ([]PublicKey, [][]byte, error) {


Why not do the actual verification? I'm of mixed opinion if a batch should fail if it as any invalid signatures regardless of threshold or not. The way this is setup right now is an all-or-nothing design as far as I can tell.

(I assume allowing a MultisigConfig with mixed public-key types is deliberate, though this will complicate batch verification logic.)

we'll have to have an additional step to separate the public key types. but are we moving away from batch signature verification in general?

this isn't meant to be all-or-nothing. it'll leave out the nils from the signature set (Option in the rust side). we'd advise that the transaction sender nil out invalid signatures on their own and avoid submitting a transaction with some valid and some invalid signatures

I'll open an issue to discuss this

outside of cargo fmt's jurisdiction, who knew? Co-authored-by: Jernej Kos <[email protected]>

kostko reviewed May 1, 2021

View reviewed changes

runtime-sdk/src/modules/accounts/types.rs Outdated Show resolved Hide resolved

kostko linked an issue May 3, 2021 that may be closed by this pull request

Multisig accounts #91

Closed

pro-wh force-pushed the pro-wh/feature/multisig branch from 20ca853 to 1ccdf7d Compare May 4, 2021 00:03

pro-wh changed the title ~~(scratch)~~ multisig brainstorming May 4, 2021

pro-wh force-pushed the pro-wh/feature/multisig branch from 1ccdf7d to b18baba Compare May 4, 2021 23:54

pro-wh force-pushed the pro-wh/feature/multisig branch from b18baba to 531270b Compare May 5, 2021 23:41

pro-wh commented May 5, 2021

View reviewed changes

runtime-sdk/src/crypto/multisig/mod.rs Show resolved Hide resolved

pro-wh force-pushed the pro-wh/feature/multisig branch 4 times, most recently from 4b8956f to d8ec563 Compare May 12, 2021 00:47

kostko reviewed May 12, 2021

View reviewed changes

pro-wh force-pushed the pro-wh/feature/multisig branch 4 times, most recently from 061356e to 761717b Compare May 12, 2021 21:36

pro-wh marked this pull request as ready for review May 13, 2021 00:03

pro-wh requested a review from ptrus as a code owner May 13, 2021 00:03

ptrus reviewed May 13, 2021

View reviewed changes

runtime-sdk/src/types/transaction.rs Outdated Show resolved Hide resolved

kostko reviewed May 13, 2021

View reviewed changes

client-sdk/go/modules/consensusaccounts/consensus_accounts.go Outdated Show resolved Hide resolved

runtime-sdk/src/types/transaction.rs Outdated Show resolved Hide resolved

runtime-sdk/src/types/transaction.rs Show resolved Hide resolved

kostko mentioned this pull request May 14, 2021

tests/e2e: Add transaction generator #135

Merged

pro-wh force-pushed the pro-wh/feature/multisig branch from 1431571 to 88af89c Compare May 14, 2021 23:30

pro-wh force-pushed the pro-wh/feature/multisig branch from 0f3d93a to 5c396bf Compare May 17, 2021 23:51

pro-wh added 3 commits May 18, 2021 11:32

tests: add multisig e2e scenario

09761b7

runtime-sdk: call multisig verify as part of batch

9fbd448

go: add multisig Verify

6811576

pro-wh force-pushed the pro-wh/feature/multisig branch 2 times, most recently from 2a5a2eb to d50dd17 Compare May 18, 2021 20:59

pro-wh and others added 5 commits May 18, 2021 14:28

runtime-sdk: add approve_utx step

169cad8

runtime-sdk: solo -> signature

e4688d5

runtime-sdk: approve_utx -> approve_unverified_tx

a6fe76b

Co-authored-by: Jernej Kos <[email protected]>

tests: new u64::MAX

86b484c

runtime-sdk: test core approve_unverified_tx

d2d9513

pro-wh force-pushed the pro-wh/feature/multisig branch from d50dd17 to d2d9513 Compare May 18, 2021 21:28

pro-wh added 2 commits May 18, 2021 14:34

runtime-sdk: add serde attributes

73f2d1d

go: add helpers for AddressSpec variants

070ee51

pro-wh force-pushed the pro-wh/feature/multisig branch 2 times, most recently from a2e312e to 095a131 Compare May 18, 2021 23:49

pro-wh mentioned this pull request May 18, 2021

use gas per signature slot #141

Closed

kostko approved these changes May 19, 2021

View reviewed changes

Yawning reviewed May 19, 2021

View reviewed changes

pro-wh force-pushed the pro-wh/feature/multisig branch from b131bf5 to 62c4901 Compare May 19, 2021 21:09

pro-wh and others added 6 commits May 19, 2021 14:16

docs pass for multisig

4547433

ts-web/rt: add auth limit error codes

acbdc24

go: move some AppendSign branches to new func

9577f92

tests: split up overlong test

c0f1e08

runtime-sdk: verify -> validate_basic

3769223

runtime-sdk: uses rearrangement

9388aa1

outside of cargo fmt's jurisdiction, who knew? Co-authored-by: Jernej Kos <[email protected]>

pro-wh force-pushed the pro-wh/feature/multisig branch from 62c4901 to 9388aa1 Compare May 19, 2021 21:16

pro-wh mentioned this pull request May 19, 2021

batch signature verification #143

Open

pro-wh merged commit 8452c50 into main May 19, 2021

pro-wh deleted the pro-wh/feature/multisig branch May 19, 2021 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multisig brainstorming #118

multisig brainstorming #118

pro-wh commented May 1, 2021 •

edited

Loading

codecov bot commented May 1, 2021 •

edited

Loading

pro-wh commented May 4, 2021 •

edited

Loading

pro-wh commented May 4, 2021

pro-wh commented May 5, 2021 •

edited

Loading

kostko left a comment

pro-wh commented May 12, 2021

pro-wh commented May 13, 2021 •

edited

Loading

kostko left a comment

pro-wh commented May 14, 2021

pro-wh commented May 18, 2021

kostko May 19, 2021

pro-wh May 19, 2021

Yawning May 19, 2021

pro-wh May 19, 2021

pro-wh May 19, 2021

pro-wh May 19, 2021

multisig brainstorming #118

multisig brainstorming #118

Conversation

pro-wh commented May 1, 2021 • edited Loading

codecov bot commented May 1, 2021 • edited Loading

Codecov Report

pro-wh commented May 4, 2021 • edited Loading

pro-wh commented May 4, 2021

pro-wh commented May 5, 2021 • edited Loading

kostko left a comment

Choose a reason for hiding this comment

pro-wh commented May 12, 2021

pro-wh commented May 13, 2021 • edited Loading

kostko left a comment

Choose a reason for hiding this comment

pro-wh commented May 14, 2021

pro-wh commented May 18, 2021

kostko May 19, 2021

Choose a reason for hiding this comment

pro-wh May 19, 2021

Choose a reason for hiding this comment

Yawning May 19, 2021

Choose a reason for hiding this comment

pro-wh May 19, 2021

Choose a reason for hiding this comment

pro-wh May 19, 2021

Choose a reason for hiding this comment

pro-wh May 19, 2021

Choose a reason for hiding this comment

pro-wh commented May 1, 2021 •

edited

Loading

codecov bot commented May 1, 2021 •

edited

Loading

pro-wh commented May 4, 2021 •

edited

Loading

pro-wh commented May 5, 2021 •

edited

Loading

pro-wh commented May 13, 2021 •

edited

Loading