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 Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29675. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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! |
817c462 to
7d43eac
Compare
There was a problem hiding this comment.
Reviewed the non-base commits up to ae68895 sign: Add CreateMuSig2Nonce, but I skipped over 25c4fd4 sign: Add CreateMuSig2AggregateSig.
It's a bit confusing that 25c4fd4 sign: Add CreateMuSig2AggregateSig checks for and uses public nonces, but those are not available until later commits like ae68895 sign: Add CreateMuSig2Nonce.
Note that PR is currently isn't based on the latest #31244 but the tests pass if I do rebase it, so that seems a minor difference.
The only thing I haven't been able to test against a Ledger is script path spending, waiting for the v2.4.1 release. I did test script path spending using just Bitcoin Core keys.
|
|
||
| // Store the secnonce in the SigningProvider | ||
| HashWriter hasher; | ||
| hasher << script_pubkey << part_pubkey << *sighash; |
There was a problem hiding this comment.
In ae68895 sign: Add CreateMuSig2Nonce: it's not clear to me how careful we need to be regarding parallel sessions.
If it's not a big deal then I guess the current choice is fine. Since the same key can be used in multiple tap leaves, we need a unique nonce per leaf, which sighash takes care of. We might have multiple participant keys, which part_pubkey takes care of.
script_pubkey isn't enough to cover address reuse, but for now ComputeSchnorrSignatureHash is limited to SIGHASH_DEFAULT, which commits to the prevout. Maybe for good measure add an Assume for SIGHASH_DEFAULT?
If it is a big deal, then I think libsecp should give us a function to generate it.
There was a problem hiding this comment.
If it's not a big deal then I guess the current choice is fine. Since the same key can be used in multiple tap leaves, we need a unique nonce per leaf, which sighash takes care of. We might have multiple participant keys, which
part_pubkeytakes care of.
script_pubkeyisn't enough to cover address reuse,
Data unique to the input is always included in the sighash, so address reuse is not an issue either.
but for now
ComputeSchnorrSignatureHashis limited toSIGHASH_DEFAULT, which commits to the prevout. Maybe for good measure add anAssumeforSIGHASH_DEFAULT?
Other sighash types are allowed, they're a member of MutableTransactionSignatureCreator.
5fe7915 doc: Add musig() example (Ava Chow) d576079 tests: Test musig() parsing (Ava Chow) a53924b descriptor: Parse musig() key expressions (Ava Chow) 9473e96 descriptors: Move DeriveType parsing into its own function (Ava Chow) 4af0dca descriptor: Add MuSigPubkeyProvider (Ava Chow) d00d954 Add MuSig2 Keyagg Cache helper functions (Ava Chow) 8ecea91 sign: Add GetMuSig2ParticipantPubkeys to SigningProvider (Ava Chow) fac0ee0 build: Enable secp256k1 musig module (Ava Chow) 1894f97 descriptors: Add PubkeyProvider::IsBIP32() (Ava Chow) 12bc1d0 util/string: Allow Split to include the separator (Ava Chow) 8811312 script/parsing: Allow Const to not skip the found constant (Ava Chow) 5fe4c66 XOnlyPubKey: Add GetCPubKeys (Ava Chow) Pull request description: Implements parsing of BIP 390 `musig()` descriptors. Split from #29675 ACKs for top commit: w0xlt: reACK 5fe7915 rkrux: ACK 5fe7915 theStack: re-ACK 5fe7915 🎹 Sjors: ACK 5fe7915 Tree-SHA512: a5be6288e277187fb9a1e2adf4e9822b46b1b8380d732b2fabd53f317c839aecb1971b04410486cbd65047fbc20956675d4d676f56caa37a44ff0e4d12b9b081
Invert any_key_parsed so that the name matches the behavior.
There will be other functions within MutableTransactionSignatureCreator that need to compute the same sighash, so make it a separate member function.
|
All prerequisites have been merged, ready for review. |
| // Aggregate partial sigs | ||
| std::vector<uint8_t> sig; | ||
| sig.resize(64); |
There was a problem hiding this comment.
in 0c42077: nit: since the size is known at compile-time, could alternatively return a std::array (OTOH, at the call-site a std::vector is still needed due to potential adding of the sighash byte)
There was a problem hiding this comment.
Leaving as-is since we need to pass around the signature in a std::vector elsewhere in signing.
| @@ -349,6 +350,39 @@ KeyPair CKey::ComputeKeyPair(const uint256* merkle_root) const | |||
| return KeyPair(*this, merkle_root); | |||
| } | |||
|
|
|||
| std::vector<uint8_t> CKey::CreateMuSig2Nonce(MuSig2SecNonce& secnonce, const uint256& hash, const CPubKey& aggregate_pubkey, const std::vector<CPubKey>& pubkeys) | |||
There was a problem hiding this comment.
module organization/refactoring nit: I think conceptually this functionality would be better fit in the musig.cpp module instead of being a method of the CKey class. Even though a secret key is also passed in for generating the musig nonce here, it merely serves as (optional) additional data to derive the nonce for increasing misuse-resistance, rather than being a central part that would justify an own method. The cleanest approach would be in general to only include and use the secp256k1 musig module in musig.cpp (also for partial signing), IMHO.
Can be dealt in a follow-up though, as the secp256k1_context_sign object would have to be made non-static and shared with other modules (presumably that was the main reason why nonce generation and partial signing was decided to be in key.cpp), which might lead to more general discussions that are probably best kept in a separate PR to not block this one.
There was a problem hiding this comment.
It's mainly here to use secp256k1_context_sign.
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.
A common pattern that MuSig2 functions will use is to aggregate the pubkeys to get the keyagg_cache and then validate the aggregated pubkey against a provided aggregate pubkey. A variant of MuSig2AggregatePubkeys is added which does that. The functionality of GetMuSig2KeyAggCache and GetCPubKeyFromMuSig2KeyAggCache are included in MuSig2AggregatePubkeys (and used internally) so there is no expectation that callers will need these so they are made static.
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.
This PR implements MuSig2 descriptors (BIP 390), derivation (BIP 328), and PSBT fields (BIP 373) so that the wallet can receive and spend from taproot addresses that have keys involving a MuSig2 aggregate key.
The libsecp musig module is enabled so that it can be used for all of the MuSig2 cryptography.
Secnonces are handled in a separate class which holds the libsecp secnonce object in a
secure_unique_ptr. Since secnonces must not be used, this class has no serialization and will only live in memory. A restart of the software will require a restart of the MuSig2 signing process.