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

Add MuSig module #35

Merged
merged 5 commits into from Feb 7, 2019

Conversation

Projects
None yet
4 participants
@jonasnick
Copy link
Contributor

commented Dec 22, 2018

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).

Add trivial ecmult_multi algorithm. It is selected when no scratch sp…
…ace is given and just multiplies and adds the points.

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

* 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.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Jan 13, 2019

Member

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

  1. In a public session, use musig_session_initialize_public to initialize the nonce_commitment field.
  2. 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.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jan 16, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Jan 16, 2019

Member

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.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jan 16, 2019

Author Contributor

I rephrased the comment according to your suggestion.

@apoelstra

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

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

the adaptor signature implementation should already match the paper

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

I added state machine tests and overflow tests. Now the tests cover the musig module as much as possible (https://htmlpreview.github.io/?https://raw.githubusercontent.com/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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

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.

Show resolved Hide resolved include/secp256k1_musig.h Outdated
Show resolved Hide resolved include/secp256k1_musig.h Outdated
@apoelstra

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

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

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

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

@jonasnick jonasnick force-pushed the jonasnick:2018-10-musig branch from 174e6ca to 3860876 Jan 29, 2019

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Picked most recent Schnorr commits from upstream and squashed MuSig.

@real-or-random
Copy link
Contributor

left a comment

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).

Show resolved Hide resolved include/secp256k1_musig.h Outdated

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

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jan 29, 2019

Contributor

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.

* 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

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jan 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jan 30, 2019

Author Contributor

Yeah I suggest we remove that part for now

* 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.

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jan 29, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jan 30, 2019

Author Contributor

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

* 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.

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jan 29, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@apoelstra

apoelstra Feb 1, 2019

Member

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.

Show resolved Hide resolved include/secp256k1_musig.h Outdated
Show resolved Hide resolved src/modules/musig/main_impl.h
Show resolved Hide resolved src/modules/musig/main_impl.h
unsigned char buf[32];
secp256k1_sha256_initialize(&sha);
secp256k1_sha256_write(&sha, ell, 32);
while (idx > 0) {

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jan 29, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jan 30, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jan 30, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Feb 1, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Feb 1, 2019

Member

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

This comment has been minimized.

Copy link
@jonasnick

jonasnick Feb 1, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Feb 1, 2019

Author Contributor

... 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 :).

This comment has been minimized.

Copy link
@real-or-random

real-or-random Feb 1, 2019

Contributor

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...)

This comment has been minimized.

Copy link
@jonasnick

jonasnick Feb 4, 2019

Author Contributor

using 4 bytes now

Show resolved Hide resolved src/modules/musig/main_impl.h Outdated
@real-or-random
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

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

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

The changes look good to me. 👍

@jonasnick jonasnick changed the title WIP: Add MuSig module Add MuSig module Jan 31, 2019

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

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.

@real-or-random
Copy link
Contributor

left a comment

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.

Show resolved Hide resolved src/modules/musig/example.c Outdated
@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

Added commit with nit fix

@@ -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.

This comment has been minimized.

Copy link
@real-or-random

real-or-random Feb 4, 2019

Contributor

"at most"

This comment has been minimized.

Copy link
@jonasnick

jonasnick Feb 4, 2019

Author Contributor

fixed

@@ -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.

This comment has been minimized.

Copy link
@real-or-random

real-or-random Feb 4, 2019

Contributor

should be consistent with the above

This comment has been minimized.

Copy link
@jonasnick

jonasnick Feb 4, 2019

Author Contributor

fixed

@@ -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) {

This comment has been minimized.

Copy link
@real-or-random

real-or-random Feb 4, 2019

Contributor

here we overflow :)

This comment has been minimized.

Copy link
@jonasnick

jonasnick Feb 4, 2019

Author Contributor

fixed

@jonasnick jonasnick force-pushed the jonasnick:2018-10-musig branch from 630e1ac to c5e9fa2 Feb 4, 2019

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

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

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

ACK c5e9fa2

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Added a fix

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

Good catch! ACK 950054e

@apoelstra

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

ACK 950054e

@jonasnick jonasnick force-pushed the jonasnick:2018-10-musig branch from 950054e to 2fc700a Feb 6, 2019

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

squashed

@apoelstra

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

ACK 2fc700a

@apoelstra apoelstra merged commit d5e22a5 into ElementsProject:secp256k1-zkp Feb 7, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

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

@markblundeberg

This comment has been minimized.

Copy link

commented Feb 18, 2019

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

@apoelstra

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.