From dc4e364fda73b11b57c83ae0fe79568148b2350d Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Mon, 23 May 2022 12:08:22 -0400
Subject: [PATCH 01/11] Merge bitcoin/bitcoin#25122: rpc: getreceivedbylabel,
 return early if no addresses were found in the address book

baa3ddc49c46d00e3e0de06e494656f0f00b0ee8 doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy)
8897a21658ad93f7b628eb2a3411fec2265d73fb rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy)

Pull request description:

  Built on top of #23662, coming from comment https://github.com/bitcoin/bitcoin/pull/23662#pullrequestreview-971407999.

  If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
  Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty).

ACKs for top commit:
  achow101:
    ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8
  theStack:
    re-ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8
  w0xlt:
    ACK https://github.com/bitcoin/bitcoin/pull/25122/commits/baa3ddc49c46d00e3e0de06e494656f0f00b0ee8

Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911

Co-authored-by: Andrew Chow <achow101-github@achow101.com>
---
 doc/release-notes-25122.md               |  7 ++++++
 src/wallet/rpc/coins.cpp                 | 28 ++++++++++++++----------
 test/functional/wallet_listreceivedby.py |  3 +++
 3 files changed, 26 insertions(+), 12 deletions(-)
 create mode 100644 doc/release-notes-25122.md

diff --git a/doc/release-notes-25122.md b/doc/release-notes-25122.md
new file mode 100644
index 000000000000..5c5f0f911154
--- /dev/null
+++ b/doc/release-notes-25122.md
@@ -0,0 +1,7 @@
+
+Wallet
+------
+
+- RPC `getreceivedbylabel` now returns an error, "Label not found
+  in wallet" (-4), if the label is not in the address book. (dash#7550)
+
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index 845d8003230c..2f0dcbd6c67b 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -17,28 +17,32 @@
 namespace wallet {
 static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
-    std::vector<CScript> output_scripts;
+    std::vector<CTxDestination> addresses;
 
     if (by_label) {
         // Get the set of addresses assigned to label
-        std::string label = LabelFromValue(params[0]);
-        for (const auto& address : wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{label})) {
-            auto output_script{GetScriptForDestination(address)};
-            if (wallet.IsMine(output_script)) {
-                output_scripts.emplace_back(output_script);
-            }
-        }
+        addresses = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{LabelFromValue(params[0])});
+        if (addresses.empty()) throw JSONRPCError(RPC_WALLET_ERROR, "Label not found in wallet");
     } else {
         // Get the address
         CTxDestination dest = DecodeDestination(params[0].get_str());
         if (!IsValidDestination(dest)) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Dash address");
         }
-        CScript script_pub_key = GetScriptForDestination(dest);
-        if (!wallet.IsMine(script_pub_key)) {
-            throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet");
+        addresses.emplace_back(dest);
+    }
+
+    // Filter by own scripts only
+    std::vector<CScript> output_scripts;
+    for (const auto& address : addresses) {
+        auto output_script{GetScriptForDestination(address)};
+        if (wallet.IsMine(output_script)) {
+            output_scripts.emplace_back(output_script);
         }
-        output_scripts.emplace_back(script_pub_key);
+    }
+
+    if (output_scripts.empty()) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet");
     }
 
     // Minimum confirmations
diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py
index f3e3fd001dfc..b172029c04e7 100755
--- a/test/functional/wallet_listreceivedby.py
+++ b/test/functional/wallet_listreceivedby.py
@@ -139,6 +139,9 @@ def run_test(self):
         txid = self.nodes[0].sendtoaddress(addr, 0.1)
         self.sync_all()
 
+        # getreceivedbylabel returns an error if the wallet doesn't own the label
+        assert_raises_rpc_error(-4, "Label not found in wallet", self.nodes[0].getreceivedbylabel, "dummy")
+
         # listreceivedbylabel should return received_by_label_json because of 0 confirmations
         assert_array_result(self.nodes[1].listreceivedbylabel(),
                             {"label": label},

From 88be18919b519acede47f854b6afc8452bdd2a42 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Tue, 16 Aug 2022 12:55:17 -0400
Subject: [PATCH 02/11] Merge bitcoin/bitcoin#25504: RPC: allow to track coins
 by parent descriptors

a6b0c1fcc06485ecd320727fa7534a51a20608c1 doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot)
0fd2d144540b720626fc065a3cef5188831b5ee2 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot)
55f98d087efd2609d808c082d5770306cc489409 rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot)
b724476158a7dfeef9edfda3f519dfd6f93202a8 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot)
55a82eaf91d252a04a0cc8ad7d948d956c6cb24f wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot)

Pull request description:

  Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends).

  It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit.

  I'll add this to the output of `listunspent` too if this gets a few Concept ACKs.

ACKs for top commit:
  instagibbs:
    ACK https://github.com/bitcoin/bitcoin/pull/25504/commits/a6b0c1fcc06485ecd320727fa7534a51a20608c1
  achow101:
    re-ACK a6b0c1fcc06485ecd320727fa7534a51a20608c1

Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37

Co-authored-by: Andrew Chow <achow101-github@achow101.com>
---
 doc/release-notes-25504.md               |  6 +++
 src/rpc/client.cpp                       |  1 +
 src/wallet/receive.cpp                   |  6 +--
 src/wallet/receive.h                     |  3 +-
 src/wallet/rpc/coins.cpp                 |  4 ++
 src/wallet/rpc/transactions.cpp          | 22 +++++++--
 src/wallet/rpc/util.cpp                  |  9 ++++
 src/wallet/rpc/util.h                    |  4 ++
 src/wallet/wallet.cpp                    | 12 +++++
 src/wallet/wallet.h                      |  3 ++
 test/functional/wallet_basic.py          | 33 +++++++++++++
 test/functional/wallet_listsinceblock.py | 62 +++++++++++++++++++++++-
 12 files changed, 155 insertions(+), 10 deletions(-)
 create mode 100644 doc/release-notes-25504.md

diff --git a/doc/release-notes-25504.md b/doc/release-notes-25504.md
new file mode 100644
index 000000000000..bf80f180318a
--- /dev/null
+++ b/doc/release-notes-25504.md
@@ -0,0 +1,6 @@
+Updated RPCs
+------------
+
+- The `listsinceblock`, `listtransactions` and `gettransaction` output now contain a new
+  `parent_descs` field for every "receive" entry.
+- A new optional `include_change` parameter was added to the `listsinceblock` command.
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 2579ed364274..972e99c94924 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -95,6 +95,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listsinceblock", 1, "target_confirmations" },
     { "listsinceblock", 2, "include_watchonly" },
     { "listsinceblock", 3, "include_removed" },
+    { "listsinceblock", 4, "include_change" },
     { "sendmany", 1, "amounts" },
     { "sendmany", 2, "minconf" },
     { "sendmany", 3, "addlocked" },
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
index 43bf3b633529..309c43450a0f 100644
--- a/src/wallet/receive.cpp
+++ b/src/wallet/receive.cpp
@@ -194,7 +194,8 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx,
 
 void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
                         std::list<COutputEntry>& listReceived,
-                        std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter)
+                        std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter,
+                        bool include_change)
 {
     nFee = 0;
     listReceived.clear();
@@ -219,8 +220,7 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
         //   2) the output is to us (received)
         if (nDebit > 0)
         {
-            // Don't report 'change' txouts
-            if (OutputIsChange(wallet, txout))
+            if (!include_change && OutputIsChange(wallet, txout))
                 continue;
         }
         else if (!(fIsMine & filter))
diff --git a/src/wallet/receive.h b/src/wallet/receive.h
index f19901794660..0acc4c6373e7 100644
--- a/src/wallet/receive.h
+++ b/src/wallet/receive.h
@@ -42,7 +42,8 @@ struct COutputEntry
 void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
                         std::list<COutputEntry>& listReceived,
                         std::list<COutputEntry>& listSent,
-                        CAmount& nFee, const isminefilter& filter);
+                        CAmount& nFee, const isminefilter& filter,
+                        bool include_change);
 bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter);
 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx);
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index 2f0dcbd6c67b..5a060111fb3a 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -592,6 +592,9 @@ RPCHelpMan listunspent()
                             {RPCResult::Type::BOOL, "solvable", "Whether we know how to spend this output, ignoring the lack of keys"},
                             {RPCResult::Type::STR, "desc", /*optional=*/true, "(only when solvable) A descriptor for spending this output"},
                             {RPCResult::Type::BOOL, "reused", /* optional*/ true, "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"},
+                            {RPCResult::Type::ARR, "parent_descs", /*optional=*/false, "List of parent descriptors for the scriptPubKey of this coin.", {
+                                {RPCResult::Type::STR, "desc", "The descriptor string."},
+                            }},
                             {RPCResult::Type::BOOL, "safe", "Whether this output is considered safe to spend. Unconfirmed transactions"
                                                             "from outside keys and unconfirmed replacement transactions are considered unsafe\n"
                                                             "and are not eligible for spending by fundrawtransaction and sendtoaddress."},
@@ -762,6 +765,7 @@ RPCHelpMan listunspent()
                 entry.pushKV("desc", descriptor->ToString());
             }
         }
+        PushParentDescriptors(*pwallet, scriptPubKey, entry);
         if (avoid_reuse) entry.pushKV("reused", reused);
         entry.pushKV("safe", out.safe);
         entry.pushKV("coinjoin_rounds", pwallet->GetRealOutpointCoinJoinRounds(out.outpoint));
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
index 5aec8db611de..a61780891a19 100644
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -324,13 +324,16 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
  * @param  filter_label   Optional label string to filter incoming transactions.
  */
 template <class Vec>
-static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong, Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
+static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong,
+                             Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label,
+                             bool include_change = false)
+    EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
     CAmount nFee;
     std::list<COutputEntry> listReceived;
     std::list<COutputEntry> listSent;
 
-    CachedTxGetAmounts(wallet, wtx, listReceived, listSent, nFee, filter_ismine);
+    CachedTxGetAmounts(wallet, wtx, listReceived, listSent, nFee, filter_ismine, include_change);
 
     bool involvesWatchonly = CachedTxIsFromMe(wallet, wtx, ISMINE_WATCH_ONLY);
 
@@ -377,6 +380,7 @@ static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nM
                 entry.pushKV("involvesWatchonly", true);
             }
             MaybePushAddress(entry, r.destination);
+            PushParentDescriptors(wallet, wtx.tx->vout.at(r.vout).scriptPubKey, entry);
             if (wtx.IsCoinBase())
             {
                 if (wallet.GetTxDepthInMainChain(wtx) < 1)
@@ -435,7 +439,10 @@ static std::vector<RPCResult> TransactionDescriptionString()
                                               "for 'send' and 'receive' category of transactions."},
            {RPCResult::Type::STR, "DS", /*optional=*/true, "Set to \"1\" if the transaction is a CoinJoin send."},
            {RPCResult::Type::STR, "comment", /*optional=*/true, "If a comment is associated with the transaction."},
-           {RPCResult::Type::STR, "to", /*optional=*/true, "If a comment to is associated with the transaction."}};
+           {RPCResult::Type::STR, "to", /*optional=*/true, "If a comment to is associated with the transaction."},
+           {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", {
+               {RPCResult::Type::STR, "desc", "The descriptor string."},
+           }}};
 }
 
 RPCHelpMan listtransactions()
@@ -562,6 +569,7 @@ RPCHelpMan listsinceblock()
                     {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
                     {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
                                                                        "(not guaranteed to work on pruned nodes)"},
+                    {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
                 },
                 RPCResult{
                     RPCResult::Type::OBJ, "", "",
@@ -642,6 +650,7 @@ RPCHelpMan listsinceblock()
     }
 
     bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
+    bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 
     int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1;
 
@@ -651,7 +660,7 @@ RPCHelpMan listsinceblock()
         const CWalletTx& tx = pairWtx.second;
 
         if (depth == -1 || abs(wallet.GetTxDepthInMainChain(tx)) < depth) {
-            ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */);
+            ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */, /*include_change=*/include_change);
         }
     }
 
@@ -668,7 +677,7 @@ RPCHelpMan listsinceblock()
             if (it != wallet.mapWallet.end()) {
                 // We want all transactions regardless of confirmation count to appear here,
                 // even negative confirmation ones, hence the big negative.
-                ListTransactions(wallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */);
+                ListTransactions(wallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */, /*include_change=*/include_change);
             }
         }
         blockId = block.hashPrevBlock;
@@ -727,6 +736,9 @@ RPCHelpMan gettransaction()
                                 {RPCResult::Type::STR_AMOUNT, "fee", /*optional=*/true, "The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n"
                                      "'send' category of transactions."},
                                 {RPCResult::Type::BOOL, "abandoned", "'true' if the transaction has been abandoned (inputs are respendable)."},
+                                {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", {
+                                    {RPCResult::Type::STR, "desc", "The descriptor string."},
+                                }},
                             }},
                         }},
                         {RPCResult::Type::STR_HEX, "hex", "Raw data for transaction"},
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index e67be931af6d..25151961df8b 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -138,6 +138,15 @@ std::string LabelFromValue(const UniValue& value)
     return label;
 }
 
