Merge 95b2d8d8dc into a46065e36c

src/bench/wallet_ismine.cpp

@@ -46,7 +46,7 @@ static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_co
             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);
+            WalletDescriptor w_desc(std::move(desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0, /*_internal=*/false);
             auto spkm = wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false);
             assert(spkm);
         }

src/qt/test/wallettests.cpp

@@ -220,7 +220,7 @@ std::shared_ptr<CWallet> SetupDescriptorsWallet(interfaces::Node& node, TestChai
     std::string error;
     std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(test.coinbaseKey) + ")", provider, error, /* require_checksum=*/ false);
     assert(desc);
-    WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
+    WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1, /*_internal=*/false);
     if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
     CTxDestination dest = GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type);
     wallet->SetAddressBook(dest, "", wallet::AddressPurpose::RECEIVE);

src/rpc/client.cpp

@@ -73,6 +73,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listtransactions", 1, "count" },
     { "listtransactions", 2, "skip" },
     { "listtransactions", 3, "include_watchonly" },
+    { "listtransactions", 4, "include_change" },
     { "walletpassphrase", 1, "timeout" },
     { "getblocktemplate", 0, "template_request" },
     { "listsinceblock", 1, "target_confirmations" },
@@ -113,6 +114,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "getchaintxstats", 0, "nblocks" },
     { "gettransaction", 1, "include_watchonly" },
     { "gettransaction", 2, "verbose" },
+    { "gettransaction", 3, "include_change" },
     { "getrawtransaction", 1, "verbosity" },
     { "getrawtransaction", 1, "verbose" },
     { "createrawtransaction", 0, "inputs" },

src/wallet/external_signer_scriptpubkeyman.cpp

@@ -27,7 +27,7 @@ bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::uni
     int64_t creation_time = GetTime();
 
     // Make the descriptor
-    WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
+    WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0, /*_internal=*/false);
     m_wallet_descriptor = w_desc;
 
     // Store the descriptor

src/wallet/feebumper.cpp

@@ -258,7 +258,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
         const CTxOut& output = txouts.at(i);
         CTxDestination dest;
         ExtractDestination(output.scriptPubKey, dest);
