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.
a1e4c323dbff9fa5095cf216d7cd528f10a1feeb: 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.
a1e4c323dbff9fa5095cf216d7cd528f10a1feeb: 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.
a1e4c323dbff9fa5095cf216d7cd528f10a1feeb: 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
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 not be created, add our own public nonces for the private keys that we know, if they do not yet exist.
There was a problem hiding this comment.
receive and spend from imported musig(0 descriptors.
Is this a typo in the PR description?
This was a big PR that required me to split the review into multiple parts in which I tried to think of scenarios that could cause the flow to break or to be unsafe.
Few points from my understanding of this PR:
-
Appropriate steps are taken to secure the
MuSig2SecNonce.- It is stored only in memory - never backed up on disk, or serialised. Okay with the tradeoff of having to restart the MuSig2 signing session when the node is restarted.
- Uses
secure_unique_ptrthatmemory_cleanses while destructing the object. The same one that's used forCKeyin the wallet, and the same one that's recommended in thelibsecpexample - helps in having the compiler not optimising away thememsetthat clears theSecNoncebefore destruction. - Also used via
secure_unique_ptrisLockedPoolManagerthat ensures non-swappable memory. - The
secp256k1_contextis destroyed in the MuSig2 example inlibsecp; in the wallet though, it appears it is created (along with being randomised) and is destroyed on node start & shutdown.
-
There is some redundancy in
sighashcalculation wherein it's calculated multiple times in the same flow; it can be addressed in a follow-up: #29675 (comment) -
Inclining to agree that the MuSig2 functions can be moved from CKey to
musig.cpplater where it seems more suitable; in only 2 placessecp256k1_context_signis used: #29675 (comment) -
Thorough functional testing gives more confidence because it covers various cases such as:
a. both single & multiplemusigportions in the descriptor,
b. bothrawtrandtrdescriptors,
c. different sighash types,
d. same descriptor spending through key and script path in case of missing signers,
e. only 1 wallet with the Musig descriptor with rest being just plain signers having individual keys.
Thanks for addressing all the comments previously in the partial reviews.
Range diff from last review:
git range-diff 36f8355...ac599c4I might take another look after few days if I feel I missed something.
lgtm tACK ac599c4
| extpub.nDepth = 0; | ||
| std::memset(extpub.vchFingerprint, 0, 4); | ||
| extpub.nChild = 0; | ||
| extpub.chaincode = MUSIG_CHAINCODE; |
There was a problem hiding this comment.
nit: could move the MUSIG_CHAINCODE constant from the header to musig.cpp, as its only used there now
| if wallet_index < len(wallets): | ||
| continue | ||
| wallet_name = f"musig_{self.WALLET_NUM}" | ||
| self.WALLET_NUM += 1 |
There was a problem hiding this comment.
nit: seems that this shouldn't be in upper-case, if it's not really a constant
There was a problem hiding this comment.
Looks very good to me, I am just curious about the two questions I have from my last pass, but I am happy to ACK once these are addressed (with code or comment).
FWIW, I didn't like that the functional test only checked the happy path, so I drafted some tests for failure scenarios here: fjahr@889af13 I will open this as a follow-up after merge.
|
|
||
| bool MutableTransactionSignatureCreator::CreateSchnorrSig(const SigningProvider& provider, std::vector<unsigned char>& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const | ||
| { | ||
| assert(sigversion == SigVersion::TAPROOT || sigversion == SigVersion::TAPSCRIPT); |
There was a problem hiding this comment.
nit: Not sure if assert is needed in both places now, seems like keeping it in ComputeSchnorrSignatureHash might be enough.
| void FlatSigningProvider::SetMuSig2SecNonce(const uint256& session_id, MuSig2SecNonce&& nonce) const | ||
| { | ||
| if (!Assume(musig2_secnonces)) return; | ||
| musig2_secnonces->emplace(session_id, std::move(nonce)); |
There was a problem hiding this comment.
Should this first check that no value exists already for this key? Otherwise, if the key wasn't deleted properly and session id is reused somehow, there will be no effect and this might lead to danger reusing the nonce? It's a bit far fetched but might be good as belt and suspenders.
There was a problem hiding this comment.
I think it's fine as is but can change if I need to retouch.
| sigdata.musig2_partial_sigs[pub_key_leaf_hash].emplace(part_pk, partial_sig); | ||
| } | ||
| } | ||
| // If there are any partial signatures, exit early |
There was a problem hiding this comment.
nit: I wouldn't call a continue exit early, I would expect to exit the whole function here then.
| // If there are any partial signatures, exit early | ||
| auto partial_sigs_it = sigdata.musig2_partial_sigs.find(pub_key_leaf_hash); | ||
| if (partial_sigs_it != sigdata.musig2_partial_sigs.end() && !partial_sigs_it->second.empty()) { | ||
| continue; |
There was a problem hiding this comment.
Doesn't this mean that we can never recover from a situation where we have some partial sig but not all pubnonces? I guess this situation is prevented by the calling code but still, I would have expected here to rather check if any new partial sigs were added in the code above just now because that would imply the necessary pubnonce for that particular partial sig was available.
There was a problem hiding this comment.
Having some partial sigs and not all pubnonces should be a contradiction. It is not possible to create a valid partial sig without all of the pubnonces. I think in that situation it is safer to do nothing rather than try to continue by adding a new pubnonce.
|
tACK ac599c4 I might just address some of the left-over comments in a follow-up together with my additional tests, if this PR doesn't get retouched anymore. |
|
I have opened a PR with my tests and the left-over nits here: #33636 |
|
Opened a PR for removal of redundant sighash calculation here: #33665. |
| // Check the derivation, and compute the BIP 32 derivation tweaks | ||
| std::vector<std::pair<uint256, bool>> tweaks; | ||
| CPubKey plain_pub = agg_pub; | ||
| if (XOnlyPubKey(agg_pub) != script_pubkey) { |
There was a problem hiding this comment.
looks like this line crashes:
$ echo 'cHNidP8BAP0+AQIAAAAGy4dxGNHKAQgFPtEAoVgyMqb+N0ghgmdne5OjULYs2sUAAAr/AP3////L
fnEY0crZmwU+EVahszExpv43SCGCZ2d7k6NQtizaOwADAC4ABHwdAMt+cRjRytmbBT4RVqFYMTGm
/jdIIYJnZ3uTo1AAOwAsAAC2KwD9////hqbXlTl39C5ZmAtOAI4+zy32vWLVnyQjw7IvQsBa6rYA
AAcAAP3//zKG/zqgb7JIwmli68xeUjUXVp+c4b/tvc55IsAI4yQAAAAAAAAA/f///8t/0cpxGNmb
BT4RVqFYMTOm/jdIIXj///8AAAAAgNo7AACAAAD9//3/AiHf9QUAAAAAFgAU9/Ykq9yiCFGnUpsi
RsS6FFGLkQEAAAAAAAABABYAFEEQliEApQAADCBABv78gsn+/////wAAAAAAAAAAACIagHQAAAAV
DLQyJycoAHNiKDIDAEIAAEEA/wEABgDI/jdIIYJnZ3uTo1AAOwAsAAC2KwD9////hqbXAY9zeZ10
c/zcAAEBK2ECEWUrKwAAIlEgcPx0K2ICYgQkKwABAQEBAQsCAgAERUIADwADAAADmSsAAAA=' | base64 --decode > /tmp/crash.bin
$ FUZZ=psbt ./bld-cmake/bin/fuzz -runs=1 /tmp/crash.bin
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 494792800
INFO: Loaded 1 modules (385403 inline 8-bit counters): 385403 [0x55c06b279420, 0x55c06b2d759b),
INFO: Loaded 1 PC tables (385403 PCs): 385403 [0x55c06b2d75a0,0x55c06b8b8d50),
./bld-cmake/bin/fuzz: Running 1 inputs 1 time(s) each.
Running: /tmp/crash.bin
/usr/bin/../include/c++/v1/span:512: assertion __offset <= size() failed: span<T>::subspan(offset, count): offset out of range
(needs a hardened libc++, msan, or valgrind)
| if (sigversion == SigVersion::TAPROOT && merkle_root) { | ||
| tweaks.emplace_back(script_pubkey.ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root), true); | ||
| std::optional<std::pair<XOnlyPubKey, bool>> tweaked = script_pubkey.CreateTapTweak(merkle_root->IsNull() ? nullptr : merkle_root); | ||
| if (!Assume(tweaked)) return false; |
There was a problem hiding this comment.
$ FUZZ=rpc ./bld-cmake/bin/fuzz /tmp/clusterfuzz-testcase-minimized-rpc-4681957738086400
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2426494106
INFO: Loaded 1 modules (402716 inline 8-bit counters): 402716 [0x561fca7217f8, 0x561fca783d14),
INFO: Loaded 1 PC tables (402716 PCs): 402716 [0x561fca783d18,0x561fcada8ed8),
./bld-cmake/bin/fuzz: Running 1 inputs 1 time(s) each.
Running: /tmp/clusterfuzz-testcase-minimized-rpc-4681957738086400
script/sign.cpp:321 bool SignMuSig2(const BaseSignatureCreator &, SignatureData &, const SigningProvider &, std::vector<unsigned char> &, const XOnlyPubKey &, const uint256 *, const uint256 *, SigVersion): Assertion `tweaked' failed.
14 participants
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.