+void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry)
+{
+    UniValue parent_descs(UniValue::VARR);
+    for (const auto& desc: wallet.GetWalletDescriptors(script_pubkey)) {
+        parent_descs.push_back(desc.descriptor->ToString());
+    }
+    entry.pushKV("parent_descs", parent_descs);
+}
+
 void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error)
 {
     if (!wallet) {
diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h
index c24cfe9f8485..d621308ee703 100644
--- a/src/wallet/rpc/util.h
+++ b/src/wallet/rpc/util.h
@@ -9,6 +9,8 @@
 #include <rpc/util.h>
 #include <wallet/wallet.h>
 
+#include <script/script.h>
+
 #include <memory>
 #include <string>
 #include <vector>
@@ -46,6 +48,8 @@ const LegacyScriptPubKeyMan& EnsureConstLegacyScriptPubKeyMan(const CWallet& wal
 bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param);
 bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet);
 std::string LabelFromValue(const UniValue& value);
+//! Fetch parent descriptors of this scriptPubKey.
+void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry);
 
 void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error);
 int64_t ParseISO8601DateTime(const std::string& str);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index e02442a653ec..6078d6377e72 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4038,6 +4038,18 @@ std::unique_ptr<SigningProvider> CWallet::GetSolvingProvider(const CScript& scri
     return nullptr;
 }
 
+std::vector<WalletDescriptor> CWallet::GetWalletDescriptors(const CScript& script) const
+{
+    std::vector<WalletDescriptor> descs;
+    for (const auto spk_man: GetScriptPubKeyMans(script)) {
+        if (const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man)) {
+            LOCK(desc_spk_man->cs_desc_man);
+            descs.push_back(desc_spk_man->GetWalletDescriptor());
+        }
+    }
+    return descs;
+}
+
 LegacyScriptPubKeyMan* CWallet::GetLegacyScriptPubKeyMan() const
 {
     if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index fd5eb5b11e1c..0be6433fceb5 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1059,6 +1059,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script) const;
     std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script, SignatureData& sigdata) const;
 
+    //! Get the wallet descriptors for a script.
+    std::vector<WalletDescriptor> GetWalletDescriptors(const CScript& script) const;
+
     //! Get the LegacyScriptPubKeyMan which is used for all types, internal, and external.
     LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const;
     LegacyScriptPubKeyMan* GetOrCreateLegacyScriptPubKeyMan();
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index de4608d8c79c..d3158b7c942a 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -7,6 +7,7 @@
 from itertools import product
 
 from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.descriptors import descsum_create
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_array_result,
@@ -752,6 +753,38 @@ def run_test(self):
         txid_feeReason_four = self.nodes[2].sendmany(dummy='', amounts={address: 5}, verbose=False)
         assert_equal(self.nodes[2].gettransaction(txid_feeReason_four)['txid'], txid_feeReason_four)
 
+        self.log.info("Testing 'listunspent' outputs the parent descriptor(s) of coins")
+        # Create two multisig descriptors, and send a UTxO each.
+        multi_a = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YBNjUo96Jxd1u4XKWgnoc7LsA1jz3Yc2NiDbhtfBhaBtemB73n9V5vtJHwU6FVXwggTbeoJWQ1rzdz8ysDuQkpnaHyvnvzR/*,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*))")
+        multi_b = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*,tpubD6NzVbkrYhZ4Y2RLiuEzNQkntjmsLpPYDm3LTRBYynUQtDtpzeUKAcb9sYthSFL3YR74cdFgF5mW8yKxv2W2CWuZDFR2dUpE5PF9kbrVXNZ/*))")
+        addr_a = self.nodes[0].deriveaddresses(multi_a, 0)[0]
+        addr_b = self.nodes[0].deriveaddresses(multi_b, 0)[0]
+        txid_a = self.nodes[0].sendtoaddress(addr_a, 0.01)
+        txid_b = self.nodes[0].sendtoaddress(addr_b, 0.01)
+        self.generate(self.nodes[0], 1, sync_fun=self.no_op)
+        # Now import the descriptors, make sure we can identify on which descriptor each coin was received.
+        self.nodes[0].createwallet(wallet_name="wo", descriptors=True, disable_private_keys=True)
+        wo_wallet = self.nodes[0].get_wallet_rpc("wo")
+        wo_wallet.importdescriptors([
+            {
+                "desc": multi_a,
+                "active": False,
+                "timestamp": "now",
+            },
+            {
+                "desc": multi_b,
+                "active": False,
+                "timestamp": "now",
+            },
+        ])
+        coins = wo_wallet.listunspent(minconf=0)
+        assert_equal(len(coins), 2)
+        coin_a = next(c for c in coins if c["txid"] == txid_a)
+        assert_equal(coin_a["parent_descs"][0], multi_a)
+        coin_b = next(c for c in coins if c["txid"] == txid_b)
+        assert_equal(coin_b["parent_descs"][0], multi_b)
+        self.nodes[0].unloadwallet("wo")
+
 
 if __name__ == '__main__':
     WalletTest().main()
diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py
index 6cea156b1e58..d2413838c396 100755
--- a/test/functional/wallet_listsinceblock.py
+++ b/test/functional/wallet_listsinceblock.py
@@ -6,6 +6,7 @@
 
 from test_framework.address import key_to_p2pkh
 from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.descriptors import descsum_create
 from test_framework.key import ECKey
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.messages import BIP125_SEQUENCE_NUMBER
@@ -44,6 +45,8 @@ def run_test(self):
         self.test_double_send()
         self.double_spends_filtered()
         self.test_targetconfirmations()
+        self.test_desc()
+        self.test_send_to_self()
         self.test_op_return()
 
     def test_no_blockhash(self):
@@ -403,6 +406,64 @@ def double_spends_filtered(self):
         assert_equal(original_found, False)
         assert_equal(double_found, False)
 
+    def test_desc(self):
+        """Make sure we can track coins by descriptor."""
+        self.log.info("Test descriptor lookup by scriptPubKey.")
+
+        # Create a watchonly wallet tracking two multisig descriptors.
+        multi_a = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YBNjUo96Jxd1u4XKWgnoc7LsA1jz3Yc2NiDbhtfBhaBtemB73n9V5vtJHwU6FVXwggTbeoJWQ1rzdz8ysDuQkpnaHyvnvzR/*,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*))")
+        multi_b = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*,tpubD6NzVbkrYhZ4Y2RLiuEzNQkntjmsLpPYDm3LTRBYynUQtDtpzeUKAcb9sYthSFL3YR74cdFgF5mW8yKxv2W2CWuZDFR2dUpE5PF9kbrVXNZ/*))")
+        self.nodes[0].createwallet(wallet_name="wo", descriptors=True, disable_private_keys=True)
+        wo_wallet = self.nodes[0].get_wallet_rpc("wo")
+        wo_wallet.importdescriptors([
+            {
+                "desc": multi_a,
+                "active": False,
+                "timestamp": "now",
+            },
+            {
+                "desc": multi_b,
+                "active": False,
+                "timestamp": "now",
+            },
+        ])
+
+        # Send a coin to each descriptor.
+        assert_equal(len(wo_wallet.listsinceblock()["transactions"]), 0)
+        addr_a = self.nodes[0].deriveaddresses(multi_a, 0)[0]
+        addr_b = self.nodes[0].deriveaddresses(multi_b, 0)[0]
+        self.nodes[2].sendtoaddress(addr_a, 1)
+        self.nodes[2].sendtoaddress(addr_b, 2)
+        self.generate(self.nodes[2], 1)
+
+        # We can identify on which descriptor each coin was received.
+        coins = wo_wallet.listsinceblock()["transactions"]
+        assert_equal(len(coins), 2)
+        coin_a = next(c for c in coins if c["amount"] == 1)
+        assert_equal(coin_a["parent_descs"][0], multi_a)
+        coin_b = next(c for c in coins if c["amount"] == 2)
+        assert_equal(coin_b["parent_descs"][0], multi_b)
+
+    def test_send_to_self(self):
+        """We can make listsinceblock output our change outputs."""
+        self.log.info("Test the inclusion of change outputs in the output.")
+
+        # Create a UTxO paying to one of our change addresses.
+        block_hash = self.nodes[2].getbestblockhash()
+        addr = self.nodes[2].getrawchangeaddress()
+        self.nodes[2].sendtoaddress(addr, 1)
+
+        # If we don't list change, we won't have an entry for it.
+        coins = self.nodes[2].listsinceblock(blockhash=block_hash)["transactions"]
+        assert not any(c["address"] == addr for c in coins)
+
+        # Now if we list change, we'll get both the send (to a change address) and
+        # the actual change.
+        res = self.nodes[2].listsinceblock(blockhash=block_hash, include_change=True)
+        coins = [entry for entry in res["transactions"] if entry["category"] == "receive"]
+        assert_equal(len(coins), 2)
+        assert any(c["address"] == addr for c in coins)
+        assert all(self.nodes[2].getaddressinfo(c["address"])["ischange"] for c in coins)
 
     def test_op_return(self):
         """Test if OP_RETURN outputs will be displayed correctly."""
@@ -417,6 +478,5 @@ def test_op_return(self):
 
         assert 'address' not in op_ret_tx
 
-
 if __name__ == '__main__':
     ListSinceBlockTest().main()

From 75892685f4418ac320fe27090ecce148aa7c07a6 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Mon, 22 Aug 2022 12:32:33 -0400
Subject: [PATCH 03/11] Merge bitcoin/bitcoin#25647: wallet: return change from
 SelectionResult

4fef5344288e454460b80db0316294e1ec1ad8ad wallet: use GetChange() when computing waste (S3RK)
87e0ef903133492e76b7c7556209554d4a0c3d66 wallet: use GetChange() in tx building (S3RK)
15e97a6886902ebb378829993a972dc52558aa92 wallet: add SelectionResult::GetChange (S3RK)
72cad28da05cfce9e4950f2dc5a709da41d251f4 wallet: calculate and store min_viable_change (S3RK)
e3210a722542a9cb5f7e4be72470dbe488c281fd wallet: account for preselected inputs in target (S3RK)
f8e796348b644c011ad9a8312356d4426c16cc4b wallet: add SelectionResult::Merge (S3RK)
06f558e4e2164d1916f258c731efe4586728a23b wallet: accurate SelectionResult::m_target (S3RK)
c8cf08ea743e430c2bf3fe46439594257b0937e5 wallet: ensure m_min_change_target always covers change fee (S3RK)

Pull request description:

  Benefits:
  1. more accurate waste calculation for knapsack. Waste calculation is now consistent with tx building code. Before we always assumed change for knapsack even when the solution is changeless4.
  2. simpler tx building code. Only create change output when it's needed
  3. makes it easier to correctly account for fees for CPFP inputs (should be done in a follow up)

  In the first three commits we fix the code to accurately track selection target in `SelectionResult::m_target`
  Then we introduce new variable `min_change` that represents the minimum viable change amount
  Then we introduce `SelectionResult::GetChange()` which incapsulates dropping change for fee logic and uses correct values of `SelectionResult::m_target`
  Then we use `SelectionResult::GetChange()` in both tx building and waste calculation code

  This PR is a refactoring and shouldn't change the behaviour.
  There is only one known small change (arguably a bug fix). Before we dropped change output if it's smaller than `cost_of_change` after paying change fees. This is incorrect as `cost_of_change` already includes `change_fee`.

ACKs for top commit:
  achow101:
    ACK 4fef5344288e454460b80db0316294e1ec1ad8ad
  Xekyo:
    crACK 4fef5344288e454460b80db0316294e1ec1ad8ad
  furszy:
    Code review ACK 4fef5344
  w0xlt:
    ACK https://github.com/bitcoin/bitcoin/pull/25647/commits/4fef5344288e454460b80db0316294e1ec1ad8ad

Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941

Co-authored-by: Andrew Chow <github@achow101.com>
---
 src/wallet/coinselection.cpp           |  63 +++++++++++--
 src/wallet/coinselection.h             |  48 ++++++++--
 src/wallet/spend.cpp                   | 119 +++++++++++--------------
 src/wallet/test/coinselector_tests.cpp |  15 ++--
 src/wallet/test/fuzz/coinselection.cpp |  10 ++-
 5 files changed, 162 insertions(+), 93 deletions(-)

diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index eff3a31e8322..28c73863dff9 100644
--- a/src/wallet/coinselection.cpp
+++ b/src/wallet/coinselection.cpp
@@ -190,7 +190,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
     for (const size_t& i : best_selection) {
         result.AddInput(utxo_pool.at(i));
     }
-    result.ComputeAndSetWaste(CAmount{0});
+    result.ComputeAndSetWaste(cost_of_change, cost_of_change, CAmount{0});
     assert(best_waste == result.GetWaste());
 
     return result;
@@ -206,13 +206,21 @@ class MinOutputGroupComparator
 };
 
 util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng,
-                                             int max_weight)
+                                             int max_weight, bool only_fully_mixed)
 {
     if (utxo_pool.empty()) return util::Error();
 
     SelectionResult result(target_value, SelectionAlgorithm::SRD);
     std::priority_queue<OutputGroup, std::vector<OutputGroup>, MinOutputGroupComparator> heap;
 
+    // Include change for SRD as we want to avoid making really small change if the selection just
+    // barely meets the target. Just use the lower bound change target instead of the randomly
+    // generated one, since SRD will result in a random change amount anyway; avoid making the
+    // target needlessly large.
+    if (!only_fully_mixed) {
+        target_value += CHANGE_LOWER;
+    }
+
     std::vector<size_t> indexes;
     indexes.resize(utxo_pool.size());
     std::iota(indexes.begin(), indexes.end(), 0);
@@ -530,20 +538,26 @@ CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost,
     return waste;
 }
 
-CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng)
+CAmount GenerateChangeTarget(const CAmount payment_value, const CAmount change_fee, FastRandomContext& rng)
 {
     if (payment_value <= CHANGE_LOWER / 2) {
-        return CHANGE_LOWER;
+        return change_fee + CHANGE_LOWER;
     } else {
         // random value between 50ksat and min (payment_value * 2, 1milsat)
         const auto upper_bound = std::min(payment_value * 2, CHANGE_UPPER);
-        return rng.randrange(upper_bound - CHANGE_LOWER) + CHANGE_LOWER;
+        return change_fee + rng.randrange(upper_bound - CHANGE_LOWER) + CHANGE_LOWER;
     }
 }
 
-void SelectionResult::ComputeAndSetWaste(CAmount change_cost)
+void SelectionResult::ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee)
 {
-    m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective);
+    const CAmount change = GetChange(min_viable_change, change_fee);
+
+    if (change > 0) {
+        m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective);
+    } else {
+        m_waste = GetSelectionWaste(m_selected_inputs, 0, m_target, m_use_effective);
+    }
 }
 
 CAmount SelectionResult::GetWaste() const
@@ -556,6 +570,11 @@ CAmount SelectionResult::GetSelectedValue() const
     return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.txout.nValue; });
 }
 
+CAmount SelectionResult::GetSelectedEffectiveValue() const
+{
+    return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.GetEffectiveValue(); });
+}
+
 void SelectionResult::Clear()
 {
     m_selected_inputs.clear();
@@ -568,6 +587,16 @@ void SelectionResult::AddInput(const OutputGroup& group)
     m_use_effective = !group.m_subtract_fee_outputs;
 }
 
+void SelectionResult::Merge(const SelectionResult& other)
+{
+    m_target += other.m_target;
+    m_use_effective |= other.m_use_effective;
+    if (m_algo == SelectionAlgorithm::MANUAL) {
+        m_algo = other.m_algo;
+    }
+    util::insert(m_selected_inputs, other.m_selected_inputs);
+}
+
 const std::set<COutput>& SelectionResult::GetInputSet() const
 {
     return m_selected_inputs;
@@ -605,4 +634,24 @@ std::string GetAlgorithmName(const SelectionAlgorithm algo)
     }
     assert(false);
 }
+
+CAmount SelectionResult::GetChange(const CAmount min_viable_change, const CAmount change_fee) const
+{
+    // change = SUM(inputs) - SUM(outputs) - fees
+    // 1) With SFFO we don't pay any fees
+    // 2) Otherwise we pay all the fees:
+    //  - input fees are covered by GetSelectedEffectiveValue()
+    //  - non_input_fee is included in m_target
+    //  - change_fee
+    const CAmount change = m_use_effective
+                           ? GetSelectedEffectiveValue() - m_target - change_fee
+                           : GetSelectedValue() - m_target;
+
+    if (change < min_viable_change) {
+        return 0;
+    }
+
+    return change;
+}
+
 } // namespace wallet
diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h
index 7d3456297054..2d97331942b4 100644
--- a/src/wallet/coinselection.h
+++ b/src/wallet/coinselection.h
@@ -124,6 +124,10 @@ struct CoinSelectionParams {
     /** Mininmum change to target in Knapsack solver: select coins to cover the payment and
      * at least this value of change. */
     CAmount m_min_change_target{0};
