Merge bitcoin#22838: descriptors: Be able to specify change and recei…

…ving in a single descriptor string

a0abcbd doc: Mention multipath specifier (Ava Chow)
0019f61 tests: Test importing of multipath descriptors (Ava Chow)
f97d5c1 wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3c wallet: Move internal to be per key when importing (Ava Chow)
1692245 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456c tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee4 tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6 descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b1 descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55dea descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541 descriptors: Add PubkeyProvider::Clone (Ava Chow)

Pull request description:

  It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in bitcoin#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 bitcoin#17190

ACKs for top commit:
  darosior:
    re-ACK a0abcbd
  mjdietzx:
    reACK a0abcbd
  pythcoiner:
    reACK a0abcbd
  furszy:
    Code review ACK a0abcbd
  glozow:
    light code review ACK a0abcbd

Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2

doc/descriptors.md

@@ -98,6 +98,7 @@ Descriptors consist of several types of expressions. The top level expression is
   - [WIF](https://en.bitcoin.it/wiki/Wallet_import_format) encoded private keys may be specified instead of the corresponding public key, with the same meaning.
   - `xpub` encoded extended public key or `xprv` encoded extended private key (as defined in [BIP 32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki)).
     - Followed by zero or more `/NUM` unhardened and `/NUM'` hardened BIP32 derivation steps.
+      - No more than one of these derivation steps may be of the form `<NUM;NUM;...;NUM>` (including hardened indicators with either or both `NUM`). If such specifiers are included, the descriptor will be parsed as multiple descriptors where the first descriptor uses all of the first `NUM` in the pair, and the second descriptor uses the second `NUM` in the pair for all `KEY` expressions, and so on.
     - Optionally followed by a single `/*` or `/*'` final step to denote all (direct) unhardened or hardened children.
     - The usage of hardened derivation steps requires providing the private key.
 
@@ -257,6 +258,30 @@ Note how the first key is an xprv private key with a specific derivation path,
 while the other two are public keys.
 
 
+### Specifying receiving and change descriptors in one descriptor
+
+Since receiving and change addresses are frequently derived from the same
+extended key(s) but with a single derivation index changed, it is convenient
+to be able to specify a descriptor that can derive at the two different
+indexes. Thus a single tuple of indexes is allowed in each derivation path
+following the extended key. When this descriptor is parsed, multiple descriptors
+will be produced, the first one will use the first index in the tuple for all
+key expressions, the second will use the second index, the third will use the
+third index, and so on..
+
+For example, a descriptor of the form:
+
+    multi(2,xpub.../<0;1;2>/0/*,xpub.../<2;3;4>/*)
+
+will expand to the two descriptors
+
+   multi(2,xpub.../0/0/*,xpub.../2/*)
+   multi(2,xpub.../1/0/*,xpub.../3/*)
+   multi(2,xpub.../2/0/*,xpub.../4*)
+
+When this tuple contains only two elements, wallet implementations can use the
+first descriptor for receiving addresses and the second descriptor for change addresses.
+
 ### Compatibility with old wallets
 
 In order to easily represent the sets of scripts currently supported by

src/bench/descriptors.cpp

@@ -23,12 +23,12 @@ static void ExpandDescriptor(benchmark::Bench& bench)
     const std::pair<int64_t, int64_t> range = {0, 1000};
     FlatSigningProvider provider;
     std::string error;
-    auto desc = Parse(desc_str, provider, error);
+    auto descs = Parse(desc_str, provider, error);
 
     bench.run([&] {
         for (int i = range.first; i <= range.second; ++i) {
             std::vector<CScript> scripts;
-            bool success = desc->Expand(i, provider, scripts, provider);
+            bool success = descs[0]->Expand(i, provider, scripts, provider);
             assert(success);
         }
     });

src/bench/wallet_ismine.cpp

@@ -52,8 +52,8 @@ static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_co
             key.MakeNewKey(/*fCompressed=*/true);
             FlatSigningProvider keys;
             std::string error;
-            std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /*require_checksum=*/false);
-            WalletDescriptor w_desc(std::move(desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0);
+            std::vector<std::unique_ptr<Descriptor>> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /*require_checksum=*/false);
+            WalletDescriptor w_desc(std::move(desc.at(0)), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0);
             auto spkm = wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false);
             assert(spkm);
         }

