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

pull achow101 wants to merge 14 commits into bitcoin:master from achow101:multipath-descs changing 30 files +1242 −362
  1. 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

  2. DrahtBot added the label Descriptors on Aug 30, 2021
  3. DrahtBot added the label Mining on Aug 30, 2021
  4. DrahtBot added the label RPC/REST/ZMQ on Aug 30, 2021
  5. DrahtBot added the label Wallet on Aug 30, 2021
  6. 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 furszy, darosior, glozow, mjdietzx, pythcoiner
    Concept ACK craigraw
    Stale ACK Rspigler

    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)
    • #25979 ([WIP] wallet: standardize change output detection process by furszy)

    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.

  7. 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.

  8. 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)
  9. 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).

  10. 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.

  11. 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);
  12. achow101 commented at 8:58 pm on October 18, 2021: member
    Fixed the silent merge conflict.
  13. achow101 force-pushed on Oct 18, 2021
  14. 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

  15. 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?

  16. 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

  17. 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}
  18. 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.

  19. 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).

  20. 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
  21. 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
  22. achow101 force-pushed on Oct 19, 2021
  23. 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
  24. 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
  25. 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
  26. 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.
  27. 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?

  28. achow101 force-pushed on Oct 24, 2021
  29. achow101 commented at 10:23 pm on October 24, 2021: member
    Rebased again
  30. mjdietzx commented at 11:41 pm on October 24, 2021: contributor
    reACK a6ff1c7
  31. maflcko removed the label Mining on Oct 25, 2021
  32. prusnak referenced this in commit 7088ca639e on Nov 2, 2021
  33. prusnak referenced this in commit 2755f4a7a9 on Nov 2, 2021
  34. prusnak referenced this in commit 21521d45aa on Nov 2, 2021
  35. prusnak referenced this in commit 5dc60b35db on Nov 2, 2021
  36. DrahtBot added the label Needs rebase on Nov 25, 2021
  37. achow101 force-pushed on Dec 5, 2021
  38. DrahtBot removed the label Needs rebase on Dec 5, 2021
  39. DrahtBot added the label Needs rebase on Dec 8, 2021
  40. achow101 force-pushed on Dec 8, 2021
  41. DrahtBot removed the label Needs rebase on Dec 8, 2021
  42. 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
  43. DrahtBot added the label Needs rebase on Dec 10, 2021
  44. achow101 force-pushed on Dec 10, 2021
  45. DrahtBot removed the label Needs rebase on Dec 10, 2021
  46. DrahtBot added the label Needs rebase on Dec 14, 2021
  47. achow101 force-pushed on Dec 14, 2021
  48. DrahtBot removed the label Needs rebase on Dec 14, 2021
  49. achow101 force-pushed on Dec 14, 2021
  50. 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?
  51. DrahtBot added the label Needs rebase on Dec 26, 2021
  52. achow101 force-pushed on Dec 26, 2021
  53. DrahtBot removed the label Needs rebase on Dec 26, 2021
  54. DrahtBot added the label Needs rebase on Dec 31, 2021
  55. achow101 force-pushed on Jan 10, 2022
  56. DrahtBot removed the label Needs rebase on Jan 10, 2022
  57. DrahtBot added the label Needs rebase on Jan 20, 2022
  58. achow101 force-pushed on Jan 24, 2022
  59. DrahtBot removed the label Needs rebase on Jan 24, 2022
  60. DrahtBot added the label Needs rebase on Feb 21, 2022
  61. achow101 force-pushed on Feb 22, 2022
  62. DrahtBot removed the label Needs rebase on Feb 22, 2022
  63. DrahtBot added the label Needs rebase on Mar 4, 2022
  64. achow101 force-pushed on Mar 4, 2022
  65. DrahtBot removed the label Needs rebase on Mar 4, 2022
  66. DrahtBot added the label Needs rebase on Mar 23, 2022
  67. achow101 force-pushed on Mar 24, 2022
  68. DrahtBot removed the label Needs rebase on Mar 24, 2022
  69. achow101 force-pushed on Apr 16, 2022
  70. DrahtBot added the label Needs rebase on May 4, 2022
  71. 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.
  72. achow101 force-pushed on May 4, 2022
  73. DrahtBot removed the label Needs rebase on May 4, 2022
  74. 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.

  75. wolfmcnally referenced this in commit 318584aa5b on Jun 26, 2022
  76. 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

  77. DrahtBot added the label Needs rebase on Jul 14, 2022
  78. achow101 force-pushed on Jul 15, 2022
  79. DrahtBot removed the label Needs rebase on Jul 15, 2022
  80. achow101 force-pushed on Jul 28, 2022
  81. achow101 force-pushed on Jul 29, 2022
  82. achow101 force-pushed on Jul 29, 2022
  83. 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.
  84. DrahtBot added the label Needs rebase on Aug 17, 2022
  85. achow101 force-pushed on Aug 18, 2022
  86. achow101 force-pushed on Aug 18, 2022
  87. achow101 force-pushed on Aug 18, 2022
  88. DrahtBot removed the label Needs rebase on Aug 18, 2022
  89. DrahtBot added the label Needs rebase on Aug 18, 2022
  90. achow101 force-pushed on Aug 18, 2022
  91. achow101 force-pushed on Aug 18, 2022
  92. DrahtBot removed the label Needs rebase on Aug 19, 2022
  93. 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
  94. DrahtBot added the label Needs rebase on Aug 24, 2022
  95. achow101 force-pushed on Aug 24, 2022
  96. DrahtBot removed the label Needs rebase on Aug 24, 2022
  97. achow101 commented at 10:51 pm on August 24, 2022: member
    It’s unclear to me what the fuzzer is OOM’ing on.
  98. achow101 force-pushed on Aug 30, 2022
  99. achow101 commented at 9:59 pm on August 30, 2022: member
    I think I fixed the fuzzer issue.
  100. DrahtBot added the label Needs rebase on Sep 1, 2022
  101. achow101 force-pushed on Sep 1, 2022
  102. achow101 force-pushed on Sep 1, 2022
  103. DrahtBot removed the label Needs rebase on Sep 1, 2022
  104. DrahtBot added the label Needs rebase on Sep 20, 2022
  105. achow101 force-pushed on Sep 20, 2022
  106. DrahtBot removed the label Needs rebase on Sep 20, 2022
  107. DrahtBot added the label Needs rebase on Oct 26, 2022
  108. achow101 force-pushed on Nov 4, 2022
  109. DrahtBot removed the label Needs rebase on Nov 5, 2022
  110. darosior commented at 11:55 pm on November 24, 2022: member
    Concept ACK.
  111. DrahtBot added the label Needs rebase on Dec 6, 2022
  112. achow101 force-pushed on Dec 6, 2022
  113. DrahtBot removed the label Needs rebase on Dec 6, 2022
  114. DrahtBot added the label Needs rebase on Jan 10, 2023
  115. achow101 force-pushed on Jan 10, 2023
  116. DrahtBot removed the label Needs rebase on Jan 11, 2023
  117. DrahtBot added the label Needs rebase on Jan 16, 2023
  118. achow101 force-pushed on Jan 16, 2023
  119. DrahtBot removed the label Needs rebase on Jan 16, 2023
  120. DrahtBot added the label Needs rebase on Feb 16, 2023
  121. achow101 force-pushed on Feb 16, 2023
  122. DrahtBot removed the label Needs rebase on Feb 16, 2023
  123. DrahtBot added the label Needs rebase on Feb 21, 2023
  124. achow101 force-pushed on Mar 1, 2023
  125. DrahtBot removed the label Needs rebase on Mar 1, 2023
  126. DrahtBot added the label Needs rebase on Mar 16, 2023
  127. achow101 force-pushed on Mar 17, 2023
  128. DrahtBot removed the label Needs rebase on Mar 17, 2023
  129. DrahtBot added the label Needs rebase on Mar 23, 2023
  130. achow101 force-pushed on Apr 10, 2023
  131. DrahtBot removed the label Needs rebase on Apr 10, 2023
  132. achow101 force-pushed on Apr 10, 2023
  133. DrahtBot added the label Needs rebase on Apr 11, 2023
  134. achow101 force-pushed on Apr 11, 2023
  135. DrahtBot removed the label Needs rebase on Apr 11, 2023
  136. DrahtBot added the label Needs rebase on May 8, 2023
  137. achow101 force-pushed on May 10, 2023
  138. DrahtBot removed the label Needs rebase on May 10, 2023
  139. DrahtBot added the label CI failed on May 10, 2023
  140. achow101 force-pushed on May 11, 2023
  141. DrahtBot removed the label CI failed on May 11, 2023
  142. DrahtBot added the label Needs rebase on May 22, 2023
  143. achow101 force-pushed on May 22, 2023
  144. DrahtBot removed the label Needs rebase on May 22, 2023
  145. 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
  146. 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
  147. 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
  148. DrahtBot added the label CI failed on Jun 9, 2023
  149. 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
  150. 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
  151. in src/test/descriptor_tests.cpp:824 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
  152. 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
  153. 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
  154. 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
  155. DrahtBot removed the label CI failed on Jun 12, 2023
  156. 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
  157. 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
  158. 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?
    darosior commented at 7:54 am on July 11, 2024:
    I still think this behaviour should be documented in the help of importmulti and importdescriptors.
    achow101 commented at 3:29 pm on July 16, 2024:
    Added a help text.
  159. 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
  160. 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.
  161. 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.
  162. 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.

  163. achow101 force-pushed on Jun 13, 2023
  164. achow101 force-pushed on Jun 13, 2023
  165. DrahtBot added the label CI failed on Jun 13, 2023
  166. 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.

  167. darosior commented at 3:02 pm on June 26, 2023: member
    This needs rebase for CI to pass.
  168. achow101 force-pushed on Jun 28, 2023
  169. 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
  170. 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.

  171. 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.
  172. DrahtBot commented at 3:37 pm on July 6, 2023: contributor
    Needs rebase if still relevant, due to silent merge conflict?
  173. achow101 force-pushed on Jul 6, 2023
  174. achow101 referenced this in commit bc88f3ab90 on Jul 17, 2023
  175. DrahtBot added the label Needs rebase on Jul 18, 2023
  176. achow101 force-pushed on Jul 18, 2023
  177. DrahtBot removed the label Needs rebase on Jul 18, 2023
  178. DrahtBot removed the label CI failed on Jul 18, 2023
  179. sidhujag referenced this in commit 036a5dfa46 on Jul 19, 2023
  180. darosior commented at 8:37 am on July 21, 2023: member
    re-ACK 33a2f6dc3c2512129df7882586acb3b3ddce62cb
  181. DrahtBot added the label Needs rebase on Jul 27, 2023
  182. achow101 force-pushed on Jul 27, 2023
  183. DrahtBot removed the label Needs rebase on Jul 27, 2023
  184. darosior commented at 10:35 pm on July 31, 2023: member
    re-ACK 8f6eeaab38
  185. fanquake requested review from furszy on Aug 1, 2023
  186. fanquake requested review from Sjors on Aug 1, 2023
  187. darosior commented at 8:20 am on August 31, 2023: member
    Happy 2 years birthday, #22838! :birthday:
  188. DrahtBot requested review from mjdietzx on Aug 31, 2023
  189. DrahtBot added the label Needs rebase on Sep 6, 2023
  190. achow101 force-pushed on Sep 7, 2023
  191. DrahtBot removed the label Needs rebase on Sep 7, 2023
  192. achow101 force-pushed on Sep 7, 2023
  193. darosior changes_requested
  194. 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 }
  195. 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?

  196. 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.

  197. DrahtBot added the label CI failed on Sep 10, 2023
  198. achow101 commented at 4:01 pm on September 11, 2023: member
    Fixed the fuzz crash
  199. achow101 force-pushed on Sep 11, 2023
  200. darosior commented at 4:09 pm on September 11, 2023: member
    re-ACK ba32818d09310106094127b64cc2c249c7f723c0
  201. DrahtBot removed review request from mjdietzx on Sep 11, 2023
  202. DrahtBot requested review from mjdietzx on Sep 11, 2023
  203. DrahtBot removed the label CI failed on Sep 11, 2023
  204. achow101 force-pushed on Sep 19, 2023
  205. furszy commented at 9:25 am on September 20, 2023: member
    Added to my review queue. Will come here after finishing with #26728.
  206. DrahtBot added the label CI failed on Oct 5, 2023
  207. DrahtBot added the label Needs rebase on Oct 8, 2023
  208. achow101 force-pushed on Oct 9, 2023
  209. DrahtBot removed the label Needs rebase on Oct 9, 2023
  210. DrahtBot removed the label CI failed on Oct 9, 2023
  211. DrahtBot added the label Needs rebase on Oct 11, 2023
  212. achow101 force-pushed on Oct 11, 2023
  213. DrahtBot removed the label Needs rebase on Oct 11, 2023
  214. DrahtBot added the label Needs rebase on Nov 6, 2023
  215. achow101 force-pushed on Nov 13, 2023
  216. DrahtBot removed the label Needs rebase on Nov 13, 2023
  217. DrahtBot added the label CI failed on Nov 13, 2023
  218. 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(
  219. achow101 force-pushed on Nov 16, 2023
  220. DrahtBot removed the label CI failed on Nov 16, 2023
  221. DrahtBot added the label CI failed on Nov 27, 2023
  222. achow101 force-pushed on Nov 27, 2023
  223. achow101 force-pushed on Dec 7, 2023
  224. DrahtBot removed the label CI failed on Dec 7, 2023
  225. DrahtBot added the label Needs rebase on Dec 21, 2023
  226. achow101 force-pushed on Dec 21, 2023
  227. DrahtBot removed the label Needs rebase on Dec 21, 2023
  228. DrahtBot added the label CI failed on Jan 14, 2024
  229. 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.

  230. achow101 commented at 5:00 pm on February 2, 2024: member
    Rebasing for CI
  231. achow101 force-pushed on Feb 2, 2024
  232. DrahtBot removed the label CI failed on Feb 2, 2024
  233. DrahtBot added the label CI failed on Feb 23, 2024
  234. 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

  235. DrahtBot added the label Needs rebase on Mar 29, 2024
  236. achow101 force-pushed on Mar 29, 2024
  237. DrahtBot removed the label Needs rebase on Mar 29, 2024
  238. DrahtBot removed the label CI failed on Mar 29, 2024
  239. DrahtBot added the label Needs rebase on Apr 8, 2024
  240. achow101 requested review from darosior on Apr 9, 2024
  241. achow101 removed review request from mjdietzx on Apr 9, 2024
  242. achow101 requested review from laanwj on Apr 9, 2024
  243. achow101 force-pushed on Apr 25, 2024
  244. DrahtBot removed the label Needs rebase on Apr 25, 2024
  245. DrahtBot added the label Needs rebase on Jun 13, 2024
  246. achow101 force-pushed on Jun 13, 2024
  247. achow101 force-pushed on Jun 13, 2024
  248. DrahtBot removed the label Needs rebase on Jun 13, 2024
  249. 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.
    achow101 commented at 7:12 pm on July 9, 2024:
    Fixed
  250. 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"])
  251. 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.
  252. pythcoiner commented at 4:12 am on July 8, 2024: none

    I’m not sure if it’s the intended behavior, but passing the same NUM twice to importdescriptors does not fail and ends up with a single change descriptor:

     0self.log.info("Multipath descriptors: duplicate NUM")
 1self.nodes[1].createwallet(wallet_name="multipath3", descriptors=True, blank=True)
 2w_multipath = self.nodes[1].get_wallet_rpc("multipath3")
 3self.test_importdesc({"desc": descsum_create(f"wpkh({xpriv}/<1;1>/*)"),
 4                      "active": True,
 5                      "range": 10,
 6                      "timestamp": "now"},
 7                      success=True,
 8                      wallet=w_multipath)
 9result = w_multipath.listdescriptors()
10self.log.info(f"{result=}")
11assert_equal(len(result["descriptors"]), 1)
12
13assert_equal(result["descriptors"][0]["desc"],"wpkh(tpubD6NzVbkrYhZ4YNXVQbNhMK1WqguFsUXceaVJKbmno2aZ3B6QfbMeraaYvnBSGpV3vxLyTTK9DYT1yoEck4XUScMzXoQ2U2oSmE2JyMedq3H/1/*)#hf9h03qd" )
14assert_equal(result["descriptors"][0]["internal"], True)

    Edit: is there any reason to accept duplictaes NUM in multipath?

  253. achow101 force-pushed on Jul 9, 2024
  254. achow101 commented at 7:13 pm on July 9, 2024: member

    I’m not sure if it’s the intended behavior, but passing the same NUM twice to importdescriptors does not fail and ends up with a single change descriptor:

    Good catch.

    The BIP does not specify either way yet, but I don’t think there’s any reason to allow duplicates here, so I’ve implemented that and added a test.

  255. pythcoiner commented at 6:06 am on July 10, 2024: none

    note: overlapping multipath are allowed

    0        self.test_importdesc({"desc": descsum_create(f"wsh(and_v(v:pk({xpriv}/<0;1>/*),or_d(pk({xpriv}/<1;2>/*),older(1000))))"),
1                              "active": True,
2                              "range": 10,
3                              "timestamp": "now"},
4                              success=True,
5                              wallet=w_multipath)

    but I don’t see how it can be harmfull as it ends up in valid miniscript descriptor

  256. pythcoiner commented at 6:43 am on July 10, 2024: none

    importing a descriptor containing a multipath w/ "internal": false fail because checks here and here are against if the key exist, not against its value.

    0~/cpp/bitcoin/src pr22838 *1 ?3 ❯ bitcoin-cli -regtest -rpcwallet=liana_recovery importdescriptors "[{\"desc\": \"wsh(or_d(pk([8d757e4c/48'/1'/0'/2']tpubDEmyHnnFiU6hyYrKsnJjfYmeqabZjG69mh1fosrYjf8XFaeppCkLKuc6JefqyZKHxNtgZwJ5KuJ51yLLSt66zc1iyFnL9J8gpN7kSfLvnvG/<0;1>/*),and_v(v:pkh([8d757e4c/48'/1'/0'/2']tpubDEmyHnnFiU6hyYrKsnJjfYmeqabZjG69mh1fosrYjf8XFaeppCkLKuc6JefqyZKHxNtgZwJ5KuJ51yLLSt66zc1iyFnL9J8gpN7kSfLvnvG/<2;3>/*),older(65535))))#87grqs3t\", \"range\": [0,10000], \"timestamp\": \"now\", \"active\": true, \"internal\": false}]"
1[
2  {
3    "success": false,
4    "error": {
5      "code": -5,
6      "message": "Cannot have multipath descriptor while also specifying 'internal'"
7    }
8  }
9]
  257. in src/script/descriptor.cpp:1467 in fa53d191a0 outdated 
    1472+            } else if (p > 0x7FFFFFFFUL) {
1473+                error = strprintf("Key path value %u is out of range", p);
1474+                return false;
1475+            }
1476+            p |= ((uint32_t)hardened) << 31;
1477+            if (std::find(multipath_elems.begin(), multipath_elems.end(), p) != multipath_elems.end()) {
    darosior commented at 10:49 am on July 10, 2024:

    nit: not dramatic but could have used a set to avoid O(n^2) complexity.

    In any case we’ll probably have to put some bounds on the number of multipaths steps in the fuzzer to avoid timeouts (as in #30197) as there is a few operations quadratic in the number of steps. Of course, not in this PR.

    achow101 commented at 2:25 pm on July 11, 2024:
    We need the vector since order has to be preserved, and since the number of elements should be small, I don’t think the performance impact is significant. Also, std::find is O(n).
    darosior commented at 4:33 pm on July 11, 2024:
    It’s O(n) but it’s called in a loop over n.. So it effectively does (n^2 - n)/2 operations, which is O(n^2).
  258. darosior approved
  259. darosior commented at 8:10 am on July 11, 2024: member
    ACK 00337ef0e115f8bd8c1ede425f21ae7a1b6d30de
  260. DrahtBot requested review from mjdietzx on Jul 11, 2024
  261. pythcoiner commented at 12:36 pm on July 12, 2024: none
    ACK 00337ef I’ve written some tests and tested import descriptors produced by liana wallet
  262. achow101 force-pushed on Jul 16, 2024
  263. darosior commented at 10:05 am on July 22, 2024: member
    ACK a512245e217199208807214b09bdb256362d08af
  264. pythcoiner commented at 1:19 pm on July 22, 2024: none
    ACK a512245
  265. darosior commented at 3:53 pm on July 27, 2024: member
    I’d really like to get this in before the feature freeze for v28. I think it’s in good shape and it’s regaining momentum. @mjdietzx would you be able to go over the diff since your last review soon? It didn’t change much since your ACK. @furszy are you still interested in reviewing this work?
  266. in test/functional/rpc_getdescriptorinfo.py:30 in a512245e21 outdated 
    26-        assert_equal(info['descriptor'], descsum_create(desc))
27+        if expanded_descs is not None:
28+            assert_equal(info["descriptor"], descsum_create(expanded_descs[0]))
29+            assert_equal(info["multipath_expansion"], [descsum_create(x) for x in expanded_descs])
30+        else:
31+            assert "multipath_expansion" not in info
    mjdietzx commented at 8:49 pm on July 27, 2024:
    assert_equal(info['descriptor'], descsum_create(desc)) was lost here and needs to be added back in else condition
    achow101 commented at 4:49 pm on August 5, 2024:
    Good catch, added it back in.
  267. mjdietzx approved
  268. mjdietzx commented at 10:15 pm on July 27, 2024: contributor

    ACK a512245e217199208807214b09bdb256362d08af

    Great job and thank you for implementing this!

  269. furszy commented at 2:04 pm on July 29, 2024: member

    @furszy are you still interested in reviewing this work?

    Yes. Starting with it. Thanks for the ping.

  270. glozow added this to the milestone 28.0 on Aug 1, 2024
  271. in src/script/descriptor.cpp:1493 in fa53d191a0 outdated 
    1499+        for (size_t j = 0; j < multipath_elems.size(); ++j) {
1500+            out[j].push_back(multipath_elems.at(j));
1501         }
1502-        out.push_back(p | (((uint32_t)hardened) << 31));
1503     }
1504+
    furszy commented at 8:49 pm on August 2, 2024:
    I didn’t find this implementation friendly enough and ended up reimplementing the function in a more readable way (at least for me) https://github.com/furszy/bitcoin-core/commit/84bbef2de7f4d3a697b28f6b9177e20dc974b8cc. All yours if you like it. It contain other small code improvements too.
    achow101 commented at 4:48 pm on August 5, 2024:
    Pulled in the suggested changes.
  272. in src/script/descriptor.cpp:1622 in fa53d191a0 outdated 
    1599@@ -1556,7 +1600,9 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t key_exp_index,
1600     static_assert(sizeof(info.fingerprint) == 4, "Fingerprint must be 4 bytes");
1601     assert(fpr_bytes.size() == 4);
1602     std::copy(fpr_bytes.begin(), fpr_bytes.end(), info.fingerprint);
1603-    if (!ParseKeyPath(slash_split, info.path, apostrophe, error)) return {};
1604+    std::vector<KeyPath> path;
1605+    if (!ParseKeyPath(slash_split, path, apostrophe, error)) return {};
1606+    info.path = path.at(0);
    furszy commented at 2:11 pm on August 3, 2024:

    In fa53d191a02:

    Could be explicit here. The general slash_split variable naming doesn’t really help.

    0    // Extract origin path elements
1    std::vector<KeyPath> path;
2    if (!ParseKeyPath(slash_split, path, apostrophe, error, /**allow_multipath=**/false)) return {};
3    info.path = path.at(0); // multipath specifier disabled for origin information

    . Can also remove the allow_multipath default value from the ParseKeyPath signature.

    achow101 commented at 4:49 pm on August 5, 2024:
    Removed the default value. The variable names weren’t added/changed in this PR.
  273. furszy commented at 4:07 pm on August 3, 2024: member
    Thinking out loud here. Since we are not expanding the multipath specifier to cover all possibilities due to a potentially large combinatorial space and are instead returning only those matching the Ith term, I believe we could enable value duplication within the new specifier for multisig descriptors. For example multi(1, pkA/<0;1;2>/0, pkB/<1;2;1>/0) could allow users to retrieve multi(1, pkA/0/0, pkB/1/0), multi(1, pkA/1/0, pkB/2/0), and multi(1, pkA/2/0, pkB/1/0). This would play well with the multipath values matching lengths requirement.
  274. achow101 force-pushed on Aug 5, 2024
  275. achow101 commented at 4:51 pm on August 5, 2024: member

    Addressed the recent comments, please re-review @darosior @pythcoiner @mjdietzx @furszy

    Thinking out loud here. Since we are not expanding the multipath specifier to cover all possibilities due to a potentially large combinatorial space and are instead returning only those matching the Ith term, I believe we could enable value duplication within the new specifier for multisig descriptors. For example multi(1, pkA/<0;1;2>/0, pkB/<1;2;1>/0) could allow users to retrieve multi(1, pkA/0/0, pkB/1/0), multi(1, pkA/1/0, pkB/2/0), and multi(1, pkA/2/0, pkB/1/0). This would play well with the multipath values matching lengths requirement.

    I think that’s a discussion more suitable for the mailing list. Maybe someone has a use for that construction, but I don’t quite see why anyone would do that in practice.

  276. in src/script/descriptor.cpp:1476 in 43e3027ecc outdated 
    1482+            }
