wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
There was a problem hiding this comment.
Very cool stuff! Will review more later.
This pulls in (an older version of) the musig module in libsecp
What do you mean by "older"? Just that the PR to libsecp needs another rebase?
An open question is whether the approach for handling the secnonces is ideal and safe. Since nonces must not be reused, this PR holds them exclusively in memory, so a restart of the software will require a restart of the MuSig2 signing process.
It sounds safe, but not ideal, which might make it unsafe. Every Bitcoin Core instance involved would need to keep running, with the wallet loaded (and decrypted?) throughout the two rounds. For an airgapped setup with keys in multiple locations, the node in each location would have to be left running unattended (assuming one person running between them).
My understanding is that Ledger (cc @bigspider) creates a nonce, stores it, and then deletes it from storage as soon as it's loaded (before signing). We could similarly store the nonce in our wallet and then delete the field at the start of the new round. For safety we could disable backups and dump RPC's while a round is in progress (e.g. with a NO_BACKUP flag).
That only prevents accidental replay, not a replay attack, but it seems that anyone who is able to replay a node, already has access to its private keys (from the time a wallet was decrypted), so can't do additional harm?
Implementation questions.
I tried making a 2 party tr(musig(A,B)) in a blank wallet. Initially I obtained two private keys and their public keys from another legacy wallet. I gave the new Alice wallet her private key and Bob's public key, i.e. tr(musig(a,B)/0/*) but this failed with Ranged musig() requires all participants to be xpubs. Why though? Given that bip-musig2-derivation defines a virtual root xpub, and providers a fake chaincode, this restriction seems unneeded? (though it's not blocker either, with descriptor wallets it's easy to get an xpub - after #29130 anyway)
Once I had two wallets, I could see they generated the same receive address, nice! I then imported the same xpub/xpriv pair for the change address 1/*. I sent some (signet) coins to it, which arrived and confirmed.
Sadly after the GUI rugged me :-) Trying to send any amount elsewhere resulted in "Signing transaction failed" followed by "Transaction creation failed!". Whereas I was hoping to get a PSBT this way.
Using the send RPC I do get a PSBT (from Alice). I had the musig2_participant_pubkeys set, but no musig2_pubnonces. That required calling walletprocesspsbt which seems an unnecessary extra step (but such fine tuning can wait). On Bob's side the GUI complained with "Could not sign any more inputs", but it did add a nonce.
At this point all the nonces were commited, so Bob could have added his partial signature. But at the stage the GUI crashes when trying to sign: [libsecp256k1] illegal argument: secp256k1_memcmp_var(&nonce->data[0], secp256k1_musig_pubnonce_magic, 4) == 0.
After a restart Bob's walletprocesspsbt command didn't fail. Which seems wrong: at this point the nonce should be gone, which he should complain about.
Starting with a fresh transaction, sing only the RPC I got the same crash, i.e.:
- Alice:
send - Alice:
processpsbt - Bob:
processpsbt - Bob:
processpsbt: crash
Perhaps relevant: Bob's wallet is encrypted, though it was unlocked throughout steps 3 and 4.
% test/functional/wallet_musig.py
2024-03-19T14:23:33.113000Z TestFramework (INFO): PRNG seed is: 6470719924404054174
2024-03-19T14:23:33.115000Z TestFramework (INFO): Initializing test directory /var/folders/h6/qrb4j9vn6530kp7j4ymj934h0000gn/T/bitcoin_func_test_66knao3l
2024-03-19T14:23:35.070000Z TestFramework (INFO): Testing rawtr(musig(keys/*))
2024-03-19T14:23:35.192000Z TestFramework (ERROR): Unexpected exception caught during testing
(didn't check if it's the same crash)
| part_pks.remove(deriv_path["pubkey"]) | ||
| assert_equal(len(part_pks), 0) | ||
|
|
||
| nonce_psbts = [] |
There was a problem hiding this comment.
a1e4c32: I assume this where the first nonce collection round starts, maybe say so in a comment?
| dec_psbt = self.nodes[0].decodepsbt(comb_nonce_psbt) | ||
| assert_equal(len(dec_psbt["inputs"][0]["musig2_pubnonces"]), exp_key_leaf) | ||
|
|
||
| psig_psbts = [] |
There was a problem hiding this comment.
a1e4c32: and that this is where round 2 happens (maybe link to the BIP at the top of the test and briefly summarise the steps)
| assert_equal(proc["complete"], False) | ||
| psig_psbts.append(proc["psbt"]) | ||
|
|
||
| comb_psig_psbt = self.nodes[0].combinepsbt(psig_psbts) |
There was a problem hiding this comment.
a1e4c32: because all wallets live on the same node, it's useful to point out here that anyone, including non-participants can combine the partial signatures. Which is why the non-wallet combinepsbt and finalizepsbt RPC's are used.
Not yet implemented, but that's the plan: store nonces in flash memory (persistent memory) after generation; remove them from flash memory before signing starts (therefore, they're gone even if there is a later failure, and signing must restart from nonce generation). Note that there is no backup possibility for the persistent memory. |
I pulled in a commit that is probably outdated at this point. There may have been API changes since.
Disabling backups with a flag would not help as an oft suggested method for backing up a wallet is by copying the wallet file. There's nothing that we can do about that, so to be safe, I don't think we can store the nonces in the wallet file.
It's specified in bip-musig2-descriptors that the musig must only contain xpubs if the aggregate will be derived from. I believe the rationale for this is that xpubs are intended to have derivation done on them whereas normal keys are not, and so there may be particular handling of such keys to deal with possibilities of derivation doing something unexpected, and so if we do anything with derivation, we should only use keys that are intended for derivation to avoid any confusion. I think @sipa was the one who made this suggestion.
The GUI may be expecting that at least one signature is produced, but we can't do that with musig without at least one round with the cosigners. I have it implemented such that
Currently it just ignores if there is already a nonce for a participant's key. It doesn't replace the nonce, but it also doesn't validate whether that key belongs to the wallet or whether the nonce exists in the wallet.
Huh, works fine for me. |
This was on Intel macOS 14.4 with a clean checkout and On Ubuntu 23.10 with gcc 13.2.0 the test do pass, odd. (if this still happens after CI passes, I'll dig a bit deeper, for now I'll just test on Ubuntu)
Storing them in some other file might be fine too. As long as we delete it upon read, don't sign anything if deletion fails and maybe also commit to some unique property of the PSBT.
I guess we need to distinguish here between a nonce for our own key and one for other participants. We have no idea if some other node crashed. But it does seem reasonable to fail if we see a nonce for ourselves. Whether we previously crashed or if someone is trying a replay attack doesn't really matter. Though it's unusual for Update: successfully completed the MuSig2 signing on Ubuntu! |
achow101
force-pushed
the
musig2
branch
5 times, most recently
from
817c462
to
7d43eac
Compare
093af5dc48e Merge 3b6c90a93fb55b2ea5fc629762bf4c3c569211cf into 05bfab69aef3622f77f754cfb01220108a109c91 05bfab69aef Merge bitcoin-core/secp256k1#1507: ci: Add workaround for ASLR bug in sanitizers a5e8ab24844 ci: Add sanitizer env variables to debug output 84a93de4d2b ci: Add workaround for ASLR bug in sanitizers 3b6c90a93fb fixup! improve nonce_gen doc 2512e4b9431 fixup! add musig module to change log 35dd4304c20 fixup! Add pubkey_sort to changelog 461970682f5 fixup! split counter mode from musig_nonce_gen dfd9849a193 fixup! rename pubkey_sort -> ec_pubkey_sort d3a8952d5e3 fixup! move extrakeys to main 468c5c41e4a fixup! move musig_ge_{to,from} to group.h 2a0d934540e fixup! use optimized tagged hashes 427e86b9edc Merge bitcoin-core/secp256k1#1490: tests: improve fe_sqr test (issue bitcoin#1472) 2028069df2e doc: clarify input requirements for secp256k1_fe_mul 11420a7a283 tests: improve fe_sqr test cdc9a6258e9 Merge bitcoin-core/secp256k1#1489: tests: add missing fe comparison checks for inverse field test cases d926510cf76 Merge bitcoin-core/secp256k1#1496: msan: notate variable assignments from assembly code 31ba4049442 msan: notate variable assignments from assembly code e7ea32e30a9 msan: Add SECP256K1_CHECKMEM_MSAN_DEFINE which applies to memory sanitizer and not valgrind e7bdddd9c9c refactor: rename `check_fe_equal` -> `fe_equal` 00111c9c563 tests: add missing fe comparison checks for inverse field test cases 0653a25d50f Merge bitcoin-core/secp256k1#1486: ci: Update cache action 94a14d5290e ci: Update cache action 24836272992 Merge bitcoin-core/secp256k1#1483: cmake: Recommend native CMake commands in README 5ad3aa3dcd2 Merge bitcoin-core/secp256k1#1484: tests: Drop redundant _scalar_check_overflow calls dd4932b67b5 build: allow enabling the musig module in cmake 6e104fbdbb0 Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 0c7ca2732d8 group: add ge_to_bytes and ge_from_bytes 26dde295d0a extrakeys: add secp256k1_pubkey_sort 51df2d9ab3a tests: Drop redundant _scalar_check_overflow calls 3777e3f36a6 cmake: Recommend native CMake commands in README e4af41c61b0 Merge bitcoin-core/secp256k1#1249: cmake: Add `SECP256K1_LATE_CFLAGS` configure option 3bf4d68fc00 Merge bitcoin-core/secp256k1#1482: build: Clean up handling of module dependencies e6822678ea0 build: Error if required module explicitly off 89ec583ccf0 build: Clean up handling of module dependencies 44378867a01 Merge bitcoin-core/secp256k1#1468: v0.4.1 release aftermath a9db9f2d75a Merge bitcoin-core/secp256k1#1480: Get rid of untested sizeof(secp256k1_ge_storage) == 64 code path 74b7c3b53e1 Merge bitcoin-core/secp256k1#1476: include: make docs more consistent b37fdb28ce3 check-abi: Minor UI improvements ad5f589a94c check-abi: Default to HEAD for new version 9fb7e2f1568 release process: Style and formatting nits ba5d72d6265 assumptions: Use new STATIC_ASSERT macro e53c2d9ffc0 Require that sizeof(secp256k1_ge_storage) == 64 d0ba2abbff2 util: Add STATIC_ASSERT macro da7bc1b803b include: in doc, remove article in front of "pointer" aa3dd5280b4 include: make doc about ctx more consistent e3f690015a2 include: remove obvious "cannot be NULL" doc d373bf6d08c Merge bitcoin-core/secp256k1#1474: tests: restore scalar_mul test 79e094517c9 Merge bitcoin-core/secp256k1#1473: Fix typos 3dbfb48946b tests: restore scalar_mul test d77170a88d0 Fix typos e7053d065b9 release process: Add email step 429d21dc79e release process: Run sanity checks on release PR 42f8c514022 cmake: Add `SECP256K1_LATE_CFLAGS` configure option git-subtree-dir: src/secp256k1 git-subtree-split: 093af5dc48e57e31b5f451ef0b483d7605e6817c
|
Only 3 red CI machines to go :-) |
Only the tidy job is an actual failure from this PR. MSan is an issue with libsecp that needs to be fixed in bitcoin-core/secp256k1#1479. The ASan failure affects all PRs currently, see #29788 |
|
The test passes for me now on macOS. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
There will be other functions within MutableTransactionSignatureCreator that need to compute the same sighash, so make it a separate member function.
Adds GetMuSig2SecNonces which returns secp256k1_musig_secnonce*, and DeleteMuSig2Session which removes the MuSig2 secnonce from wherever it was retrieved. FlatSigningProvider stores it as a pointer to a map of session id to secnonce so that deletion will actually delete from the object that actually owns the secnonces. The session id is just a unique identifier for the caller to determine what secnonces have been created.
When creating Taproot signatures, if the key being signed for is known to be a MuSig2 aggregate key, do the MuSig2 signing algorithms. First try to create the aggregate signature. This will fail if there are not enough partial signatures or public nonces. If it does fail, try to create a partial signature with all participant keys. This will fail for those keys that we do not have the private keys for, and if there are not enough public nonces. Lastly, if the partial signatures could be created, add our own public nonces for the private keys that we know, if they do not yet exist.
When dummy signing for finalizing, use either the specificed sighash, or SIGHASH_DEFAULT, rather than always SIGHASH_ALL. For outputs, just use SIGHASH_DEFAULT.
This PR implements the proposed MuSig2 descriptors, derivation, and PSBT fields so that the wallet can receive and spend from taproot addresses that have keys involving a MuSig2 aggregate key.
This pulls in (an older version of) the musig module in libsecp (bitcoin-core/secp256k1#1479) in order to do all of the necessary MuSig2 computation.
An open question is whether the approach for handling the secnonces is ideal and safe. Since nonces must not be reused, this PR holds them exclusively in memory, so a restart of the software will require a restart of the MuSig2 signing process. Additionally, the current MuSig API in libsecp basically involves us holding onto and passing around an opaque libsecp object. There may be some additional work there to make the code more ergonomic.