-
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
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 :)
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
.
src/modules/musig/session_impl.h
Outdated
secp256k1_gej_set_ge(&aggnoncej[1], &aggnonce[1]); | ||
/* fin_nonce = aggnonce[0] + b*aggnonce[1] */ | ||
secp256k1_scalar_set_b32(b, noncehash, NULL); | ||
secp256k1_gej_set_infinity(&fin_nonce_ptj); |
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: this line shouldn't be needed, as fin_nonce_ptj
is overwritten in the next line anyways
secp256k1_gej_set_infinity(&fin_nonce_ptj); |
fwiw, out of curiosity I tried deduplicating the calculation of _musig_nonce_process
and for the "effective" nonce in the partial sig verification _musig_partial_sig_verify
) on the following branch: theStack@419c05c
Not sure if its worth it to pick this up, but if yes, it probably needs a better name for the helper function 😅
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.
Yes, done. Also added your helper function.
*/ | ||
typedef struct { | ||
unsigned char data[36]; | ||
} secp256k1_musig_partial_sig; |
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.
It's unclear to me if these structs have any alignment needs. On the surface it looks like they don't but that might make the performance worse on some architectures? If it's not the case then it could be still nice to explicitly say they are unaligned in the doc. If it's the case maybe instead align them and say it in the doc?
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.
Hm, I don't see how anyone could get the idea that any type in a public header would have stricter alignment than what C anyway requires (unless otherwise specified in the header, of course). And The implied alignment is 1 in this case.
As far as I remember, we "read" from these char arrays into our internal data structures (the performance penalty is probably negligible), and so alignment shouldn't matter for performance either.
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.
Someone coming from Rust and being paranoid about everything. :D Thanks for clarifying, though I think a documentation comment would still be nice.
I have a question that's maybe more about MuSig itself not this PR but it may have implications that could affect the API. But firstly I want to apologize for the question not being well-researched upfront, maybe this was already addressed somewhere, I don't have that much time right now and I feel like I should speak up before this merges and cements the API. From my understanding the reason we have this "toxic data" of making sure nonces are not reused is that the counterparties could send nonces that are computed as sum of their nonce and inverse of our nonce which would allow them to compute private key. However can't the same be achieved by simply proving that they know the discrete log? Which IIUC could be done by simply single-signing the message they're going to sign (or some HMAC of it to avoid cryptographic interactions) producing a 64-byte proof. And once the nonces are proven to be random they can be deterministically derived from the message, the set of public keys and the private key. So the tradeoff would be to make all signing messages 64-byte larger to eliminate the risk that the state leaks somehow and with it also the keys or that nonces get reused by accident because of HW failure or a bug. For an application I intend to use MuSig for I could easily have a kilobyte proof and it would still be worth the tradeoff. But I don't see any API here providing this. Sorry again if I missed it or if there's some serious problem in my reasoning that's documented somewhere. If this can be refuted with a link, you can just paste the link. Thanks for consideration and all the hard work that went into this! |
I think you're mixing up two different issues. What you describe doesn't lead to an attack. The attacker can choose nonces that cancel out a honest signer's nonces, but the only effect this will have is that the attacker won't be able to come up with a valid partial signature and thus the protocol will fail. This cancellation attack you describe is a concern not with nonces, but with the individual public keys in the key aggregation. Indeed, cancelling a key of an honest signer would work if the aggregate public key was just the sum of the individual keys. (This is called a "rouge key attack" or literally "a key cancellation attack"). But MuSig2 eliminates this attack vector by using a key aggregation function that is not just the sum of the keys, but instead a sum with random coefficients. But yes, letting everyone prove knowledge of the discrete logarithm of their public individual public key using a Schnorr signature is a different way to eliminate rogue key attacks. This is used in SpeedyMuSig: https://eprint.iacr.org/2021/1375.pdf The drawback of this method is that it doesn't suffice to send these additional Schnorr signatures around during the signing protocol. For this to be secure, the signatures need to be there already at the key aggregation stage, which is possibly performed by a different party. In MuSig2, anyone (i.e., not just the involved signers) can combine some keys A and B into an aggregate key and use it, e.g., send money to it, and it will be ensured that A and B can only spend it if they work together. In SpeedyMuSig, the party who performs the key aggregation needs to see the Schnorr signatures valid for A and B before they can securely send money to the aggregate key A+B. This complicates key management and is a potential footgun (people could send to A+B without checking the additional Schnorr sigs). edit: For the actual problem with reusing nonces, see either the paper or https://delvingbitcoin.org/t/how-many-nonce-reuse-before-exposing-your-musig2-private-key/217, but both are rather involved... |
Ah, OK I think I understand better now. Thanks for explaining! What I was really thinking was if all nonces are deterministic then for the same key and same message they produce the same signature so how could an attacker possibly extract additional information from that? What I was missing is there's no way for a signer to know if the pubnonce sent by another party was generated deterministically except for having some kind of ZKP that would be likely expensive to construct and verify. Still it would be nice to not have to store the secnonce on a permanent storage so maybe a viable solution for cases when there's one "unreliable" signer (random user with a smartphone that could kill the app at any time) and other parties are "reliable" (run on server 24/7, can hold the secnonces in RAM) would be for the "unreliable" signer to collect all nonces first and compute his own as an HMAC(secret_key, message || aggregate_pubkey || all_nonces_except_signers) and be the last one who sends pubnonce. The other signers hold their secnonces in RAM in the meantime. Does that make sense? |
Proving that the nonces were generated deterministically from the session parameters is the idea behind deterministic signing in MuSig-DN and Exponent-VRFs and Their Applications.
It sounds like you're describing a variant of the "Deterministic and Stateless Signing for a Single Signer"-mode of BIP MuSig. In this mode, one signer is able to derive the nonces deterministically from the session parameters and the pubnonces of all other signers. |
265297b
to
5bd0891
Compare
@jonasnick thanks for references!
Yes, that's what I want! From what I can see it's not currently supported by the API. Is this planned or can it be implemented some other way? |
SECP256K1_API int secp256k1_musig_pubnonce_parse( | ||
const secp256k1_context *ctx, | ||
secp256k1_musig_pubnonce *nonce, | ||
const unsigned char *in66 |
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.
Wouldn't it be better to declare this as const unsigned char (*in66)[66]
? Aside from clarity it could help static analysis tools or perhaps bindings generators to generate correct bindings.
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 an interesting point. Right now, we consistently use declarations of the form const unsigned char *in66
consistently in the library. I prefer to have a standard across the whole API and would avoid just changing this in the musig module.
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 makes sense. Might be nice to change all of them at some point unless there's a problem I'm missing.
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 cannot pass arrays as arguments in C; they collapse into pointers. The syntax
int some_func(char c[32]) {
...
}
is for all intents and purposes equivalent to int some_func(char *c) { ...}
, including the fact that sizeof(c)
will be 4 or 8 bytes (same as sizeof(char*)
). Because it is so confusing, it's generally considered misleading to use arrays as arguments in C code.
EDIT: Oh, you're not suggesting this, but int some_func(char (*c)[32])
instead. That does work, and equivalent after compilation, but it does mean an additional &
at the caller, and an additional *
inside the callee. Can you elaborate on the (potential) advantages for static analysis or binding generators?
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.
For instance in Rust we have bindgen
, a tool that generates bindings automatically. I've tried to pass it this header file:
void foo(const unsigned char *arg);
void bar(const unsigned char arg[32]);
void baz(const unsigned char (*arg)[32]);
And it generated this code:
extern "C" {
pub fn foo(arg: *const ::std::os::raw::c_uchar);
}
extern "C" {
pub fn bar(arg: *const ::std::os::raw::c_uchar);
}
extern "C" {
pub fn baz(arg: *const [::std::os::raw::c_uchar; 32usize]);
}
As you can see, the last one has a different type. And while it's the same from ABI perspective, there's a real difference between them in Rust since you can directly pass in &array
without any casts because the types match making the code more obviously correct.
Currently, we aren't using bidgen
in rust-secp256k1 but we might in the future and I'd be surprised if similar tools don't exist in other languages.
Also I'm not familiar with the current state of static analysis in C but I think a reasonable static analyzer should be able to see that statements like out[33] = 42
or baz(&twentyone_item_array)
are obviously wrong. gcc
already complains about the these when compiling with -O2 -W -Wall -Warray-bounds
.
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.
Interesting!
It's unfortunate that it's an API break to introduce this (but indeed not an ABI break, I think).
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.
Oh, I found that bindgen
has an option to generate *const [c_uchar; 32]
instead of *const c_uchar
for arguments defined as const unsigned char arg[32]
, so maybe even that is already valuable? (IIUC not API-breaking) I think it has documentation value too.
It looks like cppcheck is also able to detect mistakes when you use these (I haven't tested but I found a PR that supposedly does it.)
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.
Mostly looked through the interface and documentation.
doc/musig.md
Outdated
2. Call `secp256k1_musig_pubkey_agg` with the pubkeys of all participants. | ||
3. Optionally add a (Taproot) tweak with `secp256k1_musig_pubkey_xonly_tweak_add` and a plain tweak with `secp256k1_musig_pubkey_ec_tweak_add`. | ||
4. Generate a pair of secret and public nonce with `secp256k1_musig_nonce_gen` and send the public nonce to the other signers. | ||
5. Someone (not necessarily the signer) aggregates the public nonce with `secp256k1_musig_nonce_agg` and sends it to the 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.
Nit: the public nonces.
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.
fixed
doc/musig.md
Outdated
|
||
The aggregate signature can be verified with `secp256k1_schnorrsig_verify`. | ||
|
||
Note that steps 1 to 5 can happen before the message to be signed is known to the 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.
I think it makes sense to stress that the normal (safer) mode is to construct the nonces after the message is known, rather than only pointing out the alternative here.
Suggested language:
Steps 1 through 5 above can happen before or after the message to be signed is known to the signers. Wherever possible, it is recommended to only generate the nonces after the message is known, as it has more defense-in-depth measures, but requires two communication rounds at signing time. The alternative, generating the nonces in a pre-processing step before the message is known, removes those measures, but means signing can happen non-interatively.
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, improved the wording.
* misuse-resistance. The extra_input32 argument can be used to provide | ||
* additional data that does not repeat in normal scenarios, such as the | ||
* current time. | ||
* 3. Avoid copying (or serializing) the secnonce. This reduces the possibility |
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 case someone has no way around storing session data on disk somewhere, would the recommendation be:
- To store/load the
secnonce
to/from disk (violating rule 3 here, plus risking platform dependence). - To store/load the
session_secrand32
to/from disk and re-runsecp256k1_musig_nonce_gen
to obtain the samesecnonce
(violating rule 1 here, and duplicating computation).
I suspect some users will have not have the ability to leave the secnonce
in volatile memory, so it may be useful to give advice on how to do that (plus reiterate the dangers of not wiping the stored data after signing).
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 don't provide the ability to serialize the secnonce right now, which prevents users from storing/loading from disk. Maybe worth a separate PR?
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.
Well they can just store the raw bytes on disk, which works, but risks platform-dependency issues.
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.
If we provide the on store/load the secnonce from disk, we should also provide de/serialization functions because our API docs instruct users to not read the raw bytes. I had considered this out of scope for this initial PR because it seems dangerous. Especially since so far no one requested this feature, just "don't store to disk" seems to be a sensible answer.
Actually one developer asked me about the missing de/serialization functions for the secnonce but further discussion on their motivation for requesting this revealed that they planned to build a very insecure protocol because they had entirely misunderstood what nonce reuse actually is. In the end they agreed that they should not store the secnonce to disk and changed their implementation accordingly.
Also I had hoped that someone would eventually come up with a more clever API that can protect from misuse better than simple de/serialization functions (at least in some cases). But maybe that just doesn't exist.
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 totally reasonable, but I don't think you have answered my question. If someone is somehow not able to keep everything in volatile memory, they have two (bad) options (store secnonce, or store session_secrand32), and both options violate a rule. Which of the two would you, as API and protocol designer, recommend people take?
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.
If someone really needs to do this right now, then I would probably recommend option 2. In contrast to option 1, if you reuse session_secrand
but you're signing for a different message or aggregate public key, you're protected.
One downside of that method is that session_secrand
has no "in-memory protection", in the sense that the session_secrand
buffer is not cleared after nonce_generation. We do wipe the secnonce
after signing. Maybe we should change nonce generation to overwrite the session_secrand
buffer?
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 agree that option 2 is better for the reasons mentioned.
We do wipe the
secnonce
after signing. Maybe we should change nonce generation to overwrite thesession_secrand
buffer?
That sounds reasonable to me.
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.
Seems like a good idea, yes.
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. The secrand buffer is invalidated whenever nonce_gen returns 1. The alternative is to invalidate whenever a session_secrand argument is given, which is a tiny bit more tedious to test and document. Anyway, if nonce_gen returns 0, then the secnonce is invalid as well, so reusing the secrand buffer in that case is not an issue.
* In: | ||
* session_secrand32: a 32-byte session_secrand32 as explained above. Must be unique to this | ||
* call to secp256k1_musig_nonce_gen and must be uniformly random. | ||
* seckey: the 32-byte secret key that will later be used for signing, if |
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.
If provided, what happens if the seckey
does not match the pubkey
argument? (also applies to secp256k1_musig_nonce_gen_counter
).
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.
The musig_nonce_gen functions do not check if the seckey matches the given pubkey.
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.
But will for example the resulting signature be invalid, or does doing this risk leaking private 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.
If the wrong public key is provided, signing will fail. This works because the public key is appended to the secnonce output of the nonce_gen
functions and partial_sign
will compare this public key with the public key the signer is trying to sign for.
Providing the optional seckey
argument is a defense-in-depth measure to strengthen nonce generation against a low entropy session_secrand
argument. If provided, the seckey
is simply hashed into the nonce and does not have an effect on whether the resulting signature is valid.
So, the idea behind the defense-in-depth measure is that if the seckey
does correspond to the pubkey
argument and has high entropy, but the session_secrand
argument has low entropy (but does not repeat), the generated nonce is secure. On the other hand, if the seckey
does not match the pubkey
, nonce generation is only insecure if both the provided seckey
and secrand
have low entropy. Providing a wrong seckey
cannot negatively affect nonce generation as long as secrand
has enough entropy.
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.
If the wrong public key is provided, signing will fail. This works because the public key is appended to the secnonce output of the
nonce_gen
functions andpartial_sign
will compare this public key with the public key the signer is trying to sign for.
Let me add that the reason for this (and for the need for a pubkey argument in the first place) is that it prevents a vulnerability that could occur when people sign with tweaked individual keys, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/021000.html (you should remember the story).
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.
The docs currently just say "Returns: 0 if the arguments are invalid and 1 otherwise" Perhaps we should note that a pubkey/seckey mismatch does not count as invalid, but that it will be caught later during signing.
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.
Perhaps we should note that a pubkey/seckey mismatch does not count as invalid, but that it will be caught later during signing.
I added a sentence, but it may be confusing. A pubkey/seckey mismatch in nonce generation may not be caught during signing, because the signer can still provide the correct seckey for signing.
753cf07
to
49f69cb
Compare
49f69cb
to
fdc0960
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.
Code-review ACK fdc0960
Reviewed once again that the code matches the specification in BIP327 and also took a closer look on the tests. Regarding the interface discussion #1479 (comment), I wonder if it's acceptable for now to not support (and not even mention) the use-case where users can't store the secnonce
in volatile memory by now, and leave that for a follow-up? If a suggested way to work around that is described in the API docs ("if you really need to do this..."), that could be seen by users as invitation to break the rules. On the other hand, if nothing is mentioned, users might break the rules anyway and do it in an even worse way, so not sure what is better. Anyways, happy to re-review on changes in that area (e.g. suggestions in #1479 (comment)).
for (i = 0; i < np; i++) { | ||
unsigned char ser[33]; | ||
size_t ser_len = sizeof(ser); | ||
if (!secp256k1_ec_pubkey_serialize(ctx, ser, &ser_len, pks[i], SECP256K1_EC_COMPRESSED)) { |
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.
note for other reviewers: it's intentional to use the public API function for serializing a pubkey here (as opposed to the internal one called _eckey_pubkey_serialize
, which is faster and used at all other places in the musig module) , as we want to check the validity of the input pubkeys of secp256k1_musig_pubkey_agg
.
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.
* misuse-resistance. The extra_input32 argument can be used to provide | ||
* additional data that does not repeat in normal scenarios, such as the | ||
* current time. | ||
* 3. Avoid copying (or serializing) the secnonce. This reduces the possibility |
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 agree that option 2 is better for the reasons mentioned.
We do wipe the
secnonce
after signing. Maybe we should change nonce generation to overwrite thesession_secrand
buffer?
That sounds reasonable to me.
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.
static const
?
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
doc/musig.md
Outdated
## API misuse | ||
|
||
The musig API is designed to be as misuse resistant as possible. | ||
However, the MuSig protocol has some additional failure modes (mainly due to interactivity) that do not appear in single-signing. |
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: reads a bit unnatural. How about "that do not apply to single signatures"?
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.
improved the wording
doc/musig.md
Outdated
|
||
The musig API is designed to be as misuse resistant as possible. | ||
However, the MuSig protocol has some additional failure modes (mainly due to interactivity) that do not appear in single-signing. | ||
While the results can be catastrophic (e.g. leaking of the secret key), it is unfortunately not possible for the musig implementation to rule out all such failure modes. |
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: "ruling out" sounds like something a human does when assessing a protocol. How about "prevent"?
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
## Key Aggregation and (Taproot) Tweaking | ||
|
||
Given a set of public keys, the aggregate public key is computed with `secp256k1_musig_pubkey_agg`. | ||
A plain tweak can be added to the resulting public key with `secp256k1_ec_pubkey_tweak_add` by setting the `tweak32` argument to the hash defined in BIP 32. Similarly, a Taproot tweak can be added with `secp256k1_xonly_pubkey_tweak_add` by setting the `tweak32` argument to the TapTweak hash defined in BIP 341. |
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.
(If true) perhaps mention that can be combined and/or invoked multiple times if the setting calls for that?
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
include/secp256k1_musig.h
Outdated
|
||
/** Opaque data structure that caches information about public key aggregation. | ||
* | ||
* Guaranteed to be 197 bytes in size. It can be safely copied/moved. No |
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.
Worth pointing out (here and elsewhere) that despite being copyable/movable, the encoding is platform- and version-dependent?
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 moved the "It can be safely copied/moved." to the above general section on opaque data structures; right after the sentence that the representation is platform dependent (which is the same as the doc for the other opaque data structures in the API).
include/secp256k1_musig.h
Outdated
|
||
/** Serialize a signer's public nonce | ||
* | ||
* Returns: 1 when the nonce could be serialized, 0 otherwise |
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 it's only possible that 0 is returned when the API is misused? If so, I think it's better to say "1 always" (in the sense that API-compliant callers do not need to care about the return value). Same below.
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.
fixed
* misuse-resistance. The extra_input32 argument can be used to provide | ||
* additional data that does not repeat in normal scenarios, such as the | ||
* current time. | ||
* 3. Avoid copying (or serializing) the secnonce. This reduces the possibility |
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.
Seems like a good idea, yes.
include/secp256k1_musig.h
Outdated
* | ||
* This function differs from `secp256k1_musig_nonce_gen` by accepting a | ||
* non-repeating counter value instead of a secret random value. This requires | ||
* that the seckey argument (which is part of the keypair argument in |
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.
This comment seems outdated now.
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.
Outdated in what way?
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.
There is no seckey
argument anymore.
EDIT: Ok, it does day "which is part of the keypair argument in secp256k1_musig_nonce_gen_counter
", but that sounds a bit confusing given that this is the documentation for that function.
What about "This requires that a private key is provided to secp256k1_musig_nonce_gen_counter
(through the keypair argument), as opposed to secp256k1_musig_nonce_gen
where the seckey argument is optional."?
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 like the suggestion, s/private key/secret key is more consistent.
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.
Replaced the sentence with the suggestion.
fdc0960
to
55d3dc0
Compare
examples/musig.c
Outdated
pubnonces[i] = &signer[i].pubnonce; | ||
|
||
secure_erase(seckey, sizeof(seckey)); | ||
secure_erase(session_secrand, sizeof(session_secrand)); |
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.
with the latest change to secp256k1_musig_nonce_gen
, this line wouldn't be needed anymore (doesn't hurt to keep it though for demonstrating good practices, I presume)
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.
Good catch. I removed that line. Imo explicitly erasaing secrand is more overkill than good practice.
src/modules/musig/session_impl.h
Outdated
is_zero = secp256k1_is_zero_array(&secnonce->data[4], 32); | ||
is_zero |= secp256k1_is_zero_array(&secnonce->data[36], 32); |
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.
This is what I meant. :) Yeah, it's not so much simpler, but I think it's an improvement overall.
If you want to make it one line simpler, you could do this (not sure if it's better):
is_zero = secp256k1_is_zero_array(&secnonce->data[4], 32); | |
is_zero |= secp256k1_is_zero_array(&secnonce->data[36], 32); | |
is_zero = secp256k1_is_zero_array(&secnonce->data[4], 2 * 32); |
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.
Oh, yes! Replaced the two lines with the suggestion.
55d3dc0
to
5e55f09
Compare
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/.