1483+
1484+            for (const auto& num : nums) {
1485+                const auto& op_num = ParseKeyPathNum(num, apostrophe, error);
1486+                if (!op_num) return false;
1487+                if (std::find(multipath_values.begin(), multipath_values.end(), *op_num) != multipath_values.end()) {
    darosior commented at 10:24 am on August 6, 2024:

    If we are going to rewrite stuff just for style purpose, i’ll reiterate that this search is quadratic in the number of multipath values. Inserting each value in an unordered set and checking against it instead would make it linear in the number of multipath values.

    That said i don’t think it matters for any real world usage and therefore we should keep it for a follow-up.

    achow101 commented at 1:12 am on August 7, 2024:
    Done, since I’m making changes anyways.
  277. in src/script/descriptor.cpp:1499 in 43e3027ecc outdated 
    1512+    } else {
1513+        // Replace the multipath placeholder with each value while generating paths
1514+        for (size_t i = 0; i < multipath_values.size(); i++) {
1515+            KeyPath branch_path = path;
1516+            branch_path[*multipath_segment_index] = multipath_values[i];
1517+            out.emplace_back(branch_path);
    darosior commented at 10:34 am on August 6, 2024:

    nit:

     0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index 23faa78dac..7ef72bdda4 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -1490,13 +1490,13 @@ std::optional<uint32_t> ParseKeyPathNum(Span<const char> elem, bool& apostrophe,
 5     }
 6 
 7     if (!multipath_segment_index) {
 8-        out.emplace_back(path);
 9+        out.emplace_back(std::move(path));
10     } else {
11         // Replace the multipath placeholder with each value while generating paths
12         for (size_t i = 0; i < multipath_values.size(); i++) {
13             KeyPath branch_path = path;
14             branch_path[*multipath_segment_index] = multipath_values[i];
15-            out.emplace_back(branch_path);
16+            out.emplace_back(std::move(branch_path));
17         }
18     }
19     return true;
    achow101 commented at 1:12 am on August 7, 2024:
    Done
  278. darosior commented at 11:46 am on August 6, 2024: member

    re-ACK 8208454eae7484d34a162ffa7701157e56e3cb80

    Although i agree it’s more readable now it’s unfortunate to ask to rewrite part of the parsing logic only for style purpose after it had 3 ACKs and underwent significant fuzzing.

    That said i didn’t find any issue and neither did my fuzzer (for now). I’ll keep fuzzing this PR rebased on master for a day.

  279. DrahtBot requested review from mjdietzx on Aug 6, 2024
  280. furszy commented at 3:41 pm on August 6, 2024: member

    Dropping another out loud thinking: The multipath specifier can be used in a way it degrades wallet performance for no reason if it is used in the last segment of single key constructions. For instance, a pkh(xpub../0/<0;1;2;3;4;5;6;7;8;9;10,...>) imports 10 different descriptors into the wallet while the same can be expressed using a single ranged descriptor pkh(xpub../0/*) with range 0-10.

    Topic here is whether we should disable it for top-level single key functions, alert users about the degraded performance or do nothing. I am not suggesting disabling it for all functions because the functionality is useful for multi-key constructions. Thoughts?

  281. in src/script/descriptor.cpp:1805 in d60c6bba04 outdated 
    1818-            return std::make_unique<MultiADescriptor>(thres, std::move(providers), sortedmulti_a);
1819+
1820+        size_t num_multipath = std::max_element(providers.begin(), providers.end(),
1821+                [](const std::vector<std::unique_ptr<PubkeyProvider>>& a, const std::vector<std::unique_ptr<PubkeyProvider>>& b) {
1822+                    return a.size() < b.size();
1823+                })->size();
    furszy commented at 9:56 pm on August 6, 2024:

    In d60c6bba04f23f:

    tiny nit: could cache the largest providers length instead of looping through the elements again here:

     0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1--- a/src/script/descriptor.cpp	(revision 8208454eae7484d34a162ffa7701157e56e3cb80)
 2+++ b/src/script/descriptor.cpp	(date 1722981112377)
 3@@ -1818,6 +1818,7 @@
 4             return {};
 5         }
 6         size_t script_size = 0;
 7+        size_t max_providers_length = 1; // if multipath was used, this will be size of the largest multipath
 8         while (expr.size()) {
 9             if (!Const(",", expr)) {
10                 error = strprintf("Multi: expected ',', got '%c'", expr[0]);
11@@ -1830,6 +1831,7 @@
12                 return {};
13             }
14             script_size += pks.at(0)->GetSize() + 1;
15+            if (pks.size() > max_providers_length) max_providers_length = pks.size();
16             providers.emplace_back(std::move(pks));
17             key_exp_index++;
18         }
19@@ -1860,26 +1862,21 @@
20             }
21         }
22 
23-        size_t num_multipath = std::max_element(providers.begin(), providers.end(),
24-                [](const std::vector<std::unique_ptr<PubkeyProvider>>& a, const std::vector<std::unique_ptr<PubkeyProvider>>& b) {
25-                    return a.size() < b.size();
26-                })->size();
27-
28         // Make sure all vecs are of the same length, or exactly length 1
29         // For length 1 vectors, clone subdescs until vector is the same length
30         for (auto& vec : providers) {
31             if (vec.size() == 1) {
32-                for (size_t i = 1; i < num_multipath; ++i) {
33+                for (size_t i = 1; i < max_providers_length; ++i) {
34                     vec.emplace_back(vec.at(0)->Clone());
35                 }
36-            } else if (vec.size() != num_multipath) {
37+            } else if (vec.size() != max_providers_length) {
38                 error = strprintf("multi(): Multipath derivation paths have mismatched lengths");
39                 return {};
40             }
41         }
42 
43         // Build the final descriptors vector
44-        for (size_t i = 0; i < num_multipath; ++i) {
45+        for (size_t i = 0; i < max_providers_length; ++i) {
46             // Build final pubkeys vectors by retrieving the i'th subscript for each vector in subscripts
47             std::vector<std::unique_ptr<PubkeyProvider>> pubs;
48             pubs.reserve(providers.size());
    achow101 commented at 1:12 am on August 7, 2024:
    Done
  282. in src/script/descriptor.cpp:1808 in d60c6bba04 outdated 
    1821+                [](const std::vector<std::unique_ptr<PubkeyProvider>>& a, const std::vector<std::unique_ptr<PubkeyProvider>>& b) {
1822+                    return a.size() < b.size();
1823+                })->size();
1824+
1825+        // Make sure all vecs are of the same length, or exactly length 1
1826+        // For length 1 vectors, clone subdescs until vector is the same length
    furszy commented at 9:57 pm on August 6, 2024:

    In https://github.com/bitcoin/bitcoin/commit/d60c6bba04f23f2956371b87dcc92c263ed5bb1e:

    0        // For length 1 vectors, clone key providers until vector is the same length
    achow101 commented at 1:12 am on August 7, 2024:
    Done
  283. furszy commented at 10:36 pm on August 6, 2024: member

    still reviewing, only nits so far.

    Although i agree it’s more readable now it’s unfortunate to ask to rewrite part of the parsing logic only for style purpose after it had 3 ACKs and underwent significant fuzzing.

    I think it depends on the proposed changes. If readability is improved substantially (bolding substantially), it helps maintenance and might also uncover logical issues that the fuzzer might not easily encounter. I don’t think we should settle anything in stone because of a good fuzzing coverage. We should have enough time for fuzzing this properly during the freeze period.

    Yet, thats my general opinion. I understand that it is annoying to re-review code you’ve checked dozens of times over the past years and want to move on once for all. So.. will just continue reviewing 🚜.

  284. achow101 force-pushed on Aug 7, 2024
  285. descriptors: Add PubkeyProvider::Clone 7e86541f72
  286. descriptors: Add DescriptorImpl::Clone 0d55deae15
  287. in src/script/descriptor.cpp:2312 in 34691dc0fb outdated 
    2245+            std::vector<std::unique_ptr<PubkeyProvider>> keys;
2246+            keys.reserve(parser.m_keys.size());
2247+            for (auto& key : parser.m_keys) {
2248+                keys.emplace_back(std::move(key.at(0)));
2249+            }
2250+            return std::make_unique<MiniscriptDescriptor>(std::move(keys), std::move(node));
    furszy commented at 8:16 pm on August 7, 2024:

    Small comment about this (and a self-reminder): m_keys is actually a vector of pubkey providers (a vector of vectors). Each top-level vector corresponds to a key in the expression, and the inner vectors relates to the multipath values. Thus, because there is no possible multipath specifier outcome from miniscript::FromScript, the resulting Miniscript is built from the combination of all first terms of each inner vector.

    Maybe a comment here and something at the m_keys field declaration would help decrease any future mental burden.

    achow101 commented at 4:50 pm on August 8, 2024:
    I’ve updated the comment for m_keys to hopefully make this clearer.
  288. in src/rpc/output_script.cpp:272 in d62800722a outdated 
    270+            if (descs.empty()) {
271                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
272             }
273-
274+            if (descs.size() > 1) {
275+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor with mulitpath derivation path specifiers are not allowed");
    furszy commented at 8:24 pm on August 7, 2024: 
    0                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor with multipath derivation path specifiers are not allowed");
    achow101 commented at 4:50 pm on August 8, 2024:
    Done
  289. achow101 force-pushed on Aug 7, 2024
  290. in src/rpc/output_script.cpp:329 in 29825fda8d outdated 
    351+                for (size_t i = 1; i < descs.size(); ++i) {
352+                    ret.push_back(DeriveAddresses(descs.at(i).get(), range_begin, range_end, key_provider));
353                 }
354+                return ret;
355+            } else {
356+                return addresses;
    furszy commented at 1:33 pm on August 8, 2024:

    In 29825fda8d6eaa:

    nano styling nit:

     0// Return single descriptor
 1if (descs.size() == 1) {
 2    return addresses;
 3}
 4
 5// Expand multipath descriptor
 6UniValue ret(UniValue::VARR);
 7ret.push_back(addresses);
 8for (size_t i = 1; i < descs.size(); ++i) {
 9    ret.push_back(DeriveAddresses(descs.at(i).get(), range_begin, range_end, key_provider));
10}
11return ret;
    achow101 commented at 4:50 pm on August 8, 2024:
    Done
  291. in src/wallet/rpc/backup.cpp:1618 in 7ca8b5b5ca outdated 
    1614@@ -1596,6 +1615,7 @@ RPCHelpMan importdescriptors()
1615 {
1616     return RPCHelpMan{"importdescriptors",
1617                 "\nImport descriptors. This will trigger a rescan of the blockchain based on the earliest timestamp of all descriptors being imported. Requires a new wallet backup.\n"
1618+            "When importing descriptors with multipath key expressions, if the multipath specifier contains exactly two elements, the descriptor produced from the second elements will be imported as an internal descriptor.\n"
    furszy commented at 4:09 pm on August 8, 2024:

    In 7ca8b5b5c:

    This could be the default behavior but when the “internal” argument is provided, I think all descriptors should follow that argument (all internal or all external if the flag is set). Then, on a follow-up, we could make “internal” accept an array, allowing users to precisely determine which descriptor in the multipath is internal and which is not.

    achow101 commented at 4:50 pm on August 8, 2024:
    The intended and expected usage is for 2 multipath elements for receiving and change. I think diverging from that for default behavior will be confusing, especially since multipath descriptors are in use by other software and use this pattern. I’m fine with a followup that allows the user to override that and specify which should be internal.
    glozow commented at 11:24 am on August 13, 2024:
    nit “second elements”. Also could be helpful that apart from this special case, all multipath are non-internal
  292. furszy commented at 4:12 pm on August 8, 2024: member
    Almost finish, left a question #22838 (review). The rest are nits.
  293. 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.
    a5f39b1034
  294. 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.

Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>
    0d640c6f02
  295. 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.
    1bbf46e2da
  296. tests: Add unit tests for multipath descriptors a90eee444c
  297. tests: Multipath descriptors for getdescriptorinfo 360456cd22
  298. 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.
    cddc0ba9a9
  299. tests: Multipath descriptors for scantxoutset and deriveaddresses 1692245525
  300. 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.
    64dfe3ce4b
  301. 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.
    32dcbca3fb
  302. 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.
    f97d5c137d
  303. tests: Test importing of multipath descriptors 
    Test that both importmulti and importdescriptors behave as expected when
importing a multipath descriptor.
    0019f61fc5
  304. doc: Mention multipath specifier a0abcbd382
  305. achow101 force-pushed on Aug 8, 2024
  306. furszy commented at 7:42 pm on August 8, 2024: member
    Code review ACK a0abcbd
  307. DrahtBot requested review from darosior on Aug 8, 2024
  308. darosior commented at 10:13 am on August 10, 2024: member

    re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501

    Running a fuzzer for a couple days on the previous push (rebased on master to get the speedups from #30197) and with a dictionary specifically targeting the multipath expressions did not uncover anything.

  309. in doc/descriptors.md:275 in a0abcbd382 
    270+
271+For example, a descriptor of the form:
272+
273+    multi(2,xpub.../<0;1;2>/0/*,xpub.../<2;3;4>/*)
274+
275+will expand to the two descriptors
    glozow commented at 12:52 pm on August 12, 2024:
    Isn’t this 3?
  310. in src/wallet/rpc/backup.cpp:1099 in 32dcbca3fb outdated 
    1103-        }
1104+    for (size_t j = 0; j < parsed_descs.size(); ++j) {
1105+        const auto& parsed_desc = parsed_descs.at(j);
1106+        bool desc_internal = internal.has_value() && internal.value();
1107+        if (parsed_descs.size() == 2) {
1108+            desc_internal = j == 1;
    glozow commented at 11:15 am on August 13, 2024:
    Could also add a comment here on the j==1, “first is receiving, second is change” or something
  311. in src/rpc/output_script.cpp:283 in cddc0ba9a9 outdated 
    285+                RPCResult::Type::ARR, "", "",
286+                {
287+                    {RPCResult::Type::STR, "address", "the derived addresses"},
288+                }
289+            },
290+            RPCResult{"for multipath descriptors",
    glozow commented at 11:40 am on August 13, 2024:
    (I don’t know how frequently we expect each type of usage to be.) Did you consider returning single derivation descriptors in both result arrays, so that the original result can eventually be removed (after deprecation, everyone adds a [0] to their scripts when using single)?
    achow101 commented at 1:27 pm on August 13, 2024:
    I expect single path to still be most used and I thought that returning an array for that would be confusing.
    glozow commented at 1:34 pm on August 13, 2024:
    Makes sense
  312. in src/script/descriptor.cpp:1451 in 0d640c6f02 outdated 
    1449+[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath)
1450 {
1451+    KeyPath path;
1452+    std::optional<size_t> multipath_segment_index;
1453+    std::vector<uint32_t> multipath_values;
1454+    std::unordered_set<uint32_t> seen_multipath;
    glozow commented at 11:42 am on August 13, 2024:
    Could add some comments for future wallet reviewers, maybe mentioning that they should be consistent with each other (multipath_segment_index.has_value() == !multipath_values.empty() == !seen_multipath.empty())
  313. in src/script/descriptor.cpp:1497 in 0d640c6f02 outdated 
    1510+    }
1511+
1512+    if (!multipath_segment_index) {
1513+        out.emplace_back(std::move(path));
1514+    } else {
1515+        // Replace the multipath placeholder with each value while generating paths
    glozow commented at 11:43 am on August 13, 2024:
    Can Assume(!multipath_values.empty()) here
  314. glozow commented at 12:39 pm on August 13, 2024: member

    light code review ACK a0abcbd3822

    I’m unfamiliar with the descriptor code, but the refactoring commits look like pure refactoring, behavior change commits look correct, RPC changes seem intuitive, and tests seem to cover things when I introduce mutations.

  315. achow101 commented at 1:28 pm on August 13, 2024: member
    @pythcoiner @mjdietzx Can you please re-review?
  316. mjdietzx commented at 10:34 pm on August 13, 2024: contributor
    reACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
  317. achow101 removed this from the milestone 28.0 on Aug 15, 2024
  318. achow101 added this to the milestone 29.0 on Aug 15, 2024
  319. pythcoiner commented at 10:33 am on August 17, 2024: none
    reACK a0abcbd
  320. glozow merged this on Aug 28, 2024
  321. glozow closed this on Aug 28, 2024
  322. in src/script/descriptor.cpp:1456 in a0abcbd382 
    1462-                elem = elem.first(elem.size() - 1);
1463-                hardened = true;
1464-                apostrophe = last == '\'';
1465+        const Span<const char>& elem = split[i];
1466+
1467+        // Check if element contain multipath specifier
    jonatack commented at 5:16 pm on August 28, 2024:
    nit s/contain/contains (if anyone updates for the other review suggestions)
  323. in src/script/descriptor.cpp:1498 in a0abcbd382 
    1511+
1512+    if (!multipath_segment_index) {
1513+        out.emplace_back(std::move(path));
1514+    } else {
1515+        // Replace the multipath placeholder with each value while generating paths
1516+        for (size_t i = 0; i < multipath_values.size(); i++) {
    jonatack commented at 5:17 pm on August 28, 2024:
    nit, compilers may auto-optimize here but if anyone handles the other feedback suggestions: s/i++/++i/
  324. jonatack commented at 5:20 pm on August 28, 2024: member

    With this merge, would it be a good idea to update relevant BIPs (possibly 88, 388, 389)?

    e.g. https://github.com/bitcoin/bips/blob/master/bip-0088.mediawiki#compatibility

    “There is a discussion on path templating for bitcoin script descriptors at #17190, which proposes the format xpub…{0,1}/, of which the {0,1}/ part would correspond to the partial path template in the format of this BIP.”


achow101 DrahtBot Rspigler mjdietzx Sjors prusnak craigraw maflcko fanquake darosior furszy pythcoiner glozow jonatack


Sjors laanwj mjdietzx darosior

Labels
Wallet RPC/REST/ZMQ Descriptors

Milestone
29.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me