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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_5_m |
|
🚧 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 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.
8e32747 to
ae645ef
Compare
There was a problem hiding this comment.
Reviewed up to 38d7413, left a few non-blocking suggestions below.
I think the order of commits d65c8df ... 38d7413 is currently slightly confusing for reviewers, as it doesn't reflect the protocol flow, i.e. the signature aggregation function CreateMuSig2AggregateSig should ideally be introduced after the partial signature creation function CreateMuSig2PartialSig.
|
Concept ACK ae645ef, started reviewing. Maybe the PR description could be updated to remove the following references as their detailed implementations were done in prior PRs.
|
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.
|
Meanwhile I managed to test a script path spend with Ledger as well (though I haven't tried MuSig2 inside a script path), using bitcoin-core/HWI#794. With that I'm pretty happy with interoperability. Will do another code review round soon(tm). |
This PR implements MuSig2 signing so that the wallet can receive and spend from imported
musig(0descriptors.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.