-        if (original_change_index.has_value() ?  original_change_index.value() == i : OutputIsChange(wallet, output)) {
+        if (original_change_index.has_value() ?  original_change_index.value() == i : ScriptIsChange(wallet, output.scriptPubKey)) {
             new_coin_control.destChange = dest;
         } else {
             CRecipient recipient = {dest, output.nValue, false};

src/wallet/interfaces.cpp

@@ -66,9 +66,10 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
     result.txout_is_mine.reserve(wtx.tx->vout.size());
     result.txout_address.reserve(wtx.tx->vout.size());
     result.txout_address_is_mine.reserve(wtx.tx->vout.size());
-    for (const auto& txout : wtx.tx->vout) {
+    for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
+        const CTxOut& txout = wtx.tx->vout.at(i);
         result.txout_is_mine.emplace_back(wallet.IsMine(txout));
-        result.txout_is_change.push_back(OutputIsChange(wallet, txout));
+        result.txout_is_change.push_back(IsOutputChange(wallet, *wtx.tx, i));
         result.txout_address.emplace_back();
         result.txout_address_is_mine.emplace_back(ExtractDestination(txout.scriptPubKey, result.txout_address.back()) ?
                                                       wallet.IsMine(result.txout_address.back()) :

src/wallet/receive.cpp

@@ -48,48 +48,83 @@ CAmount TxGetCredit(const CWallet& wallet, const CTransaction& tx, const isminef
     return nCredit;
 }
 
+bool IsOutputChange(const CWallet& wallet, const CTransaction& tx, unsigned int change_pos)
+{
+    AssertLockHeld(wallet.cs_wallet);
+    // If at least one of the inputs is from the wallet, then there might be a change output.
+    // Otherwise, it's definitely not a change output.
+    if (!std::any_of(tx.vin.begin(), tx.vin.end(),
+                     [&wallet](const CTxIn& in) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { return InputIsMine(wallet, in); })) {
+        return false;
+    }
+
+    return ScriptIsChange(wallet, tx.vout.at(change_pos).scriptPubKey);
+}
+
 bool ScriptIsChange(const CWallet& wallet, const CScript& script)
 {
-    // TODO: fix handling of 'change' outputs. The assumption is that any
-    // payment to a script that is ours, but is not in the address book
-    // is change. That assumption is likely to break when we implement multisignature
-    // wallets that return change back into a multi-signature-protected address;
-    // a better way of identifying which outputs are 'the send' and which are
-    // 'the change' will need to be implemented (maybe extend CWalletTx to remember
-    // which output, if any, was change).
     AssertLockHeld(wallet.cs_wallet);
-    if (wallet.IsMine(script))
-    {
-        CTxDestination address;
-        if (!ExtractDestination(script, address))
-            return true;
-        if (!wallet.FindAddressBookEntry(address)) {
-            return true;
+    // If the script is not from one of our internal spkms, we don't consider it change.
+    // This applies to transactions sending coins back to the source as well. Eg: If the
+    // source address is not under one of the wallet internal spkms, we don't consider
+    // the output as change.
+    // Important note: the legacy spkm is on the internal spkm set as well
+    std::set<ScriptPubKeyMan*> internal_spkms = wallet.GetAllScriptPubKeyMans(/*only_internal=*/true);
+    SignatureData sigdata;
+    ScriptPubKeyMan* spkm{nullptr};
+    for (const auto& iter_spk_man : internal_spkms) {
+        if (iter_spk_man && iter_spk_man->IsMine(script)) {
+            spkm = iter_spk_man;
+            break;
         }
     }
-    return false;
+    if (!spkm) return false;
+
+    // check if it's solvable
+    if (auto desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(spkm)) {
+        return WITH_LOCK(desc_spkm->cs_desc_man, return desc_spkm->GetWalletDescriptor().descriptor->IsSolvable());
+    } else {
+        // Legacy
+        auto legacy_spkm = dynamic_cast<LegacyScriptPubKeyMan*>(spkm);
+        if (!legacy_spkm) return false; // unknown spkm
+
+        CTxDestination dest;
+        if (!ExtractDestination(script, dest) || !IsValidDestination(dest)) return false;
+
+        // If the wallet supports HD split
+        if (legacy_spkm->IsHDEnabled() && wallet.CanSupportFeature(FEATURE_HD_SPLIT)) {
+            std::unique_ptr<CKeyMetadata> meta = legacy_spkm->GetMetadata(dest);
+            if (meta && meta->has_key_origin) {
+                // bip32 internal path /0'/1'/*
+                if (meta->key_origin.path.empty() || meta->key_origin.path.size() > 3) return false; // shouldn't happen, legacy supports bip32 derivation path only
+                return (meta->key_origin.path.at(1) & ~0x80000000); // BIP32_HARDENED_KEY_LIMIT=0x80000000
+            }
+        }
+
+        // HD wallet not enabled or no key origin (legacy behaviour):
+        // As last resource fallback for legacy wallets, use the address book data to return whether the script is change or not.
+        // This is based on the following naive assumptions:
+        // 1) The lack of address book entry make any destination a "change destination".
+        // 2) Any entry in the address book without a label is a "change destination".
+        return wallet.FindAddressBookEntry(dest, /*allow_change=*/false) == nullptr;
+    }
 }
 
-bool OutputIsChange(const CWallet& wallet, const CTxOut& txout)
-{
-    return ScriptIsChange(wallet, txout.scriptPubKey);
-}
-
-CAmount OutputGetChange(const CWallet& wallet, const CTxOut& txout)
+CAmount OutputGetChange(const CWallet& wallet, const CTransaction& tx, unsigned int change_pos)
 {
     AssertLockHeld(wallet.cs_wallet);
+    const CTxOut& txout = tx.vout.at(change_pos);
     if (!MoneyRange(txout.nValue))
         throw std::runtime_error(std::string(__func__) + ": value out of range");
-    return (OutputIsChange(wallet, txout) ? txout.nValue : 0);
+    return IsOutputChange(wallet, tx, change_pos) ? txout.nValue : 0;
 }
 
 CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx)
 {
     LOCK(wallet.cs_wallet);
     CAmount nChange = 0;
-    for (const CTxOut& txout : tx.vout)
-    {
-        nChange += OutputGetChange(wallet, txout);
+    for (size_t change_pos=0 ; change_pos<tx.vout.size(); change_pos++) {
+        nChange += OutputGetChange(wallet, tx, change_pos);
         if (!MoneyRange(nChange))
             throw std::runtime_error(std::string(__func__) + ": value out of range");
     }
@@ -219,7 +254,7 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
         //   2) the output is to us (received)
         if (nDebit > 0)
         {
-            if (!include_change && OutputIsChange(wallet, txout))
+            if (!include_change && IsOutputChange(wallet, *wtx.tx, i))
                 continue;
         }
         else if (!(fIsMine & filter))
@@ -385,14 +420,14 @@ std::set< std::set<CTxDestination> > GetAddressGroupings(const CWallet& wallet)
             // group change with input addresses
             if (any_mine)
             {
-               for (const CTxOut& txout : wtx.tx->vout)
-                   if (OutputIsChange(wallet, txout))
-                   {
+               for (size_t pos = 0; pos < wtx.tx->vout.size(); pos++) {
+                   if (IsOutputChange(wallet, *wtx.tx, pos)) {
                        CTxDestination txoutAddr;
-                       if(!ExtractDestination(txout.scriptPubKey, txoutAddr))
+                       if (!ExtractDestination(wtx.tx->vout[pos].scriptPubKey, txoutAddr))
                            continue;
                        grouping.insert(txoutAddr);
                    }
+               }
             }
             if (grouping.size() > 0)
             {

src/wallet/receive.h

@@ -19,9 +19,9 @@ bool AllInputsMine(const CWallet& wallet, const CTransaction& tx, const isminefi
 CAmount OutputGetCredit(const CWallet& wallet, const CTxOut& txout, const isminefilter& filter);
 CAmount TxGetCredit(const CWallet& wallet, const CTransaction& tx, const isminefilter& filter);
 
+bool IsOutputChange(const CWallet& wallet, const CTransaction& tx, unsigned int change_pos) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 bool ScriptIsChange(const CWallet& wallet, const CScript& script) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
-bool OutputIsChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
-CAmount OutputGetChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
+CAmount OutputGetChange(const CWallet& wallet, const CTransaction& tx, unsigned int change_pos) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx);
 
 CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)

src/wallet/rpc/backup.cpp

@@ -1550,7 +1550,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
            }
         }
 
-        WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index);
+        WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index, internal);
 
         // Check if the wallet already contains the descriptor
         auto existing_spk_manager = wallet.GetDescriptorScriptPubKeyMan(w_desc);

src/wallet/rpc/transactions.cpp

@@ -362,7 +362,7 @@ static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nM
         for (const COutputEntry& r : listReceived)
         {
             std::string label;
-            const auto* address_book_entry = wallet.FindAddressBookEntry(r.destination);
+            const auto* address_book_entry = wallet.FindAddressBookEntry(r.destination, /*allow_change=*/include_change);
             if (address_book_entry) {
                 label = address_book_entry->GetLabel();
             }
@@ -448,6 +448,7 @@ RPCHelpMan listtransactions()
                     {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
                     {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
                     {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
+                    {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
                 },
                 RPCResult{
                     RPCResult::Type::ARR, "", "",
@@ -511,6 +512,8 @@ RPCHelpMan listtransactions()
         filter |= ISMINE_WATCH_ONLY;
     }
 
+    bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
+
     if (nCount < 0)
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative count");
     if (nFrom < 0)
@@ -526,7 +529,7 @@ RPCHelpMan listtransactions()
         for (CWallet::TxItems::const_reverse_iterator it = txOrdered.rbegin(); it != txOrdered.rend(); ++it)
         {
             CWalletTx *const pwtx = (*it).second;
-            ListTransactions(*pwallet, *pwtx, 0, true, ret, filter, filter_label);
+            ListTransactions(*pwallet, *pwtx, 0, true, ret, filter, filter_label, include_change);
             if ((int)ret.size() >= (nCount+nFrom)) break;
         }
     }
@@ -701,6 +704,7 @@ RPCHelpMan gettransaction()
                             "Whether to include watch-only addresses in balance calculation and details[]"},
                     {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false},
                             "Whether to include a `decoded` field containing the decoded transaction (equivalent to RPC decoderawtransaction)"},
+                    {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also include change output/s.\n"},
                 },
                 RPCResult{
                     RPCResult::Type::OBJ, "", "", Cat(Cat<std::vector<RPCResult>>(
@@ -788,7 +792,8 @@ RPCHelpMan gettransaction()
     WalletTxToJSON(*pwallet, wtx, entry);
 
     UniValue details(UniValue::VARR);
-    ListTransactions(*pwallet, wtx, 0, false, details, filter, /*filter_label=*/std::nullopt);
+    bool include_change = (!request.params[3].isNull() && request.params[3].get_bool());
+    ListTransactions(*pwallet, wtx, 0, false, details, filter, /*filter_label=*/std::nullopt, include_change);
     entry.pushKV("details", details);
 
     entry.pushKV("hex", EncodeHexTx(*wtx.tx));

src/wallet/scriptpubkeyman.cpp

@@ -1809,7 +1809,7 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
         FlatSigningProvider keys;
         std::string error;
         std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);
-        WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
+        WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0, /*_internal=*/false);
 
         // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
         auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size));
@@ -1853,8 +1853,9 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
             FlatSigningProvider keys;
             std::string error;
             std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);
-            uint32_t chain_counter = std::max((i == 1 ? chain.nInternalChainCounter : chain.nExternalChainCounter), (uint32_t)0);
-            WalletDescriptor w_desc(std::move(desc), 0, 0, chain_counter, 0);
+            bool is_internal = i == 1;
+            uint32_t chain_counter = std::max((is_internal ? chain.nInternalChainCounter : chain.nExternalChainCounter), (uint32_t)0);
+            WalletDescriptor w_desc(std::move(desc), 0, 0, chain_counter, 0, is_internal);
 
             // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
             auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size));
@@ -1918,7 +1919,7 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
             desc->Expand(0, provider, desc_spks, provider);
         } else {
             // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
-            WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
+            WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0, /*_internal=*/false);
             auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size));
             for (const auto& keyid : privkeyids) {
                 CKey key;

src/wallet/spend.cpp

@@ -479,7 +479,7 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint&
 
     const CTransaction* ptx = wtx->tx.get();
     int n = outpoint.n;
-    while (OutputIsChange(wallet, ptx->vout[n]) && ptx->vin.size() > 0) {
+    while (IsOutputChange(wallet, *ptx, n) && ptx->vin.size() > 0) {
         const COutPoint& prevout = ptx->vin[0].prevout;
         const CWalletTx* it = wallet.GetWalletTx(prevout.hash);
         if (!it || it->tx->vout.size() <= prevout.n ||

src/wallet/test/fuzz/notifications.cpp

@@ -74,7 +74,7 @@ void ImportDescriptors(CWallet& wallet, const std::string& seed_insecure)
             assert(parsed_desc->IsRange());
             assert(parsed_desc->IsSingleType());
             assert(!keys.keys.empty());
-            WalletDescriptor w_desc{std::move(parsed_desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/0};
+            WalletDescriptor w_desc{std::move(parsed_desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/0, /*_internal=*/internal};
             assert(!wallet.GetDescriptorScriptPubKeyMan(w_desc));
             LOCK(wallet.cs_wallet);
             auto spk_manager{wallet.AddWalletDescriptor(w_desc, keys, /*label=*/"", internal)};

src/wallet/test/fuzz/scriptpubkeyman.cpp

@@ -72,7 +72,7 @@ static std::optional<std::pair<WalletDescriptor, FlatSigningProvider>> CreateWal
     std::unique_ptr<Descriptor> parsed_desc{Parse(desc_str.value(), keys, error, false)};
     if (!parsed_desc) return std::nullopt;
 
-    WalletDescriptor w_desc{std::move(parsed_desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/1};
+    WalletDescriptor w_desc{std::move(parsed_desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/1, /*_internal=*/false};
     return std::make_pair(w_desc, keys);
 }
 

src/wallet/test/ismine_tests.cpp

@@ -31,7 +31,7 @@ wallet::ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string&
 
     const int64_t range_start = 0, range_end = 1, next_index = 0, timestamp = 1;
 
-    WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index);
+    WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index, /*_internal=*/false);
 
     LOCK(keystore.cs_wallet);
 

src/wallet/test/psbt_wallet_tests.cpp

@@ -22,7 +22,7 @@ static void import_descriptor(CWallet& wallet, const std::string& descriptor)
     std::string error;
     std::unique_ptr<Descriptor> desc = Parse(descriptor, provider, error, /* require_checksum=*/ false);
     assert(desc);
-    WalletDescriptor w_desc(std::move(desc), 0, 0, 10, 0);
+    WalletDescriptor w_desc(std::move(desc), 0, 0, 10, 0, /*_internal=*/false);
     wallet.AddWalletDescriptor(w_desc, provider, "", false);
 }
 

src/wallet/test/util.cpp

@@ -33,7 +33,7 @@ std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cc
         std::string error;
         std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", provider, error, /* require_checksum=*/ false);
         assert(desc);
-        WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
+        WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1, /*_internal=*/false);
         if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
     }
     WalletRescanReserver reserver(*wallet);

src/wallet/test/wallet_tests.cpp

@@ -67,7 +67,7 @@ static void AddKey(CWallet& wallet, const CKey& key)
     std::string error;
     std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", provider, error, /* require_checksum=*/ false);
     assert(desc);
-    WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
+    WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1, /*_internal=*/false);
     if (!wallet.AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
 }
 

src/wallet/test/wallet_transaction_tests.cpp

@@ -3,7 +3,13 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <wallet/transaction.h>
-
+#include <wallet/coincontrol.h>
+#include <key_io.h>
+#include <kernel/chain.h>
+#include <validation.h>
+#include <wallet/receive.h>
+#include <wallet/spend.h>
+#include <wallet/test/util.h>
 #include <wallet/test/wallet_test_fixture.h>
 
 #include <boost/test/unit_test.hpp>
@@ -11,6 +17,371 @@
 namespace wallet {
 BOOST_FIXTURE_TEST_SUITE(wallet_transaction_tests, WalletTestingSetup)
 
+// Create a descriptor wallet synced with the current chain.
+// The wallet will have all the coinbase txes created up to block 110.
+// So, only 10 UTXO available for spending. The other 100 UTXO immature.
+std::unique_ptr<CWallet> CreateDescriptorWallet(TestChain100Setup* context)
+{
+    std::unique_ptr<CWallet> wallet = CreateSyncedWallet(*context->m_node.chain, WITH_LOCK(::cs_main, return context->m_node.chainman->ActiveChain()), context->coinbaseKey);
+    for (int i=0; i<10; i++) {
+        const CBlock& block = context->CreateAndProcessBlock({}, GetScriptForDestination(PKHash(context->coinbaseKey.GetPubKey().GetID())));
+        wallet->blockConnected(ChainstateRole::NORMAL, kernel::MakeBlockInfo(WITH_LOCK(::cs_main, return context->m_node.chainman->ActiveChain().Tip()), &block));
+    }
+    BOOST_REQUIRE(WITH_LOCK(wallet->cs_wallet, return AvailableCoins(*wallet).GetTotalAmount()) == 50 * COIN * 10);
+    return wallet;
+}
+
+// Create a legacy wallet synced with the current chain.
+// The wallet will have all the coinbase txes created up to block 110.
+// So, only 10 UTXO available for spending. The other 100 UTXO immature.
+std::unique_ptr<CWallet> CreateLegacyWallet(TestChain100Setup* context, WalletFeature wallet_feature, CKey coinbase_key, bool gen_blocks)
+{
+    auto wallet = std::make_unique<CWallet>(context->m_node.chain.get(), "", CreateMockableWalletDatabase());
+    {
+        LOCK2(wallet->cs_wallet, ::cs_main);
+        const auto& active_chain = context->m_node.chainman->ActiveChain();
+        wallet->SetLastBlockProcessed(active_chain.Height(), active_chain.Tip()->GetBlockHash());
+        BOOST_REQUIRE(wallet->LoadWallet() == DBErrors::LOAD_OK);
+        wallet->SetMinVersion(wallet_feature);
+        wallet->SetupLegacyScriptPubKeyMan();
+        BOOST_CHECK(wallet->GetLegacyScriptPubKeyMan()->SetupGeneration());
+
+        std::map<CKeyID, CKey> privkey_map;
+        privkey_map.insert(std::make_pair(coinbase_key.GetPubKey().GetID(), coinbase_key));
+        BOOST_REQUIRE(wallet->ImportPrivKeys(privkey_map, 0));
+
+        WalletRescanReserver reserver(*wallet);
+        reserver.reserve();
+        CWallet::ScanResult result = wallet->ScanForWalletTransactions(
+                active_chain.Genesis()->GetBlockHash(), /*start_height=*/0, /*max_height=*/{}, reserver, /*fUpdate=*/
+                false, /*save_progress=*/false);
+        BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
+        BOOST_CHECK_EQUAL(result.last_scanned_block, active_chain.Tip()->GetBlockHash());
+        BOOST_CHECK_EQUAL(*result.last_scanned_height, active_chain.Height());
+        BOOST_CHECK(result.last_failed_block.IsNull());
+    }
+    // Gen few more blocks to have available balance
+    if (gen_blocks) {
+        for (int i = 0; i < 15; i++) {
+            const CBlock& block = context->CreateAndProcessBlock({}, GetScriptForDestination(PKHash(context->coinbaseKey.GetPubKey().GetID())));
+            wallet->blockConnected(ChainstateRole::NORMAL, kernel::MakeBlockInfo(WITH_LOCK(::cs_main, return context->m_node.chainman->ActiveChain().Tip()), &block));
+        }
+    }
+    return wallet;
+}
+
+std::unique_ptr<CWallet> CreateEmtpyWallet(TestChain100Setup* context, bool only_legacy, WalletFeature legacy_wallet_features = FEATURE_BASE)
+{
+    CKey random_key;
+    random_key.MakeNewKey(true);
+    if (only_legacy) return CreateLegacyWallet(context, legacy_wallet_features, random_key, /*gen_blocks=*/false);
+    else return CreateSyncedWallet(*context->m_node.chain,
+                                   WITH_LOCK(Assert(context->m_node.chainman)->GetMutex(), return context->m_node.chainman->ActiveChain()), random_key);
+}
+
+bool OutputIsChange(const std::unique_ptr<CWallet>& wallet, const CTransactionRef& tx, unsigned int change_pos)
+{
+    return WITH_LOCK(wallet->cs_wallet, return IsOutputChange(*wallet, *tx, change_pos));
+}
+
+// Creates a transaction and adds it to the wallet via the mempool signal
+CreatedTransactionResult CreateAndAddTx(const std::unique_ptr<CWallet>& wallet, const CTxDestination& dest, CAmount amount)
+{
+    CCoinControl coin_control;
+    auto op_tx = *Assert(CreateTransaction(*wallet, {{dest, amount, true}}, /*change_pos=*/std::nullopt, coin_control));
+    // Prior to adding it to the wallet, check if the wallet can detect the change script
+    BOOST_CHECK(OutputIsChange(wallet, op_tx.tx, *op_tx.change_pos));
+    wallet->transactionAddedToMempool(op_tx.tx);
+    return op_tx;
+}
+
+void CreateTxAndVerifyChange(const std::unique_ptr<CWallet>& wallet, const std::unique_ptr<CWallet>& external_wallet, OutputType dest_type, CAmount dest_amount)
+{
+    CTxDestination dest = *Assert(external_wallet->GetNewDestination(dest_type, ""));
+    auto res = CreateAndAddTx(wallet, dest, dest_amount);
+    assert(res.tx->vout.size() == 2); // always expect 2 outputs (destination and change).
+    unsigned int change_pos = *res.change_pos;
+    BOOST_CHECK(res.tx->vout.at(change_pos).nValue == (50 * COIN - dest_amount));
+    BOOST_CHECK(OutputIsChange(wallet, res.tx, change_pos));
+    BOOST_CHECK(!OutputIsChange(wallet, res.tx, !change_pos));
+}
+
+std::unique_ptr<Descriptor> CreateSingleKeyDescriptor(const std::string& type, FlatSigningProvider& provider)
+{
+    CKey seed;
+    seed.MakeNewKey(true);
+    CExtKey master_key;
+    master_key.SetSeed(seed);
+    std::string descriptor = type + "(" + EncodeExtKey(master_key) + ")";
+    std::string error;
+    std::unique_ptr<Descriptor> desc = Parse(descriptor, provider, error);
+    BOOST_REQUIRE(desc);
+    return desc;
+}
+
+/**
+ * Test descriptor wallet change output detection.
+ *
+ * Context:
+ * A descriptor wallet with legacy support is created ("local" wallet from now on).
+ * A second wallet that provides external destinations is created ("external" wallet from now on).
+ *
+ * General test concept:
+ * Create transactions using the local wallet to different destinations provided by the external wallet.
+ * Verifying that the wallet can detect the change output prior and post the tx is added to the wallet.
+ *
+ * The following cases are covered:
+ * 1) Create tx that sends to a legacy p2pkh addr and verify change detection.
+ * 2) Create tx that sends to a p2wpkh addr and verify change detection.
+ * 3) Create tx that sends to a wrapped p2wpkh addr and verify change detection.
+ * 4) Create tx that sends to a taproot addr and verify change detection.
+ */
+BOOST_FIXTURE_TEST_CASE(descriptors_wallet_detect_change_output, TestChain100Setup)
+{
+    // Create wallet, 10 UTXO spendable, 100 UTXO immature.
+    std::unique_ptr<CWallet> wallet = CreateDescriptorWallet(this);
+    wallet->SetupLegacyScriptPubKeyMan();
+    // External wallet mimicking another user
+    std::unique_ptr<CWallet> external_wallet = CreateEmtpyWallet(this, /*only_legacy=*/false);
+    external_wallet->SetupLegacyScriptPubKeyMan();
+
+    // 1) send to legacy p2pkh and verify that the wallet detects the change output
+    // 2) send to p2wpkh and verify that the wallet detects the change output
+    // 3) send to a wrapped p2wpkh and verify that the wallet detects the change output
+    // 4) send to a taproot and verify that the wallet detects the change output
+    for (const auto& dest_type : {OutputType::LEGACY, OutputType::BECH32, OutputType::P2SH_SEGWIT, OutputType::BECH32M}) {
+        CreateTxAndVerifyChange(wallet, external_wallet, dest_type, 15 * COIN);
+    }
+
+    {
+        // Inactive descriptors tests
+
+        // 1) Verify importing a not-ranged internal descriptor, then create a tx and send coins to it.
+        // the wallet should be able to detect inactive descriptors as change.
+        FlatSigningProvider provider;
+        std::unique_ptr<Descriptor> desc = CreateSingleKeyDescriptor("pkh", provider);
+        WalletDescriptor wdesc(std::move(desc), 0, 0, 0, 0, /*_internal=*/true);
+        ScriptPubKeyMan* spkm = WITH_LOCK(wallet->cs_wallet, return wallet->AddWalletDescriptor(wdesc, provider, "", /*internal=*/true));
+
+        CTxDestination dest_change;
+        BOOST_CHECK(ExtractDestination(*spkm->GetScriptPubKeys().begin(), dest_change));
+
+        CCoinControl coin_control;
+        coin_control.destChange = dest_change;
+        auto op_tx = *Assert(CreateTransaction(*wallet, {{CNoDestination{CScript() << OP_TRUE}, 1 * COIN, true}}, /*change_pos=*/std::nullopt, coin_control));
+        BOOST_CHECK(OutputIsChange(wallet, op_tx.tx, *op_tx.change_pos));
+
+
+        // 2) Verify importing a not-ranged descriptor that contains the key of an output already stored in the wallet.
+        //    The wallet should process the import and detect the output as change.
+        FlatSigningProvider provider2;
+        std::unique_ptr<Descriptor> desc2 = CreateSingleKeyDescriptor("wpkh", provider2);
+        std::vector<CScript> scripts;
+        FlatSigningProvider out_provider;
+        BOOST_CHECK(desc2->Expand(/*pos=*/0, provider2, scripts, out_provider));
+        BOOST_CHECK(ExtractDestination(scripts[0], dest_change));
+
+        coin_control.destChange = dest_change;
+        auto op_tx2 = *Assert(CreateTransaction(*wallet, {{CNoDestination{CScript() << OP_TRUE}, 1 * COIN, true}}, /*change_pos=*/std::nullopt, coin_control));
+        BOOST_CHECK(!OutputIsChange(wallet, op_tx2.tx, *op_tx2.change_pos));
+
+        // Now add the descriptor and verify that change is detected
+        WalletDescriptor wdesc2(std::move(desc2), 0, 0, 0, 0, /*_internal=*/true);
+        WITH_LOCK(wallet->cs_wallet, return wallet->AddWalletDescriptor(wdesc2, provider2, "", /*internal=*/true));
+        BOOST_CHECK(OutputIsChange(wallet, op_tx2.tx, *op_tx2.change_pos));
+    }
+
+
+    // ### Now verify the negative paths:
+
+    {
+        // 1) The change output goes to an address in one of the wallet external path
+        CCoinControl coin_control;
+        coin_control.destChange = *Assert(wallet->GetNewDestination(OutputType::BECH32, ""));
+        auto op_tx = *Assert(CreateTransaction(*wallet, {{CNoDestination{CScript() << OP_TRUE}, 1 * COIN, true}}, /*change_pos=*/std::nullopt, coin_control));
+        BOOST_CHECK(!OutputIsChange(wallet, op_tx.tx, *op_tx.change_pos));
+        wallet->transactionAddedToMempool(op_tx.tx);
+        auto wtx = WITH_LOCK(wallet->cs_wallet, return wallet->GetWalletTx(op_tx.tx->GetHash()));
+        assert(wtx);
+        BOOST_CHECK(!OutputIsChange(wallet, wtx->tx, *op_tx.change_pos));
+    }
+
+    {
+        // 2) The change goes back to the source.
+        // As the source is an external destination, and we are currently detecting change through it output script, the change will be marked as external (not change).
+
+        // Create source address
+        auto dest = *Assert(wallet->GetNewDestination(OutputType::BECH32, ""));
+        auto op_tx_source = CreateAndAddTx(wallet, dest, 15 * COIN);
+        const CBlock& block = CreateAndProcessBlock({CMutableTransaction(*op_tx_source.tx)}, GetScriptForDestination(PKHash(coinbaseKey.GetPubKey().GetID())));
+        wallet->blockConnected(ChainstateRole::NORMAL, kernel::MakeBlockInfo(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip()), &block));
+
+        // Now create the tx that spends from the source and sends the change to it.
+        LOCK(wallet->cs_wallet);
+        CCoinControl coin_control;
+        unsigned int change_pos = *op_tx_source.change_pos;
+        coin_control.Select(COutPoint(op_tx_source.tx->GetHash(), !change_pos));
+        coin_control.destChange = dest;
+        auto op_tx = *Assert(CreateTransaction(*wallet, {{CNoDestination{CScript() << OP_TRUE}, 1 * COIN, true}}, /*change_pos=*/std::nullopt, coin_control));
+        BOOST_CHECK(!OutputIsChange(wallet, op_tx.tx, *op_tx.change_pos));
+        wallet->transactionAddedToMempool(op_tx.tx);
+        auto wtx = wallet->GetWalletTx(op_tx.tx->GetHash());
+        assert(wtx);
+        BOOST_CHECK(!OutputIsChange(wallet, wtx->tx, *op_tx.change_pos));
+    }
+
+    {
+        // 3) Test what happens when the user sets an address book label to a destination created from an internal key.
+        auto op_tx = CreateAndAddTx(wallet, *Assert(wallet->GetNewDestination(OutputType::BECH32, "")), 15 * COIN);
+        BOOST_CHECK(OutputIsChange(wallet, op_tx.tx, *op_tx.change_pos));
+        // Now change the change destination label
+        CTxDestination change_dest;
+        BOOST_CHECK(ExtractDestination(op_tx.tx->vout.at(*op_tx.change_pos).scriptPubKey, change_dest));
+        BOOST_CHECK(wallet->SetAddressBook(change_dest, "not_a_change_address", AddressPurpose::RECEIVE));
+        // TODO: FIXME, Currently, change detection fails when the user sets a custom label to a destination created from an internal path.
+        BOOST_CHECK(OutputIsChange(wallet, op_tx.tx, *op_tx.change_pos));
+    }
+
+    {
+        // 4) Test coins reception into an internal address directly.
+        //    As the wallet is receiving those coins from outside, them should not be detected as change.
+
+        // Add balance to external wallet
+        auto op_funding_tx = CreateAndAddTx(wallet, *external_wallet->GetNewDestination(OutputType::BECH32, ""), 15 * COIN);
+        const CBlock& block = CreateAndProcessBlock({CMutableTransaction(*op_funding_tx.tx)}, GetScriptForDestination(PKHash(coinbaseKey.GetPubKey().GetID())));
+        auto block_info = kernel::MakeBlockInfo(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip()), &block);
+        for (const auto& it_wallet : {wallet.get(), external_wallet.get()}) it_wallet->blockConnected(ChainstateRole::NORMAL, block_info);
+
+        // Send coins to the local wallet internal address directly.
+        auto op_tx = CreateAndAddTx(external_wallet, *Assert(wallet->GetNewChangeDestination(OutputType::BECH32)), 10 * COIN);
+        BOOST_CHECK(!OutputIsChange(wallet, op_tx.tx, *op_tx.change_pos));
+    }
+}
+
+CTxDestination deriveDestination(const std::unique_ptr<Descriptor>& descriptor, int index, const FlatSigningProvider& privkey_provider)
+{
+    FlatSigningProvider provider;
+    std::vector<CScript> scripts;
+    BOOST_CHECK(descriptor->Expand(index, privkey_provider, scripts, provider));
+    CTxDestination dest;
+    BOOST_CHECK(ExtractDestination(scripts[0], dest));
+    return dest;
+}
+
+/**
+ * The old change detection process assumption can easily be broken by creating an address manually,
+ * from an external derivation path, and send coins to it. As the address was not created by the
+ * wallet, it will not be in the addressbook, there by will be treated as change when it's clearly not.
+ *
+ * The wallet will properly detect it once the transaction gets added to the wallet and the address
+ * (and all the previous unused address) are derived and stored in the address book.
+ */
+BOOST_FIXTURE_TEST_CASE(external_tx_creation_change_output_detection, TestChain100Setup)
+{
+    // Create miner wallet, 10 UTXO spendable, 100 UTXO immature.
+    std::unique_ptr<CWallet> external_wallet = CreateDescriptorWallet(this);
+    external_wallet->SetupLegacyScriptPubKeyMan();
+    // Local wallet
+    std::unique_ptr<CWallet> wallet = CreateEmtpyWallet(this, /*only_legacy=*/false);
+    wallet->SetupLegacyScriptPubKeyMan();
+
+    auto external_desc_spkm = static_cast<DescriptorScriptPubKeyMan*>(wallet->GetScriptPubKeyMan(OutputType::BECH32, /*internal=*/false));
+    std::string desc_str;
+    BOOST_CHECK(external_desc_spkm->GetDescriptorString(desc_str, true));
+    FlatSigningProvider key_provider;
+    std::string error;
+    std::unique_ptr<Descriptor> descriptor = Assert(Parse(desc_str, key_provider, error, /*require_checksum=*/false));
+
+    {
+        // Now derive a key at an index that wasn't created by the wallet and send coins to it
+        // TODO: Test this same point on a legacy wallet.
+        int addr_index = 100;
+        const CTxDestination& dest = deriveDestination(descriptor, addr_index, key_provider);
+        CAmount dest_amount = 1 * COIN;
+        CCoinControl coin_control;
+        auto res_tx = CreateTransaction(*external_wallet, {{dest, dest_amount, true}},
+                                               /*change_pos=*/std::nullopt, coin_control);
+        assert(res_tx);
+        assert(res_tx->tx->vout.size() == 2); // dest + change
+        unsigned int change_pos = *res_tx->change_pos;
+        unsigned int pos_not_change = !change_pos;
+
+        // Prior to adding the tx to the local wallet, check if can detect the output script
+        const CTxOut& output = res_tx->tx->vout.at(pos_not_change);
+        BOOST_CHECK(output.nValue == dest_amount);
+        BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsMine(output)));
+        // ----> todo: currently the wallet invalidly detects the external receive address as change when it is not.
+        //             this is due an non-existent address book record.
+        BOOST_CHECK(!OutputIsChange(wallet, res_tx->tx, pos_not_change));
+
+        // Now add it to the wallet and verify that is not invalidly marked as change
+        wallet->transactionAddedToMempool(res_tx->tx);
+        const CWalletTx* wtx = Assert(WITH_LOCK(wallet->cs_wallet, return wallet->GetWalletTx(res_tx->tx->GetHash())));
+        BOOST_CHECK(!OutputIsChange(wallet, wtx->tx, pos_not_change));
+        BOOST_CHECK_EQUAL(TxGetChange(*wallet, *res_tx->tx), 0);
+    }
+
+    {
+        // Now check the wallet limitations by sending coins to an address that is above the keypool size
+        int addr_index = DEFAULT_KEYPOOL_SIZE + 300;
+        const CTxDestination& dest = deriveDestination(descriptor, addr_index, key_provider);
+        CAmount dest_amount = 2 * COIN;
+        CCoinControl coin_control;
+        auto op_tx = *Assert(CreateTransaction(*external_wallet, {{dest, dest_amount, true}}, /*change_pos=*/std::nullopt, coin_control));
+
+        // Prior to adding the tx to the local wallet, check if can detect the output script
+        unsigned int change_pos = *op_tx.change_pos;
+        unsigned int pos_not_change = !change_pos;
+        const CTxOut& output = op_tx.tx->vout.at(pos_not_change);
+        BOOST_CHECK(output.nValue == dest_amount);
+
+        // As this address is above our keypool, we are not able to consider it from the wallet!
+        BOOST_CHECK(!WITH_LOCK(wallet->cs_wallet, return wallet->IsMine(output)));
+        BOOST_CHECK(!OutputIsChange(wallet, op_tx.tx, pos_not_change));
+
+        const CBlock& block = CreateAndProcessBlock({CMutableTransaction(*op_tx.tx)}, GetScriptForDestination(PKHash(coinbaseKey.GetPubKey().GetID())));
+        wallet->blockConnected(ChainstateRole::NORMAL, kernel::MakeBlockInfo(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip()), &block));
+
+        const CWalletTx* wtx = WITH_LOCK(wallet->cs_wallet, return wallet->GetWalletTx(op_tx.tx->GetHash()));
+        BOOST_CHECK(!wtx); // --> the transaction is not added.
+    }
+}
+
+/**
+ * Test legacy-only wallet change output detection.
+ *
+ * Context:
+ * A legacy-only wallet is created ("local" wallet from now on).
+ * A second wallet that provides external destinations is created ("external" wallet from now on).
+ *
+ * General test concept:
+ * Create transactions using the local wallet to different destinations provided by the external wallet.
+ * Verifying that the wallet can detect the change output prior and post the tx is added to the wallet.
+ *
+ * The following cases are covered:
+ * 1) Create tx that sends to a legacy p2pkh addr and verify change detection.
+ * 2) Create tx that sends to a p2wpkh addr and verify change detection.
+ * 3) Create tx that sends to a wrapped p2wpkh addr and verify change detection.
+ */
+BOOST_FIXTURE_TEST_CASE(legacy_wallet_detect_change_output, TestChain100Setup)
+{
+    // FEATURE_HD doesn't create the internal derivation path, all the addresses are external
+    // FEATURE_HD_SPLIT creates internal and external paths.
+    for (WalletFeature feature : {FEATURE_HD, FEATURE_HD_SPLIT}) {
+        // Create wallet, 10 UTXO spendable, 100 UTXO immature.
+        std::unique_ptr<CWallet> wallet = CreateLegacyWallet(this, feature, coinbaseKey, /*gen_blocks=*/true);
+        // External wallet mimicking another user
+        std::unique_ptr<CWallet> external_wallet = CreateEmtpyWallet(this, /*only_legacy=*/true, feature);
+
+        // 1) Create a transaction that sends to a legacy p2pkh and verify that the wallet detects the change output
+        // 2) Create a transaction that sends to a p2wpkh and verify that the wallet detects the change output
+        // 3) Create a transaction that sends to a wrapped p2wpkh and verify that the wallet detects the change output
+        for (const auto& dest_type : {OutputType::LEGACY, OutputType::BECH32, OutputType::P2SH_SEGWIT}) {
+            CreateTxAndVerifyChange(wallet, external_wallet, dest_type, 10 * COIN);
+        }
+    }
+}
+
 BOOST_AUTO_TEST_CASE(roundtrip)
 {
     for (uint8_t hash = 0; hash < 5; ++hash) {

src/wallet/test/walletload_tests.cpp

@@ -44,7 +44,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)
         // Write unknown active descriptor
         WalletBatch batch(*database, false);
         std::string unknown_desc = "trx(tpubD6NzVbkrYhZ4Y4S7m6Y5s9GD8FqEMBy56AGphZXuagajudVZEnYyBahZMgHNCTJc2at82YX6s8JiL1Lohu5A3v1Ur76qguNH4QVQ7qYrBQx/86'/1'/0'/0/*)#8pn8tzdt";
-        WalletDescriptor wallet_descriptor(std::make_shared<DummyDescriptor>(unknown_desc), 0, 0, 0, 0);
+        WalletDescriptor wallet_descriptor(std::make_shared<DummyDescriptor>(unknown_desc), 0, 0, 0, 0, /*_internal*/false);
         BOOST_CHECK(batch.WriteDescriptor(uint256(), wallet_descriptor));
         BOOST_CHECK(batch.WriteActiveScriptPubKeyMan(static_cast<uint8_t>(OutputType::UNKNOWN), uint256(), false));
     }