+    /** Minimum amount for creating a change output.
+     * If change budget is smaller than min_change then we forgo creation of change output.
+     */
+    CAmount min_viable_change{0};
     /** Cost of creating the change output. */
     CAmount m_change_fee{0};
     /** Cost of creating the change output + cost of spending the change output in the future. */
@@ -251,6 +255,7 @@ struct OutputGroup
 
 /** Choose a random change target for each transaction to make it harder to fingerprint the Core
  * wallet based on the change output values of transactions it creates.
+ * Change target covers at least change fees and adds a random value on top of it.
  * The random value is between 50ksat and min(2 * payment_value, 1milsat)
  * When payment_value <= 25ksat, the value is just 50ksat.
  *
@@ -260,8 +265,9 @@ struct OutputGroup
  * coins selected are just sufficient to cover the payment amount ("unnecessary input" heuristic).
  *
  * @param[in]   payment_value   Average payment value of the transaction output(s).
+ * @param[in]   change_fee      Fee for creating a change output.
  */
-[[nodiscard]] CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng);
+[[nodiscard]] CAmount GenerateChangeTarget(const CAmount payment_value, const CAmount change_fee, FastRandomContext& rng);
 
 enum class SelectionAlgorithm : uint8_t
 {
@@ -278,17 +284,16 @@ struct SelectionResult
 private:
     /** Set of inputs selected by the algorithm to use in the transaction */
     std::set<COutput> m_selected_inputs;
+    /** The target the algorithm selected for. Equal to the recipient amount plus non-input fees */
+    CAmount m_target;
+    /** The algorithm used to produce this result */
+    SelectionAlgorithm m_algo;
     /** Whether the input values for calculations should be the effective value (true) or normal value (false) */
     bool m_use_effective{false};
     /** The computed waste */
     std::optional<CAmount> m_waste;
 
 public:
-    /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */
-    const CAmount m_target;
-    /** The algorithm used to produce this result */
-    const SelectionAlgorithm m_algo;
-
     explicit SelectionResult(const CAmount target, SelectionAlgorithm algo)
         : m_target(target), m_algo(algo) {}
 
@@ -297,20 +302,47 @@ struct SelectionResult
     /** Get the sum of the input values */
     [[nodiscard]] CAmount GetSelectedValue() const;
 
+    [[nodiscard]] CAmount GetSelectedEffectiveValue() const;
+
     void Clear();
 
     void AddInput(const OutputGroup& group);
 
     /** Calculates and stores the waste for this selection via GetSelectionWaste */
-    void ComputeAndSetWaste(CAmount change_cost);
+    void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee);
     [[nodiscard]] CAmount GetWaste() const;
 
+    void Merge(const SelectionResult& other);
+
     /** Get m_selected_inputs */
     const std::set<COutput>& GetInputSet() const;
     /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */
     std::vector<COutput> GetShuffledInputVector() const;
 
     bool operator<(SelectionResult other) const;
+
+    /** Get the amount for the change output after paying needed fees.
+     *
+     * The change amount is not 100% precise due to discrepancies in fee calculation.
+     * The final change amount (if any) should be corrected after calculating the final tx fees.
+     * When there is a discrepancy, most of the time the final change would be slightly bigger than estimated.
+     *
+     * Following are the possible factors of discrepancy:
+     *  + non-input fees always include segwit flags
+     *  + input fee estimation always include segwit stack size
+     *  + input fees are rounded individually and not collectively, which leads to small rounding errors
+     *  - input counter size is always assumed to be 1vbyte
+     *
+     * @param[in]  min_viable_change  Minimum amount for change output, if change would be less then we forgo change
+     * @param[in]  change_fee         Fees to include change output in the tx
+     * @returns Amount for change output, 0 when there is no change.
+     *
+     */
+    CAmount GetChange(const CAmount min_viable_change, const CAmount change_fee) const;
+
+    CAmount GetTarget() const { return m_target; }
+
+    SelectionAlgorithm GetAlgo() const { return m_algo; }
 };
 
 util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change,
@@ -326,7 +358,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
  * @returns If successful, a valid SelectionResult, otherwise, util::Error
  */
 util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng,
-                                             int max_weight);
+                                             int max_weight, bool only_fully_mixed);
 
 // Original coin selection algorithm as a fallback
 util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue,
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 8b86c122a363..2896d88f1457 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -472,8 +472,7 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons
 
     int max_inputs_weight = MAX_STANDARD_TX_SIZE - coin_selection_params.tx_noinputs_size;
 
-    // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output.
-    std::vector<OutputGroup> positive_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, true /* positive_only */);
+    std::vector<OutputGroup> positive_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/true);
     positive_groups.clear(); // Cleared to skip BnB and SRD as they're unaware of mixed coins
     if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) {
         results.push_back(*bnb_result);
@@ -482,28 +481,16 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons
     max_inputs_weight -= coin_selection_params.change_output_size;
 
     // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here.
-    std::vector<OutputGroup> all_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, false /* positive_only */);
-    CAmount target_with_change = nTargetValue;
-    // While nTargetValue includes the transaction fees for non-input things, it does not include the fee for creating a change output.
-    // So we need to include that for KnapsackSolver as well, as we are expecting to create a change output.
-    // There is also no change output when spending fully mixed coins.
-    if (!coin_selection_params.m_subtract_fee_outputs && nCoinType != CoinType::ONLY_FULLY_MIXED) {
-        target_with_change += coin_selection_params.m_change_fee;
-    }
-    if (auto knapsack_result{KnapsackSolver(all_groups, target_with_change, coin_selection_params.m_min_change_target,
+    std::vector<OutputGroup> all_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/false);
+    if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue, coin_selection_params.m_min_change_target,
                                             coin_selection_params.rng_fast, max_inputs_weight, nCoinType == CoinType::ONLY_FULLY_MIXED,
                                             wallet.m_default_max_tx_fee)}) {
-        knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
+        knapsack_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
         results.push_back(*knapsack_result);
     }
 
-    // Include change for SRD as we want to avoid making really small change if the selection just
-    // barely meets the target. Just use the lower bound change target instead of the randomly
-    // generated one, since SRD will result in a random change amount anyway; avoid making the
-    // target needlessly large.
-    const CAmount srd_target = target_with_change + CHANGE_LOWER;
-    if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast, max_inputs_weight)}) {
-        srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
+    if (auto srd_result{SelectCoinsSRD(positive_groups, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight, nCoinType == CoinType::ONLY_FULLY_MIXED)}) {
+        srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
         results.push_back(*srd_result);
     }
 
@@ -604,7 +591,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
             result.AddInput(preset_inputs);
         }
         if (result.GetSelectedValue() < nTargetValue) return std::nullopt;
-        result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
+        result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
         return result;
     }
 
@@ -630,12 +617,16 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
         Shuffle(available_coins.legacy.begin(), available_coins.legacy.end(), coin_selection_params.rng_fast);
         Shuffle(available_coins.other.begin(), available_coins.other.end(), coin_selection_params.rng_fast);
     }
+
+    SelectionResult preselected(preset_inputs.GetSelectionAmount(), SelectionAlgorithm::MANUAL);
+    preselected.AddInput(preset_inputs);
+
     // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the
     // transaction at a target feerate. If an attempt fails, more attempts may be made using a more
     // permissive CoinEligibilityFilter.
     std::optional<SelectionResult> res = [&] {
         // Pre-selected inputs already cover the target amount.
-        if (value_to_select <= 0) return std::make_optional(SelectionResult(nTargetValue, SelectionAlgorithm::MANUAL));
+        if (value_to_select <= 0) return std::make_optional(SelectionResult(value_to_select, SelectionAlgorithm::MANUAL));
 
         // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six
         // confirmations on outputs received from other wallets and only spend confirmed change.
@@ -689,9 +680,9 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
     if (!res) return std::nullopt;
 
     // Add preset inputs to result
-    res->AddInput(preset_inputs);
-    if (res->m_algo == SelectionAlgorithm::MANUAL) {
-        res->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
+    res->Merge(preselected);
+    if (res->GetAlgo() == SelectionAlgorithm::MANUAL) {
+        res->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
     }
 
     return res;
@@ -846,7 +837,6 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
             coin_selection_params.m_subtract_fee_outputs = true;
         }
     }
-    coin_selection_params.m_min_change_target = GenerateChangeTarget(std::floor(recipients_sum / vecSend.size()), rng_fast);
 
     // Create change script that will be used if we need change
     CScript scriptChange;
@@ -915,6 +905,15 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size);
     coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee;
 
+    coin_selection_params.m_min_change_target = GenerateChangeTarget(std::floor(recipients_sum / vecSend.size()), coin_selection_params.m_change_fee, rng_fast);
+
+    // The smallest change amount should be:
+    // 1. at least equal to dust threshold
+    // 2. at least 1 sat greater than fees to spend it at m_discard_feerate
+    const auto dust = GetDustThreshold(change_prototype_txout, coin_selection_params.m_discard_feerate);
+    const auto change_spend_fee = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size);
+    coin_selection_params.min_viable_change = std::max(change_spend_fee + 1, dust);
+
     // vouts to the payees
     if (!coin_selection_params.m_subtract_fee_outputs) {
         coin_selection_params.tx_noinputs_size = 9; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count
@@ -966,25 +965,26 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
         }
         return util::Error{_("Insufficient funds.")};
     }
