achow101
commented at 10:32 pm on March 18, 2024:
member
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.
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.
DrahtBot
commented at 10:32 pm on March 18, 2024:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
DrahtBot added the label
Wallet
on Mar 18, 2024
DrahtBot added the label
CI failed
on Mar 19, 2024
DrahtBot
commented at 2:45 am on March 19, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
in
test/functional/wallet_musig.py:149
in
a1e4c323dboutdated
144+ for deriv_path in dec_psbt["inputs"][0]["taproot_bip32_derivs"]:
145+ if deriv_path["pubkey"] in part_pks:
146+ part_pks.remove(deriv_path["pubkey"])
147+ assert_equal(len(part_pks), 0)
148+
149+ nonce_psbts = []
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)
in
test/functional/wallet_musig.py:167
in
a1e4c323dboutdated
161+ for wallet in wallets:
162+ proc = wallet.walletprocesspsbt(comb_nonce_psbt)
163+ assert_equal(proc["complete"], False)
164+ psig_psbts.append(proc["psbt"])
165+
166+ comb_psig_psbt = self.nodes[0].combinepsbt(psig_psbts)
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.
Sjors
commented at 2:22 pm on March 19, 2024:
member
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.
0% test/functional/wallet_musig.py
12024-03-19T14:23:33.113000Z TestFramework (INFO): PRNG seed is: 647071992440405417422024-03-19T14:23:33.115000Z TestFramework (INFO): Initializing test directory /var/folders/h6/qrb4j9vn6530kp7j4ymj934h0000gn/T/bitcoin_func_test_66knao3l
32024-03-19T14:23:35.070000Z TestFramework (INFO): Testing rawtr(musig(keys/*))
42024-03-19T14:23:35.192000Z TestFramework (ERROR): Unexpected exception caught during testing
(didn’t check if it’s the same crash)
bigspider
commented at 2:36 pm on March 19, 2024:
none
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).
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.
achow101
commented at 3:52 pm on March 19, 2024:
member
What do you mean by “older”? Just that the PR to libsecp needs another rebase?
I pulled in a commit that is probably outdated at this point. There may have been API changes since.
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).
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.
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)
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.
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.
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 ProduceSignature does not report the tx as being signed until there is actually a signature, so even the partial sigs generation will not return “signed”.
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.
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.
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.
…
Starting with a fresh transaction, sing only the RPC I got the same crash, i.e.:
Perhaps relevant: Bob’s wallet is encrypted, though it was unlocked throughout steps 3 and 4.
0% test/functional/wallet_musig.py
12024-03-19T14:23:33.113000Z TestFramework (INFO): PRNG seed is: 647071992440405417422024-03-19T14:23:33.115000Z TestFramework (INFO): Initializing test directory /var/folders/h6/qrb4j9vn6530kp7j4ymj934h0000gn/T/bitcoin_func_test_66knao3l
32024-03-19T14:23:35.070000Z TestFramework (INFO): Testing rawtr(musig(keys/*))
42024-03-19T14:23:35.192000Z TestFramework (ERROR): Unexpected exception caught during testing
(didn’t check if it’s the same crash)
Huh, works fine for me.
Sjors
commented at 4:37 pm on March 19, 2024:
member
Huh, works fine for me.
This was on Intel macOS 14.4 with a clean checkout and ./configure --disable-bench --disable-tests --enable-wallet --disable-fuzz-binary --disable-zmq --with-gui.
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)
I don’t think we can store the nonces in the wallet file.
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.
Currently it just ignores if there is already a nonce for a participant’s key.
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 processpsbt to fail when called twice normally, here it seems justifiable.
Update: successfully completed the MuSig2 signing on Ubuntu!
achow101 force-pushed
on Mar 19, 2024
achow101 force-pushed
on Mar 20, 2024
achow101 force-pushed
on Mar 25, 2024
achow101 force-pushed
on Mar 25, 2024
achow101 force-pushed
on Mar 25, 2024
achow101 force-pushed
on Mar 26, 2024
DrahtBot added the label
Needs rebase
on Mar 29, 2024
Squashed 'src/secp256k1/' changes from efe85c70a2e..093af5dc48e
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 #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
2189d2f841
Merge commit '2189d2f8412dcd2eac08da2cfc467782ec258142' into musig253887377b7
build: Enable libsecp256k1 MuSig2 module in MSVC9a2ffeb6dc
XOnlyPubKey: Add GetCPubKeys
We need to retrieve the even and odd compressed pubkeys for xonly
pubkeys, so add a function to do that. Also reuse it in GetKeyIDs.
a5dae2b7e6
desc spkm: Return SigningProvider only if we have the privkey
If we know about a pubkey that's in our descriptor, but we don't have
the private key, don't return a SigningProvider for that pubkey.
This is specifically an issue for Taproot outputs that use the H point
as the resulting PSBTs may end up containing irrelevant information
because the H point was detected as a pubkey each unrelated descriptor
knew about.
d9f4264f2b
pubkey: Add functions for MuSig2 function
- GetMuSig2KeyAggCache creates the secp256k1_musig_keyagg_cache from a
std::vector<CPubKey>
- GetCPubKeyFromMuSig2KeyAggCache creates a CPubKey from a cache created
with GetMuSig2KeyAggCache
- MuSig2AggregatePubKeys does the two above functions together.
bb41d42647
spanparsing: Allow Const to not skip the found constant1bfab22490
wallet, rpc: Only allow keypool import from single key descriptors
Instead of relying on implicit assumptions about whether pubkeys show up
or now after expanding a descriptor, just explicitly allow only single
key descriptors to import keys into a legacy wallet's keypool.
6464afc774
descriptors: Have GetPubKey fill origins directly
Instead of having ExpandHelper fill in the origins in the
FlatSigningProvider output, have GetPubKey do it by itself. This reduces
the extra variables needed in order to track and set origins in
ExpandHelper.
Also changes GetPubKey to return a std::optional<CPubKey> rather than
using a bool and output parameters.
3e939d5824
descriptors: Move FlatSigningProvider pubkey filling to GetPubKey
Instead of MakeScripts inconsistently filling the output
FlatSigningProvider with the pubkeys involved, just do it in GetPubKey.
1a1f1fd722
descriptors: Have GetPrivKey fill keys directly
Instead of GetPrivKey returning a key and having the caller fill the
FlatSigningProvider, have GetPrivKey take the FlatSigningProvider and
fill it by itself.
GetPrivKey is now changed to void as the caller no longer cares whether
it succeeds or fails.
6144bebf2e
achow101 force-pushed
on Apr 1, 2024
DrahtBot removed the label
Needs rebase
on Apr 1, 2024
Sjors
commented at 10:36 am on April 2, 2024:
member
Only 3 red CI machines to go :-)
achow101 force-pushed
on Apr 2, 2024
achow101
commented at 5:10 pm on April 2, 2024:
member
sign: Add GetAggregateParticipantPubkeys to SigningProvider8add6c1a9b
tests: Test musig() parsinge94f852533
pubkey: Return tweaks from BIP32 derivation2f29826a65
sign: Include taproot output key's KeyOriginInfo in sigdataff1cd6a130
sign: Refactor Schnorr sighash computation out of CreateSchnorrSig
There will be other functions within MutableTransactionSignatureCreator
that need to compute the same sighash, so make it a separate member
function.
b029a2ad3f
Add MuSig2SecNonce class for secure allocation of musig noncese904c213e4
signingprovider: Add musig2 secnonces
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.
aa09b63167
sign: Add CreateMuSig2AggregateSig104b03e76f
sign: Add CreateMuSig2Noncecd2c130ad5
sign: Add CreateMuSig2PartialSig724f086914
sign: Create MuSig2 signatures for known MuSig2 aggregate keys
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.
af44758228
wallet: Keep secnonces in DescriptorScriptPubKeyMan283811e1f8
psbt: Implement un/ser of musig2 fields94ab405e87
rpc: Include MuSig2 fields in decodepsbtf6b18dccf6
psbt: MuSig2 data in Fill/FromSignatureData45fff8ca5d
psbt: Use specified sighash or SIGHASH_DEFAULT when dummy signing
When dummy signing for finalizing, use either the specificed sighash, or
SIGHASH_DEFAULT, rather than always SIGHASH_ALL.
For outputs, just use SIGHASH_DEFAULT.
84db66d9bc
test: Test MuSig2 in the wallete87f76845b
achow101 force-pushed
on Apr 16, 2024
bitcoin deleted a comment
on May 23, 2024
bigspider
commented at 11:43 am on June 6, 2024:
none
Hi all,
an early alpha of the Ledger Bitcoin Testnet app with MuSig2 support is available for testing. (NB: the app is called Bitcoin Test Musig and not Bitcoin Test). It should be compatible with the latest draft of the specs.
Instructions and an easy end-2-end script for MuSig signing to play with it is available here for anyone interested in trying it out:
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
