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

achow101 commented at 7:46 pm on August 30, 2021: member

It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in #17190 (comment), it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.

To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor wpkh(xpub.../0/0/*) and wpkh(xpub.../0/1/*) to represent receive and change addresses, this could be written as wpkh(xpub.../0/<0;1>/*). The multipath specifier is of the form <NUM;NUM>. Each NUM can have its own hardened specifier, e.g. <0;1h> is valid. The multipath specifier can also only appear in one path index in the derivation path.

This results in the parser returning two descriptors. The first descriptor uses the first NUM in all pairs present, and the second uses the second NUM. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just nullptr.

The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).

Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.

Closes #17190

DrahtBot added the label Descriptors on Aug 30, 2021

DrahtBot added the label Mining on Aug 30, 2021

DrahtBot added the label RPC/REST/ZMQ on Aug 30, 2021

DrahtBot added the label Wallet on Aug 30, 2021

DrahtBot commented at 10:28 pm on August 30, 2021: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mjdietzx
Concept ACK	craigraw
Stale ACK	Rspigler, darosior

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:

#30243 (Tr partial descriptors by Eunovo)
#29936 (fuzz: wallet: add target for CreateTransaction by brunoerg)
#29396 (rpc: getdescriptorinfo also returns normalized descriptor by araujo88)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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.

Rspigler commented at 5:48 am on September 1, 2021: contributor

Strong concept ACK!

Glad this is getting a fix. As discussed in 17190 and OP, there’s more improvements that can come after this.

mjdietzx commented at 7:10 pm on September 3, 2021: contributor

Concept and Approach ACK. I plan on updating the test and docs introduced in #22067 to take advantage of this in a follow up PR. So I will try to do a more thorough review and testing of this after that is merged and I start on the followup based on this branch. (or if it looks like this will get merged first I will do that earlier)

Sjors commented at 9:36 am on September 29, 2021: member

Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.

{0,1} seems more readable and would at least be a subset of BIP 88. The conflict is with taproot descriptors, which use { and , for nesting the scripts.

As an example tr([00000000/1']xpub/{0,1}/*,{pk([00000000/2']xpub/{0,1}/*),pk([00000000/3']xpub/{0,1}/*)}) doesn’t look terrible to me. How much does it really complicate the parsing code? They’re always either inside () of a descriptor function like pkh() or within [] of an origin (if we even allow that).

Allowing a mix of hardened and unhardened like <0;1h> seems unnecessary to me and might make alternative implementations more complicated (though perhaps it’s trivial; I haven’t tried).

achow101 commented at 5:16 pm on September 29, 2021: member

The conflict is with taproot descriptors, which use { and , for nesting the scripts.

The conflict is actually with the , in a variety of descriptors. The particular issue I had run into was using , in a multi() descriptor. Since , is used to separate the components of multi(), the parser would look for the next , for the next item to parse. If we use , for the multipath specifier, then it would run into that and parse too little. The parser could be changed to not do that, but that was more work than to just use unused characters. { did not cause any issues, but to make it easier for other implementations which may be parsing differently, I decided to go with a different character as well.

mjdietzx commented at 7:09 pm on October 18, 2021: contributor

I’m doing some more review/testing of this PR, but when I checkout this PR and rebase to master, I get build error:

0wallet/rpcwallet.cpp:3387:45: error: no viable conversion from 'std::pair<std::unique_ptr<Descriptor>, std::unique_ptr<Descriptor> >' to 'std::unique_ptr<Descriptor>'
1  std::unique_ptr<Descriptor> desc = Parse(desc_str, desc_out, error, true);

achow101 commented at 8:58 pm on October 18, 2021: member

Fixed the silent merge conflict.

achow101 force-pushed on Oct 18, 2021

mjdietzx commented at 10:32 pm on October 18, 2021: contributor

So in my tests, from this multipath descriptor string: f"wsh(sortedmulti(M},{f'/<0;1>/*,'.join(xpubs)}/<0;1>/*))"

I have a descriptor that looks like this:

0{
1  'descriptor': 'wsh(sortedmulti(2,tpubDCcZnBitiPCBqsdLWx8Lkc2BrsvVTdZU2Gcioi8yY76HrGSvp4oWyxZ5GUKGVcjdv4wzWbfntfLdcEEfVRamhSgwi1ZxgfNtayQ2QEEjfTv/0/*,tpubDCMtV5vMS9Zn2paRcw7o83ytLY7WHMCGYRSDxhz7HWnZ4ix19EQpi7g3wvYxEvuoXRfLy5XpXJP5VEr7qPzS92TWnVrBMPJYzfupdACC6vP/0/*,tpubDDBi5pEGAmG7k8t2udh8vbYb1Zpwnt5E9bxd86z6xR66r6tYH1aXy5J3kxR2NDBWhVzgcyiAjFw3kCbkf8YcxDx4wjsnvKjKu3SfPVi4dzf/0/*))#shfp6kvs', 
2  'descriptor_change': 'wsh(sortedmulti(2,tpubDCcZnBitiPCBqsdLWx8Lkc2BrsvVTdZU2Gcioi8yY76HrGSvp4oWyxZ5GUKGVcjdv4wzWbfntfLdcEEfVRamhSgwi1ZxgfNtayQ2QEEjfTv/1/*,tpubDCMtV5vMS9Zn2paRcw7o83ytLY7WHMCGYRSDxhz7HWnZ4ix19EQpi7g3wvYxEvuoXRfLy5XpXJP5VEr7qPzS92TWnVrBMPJYzfupdACC6vP/1/*,tpubDDBi5pEGAmG7k8t2udh8vbYb1Zpwnt5E9bxd86z6xR66r6tYH1aXy5J3kxR2NDBWhVzgcyiAjFw3kCbkf8YcxDx4wjsnvKjKu3SfPVi4dzf/1/*))#ghxep260', 
3  'checksum': 'e82r9xv3', 
4  'isrange': True, 
5  'issolvable': True, 
6  'hasprivatekeys': False
7}

But now, multisig.getrawchangeaddress() fails with error: This wallet has no available keys (-4):

 02021-10-18T22:26:50.595000Z TestFramework (INFO): Check that every participant's multisig generates the same addresses...
 12021-10-18T22:26:50.602000Z TestFramework (ERROR): JSONRPC error
 2Traceback (most recent call last):
 3  File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
 4    self.run_test()
 5  File "/Users/michaeldietz/Documents/bitcoin/test/functional/wallet_multisig_descriptor_psbt.py", line 95, in run_test
 6    change_addresses = [multisig.getrawchangeaddress() for multisig in participants["multisigs"]]
 7  File "/Users/michaeldietz/Documents/bitcoin/test/functional/wallet_multisig_descriptor_psbt.py", line 95, in <listcomp>
 8    change_addresses = [multisig.getrawchangeaddress() for multisig in participants["multisigs"]]
 9  File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
10    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
11  File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/authproxy.py", line 144, in __call__
12    raise JSONRPCException(response['error'], status)
13test_framework.authproxy.JSONRPCException: Error: This wallet has no available keys (-4)

Any ideas? I am updating #22067 to use multipath descriptors and using it as a chance to test this PR more

achow101 commented at 10:52 pm on October 18, 2021: member

@mjdietzx importdescriptors is probably failing. You should check the result of the import and see if there are any errors.

Is your branch pushed so I can have a look?

mjdietzx commented at 10:56 pm on October 18, 2021: contributor

In my code I actually have:

0result = multisig.importdescriptors([
1                {
2                    "desc": descriptor["descriptor"],
3                    "active": True,
4                    "timestamp": "now",
5                },
6            ])
7            assert all(r["success"] for r in result)

And that doesn’t catch any errors. I will push my branch shortly and reply w/ it

mjdietzx commented at 11:41 pm on October 18, 2021: contributor

I had an error in my tests - it seems everything in this PR is working properly.

This works:

0result = multisig.importdescriptors([
1  {
2    "desc": descsum_create(f"wsh(sortedmulti({self.M},{f'/<0;1>/*,'.join(xpubs)}/<0;1>/*))"),
3    "active": True,
4    "timestamp": "now",
5  },
6])

re the error I posted eariler, I was trying to do this (using getdescriptorinfo instead of descsum_create):

0descriptor = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/<0;1>/*,'.join(xpubs)}/<0;1>/*))")
1result = multisig.importdescriptors([
2    {
3        "desc": descriptor["descriptor"],
4        "active": True,
5        "timestamp": "now",
6    },
7])

But I was not importing or doing anything with descriptor["descriptor_change"], hence the (obvious in hindsight) error.

We used getdescriptorinfo instead of descsum_create so test/functional/wallet_multisig_descriptor_psbt.py will only use node functions without an additional script for the checksum (see review of #22067 for more info). I wonder if this is useful user feedback at all @achow101? I’m specifically wondering about the interface of getdescriptorinfo now, and if it would make sense to return something like:

0{
1  descriptor // still the multipath descriptor, similar to output of descsum_create
2  // now the broken out descriptors:
3  descriptor_receiving
4  descriptor_change
5 // ... and the other fields are unchanged
6}

achow101 commented at 11:50 pm on October 18, 2021: member

We used getdescriptorinfo instead of descsum_create so test/functional/wallet_multisig_descriptor_psbt.py will only use node functions without an additional script for the checksum (see review of #22067 for more info). I wonder if this is useful user feedback at all @achow101? I’m specifically wondering about the interface of getdescriptorinfo now, and if it would make sense to return something like:

The checksum field is for that purpose. You can concatenate the descriptor input, #, and that checksum.

in src/rpc/misc.cpp:196 in f4a9ed5583 outdated

169@@ -170,6 +170,7 @@ static RPCHelpMan getdescriptorinfo()
170                 RPCResult::Type::OBJ, "", "",
171                 {
172                     {RPCResult::Type::STR, "descriptor", "The descriptor in canonical form, without private keys"},
173+                    {RPCResult::Type::STR, "descriptor_change", "The change descriptor in canonical form, without private keys. Only if the provided descriptor specifies derivation paths for both receiving and change."},

mjdietzx commented at 2:50 pm on October 19, 2021:

I’m still not convinced this is the best interface for getdescriptorinfo

I’d expect descriptor to always be the full descriptor (whether multipath or not). In the case of multipath, I’d then expect two optional attributes descriptor_receiving and descriptor_change to be present in the response.

mjdietzx commented at 2:50 pm on October 19, 2021:

nit: descriptor_change should be optional

achow101 commented at 5:02 pm on October 19, 2021:

descriptor cannot currently be the multipath descriptor as generating the string output for one is not yet implemented. I’m not sure on a good approach for doing that yet.

mjdietzx commented at 8:44 pm on October 19, 2021:

Are you saying something like:

The multipath descriptor string input to getdescriptorinfo is parsed/expanded to the two descriptors
Now each of these descriptors is converted back ToString and returned as descriptor (first) and descriptor_change (second)

And that we currently don’t have a way to go from the two parsed/expanded descriptors back to the single multipath descriptor string?

That does sound tricky, just trying to make sure I understand

achow101 commented at 9:12 pm on October 19, 2021:

Yes, that is correct.

mjdietzx commented at 9:24 pm on October 19, 2021:

Ok, so what’s the downside of something like:

Each of these parsed descriptors is converted back ToString and returned as descriptor_receiving (first) and descriptor_change (second)
The same multipath descriptor string input to getdescriptorinfo is just returned for the descriptor output (with a checksum added)

mjdietzx commented at 3:27 pm on October 23, 2021:

@achow101 could you entertain me on this, if nothing else it’ll help me understand this better as I finish up my review

achow101 commented at 8:41 pm on October 23, 2021:

#15986 provides a bit more context for this.

There are a few reasons I would prefer to not change the behavior of the descriptor result. The first is for backwards compatibility. This RPC may already be used in a way that expects the canonicalized descriptor produced by getdescriptorinfo in the descriptor field. Additionally, if a descriptor with a private key were provided, if we were to just output that same string again, then we would be echoing private keys which is something that we want to avoid doing.

Sjors commented at 4:12 pm on January 28, 2022:

descriptor cannot currently be the multipath descriptor as generating the string output for one is not yet implemented. I’m not sure on a good approach for doing that yet.

Mmm, that has me a bit worried. If a user imports a multipath descriptor, I think we should remember it in that form. And if we embrace multipath descriptors, we should probably also use them for new wallets.

Perhaps a better design would be a MultipathDescriptor subclass that only has a receive and change method, or more neutral: a pair method that returns a pair of Descriptors that then work as usual.

In the future std::pair<Descriptor> can be generalized to std::map<uint32, Descriptor> to allow <NUM,NUM,...,NUM>, etc. That may even be appropriate now: we could treat <0;1> as a typical receive/change setup, and refuse to import any other combination (though that’s an RPC level thing, and we might as well use std::pair<Descriptor> given such an import restriction).

in src/test/fuzz/descriptor_parse.cpp:29 in f4a9ed5583 outdated

28+        if (desc.first) {
29+            (void)desc.first->ToString();
30+            (void)desc.first->IsRange();
31+            (void)desc.first->IsSolvable();
32+        }
33+        if (desc.second) {

mjdietzx commented at 2:53 pm on October 19, 2021:

Is it worth having an assertion that if desc.second is defined, then desc.first must also be defined here?

Then for L31,L32 should we be asserting that desc.first->IsRange() == desc.second->IsRange() and desc.first->IsSolvable() == desc.second->IsSolvable()?

achow101 commented at 5:46 pm on October 19, 2021:

Done

in src/script/descriptor.cpp:1298 in f4a9ed5583 outdated

1351-        if (!internal_key) return nullptr;
1352+        auto internal_keys = ParsePubkey(key_exp_index, arg, ParseScriptContext::P2TR, out, error);
1353+        if (!internal_keys.first) return {nullptr, nullptr};
1354+        bool has_multipath = (bool)internal_keys.second;
1355+        if (!has_multipath) {
1356+            internal_keys.second = std::move(internal_keys.first->Clone());

achow101 commented at 5:45 pm on October 19, 2021:

Fixed

achow101 force-pushed on Oct 19, 2021

in src/rpc/misc.cpp:283 in 796c439b8c outdated

260-                RPCResult::Type::ARR, "", "",
261-                {
262-                    {RPCResult::Type::STR, "address", "the derived addresses"},
263-                }
264+            {
265+                RPCResult{"for single derivation descriptors",

mjdietzx commented at 3:24 pm on October 23, 2021:

I’ve been thinking through the response returned here. It doesn’t seem ideal that the shape of this response differs between a single descriptor and a multipath descriptor. Ie this returns an array of addresses if we pass in a descriptor, but it returns an object if we pass in a multipath descriptor.

Have you thought through an api where the response is more uniform and “feels” the same for both types? I don’t necessarily know a better way to do it, wondering if you’ve thought this through though

achow101 commented at 8:35 pm on October 23, 2021:

The primary reason for having the two different return results is to maintain backwards compatibility with anyone who may be using the RPC now.

mjdietzx commented at 10:00 pm on October 23, 2021:

Yeah, I figured there’d need to be a deprecation process if we wanted to do this. Anyways, if you think it’s enough of an improvement to warrant that I’d be happy to do give it a go as a followup PR. Just lmk

in src/rpc/misc.cpp:348 in 796c439b8c outdated

341+        UniValue obj(UniValue::VOBJ);
342+        obj.pushKV("receive", addresses);
343+        obj.pushKV("change", DeriveAddresses(descs.second.get(), range_begin, range_end, key_provider));
344+        return obj;
345+    } else {
346+        return addresses;

mjdietzx commented at 3:25 pm on October 23, 2021:

Related to my above comment, if the response is uniform we could potentially simplify this a bit and have a single return at the end of the function

mjdietzx commented at 3:26 pm on October 23, 2021: contributor

Getting very close to signing off on this PR, just a few minor things/questions

mjdietzx commented at 9:58 pm on October 23, 2021: contributor

Review ACK 796c439. I also lightly used/tested this by running #23308 on top of this as additional test coverage.

mjdietzx commented at 5:57 pm on October 24, 2021: contributor

I think there may be another silent merge conflict w/ master:

0wallet/test/util.cpp:33:37: error: no viable conversion from 'std::pair<std::unique_ptr<Descriptor>, std::unique_ptr<Descriptor> >' to 'std::unique_ptr<Descriptor>'
1        std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", provider, error, /* require_checksum=*/ false);

