descriptors: Add a KEY expression representing a list of individual keys #26626

achow101 · 2022-12-02T20:09:34Z

There are some cases where several individual keys are being used in the same descriptor functions. It can be inefficient to treat them all as separate descriptors. This PR adds a new syntax where multiple keys are provided in an array. This list of keys is a ranged KEY expression (like xpubs) with position in the range being explicitly provided. As the list of descriptors is a range, only non-ranged key expressions are allowed. This is to avoid issues with expanding nested ranges. xpubs can be used as key expressions so long as they do not specify a *.

The descriptors will be of the form func({KEY, KEY, KEY,...}).

DrahtBot · 2022-12-02T20:09:37Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	apoelstra, Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

darosior

So when expanding the descriptor containing func({A1,A2,A3},{B1,B2,B3}) at index 0, 1 and 2 you'd get func(A1,B1), func(A2,B2) and func(A3,B3). This can be expressed more concisely with a ranged xpub: func(A/*,B/*) will give you a similar behaviour when expanded at index 0, 1 and 2. The only differences in behaviour being:

This new syntax may be used with non-xpub keys
This new syntax would fail to be expanded at any index >2

I assume that 2) is never desirable for descriptors, the limitation can always be enforced by the application ("never expand at an index superior to 2"). What is a common situation where you wouldn't be able to swap a "raw" key for an xpub?

apoelstra · 2022-12-03T15:20:32Z

concept ACK from me. I understand that the use is primarily in expressing pre-descriptor wallet patterns in descriptors.

This concept feels like it could be used (or is related to) multipath descriptors, like if it were possible to use multiple wildcards, one for "set indexing" and the other for "range indexing".

achow101 · 2022-12-03T16:35:09Z

This concept feels like it could be used (or is related to) multipath descriptors, like if it were possible to use multiple wildcards, one for "set indexing" and the other for "range indexing".

It does, but they're slightly different. I had a discussion with @sipa about that and we concluded that it wasn't useful at least for the purpose that this PR targets.

Multipath descriptors expands into multiple descriptors. In #22838, this is implemented as Parse returning multiple Descriptor objects. However in this PR, {KEY,KEY,KEY...} is interpreted as a single ranged PubkeyProvider like we do for xpubs. Doing it this way allows us to get the intended performance benefit for migrated non-HD wallets as interpreting it like multipath descriptors would just result in the same issue of having thousands of descriptors.

apoelstra · 2022-12-04T17:20:31Z

That makes sense. We may want to revisit, from a conceptual point of view (vs the concrete needs of the Core wallet), later on.

bigspider · 2022-12-05T14:52:30Z

This seems a rather ad-hoc extension that doesn't match the usage of descriptors outside of bitcoin-core.
Therefore, it might imho be detrimental to the adoption of descriptors, as it adds complexity to every other implementation, even if they don't benefit of the additional feature.

Perhaps core could consider having some "extensions" to descriptors for these special use cases, while keeping the standardized descriptor language as lean as possible?

sipa · 2022-12-05T14:57:58Z

@bigspider I think that's how we've been thinking about descriptors in the first place, a base language, with a set of extensions. There is no requirement that everyone implements all of them, as long as supported keywords have the same meaning across implementations. For example, new software might not care about pre-segwit pk/pkh/sh; that would mean implementing BIP380 (base language) and BIP382 (wpkh/wsh), but not BIP381 (pk/pkh/sh). But if they don't implement those, they can't give a different meaning to the same keywords.

Things like BIP384 (combo) and BIP385 (addr/raw) are probably already fairly Bitcoin Core specific.

luke-jr · 2022-12-07T08:58:33Z

Would this scale well to migrating large pre-HD wallets?

achow101 · 2022-12-07T16:38:39Z

Would this scale well to migrating large pre-HD wallets?

I believe it will. It's the intended use of this syntax.

Sjors

Concept ACK, mostly happy with the code.

src/script/descriptor.cpp

Sjors · 2022-12-13T17:58:55Z

src/script/descriptor.cpp