src/qt/test/wallettests.cpp

@@ -199,7 +199,7 @@ std::shared_ptr<CWallet> SetupLegacyWatchOnlyWallet(interfaces::Node& node, Test
         wallet->SetupLegacyScriptPubKeyMan();
         // Add watched key
         CPubKey pubKey = test.coinbaseKey.GetPubKey();
-        bool import_keys = wallet->ImportPubKeys({pubKey.GetID()}, {{pubKey.GetID(), pubKey}} , /*key_origins=*/{}, /*add_keypool=*/false, /*internal=*/false, /*timestamp=*/1);
+        bool import_keys = wallet->ImportPubKeys({{pubKey.GetID(), false}}, {{pubKey.GetID(), pubKey}} , /*key_origins=*/{}, /*add_keypool=*/false, /*timestamp=*/1);
         assert(import_keys);
         wallet->SetLastBlockProcessed(105, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
     }
@@ -218,8 +218,10 @@ std::shared_ptr<CWallet> SetupDescriptorsWallet(interfaces::Node& node, TestChai
     // Add the coinbase key
     FlatSigningProvider provider;
     std::string error;
-    std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(test.coinbaseKey) + ")", provider, error, /* require_checksum=*/ false);
-    assert(desc);
+    auto descs = Parse("combo(" + EncodeSecret(test.coinbaseKey) + ")", provider, error, /* require_checksum=*/ false);
+    assert(!descs.empty());
+    assert(descs.size() == 1);
+    auto& desc = descs.at(0);
     WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
     if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
     CTxDestination dest = GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type);

src/rpc/mining.cpp

@@ -181,35 +181,36 @@ static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const
 static bool getScriptFromDescriptor(const std::string& descriptor, CScript& script, std::string& error)
 {
     FlatSigningProvider key_provider;
-    const auto desc = Parse(descriptor, key_provider, error, /* require_checksum = */ false);
-    if (desc) {
-        if (desc->IsRange()) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Ranged descriptor not accepted. Maybe pass through deriveaddresses first?");
-        }
-
-        FlatSigningProvider provider;
-        std::vector<CScript> scripts;
-        if (!desc->Expand(0, key_provider, scripts, provider)) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot derive script without private keys");
-        }
+    const auto descs = Parse(descriptor, key_provider, error, /* require_checksum = */ false);
+    if (descs.empty()) return false;
+    if (descs.size() > 1) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Multipath descriptor not accepted");
+    }
+    const auto& desc = descs.at(0);
+    if (desc->IsRange()) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Ranged descriptor not accepted. Maybe pass through deriveaddresses first?");
+    }
 
-        // Combo descriptors can have 2 or 4 scripts, so we can't just check scripts.size() == 1
-        CHECK_NONFATAL(scripts.size() > 0 && scripts.size() <= 4);
+    FlatSigningProvider provider;
+    std::vector<CScript> scripts;
+    if (!desc->Expand(0, key_provider, scripts, provider)) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot derive script without private keys");
+    }
 
-        if (scripts.size() == 1) {
-            script = scripts.at(0);
-        } else if (scripts.size() == 4) {
-            // For uncompressed keys, take the 3rd script, since it is p2wpkh
-            script = scripts.at(2);
-        } else {
-            // Else take the 2nd script, since it is p2pkh
-            script = scripts.at(1);
-        }
+    // Combo descriptors can have 2 or 4 scripts, so we can't just check scripts.size() == 1
+    CHECK_NONFATAL(scripts.size() > 0 && scripts.size() <= 4);
 
-        return true;
+    if (scripts.size() == 1) {
+        script = scripts.at(0);
+    } else if (scripts.size() == 4) {
+        // For uncompressed keys, take the 3rd script, since it is p2wpkh
+        script = scripts.at(2);
     } else {
-        return false;
+        // Else take the 2nd script, since it is p2pkh
+        script = scripts.at(1);
     }
+
+    return true;
 }
 
 static RPCHelpMan generatetodescriptor()

src/rpc/output_script.cpp