@@ -71,7 +71,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)
         // Write valid descriptor with invalid ID
         WalletBatch batch(*database, false);
         std::string desc = "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#cjjspncu";
-        WalletDescriptor wallet_descriptor(std::make_shared<DummyDescriptor>(desc), 0, 0, 0, 0);
+        WalletDescriptor wallet_descriptor(std::make_shared<DummyDescriptor>(desc), 0, 0, 0, 0, /*_internal*/false);
         BOOST_CHECK(batch.WriteDescriptor(uint256::ONE, wallet_descriptor));
     }
 

src/wallet/wallet.cpp

@@ -3508,13 +3508,30 @@ bool CWallet::IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const
     return false;
 }
 
-std::set<ScriptPubKeyMan*> CWallet::GetAllScriptPubKeyMans() const
+std::set<ScriptPubKeyMan*> CWallet::GetAllScriptPubKeyMans(bool only_internal) const
 {
     std::set<ScriptPubKeyMan*> spk_mans;
-    for (const auto& spk_man_pair : m_spk_managers) {
-        spk_mans.insert(spk_man_pair.second.get());
+    if (only_internal) {
+        for (const auto& spk_man_pair : m_internal_spk_managers) {
+            spk_mans.insert(spk_man_pair.second);
+        }
+
+        for (const auto& spk_man_pair : m_spk_managers) {
+            if (const auto& desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man_pair.second.get())) {
+                LOCK(desc_spk_man->cs_desc_man);
+                if (desc_spk_man->GetWalletDescriptor().internal) {
+                    spk_mans.insert(desc_spk_man);
+                }
+            }
+        }
+
+        return spk_mans;
+    } else {
+        for (const auto &spk_man_pair: m_spk_managers) {
+            spk_mans.insert(spk_man_pair.second.get());
+        }
+        return spk_mans;
     }
-    return spk_mans;
 }
 
 ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const OutputType& type, bool internal) const