-    TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->m_algo).c_str(), result->m_target, result->GetWaste(), result->GetSelectedValue());
-
-    // Always make a change output
-    // We will reduce the fee from this change output later, and remove the output if it is too small.
-    const CAmount change_and_fee = result->GetSelectedValue() - recipients_sum;
-    assert(change_and_fee >= 0);
-    CTxOut newTxOut(change_and_fee, scriptChange);
+    TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->GetAlgo()).c_str(), result->GetTarget(), result->GetWaste(), result->GetSelectedValue());
+
+    // Fully mixed coins are spent in whole denominations, so no change is allowed there and
+    // whatever the solver selected above the target is paid as fees instead.
+    const CAmount change_amount = coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED
+                                      ? 0
+                                      : result->GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee);
+    CTxOut newTxOut(change_amount, scriptChange);
+    if (change_amount > 0) {
+        if (nChangePosInOut == -1) {
+            // Insert change txn at random position:
+            nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1);
+        } else if ((unsigned int)nChangePosInOut > txNew.vout.size()) {
+            return util::Error{_("Transaction change output index out of range")};
+        }
+        txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
 
-    if (nChangePosInOut == -1) {
-        // Insert change txn at random position:
-        nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1);
-    }
-    else if ((unsigned int)nChangePosInOut > txNew.vout.size()) {
-        return util::Error{_("Transaction change output index out of range")};
+    } else {
+        nChangePosInOut = -1;
     }
-
-    assert(nChangePosInOut != -1);
-    auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
-
     // We're making a copy of vecSend because it's const, sortedVecSend should be used
     // in place of vecSend in all subsequent usage.
     std::vector<CRecipient> sortedVecSend{vecSend};
@@ -999,10 +999,9 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
 
         // If there was a change output added before, we must update its position now
         if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) {
-            change_position = it;
-            nChangePosInOut = std::distance(txNew.vout.begin(), change_position);
+            nChangePosInOut = std::distance(txNew.vout.begin(), it);
         }
-    };
+    }
 
     // The sequence number is set to non-maxint so that DiscourageFeeSniping
     // works.
@@ -1025,26 +1024,6 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     nBytes += extra_payload_bytes;
 
     CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
-
-    if (!coin_selection_params.m_subtract_fee_outputs) {
-        change_position->nValue -= fee_needed;
-    }
-
-    // We want to drop the change to fees if:
-    // 1. The change output would be dust
-    // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change)
-    // 3. We are working with fully mixed CoinJoin denominations
-    CAmount change_amount = change_position->nValue;
-    if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change || coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED)
-    {
-        nChangePosInOut = -1;
-        change_amount = 0;
-        txNew.vout.erase(change_position);
-
-        nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control) + extra_payload_bytes;
-        fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
-    }
-
     nFeeRet = result->GetSelectedValue() - recipients_sum - change_amount;
 
     // The only time that fee_needed should be less than the amount available for fees is when
@@ -1053,14 +1032,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
         return util::Error{Untranslated(STR_INTERNAL_BUG("Fee needed > fee paid"))};
     }
 
-    // Update nFeeRet in case fee_needed changed due to dropping the change output
-    if (fee_needed <= change_and_fee - change_amount) {
-        nFeeRet = change_and_fee - change_amount;
+    // If there is a change output and we overpay the fees then increase the change to match the fee needed
+    if (nChangePosInOut != -1 && fee_needed < nFeeRet) {
+        auto& change = txNew.vout.at(nChangePosInOut);
+        change.nValue += nFeeRet - fee_needed;
+        nFeeRet = fee_needed;
     }
 
     // Reduce output values for subtractFeeFromAmount
     if (coin_selection_params.m_subtract_fee_outputs) {
-        CAmount to_reduce = fee_needed + change_amount - change_and_fee;
+        CAmount to_reduce = fee_needed - nFeeRet;
         int i = 0;
         bool fFirst = true;
         for (const auto& recipient : sortedVecSend)
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index 0b940dcb7635..7e34cea303ef 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -258,9 +258,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
 
     // Iteration exhaustion test
     CAmount target = make_hard_case(17, utxo_pool);
-    BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0)); // Should exhaust
+    BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 1)); // Should exhaust
     target = make_hard_case(14, utxo_pool);
-    const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 0); // Should not exhaust
+    const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 1); // Should not exhaust
     BOOST_CHECK(result7);
 
     // Test same value early bailout optimization
@@ -299,8 +299,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
     // Make sure that effective value is working in AttemptSelection when BnB is used
     CoinSelectionParams coin_selection_params_bnb{
         rand,
-        /*change_output_size=*/ 0,
-        /*change_spend_size=*/ 0,
+        /*change_output_size=*/ 31,
+        /*change_spend_size=*/ 68,
         /*min_change_target=*/ 0,
         /*effective_feerate=*/ CFeeRate(3000),
         /*long_term_feerate=*/ CFeeRate(1000),
@@ -310,6 +310,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
     };
     coin_selection_params_bnb.m_subtract_fee_outputs = true;
 
+    coin_selection_params_bnb.m_change_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_output_size);
+    coin_selection_params_bnb.m_cost_of_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee;
+    coin_selection_params_bnb.min_viable_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size);
     {
         std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
         wallet->LoadWallet();
@@ -789,6 +792,8 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
             /*tx_noinputs_size=*/ 0,
             /*avoid_partial=*/ false,
         };
+        cs_params.m_cost_of_change = 1;
+        cs_params.min_viable_change = 1;
         CCoinControl cc;
         const auto result = SelectCoins(*wallet, available_coins, target, cc, cs_params);
         BOOST_CHECK(result);
@@ -944,7 +949,7 @@ static util::Result<SelectionResult> SelectCoinsSRDResult(const CAmount& target,
     CoinEligibilityFilter filter(0, 0, 0);
     auto available_coins = coin_setup(*wallet);
     auto groups = GroupOutputs(*wallet, available_coins.all(), cs_params, filter, /*positive_only=*/true);
-    return wallet::SelectCoinsSRD(groups, target, cs_params.rng_fast, max_weight);
+    return wallet::SelectCoinsSRD(groups, target, cs_params.rng_fast, max_weight, false);
 }
 
 BOOST_AUTO_TEST_CASE(srd_tests)
diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index e10a8abbf1d4..f8d8769ea900 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -61,6 +61,8 @@ FUZZ_TARGET(coinselection)
     coin_params.m_subtract_fee_outputs = subtract_fee_outputs;
     coin_params.m_long_term_feerate = long_term_fee_rate;
     coin_params.m_effective_feerate = effective_fee_rate;
+    coin_params.change_output_size = fuzzed_data_provider.ConsumeIntegralInRange<int>(10, 1000);
+    coin_params.m_change_fee = effective_fee_rate.GetFee(coin_params.change_output_size);
 
     // Create some coins
     CAmount total_balance{0};
@@ -85,13 +87,13 @@ FUZZ_TARGET(coinselection)
     // Run coinselection algorithms
     const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_SIZE);
 
-    auto result_srd = SelectCoinsSRD(group_pos, target, fast_random_context, MAX_STANDARD_TX_SIZE);
-    if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change);
+    auto result_srd = SelectCoinsSRD(group_pos, target, fast_random_context, MAX_STANDARD_TX_SIZE, false);
+    if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0);
 
-    CAmount change_target{GenerateChangeTarget(target, fast_random_context)};
+    CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)};
     auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context,
                                           MAX_STANDARD_TX_SIZE, /*fFullyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE);
-    if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change);
+    if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change, cost_of_change, 0);
 
     // If the total balance is sufficient for the target and we are not using
     // effective values, Knapsack should always find a solution unless the selection exceeds max tx weight.

From 9024c339a3e9224e82869c559fa858ea277a7046 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 25 Aug 2022 17:11:58 -0400
Subject: [PATCH 04/11] Merge bitcoin/bitcoin#25924: scripted-diff: rpc: fix
 rescan RPC name (s/rescanwallet/rescanblockchain/)

e90a445d7e84df4bfdd689c6593f6688cbace746 scripted-diff: rpc: fix rescan RPC name (s/rescanwallet/rescanblockchain/) (Sebastian Falbesoner)

Pull request description:

  There is no RPC call named `rescanwallet`, i.e. fix this by renaming to the actual RPC called `rescanblockchain`.

ACKs for top commit:
  achow101:
    ACK e90a445d7e84df4bfdd689c6593f6688cbace746
  aureleoules:
    ACK e90a445d7e84df4bfdd689c6593f6688cbace746.
  promag:
    ACK e90a445d7e84df4bfdd689c6593f6688cbace746

Tree-SHA512: abf1d1c18de32d87c29e4ff2b782dfb0e4a46dc2c2cc51ab616d12674a0f4a5d22214e00955663ae897cbb88f4f6ced913850f28ea3f5c1b3a54577a25fbf399

Co-authored-by: Andrew Chow <achow101-github@achow101.com>
---
 src/wallet/rpc/backup.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index ce9e6b1dc3da..656ed98d98e2 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -92,7 +92,7 @@ RPCHelpMan importprivkey()
             "\nNote: This call can take over an hour to complete if rescan is true, during that time, other rpc calls\n"
             "may report that the imported key exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
             "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n"
-            "but the key was used to create transactions, rescanwallet needs to be called with the appropriate block range.\n"
+            "but the key was used to create transactions, rescanblockchain needs to be called with the appropriate block range.\n"
             "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"combo(X)\" for descriptor wallets.\n",
                 {
                     {"privkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The private key (see dumpprivkey)"},
@@ -211,7 +211,7 @@ RPCHelpMan importaddress()
             "\nNote: This call can take over an hour to complete if rescan is true, during that time, other rpc calls\n"
             "may report that the imported address exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
             "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n"
-            "but the key was used to create transactions, rescanwallet needs to be called with the appropriate block range.\n"
+            "but the key was used to create transactions, rescanblockchain needs to be called with the appropriate block range.\n"
             "If you have the full public key, you should call importpubkey instead of this.\n"
             "Hint: use importmulti to import more than one address.\n"
             "\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n"
@@ -409,7 +409,7 @@ RPCHelpMan importpubkey()
             "\nNote: This call can take over an hour to complete if rescan is true, during that time, other rpc calls\n"
             "may report that the imported pubkey exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
             "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n"
-            "but the key was used to create transactions, rescanwallet needs to be called with the appropriate block range.\n"
+            "but the key was used to create transactions, rescanblockchain needs to be called with the appropriate block range.\n"
             "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"combo(X)\" for descriptor wallets.\n",
                 {
                     {"pubkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The hex-encoded public key"},
@@ -1490,7 +1490,7 @@ RPCHelpMan importmulti()
             "\nNote: This call can take over an hour to complete if rescan is true, during that time, other rpc calls\n"
             "may report that the imported keys, addresses or scripts exists but related transactions are still missing.\n"
             "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n"
-            "but the key was used to create transactions, rescanwallet needs to be called with the appropriate block range.\n"
+            "but the key was used to create transactions, rescanblockchain needs to be called with the appropriate block range.\n"
             "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" for descriptor wallets.\n",
                 {
                     {"requests", RPCArg::Type::ARR, RPCArg::Optional::NO, "Data to be imported",

From 3a54613bbebedbde658f26302508e07420aaef2f Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Mon, 5 Sep 2022 13:54:02 +0100
Subject: [PATCH 05/11] Merge bitcoin/bitcoin#25768: wallet: Properly
 rebroadcast unconfirmed transaction chains

3405f3eed5cf841b23a569b64a376c2e5b5026cd test: Test that an unconfirmed not-in-mempool chain is rebroadcast (Andrew Chow)
10d91c5abe9ed7dcc237c9d52c588e7d26e162a4 wallet: Deduplicate Resend and ReacceptWalletTransactions (Andrew Chow)

Pull request description:

  Currently `ResendWalletTransactions` (used for normal rebroadcasts) will attempt to rebroadcast all of the transactions in the wallet in the order they are stored in `mapWallet`. This ends up being random as `mapWallet` is a `std::unordered_map`. However `ReacceptWalletTransactions` (used for adding to the mempool on loading) first sorts the txs by wallet insertion order, then submits them. The result is that `ResendWalletTranactions` will fail to rebroadcast child transactions if their txids happen to be lexicographically less than their parent's txid. This PR resolves this issue by combining `ReacceptWalletTransactions` and `ResendWalletTransactions` into a new `ResubmitWalletTransactions` so that the iteration code and basic checks are shared.

  A test has also been added that checks that such transaction chains are rebroadcast correctly.

ACKs for top commit:
  naumenkogs:
    utACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd
  1440000bytes:
    reACK https://github.com/bitcoin/bitcoin/pull/25768/commits/3405f3eed5cf841b23a569b64a376c2e5b5026cd
  furszy:
    Late code review ACK 3405f3ee
  stickies-v:
    ACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd

Tree-SHA512: 1240d9690ecc2ae8d476286b79e2386f537a90c41dd2b8b8a5a9c2a917aa3af85d6aee019fbbb05e772985a2b197e2788305586d9d5dac78ccba1ee5aa31d77a

Co-authored-by: glozow <gloriajzhao@gmail.com>
---
 src/wallet/rpc/backup.cpp                     |  8 +-
 src/wallet/transaction.h                      |  7 ++
 src/wallet/wallet.cpp                         | 90 +++++++++----------
 src/wallet/wallet.h                           |  3 +-
 test/functional/mempool_expiry.py             |  2 +-
 test/functional/test_framework/messages.py    |  2 +
 .../wallet_resendwallettransactions.py        | 75 ++++++++++++----
 7 files changed, 117 insertions(+), 70 deletions(-)

diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 656ed98d98e2..54d6786b1090 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -298,7 +298,7 @@ RPCHelpMan importaddress()
         RescanWallet(*pwallet, reserver);
         {
             LOCK(pwallet->cs_wallet);
-            pwallet->ReacceptWalletTransactions();
+            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
         }
     }
 
@@ -477,7 +477,7 @@ RPCHelpMan importpubkey()
         RescanWallet(*pwallet, reserver);
         {
             LOCK(pwallet->cs_wallet);
-            pwallet->ReacceptWalletTransactions();
+            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
         }
     }
 
@@ -1640,7 +1640,7 @@ RPCHelpMan importmulti()
         int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */);
         {
             LOCK(pwallet->cs_wallet);
-            pwallet->ReacceptWalletTransactions();
+            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
         }
 
         if (pwallet->IsAbortingRescan()) {
@@ -1935,7 +1935,7 @@ RPCHelpMan importdescriptors() {
         int64_t scanned_time = pwallet->RescanFromTime(lowest_timestamp, reserver, true /* update */);
         {
             LOCK(pwallet->cs_wallet);
-            pwallet->ReacceptWalletTransactions();
+            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
         }
 
         if (pwallet->IsAbortingRescan()) {
diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
index bac89c2f633f..3717be956832 100644
--- a/src/wallet/transaction.h
+++ b/src/wallet/transaction.h
@@ -323,6 +323,13 @@ class CWalletTx
     CWalletTx(CWalletTx const &) = delete;
     void operator=(CWalletTx const &x) = delete;
 };
+
+struct WalletTxOrderComparator {
+    bool operator()(const CWalletTx* a, const CWalletTx* b) const
+    {
+        return a->nOrderPos < b->nOrderPos;
+    }
+};
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_TRANSACTION_H
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 6078d6377e72..b3e7a2eedfcc 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2066,34 +2066,6 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
     return result;
 }
 
-void CWallet::ReacceptWalletTransactions()
-{
-    // If transactions aren't being broadcasted, don't let them into local mempool either
-    if (!fBroadcastTransactions)
-        return;
-    std::map<int64_t, CWalletTx*> mapSorted;
-
-    // Sort pending wallet transactions based on their initial wallet insertion order
-    for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
-        const uint256& wtxid = item.first;
-        CWalletTx& wtx = item.second;
-        assert(wtx.GetHash() == wtxid);
-
-        int nDepth = GetTxDepthInMainChain(wtx);
-
-        if (!wtx.IsCoinBase() && (nDepth == 0 && !IsTxLockedByInstantSend(wtx) && !wtx.isAbandoned())) {
-            mapSorted.insert(std::make_pair(wtx.nOrderPos, &wtx));
-        }
-    }
-
-    // Try to add wallet transactions to memory pool
-    for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
-        CWalletTx& wtx = *(item.second);
-        bilingual_str unused_err_string;
-        SubmitTxMemoryPoolAndRelay(wtx, unused_err_string, false);
-    }
-}
-
 bool CWallet::CanTxBeResent(const CWalletTx& wtx) const
 {
     AssertLockHeld(cs_wallet);
@@ -2144,43 +2116,69 @@ std::set<uint256> CWallet::GetTxConflicts(const CWalletTx& wtx) const
     return result;
 }
 
