Add (sorted)multi_a descriptor for k-of-n multisig inside tr #24043

sipa · 2022-01-11T19:20:32Z

This adds a new multi_a(k,key_1,key_2,...,key_n) (and corresponding sortedmulti_a) descriptor for k-of-n policies inside tr(). Semantically it is very similar to the existing multi() descriptor, but with the following changes:

The corresponding script is <key1> OP_CHECKSIG <key2> OP_CHECKSIGADD <key3> OP_CHECKSIGADD ... <key_n> OP_CHECKSIGADD <k> OP_NUMEQUAL, rather than the traditional OP_CHECKMULTISIG-based script, making it usable inside the tr() descriptor.
The keys can optionally be specified in x-only notation.
Both the number of keys and the threshold can be as high as 999; this is the limit due to the consensus stacksize=1000 limit

I expect that this functionality will later be replaced with a miniscript-based implementation, but I don't think it's necessary to wait for that.

Limitations:

The wallet code will for not estimate witness size incorrectly for script path spends, which may result in a (dramatic) fee underpayment with large multi_a scripts.
The multi_a script construction is (slightly) suboptimal for n-of-n (where a <key1> OP_CHECKSIGVERIFY ... <key_n-1> OP_CHECKSIGVERIFY <key_n> OP_CHECKSIG would be better). Such a construction is not included here.

sanket1729

Left a few comments, did not review the tests.

src/script/descriptor.cpp

src/script/standard.cpp

DrahtBot · 2022-01-12T03:17:29Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24149 (Signing support for Miniscript Descriptors by darosior)
#24148 (Miniscript support in Output Descriptors by darosior)
#24147 (Miniscript integration by darosior)
#23578 (Add external signer taproot support by Sjors)
#23502 (wallet: Avoid underpaying transaction fees when signing taproot spends by achow101)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#22558 (psbt: Taproot fields for PSBT by achow101)

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.

luke-jr · 2022-01-12T08:02:57Z

It seems like the distinction between the normal and _a variants is likely to cause confusion. Can we not support multi being smart enough to pick the right script? Is there a reason not to do that? How bad of a situation can exist if someone uses the wrong one?

sipa · 2022-01-12T16:35:31Z

@luke-jr I like to keep the invariant that every "function" in descriptors (and later, miniscript) refer to the same opcode construction.

The reason is that while for this example it's unambiguous (multi only in toplevel/sh/wsh, multi_a only in tr), that's not going to be the case for all (or in the case of miniscript, even most) functions. So there will inevitably be constructions with variants which cannot be derived from the context, and given that it seems more consistent to keep the contextual smartness out of this.

luke-jr · 2022-01-12T18:06:57Z

How about erroring if they're used in the wrong context?

sipa · 2022-01-12T18:08:00Z

@luke-jr They do (even before this PR).

Sjors · 2022-01-12T18:35:50Z

