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

BIPs for MuSig2 derivation, descriptors, and PSBT fields #1540

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

achow101
Copy link
Member

This PR contains 3 proposed BIPs, the main contents of which were sent to the bitcoin-dev mailing list in October. There have been a few changes, notably the addition of a new BIP for the derivation methodology which was split from the descriptors BIP.

I've also changed the PSBT BIP to use 33 byte plain aggregate pubkeys rather than xonly.

@benma
Copy link

benma commented Jan 17, 2024

I've also changed the PSBT BIP to use 33 byte plain aggregate pubkeys rather than xonly.

It would be helpful to document the reasoning in the commit msg or here in the PR to have a record of design decisions.

@achow101 achow101 force-pushed the musig2 branch 2 times, most recently from 61fe36a to 6b482ea Compare January 17, 2024 18:00
@achow101
Copy link
Member Author

I've also changed the PSBT BIP to use 33 byte plain aggregate pubkeys rather than xonly.

It would be helpful to document the reasoning in the commit msg or here in the PR to have a record of design decisions.

I've added a rationale section which addresses this.

bip-0174.mediawiki Outdated Show resolved Hide resolved
bip-0174.mediawiki Outdated Show resolved Hide resolved
bip-0174.mediawiki Outdated Show resolved Hide resolved
bip-musig2-descriptors.mediawiki Show resolved Hide resolved
However, it is much simpler to be able to derive from a single extended public key instead of having
to derive from many extended public keys and aggregate them. As MuSig2 produces a normal looking
public key, the aggregate public can be used in this way. This reduces the storage and computation
requirements for generating new aggregate pubkeys.
Copy link
Member

Choose a reason for hiding this comment

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

An additional benefit is that such wallets can be watched by a musig2-unaware wallet, or a tapleaf co-signer who does not need to know about the individual keys, by casting the tr(musig()) descriptor to tr(). Similarly it's compatible with the privacy-preserving rawtr descriptor (needs a BIP).

bip-0174.mediawiki Outdated Show resolved Hide resolved
bip-0174.mediawiki Outdated Show resolved Hide resolved
bip-0174.mediawiki Outdated Show resolved Hide resolved
@achow101
Copy link
Member Author

I've made a minor change with the descriptors regarding sorting. Keys in a musig() can be specified in any order, but will be sorted prior to aggregation, similar to sortedmulti(). However for PSBTs, keys must be supplied in the expected order for aggregation (this is unchanged), which means sorting them if they weren't already.

@bigspider
Copy link
Contributor

bigspider commented Apr 17, 2024

I've made a minor change with the descriptors regarding sorting. Keys in a musig() can be specified in any order, but will be sorted prior to aggregation, similar to sortedmulti(). However for PSBTs, keys must be supplied in the expected order for aggregation (this is unchanged), which means sorting them if they weren't already.

What is the rationale for this change? It seems to me that this complicates parsing/processing the descriptor unnecessarily. The only practical advantage I know of sortedmulti versus multi is that with multi an external observer can possibly fingerprint who signed different UTXOs even without knowing the descriptor; but that's not the case anyway for musig.

@achow101
Copy link
Member Author

What is the rationale for this change?

I had a discussion involving @jonasnick and @sipa about this. Since BIP 327 suggests to do sorting, we decided it made sense to do in descriptors as well.

@bigspider
Copy link
Contributor

bigspider commented Apr 18, 2024

I had a discussion involving @jonasnick and @sipa about this. Since BIP 327 suggests to do sorting, we decided it made sense to do in descriptors as well.

BIP 327 discusses the possibility of sorting and describes a canonical sorting, but I didn't read it as 'suggesting' one way or another.

In general, I'd prefer maximum simplicity and less chances of malleability in descriptors, and this imho goes against both (albeit slightly, not a huge deal).

Note that this removes functionality, as one of $n! - 1$ of the $n!$ possible combinations become inexpressible with descriptors.