-// Rebroadcast transactions from the wallet. We do this on a random timer
-// to slightly obfuscate which transactions come from our wallet.
+// Resubmit transactions from the wallet to the mempool, optionally asking the
+// mempool to relay them. On startup, we will do this for all unconfirmed
+// transactions but will not ask the mempool to relay them. We do this on startup
+// to ensure that our own mempool is aware of our transactions, and to also
+// initialize nNextResend so that the actual rebroadcast is scheduled. There
+// is a privacy side effect here as not broadcasting on startup also means that we won't
+// inform the world of our wallet's state, particularly if the wallet (or node) is not
+// yet synced.
+//
+// Otherwise this function is called periodically in order to relay our unconfirmed txs.
+// We do this on a random timer to slightly obfuscate which transactions
+// come from our wallet.
 //
-// Ideally, we'd only resend transactions that we think should have been
+// TODO: Ideally, we'd only resend transactions that we think should have been
 // mined in the most recent block. Any transaction that wasn't in the top
 // blockweight of transactions in the mempool shouldn't have been mined,
 // and so is probably just sitting in the mempool waiting to be confirmed.
 // Rebroadcasting does nothing to speed up confirmation and only damages
 // privacy.
-void CWallet::ResendWalletTransactions()
+//
+// The `force` option results in all unconfirmed transactions being submitted to
+// the mempool. This does not necessarily result in those transactions being relayed,
+// that depends on the `relay` option. Periodic rebroadcast uses the pattern
+// relay=true force=false (also the default values), while loading into the mempool
+// (on start, or after import) uses relay=false force=true.
+void CWallet::ResubmitWalletTransactions(bool relay, bool force)
 {
+    // Don't attempt to resubmit if the wallet is configured to not broadcast,
+    // even if forcing.
+    if (!fBroadcastTransactions) return;
+
     // During reindex, importing and IBD, old wallet transactions become
     // unconfirmed. Don't resend them as that would spam other nodes.
-    if (!chain().isReadyToBroadcast()) return;
+    // We only allow forcing mempool submission when not relaying to avoid this spam.
+    if (!force && relay && !chain().isReadyToBroadcast()) return;
 
     // Do this infrequently and randomly to avoid giving away
     // that these are our transactions.
-    if (GetTime() < m_next_resend || !fBroadcastTransactions) return;
-    bool fFirst = (m_next_resend == 0);
+    if (!force && GetTime() < m_next_resend) return;
     // resend 1-3 hours from now, ~2 hours on average.
     m_next_resend = GetTime() + (1 * 60 * 60) + GetRand(2 * 60 * 60);
-    if (fFirst) return;
 
     int submitted_tx_count = 0;
 
     { // cs_wallet scope
         LOCK(cs_wallet);
 
-        // Relay transactions
-        for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
-            CWalletTx& wtx = item.second;
-            // Attempt to rebroadcast all txes more than 5 minutes older than
-            // the last block. SubmitTxMemoryPoolAndRelay() will not rebroadcast
-            // any confirmed or conflicting txs.
-            if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
+        // First filter for the transactions we want to rebroadcast.
+        // We use a set with WalletTxOrderComparator so that rebroadcasting occurs in insertion order
+        std::set<CWalletTx*, WalletTxOrderComparator> to_submit;
+        for (auto& [txid, wtx] : mapWallet) {
+            // Only rebroadcast unconfirmed txs
+            if (!wtx.isUnconfirmed()) continue;
+
+            // attempt to rebroadcast all txes more than 5 minutes older than
+            // the last block, or all txs if forcing.
+            if (!force && wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
+            to_submit.insert(&wtx);
+        }
+        // Now try submitting the transactions to the memory pool and (optionally) relay them.
+        for (auto wtx : to_submit) {
             bilingual_str unused_err_string;
-            if (SubmitTxMemoryPoolAndRelay(wtx, unused_err_string, true)) ++submitted_tx_count;
+            if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, relay)) ++submitted_tx_count;
         }
     } // cs_wallet
 
@@ -2194,7 +2192,7 @@ void CWallet::ResendWalletTransactions()
 void MaybeResendWalletTxs(WalletContext& context)
 {
     for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
-        pwallet->ResendWalletTransactions();
+        pwallet->ResubmitWalletTransactions(/*relay=*/true, /*force=*/false);
     }
 }
 
@@ -3582,7 +3580,7 @@ void CWallet::postInitProcess()
 
     // Add wallet transactions that aren't already in a block to mempool
     // Do this here as mempool requires genesis block to be loaded
-    ReacceptWalletTransactions();
+    ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
 
     // Update wallet transactions with current mempool transactions.
     chain().requestMempoolTransactions(*this);
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 0be6433fceb5..cbf419734f54 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -706,8 +706,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     };
     ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress);
     void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override;
-    void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    void ResendWalletTransactions();
+    void ResubmitWalletTransactions(bool relay, bool force);
 
     CAmount GetAnonymizableBalance(bool fSkipDenominated = false, bool fSkipUnconfirmed = true) const;
     float GetAverageAnonymizedRounds() const;
diff --git a/test/functional/mempool_expiry.py b/test/functional/mempool_expiry.py
index 7c916f84eccd..7d6f760a868a 100755
--- a/test/functional/mempool_expiry.py
+++ b/test/functional/mempool_expiry.py
@@ -12,6 +12,7 @@
 
 from datetime import timedelta
 
+from test_framework.messages import DEFAULT_MEMPOOL_EXPIRY_HOURS
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -19,7 +20,6 @@
 )
 from test_framework.wallet import MiniWallet
 
-DEFAULT_MEMPOOL_EXPIRY_HOURS = 336  # hours
 CUSTOM_MEMPOOL_EXPIRY = 10  # hours
 
 
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 5f66dc6af064..e055e24a205e 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -71,6 +71,8 @@
 # Default setting for -datacarriersize. 80 bytes of data, +1 for OP_RETURN, +2 for the pushdata opcodes.
 MAX_OP_RETURN_RELAY = 83
 
+DEFAULT_MEMPOOL_EXPIRY_HOURS = 336  # hours
+
 MAGIC_BYTES = {
     "mainnet": b"\xbf\x0c\x6b\xbd",   # mainnet
     "testnet3": b"\xce\xe2\xca\xff",  # testnet3
diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py
index 8356b89d401b..5a08e277e1f8 100755
--- a/test/functional/wallet_resendwallettransactions.py
+++ b/test/functional/wallet_resendwallettransactions.py
@@ -8,10 +8,13 @@
     create_block,
     create_coinbase,
 )
+from test_framework.messages import DEFAULT_MEMPOOL_EXPIRY_HOURS
 from test_framework.p2p import P2PTxInvStore
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal
-
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+)
 
 class ResendWalletTransactionsTest(BitcoinTestFramework):
     def add_options(self, parser):
@@ -29,13 +32,9 @@ def run_test(self):
         peer_first = node.add_p2p_connection(P2PTxInvStore())
 
         self.log.info("Create a new transaction and wait until it's broadcast")
-        txid = node.sendtoaddress(node.getnewaddress(), 1)
-
-        # Wallet rebroadcast is first scheduled 1 min sec after startup (see
-        # nNextResend in ResendWalletTransactions()). Tell scheduler to call
-        # MaybeResendWalletTxs now to initialize nNextResend before the first
-        # setmocktime call below.
-        node.mockscheduler(60)
+        parent_utxo, indep_utxo = node.listunspent()[:2]
+        addr = node.getnewaddress()
+        txid = node.send(outputs=[{addr: 1}], options={"inputs": [parent_utxo]})["txid"]
 
         # Can take a few seconds due to transaction trickling
         def wait_p2p():
@@ -56,28 +55,70 @@ def wait_p2p():
         block.solve()
         node.submitblock(block.serialize().hex())
 
-        # Set correct m_best_block_time, which is used in ResendWalletTransactions
+        # Set correct m_best_block_time, which is used in ResubmitWalletTransactions
         node.syncwithvalidationinterfacequeue()
 
-        # Transaction should not be rebroadcast within first 12 hours
+        # Transaction should not be rebroadcast within first hour
         # Leave 2 mins for buffer
-        twelve_hrs = 12 * 60 * 60
+        one_hour = 60 * 60
         two_min = 2 * 60
-        node.setmocktime(self.mocktime + twelve_hrs - two_min)
+        node.setmocktime(self.mocktime + one_hour - two_min)
         node.mockscheduler(60)  # Tell scheduler to call MaybeResendWalletTxs now
         assert_equal(int(txid, 16) in peer_second.get_invs(), False)
 
         self.log.info("Bump time & check that transaction is rebroadcast")
-        # Transaction should be rebroadcast approximately 24 hours in the future,
-        # but can range from 12-36. So bump 36 hours to be sure.
-        with node.assert_debug_log(['ResendWalletTransactions: resubmit 1 unconfirmed transactions']):
-            node.setmocktime(self.mocktime + 36 * 60 * 60)
+        # Transaction should be rebroadcast approximately 2 hours in the future,
+        # but can range from 1-3. So bump 3 hours to be sure.
+        with node.assert_debug_log(['resubmit 1 unconfirmed transactions']):
+            node.setmocktime(self.mocktime + 3 * 60 * 60)
             # Tell scheduler to call MaybeResendWalletTxs now.
             node.mockscheduler(60)
         # Give some time for trickle to occur
         node.setmocktime(self.mocktime + 36 * 60 * 60 + 600)
         peer_second.wait_for_broadcast([txid])
 
