388: Add support for musig in descriptor templates

[bigspider]

This required a few changes in the grammar describing descriptor templates: the single symbol KP is no longer enough to cleanly represent the syntax. Therefore, it's now replaced by a more fine-grained symbol hierarchy:

• KEY, mapping 1-on-1 to KEY expressions of descriptors
• KP, key placeholders, that represent a single root key, or an aggregate musig key; regardless of the nature of the key, key placeholders are what is followed by /** or /<M;N>/*.
• KI, key index, the actual "pointer" to the key information vector.

While the validity checks to avoid pubkey reuse before this change were feasible by looking at all the @i expressions in the descriptor template, they are now at the level of the KEY expressions, by comparing their key placeholders.
This means that the specs do not prevent having @i somewhere and musig(@i,...) elsewhere (which does not cause pubkey reuse in the actual scripts).

Note that there is no breaking change with the previous specs, just more fine-grained grammar in order to keep everything working in the presence of musig.

[bigspider]

cc @jgriffiths @benma

[jgriffiths]

@bigspider thanks for the ping. Only one initial question before I review more thoroughly - why do we not allow derivation inside the musig expression KP elements, subject to the same rules as BIP-0390?

[bigspider]

@bigspider thanks for the ping. Only one initial question before I review more thoroughly - why do we not allow derivation inside the musig expression KP elements, subject to the same rules as BIP-0390?

I did indeed suggest removing that from descriptors as well, and I don't know of use cases that justify the (substantial) added complexity for signers.

It is much more inefficient in practice (especially for a transaction with many inputs coming from the same account, since the result of the key aggregation can be cached for all the inputs/change outputs, instead of repeating it each time with different keys).

Finally, it breaks the key placeholder concept, as it would imply that we need to support derivations at different nesting levels in the grammar.

[bigspider]

I added a sentence explicitly mentioning that the same key index can't be repeated inside musig.

[jonatack]

A few minor comments.

[jgriffiths]

It is much more inefficient in practice

This is true but only applies if the feature is used. Support for it could be optional for example; wallets are free to limit the format and type of templates they support.

Finally, it breaks the key placeholder concept, as it would imply that we need to support derivations at different nesting levels in the grammar.

IIUC this only requires that any wildcards in the musig keys to aggregate must be derived at the same index as other keys when a wildcard is present, which is the same restriction as any wildcards in any key expression elsewhere. IOW, the entire expression must be derivable with a single value for all wildcards, whether inside musig() or elsewhere. Perhaps I'm failing to grok your meaning here, or perhaps things would be clearer if I worked through implementing support for this extension personally.

I did indeed suggest removing that from descriptors as well, and I don't know of use cases that justify the (substantial) added complexity for signers.

You raise reasonable points in the linked mail IMO. I can see that there may be a desire to not limit future use-cases, and I can also see that potentially allowing a mixture of fixed and derived keys to aggregate may have some uses (assuming its safe to do so). It is disappointing that (AFAICS) you did not receive a reply.

My concern here is that it appears on the descriptor side we have a technical decision with no clear rationale/documented justification, while on the descriptor template side there is an opposing technical decision with rationale but that may limit future interop or valuable use-cases.

I would prefer to have these two sides aligned, or at least the rationale for differing be explicit on both side (i.e. a reply to the points you raised on the record somewhere).

[bigspider]

This is true but only applies if the feature is used. Support for it could be optional for example; wallets are free to limit the format and type of templates they support.

If the language supports it, software wallet developers will use it; performance will probably be ok on their test transactions spending 1 UTXO; then one of their users who is trying to consolidate 50 UTXOs will discover it's incredibly slow, complain to the vendor, and there will be no way to fix it at that point.

To make a stronger case on the stark difference in performance: for a n-of-n multisig and a transaction spending t inputs

• aggregate-and-derive: 1 KeyAgg, plus 2 * t BIP32 child pubkey derivations
• derive-and-aggregate: t KeyAgg, plus 2 * n * t BIP32 child pubkey derivations
  (some caching might be possible, but it doesn't change the substance).

In other words, aggregate-and-derive doesn't cost significantly more than normal multisigs in hardware signer's performance once you have a few UTXOs (apart from needing 2 rounds, of course), while the penalty cost of derive-and-aggregate is enormous. BIP32 derivations are currently the biggest performance bottleneck in the Ledger bitcoin app, when using multi-user policies.

Therefore, by not supporting it, I expect it will result in more efficient implementations, less debugging, less support tickets, and no loss of functionality.

Finally, it breaks the key placeholder concept, as it would imply that we need to support derivations at different nesting levels in the grammar.

IIUC this only requires that any wildcards in the musig keys to aggregate must be derived at the same index as other keys when a wildcard is present, which is the same restriction as any wildcards in any key expression elsewhere. IOW, the entire expression must be derivable with a single value for all wildcards, whether inside musig() or elsewhere. Perhaps I'm failing to grok your meaning here, or perhaps things would be clearer if I worked through implementing support for this extension personally.

Even just parsing a descriptor template supporting both versions becomes a lot harder. This is more or less what a key expression in my AST looks like:

typedef struct {
    uint32_t num_first;   // NUM_a of /<NUM_a,NUM_b>/*
    uint32_t num_second;  // NUM_b of /<NUM_a,NUM_b>/*

    KeyExpressionType type;
    union {
        // type == 0
        struct {
            int16_t key_index;  // index of the key
        } k;
        // type == 1
        struct {
            rptr_musig_aggr_key_info_t musig_info;
        } m;
    };
} policy_node_keyexpr_t;

They are very simple objects, derivations can only go in one place, and the grammar to describe them is context-free (so parsing for them is based on a trivial lookahead on the next one or few characters).

My concern here is that it appears on the descriptor side we have a technical decision with no clear rationale/documented justification, while on the descriptor template side there is an opposing technical decision with rationale but that may limit future interop or valuable use-cases.

I would prefer to have these two sides aligned, or at least the rationale for differing be explicit on both side (i.e. a reply to the points you raised on the record somewhere).

I understand the concern, and it is my desire to keep wallet policies as aligned as possible in order to minimize friction.

However, wallet policies are not representing the same thing as descriptors:

• descriptors represent arbitrary outputs that can be spent with arbitrarily complicated spending logic that hardware signing devices might or might not support.
• wallet policies represent scripts for a vast class of smart contracts where the logic is however quite simple and uniform: "you're spending from an account, and change (if any) goes back to the same account".

Therefore, wallet policies do not need to be as general as descriptors. It is fine if they are opinionated and much more structured - in fact, this makes them more useful, by implicitly leading wallet developers to only use sane descriptors.
It's the same reason why miniscript doesn't (and can't) support arbitrary Scripts, despite many other Scripts have perfectly valid use cases.

[jgriffiths]

@bigspider Thanks for the detailed response.

Note that as stated I don't disagree with your points on performance (original or as further fleshed out here), except to note that we already have an extension for non-bip44-style policies which e.g. Ledger doesn't support. However, on reflection I think there is nothing here that prevents derivation-before-aggregation being supported later, so this is not a blocker.

BIP32 derivations are currently the biggest performance bottleneck in the Ledger bitcoin app, when using multi-user policies.

Somewhat OT, but you should implement the libwally optimization to avoid computing the parent fingerprint (a) when it is not needed at all (saves a hash160 per derivation) and (b) when deriving a path (only the final derived key may need the fingerprint, the intermediate steps do not). In your impl, get_derived_pubkey never needs the fingerprint so adding a flag to bip32_CKDpub to skip calculating it would be a trivially implemented win there. In fact AFAICS none of your uses of that call require the fingerprint (e.g. PSBT).

Even just parsing a descriptor template supporting both versions becomes a lot harder.

Agreed the parsing gets more difficult. I will likely try adding support for this in wally to see how true that is for different impls.

Therefore, wallet policies do not need to be as general as descriptors. It is fine if they are opinionated and much more structured - in fact, this makes them more useful, by implicitly leading wallet developers to only use sane descriptors.

Disagree with the the sane moniker but I accept/agree with this statement otherwise. I will final review and ack next week when I get a chance to compare implementing these changes with the PR contents, thanks.

[bigspider]

Somewhat OT, but you should implement the libwally optimization to avoid computing the parent fingerprint (a) when it is not needed at all (saves a hash160 per derivation) and (b) when deriving a path (only the final derived key may need the fingerprint, the intermediate steps do not). In your impl, get_derived_pubkey never needs the fingerprint so adding a flag to bip32_CKDpub to skip calculating it would be a trivially implemented win there. In fact AFAICS none of your uses of that call require the fingerprint (e.g. PSBT).

I actually had this implemented few months ago but ended up not merging it because the performance impact is negligible for our implementation. It turns out that almost the entire cost of bip32_CKDpub is the single scalar multiplication. A one line-change with a faster scalar implementation ended up saving close to 50% total running time for some complex transactions! We still have some room for improvements there.

[murchandamus]

These changes appear to be only adding additional options, specifically to support MuSig2, and to improve a few phrases.

@bigspider: What would you consider the next steps? Are you waiting for some ACKs from stakeholders, or do you feel this is ready to go?

[bigspider]

@bigspider: What would you consider the next steps? Are you waiting for some ACKs from stakeholders, or do you feel this is ready to go?

If nobody has further comments, it's good to go from my point of view. I already implemented MuSig2 support in the Ledger Bitcoin app matching these specs.

[murchandamus]

Looks good to me from an editorial perspective. I have not contributed to any implementations of this BIP, nor fully analyzed it from the perspective of an implementer, but it looks to me that the KP expression was simply extended with MuSig support.

I’m gonna leave this open for a few more days in case someone else is still looking to add review, and merge it after that unless the subsequent interactions indicate otherwise or someone else does so before me.

[murchandamus]

LGTM

[jonatack]

Post-merge LGTM.