+            m_pubkeys.begin(), m_pubkeys.end(),
+            0,
+            [](const size_t acc, const std::unique_ptr<PubkeyProvider>& pubkey) {
+                return std::max(acc, pubkey->GetSize());


8f4ee67: Is this going to cause problems if compressed and uncompressed keys end up in a single descriptor? Maybe we should enforce keeping those separate (including when migrating from legacy)?

If there are mixed compressed and uncompressed keys in the list, then this will always use the uncompressed size, which may result in overestimating the size. I think this will only be used by miniscript though.

Sjors · 2022-12-13T20:49:06Z

src/test/descriptor_tests.cpp

@@ -450,6 +458,11 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
    CheckUnparsable("sh(sh(pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)))", "sh(sh(pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)))", "Can only have sh() at top level"); // Cannot embed P2SH inside P2SH
    CheckUnparsable("wsh(wsh(pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)))", "wsh(wsh(pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)))", "Can only have wsh() at top level or inside sh()"); // Cannot embed P2WSH inside P2WSH

+    // Using list of keys expression
+    Check("pkh({L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,KzoAz5CanayRKex3fSLQ2BwJpN7U52gZvxMyk78nDMHuqrUxuSJy,KwGNz6YCCQtYvFzMtrC6D3tKTKdBBboMrLTsjr2NYVBwapCkn7Mr})", "pkh({03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,03669b8afcec803a0d323e9a17f3ea8e68e8abe5a278020a929adbec52421adbd0,0260b2003c386519fc9eadf2b5cf124dd8eea4c4e68d5e154050a9346ea98ce600})", "pkh({03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,03669b8afcec803a0d323e9a17f3ea8e68e8abe5a278020a929adbec52421adbd0,0260b2003c386519fc9eadf2b5cf124dd8eea4c4e68d5e154050a9346ea98ce600})", RANGE | DERIVE_HARDENED | SIGNABLE, {{"76a9149a1c78a507689f6f54b847ad1cef1e614ee23f1e88ac"}, {"76a914a48e80ddd68b1a9ac7b3bdf028b00a1dbf5e450388ac"}}, OutputType::LEGACY, {{}, {}});


70189d9 Why are RANGE | DERIVE_HARDENED set?

RANGE to do the range tests, DERIVE_HARDENED to avoid the xpub cache checks.

maflcko · 2023-07-04T17:36:39Z

Needs rebase if still relevant

maflcko · 2023-07-21T08:48:22Z

Maybe mark as draft for as long as CI is red?

ListPubkeyProvider is a ranged PubkeyProvider that returns individual public keys that it was constructed with.

A new KEY expression is introduced which takes a list of non-ranged public keys. This expression itself is ranged and can provide the key at a particular index. It can take any KEY as long as it is not ranged.

DrahtBot added the Descriptors label Dec 2, 2022

achow101 mentioned this pull request Dec 2, 2022

wallet: Migrate non-HD keys with single combo containing a list of keys #26627

Closed

achow101 force-pushed the desc-key-list-expr branch from 3cc02e4 to 7b504c9 Compare December 2, 2022 22:25

This was referenced Dec 3, 2022

Signing support for Miniscript Descriptors #24149

Merged

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

Open

darosior reviewed Dec 3, 2022

View reviewed changes

achow101 force-pushed the desc-key-list-expr branch from 7b504c9 to 70189d9 Compare December 12, 2022 16:17

Sjors reviewed Dec 13, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 14, 2022

Switch hardened derivation marker to h #26076

Merged

achow101 force-pushed the desc-key-list-expr branch from e8a4225 to 78c2e41 Compare January 7, 2023 01:04

DrahtBot added the Needs rebase label Feb 16, 2023

achow101 force-pushed the desc-key-list-expr branch from 78c2e41 to 36e3b3c Compare February 16, 2023 17:59

DrahtBot removed the Needs rebase label Feb 16, 2023

bigspider mentioned this pull request Feb 21, 2023

BIP 388: Wallet Policies for Descriptor Wallets bitcoin/bips#1389

Open

achow101 requested review from sipa and josibake April 25, 2023 16:21

DrahtBot added the Needs rebase label May 8, 2023

achow101 force-pushed the desc-key-list-expr branch from 36e3b3c to e831fd9 Compare May 8, 2023 21:51

DrahtBot removed the Needs rebase label May 8, 2023

DrahtBot mentioned this pull request Jun 14, 2023

Fuzz: a more efficient descriptor parsing target #27888

Merged

DrahtBot added the CI failed label Jul 4, 2023

achow101 added 4 commits July 21, 2023 11:10

descriptors: Add ListPubkeyProvider for lists of single keys

ec94996

ListPubkeyProvider is a ranged PubkeyProvider that returns individual public keys that it was constructed with.

descriptors: Add key expression for list of public keys

29c53f9

A new KEY expression is introduced which takes a list of non-ranged public keys. This expression itself is ranged and can provide the key at a particular index. It can take any KEY as long as it is not ranged.

tests: Test descriptors with list of keys

ee4c4bc

descriptors: Change pos to uint32_t

1488496

achow101 force-pushed the desc-key-list-expr branch from e831fd9 to 1488496 Compare July 21, 2023 15:13

DrahtBot removed the CI failed label Jul 21, 2023

achow101 closed this Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

descriptors: Add a KEY expression representing a list of individual keys #26626

descriptors: Add a KEY expression representing a list of individual keys #26626

achow101 commented Dec 2, 2022 •

edited

DrahtBot commented Dec 2, 2022 •

edited

darosior left a comment

apoelstra commented Dec 3, 2022

achow101 commented Dec 3, 2022

apoelstra commented Dec 4, 2022

bigspider commented Dec 5, 2022

sipa commented Dec 5, 2022 •

edited

luke-jr commented Dec 7, 2022

achow101 commented Dec 7, 2022

Sjors left a comment

Sjors Dec 13, 2022

achow101 Dec 13, 2022

Sjors Dec 13, 2022

achow101 Dec 13, 2022

maflcko commented Jul 4, 2023

maflcko commented Jul 21, 2023

descriptors: Add a KEY expression representing a list of individual keys #26626

descriptors: Add a KEY expression representing a list of individual keys #26626

Conversation

achow101 commented Dec 2, 2022 • edited

DrahtBot commented Dec 2, 2022 • edited

Reviews

Conflicts

darosior left a comment

Choose a reason for hiding this comment

apoelstra commented Dec 3, 2022

achow101 commented Dec 3, 2022

apoelstra commented Dec 4, 2022

bigspider commented Dec 5, 2022

sipa commented Dec 5, 2022 • edited

luke-jr commented Dec 7, 2022

achow101 commented Dec 7, 2022

Sjors left a comment

Choose a reason for hiding this comment

Sjors Dec 13, 2022

Choose a reason for hiding this comment

achow101 Dec 13, 2022

Choose a reason for hiding this comment

Sjors Dec 13, 2022

Choose a reason for hiding this comment

achow101 Dec 13, 2022

Choose a reason for hiding this comment

maflcko commented Jul 4, 2023

maflcko commented Jul 21, 2023

achow101 commented Dec 2, 2022 •

edited

DrahtBot commented Dec 2, 2022 •

edited

sipa commented Dec 5, 2022 •

edited