Merge pull request #5479 from knst/bc-bp-v20-missing-9

backport: bitcoin#18160, #18454, #18487, #18524, #18532, #18546, #18551, #18561, #18563, #18587

src/Makefile.test.include

@@ -164,6 +164,7 @@ BITCOIN_TESTS =\
   test/validation_chainstate_tests.cpp \
   test/validation_chainstatemanager_tests.cpp \
   test/validation_flush_tests.cpp \
+  test/validationinterface_tests.cpp \
   test/versionbits_tests.cpp
 
 if ENABLE_WALLET

src/net.h

@@ -1129,8 +1129,8 @@ public:
     std::vector<CAddress> vAddrToSend;
     const std::unique_ptr<CRollingBloomFilter> m_addr_known;
     bool fGetAddr{false};
-    int64_t nNextAddrSend GUARDED_BY(cs_sendProcessing){0};
+    std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0};
-    int64_t nNextLocalAddrSend GUARDED_BY(cs_sendProcessing){0};
+    std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0};
 
     // Don't relay addr messages to peers that we connect to as block-relay-only
     // peers (to prevent adversaries from inferring these links from addr

src/net_processing.cpp

@@ -138,9 +138,9 @@ static const int MAX_UNCONNECTING_HEADERS = 10;
 static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288;
 
 /** Average delay between local address broadcasts in seconds. */
-static constexpr unsigned int AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24 * 60 * 60;
+static constexpr std::chrono::hours AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24};
 /** Average delay between peer address broadcasts in seconds. */
-static const unsigned int AVG_ADDRESS_BROADCAST_INTERVAL = 30;
+static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30};
 /** Average delay between trickled inventory transmissions in seconds.
  *  Blocks and peers with noban permission bypass this, regular outbound peers get half this delay,
  *  Masternode outbound peers get quarter this delay. */
@@ -1889,13 +1889,13 @@ void RelayTransaction(const uint256& txid, const CConnman& connman)
     });
 }
 
-static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connman)
+static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman& connman)
 {
     // Relay to a limited number of other nodes
     // Use deterministic randomness to send to the same nodes for 24 hours
     // at a time so the m_addr_knowns of the chosen nodes prevent repeats
     uint64_t hashAddr = addr.GetHash();
-    const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24*60*60));
+    const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
     FastRandomContext insecure_rand;
 
     // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers.
@@ -4678,16 +4678,16 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
         int64_t nNow = GetTimeMicros();
         auto current_time = GetTime<std::chrono::microseconds>();
 
-        if (pto->IsAddrRelayPeer() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) {
+        if (pto->IsAddrRelayPeer() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) {
             AdvertiseLocal(pto);
-            pto->nNextLocalAddrSend = PoissonNextSend(nNow, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL);
+            pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL);
         }
 
         //
         // Message: addr
         //
-        if (pto->IsAddrRelayPeer() && pto->nNextAddrSend < nNow) {
+        if (pto->IsAddrRelayPeer() && pto->m_next_addr_send < current_time) {
-            pto->nNextAddrSend = PoissonNextSend(nNow, AVG_ADDRESS_BROADCAST_INTERVAL);
+            pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL);
             std::vector<CAddress> vAddr;
             vAddr.reserve(pto->vAddrToSend.size());
 

src/qt/walletmodel.h

@@ -151,7 +151,6 @@ public:
     interfaces::Node& node() const { return m_node; }
     interfaces::Wallet& wallet() const { return *m_wallet; }
     void setClientModel(ClientModel* client_model);
-    ClientModel& clientModel() const { return *m_client_model; }
     interfaces::CoinJoin::Client& coinJoin() const { return m_wallet->coinJoin(); }
 
     QString getWalletName() const;

src/rpc/blockchain.cpp

@@ -2946,6 +2946,8 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil
     return result;
 }
 
+void RegisterBlockchainRPCCommands(CRPCTable &t)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         argNames
@@ -2991,8 +2993,6 @@ static const CRPCCommand commands[] =
 };
 // clang-format on
 
-void RegisterBlockchainRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/rpc/coinjoin.cpp

@@ -170,6 +170,8 @@ static UniValue getcoinjoininfo(const JSONRPCRequest& request)
 
     return obj;
 }
+void RegisterCoinJoinRPCCommands(CRPCTable &t)
+{
 // clang-format off
 static const CRPCCommand commands[] =
     { //  category              name                      actor (function)         argNames
@@ -181,8 +183,6 @@ static const CRPCCommand commands[] =
 #endif // ENABLE_WALLET
 };
 // clang-format on
-void RegisterCoinJoinRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/rpc/evo.cpp

@@ -1755,6 +1755,8 @@ static UniValue _bls(const JSONRPCRequest& request)
         bls_help();
     }
 }
+void RegisterEvoRPCCommands(CRPCTable &tableRPC)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)
@@ -1763,8 +1765,6 @@ static const CRPCCommand commands[] =
     { "evo",                "protx",                  &protx,                  {}  },
 };
 // clang-format on
-void RegisterEvoRPCCommands(CRPCTable &tableRPC)
-{
     for (const auto& command : commands) {
         tableRPC.appendCommand(command.name, &command);
     }

src/rpc/governance.cpp

@@ -1191,6 +1191,8 @@ static UniValue getsuperblockbudget(const JSONRPCRequest& request)
 
     return ValueFromAmount(CSuperblock::GetPaymentsLimit(nBlockHeight));
 }
+void RegisterGovernanceRPCCommands(CRPCTable &t)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         argNames
@@ -1203,8 +1205,6 @@ static const CRPCCommand commands[] =
 
 };
 // clang-format on
-void RegisterGovernanceRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/rpc/masternode.cpp

@@ -736,6 +736,9 @@ static UniValue masternodelist(const JSONRPCRequest& request, ChainstateManager&
 
     return obj;
 }
+
+void RegisterMasternodeRPCCommands(CRPCTable &t)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         argNames
@@ -744,8 +747,6 @@ static const CRPCCommand commands[] =
     { "dash",               "masternodelist",         &masternode,             {} },
 };
 // clang-format on
-void RegisterMasternodeRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/rpc/mining.cpp

@@ -1249,6 +1249,8 @@ static UniValue estimaterawfee(const JSONRPCRequest& request)
     return result;
 }
 
+void RegisterMiningRPCCommands(CRPCTable &t)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         argNames
@@ -1276,8 +1278,6 @@ static const CRPCCommand commands[] =
 };
 // clang-format on
 
-void RegisterMiningRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/rpc/misc.cpp

@@ -1363,11 +1363,12 @@ static RPCHelpMan getindexinfo()
     });
 
     return result;
-},
+}
     };
 }
 
-// clang-format off
+void RegisterMiscRPCCommands(CRPCTable &t)
+{
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         argNames
   //  --------------------- ------------------------  -----------------------  ----------
@@ -1404,8 +1405,6 @@ static const CRPCCommand commands[] =
 };
 // clang-format on
 
-void RegisterMiscRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/rpc/net.cpp

@@ -884,6 +884,8 @@ static UniValue addpeeraddress(const JSONRPCRequest& request)
     return obj;
 }
 
+void RegisterNetRPCCommands(CRPCTable &t)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         argNames
@@ -906,8 +908,6 @@ static const CRPCCommand commands[] =
 };
 // clang-format on
 
-void RegisterNetRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/rpc/quorums.cpp

@@ -989,6 +989,8 @@ static UniValue verifyislock(const JSONRPCRequest& request)
     return llmq_ctx.sigman->VerifyRecoveredSig(llmqType, *llmq_ctx.qman, signHeight, id, txid, sig, 0) ||
            llmq_ctx.sigman->VerifyRecoveredSig(llmqType, *llmq_ctx.qman, signHeight, id, txid, sig, signOffset);
 }
+void RegisterQuorumsRPCCommands(CRPCTable &tableRPC)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)
@@ -998,8 +1000,6 @@ static const CRPCCommand commands[] =
     { "evo",                "verifyislock",           &verifyislock,           {"id", "txid", "signature", "maxHeight"}  },
 };
 // clang-format on
-void RegisterQuorumsRPCCommands(CRPCTable &tableRPC)
-{
     for (const auto& command : commands) {
         tableRPC.appendCommand(command.name, &command);
     }

src/rpc/rawtransaction.cpp

@@ -1682,6 +1682,8 @@ UniValue analyzepsbt(const JSONRPCRequest& request)
     return result;
 }
 
+void RegisterRawTransactionRPCCommands(CRPCTable &t)
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                            actor (function)            argNames
@@ -1708,8 +1710,6 @@ static const CRPCCommand commands[] =
 };
 // clang-format on
 
-void RegisterRawTransactionRPCCommands(CRPCTable &t)
-{
     for (const auto& command : commands) {
         t.appendCommand(command.name, &command);
     }

src/test/validationinterface_tests.cpp

@@ -0,0 +1,60 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <boost/test/unit_test.hpp>
+#include <consensus/validation.h>
+#include <primitives/block.h>
+#include <scheduler.h>
+#include <test/util/setup_common.h>
+#include <util/check.h>
+#include <validationinterface.h>
+
+BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup)
+
+class TestInterface : public CValidationInterface
+{
+public:
+    TestInterface(std::function<void()> on_call = nullptr, std::function<void()> on_destroy = nullptr)
+        : m_on_call(std::move(on_call)), m_on_destroy(std::move(on_destroy))
+    {
+    }
+    virtual ~TestInterface()
+    {
+        if (m_on_destroy) m_on_destroy();
+    }
+    void BlockChecked(const CBlock& block, const BlockValidationState& state) override
+    {
+        if (m_on_call) m_on_call();
+    }
+    static void Call()
+    {
+        CBlock block;
+        BlockValidationState state;
+        GetMainSignals().BlockChecked(block, state);
+    }
+    std::function<void()> m_on_call;
+    std::function<void()> m_on_destroy;
+};
+
+// Regression test to ensure UnregisterAllValidationInterfaces calls don't
+// destroy a validation interface while it is being called. Bug:
+// https://github.com/bitcoin/bitcoin/pull/18551
+BOOST_AUTO_TEST_CASE(unregister_all_during_call)
+{
+    bool destroyed = false;
+    RegisterSharedValidationInterface(std::make_shared<TestInterface>(
+        [&] {
+            // First call should decrements reference count 2 -> 1
+            UnregisterAllValidationInterfaces();
+            BOOST_CHECK(!destroyed);
+            // Second call should not decrement reference count 1 -> 0
+            UnregisterAllValidationInterfaces();
+            BOOST_CHECK(!destroyed);
+        },
+        [&] { destroyed = true; }));
+    TestInterface::Call();
+    BOOST_CHECK(destroyed);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/validationinterface.cpp

@@ -21,56 +21,75 @@
 #include <unordered_map>
 #include <utility>
 
-#include <boost/signals2/signal.hpp>
+//! The MainSignalsInstance manages a list of shared_ptr<CValidationInterface>
-
+//! callbacks.
-struct ValidationInterfaceConnections {
+//!
-    boost::signals2::scoped_connection UpdatedBlockTip;
+//! A std::unordered_map is used to track what callbacks are currently
-    boost::signals2::scoped_connection SynchronousUpdatedBlockTip;
+//! registered, and a std::list is to used to store the callbacks that are
-    boost::signals2::scoped_connection TransactionAddedToMempool;
+//! currently registered as well as any callbacks that are just unregistered
-    boost::signals2::scoped_connection BlockConnected;
+//! and about to be deleted when they are done executing.
-    boost::signals2::scoped_connection BlockDisconnected;
-    boost::signals2::scoped_connection TransactionRemovedFromMempool;
-    boost::signals2::scoped_connection ChainStateFlushed;
-    boost::signals2::scoped_connection BlockChecked;
-    boost::signals2::scoped_connection NewPoWValidBlock;
-    boost::signals2::scoped_connection AcceptedBlockHeader;
-    boost::signals2::scoped_connection NotifyHeaderTip;
-    boost::signals2::scoped_connection NotifyTransactionLock;
-    boost::signals2::scoped_connection NotifyChainLock;
-    boost::signals2::scoped_connection NotifyGovernanceVote;
-    boost::signals2::scoped_connection NotifyGovernanceObject;
-    boost::signals2::scoped_connection NotifyInstantSendDoubleSpendAttempt;
-    boost::signals2::scoped_connection NotifyMasternodeListChanged;
-    boost::signals2::scoped_connection NotifyRecoveredSig;
-
-};
-
 struct MainSignalsInstance {
-    boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> UpdatedBlockTip;
+private:
-    boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> SynchronousUpdatedBlockTip;
+    Mutex m_mutex;
-    boost::signals2::signal<void (const CTransactionRef &, int64_t)> TransactionAddedToMempool;
+    //! List entries consist of a callback pointer and reference count. The
-    boost::signals2::signal<void (const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex)> BlockConnected;
+    //! count is equal to the number of current executions of that entry, plus 1
-    boost::signals2::signal<void (const std::shared_ptr<const CBlock>&, const CBlockIndex* pindex)> BlockDisconnected;
+    //! if it's registered. It cannot be 0 because that would imply it is
-    boost::signals2::signal<void (const CTransactionRef &, MemPoolRemovalReason)> TransactionRemovedFromMempool;
+    //! unregistered and also not being executed (so shouldn't exist).
-    boost::signals2::signal<void (const CBlockLocator &)> ChainStateFlushed;
+    struct ListEntry { std::shared_ptr<CValidationInterface> callbacks; int count = 1; };
-    boost::signals2::signal<void (const CBlock&, const BlockValidationState&)> BlockChecked;
+    std::list<ListEntry> m_list GUARDED_BY(m_mutex);
-    boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
+    std::unordered_map<CValidationInterface*, std::list<ListEntry>::iterator> m_map GUARDED_BY(m_mutex);
-    boost::signals2::signal<void (const CBlockIndex *)>AcceptedBlockHeader;
+
-    boost::signals2::signal<void (const CBlockIndex *, bool)>NotifyHeaderTip;
+public:
-    boost::signals2::signal<void (const CTransactionRef& tx, const std::shared_ptr<const llmq::CInstantSendLock>& islock)>NotifyTransactionLock;
-    boost::signals2::signal<void (const CBlockIndex* pindex, const std::shared_ptr<const llmq::CChainLockSig>& clsig)>NotifyChainLock;
-    boost::signals2::signal<void (const std::shared_ptr<const CGovernanceVote>& vote)>NotifyGovernanceVote;
-    boost::signals2::signal<void (const std::shared_ptr<const CGovernanceObject>& object)>NotifyGovernanceObject;
-    boost::signals2::signal<void (const CTransactionRef& currentTx, const CTransactionRef& previousTx)>NotifyInstantSendDoubleSpendAttempt;
-    boost::signals2::signal<void (bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff, CConnman& connman)>NotifyMasternodeListChanged;
-    boost::signals2::signal<void (const std::shared_ptr<const llmq::CRecoveredSig>& sig)>NotifyRecoveredSig;
     // We are not allowed to assume the scheduler only runs in one thread,
     // but must ensure all callbacks happen in-order, so we end up creating
     // our own queue here :(
     SingleThreadedSchedulerClient m_schedulerClient;
-    std::unordered_map<CValidationInterface*, ValidationInterfaceConnections> m_connMainSignals;
 
     explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {}
+
+    void Register(std::shared_ptr<CValidationInterface> callbacks)
+    {
+        LOCK(m_mutex);
+        auto inserted = m_map.emplace(callbacks.get(), m_list.end());
+        if (inserted.second) inserted.first->second = m_list.emplace(m_list.end());
+        inserted.first->second->callbacks = std::move(callbacks);
+    }
+
+    void Unregister(CValidationInterface* callbacks)
+    {
+        LOCK(m_mutex);
+        auto it = m_map.find(callbacks);
+        if (it != m_map.end()) {
+            if (!--it->second->count) m_list.erase(it->second);
+            m_map.erase(it);
+        }
+    }
+
+    //! Clear unregisters every previously registered callback, erasing every
+    //! map entry. After this call, the list may still contain callbacks that
+    //! are currently executing, but it will be cleared when they are done
+    //! executing.
+    void Clear()
+    {
+        LOCK(m_mutex);
+        for (const auto& entry : m_map) {
+            if (!--entry.second->count) m_list.erase(entry.second);
+        }
+        m_map.clear();
+    }
+
+    template<typename F> void Iterate(F&& f)
+    {
+        WAIT_LOCK(m_mutex, lock);
+        for (auto it = m_list.begin(); it != m_list.end();) {
+            ++it->count;
+            {
+                REVERSE_LOCK(lock);
+                f(*it->callbacks);
+            }
+            it = --it->count ? std::next(it) : m_list.erase(it);
+        }
+    }
 };
 
 static CMainSignals g_signals;
@@ -103,25 +122,7 @@ CMainSignals& GetMainSignals()
 void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn) {
     // Each connection captures pwalletIn to ensure that each callback is
     // executed before pwalletIn is destroyed. For more details see #18338.
-    ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn.get()];
+    g_signals.m_internals->Register(std::move(pwalletIn));
-    conns.AcceptedBlockHeader = g_signals.m_internals->AcceptedBlockHeader.connect(std::bind(&CValidationInterface::AcceptedBlockHeader, pwalletIn, std::placeholders::_1));
-    conns.NotifyHeaderTip = g_signals.m_internals->NotifyHeaderTip.connect(std::bind(&CValidationInterface::NotifyHeaderTip, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.UpdatedBlockTip = g_signals.m_internals->UpdatedBlockTip.connect(std::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3));
-    conns.SynchronousUpdatedBlockTip = g_signals.m_internals->SynchronousUpdatedBlockTip.connect(std::bind(&CValidationInterface::SynchronousUpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3));
-    conns.TransactionAddedToMempool = g_signals.m_internals->TransactionAddedToMempool.connect(std::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.BlockConnected = g_signals.m_internals->BlockConnected.connect(std::bind(&CValidationInterface::BlockConnected, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.NotifyTransactionLock = g_signals.m_internals->NotifyTransactionLock.connect(std::bind(&CValidationInterface::NotifyTransactionLock, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.NotifyChainLock = g_signals.m_internals->NotifyChainLock.connect(std::bind(&CValidationInterface::NotifyChainLock, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, std::placeholders::_1));
-    conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.NotifyGovernanceObject = g_signals.m_internals->NotifyGovernanceObject.connect(std::bind(&CValidationInterface::NotifyGovernanceObject, pwalletIn, std::placeholders::_1));
-    conns.NotifyGovernanceVote = g_signals.m_internals->NotifyGovernanceVote.connect(std::bind(&CValidationInterface::NotifyGovernanceVote, pwalletIn, std::placeholders::_1));
-    conns.NotifyInstantSendDoubleSpendAttempt = g_signals.m_internals->NotifyInstantSendDoubleSpendAttempt.connect(std::bind(&CValidationInterface::NotifyInstantSendDoubleSpendAttempt, pwalletIn, std::placeholders::_1, std::placeholders::_2));
-    conns.NotifyRecoveredSig = g_signals.m_internals->NotifyRecoveredSig.connect(std::bind(&CValidationInterface::NotifyRecoveredSig, pwalletIn, std::placeholders::_1));
-    conns.NotifyMasternodeListChanged = g_signals.m_internals->NotifyMasternodeListChanged.connect(std::bind(&CValidationInterface::NotifyMasternodeListChanged, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4));
 }
 
 void RegisterValidationInterface(CValidationInterface* callbacks)
@@ -138,7 +139,7 @@ void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> c
 
 void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
     if (g_signals.m_internals) {
-        g_signals.m_internals->m_connMainSignals.erase(pwalletIn);
+        g_signals.m_internals->Unregister(pwalletIn);
     }
 }
 
@@ -146,7 +147,7 @@ void UnregisterAllValidationInterfaces() {
     if (!g_signals.m_internals) {
         return;
     }
-    g_signals.m_internals->m_connMainSignals.clear();
+    g_signals.m_internals->Clear();
 }
 
 void CallFunctionInValidationInterfaceQueue(std::function<void ()> func) {
@@ -186,7 +187,7 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
     // in the same critical section where the chain is updated
 
     auto event = [pindexNew, pindexFork, fInitialDownload, this] {
-        m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__,
                           pindexNew->GetBlockHash().ToString(),
@@ -195,12 +196,12 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
 }
 
 void CMainSignals::SynchronousUpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {
-    m_internals->SynchronousUpdatedBlockTip(pindexNew, pindexFork, fInitialDownload);
+    m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.SynchronousUpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); });
 }
 
 void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx, int64_t nAcceptTime) {
     auto event = [ptx, nAcceptTime, this] {
-        m_internals->TransactionAddedToMempool(ptx, nAcceptTime);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(ptx, nAcceptTime); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s", __func__,
                           ptx->GetHash().ToString());
@@ -208,7 +209,7 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx, int64_t
 
 void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
     auto event = [ptx, reason, this] {
-        m_internals->TransactionRemovedFromMempool(ptx, reason);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx, reason); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s", __func__,
                           ptx->GetHash().ToString());
@@ -216,7 +217,7 @@ void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, Mem
 
 void CMainSignals::BlockConnected(const std::shared_ptr<const CBlock> &pblock, const CBlockIndex *pindex) {
     auto event = [pblock, pindex, this] {
-        m_internals->BlockConnected(pblock, pindex);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(pblock, pindex); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__,
                           pblock->GetHash().ToString(),
@@ -225,7 +226,7 @@ void CMainSignals::BlockConnected(const std::shared_ptr<const CBlock> &pblock, c
 
 void CMainSignals::BlockDisconnected(const std::shared_ptr<const CBlock> &pblock, const CBlockIndex* pindex) {
     auto event = [pblock, pindex, this] {
-        m_internals->BlockDisconnected(pblock, pindex);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__,
                           pblock->GetHash().ToString(),
@@ -234,7 +235,7 @@ void CMainSignals::BlockDisconnected(const std::shared_ptr<const CBlock> &pblock
 
 void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) {
     auto event = [locator, this] {
-        m_internals->ChainStateFlushed(locator);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(locator); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s", __func__,
                           locator.IsNull() ? "null" : locator.vHave.front().ToString());
@@ -243,27 +244,27 @@ void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) {
 void CMainSignals::BlockChecked(const CBlock& block, const BlockValidationState& state) {
     LOG_EVENT("%s: block hash=%s state=%s", __func__,
               block.GetHash().ToString(), state.ToString());
-    m_internals->BlockChecked(block, state);
+    m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockChecked(block, state); });
 }
 
 void CMainSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock> &block) {
     LOG_EVENT("%s: block hash=%s", __func__, block->GetHash().ToString());
-    m_internals->NewPoWValidBlock(pindex, block);
+    m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NewPoWValidBlock(pindex, block); });
 }
 
 void CMainSignals::AcceptedBlockHeader(const CBlockIndex *pindexNew) {
     LOG_EVENT("%s: accepted block header hash=%s", __func__, pindexNew->GetBlockHash().ToString());
-    m_internals->AcceptedBlockHeader(pindexNew);
+    m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.AcceptedBlockHeader(pindexNew); });
 }
 
 void CMainSignals::NotifyHeaderTip(const CBlockIndex *pindexNew, bool fInitialDownload) {
     LOG_EVENT("%s: accepted block header hash=%s initial=%d", __func__, pindexNew->GetBlockHash().ToString(), fInitialDownload);
-    m_internals->NotifyHeaderTip(pindexNew, fInitialDownload);
+    m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyHeaderTip(pindexNew, fInitialDownload); });
 }
 
 void CMainSignals::NotifyTransactionLock(const CTransactionRef &tx, const std::shared_ptr<const llmq::CInstantSendLock>& islock) {
     auto event = [tx, islock, this] {
-        m_internals->NotifyTransactionLock(tx, islock);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyTransactionLock(tx, islock); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: transaction lock txid=%s", __func__,
                           tx->GetHash().ToString());
@@ -271,7 +272,7 @@ void CMainSignals::NotifyTransactionLock(const CTransactionRef &tx, const std::s
 
 void CMainSignals::NotifyChainLock(const CBlockIndex* pindex, const std::shared_ptr<const llmq::CChainLockSig>& clsig) {
     auto event = [pindex, clsig, this] {
-        m_internals->NotifyChainLock(pindex, clsig);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyChainLock(pindex, clsig); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: notify chainlock at block=%s cl=%s", __func__,
             pindex->GetBlockHash().ToString(),
@@ -280,21 +281,21 @@ void CMainSignals::NotifyChainLock(const CBlockIndex* pindex, const std::shared_
 
 void CMainSignals::NotifyGovernanceVote(const std::shared_ptr<const CGovernanceVote>& vote) {
     auto event = [vote, this] {
-        m_internals->NotifyGovernanceVote(vote);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceVote(vote); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: notify governance vote: %s", __func__, vote->GetHash().ToString());
 }
 
 void CMainSignals::NotifyGovernanceObject(const std::shared_ptr<const CGovernanceObject>& object) {
     auto event = [object, this] {
-        m_internals->NotifyGovernanceObject(object);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceObject(object); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: notify governance object: %s", __func__, object->GetHash().ToString());
 }
 
 void CMainSignals::NotifyInstantSendDoubleSpendAttempt(const CTransactionRef& currentTx, const CTransactionRef& previousTx) {
     auto event = [currentTx, previousTx, this] {
-        m_internals->NotifyInstantSendDoubleSpendAttempt(currentTx, previousTx);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyInstantSendDoubleSpendAttempt(currentTx, previousTx); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: notify instant doublespendattempt currenttxid=%s previoustxid=%s", __func__,
             currentTx->GetHash().ToString(),
@@ -303,7 +304,7 @@ void CMainSignals::NotifyInstantSendDoubleSpendAttempt(const CTransactionRef& cu
 
 void CMainSignals::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig) {
     auto event = [sig, this] {
-        m_internals->NotifyRecoveredSig(sig);
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyRecoveredSig(sig); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: notify recoveredsig=%s", __func__,
             sig->GetHash().ToString());
@@ -311,5 +312,5 @@ void CMainSignals::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecover
 
 void CMainSignals::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff, CConnman& connman) {
     LOG_EVENT("%s: notify mn list changed undo=%d", __func__, undo);
-    m_internals->NotifyMasternodeListChanged(undo, oldMNList, diff, connman);
+    m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyMasternodeListChanged(undo, oldMNList, diff, connman); });
 }

src/validationinterface.h

@@ -194,9 +194,7 @@ protected:
      * Notifies listeners that a block which builds directly on our current tip
      * has been received and connected to the headers tree, though not validated yet */
     virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) {};
-    friend void ::RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>);
+    friend class CMainSignals;
-    friend void ::UnregisterValidationInterface(CValidationInterface*);
-    friend void ::UnregisterAllValidationInterfaces();
 };
 
 struct MainSignalsInstance;

src/wallet/interfaces.cpp

@@ -245,7 +245,7 @@ public:
             return false;
         }
         if (name) {
-            *name = it->second.name;
+            *name = it->second.GetLabel();
         }
         if (is_mine) {
             *is_mine = m_wallet->IsMine(dest);
@@ -261,7 +261,7 @@ public:
         std::vector<WalletAddress> result;
         for (const auto& item : m_wallet->m_address_book) {
             if (item.second.IsChange()) continue;
-            result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.name, item.second.purpose);
+            result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.GetLabel(), item.second.purpose);
         }
         return result;
     }

src/wallet/rpcdump.cpp

@@ -1014,7 +1014,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
             file << strprintf("%s %s ", EncodeSecret(key), strTime);
             const auto* address_book_entry = pwallet->FindAddressBookEntry(pkhash);
             if (address_book_entry) {
-                file << strprintf("label=%s", EncodeDumpString(address_book_entry->name));
+                file << strprintf("label=%s", EncodeDumpString(address_book_entry->GetLabel()));
             } else if (mapKeyPool.count(keyid)) {
                 file << "reserve=1";
             } else {

src/wallet/rpcwallet.cpp

@@ -532,7 +532,7 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request)
             {
                 const auto* address_book_entry = pwallet->FindAddressBookEntry(address);
                 if (address_book_entry) {
-                    addressInfo.push_back(address_book_entry->name);
+                    addressInfo.push_back(address_book_entry->GetLabel());
                 }
             }
             jsonGrouping.push_back(addressInfo);
@@ -1151,7 +1151,7 @@ static UniValue ListReceived(const CWallet * const pwallet, const UniValue& para
     {
         if (item_it->second.IsChange()) continue;
         const CTxDestination& address = item_it->first;
-        const std::string& label = item_it->second.name;
+        const std::string& label = item_it->second.GetLabel();
         auto it = mapTally.find(address);
         if (it == mapTally.end() && !fIncludeEmpty)
             continue;
@@ -1354,7 +1354,7 @@ static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx,
             entry.pushKV("amount", ValueFromAmount(-s.amount));
             const auto* address_book_entry = pwallet->FindAddressBookEntry(s.destination);
             if (address_book_entry) {
-                entry.pushKV("label", address_book_entry->name);
+                entry.pushKV("label", address_book_entry->GetLabel());
             }
             entry.pushKV("vout", s.vout);
             entry.pushKV("fee", ValueFromAmount(-nFee));
@@ -1373,7 +1373,7 @@ static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx,
             std::string label;
             const auto* address_book_entry = pwallet->FindAddressBookEntry(r.destination);
             if (address_book_entry) {
-                label = address_book_entry->name;
+                label = address_book_entry->GetLabel();
             }
             if (filter_label && label != *filter_label) {
                 continue;
@@ -1934,54 +1934,62 @@ static UniValue walletpassphrase(const JSONRPCRequest& request)
     if (!wallet) return NullUniValue;
     CWallet* const pwallet = wallet.get();
 
-    LOCK(pwallet->cs_wallet);
+    int64_t nSleepTime;
+    {
+        LOCK(pwallet->cs_wallet);
+
+        if (!pwallet->IsCrypted()) {
+            throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrase was called.");
+        }
+
+        // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed
+        SecureString strWalletPass;
+        strWalletPass.reserve(100);
+        // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
+        // Alternately, find a way to make request.params[0] mlock()'d to begin with.
+        strWalletPass = request.params[0].get_str().c_str();
+
+        // Get the timeout
+        nSleepTime = request.params[1].get_int64();
+        // Timeout cannot be negative, otherwise it will relock immediately
+        if (nSleepTime < 0) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
+        }
+        // Clamp timeout
+        constexpr int64_t MAX_SLEEP_TIME = 100000000; // larger values trigger a macos/libevent bug?
+        if (nSleepTime > MAX_SLEEP_TIME) {
+            nSleepTime = MAX_SLEEP_TIME;
+        }
+
+        bool fForMixingOnly = false;
+        if (!request.params[2].isNull())
+            fForMixingOnly = request.params[2].get_bool();
+
+        if (fForMixingOnly && !pwallet->IsLocked()) {
+            // Downgrading from "fuly unlocked" mode to "mixing only" one is not supported.
+            // Updating unlock time when current unlock mode is not changed or when it is upgraded
+            // from "mixing only" to "fuly unlocked" is ok.
+            throw JSONRPCError(RPC_WALLET_ALREADY_UNLOCKED, "Error: Wallet is already fully unlocked.");
+        }
+
+        if (strWalletPass.empty()) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase can not be empty");
+        }
+
+        if (!pwallet->Unlock(strWalletPass, fForMixingOnly)) {
+            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect.");
+        }
+
+        pwallet->TopUpKeyPool();
+
+        pwallet->nRelockTime = GetTime() + nSleepTime;
 
-    if (!pwallet->IsCrypted()) {
-        throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrase was called.");
     }
-
+    // rpcRunLater must be called without cs_wallet held otherwise a deadlock
-    // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed
+    // can occur. The deadlock would happen when RPCRunLater removes the
-    SecureString strWalletPass;
+    // previous timer (and waits for the callback to finish if already running)
-    strWalletPass.reserve(100);
+    // and the callback locks cs_wallet.
-    // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
+    AssertLockNotHeld(wallet->cs_wallet);
-    // Alternately, find a way to make request.params[0] mlock()'d to begin with.
-    strWalletPass = request.params[0].get_str().c_str();
-
-    // Get the timeout
-    int64_t nSleepTime = request.params[1].get_int64();
-    // Timeout cannot be negative, otherwise it will relock immediately
-    if (nSleepTime < 0) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
-    }
-    // Clamp timeout
-    constexpr int64_t MAX_SLEEP_TIME = 100000000; // larger values trigger a macos/libevent bug?
-    if (nSleepTime > MAX_SLEEP_TIME) {
-        nSleepTime = MAX_SLEEP_TIME;
-    }
-
-    bool fForMixingOnly = false;
-    if (!request.params[2].isNull())
-        fForMixingOnly = request.params[2].get_bool();
-
-    if (fForMixingOnly && !pwallet->IsLocked()) {
-        // Downgrading from "fuly unlocked" mode to "mixing only" one is not supported.
-        // Updating unlock time when current unlock mode is not changed or when it is upgraded
-        // from "mixing only" to "fuly unlocked" is ok.
-        throw JSONRPCError(RPC_WALLET_ALREADY_UNLOCKED, "Error: Wallet is already fully unlocked.");
-    }
-
-    if (strWalletPass.empty()) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase can not be empty");
-    }
-
-    if (!pwallet->Unlock(strWalletPass, fForMixingOnly)) {
-        throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect.");
-    }
-
-    pwallet->TopUpKeyPool();
-
-    pwallet->nRelockTime = GetTime() + nSleepTime;
-
     // Keep a weak pointer to the wallet so that it is possible to unload the
     // wallet before the following callback is called. If a valid shared pointer
     // is acquired in the callback then the wallet is still loaded.
@@ -3186,7 +3194,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
 
             const auto* address_book_entry = pwallet->FindAddressBookEntry(address);
             if (address_book_entry) {
-                entry.pushKV("label", address_book_entry->name);
+                entry.pushKV("label", address_book_entry->GetLabel());
             }
 
             std::unique_ptr<SigningProvider> provider = pwallet->GetSolvingProvider(scriptPubKey);
@@ -3706,7 +3714,7 @@ static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool v
 {
     UniValue ret(UniValue::VOBJ);
     if (verbose) {
-        ret.pushKV("name", data.name);
+        ret.pushKV("name", data.GetLabel());
     }
     ret.pushKV("purpose", data.purpose);
     return ret;
@@ -3808,7 +3816,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
     // value of the name key/value pair in the labels array below.
     const auto* address_book_entry = pwallet->FindAddressBookEntry(dest);
     if (pwallet->chain().rpcEnableDeprecated("label") && address_book_entry) {
-        ret.pushKV("label", address_book_entry->name);
+        ret.pushKV("label", address_book_entry->GetLabel());
     }
 
     ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
@@ -3844,7 +3852,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
         if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) {
             labels.push_back(AddressBookDataToJSON(*address_book_entry, true));
         } else {
-            labels.push_back(address_book_entry->name);
+            labels.push_back(address_book_entry->GetLabel());
         }
     }
     ret.pushKV("labels", std::move(labels));