@@ -3855,7 +3872,7 @@ std::optional<bool> CWallet::IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man)
 ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal)
 {
     AssertLockHeld(cs_wallet);
-
+    desc.internal = internal;
     if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
         WalletLogPrintf("Cannot add WalletDescriptor to a non-descriptor wallet\n");
         return nullptr;
@@ -4281,7 +4298,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
                 assert(!desc->IsRange()); // It shouldn't be possible to have LegacyScriptPubKeyMan make a ranged watchonly descriptor
 
                 // Add to the wallet
-                WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
+                WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0, /*_internal=*/false);
                 data->watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false);
             }
 
@@ -4318,7 +4335,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
                 assert(!desc->IsRange()); // It shouldn't be possible to have LegacyScriptPubKeyMan make a ranged watchonly descriptor
 
                 // Add to the wallet
-                WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
+                WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0, /*_internal=*/false);
                 data->solvable_wallet->AddWalletDescriptor(w_desc, keys, "", false);
             }
 

src/wallet/wallet.h

@@ -941,7 +941,7 @@ public:
     bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const;
 
     //! Returns all unique ScriptPubKeyMans
-    std::set<ScriptPubKeyMan*> GetAllScriptPubKeyMans() const;
+    std::set<ScriptPubKeyMan*> GetAllScriptPubKeyMans(bool only_internal=false) const;
 
     //! Get the ScriptPubKeyMan for the given OutputType and internal/external chain.
     ScriptPubKeyMan* GetScriptPubKeyMan(const OutputType& type, bool internal) const;

