ac1e36769dda3964f7294319ecb06fb5c414938d musig: turn off multiexponentiation for now (Jonas Nick)
3c79d97bd92ec22cc204ff5a08c9b0e5adda12e6 ci: increase timeout for macOS tasks (Jonas Nick)
22c88815c76e6edb23baf9401f820e1a944c3ecf musig: replace MuSig(1) with MuSig2 (Jonas Nick)
Pull request description:
The main commit comprises `905 insertions(+), 1253 deletions(-)`. The diff isn't as small as I had hoped, but that's mostly because it was possible to simplify the API quite substantially which required rewriting large parts. Sorry, almost all of the changes are in one big commit which makes the diff very hard to read. Perhaps best to re-review most parts from scratch.
A few key changes:
- Obviously no commitment round. No big session struct and no `verifier` sessions. No `signer` struct.
- There's a new `secnonce` struct that is the output of musig_nonce_gen and derived from a uniformly random session_id32. The derivation can be strengthened by adding whatever session parameters (combined_pk, msg) are available. The nonce function is my ad-hoc construction that allows for these optional inputs. Please have a look at that.
- The secnonce is made invalid after being used in partial_sign.
- Adaptor signatures basically work as before, according to https://github.com/ElementsProject/scriptless-scripts/pull/24 (with the exception that they operate on aggregate instead of partial sigs)
- To avoid making this PR overly complex I did not consider how this implementation interacts with nested-MuSig, sign-to-contract, and antiklepto.
- Testing should be close to complete. There's no reachable line or branch that isn't exercised by the tests.
- [x] ~In the current implementation when a signer sends an invalid nonce (i.e. some garbage that can't be mapped to a group element), it is ignored when combining nonces. Only after receiving the signers partial signature and running `partial_sig_verify` will we notice that the signer misbehaved. The reason for this is that 1) this makes the API simpler and 2) malicious peers don't gain any additional powers because they can always interrupt the protocol by refusing to sign. However, this is up for discussion.~ EDIT: this is not the case anymore since invalid nonces are rejected when they're parsed.
- [x] ~For every partial signature we verify we have to parse the pubnonce (two compressed points), despite having parsed it in `process_nonces` already. This is not great. `process_nonces` could optionally output the array of parsed pubnonces.~ EDIT: fixed by having a dedicated type for nonces.
- [x] ~I left `src/modules/musig/musig.md` unchanged for now. Perhaps we should merge it with the `musig-spec`~ EDIT: musig.md is updated
- [x] partial verification should use multiexp to compute `R1 + b*R2 + c*P`, but this can be done in a separate PR
- [x] renaming wishlist
- pre_session -> keyagg_cache (because there is no session anymore)
- pubkey_combine, nonce_combine, partial_sig_combine -> pubkey_agg, nonce_agg, partial_sig_agg (shorter, matches terminology in musig2)
- musig_session_init -> musig_start (shorter, simpler) or [musig_generate_nonce](https://github.com/ElementsProject/secp256k1-zkp/pull/131#discussion_r654190890) or musig_prepare
- musig_partial_signature to musig_partial_sig (shorter)
- [x] perhaps remove pubnonces and n_pubnonces argument from process_nonces (and then also add a opaque type for the combined nonce?)
- [x] write the `combined_pubkey` into the `pre_session` struct (as suggested [below](https://github.com/ElementsProject/secp256k1-zkp/pull/131#issuecomment-866904975): then 1) session_init and process_nonces don't need a combined_pk argument (and there can't be mix up between tweaked and untweaked keys) and 2) pubkey_tweak doesn't need an input_pubkey and the output_pubkey can be written directly into the pre_session (reducing frustration such as Replace MuSig(1) module with MuSig2 #131 (comment))
- [x] perhaps allow adapting both partial sigs (`partial_sig` struct) and aggregate partial sigs (64 raw bytes) as suggested [below](https://github.com/ElementsProject/secp256k1-zkp/pull/131#issuecomment-867281531).
Based on #120.
ACKs for top commit:
robot-dreams:
ACK ac1e36769dda3964f7294319ecb06fb5c414938d
real-or-random:
ACK ac1e36769dda3964f7294319ecb06fb5c414938d
Tree-SHA512: 916b42811aa5c00649cfb923d2002422c338106a6936a01253ba693015a242f21f7f7b4cce60d5ab5764a129926c6fd6676977c69c9e6e0aedc51b308ac6578d
b9ebee1490cc10286780c824a2bfac6bbb961cee fix a couple things to make Elements 22's linter happy (Andrew Poelstra)
Pull request description:
In Elements 22 the linter looks for executable files that don't have a properly-formed shebang. For some reason it wants `/usr/bin/env bash` rather than `/bin/bash`, and also one of our source files was erroneously 755.
ACKs for top commit:
real-or-random:
ACK b9ebee1490cc10286780c824a2bfac6bbb961cee
Tree-SHA512: 00da8fefd67c1882c6cec39dc81ce67ae3f52f902ddf72545e902b8f5bc7cd7c1249bf71027c530245c403a99c86ffbb61a89bc18c27c5ec975f6f653200766c
72713872a8597884918bcf1edbc12f5c969ca680 Add missing static to secp256k1_schnorrsig_sign_internal (Elichai Turkel)
adec5a16383f1704d80d7c767b2a65d9221cee08 Add missing null check for ctx and input keys in the public API (Elichai Turkel)
f4edfc758142d6e100ca5d086126bf532b8a7020 Improve consistency for NULL arguments in the public interface (Elichai Turkel)
20abd52c2e107e79391a19d2d2f8845e83858dea Add tests for pre_g tables. (Russell O'Connor)
6815761cf5500f1a619965c5b4bbc8918b334a35 Remove ecmult_context. (Russell O'Connor)
f20dcbbad1b88b5635ce096257c40849b1d02f32 Correct typo. (Russell O'Connor)
16a3cc07e8450bc3b68b19240f1c729e677a01c9 Generate ecmult_static_pre_g.h (Russell O'Connor)
8de2d86a06f014b650cd81d89a370d2326c4ed71 Bump memory limits in advance of making the ecmult context static. (Russell O'Connor)
5d5c74a057f3951677691113747952f4cbdde86b tests: Rewrite code to circument potential bug in clang (Tim Ruffing)
3d2f492ceb76eea93d3a9f85f80baec7b5842160 ci: Install libasan6 (instead of 5) after Debian upgrade (Tim Ruffing)
Pull request description:
[bitcoin-core/secp256k1#969]: ci: Fixes after Debian release
[bitcoin-core/secp256k1#956]: Replace ecmult_context with a generated static array.
[bitcoin-core/secp256k1#783]: Make the public API docs more consistent and explicit
[bitcoin-core/secp256k1#976]: `secp256k1_schnorrsig_sign_internal` should be static
This PR can be recreated with `./sync-upstream.sh range 2a3a97c665475bc00d5d60f2f04830202983a631`.
ACKs for top commit:
real-or-random:
ACK 938725c1c91c73cfd76d2f830227287b9eaee300 inspected the diff between the pure output of running the sync script and this PR
Tree-SHA512: 6dd5964563497ced6afe533e4deaa82df76c071b5146a9eb7a5a998187210b5fbf19195d34320b7b2193f6b40d778cf258ad22033d7bc33479e0dc4791aceff9
72713872a8597884918bcf1edbc12f5c969ca680 Add missing static to secp256k1_schnorrsig_sign_internal (Elichai Turkel)
Pull request description:
This function isn't used outside of this module so it should be declared static
ACKs for top commit:
real-or-random:
ACK 72713872a8597884918bcf1edbc12f5c969ca680
jonasnick:
ACK 72713872a8597884918bcf1edbc12f5c969ca680
Tree-SHA512: 6107a2c84c3e11ffd68de22a5288d989a3c71c2ec1ee4827c88f6165fc27ef8339d0f6740928540e8ccd03aff49a2a96149bf698ccebe6d6d8ad6e23e38e8838
adec5a16383f1704d80d7c767b2a65d9221cee08 Add missing null check for ctx and input keys in the public API (Elichai Turkel)
f4edfc758142d6e100ca5d086126bf532b8a7020 Improve consistency for NULL arguments in the public interface (Elichai Turkel)
Pull request description:
I went over the public API and added missing explanations on when a pointer can be null and when it cannot,
and added some missing checks for null ctx and null pubkey pointers.
Open questions IMHO:
1. Can `secp256k1_context_create` return NULL? right now it could return null if you replaced the callbacks at compile time to ones that do return(unlike the default ones which never return).
2. Related to the first, should we document that the callbacks should never return? (in the tests we use returning callbacks but we can violate our own API) right now we say the following:
> After this callback returns, anything may happen, including crashing.
Is this enough to document answer `no` for the first question and just saying that if the callback returned then you violated the API so `secp256k1_context_create` can return NULL even though it is promised not to?
Right now we AFAICT we never check if it returns null
Another nit I'm not sure about is wording `(does nothing if NULL)`/`(ignored if NULL)`/`(can be NULL)`
More missing docs:
1. Documenting the `data` argument to the default nonce functions
ACKs for top commit:
ariard:
ACK adec5a16
jonasnick:
ACK adec5a16383f1704d80d7c767b2a65d9221cee08
Tree-SHA512: 6fe785776b7e451e9e8cae944987f927b1eb2e2d404dfcb1b0ceb0a30bda4ce16469708920269417e5ada09739723a430e270dea1868fe7d12ccd5699dde5976
This header contains a static array that replaces the ecmult_context pre_g and pre_g_128 tables.
The gen_ecmult_static_pre_g program generates this header file.
clang 7 to 11 (and maybe earlier versions) warn about recid being
potentially unitiliazed in "CHECK(recid >= 0 [...]", which was mitigated
in commit 3d2cf6c5bd35b0d72716b47bdd7e3892388aafc4 by initializing recid
to make clang happy but VG_UNDEF'ing the variable after initializiation
in order to ensure valgrind's memcheck analysis will still be sound and
complain if recid is not actually written to when creating a signature.
However, it turns out that at least for binaries produced by clang 11
(but not clang 7), valgrind complains about a branch on unitialized data
in the recid variable in that line before *and* after the aforementioned
commit. While the complaint after the commit could be spurious (clang
knows that recid is initialized, so it's fine to access it even though
the access is stupid), the complaint before the commit indicates a real
problem: it might be the case that clang is performing a wrong
optimization that leads to a situation where recid is really not
guaranteed to be initialized when it's accessed. As a result, clang
warns about this and generates code that just accesses the variable.
I'm not going to bother with this further because this is fixed in
clang 12 and the problem is just in our test code, not in the tested
code.
This commit rewrites the code in a way that groups the signing together
with the CHECK such that it's very easy to figure out for clang that
recid will be initialized properly. This seems to circument the issue.
8f093be374da794b835302bfb81a72e2bdd51d26 musig: use tagged hash for the list of pubkeys to aggregate (Jonas Nick)
a6a768a4bf3a243609e508c492307cb0fe754bda musig: make key agg test vector more precise (Jonas Nick)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 5369dc5b4039dd4cda2c50282db2882c088b96e1daa5801240f92be1832ed8f29317fdbfc3cab211707155c284a68dc593967f3141703e2544f6b8dc1553e44d
aeece4459977b69962bcd1e1ee8845c18c74ff8f gen_context: Don't use any ASM (Tim Ruffing)
Pull request description:
See https://github.com/bitcoin/bitcoin/issues/22441 , we need to wait for the testing results there.
ACKs for top commit:
sipa:
utACK aeece4459977b69962bcd1e1ee8845c18c74ff8f
jonasnick:
ACK aeece4459977b69962bcd1e1ee8845c18c74ff8f
Tree-SHA512: 52ff90f3dedda90124140de1c2c1c065a2f9374930d6b988d35c37f5eeae97f7d557b7ab0cf99d22add5a76ff8a3e06226572e43949e12d1048cb323d1b3d92b
