Add MuSig module #35

jonasnick · 2018-12-22T22:38:57Z

This PR adds a module that implements a Schnorr-based multi-signature scheme called MuSig.
It is based on an original implementation by @apoelstra and builds on the schnorrsig module (bitcoin-core/secp256k1#558) and trivial ecmult_multi (bitcoin-core/secp256k1#580).

~~The PR is WIP because it needs more state machine tests (there are quite a few lines that are currently not covered).~~

…ace is given and just multiplies and adds the points.

apoelstra · 2019-01-13T18:55:52Z

include/secp256k1_musig.h

+ *    takes an array of signer data structs and an array of commitments, and sets
+ *    each signer data struct's `nonce_commitment` field to the corresponding
+ *    commitment. If the signer data is used in a public session, the
+ *    `nonce_commitment` is set set in musig_session_initialize_public.


I think this comment should be rearranged so that it says, in order,

In a public session, use musig_session_initialize_public to initialize the nonce_commitment field.

In a non-public session (signing session? participatory session?) the function secp256k1_musig_get_public_nonce should also be used, which as a side-effect also returns the signer's public nonce. This ensures that the public nonce is not exposed until all commitments have been received.

I feel like it's a bit weird that we describe a "workflow of a data structure" which kind of explains how to use the module but not entirely. How to use the module isn't explained anywhere else at all, maybe we should do that instead.

Agreed, it'd be good to write a .md with some instructions. I'm working on a threshold signature scheme that reuses as much of this module as possible, and it's weird to find myself referring back to data structures' comments to remember how everything fits together.

I rephrased the comment according to your suggestion.

include/secp256k1_musig.h

src/modules/musig/main_impl.h

apoelstra · 2019-01-14T22:04:15Z

ACK except

nits
missing QA/tests
changes to adaptor signature equation needed to match the Malavolta et al paper

I should also mention that the use of large non-opaque structs may be something we regret; but on the other hand, it is possible for most users to use them as though they were opaque, and IMHO the effort required to opaque-ify them (e.g. replacing the whole struct with a giant unsigned char array and then de/serializing objects throughout the entire rest of the code) would not be worth the resulting obfuscation.

jonasnick · 2019-01-16T11:43:37Z

changes to adaptor signature equation needed to match the Malavolta et al paper

the adaptor signature implementation should already match the paper

jonasnick · 2019-01-23T14:02:09Z

I added state machine tests and overflow tests. Now the tests cover the musig module as much as possible (https://htmlpreview.github.io/?https://github.com/raw/jonasnick/secp256k1-zkp/24049468577e3821035b3e71d56b2201123096f0/coverage.src_modules_musig_main_impl.h.html).

I think the only thing that's missing right now is documentation for using the module.

jonasnick · 2019-01-23T15:41:58Z

The MuSig code should now have the invariant that for any session identified by a unique session id, either compute_messagehash returns an error or the result is constant.

include/secp256k1_musig.h

src/modules/musig/main_impl.h

apoelstra · 2019-01-26T00:31:38Z

These tests look great! Very thorough. It took me a few reads to understand what you were doing by "combining nonces with a different signer set than was initialized", but I'm not sure the comments could be improved. This is a somewhat subtle thing and the reader probably just needs to read the code.

If you fix the nits and rebase, I believe we'll be ready to merge.

jonasnick · 2019-01-28T10:38:07Z

addressed nits, improved comments in tests, fixed bug in test

jonasnick · 2019-01-28T10:43:45Z

I noticed that the current API doesn't support sign-to-contract. I think we'd need to add a noncedata and noncefp argument to session_initialize. This would probably use the s2c_commit_context that's introduced in the sign-to-contract commit for libsecp (https://github.com/bitcoin-core/secp256k1/pull/572/files#diff-222d6d707e263c79c462fb223b0e3ddcR83). I'm ok with adding that in another PR.

apoelstra · 2019-01-28T21:50:26Z

Yeah, I think we'll move s2c to another PR.

…ning, verification and batch verification. [0] https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki

jonasnick · 2019-01-29T11:06:17Z

Picked most recent Schnorr commits from upstream and squashed MuSig.

real-or-random

More later tonight. Don't feel overwhelmed by all the nits in the comments, that can be addresses later (and I'm able to spend time on this if you want me to).

include/secp256k1_musig.h

real-or-random · 2019-01-29T13:59:32Z

include/secp256k1_musig.h

+
+/** Data structure containing data related to a signing session resulting in a single
+ * signature.
+ *


I have multiple comments about the comments. None of this is crucial and I think we can postpone improving the docs but I write my thoughts down here.

real-or-random · 2019-01-29T14:01:27Z

include/secp256k1_musig.h

+ * keep this structure in memory can use the provided API functions for a safe
+ * standard workflow.
+ *
+ * A signer who goes offline and needs to import/export or save/load this structure


"Offline" sounds like it's related to the network, and is not properly defined, see above.

Yeah I suggest we remove that part for now

real-or-random · 2019-01-29T14:06:11Z

include/secp256k1_musig.h

+ * this is to attach the output of a monotonic non-resettable counter (hardware
+ * support is needed for this). Increment the counter before each output and
+ * encrypt+sign the entire package. If a package is deserialized with an old counter
+ * state or bad signature it should be rejected.


I think this can be expanded a lot:

Should the counter be stored in the package?

It's not clear what "encrypt+sign" is (and even if it's clear, this is hard to implement correctly for the user). I think it should be "authenticated symmetric encryption" where the key is derived from the signing key using a hash function.

Maybe we should just implement this functionality in a further PR to avoid that people screw up here.

That's what I was thinking, either remove the part or implement that functionality

real-or-random · 2019-01-29T14:07:32Z

include/secp256k1_musig.h

+ * any offline signer be usable for only a single session at once.
+ *
+ * Given access to such a counter, its output should be used as (or mixed into) the
+ * session ID to ensure uniqueness.


"mixed into" is another place where the user can screw up.

We could consider making the session ID variable length, this makes it easier.

Being variable length gives the user opportunity to have buffer overflows or misuse sizeof. Most of our APIs just take 32 bytes when they can get away with it. I think we should keep that here.

include/secp256k1_musig.h

src/modules/musig/main_impl.h

real-or-random · 2019-01-29T15:03:57Z

src/modules/musig/main_impl.h

+    unsigned char buf[32];
+    secp256k1_sha256_initialize(&sha);
+    secp256k1_sha256_write(&sha, ell, 32);
+    while (idx > 0) {


I don't think it's wrong but it seems somewhat weird not to write leading zeros to the hash. Or is there a good reason, maybe to keep it platform-independent? Just checking if this is intentional.

I don't think this has anything to do with platform independence (@apoelstra). I'm happy to fill the remaining bytes with 0's so we're always writing the full size_t.

At least this is one of the parts which is crucial for interoperability with other implementations, so we spend some thoughts. The advantage of the current implementation is that it supports an unlimited number of signers. But who needs more than 2^32 signers, and I think writing a x-bit integer in endianess y for some fixed x and y is less of a hassle to re-implement.

I just noticed that this is kind of crucial because we want the MuSig coefficient to be simple and easily implementable by others to come to some kind of interoperable MuSig standard.

Sure, we can always write 4 bytes. I did this so I wouldn't have to think about maximum signer size sets.

so I'm somewhat siding with the original implementation now. Added a comment The serialization of idx is least significant byte first and variable-length such that the last byte is non-zero.

... although, if you mess up the serialization you become immediately attackable via key cancellation. So maybe 4 bytes is better. Apparently it's to late for me right now to decide :).

Okay, so if Andrew didn't have a strong reason to use this format, I'm fine with both of course.
My slight preference is writing the 4 bytes because it's easier to re-implement (no while loop, no bit fiddling, you typically have some 32-bit type, etc...)

using 4 bytes now

src/modules/musig/main_impl.h

src/modules/musig/example.c

real-or-random

The API is really excellent. The state machine tests demonstrate nicely how safe it is.

By the way, the encryption of the state for offline signers could be a nice project for the next meeting.

jonasnick · 2019-01-30T12:29:59Z

I added two commits that should address most of @real-or-random's comments and resolved the nit comments.

real-or-random · 2019-01-30T13:17:47Z

The changes look good to me. 👍

jonasnick · 2019-01-31T16:26:02Z

I added a couple of commits that should resolve most of the remaining discussion (@real-or-random can you confirm and click the button if you agree?). The only comments where @apoelstra may want to weigh in is whether to hash the whole size_t (#35 (comment)) and whether we should change or remove some parts of the musig workflow documentation from this PR.

src/modules/musig/example.c

real-or-random

ACK with the two nits.
And yes, then we're done except for the size_t thing, which is fine currently but it will be good to hear @apoelstra's take on this.

For some reason (permissions?) I don't have a "mark as resolved" button in this PR but everything is resolved.

src/modules/musig/example.c

jonasnick · 2019-01-31T20:07:16Z

Added commit with nit fix

real-or-random · 2019-02-04T15:22:29Z

include/secp256k1_musig.h

@@ -161,7 +163,7 @@ SECP256K1_API int secp256k1_musig_pubkey_combine(
 *          pk_hash32: the 32-byte hash of the signers' individual keys (cannot be
 *                     NULL)
 *          n_signers: length of signers array. Number of signers participating in
- *                     the MuSig. Must be greater than 0.
+ *                     the MuSig. Must be greater than 0 and smaller than 2^32 - 1.


real-or-random · 2019-02-04T15:27:38Z

include/secp256k1_musig.h

@@ -262,7 +264,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_set_nonce(
 *                    set with `musig_set_nonce`. Array length must equal to `n_signers`
 *                    (cannot be NULL)
 *         n_signers: the length of the signers array. Must be the total number of
- *                    signers participating in the MuSig.
+ *                    signers participating in the MuSig. Must be greater than 0 and
+ *                    smaller than 2^32.


should be consistent with the above

real-or-random · 2019-02-04T15:32:58Z

src/modules/musig/tests_impl.h

@@ -122,6 +122,12 @@ void musig_api_tests(secp256k1_scratch_space *scratch) {
    CHECK(ecount == 8);
    CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, pk_hash, 0, 0, sk[0]) == 0);
    CHECK(ecount == 8);
+    /* If more than UINT32_MAX fits in a size_t, test that session_initialize
+     * rejects n_signers that high. */
+    if (SIZE_MAX > ((size_t) UINT32_MAX) + 1) {


here we overflow :)

jonasnick · 2019-02-04T16:15:52Z

fixed commit with fixed musig coefficient index and added commit to use a tagged hash

real-or-random · 2019-02-04T16:27:27Z

ACK c5e9fa2

jonasnick · 2019-02-05T15:37:20Z

While playing with rust-bitcoin/bitcoin_hashes I noticed that secp256k1_musig_sha256_init_tagged is wrong because the length of the already hashed data is not initialized (called bytes in the sha256 struct). I thought it wouldn't matter, but this length is hashed as part of the padding (https://github.com/ElementsProject/secp256k1-zkp/blob/secp256k1-zkp/src/hash_impl.h#L157). As a result, right now hash(tag, data) != hash_tag(data) where hash_tag is the hash function initialized by init_tagged.

jonasnick · 2019-02-05T16:43:39Z

Added a fix

real-or-random · 2019-02-05T19:31:27Z

Good catch! ACK 950054e

apoelstra · 2019-02-05T23:34:21Z

ACK 950054e

…ptor signatures.

jonasnick · 2019-02-06T11:27:44Z

squashed

apoelstra · 2019-02-07T00:10:53Z

ACK 2fc700a

real-or-random · 2019-02-12T16:08:36Z

We should also include the fixes in bitcoin-core/secp256k1#580

markblundeberg · 2019-02-18T21:30:26Z

Congrats, guys! I've been watching from afar and I like the session ID approach.

apoelstra · 2019-02-20T13:32:18Z

Will pull in bitcoin-core/secp256k1#580 after it is merged, as part of a rebase.

…ecp-build-flags build.rs: change build flags to eliminate compiler warnings

Add trivial ecmult_multi algorithm. It is selected when no scratch sp…

ed59fbe

…ace is given and just multiplies and adds the points.

jonasnick changed the title ~~Add MuSig module~~ WIP: Add MuSig module Dec 22, 2018

apoelstra reviewed Jan 13, 2019

View reviewed changes

include/secp256k1_musig.h Show resolved Hide resolved

apoelstra reviewed Jan 14, 2019

View reviewed changes

src/modules/musig/main_impl.h Outdated Show resolved Hide resolved

apoelstra reviewed Jan 14, 2019

View reviewed changes

src/modules/musig/main_impl.h Show resolved Hide resolved

apoelstra reviewed Jan 25, 2019

View reviewed changes

include/secp256k1_musig.h Outdated Show resolved Hide resolved

apoelstra reviewed Jan 25, 2019

View reviewed changes

include/secp256k1_musig.h Outdated Show resolved Hide resolved

apoelstra reviewed Jan 26, 2019

View reviewed changes

src/modules/musig/main_impl.h Outdated Show resolved Hide resolved

apoelstra added 2 commits January 29, 2019 10:59

add chacha20 function

8260800

Add schnorrsig module which implements BIP-schnorr [0] compatible sig…

f0e4bb9

…ning, verification and batch verification. [0] https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki

jonasnick force-pushed the 2018-10-musig branch from 174e6ca to 3860876 Compare January 29, 2019 11:05

real-or-random reviewed Jan 29, 2019

View reviewed changes