-
Notifications
You must be signed in to change notification settings - Fork 1k
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 module "musig" that implements MuSig2 multi-signatures (BIP 327) #1479
Conversation
f5a74fd
to
fb60ae9
Compare
ab7fc1e
to
eaf1e78
Compare
eaf1e78
to
70bb685
Compare
Rebased on top of master to get #1480 which allowed dropping a commit. Old state is preserved at https://github.com/jonasnick/secp256k1/tree/musig2-module-backup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs rebase :)
FWIW, we have JVM bindings on top of this branch in ACINQ/secp256k1-kmp#93 and an implementation of swap-in-potentiam (musig2 key-path with alternative delayed script path) in ACINQ/bitcoin-kmp#107 and everything is working fine, and the API is easy enough to use! |
70bb685
to
dd4932b
Compare
Rebased. Thanks @t-bast, that's good to hear. |
pubkeys_ptr[i] = &signers[i].pubkey; | ||
} | ||
printf("ok\n"); | ||
printf("Combining public keys..."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we recommend that users sort their pubkeys before aggregating them? The musig_pubkey_agg
API documentation simply says the user "can" do it.
If we recommend the sorting step, including it in the example file would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no catch-all recommendation. BIP327 says "The aggregate public key produced by KeyAgg (regardless of the type) depends on the order of the individual public keys. If the application does not have a canonical order of the signers, the individual public keys can be sorted with the KeySort algorithm to ensure that the aggregate public key is independent of the order of signers."
In other words: If in your application, the collection of pubkeys (or signers represented by them) is conceptually an (ordered) list, then don't bother with sorting. If in your application, the collection of pubkeys is conceptually an (unordered) set, i.e., the application doesn't want to care about the order of pubkeys, then sort to make sure the set has a canonical serialization.
Perhaps we can explain this somewhere in more detail, either in the API docs or in the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it would be nice to including sorting in the example for the following reasons:
- It provides a practical example on how to use the sorting API
- It's a good hook for adding a comment explaining what @real-or-random just explained above. Feels a bit nicer to have that comment in the example, vs the API docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added sorting and a comment.
2017bbe
to
4619706
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also using the heapsort commits in #1471. What do you think about splitting out the sort commits into their own PR? Also fine with cherry-picking for now, but figured I'd mention it since it might simplify both of our PRs.
src/modules/extrakeys/main_impl.h
Outdated
/* Suppress wrong warning (fixed in MSVC 19.33) */ | ||
#if defined(_MSC_VER) && (_MSC_VER < 1933) | ||
#pragma warning(push) | ||
#pragma warning(disable: 4090) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 26dde29 ("extrakeys: add secp256k1_pubkey_sort"):
Does it make sense to move this block into the secp256k1_sort
function? I ended up copying these lines while writing a secp256k1_silentpayments_recipient_sort
function, which made me realize anyone else would also need to copy these lines when writing a sort function for heapsort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean into secp256k1_hsort
? I'd guess the wrong warning is emitted when secp256k1_hsort
is called and therefore it would be to late when the warning was disabled in secp256k1_hsort
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 5e55f09
(as per $ git diff fdc09608036822afc1cebbe0c5b56cebf8ba508d 5e55f093118c2517f068841f5414f767fb35c7fe
)
As far as I can tell, none of the unadressed questions/comments are critical.
5e55f09
to
352cd3b
Compare
That's a good point. I couldn't discover a consistent pattern we're currently following (e.g, There are different rules we could follow:
I added it to a few more functions (following basically rules 2, 3, 4, 6), because removing the warning is backwards compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reACK 352cd3b
nit: musig_example
needs to be added to .gitignore
src/group_impl.h
Outdated
} | ||
|
||
static void secp256k1_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *data) { | ||
unsigned char zeros[64] = { 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you did this, but in the "wrong" commit
352cd3b
to
168c920
Compare
fixed |
reACK 168c920 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reACK 168c920
We have an open issue for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 168c920
Quoting sipa (see bitcoin-core#1479 (comment)): "When performing an EC multiplication A = aG for secret a, the resulting _affine_ coordinates of A are presumed to not leak information about a (ECDLP), but the same is not necessarily true for the Jacobian coordinates that come out of our multiplication algorithm." For the ECDH point multiplication result, the result in Jacobi coordinates should be cleared not only to avoid leaking the scalar, but even more so as it's a representation of the resulting shared secret.
a88aa935063 Merge bitcoin-core/secp256k1#1603: f can never equal -m 3660fe5e2a9 Merge bitcoin-core/secp256k1#1479: Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 168c92011f5 build: allow enabling the musig module in cmake f411841a46b Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 0be79660f38 util: add constant-time is_zero_array function c8fbdb1b972 group: add ge_to_bytes_ext and ge_from_bytes_ext ef7ff03407f f can never equal -m 4c57c7a5a95 Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code 472faaa8ee6 Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 292310fbb24 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 85e224dd97f group: add ge_to_bytes and ge_from_bytes 7c987ec89e6 cmake: Call `enable_testing()` unconditionally 6aa576515ef cmake: Delete `CTest` module git-subtree-dir: src/secp256k1 git-subtree-split: a88aa9350633c2d2472bace5c290aa291c7f12c9
4f64b9f4cd build: Drop no longer needed `-fvisibility=hidden` compiler option 5d978f1899 ci: Run `tools/symbol-check.py` 258dba9e11 test: Add `tools/symbol-check.py` 3144ba99f3 Introduce `SECP256K1_LOCAL_VAR` macro e2571385e0 Do not export `secp256k1_musig_nonce_gen_internal` e59158b6eb Merge bitcoin-core/secp256k1#1553: cmake: Set top-level target output locations 18f9b967c2 Merge bitcoin-core/secp256k1#1616: examples: do not retry generating seckey randomness in musig 5bab8f6d3c examples: make key generation doc consistent e8908221a4 examples: do not retry generating seckey randomness in musig 70b6be1834 extrakeys: improve doc of keypair_create (don't suggest retry) 01b5893389 Merge bitcoin-core/secp256k1#1599: bitcoin#1570 improve examples: remove key generation loop cd4f84f3ba Improve examples/documentation: remove key generation loops a88aa93506 Merge bitcoin-core/secp256k1#1603: f can never equal -m 3660fe5e2a Merge bitcoin-core/secp256k1#1479: Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 168c92011f build: allow enabling the musig module in cmake f411841a46 Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 0be79660f3 util: add constant-time is_zero_array function c8fbdb1b97 group: add ge_to_bytes_ext and ge_from_bytes_ext ef7ff03407 f can never equal -m c232486d84 Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" 26e4a7c214 cmake: Set top-level target output locations 4c57c7a5a9 Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code 472faaa8ee Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 292310fbb2 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 85e224dd97 group: add ge_to_bytes and ge_from_bytes 7c987ec89e cmake: Call `enable_testing()` unconditionally 6aa576515e cmake: Delete `CTest` module git-subtree-dir: src/secp256k1 git-subtree-split: 4f64b9f4cdf130d2c6d6b7bd855a04faf6f88e08
Quoting sipa (see bitcoin-core#1479 (comment)): "When performing an EC multiplication A = aG for secret a, the resulting _affine_ coordinates of A are presumed to not leak information about a (ECDLP), but the same is not necessarily true for the Jacobian coordinates that come out of our multiplication algorithm." For the ECDH point multiplication result, the result in Jacobi coordinates should be cleared not only to avoid leaking the scalar, but even more so as it's a representation of the resulting shared secret.
68b55209f1b Merge bitcoin-core/secp256k1#1619: musig: ctimetests: fix _declassify range for generated nonce points f0868a9b3d8 Merge bitcoin-core/secp256k1#1595: build: 45839th attempt to fix symbol visibility on Windows 1fae76f50c0 Merge bitcoin-core/secp256k1#1620: Remove unused scratch space from API 8be3839fb2e Remove unused scratch space from API 57eda3ba300 musig: ctimetests: fix _declassify range for generated nonce points e59158b6eb7 Merge bitcoin-core/secp256k1#1553: cmake: Set top-level target output locations 18f9b967c25 Merge bitcoin-core/secp256k1#1616: examples: do not retry generating seckey randomness in musig 5bab8f6d3c4 examples: make key generation doc consistent e8908221a45 examples: do not retry generating seckey randomness in musig 70b6be1834e extrakeys: improve doc of keypair_create (don't suggest retry) 01b5893389e Merge bitcoin-core/secp256k1#1599: bitcoin#1570 improve examples: remove key generation loop cd4f84f3ba8 Improve examples/documentation: remove key generation loops a88aa935063 Merge bitcoin-core/secp256k1#1603: f can never equal -m 3660fe5e2a9 Merge bitcoin-core/secp256k1#1479: Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 168c92011f5 build: allow enabling the musig module in cmake f411841a46b Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 0be79660f38 util: add constant-time is_zero_array function c8fbdb1b972 group: add ge_to_bytes_ext and ge_from_bytes_ext ef7ff03407f f can never equal -m c232486d84e Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" 26e4a7c2146 cmake: Set top-level target output locations 4c57c7a5a95 Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code 447334cb06d include: Avoid visibility("default") on Windows 472faaa8ee6 Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 292310fbb24 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 85e224dd97f group: add ge_to_bytes and ge_from_bytes 7c987ec89e6 cmake: Call `enable_testing()` unconditionally 6aa576515ef cmake: Delete `CTest` module git-subtree-dir: src/secp256k1 git-subtree-split: 68b55209f1ba3e6c0417789598f5f75649e9c14c
EDIT: based on #1518. Closes #1452. Most of the code is a copy from libsecp256k1-zkp. The API added in this PR is identical with the exception of two modifications:
scratch_space
argument fromsecp256k1_musig_pubkey_agg
. This argument was intended to allow usingecmult_multi
algorithms for key aggregation in the future. But at this point it's unclear whether thescratch_space
object will remain in its current form (see Rework or get rid of scratch space #1302).adaptor
argument ofmusig_nonce_process
was also removed.In contrast to the module in libsecp256k1-zkp, the module is non-experimental. I slightly cleaned up parts of the module, adjusted the code to the new definition of the VERIFY_CHECK macro and applied some simplifications that were possible because the module is now in the upstream repo (
ge_from_bytes
,ge_to_bytes
). You can follow the changes I made to the libsecp256k1-zkp module at https://github.com/jonasnick/secp256k1-zkp/commits/musig2-upstream/.