+        self.log.info("Chain of unconfirmed not-in-mempool txs are rebroadcast")
+        # This tests that the node broadcasts the parent transaction before the child transaction.
+        # To test that scenario, we need a method to reliably get a child transaction placed
+        # in mapWallet positioned before the parent. We cannot predict the position in mapWallet,
+        # but we can observe it using listreceivedbyaddress and other related RPCs.
+        #
+        # So we will create the child transaction, then use listreceivedbyaddress to see what the
+        # ordering of mapWallet is. mapWallet hashes txids with a salt drawn at random every time
+        # the wallet is loaded, so reloading the wallet reshuffles its iteration order while
+        # leaving the transactions themselves untouched. Reload until the child comes first.
+        child_txid = node.send(outputs=[{addr: 0.5}], options={"inputs": [{"txid":txid, "vout":0}]})["txid"]
+        while True:
+            txids = node.listreceivedbyaddress(minconf=0, address_filter=addr)[0]["txids"]
+            if txids == [child_txid, txid]:
+                break
+            node.unloadwallet(self.default_wallet_name)
+            node.loadwallet(self.default_wallet_name)
+        entry_time = node.getmempoolentry(child_txid)["time"]
+
+        block_time = entry_time + 6 * 60
+        node.setmocktime(block_time)
+        block = create_block(int(node.getbestblockhash(), 16), create_coinbase(node.getblockcount() + 1), block_time)
+        block.solve()
+        node.submitblock(block.serialize().hex())
+        node.syncwithvalidationinterfacequeue()
+
+        # Evict these txs from the mempool
+        evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5
+        node.setmocktime(evict_time)
+        indep_send = node.send(outputs=[{node.getnewaddress(): 1}], options={"inputs": [indep_utxo]})
+        node.syncwithvalidationinterfacequeue()
+        node.getmempoolentry(indep_send["txid"])
+        assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid)
+        assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, child_txid)
+
+        # Rebroadcast and check that parent and child are both in the mempool
+        with node.assert_debug_log(['resubmit 2 unconfirmed transactions']):
+            node.setmocktime(evict_time + 36 * 60 * 60) # 36 hrs is the upper limit of the resend timer
+            node.mockscheduler(60)
+        node.getmempoolentry(txid)
+        node.getmempoolentry(child_txid)
+
 
 if __name__ == '__main__':
     ResendWalletTransactionsTest().main()

From c2d21b51625b3717c3b60f2a1db582d881fcf14d Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 15 Sep 2022 13:15:55 -0400
Subject: [PATCH 06/11] Merge bitcoin/bitcoin#26091: test: Fix
 syncwithvalidationinterfacequeue calls

fa1ce96184a1815f453e64e14d77cb0025800be9 test: Add missing syncwithvalidationinterfacequeue (MacroFake)
faa4916529699f9a057e2bf2459d957bcec1de84 test/doc: Remove unused syncwithvalidationinterfacequeue (MacroFake)

Pull request description:

  Fixes #26071

ACKs for top commit:
  achow101:
    ACK fa1ce96184a1815f453e64e14d77cb0025800be9
  glozow:
    ACK fa1ce96184a1815f453e64e14d77cb0025800be9
  w0xlt:
    ACK https://github.com/bitcoin/bitcoin/commit/fa1ce96184a1815f453e64e14d77cb0025800be9

Tree-SHA512: d1e101b55477360ead2b99ade5d42b922aabe293ec84fb26764e29161c5be6c534aef6f22d2cc5ea63a4bd6b6e77b701f1a7a2283b8e7e815d343a604cd77656

Co-authored-by: Andrew Chow <github@achow101.com>
---
 src/wallet/wallet.cpp                              | 2 +-
 test/functional/wallet_resendwallettransactions.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index b3e7a2eedfcc..bcd726482ac3 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2139,7 +2139,7 @@ std::set<uint256> CWallet::GetTxConflicts(const CWalletTx& wtx) const
 // The `force` option results in all unconfirmed transactions being submitted to
 // the mempool. This does not necessarily result in those transactions being relayed,
 // that depends on the `relay` option. Periodic rebroadcast uses the pattern
-// relay=true force=false (also the default values), while loading into the mempool
+// relay=true force=false, while loading into the mempool
 // (on start, or after import) uses relay=false force=true.
 void CWallet::ResubmitWalletTransactions(bool relay, bool force)
 {
diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py
index 5a08e277e1f8..8a7da4b00470 100755
--- a/test/functional/wallet_resendwallettransactions.py
+++ b/test/functional/wallet_resendwallettransactions.py
@@ -101,13 +101,13 @@ def wait_p2p():
         block = create_block(int(node.getbestblockhash(), 16), create_coinbase(node.getblockcount() + 1), block_time)
         block.solve()
         node.submitblock(block.serialize().hex())
+        # Set correct m_best_block_time, which is used in ResubmitWalletTransactions
         node.syncwithvalidationinterfacequeue()
 
         # Evict these txs from the mempool
         evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5
         node.setmocktime(evict_time)
         indep_send = node.send(outputs=[{node.getnewaddress(): 1}], options={"inputs": [indep_utxo]})
-        node.syncwithvalidationinterfacequeue()
         node.getmempoolentry(indep_send["txid"])
         assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid)
         assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, child_txid)

From 9f7bae55e6660e955f46a4865965a7d10a93abdf Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 8 Sep 2022 08:56:05 +0200
Subject: [PATCH 07/11] Merge bitcoin/bitcoin#26037: test: Fix
 `wallet_{basic,listsinceblock}.py` for BDB-only wallets

9f3a315c6f6af37d562a72f7d4fb2c53b5940871 test: Fix `wallet_listsinceblock.py` for BDB-only wallets (Hennadii Stepanov)
1941ce6cd15a1123db8769d17a6fd9b1cb3debcc test: Fix `wallet_basic.py` for BDB-only wallets (Hennadii Stepanov)

Pull request description:

  Fixes bitcoin/bitcoin#26029.

ACKs for top commit:
  brunoerg:
    crACK 9f3a315c6f6af37d562a72f7d4fb2c53b5940871

Tree-SHA512: d31c76e558dedea689ff487644e9f2d2f1df1cc2bb9bb041ede4b272884871167fdb19ccc717394c6ba6af8b8c70e9575b344988e0ce55b241a3a4922d0b7f73

Co-authored-by: MacroFake <falke.marco@gmail.com>
---
 test/functional/wallet_basic.py          | 63 ++++++++++++------------
 test/functional/wallet_listsinceblock.py |  3 +-
 2 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index d3158b7c942a..46daf2de4adb 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -753,37 +753,38 @@ def run_test(self):
         txid_feeReason_four = self.nodes[2].sendmany(dummy='', amounts={address: 5}, verbose=False)
         assert_equal(self.nodes[2].gettransaction(txid_feeReason_four)['txid'], txid_feeReason_four)
 