src/wallet/walletutil.cpp

@@ -95,7 +95,7 @@ WalletDescriptor GenerateWalletDescriptor(const CExtPubKey& master_key, const Ou
     FlatSigningProvider keys;
     std::string error;
     std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);
-    WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
+    WalletDescriptor w_desc(std::move(desc), creation_time, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0, /*_internal=*/internal);
     return w_desc;
 }
 

src/wallet/walletutil.h

@@ -91,6 +91,7 @@ public:
     int32_t range_end = 0; // Item after the last; end of range, exclusive, i.e. [range_start, range_end). This will increment with each TopUp()
     int32_t next_index = 0; // Position of the next item to generate
     DescriptorCache cache;
+    bool internal{false}; // Used for change or not
 
     void DeserializeDescriptor(const std::string& str)
     {
@@ -109,10 +110,16 @@ public:
         SER_WRITE(obj, descriptor_str = obj.descriptor->ToString());
         READWRITE(descriptor_str, obj.creation_time, obj.next_index, obj.range_start, obj.range_end);
         SER_READ(obj, obj.DeserializeDescriptor(descriptor_str));
+
+        try {
+            READWRITE(obj.internal);
+        } catch (...) {
+            // swallow it
+        }
     }
 
     WalletDescriptor() {}
-    WalletDescriptor(std::shared_ptr<Descriptor> descriptor, uint64_t creation_time, int32_t range_start, int32_t range_end, int32_t next_index) : descriptor(descriptor), id(DescriptorID(*descriptor)), creation_time(creation_time), range_start(range_start), range_end(range_end), next_index(next_index) { }
+    WalletDescriptor(std::shared_ptr<Descriptor> descriptor, uint64_t creation_time, int32_t range_start, int32_t range_end, int32_t next_index, bool _internal) : descriptor(descriptor), id(DescriptorID(*descriptor)), creation_time(creation_time), range_start(range_start), range_end(range_end), next_index(next_index), internal(_internal) { }
 };
 
 WalletDescriptor GenerateWalletDescriptor(const CExtPubKey& master_key, const OutputType& output_type, bool internal);

