Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MuSig2 support #294

Merged
merged 57 commits into from
Dec 10, 2024
Merged

MuSig2 support #294

merged 57 commits into from
Dec 10, 2024

Conversation

bigspider
Copy link
Collaborator

@bigspider bigspider commented Oct 9, 2024

Add full support for musig() in wallet policies.

Unlike typical signatures, where the client sends a transaction to the device and receives a signed transaction back, MuSig2 is a 2-party protocol among a group of cosigners, where:

  • during the first round, all the cosigners generate a secret nonce, and share with each other a pubnonce
  • Once all the pubnonces are available, the cosigners can proceed to aggregate the pubnonces, producing the aggregate aggnonce, and produce a musig partial signature

Any (untrusted) coordinator, once all the partial signatures are collected, can produce the final signature.

Security considerations

Special care needs to be taken in the handling of nonces, and the way nonces are persisted in storage.
See more in the musig.md document added as part of the PR.

UX consideration

As it is a two round protocol, there were some choices to be made on how to handle it: software wallets will in fact send the same PSBT to the device twice in order to complete the protocol.

In order to minimize the UX impact for the device - but also for wallets implementing it, the following choice was made:

  • the first round is executed silently - that is without requiring any user confirmation

This is not a security problem, because the first round of MuSig2 just produces and stores a random number; private keys are not even accessed.

In cases when the other cosigners are online at the same time, this allows software wallets to use MuSig2 while essentially providing identical UX to a non-MuSig2 wallet (connect the device, , inspect and confirm the transaction).

If the first and second round happen in two separate sessions, only the second one will require user confirmation, while of course the first one still needs the device to be unlocked and connected with the bitcoin app open.

BIPs related to this PR

The details of the signing protocol are detailed in BIP-327, but there are other related BIPs: BIP-328, BIP-390, BIP-373.
BIP-388 is in process of being updated.

Structure of the implementation

The PR required some refactoring that have a fairly big impact on the codebase, particularly in order to generalize the 'key expressions' in wallet policies to support the musig() expression, whereas in the past it was always limited to a single xpub; the code to handle the PSBT fields in the transaction and identify the correct derivation path for each input also needed to be generalized.
However, most of the musig-related code is relatively self-contained and modularized in the following files:

  • doc/musig.md - Technical details, and security details, about the MuSig2 feature.
  • src/handler/sign_psbt/musig_signing.{c,j} - portions of the sign_psbt handler that are handling the logic of Round 1 and Round 2 of MuSig2,
  • src/musig/musig.{c,h} - implementation of the relevant algorithms defined in BIP-327, closely matching the Python reference implementation
  • src/musig/musig_sessions.{c,h} - encapsulates the logic to handle the persistence in NVRAM after the end of MuSig's round 1, until the beginning of Round 2.

Status of test coverage

There are some e2e tests with the corresponding bitcoin-core implementation which is still a WIP and does not support complex policies. Testing will be needed with musig nested inside miniscript as soon as possible - to be done in a separate PR ahead of release. Created issue #302 for tracking.

Incidental changes

Also improves the signing flow to only sign for spending paths that are filled in in the PSBT (that is, the corresponding PSBT_IN_BIP32_DERIVATION or PSBT_IN_TAP_BIP32_DERIVATION fields are present), optimizing certain use cases for wallet policies with multiple alternative spending paths.

Closes: #208
Closes: #177

src/handler/sign_psbt.c Fixed Show fixed Hide fixed
@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 85.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 84.70%. Comparing base (bc81ed0) to head (2d22ebe).

Files with missing lines Patch % Lines
src/common/wallet.c 84.72% 11 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #294      +/-   ##
===========================================
- Coverage    84.76%   84.70%   -0.06%     
===========================================
  Files           17       17              
  Lines         2186     2230      +44     
===========================================
+ Hits          1853     1889      +36     
- Misses         333      341       +8     
Flag Coverage Δ
unittests 84.70% <85.33%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bigspider bigspider changed the title Musig2 MuSig2 support Oct 9, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
6.0% Coverage on New Code (required ≥ 80%)
11.1% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

…ed some comments.

Generalizing to key expressions containing musig() makes it necessary to distinguish
the key expressions in the wallet policy from the actual key placeholders that are
just indexes to the list of key informations (@num in the descriptor template),
whereas the two concepts were often not clearly separated in the code base.

Renaming to "key expressions" makes the distinction more clear.
…on type is used; generalized some parts of the code that are not generalized to musig key expressions, and annotated some others.
@bigspider bigspider force-pushed the musig2 branch 3 times, most recently from 256f790 to 4616d67 Compare November 6, 2024 09:26
@bigspider bigspider force-pushed the musig2 branch 7 times, most recently from aae5a24 to 261d7d6 Compare November 7, 2024 11:55
This is redundant with the current implementation. However, the
musigsession module is written in such a way that the calling code
has no knowledge about its internal working. Therefore, it should
not assume that zeroing out is the correct way of initializing it.
@bigspider bigspider force-pushed the musig2 branch 3 times, most recently from b23a9ad to daa79a3 Compare November 29, 2024 16:57
@bigspider bigspider marked this pull request as ready for review December 2, 2024 16:06
Copy link
Contributor

@lpascal-ledger lpascal-ledger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the C part mostly, need to dive more into the Python bits.

bitcoin_client/ledger_bitcoin/bip0327.py Show resolved Hide resolved
bitcoin_client/ledger_bitcoin/wallet.py Show resolved Hide resolved
doc/bitcoin.md Outdated Show resolved Hide resolved
doc/musig.md Outdated Show resolved Hide resolved
doc/musig.md Outdated Show resolved Hide resolved
src/common/wallet.c Outdated Show resolved Hide resolved
lpascal-ledger
lpascal-ledger previously approved these changes Dec 10, 2024
Copy link
Contributor

@lpascal-ledger lpascal-ledger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing interesting to say, that all sounds fair 👍

doc/musig.md Show resolved Hide resolved
test_utils/musig2.py Outdated Show resolved Hide resolved
test_utils/musig2.py Outdated Show resolved Hide resolved
test_utils/musig2.py Show resolved Hide resolved
test_utils/musig2.py Outdated Show resolved Hide resolved
test_utils/musig2.py Outdated Show resolved Hide resolved
test_utils/musig2.py Outdated Show resolved Hide resolved
test_utils/musig2.py Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
@bigspider bigspider merged commit 24bcdae into develop Dec 10, 2024
155 checks passed
@bigspider bigspider deleted the musig2 branch December 10, 2024 13:49
@bigspider bigspider restored the musig2 branch December 10, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MuSig2 support Allow the client to only sign for a subset of key placeholders
3 participants