-        self.log.info("Testing 'listunspent' outputs the parent descriptor(s) of coins")
-        # Create two multisig descriptors, and send a UTxO each.
-        multi_a = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YBNjUo96Jxd1u4XKWgnoc7LsA1jz3Yc2NiDbhtfBhaBtemB73n9V5vtJHwU6FVXwggTbeoJWQ1rzdz8ysDuQkpnaHyvnvzR/*,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*))")
-        multi_b = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*,tpubD6NzVbkrYhZ4Y2RLiuEzNQkntjmsLpPYDm3LTRBYynUQtDtpzeUKAcb9sYthSFL3YR74cdFgF5mW8yKxv2W2CWuZDFR2dUpE5PF9kbrVXNZ/*))")
-        addr_a = self.nodes[0].deriveaddresses(multi_a, 0)[0]
-        addr_b = self.nodes[0].deriveaddresses(multi_b, 0)[0]
-        txid_a = self.nodes[0].sendtoaddress(addr_a, 0.01)
-        txid_b = self.nodes[0].sendtoaddress(addr_b, 0.01)
-        self.generate(self.nodes[0], 1, sync_fun=self.no_op)
-        # Now import the descriptors, make sure we can identify on which descriptor each coin was received.
-        self.nodes[0].createwallet(wallet_name="wo", descriptors=True, disable_private_keys=True)
-        wo_wallet = self.nodes[0].get_wallet_rpc("wo")
-        wo_wallet.importdescriptors([
-            {
-                "desc": multi_a,
-                "active": False,
-                "timestamp": "now",
-            },
-            {
-                "desc": multi_b,
-                "active": False,
-                "timestamp": "now",
-            },
-        ])
-        coins = wo_wallet.listunspent(minconf=0)
-        assert_equal(len(coins), 2)
-        coin_a = next(c for c in coins if c["txid"] == txid_a)
-        assert_equal(coin_a["parent_descs"][0], multi_a)
-        coin_b = next(c for c in coins if c["txid"] == txid_b)
-        assert_equal(coin_b["parent_descs"][0], multi_b)
-        self.nodes[0].unloadwallet("wo")
+        if self.options.descriptors:
+            self.log.info("Testing 'listunspent' outputs the parent descriptor(s) of coins")
+            # Create two multisig descriptors, and send a UTxO each.
+            multi_a = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YBNjUo96Jxd1u4XKWgnoc7LsA1jz3Yc2NiDbhtfBhaBtemB73n9V5vtJHwU6FVXwggTbeoJWQ1rzdz8ysDuQkpnaHyvnvzR/*,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*))")
+            multi_b = descsum_create("sh(multi(1,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*,tpubD6NzVbkrYhZ4Y2RLiuEzNQkntjmsLpPYDm3LTRBYynUQtDtpzeUKAcb9sYthSFL3YR74cdFgF5mW8yKxv2W2CWuZDFR2dUpE5PF9kbrVXNZ/*))")
+            addr_a = self.nodes[0].deriveaddresses(multi_a, 0)[0]
+            addr_b = self.nodes[0].deriveaddresses(multi_b, 0)[0]
+            txid_a = self.nodes[0].sendtoaddress(addr_a, 0.01)
+            txid_b = self.nodes[0].sendtoaddress(addr_b, 0.01)
+            self.generate(self.nodes[0], 1, sync_fun=self.no_op)
+            # Now import the descriptors, make sure we can identify on which descriptor each coin was received.
+            self.nodes[0].createwallet(wallet_name="wo", descriptors=True, disable_private_keys=True)
+            wo_wallet = self.nodes[0].get_wallet_rpc("wo")
+            wo_wallet.importdescriptors([
+                {
+                    "desc": multi_a,
+                    "active": False,
+                    "timestamp": "now",
+                },
+                {
+                    "desc": multi_b,
+                    "active": False,
+                    "timestamp": "now",
+                },
+            ])
+            coins = wo_wallet.listunspent(minconf=0)
+            assert_equal(len(coins), 2)
+            coin_a = next(c for c in coins if c["txid"] == txid_a)
+            assert_equal(coin_a["parent_descs"][0], multi_a)
+            coin_b = next(c for c in coins if c["txid"] == txid_b)
+            assert_equal(coin_b["parent_descs"][0], multi_b)
+            self.nodes[0].unloadwallet("wo")
 
 
 if __name__ == '__main__':
diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py
index d2413838c396..5ae6dab1042d 100755
--- a/test/functional/wallet_listsinceblock.py
+++ b/test/functional/wallet_listsinceblock.py
@@ -45,7 +45,8 @@ def run_test(self):
         self.test_double_send()
         self.double_spends_filtered()
         self.test_targetconfirmations()
-        self.test_desc()
+        if self.options.descriptors:
+            self.test_desc()
         self.test_send_to_self()
         self.test_op_return()
 

From 427067d4bf9ad681b2243cbf65ae351896c624d6 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Fri, 9 Sep 2022 10:35:00 +0200
Subject: [PATCH 08/11] Merge bitcoin/bitcoin#25990: test: apply fixed feerate
 to avoid variable dynamic fees in wallet_groups.py

BACKPORT NOTE:
This backport pinned every node to 20 duffs/byte via `-paytxfee`, but the
`maxapsfee` expectations were left at their dynamic-fee values. Upstream had
already recomputed them for 20 sat/vB before that commit landed, so it carried
no such update and the mismatch was invisible in the diff.

The expected debug lines still spelled out the transaction sizes (225/372 and
519/813 bytes) rather than the fees the wallet now logs, and the `maxapsfee`
thresholds still bracketed the un-pinned grouped/non-grouped delta of 294
duffs instead of 5880, so node 3 picked the non-grouped solution where the test
expects the grouped one.

------------

2186608172b0e6c3a9907e06fed27dfd927abd45 test: apply fixed feerate to avoid variable dynamic fees (stickies-v)

Pull request description:

  Without specifying a feerate, we let the wallet decide on an appropriate feerate, which can be influenced by various factors
  such as what's in the mempool. Since wallet_groups.py fails when feerates are unstable, we should use a fixed feerate across all nodes. The assumed feerate was 20 sats/vbyte, so this PR adopts that.

  Closes #25940. I'm not 100% sure, but I think the increased tx relay speed introduced by #25865 caused the transactions to more quickly and often enter the other nodes' mempools, affecting their feerate calculation done in [`wallet:GetMinimumFeeRate()`](https://github.com/bitcoin/bitcoin/blob/ea67232cdb80c4bc3f16fcd823f6f811fd8903e1/src/wallet/fees.cpp#L68-L72) and thus deviating slightly from the expected 20 sats/vbyte.

  Ran `wallet_groups.py` over 400 times without failure.

ACKs for top commit:
  aureleoules:
    ACK 2186608172b0e6c3a9907e06fed27dfd927abd45.
  glozow:
    Approach ACK 2186608172b0e6c3a9907e06fed27dfd927abd45

Tree-SHA512: 0ea467a67747e6f27369ccd0adacfb21cc36ef0ae728fb28b8ea18e409aab5bd3ede559d6cebb82da0b9703c0c8b2709d686feb3ae009ddf525aa253f44d5816

Co-authored-by: MacroFake <falke.marco@gmail.com>
---
 test/functional/wallet_groups.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/test/functional/wallet_groups.py b/test/functional/wallet_groups.py
index 6d921e847f2f..01cd7f126f69 100755
--- a/test/functional/wallet_groups.py
+++ b/test/functional/wallet_groups.py
@@ -25,12 +25,13 @@ def set_test_params(self):
             [],
             [],
             ["-avoidpartialspends"],
-            ["-maxapsfee=0.00000293"],
-            ["-maxapsfee=0.00000294"],
+            ["-maxapsfee=0.00005879"],
+            ["-maxapsfee=0.00005880"],
         ]
-        # whitelist peers to speed up tx relay / mempool sync
+
         for args in self.extra_args:
-            args.append("-whitelist=noban@127.0.0.1")
+            args.append("-whitelist=noban@127.0.0.1")   # whitelist peers to speed up tx relay / mempool sync
+            args.append(f"-paytxfee={20 * 1e3 / 1e8}")  # apply feerate of 20 sats/vB across all nodes
 
         self.rpc_timeout = 480
         self.supports_cli = False
@@ -125,7 +126,7 @@ def run_test(self):
         self.nodes[0].sendtoaddress(addr_aps, 1.0)
         self.nodes[0].sendtoaddress(addr_aps, 1.0)
         self.generate(self.nodes[0], 1)
-        with self.nodes[3].assert_debug_log(['Fee non-grouped = 225, grouped = 372, using grouped']):
+        with self.nodes[3].assert_debug_log(['Fee non-grouped = 4500, grouped = 7440, using grouped']):
             txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
         tx4 = self.nodes[3].getrawtransaction(txid4, True)
         # tx4 should have 2 inputs and 2 outputs although one output would
@@ -136,7 +137,7 @@ def run_test(self):
         addr_aps2 = self.nodes[3].getnewaddress()
         [self.nodes[0].sendtoaddress(addr_aps2, 1.0) for _ in range(5)]
         self.generate(self.nodes[0], 1)
-        with self.nodes[3].assert_debug_log(['Fee non-grouped = 519, grouped = 813, using non-grouped']):
+        with self.nodes[3].assert_debug_log(['Fee non-grouped = 10380, grouped = 16260, using non-grouped']):
             txid5 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 2.95)
         tx5 = self.nodes[3].getrawtransaction(txid5, True)
         # tx5 should have 3 inputs (1.0, 1.0, 1.0) and 2 outputs
@@ -149,7 +150,7 @@ def run_test(self):
         addr_aps3 = self.nodes[4].getnewaddress()
         [self.nodes[0].sendtoaddress(addr_aps3, 1.0) for _ in range(5)]
         self.generate(self.nodes[0], 1)
-        with self.nodes[4].assert_debug_log(['Fee non-grouped = 519, grouped = 813, using grouped']):
+        with self.nodes[4].assert_debug_log(['Fee non-grouped = 10380, grouped = 16260, using grouped']):
             txid6 = self.nodes[4].sendtoaddress(self.nodes[0].getnewaddress(), 2.95)
         tx6 = self.nodes[4].getrawtransaction(txid6, True)
         # tx6 should have 5 inputs and 2 outputs

From ad13ec2d27347ef5baf9aac97b079d9fa75b4998 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Tue, 13 Sep 2022 11:37:05 -0400
Subject: [PATCH 09/11] Merge bitcoin/bitcoin#26021: wallet: bugfix, load a
 wallet with an unknown/corrupt descriptor causes a fatal error

e06676377d935c69f0ee51fc18eb0772d524aba5 wallet: coverage for loading an unknown descriptor (furszy)
d26c3cc44438ecb9e4f618a2427c3c92a292aa16 wallet: bugfix, load wallet with an unknown descriptor cause fatal error (furszy)

Pull request description:

  Fixes #26015

  If the descriptor entry is unrecognized (due a soft downgrade) or corrupt, the
  unserialization fails and `LoadWallet`, instead of stop there and return the error,
  continues reading all the db records. As other records tied to the unrecognized
  or corrupt descriptor are scanned, a fatal error is being thrown.

  This fixes it by catching the descriptor parse failure and return which wallet failed.
  Logging its name/path, so the user can remove it from the settings file, to prevent
  its load at startup.

  Note: added the test in a separate file intentionally.
  Will continue adding coverage for the wallet load process in follow-up PRs.

ACKs for top commit:
  achow101:
    ACK e06676377d935c69f0ee51fc18eb0772d524aba5
  Sjors:
    re-utACK e06676377d935c69f0ee51fc18eb0772d524aba5

Tree-SHA512: d1f1a5d7e944c89c97a33b25b4411a36a11edae172c22f8524f69c84a035f84c570b284679f901fe60f1300f781b76a6c17b015a8e7ad44ebd25a0c295ef260f

Co-authored-by: Andrew Chow <github@achow101.com>
---
 src/Makefile.test.include            |  3 +-
 src/wallet/test/walletload_tests.cpp | 54 ++++++++++++++++++++++++++++
 src/wallet/wallet.cpp                |  8 +++--
 src/wallet/walletdb.cpp              | 27 ++++++++++----
 src/wallet/walletdb.h                |  3 +-
 5 files changed, 85 insertions(+), 10 deletions(-)
 create mode 100644 src/wallet/test/walletload_tests.cpp

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 427909b8e12f..42486408b59d 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -235,7 +235,8 @@ BITCOIN_TESTS += \
   wallet/test/init_tests.cpp \
   wallet/test/ismine_tests.cpp \
   wallet/test/rpc_util_tests.cpp \
-  wallet/test/scriptpubkeyman_tests.cpp
+  wallet/test/scriptpubkeyman_tests.cpp \
+  wallet/test/walletload_tests.cpp
 
 FUZZ_SUITE_LD_COMMON +=\
  $(SQLITE_LIBS) \
diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
new file mode 100644
index 000000000000..c77768a20564
--- /dev/null
+++ b/src/wallet/test/walletload_tests.cpp
@@ -0,0 +1,54 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <wallet/wallet.h>
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+namespace wallet {
+
+BOOST_AUTO_TEST_SUITE(walletload_tests)
+
+class DummyDescriptor final : public Descriptor {
+private:
+    std::string desc;
+public:
+    explicit DummyDescriptor(const std::string& descriptor) : desc(descriptor) {};
+    ~DummyDescriptor() = default;
+
+    std::string ToString() const override { return desc; }
+    std::optional<OutputType> GetOutputType() const override { return OutputType::UNKNOWN; }
+
+    bool IsRange() const override { return false; }
+    bool IsSolvable() const override { return false; }
+    bool IsSingleType() const override { return true; }
+    bool ToPrivateString(const SigningProvider& provider, std::string& out) const override { return false; }
+    bool ToNormalizedString(const SigningProvider& provider, std::string& out, const DescriptorCache* cache = nullptr) const override { return false; }
+    bool Expand(int pos, const SigningProvider& provider, std::vector<CScript>& output_scripts, FlatSigningProvider& out, DescriptorCache* write_cache = nullptr) const override { return false; };
+    bool ExpandFromCache(int pos, const DescriptorCache& read_cache, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const override { return false; }
+    void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const override {}
+};
+
+BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup)
+{
+    std::unique_ptr<WalletDatabase> database = CreateMockWalletDatabase();
+    {
+        // 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);
+        BOOST_CHECK(batch.WriteDescriptor(uint256(), wallet_descriptor));
+        BOOST_CHECK(batch.WriteActiveScriptPubKeyMan(uint256(), false));
+    }
+
+    {
+        // Now try to load the wallet and verify the error.
+        const std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, std::move(database)));
+        BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::UNKNOWN_DESCRIPTOR);
+    }
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+} // namespace wallet
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index bcd726482ac3..634a753520ff 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3135,8 +3135,12 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
             warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect."
                                            " Rescanning wallet."), walletFile));
             rescan_required = true;
-        }
-        else {
+        } else if (nLoadWalletRet == DBErrors::UNKNOWN_DESCRIPTOR) {
+            error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n"
+                                "The wallet might had been created on a newer version.\n"
+                                "Please try running the latest software version.\n"), walletFile);
+            return nullptr;
+        } else {
             error = strprintf(_("Error loading %s"), walletFile);
             return nullptr;
         }
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 6a1f62c3ff09..4b82c166a345 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -356,6 +356,7 @@ class CWalletScanState {
     std::map<std::pair<uint256, CKeyID>, std::pair<SecureString, SecureString>> mnemonics;
     std::map<std::pair<uint256, CKeyID>, std::pair<std::vector<unsigned char>, std::vector<unsigned char>>> crypted_mnemonics;
     bool tx_corrupt{false};
+    bool descriptor_unknown{false};
 
     CWalletScanState() = default;
 };
@@ -663,7 +664,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
             uint256 id;
             ssKey >> id;
             WalletDescriptor desc;
-            ssValue >> desc;
+            try {
+                ssValue >> desc;
+            } catch (const std::ios_base::failure& e) {
+                strErr = e.what();
+                wss.descriptor_unknown = true;
+                return false;
+            }
             if (wss.m_descriptor_caches.count(id) == 0) {
                 wss.m_descriptor_caches[id] = DescriptorCache();
             }
@@ -836,6 +843,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
     DBErrors result = DBErrors::LOAD_OK;
 
     LOCK(pwallet->cs_wallet);
+
+    // Last client version to open this wallet
+    int last_client = CLIENT_VERSION;
+    bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client);
+    pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client);
+
     try {
         int nMinVersion = 0;
         if (m_batch->Read(DBKeys::MINVERSION, nMinVersion)) {
@@ -901,6 +914,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                     // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
                     wss.tx_corrupt = false;
                     result = DBErrors::CORRUPT;
+                } else if (wss.descriptor_unknown) {
+                    strErr = strprintf("Error: Unrecognized descriptor found in wallet %s. ", pwallet->GetName());
+                    strErr += (last_client > CLIENT_VERSION) ? "The wallet might had been created on a newer version. " :
+                            "The database might be corrupted or the software version is not compatible with one of your wallet descriptors. ";
+                    strErr += "Please try running the latest software version";
+                    pwallet->WalletLogPrintf("%s\n", strErr);
+                    return DBErrors::UNKNOWN_DESCRIPTOR;
                 } else {
                     // Leave other errors alone, if we try to fix them we might make things worse.
                     fNoncriticalErrors = true; // ... but do warn the user there is something wrong.
@@ -985,11 +1005,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
     pwallet->nKeysLeftSinceAutoBackup = pwallet->KeypoolCountExternalKeys();
     pwallet->WalletLogPrintf("nKeysLeftSinceAutoBackup: %d\n", pwallet->nKeysLeftSinceAutoBackup);
 
-    // Last client version to open this wallet
-    int last_client = CLIENT_VERSION;
-    bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client);
-    pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client);
-
     pwallet->WalletLogPrintf("Keys: %u plaintext, %u encrypted, %u total; Watch scripts: %u; HD PubKeys: %u; Metadata: %u; Unknown wallet records: %u\n",
            wss.nKeys, wss.nCKeys, wss.nKeys + wss.nCKeys,
            wss.nWatchKeys, wss.nHDPubKeys, wss.nKeyMeta, wss.m_unknown_records);
diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h
index fac9f7453e02..30c6aebdca30 100644
--- a/src/wallet/walletdb.h
+++ b/src/wallet/walletdb.h
@@ -58,7 +58,8 @@ enum class DBErrors
     EXTERNAL_SIGNER_SUPPORT_REQUIRED,
     LOAD_FAIL,
     NEED_REWRITE,
-    NEED_RESCAN
+    NEED_RESCAN,
+    UNKNOWN_DESCRIPTOR
 };
 
 namespace DBKeys {

From b4dab95424e01e7ad4820848dbbafc4f6b0f92d6 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 13 Oct 2022 11:57:35 +0800
Subject: [PATCH 10/11] Merge bitcoin/bitcoin#26205: wallet: #25768 follow ups

b01682a812f0841170657708ef0e896b904fcd77 refactor: revert m_next_resend to not be std::atomic (stickies-v)
9245f456705b285e2d9afcc01a6155e1b3f92fad wallet: only update m_next_resend when actually resending (stickies-v)
7fbde8af5c06694eecd4ce601109bd826a54bd6f refactor: carve out tx resend timer logic into ShouldResend (stickies-v)
01f3534632d18c772901fb6ce22f6394eae96799 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v)
c6e8e11fb030ef406752761530421a9e2f0f5d4f wallet: fix capitalization in docstring (stickies-v)

Pull request description:

  This PR addresses the outstanding comments/issues from #25768:

  - capitalization [typo](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958572522) in docstring
  - remove [unused locks](https://github.com/bitcoin/bitcoin/commit/01f3534632d18c772901fb6ce22f6394eae96799) that we previously needed for `ReacceptWalletTransactions()`
  - before #25768, only `ResendWalletTransactions()` would reset `m_next_resend` (formerly called `nNextResend`). By unifying it with `ReacceptWalletTransactions()` into `ResubmitWalletTransactions()`, the number of callsites that would reset the `m_next_resend` timer increased
    - since `m_next_resend` is only used in case of `relay=true` (formerly `ResendWalletTransactions()`), this is unintuitive
    - it leads to [unexpected behaviour](https://github.com/bitcoin/bitcoin/pull/25768#issuecomment-1252619427) such as transactions potentially never being rebroadcasted.
    - it makes the ResubmitWalletTransactions()` logic [more complicated than strictly necessary](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r962828563)
    - since #25768, we relied on an earlier call of `ResubmitWalletTransactions(relay=false, force=true)` to initialize `m_next_resend()`, I think we can more elegantly do that by just providing `m_next_resend` with a default value
    - just to highlight: this commit introduces behaviour change

  Note: the `if (!fBroadcastTransactions)` in `CWallet:ShouldResend()` is duplicated on purpose, since it potentially avoids the slightly more expensive `if (!chain().isReadyToBroadcast())` check afterwards. I don't have a strong view on it, so happy to remove that additional check to reduce the diff, too.

ACKs for top commit:
  aureleoules:
    ACK b01682a812f0841170657708ef0e896b904fcd77
  achow101:
    ACK b01682a812f0841170657708ef0e896b904fcd77

Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c

Co-authored-by: fanquake <fanquake@gmail.com>
---
 src/wallet/rpc/backup.cpp | 20 ++++---------------
 src/wallet/wallet.cpp     | 41 ++++++++++++++++++++++++---------------
 src/wallet/wallet.h       |  8 +++++++-
 3 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 54d6786b1090..c0bd143be002 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -296,10 +296,7 @@ RPCHelpMan importaddress()
     if (fRescan)
     {
         RescanWallet(*pwallet, reserver);
-        {
-            LOCK(pwallet->cs_wallet);
-            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
-        }
+        pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
     }
 
     return UniValue::VNULL;
@@ -475,10 +472,7 @@ RPCHelpMan importpubkey()
     if (fRescan)
     {
         RescanWallet(*pwallet, reserver);
-        {
-            LOCK(pwallet->cs_wallet);
-            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
-        }
+        pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
     }
 
     return UniValue::VNULL;
@@ -1638,10 +1632,7 @@ RPCHelpMan importmulti()
     }
     if (fRescan && fRunScan && requests.size()) {
         int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */);