test/functional/wallet_basic.py

@@ -668,13 +668,14 @@ class WalletTest(BitcoinTestFramework):
             assert_equal(ischange, address != destination)
             if ischange:
                 change = address
+        # Internal address is detected even if it has a label
         self.nodes[0].setlabel(change, 'foobar')
-        assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)
+        assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], True)
 
         # Test gettransaction response with different arguments.
         self.log.info("Testing gettransaction response with different arguments...")
         self.nodes[0].setlabel(change, 'baz')
-        baz = self.nodes[0].listtransactions(label="baz", count=1)[0]
+        baz = self.nodes[0].listtransactions(label="baz", count=1, include_change=True)[0]
         expected_receive_vout = {"label":    "baz",
                                  "address":  baz["address"],
                                  "amount":   baz["amount"],
@@ -686,17 +687,17 @@ class WalletTest(BitcoinTestFramework):
         expected_verbose_fields = expected_fields | {verbose_field}
 
         self.log.debug("Testing gettransaction response without verbose")
-        tx = self.nodes[0].gettransaction(txid=txid)
+        tx = self.nodes[0].gettransaction(txid=txid, include_change=True)
         assert_equal(set([*tx]), expected_fields)
         assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
 
         self.log.debug("Testing gettransaction response with verbose set to False")
-        tx = self.nodes[0].gettransaction(txid=txid, verbose=False)
+        tx = self.nodes[0].gettransaction(txid=txid, verbose=False, include_change=True)
         assert_equal(set([*tx]), expected_fields)
         assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
 
         self.log.debug("Testing gettransaction response with verbose set to True")
-        tx = self.nodes[0].gettransaction(txid=txid, verbose=True)
+        tx = self.nodes[0].gettransaction(txid=txid, verbose=True, include_change=True)
         assert_equal(set([*tx]), expected_verbose_fields)
         assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
         assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))