Can you try rebasing again?

achow101 force-pushed on Oct 24, 2021

achow101 commented at 10:23 pm on October 24, 2021: member

Rebased again

mjdietzx commented at 11:41 pm on October 24, 2021: contributor

reACK a6ff1c7

maflcko removed the label Mining on Oct 25, 2021

prusnak referenced this in commit 7088ca639e on Nov 2, 2021

prusnak referenced this in commit 2755f4a7a9 on Nov 2, 2021

prusnak referenced this in commit 21521d45aa on Nov 2, 2021

prusnak referenced this in commit 5dc60b35db on Nov 2, 2021

DrahtBot added the label Needs rebase on Nov 25, 2021

achow101 force-pushed on Dec 5, 2021

DrahtBot removed the label Needs rebase on Dec 5, 2021

DrahtBot added the label Needs rebase on Dec 8, 2021

achow101 force-pushed on Dec 8, 2021

DrahtBot removed the label Needs rebase on Dec 8, 2021

prusnak commented at 7:55 pm on December 9, 2021: contributor

For the record: recently we updated the Trezor stack (Firmware + Suite) to support Taproot and we used descriptors with ranges from this proposal for accounts, e.g.

tr([5c9e228d/86'/0'/0']xpub6Bw885JisRbcKmowfBvMmCxaFHodKn1VpmRmctmJJoM8D4DzyP4qJv8ZdD9V9r3SSGjmK2KJEDnvLH6f1Q4HrobEvnCeKydNvf1eir3RHZk/<0;1>/*)#4swej4wz

DrahtBot added the label Needs rebase on Dec 10, 2021

achow101 force-pushed on Dec 10, 2021

DrahtBot removed the label Needs rebase on Dec 10, 2021

DrahtBot added the label Needs rebase on Dec 14, 2021

achow101 force-pushed on Dec 14, 2021

DrahtBot removed the label Needs rebase on Dec 14, 2021

achow101 force-pushed on Dec 14, 2021

Rspigler commented at 1:41 pm on December 21, 2021: contributor

Would like to test but don’t know how, as this PR doesn’t yet change how descriptors are stored or outputted. Any ideas?

DrahtBot added the label Needs rebase on Dec 26, 2021

achow101 force-pushed on Dec 26, 2021

DrahtBot removed the label Needs rebase on Dec 26, 2021

DrahtBot added the label Needs rebase on Dec 31, 2021

achow101 force-pushed on Jan 10, 2022

DrahtBot removed the label Needs rebase on Jan 10, 2022

DrahtBot added the label Needs rebase on Jan 20, 2022

achow101 force-pushed on Jan 24, 2022

DrahtBot removed the label Needs rebase on Jan 24, 2022

DrahtBot added the label Needs rebase on Feb 21, 2022

achow101 force-pushed on Feb 22, 2022

DrahtBot removed the label Needs rebase on Feb 22, 2022

DrahtBot added the label Needs rebase on Mar 4, 2022

achow101 force-pushed on Mar 4, 2022

DrahtBot removed the label Needs rebase on Mar 4, 2022

DrahtBot added the label Needs rebase on Mar 23, 2022

achow101 force-pushed on Mar 24, 2022

DrahtBot removed the label Needs rebase on Mar 24, 2022

achow101 force-pushed on Apr 16, 2022

DrahtBot added the label Needs rebase on May 4, 2022

prusnak commented at 11:01 am on May 4, 2022: contributor

Should <NUM;NUM;NUM;NUM> also work or we only aim to support only tuples of exactly two values? It’s not obvious from the documentation.

achow101 force-pushed on May 4, 2022

DrahtBot removed the label Needs rebase on May 4, 2022

craigraw commented at 12:39 pm on May 11, 2022: none

ACK.

Successfully tested getdescriptorinfo, deriveaddresses and importdescriptors against singlesig and multisig multipath descriptors generated by Sparrow. Wallet balances, addresses and checksums all match. Sparrow uses multipath descriptors since Nov 2021.

wolfmcnally referenced this in commit 318584aa5b on Jun 26, 2022

Rspigler commented at 2:23 am on July 10, 2022: contributor

tACK commit 057511d35375bada45a9b71b7e06db1edcb34091

Computing checksum of the multipath descriptor:

getdescriptorinfo "wpkh([2fb36ae6/84'/0'/0']xpub6Bpt6XP21UMYtMC7xjKsMYorev7si6TY3QfqFf3jLP6LvTcdV66DGnGcW6TZWduhMBKBqC8XykFj5zKPXvS4TasoeHYsozqqdfH1ZZ7fZMK/<0;1>/*)"

{ “descriptor”: “wpkh([2fb36ae6/84’/0’/0’]xpub6Bpt6XP21UMYtMC7xjKsMYorev7si6TY3QfqFf3jLP6LvTcdV66DGnGcW6TZWduhMBKBqC8XykFj5zKPXvS4TasoeHYsozqqdfH1ZZ7fZMK/0/)#ljnxrhyu”, “descriptor_change”: “wpkh([2fb36ae6/84’/0’/0’]xpub6Bpt6XP21UMYtMC7xjKsMYorev7si6TY3QfqFf3jLP6LvTcdV66DGnGcW6TZWduhMBKBqC8XykFj5zKPXvS4TasoeHYsozqqdfH1ZZ7fZMK/1/)#wxk87z5y”, “checksum”: “ava8ef96”, “isrange”: true, “issolvable”: true, “hasprivatekeys”: false }

importdescriptors "[{\"desc\":\"wpkh([2fb36ae6/84'/0'/0']xpub6Bpt6XP21UMYtMC7xjKsMYorev7si6TY3QfqFf3jLP6LvTcdV66DGnGcW6TZWduhMBKBqC8XykFj5zKPXvS4TasoeHYsozqqdfH1ZZ7fZMK/<0;1>/*)#ava8ef96\",\"range\":[0,1000],\"timestamp\":1657145357}]"

[ { “success”: true } ]

Unrelated to this specific PR, was importdescriptor, since you have to escape all the double quotes, and there’s no mention of this in the console documentation (and there’s also a discrepancy in the timestamp spacing). Putting this here for notes, I might do this myself

DrahtBot added the label Needs rebase on Jul 14, 2022

achow101 force-pushed on Jul 15, 2022

DrahtBot removed the label Needs rebase on Jul 15, 2022

achow101 force-pushed on Jul 28, 2022

achow101 force-pushed on Jul 29, 2022

in src/wallet/rpc/spend.cpp:668 in 58707b877c outdated

646                     throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unable to parse descriptor '%s': %s", desc_str, error));
647                 }
648-                desc->Expand(0, desc_out, scripts_temp, desc_out);
649-                coinControl.m_external_provider = Merge(coinControl.m_external_provider, desc_out);
650+                for (auto& desc : descs) {
651+                    desc->Expand(0, desc_out, scripts_temp, desc_out);

maflcko commented at 2:07 pm on July 30, 2022:

Wouldn’t desc_out need to be moved inside the scope of this for loop to avoid accumulating duplicate descriptors?

achow101 commented at 4:06 pm on August 1, 2022:

It doesn’t matter since it all gets accumulated into into coinControl.m_external_provider anyways.

DrahtBot added the label Needs rebase on Aug 17, 2022

achow101 force-pushed on Aug 18, 2022

DrahtBot removed the label Needs rebase on Aug 18, 2022

DrahtBot added the label Needs rebase on Aug 18, 2022

achow101 force-pushed on Aug 18, 2022

DrahtBot removed the label Needs rebase on Aug 19, 2022

fanquake commented at 3:25 pm on August 24, 2022: member

In the fuzzing job:

0MS: 0 ; base unit: 0000000000000000000000000000000000000000
10x67,0x65,0x74,0x64,0x65,0x73,0x63,0x72,0x69,0x70,0x74,0x6f,0x72,0x69,0x6e,0x66,0x6f,0x5c,0xa,0x77,0x73,0x68,0x28,0x75,0x3a,0x30,0x29,0x2c,0x50,0x75,0x75,0x6c,0x3a,0x31,0x29,
2getdescriptorinfo\\\012wsh(u:0),Puul:1)
3artifact_prefix='./'; Test unit written to ./oom-15b296231df90f4407beea7c9de1b8c129658fae
4Base64: Z2V0ZGVzY3JpcHRvcmluZm9cCndzaCh1OjApLFB1dWw6MSk=
5SUMMARY: libFuzzer: out-of-memory

DrahtBot added the label Needs rebase on Aug 24, 2022

achow101 force-pushed on Aug 24, 2022

DrahtBot removed the label Needs rebase on Aug 24, 2022

achow101 commented at 10:51 pm on August 24, 2022: member

It’s unclear to me what the fuzzer is OOM’ing on.

achow101 force-pushed on Aug 30, 2022

achow101 commented at 9:59 pm on August 30, 2022: member

I think I fixed the fuzzer issue.

DrahtBot added the label Needs rebase on Sep 1, 2022

achow101 force-pushed on Sep 1, 2022

DrahtBot removed the label Needs rebase on Sep 1, 2022

DrahtBot added the label Needs rebase on Sep 20, 2022

achow101 force-pushed on Sep 20, 2022

DrahtBot removed the label Needs rebase on Sep 20, 2022

DrahtBot added the label Needs rebase on Oct 26, 2022

achow101 force-pushed on Nov 4, 2022

DrahtBot removed the label Needs rebase on Nov 5, 2022

darosior commented at 11:55 pm on November 24, 2022: member

Concept ACK.

DrahtBot added the label Needs rebase on Dec 6, 2022

achow101 force-pushed on Dec 6, 2022

DrahtBot removed the label Needs rebase on Dec 6, 2022

DrahtBot added the label Needs rebase on Jan 10, 2023

achow101 force-pushed on Jan 10, 2023

DrahtBot removed the label Needs rebase on Jan 11, 2023

DrahtBot added the label Needs rebase on Jan 16, 2023

achow101 force-pushed on Jan 16, 2023

DrahtBot removed the label Needs rebase on Jan 16, 2023

DrahtBot added the label Needs rebase on Feb 16, 2023

achow101 force-pushed on Feb 16, 2023

DrahtBot removed the label Needs rebase on Feb 16, 2023

DrahtBot added the label Needs rebase on Feb 21, 2023

achow101 force-pushed on Mar 1, 2023

DrahtBot removed the label Needs rebase on Mar 1, 2023

DrahtBot added the label Needs rebase on Mar 16, 2023

achow101 force-pushed on Mar 17, 2023

DrahtBot removed the label Needs rebase on Mar 17, 2023

DrahtBot added the label Needs rebase on Mar 23, 2023

achow101 force-pushed on Apr 10, 2023

DrahtBot removed the label Needs rebase on Apr 10, 2023

achow101 force-pushed on Apr 10, 2023

DrahtBot added the label Needs rebase on Apr 11, 2023

achow101 force-pushed on Apr 11, 2023

DrahtBot removed the label Needs rebase on Apr 11, 2023

DrahtBot added the label Needs rebase on May 8, 2023

achow101 force-pushed on May 10, 2023

DrahtBot removed the label Needs rebase on May 10, 2023

DrahtBot added the label CI failed on May 10, 2023

achow101 force-pushed on May 11, 2023

DrahtBot removed the label CI failed on May 11, 2023

DrahtBot added the label Needs rebase on May 22, 2023

achow101 force-pushed on May 22, 2023

DrahtBot removed the label Needs rebase on May 22, 2023

in src/rpc/output_script.cpp:298 in 4ead3e653d outdated

294@@ -278,7 +295,7 @@ static RPCHelpMan deriveaddresses()
295                     throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot derive script without private keys");
296                 }
297 
298-                for (const CScript& script : scripts) {
299+                for (const CScript &script : scripts) {

darosior commented at 12:55 pm on May 31, 2023:

Looks spurious?

achow101 commented at 4:01 pm on June 13, 2023:

Removed

in src/wallet/test/psbt_wallet_tests.cpp:25 in 4ead3e653d outdated

19@@ -20,8 +20,10 @@ static void import_descriptor(CWallet& wallet, const std::string& descriptor)
20     AssertLockHeld(wallet.cs_wallet);
21     FlatSigningProvider provider;
22     std::string error;
23-    std::unique_ptr<Descriptor> desc = Parse(descriptor, provider, error, /* require_checksum=*/ false);
24-    assert(desc);
25+    auto descs = Parse(descriptor, provider, error, /* require_checksum=*/ false);
26+    assert(!descs.empty());
27+    assert(descs.size() == 1);

darosior commented at 1:09 pm on May 31, 2023:

nit (here and below): redundant to check for emptiness before asserting the size is 1

achow101 commented at 4:01 pm on June 13, 2023:

Removed

in src/rpc/output_script.cpp:312 in 6f75362e2f outdated

308@@ -268,15 +309,13 @@ static RPCHelpMan deriveaddresses()
309                 std::tie(range_begin, range_end) = ParseDescriptorRange(request.params[1]);
310             }
311 
312+

darosior commented at 1:27 pm on May 31, 2023:

nit: spurious?

achow101 commented at 4:01 pm on June 13, 2023:

Removed

DrahtBot added the label CI failed on Jun 9, 2023

in src/wallet/scriptpubkeyman.cpp:1784 in 4ead3e653d outdated

1779@@ -1780,8 +1780,9 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1780         std::string desc_str = "combo(" + origin_str + HexStr(key.GetPubKey()) + ")";
1781         FlatSigningProvider keys;
1782         std::string error;
1783-        std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);
1784-        WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
1785+        std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_str, keys, error, false);
1786+        assert(descs.size() == 1); // It shouldn't be possible to have an invalid or multipath descriptor