The multi_a script construction is (slightly) suboptimal for n-of-n (where...

Isn't it also suboptimal for m-of-n where the permutations are sufficiently tractable to go in separate leaves? There may even be a privacy benefit to never revealing N. Neither is an objection to this descriptor though.

sanket1729

code review ACK 4828d53

darosior

Code review ACK 4828d53

darosior · 2022-01-18T09:21:23Z

src/script/standard.cpp

+    // Redundant, but very fast and selective test.
+    if (script.size() == 0 || script[0] != 32 || script.back() != OP_NUMEQUAL) return {};


if (script.size() > MAX_PUBKEYS_PER_MULTI_A * 34 + 3 + 1) return {};

Would also be a fast test bounding the potential number of iterations below.

darosior · 2022-01-18T09:31:02Z

src/script/standard.cpp

+    ++it;
+    if (it != script.end()) return {};


nit: any reason to not combine these two lines?

darosior · 2022-01-18T10:14:43Z

src/script/descriptor.cpp

+    std::vector<std::unique_ptr<PubkeyProvider>> keys;
+    keys.reserve(match->second.size());
+    for (const auto keyspan : match->second) {
+        if (keyspan.size() != 32) return {};


This cannot happen?

darosior · 2022-01-18T10:19:40Z

test/functional/test_framework/script.py

@@ -27,6 +27,7 @@
 from .ripemd160 import ripemd160

 MAX_SCRIPT_ELEMENT_SIZE = 520
+MAX_PUBKEYS_PER_MULTI_A = 512


This should be 999.

Oh, actually it's part of the next commit

michaelfolkson · 2022-01-31T12:27:57Z

Concept ACK, Approach ACK

I'll attempt to summarize some additional context that was discussed in Friday's Core wallet meeting that may be helpful for other reviewers too. (It was helpful for me. Apologies if I get any of this wrong and it needs a correction)

It seems like the distinction between the normal and _a variants is likely to cause confusion.

If you consider descriptors and Miniscript as one thing under a single name (which one day hopefully they will be, we already consider Miniscript as an extension to descriptors) then Miniscript already has suffixes such as and_b. The idea would be that the user would specify a Policy of multi (Policy doesn't have suffixes) and then the Policy to Miniscript compiler would determine whether it should be multi_a, multi_b etc depending on what the most efficient Miniscript is. This will most likely to be done outside of Core as the compiler isn't expected to be part of Core.

Can we not support multi being smart enough to pick the right script?

Each descriptor and each Miniscript is one-to-one with a specific script. I think multi at the Policy level will be able to pick the most efficient script using the Policy to Miniscript compiler. Still a lot of work to do though as Miniscript doesn't currently support Taproot.

Isn't it also suboptimal for m-of-n where the permutations are sufficiently tractable to go in separate leaves? There may even be a privacy benefit to never revealing N. Neither is an objection to this descriptor though.

As Sjors says there will be likely be many alternative Miniscripts that a Taproot multi Policy could compile down to. A 2-of-3 could be a single script path (a single Tapleaf) or it could be three 2-of-2s (one key path using MuSig2 and two script paths ) and that is before we have threshold key aggregation schemes (FROST etc) that could allow us to put the 2-of-3 directly in the key path.

achow101 · 2022-02-15T18:54:03Z

ACK 4828d53

gruve-p · 2022-02-21T15:05:51Z

ACK 4828d53

michaelfolkson · 2022-03-04T14:52:55Z

@achow101: FYI there's some unresolved comments from @darosior above.

darosior · 2022-03-04T15:13:29Z

No worries, they are nits that's why i acked. :)

…

-------- Original Message --------

On Mar 4, 2022, 14:53, Michael Folkson wrote: ***@***.***(https://github.com/achow101): FYI there's some unresolved comments from ***@***.***(https://github.com/darosior) [above](#24043 (review)). — Reply to this email directly, [view it on GitHub](#24043 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F5CBPVX3DUXOI6WA63U6IPVNANCNFSM5LXDZV4A). Triage notifications on the go with GitHub Mobile for [iOS](https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or [Android](https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub). You are receiving this because you were mentioned.Message ID: ***@***.***>

achow101 · 2022-03-04T15:14:46Z

nits are non-blocking

…ig inside tr 4828d53 Add (sorted)multi_a descriptors to doc/descriptors.md (Pieter Wuille) b5f33ac Simplify wallet_taproot.py functional test (Pieter Wuille) eb0667e Add tests for (sorted)multi_a derivation/signing (Pieter Wuille) c17c6aa Add signing support for (sorted)multi_a scripts (Pieter Wuille) 3eed6fc Add multi_a descriptor inference (Pieter Wuille) 79728c4 Add (sorted)multi_a descriptor and script derivation (Pieter Wuille) 25e95f9 Merge/generalize IsValidMultisigKeyCount/GetMultisigKeyCount (Pieter Wuille) Pull request description: This adds a new `multi_a(k,key_1,key_2,...,key_n)` (and corresponding `sortedmulti_a`) descriptor for k-of-n policies inside `tr()`. Semantically it is very similar to the existing `multi()` descriptor, but with the following changes: * The corresponding script is `<key1> OP_CHECKSIG <key2> OP_CHECKSIGADD <key3> OP_CHECKSIGADD ... <key_n> OP_CHECKSIGADD <k> OP_NUMEQUAL`, rather than the traditional `OP_CHECKMULTISIG`-based script, making it usable inside the `tr()` descriptor. * The keys can optionally be specified in x-only notation. * Both the number of keys and the threshold can be as high as 999; this is the limit due to the consensus stacksize=1000 limit I expect that this functionality will later be replaced with a miniscript-based implementation, but I don't think it's necessary to wait for that. Limitations: * The wallet code will for not estimate witness size incorrectly for script path spends, which may result in a (dramatic) fee underpayment with large multi_a scripts. * The multi_a script construction is (slightly) suboptimal for n-of-n (where a `<key1> OP_CHECKSIGVERIFY ... <key_n-1> OP_CHECKSIGVERIFY <key_n> OP_CHECKSIG` would be better). Such a construction is not included here. ACKs for top commit: achow101: ACK 4828d53 gruve-p: ACK bitcoin@4828d53 sanket1729: code review ACK 4828d53 darosior: Code review ACK 4828d53 Tree-SHA512: 5dcd434b79585f0ff830f7d501d27df5e346f5749f47a3109ec309ebf2cbbad0e1da541eec654026d911ab67fd7cf7793fab0f765628d68d81b96ef2a4d234ce

ajtowns · 2022-03-07T04:41:37Z

The tr(H,multi_a(1,XPUB,XPRV)) test seems to be occasionally failing CI? https://cirrus-ci.com/task/4848707953754112 (through PSBT) https://cirrus-ci.com/task/6303111479296000 (address derivation) [EDIT: I've managed to reproduce this failure once on the master branch locally, but so far it hasn't repeated since I've added debug output]

ajtowns · 2022-03-07T10:34:28Z

I think the problem might be if different calls to do_test result in the same key being selected via keys = self.rand_keys(nkeys * 4) as in a previous invocation, then self.addr_gen will increment the derivation path from whatever was last used and choose /4 for addr_g, while addr_r will reuse the /0 path, and they'll get different results causing an assertion failure.

achow101 · 2022-03-07T10:53:30Z

I think the problem might be if different calls to do_test result in the same key being selected via keys = self.rand_keys(nkeys * 4) as in a previous invocation, then self.addr_gen will increment the derivation path from whatever was last used and choose /4 for addr_g, while addr_r will reuse the /0 path, and they'll get different results causing an assertion failure.

I'm seeing it fail with other indexes too. I think it's actually that multi_a and sortedmulti_a with the same keys will sometimes produce the same addresses for a given index. When the descriptor is imported, it finds that one of the previous addresses was used so it starts at an index other than 0, which is why we get the assertion failure.

achow101 · 2022-03-07T11:01:39Z

#24490 should fix the test failure

ajtowns · 2022-03-07T11:07:23Z

Makes sense!

kouloumos · 2022-10-29T16:41:58Z

doc/descriptors.md

@@ -33,6 +33,7 @@ Output descriptors currently support:
 - Pay-to-taproot outputs (P2TR), through the `tr` function.
 - Multisig scripts, through the `multi` function.
 - Multisig scripts where the public keys are sorted lexicographically, through the `sortedmulti` function.
+- Multisig scripts inside taproot script trees, through the `multi_a` (and `sortedmulti_a`) function.


Should this also be included in the 24.0 release notes?

Something like

tr() descriptors now support multisig scripts through the multi_a (and sortedmulti_a) function.

…veaddresses 92a4ed0 doc: add tr() descriptor example to deriveaddresses (FractalEncrypt) Pull request description: This simple PR adds a missing tr() descriptor example to the `help deriveaddresses` examples. - The functionality added in bitcoin/bitcoin#24043 is a significant departure from legacy multisig address creation, yet there is no corresponding tr() descriptor example in the help. - Having this example in combination with the examples in the descriptors documentation will be helpful to users. I needed this information to correctly create a tr multisig address but was unable. I had to leave the software and use a 3rd party site to ask two separate questions ([1](https://bitcoin.stackexchange.com/questions/115700/how-do-i-create-a-taproot-multisig-address-requiring-21-of-210-keys-to-spend), [2](https://bitcoin.stackexchange.com/questions/115742/signing-psbts-to-spend-from-taproot-multisig-address)) to create an address using the new functionality. Note: This specific example is not provided in the [descriptors.md ](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md) documentation, though there is a similar example with `sortedmulti_a. ` ACKs for top commit: instagibbs: ACK 92a4ed0 kouloumos: ACK 92a4ed0 w0xlt: ACK bitcoin/bitcoin@92a4ed0 Tree-SHA512: 8fb052bd469718157cb64439b885f8b0ecfb5a798535a02bae0a5dc748cd554a3e5ffdd9fe4acaef16156eadb59e1b2bcde7356e811397225f2783a84c8b112f

92a4ed0 doc: add tr() descriptor example to deriveaddresses (FractalEncrypt) Pull request description: This simple PR adds a missing tr() descriptor example to the `help deriveaddresses` examples. - The functionality added in bitcoin#24043 is a significant departure from legacy multisig address creation, yet there is no corresponding tr() descriptor example in the help. - Having this example in combination with the examples in the descriptors documentation will be helpful to users. I needed this information to correctly create a tr multisig address but was unable. I had to leave the software and use a 3rd party site to ask two separate questions ([1](https://bitcoin.stackexchange.com/questions/115700/how-do-i-create-a-taproot-multisig-address-requiring-21-of-210-keys-to-spend), [2](https://bitcoin.stackexchange.com/questions/115742/signing-psbts-to-spend-from-taproot-multisig-address)) to create an address using the new functionality. Note: This specific example is not provided in the [descriptors.md ](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md) documentation, though there is a similar example with `sortedmulti_a. ` ACKs for top commit: instagibbs: ACK 92a4ed0 kouloumos: ACK 92a4ed0 w0xlt: ACK bitcoin@92a4ed0 Tree-SHA512: 8fb052bd469718157cb64439b885f8b0ecfb5a798535a02bae0a5dc748cd554a3e5ffdd9fe4acaef16156eadb59e1b2bcde7356e811397225f2783a84c8b112f

sipa requested review from darosior and achow101 January 11, 2022 19:22

sipa force-pushed the 202201_multi_a branch from 8ca2f20 to 434e4a9 Compare January 11, 2022 19:27

DrahtBot added the Descriptors label Jan 11, 2022

sipa force-pushed the 202201_multi_a branch from 434e4a9 to 5b6852a Compare January 11, 2022 21:56

sipa changed the title ~~Add multi_a descriptor for k-of-n multisig inside tr~~ Add (sorted)multi_a descriptor for k-of-n multisig inside tr Jan 11, 2022

sipa force-pushed the 202201_multi_a branch from 5b6852a to e5ff8f4 Compare January 11, 2022 22:02

sanket1729 reviewed Jan 12, 2022

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

src/script/standard.cpp Outdated Show resolved Hide resolved

src/script/standard.cpp Outdated Show resolved Hide resolved

src/script/standard.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202201_multi_a branch from e5ff8f4 to 2f24733 Compare January 12, 2022 04:45

This was referenced Jan 12, 2022

Add external signer taproot support #23578

Merged

wallet: Avoid underpaying transaction fees when signing taproot spends #23502

Closed

sipa force-pushed the 202201_multi_a branch from 2f24733 to a0e907d Compare January 12, 2022 05:55

This was referenced Jan 12, 2022

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

psbt: Taproot fields for PSBT #22558

Merged

sipa added 7 commits January 12, 2022 11:08

Merge/generalize IsValidMultisigKeyCount/GetMultisigKeyCount

25e95f9

Add (sorted)multi_a descriptor and script derivation

79728c4

Add multi_a descriptor inference

3eed6fc

Add signing support for (sorted)multi_a scripts

c17c6aa

Add tests for (sorted)multi_a derivation/signing

eb0667e

Simplify wallet_taproot.py functional test

b5f33ac

Add (sorted)multi_a descriptors to doc/descriptors.md

4828d53

sipa force-pushed the 202201_multi_a branch from a0e907d to 4828d53 Compare January 12, 2022 16:30

sanket1729 approved these changes Jan 13, 2022

View reviewed changes

darosior approved these changes Jan 18, 2022

View reviewed changes

This was referenced Jan 24, 2022

Deprecate SubtractFeeFromOutputs #24142

Closed

Signing support for Miniscript Descriptors #24149

Merged

Miniscript support in Output Descriptors #24148

Merged

Miniscript integration #24147

Merged

maflcko added this to the 24.0 milestone Feb 21, 2022

achow101 merged commit bada963 into bitcoin:master Mar 4, 2022

kouloumos reviewed Oct 29, 2022

View reviewed changes

kouloumos mentioned this pull request Nov 16, 2022

[24.x] bump version to v24.0 #26503

Merged

FractalEncrypt mentioned this pull request Nov 23, 2022

doc: add tr() descriptor example to deriveaddresses #26558

Merged

michaelfolkson mentioned this pull request Sep 1, 2023

MiniTapscript: port Miniscript to Tapscript #27255

Merged

delta1 added a commit to delta1/elements that referenced this pull request Oct 13, 2023

Merge bada963 into merged_master (Bitcoin PR bitcoin/bitcoin#24043)

45f83fd

bitcoin locked and limited conversation to collaborators Oct 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add (sorted)multi_a descriptor for k-of-n multisig inside tr #24043

Add (sorted)multi_a descriptor for k-of-n multisig inside tr #24043

sipa commented Jan 11, 2022 •

edited

Loading

sanket1729 left a comment

DrahtBot commented Jan 12, 2022 •

edited

Loading

luke-jr commented Jan 12, 2022

sipa commented Jan 12, 2022 •

edited

Loading

luke-jr commented Jan 12, 2022

sipa commented Jan 12, 2022 •

edited

Loading

Sjors commented Jan 12, 2022

sanket1729 left a comment

darosior left a comment

darosior Jan 18, 2022

darosior Jan 18, 2022

darosior Jan 18, 2022

darosior Jan 18, 2022

darosior Jan 18, 2022

michaelfolkson commented Jan 31, 2022 •

edited

Loading

achow101 commented Feb 15, 2022

gruve-p commented Feb 21, 2022

michaelfolkson commented Mar 4, 2022

darosior commented Mar 4, 2022 via email

achow101 commented Mar 4, 2022

ajtowns commented Mar 7, 2022 •

edited

Loading

ajtowns commented Mar 7, 2022

achow101 commented Mar 7, 2022

achow101 commented Mar 7, 2022

ajtowns commented Mar 7, 2022

kouloumos Oct 29, 2022

		// Redundant, but very fast and selective test.
		if (script.size() == 0 \|\| script[0] != 32 \|\| script.back() != OP_NUMEQUAL) return {};

Add (sorted)multi_a descriptor for k-of-n multisig inside tr #24043

Add (sorted)multi_a descriptor for k-of-n multisig inside tr #24043

Conversation

sipa commented Jan 11, 2022 • edited Loading

sanket1729 left a comment

Choose a reason for hiding this comment

DrahtBot commented Jan 12, 2022 • edited Loading

Conflicts

luke-jr commented Jan 12, 2022

sipa commented Jan 12, 2022 • edited Loading

luke-jr commented Jan 12, 2022

sipa commented Jan 12, 2022 • edited Loading

Sjors commented Jan 12, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

darosior Jan 18, 2022

Choose a reason for hiding this comment

darosior Jan 18, 2022

Choose a reason for hiding this comment

darosior Jan 18, 2022

Choose a reason for hiding this comment

darosior Jan 18, 2022

Choose a reason for hiding this comment

darosior Jan 18, 2022

Choose a reason for hiding this comment

michaelfolkson commented Jan 31, 2022 • edited Loading

achow101 commented Feb 15, 2022

gruve-p commented Feb 21, 2022

michaelfolkson commented Mar 4, 2022

darosior commented Mar 4, 2022 via email

achow101 commented Mar 4, 2022

ajtowns commented Mar 7, 2022 • edited Loading

ajtowns commented Mar 7, 2022

achow101 commented Mar 7, 2022

achow101 commented Mar 7, 2022

ajtowns commented Mar 7, 2022

kouloumos Oct 29, 2022

Choose a reason for hiding this comment

sipa commented Jan 11, 2022 •

edited

Loading

DrahtBot commented Jan 12, 2022 •

edited

Loading

sipa commented Jan 12, 2022 •

edited

Loading

sipa commented Jan 12, 2022 •

edited

Loading

michaelfolkson commented Jan 31, 2022 •

edited

Loading

ajtowns commented Mar 7, 2022 •

edited

Loading