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
base: master
Are you sure you want to change the base?
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 :)
include/secp256k1_extrakeys.h
Outdated
/** Sort public keys using lexicographic order of their compressed | ||
* serialization. | ||
* | ||
* Returns: 0 if the arguments are invalid. 1 otherwise. | ||
* | ||
* Args: ctx: pointer to a context object | ||
* In: pubkeys: array of pointers to pubkeys to sort | ||
* n_pubkeys: number of elements in the pubkeys array | ||
*/ | ||
SECP256K1_API int secp256k1_pubkey_sort( | ||
const secp256k1_context *ctx, | ||
const secp256k1_pubkey **pubkeys, | ||
size_t n_pubkeys | ||
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2); | ||
|
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 this function should be in the main module because it works on secp256k1_pubkey
objects, and the comparison function is also there. (Extrakeys would make sense for x-only, I guess.) I don't think code size is an issue, the heap sort implementation should be tiny.
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.
That's reasonable. Done.
src/modules/musig/keyagg.h
Outdated
/* musig_ge_to_bytes_ext and musig_ge_from_bytes_ext are identical to ge_save and ge_load | ||
* except that they allow saving and loading the point at infinity */ | ||
static void secp256k1_musig_ge_to_bytes_ext(unsigned char *data, secp256k1_ge *ge); | ||
|
||
static void secp256k1_musig_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *data); |
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 these should be in group.h
, even though they'll be used only by the musig module. Infinity can be helpful in other contexts, and conceptually it's a group function.
Anyway, the comment needs to be updated ge_save
and ge_load
have been renamed.
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.
Agree, done.
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.
src/modules/musig/session_impl.h
Outdated
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.
Is there any specific reason for avoiding sha256 mid-state optimization in the musig_compute_noncehash
and nonce_function_musig
functions?
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.
Because apparently I had been to lazy so far :D. I added this optimization.
2017bbe
to
4619706
Compare
include/secp256k1_musig.h
Outdated
* If you do not provide a seckey, session_secrand32 _must_ be UNIFORMLY RANDOM | ||
* AND KEPT SECRET (even from other signers). |
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.
We should then drop the condition "If you do not provide a seckey,"
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.
We could also add a reference to the new function, maybe below the numbered list.
something like "If you don't have access to good randomness, but you have access to a non-repeating counter, then see ..."
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 both suggestions.
include/secp256k1_musig.h
Outdated
* 2. If you already know the seckey, message or aggregate public key | ||
* cache, they can be optionally provided to derive the nonce and increase | ||
* misuse-resistance. The extra_input32 argument can be used to provide |
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.
seckey is mandatory for this function anyway.
I think usually I would suggest moving this big explainer at the top of the module, so that it covers both functions and does not need to be repeated. But in this case of a big fat warning, it's probably better to keep it in the docstring, where it's harder to miss...
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.
dropped mention of seckey
include/secp256k1_musig.h
Outdated
* 1. The nonrepeating_cnt argument must be a counter value that never | ||
* repeats, i.e., you must never call `secp256k1_musig_nonce_gen_counter` | ||
* twice with the same seckey and nonrepeating_cnt value. |
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.
We could expand a bit to mention that this includes cases where the same seckey is used on multiple devices. (Any other precautions to mention?)
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.
done
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
.
3b6c90a
to
ae7ca3d
Compare
7d2591c Add secp256k1_pubkey_sort (Jonas Nick) Pull request description: This PR adds a `secp256k1_pubkey_sort` function the the public API which was originally part of the musig PR (#1479). However, I opened a separate PR because it adds internal functions that are also used by the WIP silent payments module. ACKs for top commit: sipa: ACK 7d2591c josibake: ACK 7d2591c real-or-random: ACK 7d2591c Tree-SHA512: d0e4464dc9cd4bdb35cc5d9bb4c37a7b71233328319165d49bc940d8d3394a2d74a43d2f73ee7bfe8f3f90a466ee8afcdca75cfbbf3969e218d76b89f4af55fb
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/.