darosior commented at 1:21 pm on June 12, 2023:

nit (here and below): CHECKNONFATAL to avoid crashing the node but propagate the error back to the RPC command?

achow101 commented at 4:01 pm on June 13, 2023:

Done

in src/script/descriptor.cpp:1779 in 98796fccbd outdated

1775+                } else if (vec.size() == 1) {
1776+                    for (size_t i = 0; i < *num_multipath; ++i) {
1777+                        vec.emplace_back(vec.at(0)->Clone());
1778+                    }
1779+                } else if (vec.size() != *num_multipath) {
1780+                    error = strprintf("Miniscript: Multipath subscripts have mismatched lengths");

darosior commented at 1:41 pm on June 12, 2023:

nit: s/subscripts/derivation paths/

I guess “subscripts” is a copy pasta from the tr() error?

achow101 commented at 4:01 pm on June 13, 2023:

Done

in src/test/descriptor_tests.cpp:823 in b744d51948 outdated

714+    CheckUnparsable("pkh([deadbeef/<0;1>]xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/0)", "pkh([deadbeef/<0;1>]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/0)", "pkh(): Key path value \'<0;1>\' specifies multipath in a section where multipath is not allowed");
715+    CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/6/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*)})", "tr(xpub6B4sSbNr8XFYXqqKB7PeUemqgEaVtCLjgd5Lf2VYtezSHozC7ffCvVNCyu9TCgHntRQdimjV3tHbxmNfocxtuh6saNtZEw91gjXLRhQ3Yar/6/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub6AhFhZJJGt9YB8i85RfrJ8jT3T2FF5EejDCXqXfm1DAczFEXkk8HD3CXTg2TmKM8wTbSnSw3wPg5JuyLitUrpRmkjn2BQXyZnqJx16AGy94/0/0/<3;4>/*)})", "tr(): Multipath subscripts have mismatched lengths");
716+    CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7;8;9>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7;8;9>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath subscripts have mismatched lengths");
717+    CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath internal key mismatches multipath subscripts lengths");
718+    CheckUnparsable("sh(multi(2,xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0/*,xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*))", "sh(multi(2,xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*,xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4>/*))", "multi(): Multipath derivation paths have mismatched lengths");
719+

darosior commented at 2:27 pm on June 12, 2023:

Maybe worth checking across the branches of a Miniscript too?

 0diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
 1index 3ea8d63ab3..e1864652d8 100644
 2--- a/src/test/descriptor_tests.cpp
 3+++ b/src/test/descriptor_tests.cpp
 4@@ -716,6 +731,7 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
 5     CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7;8;9>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7;8;9>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath subscripts have mismatched lengths");
 6     CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath internal key mismatches multipath subscripts lengths");
 7     CheckUnparsable("sh(multi(2,xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0/*,xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*))", "sh(multi(2,xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*,xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4>/*))", "multi(): Multipath derivation paths have mismatched lengths");
 8+    CheckUnparsable("wsh(andor(pk(xprv9xGFvhWa1Koc8dmeEG5JXVfMaNBkioYscFGmn7yx8YnhFQYeydFfudxdKRzR5p7v1kip85ohB6eUQbPpAee9cFZu9M85G9X4ovPP4xw4xbM/0'/<0;1;2;3>/*),older(10000),pk(xprv9x9bas78RYwopceXTStT8vDuTiu6g1u91L6sG3DhHfDDXKPrYdcHcDuDw4Hv1kjZBWKoZnobUHrdoFxBPUMBTMruUs8HwzL8GxGA95MmZ7v/8/<0;1;2>/*)))", "wsh(andor(pk(xpub6BFcLD3TqhMuM7r7LHcJtdc68Q2F8GGiyUCNaWPZgtKg8CsoXAZvTSH7AhaCPnuuewjwzA2gbAm1y6uaDNNxa7JqTiL76cdioT5rxjgxWXF/0'/<0;1;2;3>/*),older(10000),pk(xpub6B8wzNe2FvW736izZURTW4Ae1kjb5UczNZ2U4RdJqzkCQ7j16AvYA2DhnL8Kb5FeWAZJ43NnGPdjpeSKvAeM8YGaqhCzpD743Uv6S87hfAt/8/<0;1;2>/*)))", "Miniscript: Multipath subscripts have mismatched lengths");
 9 
10     // Multisig constructions
11     Check("multi(1,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "multi(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", SIGNABLE, {{"512103a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd4104a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea23552ae"}}, std::nullopt);

darosior commented at 1:54 pm on June 13, 2023:

And also for various invalid format for the multipath step in the derivation path (taken from my Rust implementation, this uncovered a small bug in accepting <0>):

 0diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
 1index 3ea8d63ab3..6fb2b8dfab 100644
 2--- a/src/test/descriptor_tests.cpp
 3+++ b/src/test/descriptor_tests.cpp
 4@@ -716,6 +716,13 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
 5     CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7;8;9>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7;8;9>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath subscripts have mismatched lengths");
 6     CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath internal key mismatches multipath subscripts lengths");
 7     CheckUnparsable("sh(multi(2,xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0/*,xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*))", "sh(multi(2,xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*,xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4>/*))", "multi(): Multipath derivation paths have mismatched lengths");
 8+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<>/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<>/*)", "wpkh(): Key path value '' is not a valid uint32");
 9+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0/*)", "wpkh(): Key path value '<0' is not a valid uint32");
10+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/0>/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/0>/*)", "wpkh(): Key path value '0>' is not a valid uint32");
11+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0>/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0>/*)", "");
12+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0;>/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0;>/*)", "wpkh(): Key path value '' is not a valid uint32");
13+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<;1>/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<;1>/*)", "wpkh(): Key path value '' is not a valid uint32");
14+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0;1;>/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0;1;>/*)", "wpkh(): Key path value '' is not a valid uint32");
15 
16     // Multisig constructions
17     Check("multi(1,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "multi(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", SIGNABLE, {{"512103a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd4104a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea23552ae"}}, std::nullopt);

achow101 commented at 4:02 pm on June 13, 2023:

Added tests as suggested

in src/test/descriptor_tests.cpp:783 in b744d51948 outdated

707+            {
708+                {{6, 0}, {6, 1}, {6, 2}, {1, 0, 0}, {1, 0, 1}, {1, 0, 2}, {0, 0, 3, 0}, {0, 0, 3, 1}, {0, 0, 3, 2}},
709+                {{6, 0}, {6, 1}, {6, 2}, {2, 0, 0}, {2, 0, 1}, {2, 0, 2}, {0, 0, 4, 0}, {0, 0, 4, 1}, {0, 0, 4, 2}},
710+                {{6, 0}, {6, 1}, {6, 2}, {3, 0, 0}, {3, 0, 1}, {3, 0, 2}, {0, 0, 5, 0}, {0, 0, 5, 1}, {0, 0, 5, 2}},
711+            }
712+    );

darosior commented at 3:25 pm on June 12, 2023:

Let’s also cover Miniscript (along with wsh)?

 0diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
 1index 3ea8d63ab3..adbaf48e77 100644
 2--- a/src/test/descriptor_tests.cpp
 3+++ b/src/test/descriptor_tests.cpp
 4@@ -710,6 +710,31 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
 5                 {{6, 0}, {6, 1}, {6, 2}, {3, 0, 0}, {3, 0, 1}, {3, 0, 2}, {0, 0, 5, 0}, {0, 0, 5, 1}, {0, 0, 5, 2}},
 6             }
 7     );
 8+    CheckMultipath("wsh(or_d(pk([2557c640/48h/1h/0h/2h]xprv9ws7hGFQPbDga6QrETbFM7Gqc7m15UNoJ7KF5kkDhyZCBcANAqRMUCytQ4JM1nLSYvGyFjg6TvBEfNrN3znaFdb67jQoQ7z9kFnd4BUUJiE/<0;1>/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xprv9ws7hGFQPbDgbCvNcYVfGeGK8UTSFmAho4iAXZf13yQVJmHuKHN9oMXCv7zsJn8Dcqvqy2iugFWAhDdUUX6r5VLNWkRTpxVoQJ6DbzY9eYa/<0;1>/*),older(2))))",
 9+            "wsh(or_d(pk([2557c640/48h/1h/0h/2h]xpub6ArU6mnJDxmynaVKLV8FiFDaA9bVUw6efLEqt99qGK6B4QVWiNjc21JNFKkXNjgT8NCUmpFpSSBrYFtWEAqGirbqT4J1bRFpWyAnYdzmZUm/<0;1>/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xpub6ArU6mnJDxmyogzqia2fdnD3gWHvfDtZAHdmKx4ccJwUBZd3rpgQM9qgmPAn1mqT2yh81uvGGohMkg3fNLoXZzn7sRo4a1X3KnCAVot2yuS/<0;1>/*),older(2))))",
10+            {
11+                "wsh(or_d(pk([2557c640/48h/1h/0h/2h]xprv9ws7hGFQPbDga6QrETbFM7Gqc7m15UNoJ7KF5kkDhyZCBcANAqRMUCytQ4JM1nLSYvGyFjg6TvBEfNrN3znaFdb67jQoQ7z9kFnd4BUUJiE/0/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xprv9ws7hGFQPbDgbCvNcYVfGeGK8UTSFmAho4iAXZf13yQVJmHuKHN9oMXCv7zsJn8Dcqvqy2iugFWAhDdUUX6r5VLNWkRTpxVoQJ6DbzY9eYa/0/*),older(2))))",
12+                "wsh(or_d(pk([2557c640/48h/1h/0h/2h]xprv9ws7hGFQPbDga6QrETbFM7Gqc7m15UNoJ7KF5kkDhyZCBcANAqRMUCytQ4JM1nLSYvGyFjg6TvBEfNrN3znaFdb67jQoQ7z9kFnd4BUUJiE/1/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xprv9ws7hGFQPbDgbCvNcYVfGeGK8UTSFmAho4iAXZf13yQVJmHuKHN9oMXCv7zsJn8Dcqvqy2iugFWAhDdUUX6r5VLNWkRTpxVoQJ6DbzY9eYa/1/*),older(2))))",
13+            },
14+            {
15+                "wsh(or_d(pk([2557c640/48h/1h/0h/2h]xpub6ArU6mnJDxmynaVKLV8FiFDaA9bVUw6efLEqt99qGK6B4QVWiNjc21JNFKkXNjgT8NCUmpFpSSBrYFtWEAqGirbqT4J1bRFpWyAnYdzmZUm/0/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xpub6ArU6mnJDxmyogzqia2fdnD3gWHvfDtZAHdmKx4ccJwUBZd3rpgQM9qgmPAn1mqT2yh81uvGGohMkg3fNLoXZzn7sRo4a1X3KnCAVot2yuS/0/*),older(2))))",
16+                "wsh(or_d(pk([2557c640/48h/1h/0h/2h]xpub6ArU6mnJDxmynaVKLV8FiFDaA9bVUw6efLEqt99qGK6B4QVWiNjc21JNFKkXNjgT8NCUmpFpSSBrYFtWEAqGirbqT4J1bRFpWyAnYdzmZUm/1/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xpub6ArU6mnJDxmyogzqia2fdnD3gWHvfDtZAHdmKx4ccJwUBZd3rpgQM9qgmPAn1mqT2yh81uvGGohMkg3fNLoXZzn7sRo4a1X3KnCAVot2yuS/1/*),older(2))))"
17+            },
18+            {
19+                "wsh(or_d(pk([2557c640/48h/1h/0h/2h]xpub6ArU6mnJDxmynaVKLV8FiFDaA9bVUw6efLEqt99qGK6B4QVWiNjc21JNFKkXNjgT8NCUmpFpSSBrYFtWEAqGirbqT4J1bRFpWyAnYdzmZUm/0/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xpub6ArU6mnJDxmyogzqia2fdnD3gWHvfDtZAHdmKx4ccJwUBZd3rpgQM9qgmPAn1mqT2yh81uvGGohMkg3fNLoXZzn7sRo4a1X3KnCAVot2yuS/0/*),older(2))))",
20+                "wsh(or_d(pk([2557c640/48h/1h/0h/2h]xpub6ArU6mnJDxmynaVKLV8FiFDaA9bVUw6efLEqt99qGK6B4QVWiNjc21JNFKkXNjgT8NCUmpFpSSBrYFtWEAqGirbqT4J1bRFpWyAnYdzmZUm/1/*),and_v(v:pkh([00aabb22/48h/1h/0h/2h]xpub6ArU6mnJDxmyogzqia2fdnD3gWHvfDtZAHdmKx4ccJwUBZd3rpgQM9qgmPAn1mqT2yh81uvGGohMkg3fNLoXZzn7sRo4a1X3KnCAVot2yuS/1/*),older(2))))"
21+            },
22+            RANGE,
23+            {
24+                {{"0020538436a60f2a638ea9e1e1342e9b93374aa7ec559ff0a805b3a185d4ba855d7f"},{"00203a588d107d604b6913201c7c1e1722f07a0f8fb3a382744f17b9ae5f6ccfcdd7"},{"0020d30fb375f7c491a208e77c7b5d0996ca14cf4a770c2ab5981f915c0e4565c74a"}},
25+                {{"002072b5fc3a691c48fdbaf485f27e787b4094055d4b434c90c81ed1090f3d48733b"},{"0020a9ccdf4496e5d60db4704b27494d9d74f54a16c180ff954a43ce5e3aa465113a"},{"0020d17e21820a0069ca87049513eca763f08a74b586724441e7d76fc5142bcc327c"}},
26+            },
27+            OutputType::BECH32,
28+            {
29+                {{0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 0, 0}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 0, 1}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 0, 2}},
30+                {{0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 0}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 1}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 2}},
31+            }
32+    );
33     CheckUnparsable("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0;1>/<2;3>)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0;1>/<2;3>)", "pkh(): Multiple multipath key path specifiers found");
34     CheckUnparsable("pkh([deadbeef/<0;1>]xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/0)", "pkh([deadbeef/<0;1>]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/0)", "pkh(): Key path value \'<0;1>\' specifies multipath in a section where multipath is not allowed");
35     CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/6/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*)})", "tr(xpub6B4sSbNr8XFYXqqKB7PeUemqgEaVtCLjgd5Lf2VYtezSHozC7ffCvVNCyu9TCgHntRQdimjV3tHbxmNfocxtuh6saNtZEw91gjXLRhQ3Yar/6/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub6AhFhZJJGt9YB8i85RfrJ8jT3T2FF5EejDCXqXfm1DAczFEXkk8HD3CXTg2TmKM8wTbSnSw3wPg5JuyLitUrpRmkjn2BQXyZnqJx16AGy94/0/0/<3;4>/*)})", "tr(): Multipath subscripts have mismatched lengths");

achow101 commented at 4:02 pm on June 13, 2023:

Added as suggested

in src/rpc/output_script.cpp:278 in 4ead3e653d outdated

276+            if (descs.empty()) {
277                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
278             }
279-
280+            if (descs.size() > 1) {
281+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor specifying both receiving and change derivation paths are not allowed");

darosior commented at 3:33 pm on June 12, 2023:

The error is specific to 2-paths multipaths. But it’s removed in a following commit anyways.

achow101 commented at 4:02 pm on June 13, 2023:

Changed

in src/script/descriptor.cpp:1767 in 98796fccbd outdated

1763+            }
1764+            if (parser.m_keys.size() == 0) {
1765+                // No keys, return single descriptor
1766+                ret.emplace_back(std::make_unique<MiniscriptDescriptor>(std::vector<std::unique_ptr<PubkeyProvider>>(), std::move(node)));
1767+                return ret;
1768+            }

darosior commented at 3:34 pm on June 12, 2023:

This could be a CHECKNONFATAL that it’s never empty, as any sane Miniscript must contain at least one key check.

achow101 commented at 4:02 pm on June 13, 2023:

Done

DrahtBot removed the label CI failed on Jun 12, 2023

in src/script/descriptor.cpp:1423 in 892cd5daf5 outdated

1152@@ -1153,29 +1153,64 @@ enum class ParseScriptContext {
1153  * @param[out] error parsing error message
1154  * @returns false if parsing failed
1155  **/
1156-[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, bool& apostrophe, std::string& error)
1157+[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath = false)

darosior commented at 10:46 am on June 13, 2023:

nit: the docstring could be updated with the new meaning and parameter.

achow101 commented at 4:03 pm on June 13, 2023:

Done

in src/script/descriptor.h:178 in 4ead3e653d outdated

155@@ -156,7 +156,7 @@ struct Descriptor {
156  * If a parse error occurs, or the checksum is missing/invalid, or anything
157  * else is wrong, `nullptr` is returned.
158  */
159-std::unique_ptr<Descriptor> Parse(const std::string& descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum = false);
160+std::vector<std::unique_ptr<Descriptor>> Parse(const std::string& descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum = false);

darosior commented at 10:47 am on June 13, 2023:

nit: update the docstring?

achow101 commented at 4:03 pm on June 13, 2023:

Done

in src/wallet/rpc/backup.cpp:1099 in 216df60fe9 outdated

1118-        }
1119+    for (size_t j = 0; j < parsed_descs.size(); ++j) {
1120+        const auto& parsed_desc = parsed_descs.at(j);
1121+        bool desc_internal = internal.has_value() && internal.value();
1122+        if (parsed_descs.size() == 2) {
1123+            desc_internal = j == 1;

darosior commented at 11:23 am on June 13, 2023:

Hmm. This is fairly specific to multipath descriptors that just contain a <0;1> step? We should at least document this behaviour. But i think i’d even favour not trying to implicitly detect the internal-ness of a multipath descriptor: if a user really wants one of the descriptors they import to be internal they can just “bisect” the multipath descriptor and import one of them by setting internal to true?

(Here and in the next commit.)

achow101 commented at 3:22 pm on June 13, 2023:

Since the main point of this entire proposal is to make it easier to import receiving and change as a single item, I think it makes sense to special case these.

darosior commented at 10:15 am on June 14, 2023:

Then let’s at least document in importdescriptors and importmulti that for any multipath descriptor with 2 paths, the second will be imported as internal?

in src/wallet/rpc/backup.cpp:1116 in 216df60fe9 outdated

1120+        const auto& parsed_desc = parsed_descs.at(j);
1121+        bool desc_internal = internal.has_value() && internal.value();
1122+        if (parsed_descs.size() == 2) {
1123+            desc_internal = j == 1;
1124+        } else if (parsed_descs.size() > 2) {
1125+            desc_internal = false;

darosior commented at 11:29 am on June 13, 2023:

I dont think desc_internal can ever be true here. (Here and in the next commit.)

 0diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
 1index 4bd651e652..fd9e831e0b 100644
 2--- a/src/wallet/rpc/backup.cpp
 3+++ b/src/wallet/rpc/backup.cpp
 4@@ -1113,7 +1113,7 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
 5         if (parsed_descs.size() == 2) {
 6             desc_internal = j == 1;
 7         } else if (parsed_descs.size() > 2) {
 8-            desc_internal = false;
 9+            CHECK_NONFATAL(!desc_internal);
10         }
11         // Expand all descriptors to get public keys and scripts, and private keys if available.
12         for (int i = range_start; i <= range_end; ++i) {

achow101 commented at 4:04 pm on June 13, 2023:

Done

in src/rpc/output_script.cpp:204 in 4ead3e653d outdated

208                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
209             }
210 
211             UniValue result(UniValue::VOBJ);
212-            result.pushKV("descriptor", desc->ToString());
213+            result.pushKV("descriptor", descs.at(0)->ToString());

darosior commented at 12:33 pm on June 13, 2023:

I think it’s surprising we’d return the first descriptor for a multipath. But i don’t have a good suggestion. Returning the string we were passed seems hacky (would have to detect whether there is a checksum, and append one if necessary, or remove privkeys). Maybe just drop the field if this is a multipath?

In any case we should document the new behaviour for multipath descriptors.

achow101 commented at 4:04 pm on June 13, 2023:

Added a sentence in the doc.

in src/script/descriptor.cpp:1439 in 892cd5daf5 outdated

1178+            }
1179+            if (out.size() > 1) {
1180+                error = "Multiple multipath key path specifiers found";
1181+                return false;
1182+            }
1183+            to_process = Split(item.subspan(1, item.size() - 2), ';');

darosior commented at 1:42 pm on June 13, 2023:

You need to check a ; is indeed present, or you may parse a single-path multipath. Confirmed with the following unit test:

 0diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
 1index 3ea8d63ab3..b019ee8b0e 100644
 2--- a/src/test/descriptor_tests.cpp
 3+++ b/src/test/descriptor_tests.cpp
 4@@ -716,6 +716,7 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
 5     CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7;8;9>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7;8;9>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath subscripts have mismatched lengths");
 6     CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4;5>/*)})", "tr(xpub661MyMwAqRbcF3yVrV2KyYetLMYA5mCbv4BhrKwUrFE9LZM6JRR1AEt8Jq4V4C8LwtTke6YEEdCZqgXp85YRk2j74EfJKhe3QybQ9kcUjs4/<6;7>/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4;5>/*)})", "tr(): Multipath internal key mismatches multipath subscripts lengths");
 7     CheckUnparsable("sh(multi(2,xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0/*,xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*))", "sh(multi(2,xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*,xpub661MyMwAqRbcGDZQUKLqmWodYLcoBQnQH33yYkkF3jjxeLvY8qr2wWGEWkiKFaaQfJCoi3HeEq3Dc5DptfbCyjD38fNhSqtKc1UHaP4ba3t/0/0/<3;4>/*))", "multi(): Multipath derivation paths have mismatched lengths");
 8+    CheckUnparsable("wpkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0>/*)", "wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0>/*)", "");
 9 
10     // Multisig constructions
11     Check("multi(1,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "multi(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", SIGNABLE, {{"512103a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd4104a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea23552ae"}}, std::nullopt);

achow101 commented at 4:04 pm on June 13, 2023:

Added a check and the test.

darosior commented at 2:05 pm on June 13, 2023: member

ACK on most of the core logic. Just found a small mistake in the parsing of the multipath step in the derivation path. I also have a couple questions on the modifications to the RPC interface. The rest is nits, feel free to ignore them. Nice set of unit tests!

One almost-nit but important for posterity IMO is that all your commit messages mention the previous iteration of the BIP that only allowed for 2 paths (change and receive) and should be adapted to mention “multiple” paths to avoid confusing a reader.

As i’ve already mentioned in the BIP review last year (https://github.com/bitcoin/bips/pull/1354#discussion_r985316649), restricting it to two paths would remove a lot of complexity without preventing any known usecase. Happy to keep it as it is though, i believe the code is correct.

I’ve compared this to my Rust (for rust-miniscript, see https://github.com/rust-bitcoin/rust-miniscript/pull/470) and Python (https://github.com/darosior/python-bip380/pull/21) implementations, suggesting a couple more unit test cases but without finding any issue.

I also wrote a more efficient descriptor parsing fuzzer that i grew to more branch coverage than the existing descriptor_parse target. I also used a dictionary with the multipath derivation step syntax on both the existing and my new targets. All the multipath parsing code was covered and i didn’t find any crash.

achow101 force-pushed on Jun 13, 2023

DrahtBot added the label CI failed on Jun 13, 2023

darosior commented at 10:17 am on June 14, 2023: member

ACK 15db5e0c1c03a65db4ed5b9a400817bf96237590

CI jobs that are failing seem spurious (“Failed to start an instance! Failed to pull null image! Repository does not exist or may require authentication.”), or #27879.

darosior commented at 3:02 pm on June 26, 2023: member

This needs rebase for CI to pass.

achow101 force-pushed on Jun 28, 2023

maflcko commented at 6:46 am on June 29, 2023: member

CI fuzz:

0terminate called after throwing an instance of 'NonFatalCheckError'
1  what():  Internal bug detected: "parser.m_keys.size() != 0"
2script/descriptor.cpp:1808 (ParseScript)
3Bitcoin Core v25.99.0-501b80414422
4Please report this issue here: https://github.com/bitcoin/bitcoin/issues
5==16697== ERROR: libFuzzer: deadly signal

darosior commented at 8:12 am on June 29, 2023: member

The descriptor on which it crashed is wsh(0). This is a bug in the Miniscript logic, exposed by the new check but not introduced in this PR. A 0 script should not be considered sane.

EDIT: fixed in #27997.

darosior commented at 12:49 pm on July 1, 2023: member

I think the CHECK_NONFATAL that we don’t parse Miniscript descriptors with an empty set of keys can be removed here, No need to hold off this PR and it was added in #27997 which fixes the bug it uncovered.

DrahtBot commented at 3:37 pm on July 6, 2023: contributor

Needs rebase if still relevant, due to silent merge conflict?

achow101 force-pushed on Jul 6, 2023

achow101 referenced this in commit bc88f3ab90 on Jul 17, 2023

DrahtBot added the label Needs rebase on Jul 18, 2023

achow101 force-pushed on Jul 18, 2023

DrahtBot removed the label Needs rebase on Jul 18, 2023

DrahtBot removed the label CI failed on Jul 18, 2023

sidhujag referenced this in commit 036a5dfa46 on Jul 19, 2023

darosior commented at 8:37 am on July 21, 2023: member

re-ACK 33a2f6dc3c2512129df7882586acb3b3ddce62cb

DrahtBot added the label Needs rebase on Jul 27, 2023

achow101 force-pushed on Jul 27, 2023

DrahtBot removed the label Needs rebase on Jul 27, 2023

darosior commented at 10:35 pm on July 31, 2023: member

re-ACK 8f6eeaab38

fanquake requested review from furszy on Aug 1, 2023

fanquake requested review from Sjors on Aug 1, 2023

darosior commented at 8:20 am on August 31, 2023: member

Happy 2 years birthday, #22838! :birthday:

DrahtBot requested review from mjdietzx on Aug 31, 2023

DrahtBot added the label Needs rebase on Sep 6, 2023

achow101 force-pushed on Sep 7, 2023

DrahtBot removed the label Needs rebase on Sep 7, 2023

achow101 force-pushed on Sep 7, 2023

darosior changes_requested

darosior commented at 4:18 pm on September 9, 2023: member

The fuzz target is crashing because you updated is_solvable in TestDescriptor to be an optional but didn’t update !is_solvable to !*is_solvable.

 0diff --git a/src/test/fuzz/descriptor_parse.cpp b/src/test/fuzz/descriptor_parse.cpp
 1index 2dd55fda84..4a2ad65fd0 100644
 2--- a/src/test/fuzz/descriptor_parse.cpp
 3+++ b/src/test/fuzz/descriptor_parse.cpp
 4@@ -151,7 +151,7 @@ static void TestDescriptor(const Descriptor& desc, FlatSigningProvider& sig_prov
 5     const auto max_sat_nonmaxsig{desc.MaxSatisfactionWeight(true)};
 6     const auto max_elems{desc.MaxSatisfactionElems()};
 7     // We must be able to estimate the max satisfaction size for any solvable descriptor (but combo).
 8-    const bool is_nontop_or_nonsolvable{!is_solvable || !desc.GetOutputType()};
 9+    const bool is_nontop_or_nonsolvable{!*is_solvable || !desc.GetOutputType()};
10     const bool is_input_size_info_set{max_sat_maxsig && max_sat_nonmaxsig && max_elems};
11     assert(is_input_size_info_set || is_nontop_or_nonsolvable);
12 }

darosior commented at 4:20 pm on September 9, 2023: member

Successfully tested getdescriptorinfo, deriveaddresses and importdescriptors against singlesig and multisig multipath descriptors generated by Sparrow. Wallet balances, addresses and checksums all match. Sparrow uses multipath descriptors since Nov 2021. @craigraw would you mind testing against the latest state of this PR to help move things forward here?

craigraw commented at 12:33 pm on September 10, 2023: none

Successfully tested getdescriptorinfo, deriveaddresses and importdescriptors against singlesig and multisig multipath descriptors generated by Sparrow. Wallet balances, addresses and checksums all match.

Tested the above again against the current state of the PR, no issues found.

DrahtBot added the label CI failed on Sep 10, 2023

achow101 commented at 4:01 pm on September 11, 2023: member

Fixed the fuzz crash

achow101 force-pushed on Sep 11, 2023

darosior commented at 4:09 pm on September 11, 2023: member

re-ACK ba32818d09310106094127b64cc2c249c7f723c0

DrahtBot removed review request from mjdietzx on Sep 11, 2023

DrahtBot requested review from mjdietzx on Sep 11, 2023

DrahtBot removed the label CI failed on Sep 11, 2023

achow101 force-pushed on Sep 19, 2023

furszy commented at 9:25 am on September 20, 2023: member

Added to my review queue. Will come here after finishing with #26728.

DrahtBot added the label CI failed on Oct 5, 2023

DrahtBot added the label Needs rebase on Oct 8, 2023

achow101 force-pushed on Oct 9, 2023

DrahtBot removed the label Needs rebase on Oct 9, 2023

DrahtBot removed the label CI failed on Oct 9, 2023

DrahtBot added the label Needs rebase on Oct 11, 2023

achow101 force-pushed on Oct 11, 2023

DrahtBot removed the label Needs rebase on Oct 11, 2023

DrahtBot added the label Needs rebase on Nov 6, 2023

achow101 force-pushed on Nov 13, 2023

DrahtBot removed the label Needs rebase on Nov 13, 2023

DrahtBot added the label CI failed on Nov 13, 2023

DrahtBot commented at 11:41 am on November 16, 2023: contributor

0wallet/rpc/backup.cpp:999:25: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
1  999 |         ordered_pubkeys.push_back({pubkey.GetID(), internal});
2      |                         ^~~~~~~~~~~                        ~
3      |                         emplace_back(
4wallet/rpc/backup.cpp:1125:33: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
5 1125 |                 ordered_pubkeys.push_back({key_pair.first, desc_internal});
6      |                                 ^~~~~~~~~~~                             ~
7      |                                 emplace_back(

achow101 force-pushed on Nov 16, 2023

DrahtBot removed the label CI failed on Nov 16, 2023

DrahtBot added the label CI failed on Nov 27, 2023

achow101 force-pushed on Nov 27, 2023

achow101 force-pushed on Dec 7, 2023

DrahtBot removed the label CI failed on Dec 7, 2023

DrahtBot added the label Needs rebase on Dec 21, 2023

achow101 force-pushed on Dec 21, 2023

DrahtBot removed the label Needs rebase on Dec 21, 2023

DrahtBot added the label CI failed on Jan 14, 2024

darosior commented at 3:49 pm on February 2, 2024: member

If i re-review this would anyone else review as well? ACKed it multiple times in the past but it didn’t get merged because we’d need another person.

It’s been a while, i need to review it from scratch so i’m trying to probe if that’s worth spending the time.

achow101 commented at 5:00 pm on February 2, 2024: member

Rebasing for CI

achow101 force-pushed on Feb 2, 2024

DrahtBot removed the label CI failed on Feb 2, 2024

DrahtBot added the label CI failed on Feb 23, 2024

DrahtBot commented at 5:53 pm on February 23, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/21162434173

DrahtBot added the label Needs rebase on Mar 29, 2024

achow101 force-pushed on Mar 29, 2024

DrahtBot removed the label Needs rebase on Mar 29, 2024

DrahtBot removed the label CI failed on Mar 29, 2024

DrahtBot added the label Needs rebase on Apr 8, 2024

achow101 requested review from darosior on Apr 9, 2024

achow101 removed review request from mjdietzx on Apr 9, 2024

achow101 requested review from laanwj on Apr 9, 2024

achow101 force-pushed on Apr 25, 2024

DrahtBot removed the label Needs rebase on Apr 25, 2024

DrahtBot added the label Needs rebase on Jun 13, 2024

descriptors: Add PubkeyProvider::Clone 8da4f97404

descriptors: Add DescriptorImpl::Clone ed00e60c8e

descriptors: Change ParseScript to return vector of descriptors

To prepare for returning multipath descriptors which will be a shorthand
for specifying multiple descriptors, change ParseScript's signature to return a
vector.

06caba093a

achow101 force-pushed on Jun 13, 2024

descriptors: Have ParseKeypath handle multipath specifiers

Multipath specifiers are derivation path indexes of the form `<i;j;k;...>`
used for specifying multiple derivation paths for a descriptor.
Only one multipath specifier is allowed per PubkeyProvider.
This is syntactic sugar which is parsed into multiple distinct descriptors.
One descriptor will have all of the `i` paths, the second all of the `j` paths,
the third all of the `k` paths, and so on.

ParseKeypath will always return a vector of keypaths with the same size
as the multipath specifier. The callers of this function are updated to deal
with this case and return multiple PubkeyProviders. Their callers have
also been updated to handle vectors of PubkeyProviders.

912ec4fb3e

descriptors: Change Parse to return vector of descriptors

When given a descriptor which contins a multipath derivation specifier,
a vector of descriptors will be returned.

47b1711afc

tests: Add unit tests for multipath descriptors c6cfb6c30b

tests: Multipath descriptors for getdescriptorinfo 4a6087d516

rpc: Have deriveaddresses derive receiving and change

When given a multipath descriptor, derive all of the descriptors.
The derived addresses will be returned in an object
consisting of multiple arrays. For compatibility, when given a single path
descriptor, the addresses are provided in a single array as before.

3ab0ef79a9

tests: Multipath descriptors for scantxoutset and deriveaddresses 000652fb2b

wallet: Move internal to be per key when importing

Instead of applying internal-ness to all keys being imported at the same
time, apply it on a per key basis. So each key that is imported will
carry with it whether it is for the change keypool.

c1e544a03e

rpc: Allow importmulti to import multipath descriptors correctly

Multipath descriptors will be imported as multiple separate descriptors.
When there are exactly 2 multipath items, the first descriptor will be
for receiving addreses, and the second for change
addresses. When importing a multipath descriptor, 'internal' cannot be
specified.

ca8ab095a1

wallet, rpc: Allow importdescriptors to import multipath descriptors

Multipath descriptors will be imported as multiple separate descriptors.
When there are 2 multipath items, the first descriptor will be for receiving
addresses and the second for change. This mirrors importmulti.

dee18eaefd

tests: Test importing of multipath descriptors

Test that both importmulti and importdescriptors behave as expected when
importing a multipath descriptor.

54c1bd8d0f

doc: Mention multipath specifier cb41e26eef

achow101 force-pushed on Jun 13, 2024

DrahtBot removed the label Needs rebase on Jun 13, 2024

in src/script/descriptor.cpp:794 in ed00e60c8e outdated

786@@ -786,6 +787,11 @@ class DescriptorImpl : public Descriptor
787             arg->GetPubKeys(pubkeys, ext_pubs);
788         }
789     }
790+||||||| parent of 657176b834d (descriptors: Add DescriptorImpl::Clone)
791+=======
792+
793+    virtual std::unique_ptr<DescriptorImpl> Clone() const = 0;
794+>>>>>>> 657176b834d (descriptors: Add DescriptorImpl::Clone)

furszy commented at 10:46 pm on June 19, 2024:

Rebase issues in ed00e60c8e8 that are removed on 06caba093a.

mjdietzx commented at 4:42 pm on June 27, 2024: contributor

Tested ACK cb41e26eef3a92a7704a0e01eb9b272f7ed78eeb. Note: I did not do a thorough code review

What I’ve tested:

getdescriptorinfo, importdescriptors, listdescriptors with a miniscript wallet similar to #29156. All works as expected
import this wallet from file to Coldcard Mk4 (Edge FW 6.2.2 w/ miniscript support)
- <0;1> notation works properly as Coldcard has already adopted this convention
- Coldcard also signs psbt properly
I have two PRs which would benefit from this improvement. I rebased #29156 on top of this. This diff simplifies and passes:

 0diff --git a/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py b/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py
 1index 4f50ae191c..e5f58eb4d2 100755
 2--- a/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py
 3+++ b/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py
 4@@ -32,35 +32,26 @@ class WalletMiniscriptDecayingMultisigDescriptorPSBTTest(BitcoinTestFramework):
 5         self.skip_if_no_sqlite() [@staticmethod](/bitcoin-bitcoin/contributor/staticmethod/)
 6-    def _get_xpub(wallet, internal):
 7+    def _get_xpub(wallet):
 8         """Extract the wallet's xpubs using `listdescriptors` and pick the one from the `pkh` descriptor since it's least likely to be accidentally reused (legacy addresses)."""
 9-        pkh_descriptor = next(filter(lambda d: d["desc"].startswith("pkh(") and d["internal"] == internal, wallet.listdescriptors()["descriptors"]))
10+        pkh_descriptor = next(filter(lambda d: d["desc"].startswith("pkh("), wallet.listdescriptors()["descriptors"]))
11         # keep all key origin information (master key fingerprint and all derivation steps) for proper support of hardware devices
12         # see section 'Key origin identification' in 'doc/descriptors.md' for more details...
13         return pkh_descriptor["desc"].split("pkh(")[1].split(")")[0]
14 
15-    def create_multisig(self, external_xpubs, internal_xpubs):
16+    def create_multisig(self, xpubs):
17         """The multisig is created by importing the following descriptors. The resulting wallet is watch-only and every signer can do this."""
18         self.node.createwallet(wallet_name=f"{self.name}", blank=True, descriptors=True, disable_private_keys=True)
19         multisig = self.node.get_wallet_rpc(f"{self.name}")
20         # spending policy: `thresh(4,pk(key_1),pk(key_2),pk(key_3),pk(key_4),after(t1),after(t2),after(t3))`
21         # IMPORTANT: when backing up your descriptor, the order of key_1...key_4 must be correct!
22-        external = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(external_xpubs)}),sln:after({f'),sln:after('.join(map(str, self.locktimes))})))")
23-        internal = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(internal_xpubs)}),sln:after({f'),sln:after('.join(map(str, self.locktimes))})))")
24-        result = multisig.importdescriptors([
25-            {  # receiving addresses (internal: False)
26-                "desc": external["descriptor"],
27-                "active": True,
28-                "internal": False,
29-                "timestamp": "now",
30-            },
31-            {  # change addresses (internal: True)
32-                "desc": internal["descriptor"],
33-                "active": True,
34-                "internal": True,
35-                "timestamp": "now",
36-            },
37-        ])
38+        descriptors = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(xpubs)}),sln:after({f'),sln:after('.join(map(str, self.locktimes))})))")
39+        result = multisig.importdescriptors([{
40+            "desc": desc,
41+            "active": True,
42+            "internal": "/1/*" in desc,
43+            "timestamp": "now",
44+        } for desc in descriptors["multipath_expansion"]])
45         assert all(r["success"] for r in result)
46         return multisig
47 
48@@ -77,10 +68,10 @@ class WalletMiniscriptDecayingMultisigDescriptorPSBTTest(BitcoinTestFramework):
49 
50         self.log.info("Create the signer wallets and get their xpubs...")
51         signers = [self.node.get_wallet_rpc(self.node.createwallet(wallet_name=f"signer_{i}", descriptors=True)["name"]) for i in range(self.N)]
52-        external_xpubs, internal_xpubs = [[self._get_xpub(signer, internal) for signer in signers] for internal in [False, True]]
53+        xpubs = [self._get_xpub(signer).replace("/0/*", "/<0;1>/*") for signer in signers]
54 
55         self.log.info("Create the watch-only decaying multisig using signers' xpubs...")
56-        multisig = self.create_multisig(external_xpubs, internal_xpubs)
57+        multisig = self.create_multisig(xpubs)
58 
59         self.log.info("Get a mature utxo to send to the multisig...")
60         coordinator_wallet = self.node.get_wallet_rpc(self.node.createwallet(wallet_name=f"coordinator", descriptors=True)["name"])

darosior commented at 10:30 am on June 28, 2024: member

Will re-review from scratch next week to finally get this over the finish line.

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

Code Coverage

Reviews

Conflicts