@bigspider
Copy link
Contributor

The functionality that I would suggest to consider for removal is derivation before aggregation: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-November/022132.html

Unless there are use cases for it that I'm not aware of?

|-
| MuSig2 Public Nonce
| <tt>PSBT_IN_MUSIG2_PUB_NONCE = 0x1a</tt>
| <33 byte compressed pubkey> <32 byte xonlypubkey> <32 byte hash>

Choose a reason for hiding this comment

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

have you thought about putting [] or something around the final hash here to make it clear that it will be omitted if the aggregate key is either the taproot output key or the taproot internal key

script were derived.
| <tt><33 byte compressed pubkey>*</tt>
| A list of the compressed public keys of the participants in the MuSig2 aggregate key in the order
required for aggregation. If sorting was done, then the keys must be in the sorted order.

Choose a reason for hiding this comment

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

the specification for the descriptor says that keys must be sorted with KeySort. If unsorted keys are allowed here, then its possible to have a psbt for a musig aggregate key that's not expressible in a descriptor. Maybe that's ok, just flagging it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is allowed. PSBTs is intentionally less restrictive than descriptors.

Copy link

Choose a reason for hiding this comment

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

What's wrong with taking the ordering from the descriptor directly? That seems simpler and more flexible (Not that I can imagine a specific reason why a user would want a particular order).

Choose a reason for hiding this comment

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

Yes, that is allowed. PSBTs is intentionally less restrictive than descriptors.

makes sense, you want PSBTs to be a super-set of what you can express in a descriptor

Copy link
Member Author

Choose a reason for hiding this comment

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

What's wrong with taking the ordering from the descriptor directly? That seems simpler and more flexible (Not that I can imagine a specific reason why a user would want a particular order).

There's no descriptor in psbts.

Copy link

Choose a reason for hiding this comment

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

Right I was commenting on KeySort-ing the participant keys in the descriptor. Seemed on-topic for this thread but I'll put the comment where it belongs.

expression as a key expression. The aggregate public key is produced by using the <tt>KeyAgg</tt>
algorithm on all KEYs specified in the expression after performing all specified derivation. As with
script expressions, KEY can contain child derivation specified by <tt>/*</tt>. A new aggregate
public key will be computed for each child index. Keys must be sorted with the <tt>KeySort</tt>
Copy link

Choose a reason for hiding this comment

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

Why KeySort when we already have a explicit ordering from the descriptor? It seems like an unnecessary complication.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I mentioned in #1540 (comment), there was a discussion about this and that was the conclusion. But I'm also having a hard time remembering the exact reason, maybe @jonasnick and/or @sipa can help me out here.

@murchandamus
Copy link
Contributor

The formatting seems to be in order on these documents. What is the status of the discussion on these proposals?

@murchandamus
Copy link
Contributor

In #1434 it was mentioned that this proposal may conflict with the way PSBT is used by an existing implementation. Has there been any communication to reconcile that?

@murchandamus murchandamus added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label May 8, 2024
<tt>/NUMh</tt> or <tt>/NUM'</tt> derivation steps nor <tt>/*h</tt>, or <tt>/*'</tt> child derivation.
For these <tt>musig()</tt> expressions, the KEY expressions contained within must be xpubs or derived from
xpubs, and cannot contain child derivation as specified by a <tt>/*</tt>, <tt>/*'</tt>, or <tt>/*h</tt>.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth mentioning explicitly that musig() nesting is forbidden (while musig() inside Script-based multisignatures/thresholds is fine).

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it was sufficiently implied by the above paragraph which states "The musig(KEY, KEY, ..., KEY) expression can only be used inside of a tr() expression as a key expression."

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you can put musig() inside any fragment inside tr(), I think it's very easy to miss the fact that musig() itself is not a fragment, despite using a visually identical syntax. Especially since nesting musig() inside musig() is a natural thing that some people will surely want to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New BIP PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author
Projects
None yet
8 participants