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

Conversation

jonasnick
Copy link
Contributor

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

…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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rephrased the comment according to your suggestion.

@apoelstra
Copy link
Contributor

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
Copy link
Contributor Author

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

the adaptor signature implementation should already match the paper

@jonasnick
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@apoelstra
Copy link
Contributor

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
Copy link
Contributor Author

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

@jonasnick
Copy link
Contributor Author

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
Copy link
Contributor

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

@jonasnick
Copy link
Contributor Author

Picked most recent Schnorr commits from upstream and squashed MuSig.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/modules/musig/main_impl.h Show resolved Hide resolved
src/modules/musig/main_impl.h Show resolved Hide resolved
unsigned char buf[32];
secp256k1_sha256_initialize(&sha);
secp256k1_sha256_write(&sha, ell, 32);
while (idx > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jonasnick jonasnick Feb 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using 4 bytes now

src/modules/musig/main_impl.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

jonasnick 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
Copy link
Collaborator

The changes look good to me. 👍

@jonasnick jonasnick changed the title WIP: Add MuSig module Add MuSig module Jan 31, 2019
@jonasnick
Copy link
Contributor Author

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.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@jonasnick
Copy link
Contributor Author

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"at most"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be consistent with the above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we overflow :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@jonasnick
Copy link
Contributor Author

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

@real-or-random
Copy link
Collaborator

ACK c5e9fa2

@jonasnick
Copy link
Contributor Author

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
Copy link
Contributor Author

Added a fix

@real-or-random
Copy link
Collaborator

Good catch! ACK 950054e

@apoelstra
Copy link
Contributor

ACK 950054e

@jonasnick
Copy link
Contributor Author

squashed

@apoelstra
Copy link
Contributor

ACK 2fc700a

@apoelstra apoelstra merged commit d5e22a5 into BlockstreamResearch:secp256k1-zkp Feb 7, 2019
@real-or-random
Copy link
Collaborator

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

@markblundeberg
Copy link

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

@apoelstra
Copy link
Contributor

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

tomtau pushed a commit to crypto-com/secp256k1-zkp that referenced this pull request Jul 9, 2020
…ecp-build-flags

build.rs: change build flags to eliminate compiler warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants