Merge #17792: 0.19: Backports

cd67b1dcb8 Use correct C++11 header for std::swap() (Hennadii Stepanov)
b8101fb7ac Fix comparison function signature (Hennadii Stepanov)
eac49073eb Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
e2c45d89f7 IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
a5489c9892 IsUsedDestination should count any known single-key address (Gregory Sanders)
88729d804e Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)
eafcea7a0a gui: Fix duplicate wallet showing up (João Barbosa)
7e66d04770 Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky)
179d55f052 zmq: Fix due to invalid argument and multiple notifiers (João Barbosa)

Pull request description:

  Backports
   - #16963
   - #17445
   - #17258
   - #17621
   - #17924
   - #17634

ACKs for top commit:
  laanwj:
    ACK cd67b1dcb8, checked that I got more or less the same result (including conflict resolution) backporting these commits

Tree-SHA512: 645786267cfb10a01a56f7cfd91ddead5f1475df5714595ae480237e04d40c5cfb7460b40532279cacd83e4b775a4ace68a258ec2184b8ad0e997a690a9245e5

src/cuckoocache.h

@@ -6,11 +6,11 @@
 #define BITCOIN_CUCKOOCACHE_H
 
 #include <array>
-#include <algorithm>
 #include <atomic>
-#include <cstring>
 #include <cmath>
+#include <cstring>
 #include <memory>
+#include <utility>
 #include <vector>
 
 

src/dummywallet.cpp

@@ -12,6 +12,8 @@ enum class WalletCreationStatus;
 
 namespace interfaces {
 class Chain;
+class Handler;
+class Wallet;
 }
 
 class DummyWalletInit : public WalletInitInterface {
@@ -81,9 +83,13 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString&
     throw std::logic_error("Wallet function called in non-wallet build.");
 }
 
-namespace interfaces {
+using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
+std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet)
+{
+    throw std::logic_error("Wallet function called in non-wallet build.");
+}
 
-class Wallet;
+namespace interfaces {
 
 std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet)
 {

src/interfaces/chain.cpp

@@ -339,7 +339,6 @@ public:
     void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
     void initWarning(const std::string& message) override { InitWarning(message); }
     void initError(const std::string& message) override { InitError(message); }
-    void loadWallet(std::unique_ptr<Wallet> wallet) override { ::uiInterface.LoadWallet(wallet); }
     void showProgress(const std::string& title, int progress, bool resume_possible) override
     {
         ::uiInterface.ShowProgress(title, progress, resume_possible);

src/interfaces/chain.h

@@ -43,7 +43,7 @@ class Wallet;
 //!   asynchronously
 //!   (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269).
 //!
-//! * The initMessages() and loadWallet() methods which the wallet uses to send
+//! * The initMessage() and showProgress() methods which the wallet uses to send
 //!   notifications to the GUI should go away when GUI and wallet can directly
 //!   communicate with each other without going through the node
 //!   (https://github.com/bitcoin/bitcoin/pull/15288#discussion_r253321096).
@@ -208,9 +208,6 @@ public:
     //! Send init error.
     virtual void initError(const std::string& message) = 0;
 
-    //! Send wallet load notification to the GUI.
-    virtual void loadWallet(std::unique_ptr<Wallet> wallet) = 0;
-
     //! Send progress indicator.
     virtual void showProgress(const std::string& title, int progress, bool resume_possible) = 0;
 

src/interfaces/handler.cpp

@@ -22,6 +22,15 @@ public:
     boost::signals2::scoped_connection m_connection;
 };
 
+class CleanupHandler : public Handler
+{
+public:
+    explicit CleanupHandler(std::function<void()> cleanup) : m_cleanup(std::move(cleanup)) {}
+    ~CleanupHandler() override { if (!m_cleanup) return; m_cleanup(); m_cleanup = nullptr; }
+    void disconnect() override { if (!m_cleanup) return; m_cleanup(); m_cleanup = nullptr; }
+    std::function<void()> m_cleanup;
+};
+
 } // namespace
 
 std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection)
@@ -29,4 +38,9 @@ std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection)
     return MakeUnique<HandlerImpl>(std::move(connection));
 }
 
+std::unique_ptr<Handler> MakeHandler(std::function<void()> cleanup)
+{
+    return MakeUnique<CleanupHandler>(std::move(cleanup));
+}
+
 } // namespace interfaces

src/interfaces/handler.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_INTERFACES_HANDLER_H
 #define BITCOIN_INTERFACES_HANDLER_H
 
+#include <functional>
 #include <memory>
 
 namespace boost {
@@ -30,6 +31,9 @@ public:
 //! Return handler wrapping a boost signal connection.
 std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection);
 
+//! Return handler wrapping a cleanup function.
+std::unique_ptr<Handler> MakeHandler(std::function<void()> cleanup);
+
 } // namespace interfaces
 
 #endif // BITCOIN_INTERFACES_HANDLER_H

src/interfaces/node.cpp

@@ -45,11 +45,10 @@ std::vector<fs::path> ListWalletDir();
 std::vector<std::shared_ptr<CWallet>> GetWallets();
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string& name, std::string& error, std::string& warning);
 WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, std::shared_ptr<CWallet>& result);
+std::unique_ptr<interfaces::Handler> HandleLoadWallet(interfaces::Node::LoadWalletFn load_wallet);
 
 namespace interfaces {
 
-class Wallet;
-
 namespace {
 
 class NodeImpl : public Node
@@ -286,7 +285,7 @@ public:
     }
     std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
     {
-        return MakeHandler(::uiInterface.LoadWallet_connect([fn](std::unique_ptr<Wallet>& wallet) { fn(std::move(wallet)); }));
+        return HandleLoadWallet(std::move(fn));
     }
     std::unique_ptr<Handler> handleNotifyNumConnectionsChanged(NotifyNumConnectionsChangedFn fn) override
     {

src/prevector.h

@@ -14,6 +14,7 @@
 #include <cstddef>
 #include <iterator>
 #include <type_traits>
+#include <utility>
 
 #pragma pack(push, 1)
 /** Implements a drop-in replacement for std::vector<T> which stores up to N

src/qt/bantablemodel.cpp

@@ -10,7 +10,7 @@
 #include <sync.h>
 #include <util/time.h>
 
-#include <algorithm>
+#include <utility>
 
 #include <QDebug>
 #include <QList>

src/qt/bitcoingui.cpp

@@ -633,10 +633,10 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller)
 void BitcoinGUI::addWallet(WalletModel* walletModel)
 {
     if (!walletFrame) return;
+    if (!walletFrame->addWallet(walletModel)) return;
     const QString display_name = walletModel->getDisplayName();
     setWalletActionsEnabled(true);
     rpcConsole->addWallet(walletModel);
-    walletFrame->addWallet(walletModel);
     m_wallet_selector->addItem(display_name, QVariant::fromValue(walletModel));
     if (m_wallet_selector->count() == 2) {
         m_wallet_selector_label_action->setVisible(true);

src/qt/peertablemodel.cpp

@@ -11,7 +11,7 @@
 #include <interfaces/node.h>
 #include <sync.h>
 
-#include <algorithm>
+#include <utility>
 
 #include <QDebug>
 #include <QList>

src/qt/recentrequeststablemodel.cpp

@@ -11,8 +11,7 @@
 #include <clientversion.h>
 #include <streams.h>
 
-#include <algorithm>
-
+#include <utility>
 
 RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
     QAbstractTableModel(parent), walletModel(parent)
@@ -213,10 +212,10 @@ void RecentRequestsTableModel::updateDisplayUnit()
     updateAmountColumnTitle();
 }
 
-bool RecentRequestEntryLessThan::operator()(RecentRequestEntry &left, RecentRequestEntry &right) const
+bool RecentRequestEntryLessThan::operator()(const RecentRequestEntry& left, const RecentRequestEntry& right) const
 {
-    RecentRequestEntry *pLeft = &left;
-    RecentRequestEntry *pRight = &right;
+    const RecentRequestEntry* pLeft = &left;
+    const RecentRequestEntry* pRight = &right;
     if (order == Qt::DescendingOrder)
         std::swap(pLeft, pRight);
 

src/qt/recentrequeststablemodel.h

@@ -43,7 +43,7 @@ class RecentRequestEntryLessThan
 public:
     RecentRequestEntryLessThan(int nColumn, Qt::SortOrder fOrder):
         column(nColumn), order(fOrder) {}
-    bool operator()(RecentRequestEntry &left, RecentRequestEntry &right) const;
+    bool operator()(const RecentRequestEntry& left, const RecentRequestEntry& right) const;
 
 private:
     int column;

src/qt/walletframe.cpp

@@ -40,11 +40,11 @@ void WalletFrame::setClientModel(ClientModel *_clientModel)
     this->clientModel = _clientModel;
 }
 
-void WalletFrame::addWallet(WalletModel *walletModel)
+bool WalletFrame::addWallet(WalletModel *walletModel)
 {
-    if (!gui || !clientModel || !walletModel) return;
+    if (!gui || !clientModel || !walletModel) return false;
 
-    if (mapWalletViews.count(walletModel) > 0) return;
+    if (mapWalletViews.count(walletModel) > 0) return false;
 
     WalletView *walletView = new WalletView(platformStyle, this);
     walletView->setBitcoinGUI(gui);
@@ -68,6 +68,8 @@ void WalletFrame::addWallet(WalletModel *walletModel)
     });
 
     connect(walletView, &WalletView::outOfSyncWarningClicked, this, &WalletFrame::outOfSyncWarningClicked);
+
+    return true;
 }
 
 void WalletFrame::setCurrentWallet(WalletModel* wallet_model)

src/qt/walletframe.h

@@ -36,7 +36,7 @@ public:
 
     void setClientModel(ClientModel *clientModel);
 
-    void addWallet(WalletModel *walletModel);
+    bool addWallet(WalletModel *walletModel);
     void setCurrentWallet(WalletModel* wallet_model);
     void removeWallet(WalletModel* wallet_model);
     void removeAllWallets();

src/script/standard.h

@@ -81,9 +81,14 @@ struct PKHash : public uint160
     using uint160::uint160;
 };
 
+struct WitnessV0KeyHash;
 struct ScriptHash : public uint160
 {
     ScriptHash() : uint160() {}
+    // These don't do what you'd expect.
+    // Use ScriptHash(GetScriptForDestination(...)) instead.
+    explicit ScriptHash(const WitnessV0KeyHash& hash) = delete;
+    explicit ScriptHash(const PKHash& hash) = delete;
     explicit ScriptHash(const uint160& hash) : uint160(hash) {}
     explicit ScriptHash(const CScript& script);
     using uint160::uint160;

src/test/checkqueue_tests.cpp

@@ -18,6 +18,7 @@
 #include <condition_variable>
 
 #include <unordered_set>
+#include <utility>
 
 // BasicTestingSetup not sufficient because nScriptCheckThreads is not set
 // otherwise.

src/ui_interface.cpp

@@ -16,7 +16,6 @@ struct UISignals {
     boost::signals2::signal<CClientUIInterface::NotifyNumConnectionsChangedSig> NotifyNumConnectionsChanged;
     boost::signals2::signal<CClientUIInterface::NotifyNetworkActiveChangedSig> NotifyNetworkActiveChanged;
     boost::signals2::signal<CClientUIInterface::NotifyAlertChangedSig> NotifyAlertChanged;
-    boost::signals2::signal<CClientUIInterface::LoadWalletSig> LoadWallet;
     boost::signals2::signal<CClientUIInterface::ShowProgressSig> ShowProgress;
     boost::signals2::signal<CClientUIInterface::NotifyBlockTipSig> NotifyBlockTip;
     boost::signals2::signal<CClientUIInterface::NotifyHeaderTipSig> NotifyHeaderTip;
@@ -36,7 +35,6 @@ ADD_SIGNALS_IMPL_WRAPPER(InitMessage);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyNumConnectionsChanged);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyNetworkActiveChanged);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyAlertChanged);
-ADD_SIGNALS_IMPL_WRAPPER(LoadWallet);
 ADD_SIGNALS_IMPL_WRAPPER(ShowProgress);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyBlockTip);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyHeaderTip);
@@ -48,7 +46,6 @@ void CClientUIInterface::InitMessage(const std::string& message) { return g_ui_s
 void CClientUIInterface::NotifyNumConnectionsChanged(int newNumConnections) { return g_ui_signals.NotifyNumConnectionsChanged(newNumConnections); }
 void CClientUIInterface::NotifyNetworkActiveChanged(bool networkActive) { return g_ui_signals.NotifyNetworkActiveChanged(networkActive); }
 void CClientUIInterface::NotifyAlertChanged() { return g_ui_signals.NotifyAlertChanged(); }
-void CClientUIInterface::LoadWallet(std::unique_ptr<interfaces::Wallet>& wallet) { return g_ui_signals.LoadWallet(wallet); }
 void CClientUIInterface::ShowProgress(const std::string& title, int nProgress, bool resume_possible) { return g_ui_signals.ShowProgress(title, nProgress, resume_possible); }
 void CClientUIInterface::NotifyBlockTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyBlockTip(b, i); }
 void CClientUIInterface::NotifyHeaderTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyHeaderTip(b, i); }

src/ui_interface.h

@@ -18,10 +18,6 @@ class connection;
 }
 } // namespace boost
 
-namespace interfaces {
-class Wallet;
-} // namespace interfaces
-
 /** General change type (added, updated, removed). */
 enum ChangeType
 {
@@ -106,9 +102,6 @@ public:
      */
     ADD_SIGNALS_DECL_WRAPPER(NotifyAlertChanged, void, );
 
-    /** A wallet has been loaded. */
-    ADD_SIGNALS_DECL_WRAPPER(LoadWallet, void, std::unique_ptr<interfaces::Wallet>& wallet);
-
     /**
      * Show progress e.g. for verifychain.
      * resume_possible indicates shutting down now will result in the current progress action resuming upon restart.

src/validation.h

@@ -22,7 +22,6 @@
 #include <txdb.h>
 #include <versionbits.h>
 
-#include <algorithm>
 #include <atomic>
 #include <exception>
 #include <map>

src/wallet/rpcwallet.cpp

@@ -1599,7 +1599,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
     for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
         CWalletTx tx = pairWtx.second;
 
-        if (depth == -1 || tx.GetDepthInMainChain(*locked_chain) < depth) {
+        if (depth == -1 || abs(tx.GetDepthInMainChain(*locked_chain)) < depth) {
             ListTransactions(*locked_chain, pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */);
         }
     }
@@ -2924,7 +2924,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
         CTxDestination address;
         const CScript& scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey;
         bool fValidAddress = ExtractDestination(scriptPubKey, address);
-        bool reused = avoid_reuse && pwallet->IsUsedDestination(address);
+        bool reused = avoid_reuse && pwallet->IsUsedDestination(out.tx->GetHash(), out.i);
 
         if (destinations.size() && (!fValidAddress || !destinations.count(address)))
             continue;

src/wallet/wallet.cpp

@@ -49,6 +49,7 @@ static const size_t OUTPUT_GROUP_MAX_ENTRIES = 10;
 
 static CCriticalSection cs_wallets;
 static std::vector<std::shared_ptr<CWallet>> vpwallets GUARDED_BY(cs_wallets);
+static std::list<LoadWalletFn> g_load_wallet_fns GUARDED_BY(cs_wallets);
 
 bool AddWallet(const std::shared_ptr<CWallet>& wallet)
 {
@@ -91,6 +92,13 @@ std::shared_ptr<CWallet> GetWallet(const std::string& name)
     return nullptr;
 }
 
+std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet)
+{
+    LOCK(cs_wallets);
+    auto it = g_load_wallet_fns.emplace(g_load_wallet_fns.end(), std::move(load_wallet));
+    return interfaces::MakeHandler([it] { LOCK(cs_wallets); g_load_wallet_fns.erase(it); });
+}
+
 static Mutex g_wallet_release_mutex;
 static std::condition_variable g_wallet_release_cv;
 static std::set<std::string> g_unloading_wallet_set;
@@ -1065,17 +1073,31 @@ void CWallet::SetUsedDestinationState(const uint256& hash, unsigned int n, bool
     }
 }
 
-bool CWallet::IsUsedDestination(const CTxDestination& dst) const
-{
-    LOCK(cs_wallet);
-    return ::IsMine(*this, dst) && GetDestData(dst, "used", nullptr);
-}
-
 bool CWallet::IsUsedDestination(const uint256& hash, unsigned int n) const
 {
+    AssertLockHeld(cs_wallet);
     CTxDestination dst;
     const CWalletTx* srctx = GetWalletTx(hash);
-    return srctx && ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst) && IsUsedDestination(dst);
+    if (srctx) {
+        assert(srctx->tx->vout.size() > n);
+        // When descriptor wallets arrive, these additional checks are
+        // likely superfluous and can be optimized out
+        for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *this)) {
+            WitnessV0KeyHash wpkh_dest(keyid);
+            if (GetDestData(wpkh_dest, "used", nullptr)) {
+                return true;
+            }
+            ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest));
+            if (GetDestData(sh_wpkh_dest, "used", nullptr)) {
+                return true;
+            }
+            PKHash pkh_dest(keyid);
+            if (GetDestData(pkh_dest, "used", nullptr)) {
+                return true;
+            }
+        }
+    }
+    return false;
 }
 
 bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
@@ -4585,7 +4607,12 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         }
     }
 
-    chain.loadWallet(interfaces::MakeWallet(walletInstance));
+    {
+        LOCK(cs_wallets);
+        for (auto& load_wallet : g_load_wallet_fns) {
+            load_wallet(interfaces::MakeWallet(walletInstance));
+        }
+    }
 
     // Register with the validation interface. It's ok to do this after rescan since we're still holding locked_chain.
     walletInstance->handleNotifications();

src/wallet/wallet.h

@@ -36,6 +36,8 @@
 
 #include <boost/signals2/signal.hpp>
 
+using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
+
 //! Explicitly unload and delete the wallet.
 //! Blocks the current thread after signaling the unload intent so that all
 //! wallet clients release the wallet.
@@ -49,6 +51,7 @@ bool HasWallets();
 std::vector<std::shared_ptr<CWallet>> GetWallets();
 std::shared_ptr<CWallet> GetWallet(const std::string& name);
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);
+std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet);
 
 enum class WalletCreationStatus {
     SUCCESS,
@@ -1001,9 +1004,8 @@ public:
 
     bool IsSpent(interfaces::Chain::Lock& locked_chain, const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    // Whether this or any UTXO with the same CTxDestination has been spent.
-    bool IsUsedDestination(const CTxDestination& dst) const;
-    bool IsUsedDestination(const uint256& hash, unsigned int n) const;
+    // Whether this or any known UTXO with the same single key has been spent.
+    bool IsUsedDestination(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void SetUsedDestinationState(const uint256& hash, unsigned int n, bool used);
 
     std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, bool single_coin) const;

src/zmq/zmqpublishnotifier.cpp

@@ -112,7 +112,8 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
 
 void CZMQAbstractPublishNotifier::Shutdown()
 {
-    assert(psocket);
+    // Early return if Initialize was not called
+    if (!psocket) return;
 
     int count = mapPublishNotifiers.count(address);
 

test/functional/interface_zmq.py

@@ -59,6 +59,10 @@ class ZMQTest (BitcoinTestFramework):
         # Note that the publishing order is not defined in the documentation and
         # is subject to change.
         import zmq
+
+        # Invalid zmq arguments don't take down the node, see #17185.
+        self.restart_node(0, ["-zmqpubrawtx=foo", "-zmqpubhashtx=bar"])
+
         address = 'tcp://127.0.0.1:28332'
         socket = self.ctx.socket(zmq.SUB)
         socket.set(zmq.RCVTIMEO, 60000)

test/functional/wallet_avoidreuse.py

@@ -83,7 +83,12 @@ class AvoidReuseTest(BitcoinTestFramework):
         reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
         self.test_fund_send_fund_senddirty()
         reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
-        self.test_fund_send_fund_send()
+        self.test_fund_send_fund_send("legacy")
+        reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
+        self.test_fund_send_fund_send("p2sh-segwit")
+        reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
+        self.test_fund_send_fund_send("bech32")
+
 
     def test_persistence(self):
         '''Test that wallet files persist the avoid_reuse flag.'''
@@ -174,7 +179,7 @@ class AvoidReuseTest(BitcoinTestFramework):
         assert_approx(self.nodes[1].getbalance(), 5, 0.001)
         assert_approx(self.nodes[1].getbalance(avoid_reuse=False), 5, 0.001)
 
-    def test_fund_send_fund_send(self):
+    def test_fund_send_fund_send(self, second_addr_type):
         '''
         Test the simple case where [1] generates a new address A, then
         [0] sends 10 BTC to A.
@@ -184,7 +189,7 @@ class AvoidReuseTest(BitcoinTestFramework):
         [1] tries to spend 4 BTC (succeeds; change address sufficient)
         '''
 
-        fundaddr = self.nodes[1].getnewaddress()
+        fundaddr = self.nodes[1].getnewaddress(label="", address_type="legacy")
         retaddr = self.nodes[0].getnewaddress()
 
         self.nodes[0].sendtoaddress(fundaddr, 10)
@@ -205,7 +210,19 @@ class AvoidReuseTest(BitcoinTestFramework):
         # getbalances should show no used, 5 btc trusted
         assert_balances(self.nodes[1], mine={"used": 0, "trusted": 5})
 
-        self.nodes[0].sendtoaddress(fundaddr, 10)
+        # For the second send, we transmute it to a related single-key address
+        # to make sure it's also detected as re-use
+        fund_spk = self.nodes[0].getaddressinfo(fundaddr)["scriptPubKey"]
+        fund_decoded = self.nodes[0].decodescript(fund_spk)
+        if second_addr_type == "p2sh-segwit":
+            new_fundaddr = fund_decoded["segwit"]["p2sh-segwit"]
+        elif second_addr_type == "bech32":
+            new_fundaddr = fund_decoded["segwit"]["addresses"][0]
+        else:
+            new_fundaddr = fundaddr
+            assert_equal(second_addr_type, "legacy")
+
+        self.nodes[0].sendtoaddress(new_fundaddr, 10)
         self.nodes[0].generate(1)
         self.sync_all()
 

test/functional/wallet_listsinceblock.py

@@ -5,6 +5,7 @@
 """Test the listsincelast RPC."""
 
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.messages import BIP125_SEQUENCE_NUMBER
 from test_framework.util import (
     assert_array_result,
     assert_equal,
@@ -12,6 +13,7 @@ from test_framework.util import (
     connect_nodes,
 )
 
+from decimal import Decimal
 
 class ListSinceBlockTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -33,6 +35,7 @@ class ListSinceBlockTest(BitcoinTestFramework):
         self.test_reorg()
         self.test_double_spend()
         self.test_double_send()
+        self.double_spends_filtered()
 
     def test_no_blockhash(self):
         txid = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
@@ -291,5 +294,51 @@ class ListSinceBlockTest(BitcoinTestFramework):
             if tx['txid'] == txid1:
                 assert_equal(tx['confirmations'], 2)
 
+    def double_spends_filtered(self):
+        '''
+        `listsinceblock` was returning conflicted transactions even if they
+        occurred before the specified cutoff blockhash
+        '''
+        spending_node = self.nodes[2]
+        dest_address = spending_node.getnewaddress()
+
+        tx_input = dict(
+            sequence=BIP125_SEQUENCE_NUMBER, **next(u for u in spending_node.listunspent()))
+        rawtx = spending_node.createrawtransaction(
+            [tx_input], {dest_address: tx_input["amount"] - Decimal("0.00051000"),
+                         spending_node.getrawchangeaddress(): Decimal("0.00050000")})
+        signedtx = spending_node.signrawtransactionwithwallet(rawtx)
+        orig_tx_id = spending_node.sendrawtransaction(signedtx["hex"])
+        original_tx = spending_node.gettransaction(orig_tx_id)
+
+        double_tx = spending_node.bumpfee(orig_tx_id)
+
+        # check that both transactions exist
+        block_hash = spending_node.listsinceblock(
+            spending_node.getblockhash(spending_node.getblockcount()))
+        original_found = False
+        double_found = False
+        for tx in block_hash['transactions']:
+            if tx['txid'] == original_tx['txid']:
+                original_found = True
+            if tx['txid'] == double_tx['txid']:
+                double_found = True
+        assert_equal(original_found, True)
+        assert_equal(double_found, True)
+
+        lastblockhash = spending_node.generate(1)[0]
+
+        # check that neither transaction exists
+        block_hash = spending_node.listsinceblock(lastblockhash)
+        original_found = False
+        double_found = False
+        for tx in block_hash['transactions']:
+            if tx['txid'] == original_tx['txid']:
+                original_found = True
+            if tx['txid'] == double_tx['txid']:
+                double_found = True
+        assert_equal(original_found, False)
+        assert_equal(double_found, False)
+
 if __name__ == '__main__':
     ListSinceBlockTest().main()