@@ -3887,7 +3895,7 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request)
     std::set<std::string> addresses;
     for (const std::pair<const CTxDestination, CAddressBookData>& item : pwallet->m_address_book) {
         if (item.second.IsChange()) continue;
-        if (item.second.name == label) {
+        if (item.second.GetLabel() == label) {
             std::string address = EncodeDestination(item.first);
             // CWallet::m_address_book is not expected to contain duplicate
             // address strings, but build a separate set as a precaution just in
@@ -3950,7 +3958,7 @@ static UniValue listlabels(const JSONRPCRequest& request)
     for (const std::pair<const CTxDestination, CAddressBookData>& entry : pwallet->m_address_book) {
         if (entry.second.IsChange()) continue;
         if (purpose.empty() || entry.second.purpose == purpose) {
-            label_set.insert(entry.second.name);
+            label_set.insert(entry.second.GetLabel());
         }
     }
 
@@ -4188,6 +4196,8 @@ static UniValue upgradewallet(const JSONRPCRequest& request)
     return error.original;
 }
 
+Span<const CRPCCommand> GetWalletRPCCommands()
+{
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                        actor (function)           argNames
@@ -4258,7 +4268,5 @@ static const CRPCCommand commands[] =
 };
 // clang-format on
 
-Span<const CRPCCommand> GetWalletRPCCommands()
-{
     return MakeSpan(commands);
 }

src/wallet/wallet.cpp

@@ -3938,7 +3938,10 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
     // If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted)
     // NOTE: This isn't a problem for sending addresses because they never have any DestData yet!
     // When adding new DestData, it should be considered here whether to retain or delete it (or move it?).
-    assert(!IsMine(address));
+    if (IsMine(address)) {
+        WalletLogPrintf("%s called with IsMine address, NOT SUPPORTED. Please report this bug! %s\n", __func__, PACKAGE_BUGREPORT);
+        return false;
+    }
 
     {
         LOCK(cs_wallet);
@@ -4190,7 +4193,7 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
     {
         if (item.second.IsChange()) continue;
         const CTxDestination& address = item.first;
-        const std::string& strName = item.second.name;
+        const std::string& strName = item.second.GetLabel();
         if (strName == label)
             result.insert(address);
     }

src/wallet/wallet.h

@@ -191,15 +191,15 @@ private:
     bool m_change{true};
     std::string m_label;
 public:
-    const std::string& name;
     std::string purpose;
 
-    CAddressBookData() : name(m_label), purpose("unknown") {}
+    CAddressBookData() : purpose("unknown") {}
 
     typedef std::map<std::string, std::string> StringMap;
     StringMap destdata;
 
     bool IsChange() const { return m_change; }
+    const std::string& GetLabel() const { return m_label; }
     void SetLabel(const std::string& label) {
         m_change = false;
         m_label = label;

test/functional/p2p_addr_relay.py

@@ -0,0 +1,69 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test addr relay
+"""
+
+from test_framework.messages import (
+    CAddress,
+    NODE_NETWORK,
+    msg_addr,
+)
+from test_framework.mininode import (
+    P2PInterface,
+)
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+)
+import time
+
+ADDRS = []
+for i in range(10):
+    addr = CAddress()
+    addr.time = int(time.time()) + i
+    addr.nServices = NODE_NETWORK
+    addr.ip = "123.123.123.{}".format(i % 256)
+    addr.port = 8333 + i
+    ADDRS.append(addr)
+
+
+class AddrReceiver(P2PInterface):
+    def on_addr(self, message):
+        for addr in message.addrs:
+            assert_equal(addr.nServices, 9)
+            assert addr.ip.startswith('123.123.123.')
+            assert (8333 <= addr.port < 8343)
+
+
+class AddrTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = False
+        self.num_nodes = 1
+
+    def run_test(self):
+        self.log.info('Create connection that sends addr messages')
+        addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
+        msg = msg_addr()
+
+        self.log.info('Send too large addr message')
+        msg.addrs = ADDRS * 101
+        with self.nodes[0].assert_debug_log(['addr message size = 1010']):
+            addr_source.send_and_ping(msg)
+
+        self.log.info('Check that addr message content is relayed and added to addrman')
+        addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
+        msg.addrs = ADDRS
+        with self.nodes[0].assert_debug_log([
+                'Added 10 addresses from 127.0.0.1: 0 tried',
+                'received: addr (301 bytes) peer=0',
+        ]):
+            addr_source.send_and_ping(msg)
+            self.nodes[0].setmocktime(int(time.time()) + 30 * 60)
+            addr_receiver.sync_with_ping()
+
+
+if __name__ == '__main__':
+    AddrTest().main()

test/functional/test_framework/test_node.py

@@ -272,6 +272,10 @@ class TestNode():
                 # -342 Service unavailable, RPC server started but is shutting down due to error
                 if e.error['code'] != -28 and e.error['code'] != -342:
                     raise  # unknown JSON RPC exception
+            except ConnectionResetError:
+                # This might happen when the RPC server is in warmup, but shut down before the call to getblockcount
+                # succeeds. Try again to properly raise the FailedToStartError
+                pass
             except OSError as e:
                 if e.errno == errno.ETIMEDOUT:
                     pass  # Treat identical to ConnectionResetError

test/functional/test_runner.py

@@ -174,6 +174,7 @@ BASE_SCRIPTS = [
     'rpc_blockchain.py',
     'rpc_deprecated.py',
     'wallet_disable.py',
+    'p2p_addr_relay.py',
     'p2p_getaddr_caching.py',
     'p2p_getdata.py',
     'rpc_net.py',

test/functional/wallet_avoidreuse.py

@@ -88,6 +88,7 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         self.nodes[0].generate(110)
         self.sync_all()
+        self.test_change_remains_change(self.nodes[1])
         reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
         self.test_sending_from_reused_address_without_avoid_reuse()
         reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
@@ -141,6 +142,30 @@ class AvoidReuseTest(BitcoinTestFramework):
         # Unload temp wallet
         self.nodes[1].unloadwallet(tempwallet)
 
+    def test_change_remains_change(self, node):
+        self.log.info("Test that change doesn't turn into non-change when spent")
+
+        reset_balance(node, node.getnewaddress())
+        addr = node.getnewaddress()
+        txid = node.sendtoaddress(addr, 1)
+        out = node.listunspent(minconf=0, query_options={'minimumAmount': 2})
+        assert_equal(len(out), 1)
+        assert_equal(out[0]['txid'], txid)
+        changeaddr = out[0]['address']
+
+        # Make sure it's starting out as change as expected
+        assert node.getaddressinfo(changeaddr)['ischange']
+        for logical_tx in node.listtransactions():
+            assert logical_tx.get('address') != changeaddr
+
+        # Spend it
+        reset_balance(node, node.getnewaddress())
+
+        # It should still be change
+        assert node.getaddressinfo(changeaddr)['ischange']
+        for logical_tx in node.listtransactions():
+            assert logical_tx.get('address') != changeaddr
+
     def test_sending_from_reused_address_without_avoid_reuse(self):
         '''
         Test the same as test_sending_from_reused_address_fails, except send the 10 BTC with