@@ -175,7 +175,11 @@ static RPCHelpMan getdescriptorinfo()
         RPCResult{
             RPCResult::Type::OBJ, "", "",
             {
-                {RPCResult::Type::STR, "descriptor", "The descriptor in canonical form, without private keys"},
+                {RPCResult::Type::STR, "descriptor", "The descriptor in canonical form, without private keys. For a multipath descriptor, only the first will be returned."},
+                {RPCResult::Type::ARR, "multipath_expansion", /*optional=*/true, "All descriptors produced by expanding multipath derivation elements. Only if the provided descriptor specifies multipath derivation elements.",
+                {
+                    {RPCResult::Type::STR, "", ""},
+                }},
                 {RPCResult::Type::STR, "checksum", "The checksum for the input descriptor"},
                 {RPCResult::Type::BOOL, "isrange", "Whether the descriptor is ranged"},
                 {RPCResult::Type::BOOL, "issolvable", "Whether the descriptor is solvable"},
@@ -191,22 +195,65 @@ static RPCHelpMan getdescriptorinfo()
         {
             FlatSigningProvider provider;
             std::string error;
-            auto desc = Parse(request.params[0].get_str(), provider, error);
-            if (!desc) {
+            auto descs = Parse(request.params[0].get_str(), provider, error);
+            if (descs.empty()) {
                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
             }
 
             UniValue result(UniValue::VOBJ);
-            result.pushKV("descriptor", desc->ToString());
+            result.pushKV("descriptor", descs.at(0)->ToString());
+
+            if (descs.size() > 1) {
+                UniValue multipath_descs(UniValue::VARR);
+                for (const auto& d : descs) {
+                    multipath_descs.push_back(d->ToString());
+                }
+                result.pushKV("multipath_expansion", multipath_descs);
+            }
+
             result.pushKV("checksum", GetDescriptorChecksum(request.params[0].get_str()));
-            result.pushKV("isrange", desc->IsRange());
-            result.pushKV("issolvable", desc->IsSolvable());
+            result.pushKV("isrange", descs.at(0)->IsRange());
+            result.pushKV("issolvable", descs.at(0)->IsSolvable());
             result.pushKV("hasprivatekeys", provider.keys.size() > 0);
             return result;
         },
     };
 }
 
+static UniValue DeriveAddresses(const Descriptor* desc, int64_t range_begin, int64_t range_end, FlatSigningProvider& key_provider)
+{
+    UniValue addresses(UniValue::VARR);
+
+    for (int64_t i = range_begin; i <= range_end; ++i) {
+        FlatSigningProvider provider;
+        std::vector<CScript> scripts;
+        if (!desc->Expand(i, key_provider, scripts, provider)) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot derive script without private keys");
+        }
+
+        for (const CScript& script : scripts) {
+            CTxDestination dest;
+            if (!ExtractDestination(script, dest)) {
+                // ExtractDestination no longer returns true for P2PK since it doesn't have a corresponding address
+                // However combo will output P2PK and should just ignore that script
+                if (scripts.size() > 1 && std::get_if<PubKeyDestination>(&dest)) {
+                    continue;
+                }
+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address");
+            }
+
+            addresses.push_back(EncodeDestination(dest));
+        }
+    }
+
+    // This should not be possible, but an assert seems overkill:
+    if (addresses.empty()) {
+        throw JSONRPCError(RPC_MISC_ERROR, "Unexpected empty result");
+    }
+
+    return addresses;
+}
+
 static RPCHelpMan deriveaddresses()
 {
     const std::string EXAMPLE_DESCRIPTOR = "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#cjjspncu";
@@ -226,11 +273,24 @@ static RPCHelpMan deriveaddresses()
             {"descriptor", RPCArg::Type::STR, RPCArg::Optional::NO, "The descriptor."},
             {"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED, "If a ranged descriptor is used, this specifies the end or the range (in [begin,end] notation) to derive."},
         },
-        RPCResult{
-            RPCResult::Type::ARR, "", "",
-            {
-                {RPCResult::Type::STR, "address", "the derived addresses"},
-            }
+        {
+            RPCResult{"for single derivation descriptors",
+                RPCResult::Type::ARR, "", "",
+                {
+                    {RPCResult::Type::STR, "address", "the derived addresses"},
+                }
+            },
+            RPCResult{"for multipath descriptors",
+                RPCResult::Type::ARR, "", "The derived addresses for each of the multipath expansions of the descriptor, in multipath specifier order",
+                {
+                    {
+                        RPCResult::Type::ARR, "", "The derived addresses for a multipath descriptor expansion",
+                        {
+                            {RPCResult::Type::STR, "address", "the derived address"},
+                        },
+                    },
+                },
+            },
         },
         RPCExamples{
             "First three native segwit receive addresses\n" +
@@ -250,11 +310,11 @@ static RPCHelpMan deriveaddresses()
 
             FlatSigningProvider key_provider;
             std::string error;
-            auto desc = Parse(desc_str, key_provider, error, /* require_checksum = */ true);
-            if (!desc) {
+            auto descs = Parse(desc_str, key_provider, error, /* require_checksum = */ true);
+            if (descs.empty()) {
                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
             }
-
+            auto& desc = descs.at(0);
             if (!desc->IsRange() && request.params.size() > 1) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor");
             }
@@ -263,36 +323,18 @@ static RPCHelpMan deriveaddresses()
                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Range must be specified for a ranged descriptor");
             }
 
-            UniValue addresses(UniValue::VARR);
-
-            for (int64_t i = range_begin; i <= range_end; ++i) {
-                FlatSigningProvider provider;
-                std::vector<CScript> scripts;
-                if (!desc->Expand(i, key_provider, scripts, provider)) {
-                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot derive script without private keys");
-                }
+            UniValue addresses = DeriveAddresses(desc.get(), range_begin, range_end, key_provider);
 
-                for (const CScript& script : scripts) {
-                    CTxDestination dest;
-                    if (!ExtractDestination(script, dest)) {
-                        // ExtractDestination no longer returns true for P2PK since it doesn't have a corresponding address
-                        // However combo will output P2PK and should just ignore that script
-                        if (scripts.size() > 1 && std::get_if<PubKeyDestination>(&dest)) {
-                            continue;
-                        }
-                        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address");
-                    }
-
-                    addresses.push_back(EncodeDestination(dest));
-                }
+            if (descs.size() == 1) {
+                return addresses;
             }
 
-            // This should not be possible, but an assert seems overkill:
-            if (addresses.empty()) {
-                throw JSONRPCError(RPC_MISC_ERROR, "Unexpected empty result");
+            UniValue ret(UniValue::VARR);
+            ret.push_back(addresses);
+            for (size_t i = 1; i < descs.size(); ++i) {
+                ret.push_back(DeriveAddresses(descs.at(i).get(), range_begin, range_end, key_provider));
             }
-
-            return addresses;
+            return ret;
         },
     };
 }

src/rpc/util.cpp

@@ -1345,24 +1345,26 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
     }
 
     std::string error;
-    auto desc = Parse(desc_str, provider, error);
-    if (!desc) {
+    auto descs = Parse(desc_str, provider, error);
+    if (descs.empty()) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
     }
-    if (!desc->IsRange()) {
+    if (!descs.at(0)->IsRange()) {
         range.first = 0;
         range.second = 0;
     }
     std::vector<CScript> ret;
     for (int i = range.first; i <= range.second; ++i) {
-        std::vector<CScript> scripts;
-        if (!desc->Expand(i, provider, scripts, provider)) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys: '%s'", desc_str));
-        }
-        if (expand_priv) {
-            desc->ExpandPrivate(/*pos=*/i, provider, /*out=*/provider);
+        for (const auto& desc : descs) {
+            std::vector<CScript> scripts;
+            if (!desc->Expand(i, provider, scripts, provider)) {
+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys: '%s'", desc_str));
+            }
+            if (expand_priv) {
+                desc->ExpandPrivate(/*pos=*/i, provider, /*out=*/provider);
+            }
+            std::move(scripts.begin(), scripts.end(), std::back_inserter(ret));
         }
-        std::move(scripts.begin(), scripts.end(), std::back_inserter(ret));
     }
     return ret;
 }