-        {
-            LOCK(pwallet->cs_wallet);
-            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
-        }
+        pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
 
         if (pwallet->IsAbortingRescan()) {
             throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");
@@ -1933,10 +1924,7 @@ RPCHelpMan importdescriptors() {
     // Rescan the blockchain using the lowest timestamp
     if (rescan) {
         int64_t scanned_time = pwallet->RescanFromTime(lowest_timestamp, reserver, true /* update */);
-        {
-            LOCK(pwallet->cs_wallet);
-            pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
-        }
+        pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
 
         if (pwallet->IsAbortingRescan()) {
             throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 634a753520ff..1a073153ef81 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -22,6 +22,7 @@
 #include <primitives/block.h>
 #include <primitives/transaction.h>
 #include <psbt.h>
+#include <random.h>
 #include <script/descriptor.h>
 #include <script/script.h>
 #include <script/sign.h>
@@ -2116,11 +2117,30 @@ std::set<uint256> CWallet::GetTxConflicts(const CWalletTx& wtx) const
     return result;
 }
 
+bool CWallet::ShouldResend() const
+{
+    // Don't attempt to resubmit if the wallet is configured to not broadcast
+    if (!fBroadcastTransactions) return false;
+
+    // During reindex, importing and IBD, old wallet transactions become
+    // unconfirmed. Don't resend them as that would spam other nodes.
+    // We only allow forcing mempool submission when not relaying to avoid this spam.
+    if (!chain().isReadyToBroadcast()) return false;
+
+    // Do this infrequently and randomly to avoid giving away
+    // that these are our transactions.
+    if (GetTime() < m_next_resend) return false;
+
+    return true;
+}
+
+int64_t CWallet::GetDefaultNextResend() { return GetTime() + (1 * 60 * 60) + GetRand(2 * 60 * 60); }
+
 // Resubmit transactions from the wallet to the mempool, optionally asking the
 // mempool to relay them. On startup, we will do this for all unconfirmed
 // transactions but will not ask the mempool to relay them. We do this on startup
 // to ensure that our own mempool is aware of our transactions, and to also
-// initialize nNextResend so that the actual rebroadcast is scheduled. There
+// initialize m_next_resend so that the actual rebroadcast is scheduled. There
 // is a privacy side effect here as not broadcasting on startup also means that we won't
 // inform the world of our wallet's state, particularly if the wallet (or node) is not
 // yet synced.
@@ -2147,17 +2167,6 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force)
     // even if forcing.
     if (!fBroadcastTransactions) return;
 
-    // During reindex, importing and IBD, old wallet transactions become
-    // unconfirmed. Don't resend them as that would spam other nodes.
-    // We only allow forcing mempool submission when not relaying to avoid this spam.
-    if (!force && relay && !chain().isReadyToBroadcast()) return;
-
-    // Do this infrequently and randomly to avoid giving away
-    // that these are our transactions.
-    if (!force && GetTime() < m_next_resend) return;
-    // resend 1-3 hours from now, ~2 hours on average.
-    m_next_resend = GetTime() + (1 * 60 * 60) + GetRand(2 * 60 * 60);
-
     int submitted_tx_count = 0;
 
     { // cs_wallet scope
@@ -2170,7 +2179,7 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force)
             // Only rebroadcast unconfirmed txs
             if (!wtx.isUnconfirmed()) continue;
 
-            // attempt to rebroadcast all txes more than 5 minutes older than
+            // Attempt to rebroadcast all txes more than 5 minutes older than
             // the last block, or all txs if forcing.
             if (!force && wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
             to_submit.insert(&wtx);
@@ -2192,7 +2201,9 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force)
 void MaybeResendWalletTxs(WalletContext& context)
 {
     for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
+        if (!pwallet->ShouldResend()) continue;
         pwallet->ResubmitWalletTransactions(/*relay=*/true, /*force=*/false);
+        pwallet->SetNextResend();
     }
 }
 
@@ -3580,14 +3591,12 @@ const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest
 
 void CWallet::postInitProcess()
 {
-    LOCK(cs_wallet);
-
     // Add wallet transactions that aren't already in a block to mempool
     // Do this here as mempool requires genesis block to be loaded
     ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
 
     // Update wallet transactions with current mempool transactions.
-    chain().requestMempoolTransactions(*this);
+    WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
 }
 
 void CWallet::InitAutoBackup()
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index cbf419734f54..b336b3cef90d 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -305,7 +305,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     int nWalletVersion GUARDED_BY(cs_wallet){FEATURE_BASE};
 
     /** The next scheduled rebroadcast of wallet transactions. */
-    std::atomic<int64_t> m_next_resend{};
+    int64_t m_next_resend{GetDefaultNextResend()};
     /** Whether this wallet will submit newly created transactions to the node's mempool and
      * prompt rebroadcasts (see ResendWalletTransactions()). */
     bool fBroadcastTransactions = false;
@@ -447,6 +447,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
      */
     static bool AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector<bilingual_str>& warnings);
 
+    static int64_t GetDefaultNextResend();
+
 public:
     /**
      * Main wallet lock.
@@ -706,6 +708,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     };
     ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress);
     void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override;
+    /** Set the next time this wallet should resend transactions to 1-3hours from now, ~2 hours on average. */
+    void SetNextResend() { m_next_resend = GetDefaultNextResend(); }
+    /** Return true if all conditions for periodically resending transactions are met. */
+    bool ShouldResend() const;
     void ResubmitWalletTransactions(bool relay, bool force);
 
     CAmount GetAnonymizableBalance(bool fSkipDenominated = false, bool fSkipUnconfirmed = true) const;

From 6d1968bb701e6ef065f1d2d692ff9e617826e41d Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Tue, 4 Oct 2022 09:57:05 +0100
Subject: [PATCH 11/11] Merge bitcoin/bitcoin#26203: wallet: Use correct
 effective value when checking target
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

BACKPORT NOTE to Dash Core
Kept upstream's numbers even though Dash reads `SetInputWeight()` through
`GetVirtualTransactionSize(nSize, 0, 0)`, which is the identity here rather than
upstream's weight-to-vsize division: the input costs 148 bytes instead of 37, so
the coin's effective value is 99556 rather than 99889. Either way it stays below
the 99900 target, which is what the test needs.

----

d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9 test: Check external coin effective value is used in CoinSelection (Aurèle Oulès)
76b79c1a177afa006184d716bd3d5b22ebadb168 wallet: Use correct effective value when checking target (Aurèle Oulès)

Pull request description:

  Fixes #26185. The following assert failed because it was not checked in the parent function.

  https://github.com/bitcoin/bitcoin/blob/2bd9aa5a44b88c866c4d98f8a7bf7154049cba31/src/wallet/coinselection.cpp#L391

ACKs for top commit:
  glozow:
    reACK d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9
  furszy:
    ACK d0d9cf7a

Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9

Co-authored-by: glozow <gloriajzhao@gmail.com>
---
 src/wallet/spend.cpp                   |  8 ++++-
 src/wallet/test/coinselector_tests.cpp | 47 ++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 2896d88f1457..9c8132804272 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -590,7 +590,13 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
         if (all_inputs) {
             result.AddInput(preset_inputs);
         }
-        if (result.GetSelectedValue() < nTargetValue) return std::nullopt;
+
+        if (!coin_selection_params.m_subtract_fee_outputs && result.GetSelectedEffectiveValue() < nTargetValue) {
+            return std::nullopt;
+        } else if (result.GetSelectedValue() < nTargetValue) {
+            return std::nullopt;
+        }
+
         result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
         return result;
     }
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index 7e34cea303ef..ead064077de6 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -934,6 +934,53 @@ BOOST_AUTO_TEST_CASE(effective_value_test)
     BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1
 }
 
+BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test)
+{
+    // Test that the effective value is used to check whether preset inputs provide sufficient funds when subtract_fee_outputs is not used.
+    // This test creates a coin whose value is higher than the target but whose effective value is lower than the target.
+    // The coin is selected using coin control, with m_allow_other_inputs = false. SelectCoins should fail due to insufficient funds.
+
+    std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
+    wallet->LoadWallet();
+    LOCK(wallet->cs_wallet);
+    wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+    wallet->SetupDescriptorScriptPubKeyMans("", "");
+
+    CoinsResult available_coins;
+    {
+        std::unique_ptr<CWallet> dummyWallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "dummy", m_args, CreateMockWalletDatabase());
+        dummyWallet->LoadWallet();
+        LOCK(dummyWallet->cs_wallet);
+        dummyWallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+        dummyWallet->SetupDescriptorScriptPubKeyMans("", "");
+
+        add_coin(available_coins, *dummyWallet, 100000); // 0.001 DASH
+    }
+
+    CAmount target{99900}; // 0.000999 DASH
+
+    FastRandomContext rand;
+    CoinSelectionParams cs_params{
+        rand,
+        /*change_output_size=*/34,
+        /*change_spend_size=*/148,
+        /*min_change_target=*/1000,
+        /*effective_feerate=*/CFeeRate(3000),
+        /*long_term_feerate=*/CFeeRate(1000),
+        /*discard_feerate=*/CFeeRate(1000),
+        /*tx_noinputs_size=*/0,
+        /*avoid_partial=*/false,
+    };
+    CCoinControl cc;
+    cc.m_allow_other_inputs = false;
+    COutput output = available_coins.all().at(0);
+    cc.SetInputWeight(output.outpoint, 148);
+    cc.SelectExternal(output.outpoint, output.txout);
+
+    const auto result = SelectCoins(*wallet, available_coins, target, cc, cs_params);
+    BOOST_CHECK(!result);
+}
+
 static util::Result<SelectionResult> SelectCoinsSRDResult(const CAmount& target,
                                                           const CoinSelectionParams& cs_params,
                                                           const node::NodeContext& m_node,