backport: bitcoin#28858, bitcoin#28737, bitcoin#28530, bitcoin#28224

src/.clang-tidy

@@ -2,6 +2,7 @@ Checks: '
 -*,
 bugprone-argument-comment,
 bugprone-use-after-move,
+bugprone-lambda-function-name,
 misc-unused-using-decls,
 modernize-use-default-member-init,
 modernize-use-emplace,

src/Makefile.am

@@ -604,6 +604,7 @@ libbitcoin_node_a_SOURCES = \
   kernel/coinstats.cpp \
   kernel/context.cpp \
   kernel/cs_main.cpp \
+  kernel/disconnected_transactions.cpp \
   kernel/mempool_persist.cpp \
   llmq/blockprocessor.cpp \
   llmq/commitment.cpp \
@@ -1317,6 +1318,7 @@ libdashkernel_la_SOURCES = \
   kernel/coinstats.cpp \
   kernel/context.cpp \
   kernel/cs_main.cpp \
+  kernel/disconnected_transactions.cpp \
   kernel/mempool_persist.cpp \
   key.cpp \
   key_io.cpp \

src/Makefile.test.include

@@ -109,6 +109,7 @@ BITCOIN_TESTS =\
   test/dip0020opcodes_tests.cpp \
   test/dip14_tests.cpp \
   test/descriptor_tests.cpp \
+  test/disconnected_transactions.cpp \
   test/dynamic_activation_thresholds_tests.cpp \
   test/evo_assetlocks_tests.cpp \
   test/evo_cbtx_tests.cpp \

src/bench/disconnected_transactions.cpp

@@ -73,7 +73,7 @@ static ReorgTxns CreateBlocks(size_t num_not_shared)
 
 static void Reorg(const ReorgTxns& reorg)
 {
-    DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
+    DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES};
     // Disconnect block
     const auto evicted = disconnectpool.AddTransactionsFromBlock(reorg.disconnected_txns);
     assert(evicted.empty());

src/evo/specialtxman.cpp

@@ -811,7 +811,7 @@ bool CSpecialTxProcessor::FinishListForBlock(const CBlock& block, gsl::not_null<
         if (payee != nullptr && dmn.proTxHash == payee->proTxHash && !isMNRewardReallocation) return;
         if (dmn.pdmnState->nConsecutivePayments == 0) return;
         if (debugLogs) {
-            LogPrint(BCLog::MNPAYMENTS, "%s -- MN %s, reset nConsecutivePayments %d->0\n", __func__,
+            LogPrint(BCLog::MNPAYMENTS, "FinishListForBlock -- MN %s, reset nConsecutivePayments %d->0\n",
                      dmn.proTxHash.ToString(), dmn.pdmnState->nConsecutivePayments);
         }
         auto newState = std::make_shared<CDeterministicMNState>(*dmn.pdmnState);

src/init.cpp

@@ -511,11 +511,11 @@ void Shutdown(NodeContext& node)
         PrepareShutdown(node);
     }
     // Shutdown part 2: delete wallet instance
-    node.kernel.reset();
     node.mempool.reset();
     node.fee_estimator.reset();
     node.chainman.reset();
     node.scheduler.reset();
+    node.kernel.reset();
 
     RemovePidFile(*node.args);
 
@@ -623,9 +623,9 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-maxrecsigsage=<n>", strprintf("Number of seconds to keep LLMQ recovery sigs (default: %u)", llmq::DEFAULT_MAX_RECOVERED_SIGS_AGE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-mempoolexpiry=<n>", strprintf("Do not keep transactions in the mempool longer than <n> hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY_HOURS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-minimumchainwork=<hex>", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet: %s, devnet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex(), devnetChainParams->GetConsensus().nMinimumChainWork.GetHex()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-par=<n>", strprintf("Set the number of script verification threads (0 = auto, <0 = leave that many cores free, max: %d, default: %d)",
+    argsman.AddArg("-par=<n>", strprintf("Set the number of script verification threads (0 = auto, up to %d, <0 = leave that many cores free, default: %d)",
         MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-parbls=<n>", strprintf("Set the number of BLS verification threads (0 = auto, <0 = leave that many cores free, max: %d, default: %d)",
+    argsman.AddArg("-parbls=<n>", strprintf("Set the number of BLS verification threads (0 = auto, up to %d, <0 = leave that many cores free, default: %d)",
         llmq::MAX_BLSCHECK_THREADS, llmq::DEFAULT_BLSCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-persistmempool", strprintf("Whether to save the mempool on shutdown and load on restart (default: %u)", DEFAULT_PERSIST_MEMPOOL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

src/instantsend/signing.cpp

@@ -212,7 +212,7 @@ bool InstantSendSigner::CheckCanLockAssetUnlock(const CTransaction& tx, bool pri
 {
     const auto log_refusal = [&](std::string_view reason) {
         if (printDebug) {
-            LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: asset unlock not lockable: %s\n", __func__,
+            LogPrint(BCLog::INSTANTSEND, "CheckCanLockAssetUnlock -- txid=%s: asset unlock not lockable: %s\n",
                      tx.GetHash().ToString(), reason);
         }
         return false;

src/kernel/disconnected_transactions.cpp

@@ -0,0 +1,90 @@
+// Copyright (c) 2023 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 <kernel/disconnected_transactions.h>
+
+#include <assert.h>
+#include <core_memusage.h>
+#include <memusage.h>
+#include <primitives/transaction.h>
+#include <util/hasher.h>
+
+#include <memory>
+#include <utility>
+
+// It's almost certainly a logic bug if we don't clear out queuedTx before
+// destruction, as we add to it while disconnecting blocks, and then we
+// need to re-process remaining transactions to ensure mempool consistency.
+// For now, assert() that we've emptied out this object on destruction.
+// This assert() can always be removed if the reorg-processing code were
+// to be refactored such that this assumption is no longer true (for
+// instance if there was some other way we cleaned up the mempool after a
+// reorg, besides draining this object).
+DisconnectedBlockTransactions::~DisconnectedBlockTransactions()
+{
+    assert(queuedTx.empty());
+    assert(iters_by_txid.empty());
+    assert(cachedInnerUsage == 0);
+}
+
+std::vector<CTransactionRef> DisconnectedBlockTransactions::LimitMemoryUsage()
+{
+    std::vector<CTransactionRef> evicted;
+
+    while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) {
+        evicted.emplace_back(queuedTx.front());
+        cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());
+        iters_by_txid.erase(queuedTx.front()->GetHash());
+        queuedTx.pop_front();
+    }
+    return evicted;
+}
+
+size_t DisconnectedBlockTransactions::DynamicMemoryUsage() const
+{
+    return cachedInnerUsage + memusage::DynamicUsage(iters_by_txid) + memusage::DynamicUsage(queuedTx);
+}
+
+[[nodiscard]] std::vector<CTransactionRef> DisconnectedBlockTransactions::AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx)
+{
+    iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
+    for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) {
+        auto it = queuedTx.insert(queuedTx.end(), *block_it);
+        auto [_, inserted] = iters_by_txid.emplace((*block_it)->GetHash(), it);
+        assert(inserted); // callers may never pass multiple transactions with the same txid
+        cachedInnerUsage += RecursiveDynamicUsage(*block_it);
+    }
+    return LimitMemoryUsage();
+}
+
+void DisconnectedBlockTransactions::removeForBlock(const std::vector<CTransactionRef>& vtx)
+{
+    // Short-circuit in the common case of a block being added to the tip
+    if (queuedTx.empty()) {
+        return;
+    }
+    for (const auto& tx : vtx) {
+        auto iter = iters_by_txid.find(tx->GetHash());
+        if (iter != iters_by_txid.end()) {
+            auto list_iter = iter->second;
+            iters_by_txid.erase(iter);
+            cachedInnerUsage -= RecursiveDynamicUsage(*list_iter);
+            queuedTx.erase(list_iter);
+        }
+    }
+}
+
+void DisconnectedBlockTransactions::clear()
+{
+    cachedInnerUsage = 0;
+    iters_by_txid.clear();
+    queuedTx.clear();
+}
+
+std::list<CTransactionRef> DisconnectedBlockTransactions::take()
+{
+    std::list<CTransactionRef> ret = std::move(queuedTx);
+    clear();
+    return ret;
+}

src/kernel/disconnected_transactions.h

@@ -5,17 +5,15 @@
 #ifndef BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H
 #define BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H
 
-#include <core_memusage.h>
-#include <memusage.h>
 #include <primitives/transaction.h>
 #include <util/hasher.h>
 
 #include <list>
 #include <unordered_map>
 #include <vector>
 
-/** Maximum kilobytes for transactions to store for processing during reorg */
-static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000;
+/** Maximum bytes for transactions to store for processing during reorg */
+static const unsigned int MAX_DISCONNECTED_TX_POOL_BYTES{20'000'000};
 /**
  * DisconnectedBlockTransactions
 
@@ -38,48 +36,23 @@ static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000;
  */
 class DisconnectedBlockTransactions {
 private:
-    /** Cached dynamic memory usage for the CTransactions (memory for the shared pointers is
-     * included in the container calculations). */
+    /** Cached dynamic memory usage for the `CTransactionRef`s */
     uint64_t cachedInnerUsage = 0;
     const size_t m_max_mem_usage;
     std::list<CTransactionRef> queuedTx;
     using TxList = decltype(queuedTx);
     std::unordered_map<uint256, TxList::iterator, SaltedTxidHasher> iters_by_txid;
 
     /** Trim the earliest-added entries until we are within memory bounds. */
-    std::vector<CTransactionRef> LimitMemoryUsage()
-    {
-        std::vector<CTransactionRef> evicted;
-
-        while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) {
-            evicted.emplace_back(queuedTx.front());
-            cachedInnerUsage -= RecursiveDynamicUsage(*queuedTx.front());
-            iters_by_txid.erase(queuedTx.front()->GetHash());
-            queuedTx.pop_front();
-        }
-        return evicted;
-    }
+    std::vector<CTransactionRef> LimitMemoryUsage();
 
 public:
-    DisconnectedBlockTransactions(size_t max_mem_usage) : m_max_mem_usage{max_mem_usage} {}
+    DisconnectedBlockTransactions(size_t max_mem_usage)
+        : m_max_mem_usage{max_mem_usage} {}
 
-    // It's almost certainly a logic bug if we don't clear out queuedTx before
-    // destruction, as we add to it while disconnecting blocks, and then we
-    // need to re-process remaining transactions to ensure mempool consistency.
-    // For now, assert() that we've emptied out this object on destruction.
-    // This assert() can always be removed if the reorg-processing code were
-    // to be refactored such that this assumption is no longer true (for
-    // instance if there was some other way we cleaned up the mempool after a
-    // reorg, besides draining this object).
-    ~DisconnectedBlockTransactions() {
-        assert(queuedTx.empty());
-        assert(iters_by_txid.empty());
-        assert(cachedInnerUsage == 0);
-    }
+    ~DisconnectedBlockTransactions();
 
-    size_t DynamicMemoryUsage() const {
-        return cachedInnerUsage + memusage::DynamicUsage(iters_by_txid) + memusage::DynamicUsage(queuedTx);
-    }
+    size_t DynamicMemoryUsage() const;
 
     /** Add transactions from the block, iterating through vtx in reverse order. Callers should call
      * this function for blocks in descending order by block height.
@@ -88,50 +61,16 @@ class DisconnectedBlockTransactions {
      * corresponding entry in iters_by_txid.
      * @returns vector of transactions that were evicted for size-limiting.
      */
-    [[nodiscard]] std::vector<CTransactionRef> AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx)
-    {
-        iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
-        for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) {
-            auto it = queuedTx.insert(queuedTx.end(), *block_it);
-            iters_by_txid.emplace((*block_it)->GetHash(), it);
-            cachedInnerUsage += RecursiveDynamicUsage(**block_it);
-        }
-        return LimitMemoryUsage();
-    }
+    [[nodiscard]] std::vector<CTransactionRef> AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx);
 
     /** Remove any entries that are in this block. */
-    void removeForBlock(const std::vector<CTransactionRef>& vtx)
-    {
-        // Short-circuit in the common case of a block being added to the tip
-        if (queuedTx.empty()) {
-            return;
-        }
-        for (const auto& tx : vtx) {
-            auto iter = iters_by_txid.find(tx->GetHash());
-            if (iter != iters_by_txid.end()) {
-                auto list_iter = iter->second;
-                iters_by_txid.erase(iter);
-                cachedInnerUsage -= RecursiveDynamicUsage(**list_iter);
-                queuedTx.erase(list_iter);
-            }
-        }
-    }
+    void removeForBlock(const std::vector<CTransactionRef>& vtx);
 
     size_t size() const { return queuedTx.size(); }
 
-    void clear()
-    {
-        cachedInnerUsage = 0;
-        iters_by_txid.clear();
-        queuedTx.clear();
-    }
+    void clear();
 
     /** Clear all data structures and return the list of transactions. */
-    std::list<CTransactionRef> take()
-    {
-        std::list<CTransactionRef> ret = std::move(queuedTx);
-        clear();
-        return ret;
-    }
+    std::list<CTransactionRef> take();
 };
 #endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H

src/net_processing.cpp

@@ -2624,8 +2624,8 @@ void PeerManagerImpl::RelayInvFiltered(const CInv& inv, const CTransaction& rela
             }
         } // LOCK(tx_relay->m_bloom_filter_mutex)
         if (inv.type == MSG_ISDLOCK && PeerReconstructsISLockFromRecsig(*pnode, *peer)) {
-            LogPrint(BCLog::NET, "%s -- skipping ISDLOCK inv (peer wants recsigs): %s peer=%d\n",
-                     __func__, inv.ToString(), peer->m_id);
+            LogPrint(BCLog::NET, "RelayInvFiltered -- skipping ISDLOCK inv (peer wants recsigs): %s peer=%d\n",
+                     inv.ToString(), peer->m_id);
             return;
         }
         PushInv(*peer, inv);
@@ -2654,8 +2654,8 @@ void PeerManagerImpl::RelayInvFiltered(const CInv& inv, const uint256& relatedTx
             }
         } // LOCK(tx_relay->m_bloom_filter_mutex)
         if (inv.type == MSG_ISDLOCK && PeerReconstructsISLockFromRecsig(*pnode, *peer)) {
-            LogPrint(BCLog::NET, "%s -- skipping ISDLOCK inv (peer wants recsigs): %s peer=%d\n",
-                     __func__, inv.ToString(), peer->m_id);
+            LogPrint(BCLog::NET, "RelayInvFiltered -- skipping ISDLOCK inv (peer wants recsigs): %s peer=%d\n",
+                     inv.ToString(), peer->m_id);
             return;
         }
         PushInv(*peer, inv);

src/qt/guiutil.cpp

@@ -898,7 +898,7 @@ void loadStyleSheet(bool fForceUpdate)
             for (const auto& file : vecFiles) {
                 QFile qFile(file);
                 if (!qFile.open(QFile::ReadOnly)) {
-                    throw std::runtime_error(strprintf("%s: Failed to open file: %s", __func__, file.toStdString()));
+                    throw std::runtime_error(strprintf("loadStyleSheet: Failed to open file: %s", file.toStdString()));
                 }
 
                 QString strStyle = QLatin1String(qFile.readAll());

src/qt/guiutil_font.cpp

@@ -621,7 +621,7 @@ bool loadFonts()
     std::vector<int> vecFontIds{};
     auto importFont = [&vecFontIds](const QString& font_name) -> void {
         vecFontIds.push_back(QFontDatabase::addApplicationFont(font_name));
-        qDebug() << qstrprintf("%s: %s loaded with id %d", __func__, font_name.toStdString(), vecFontIds.back());
+        qDebug() << qstrprintf("loadFonts: %s loaded with id %d", font_name.toStdString(), vecFontIds.back());
     };
 
     // Import the embedded Roboto Mono used by fixedPitchFont(use_embedded_font=true)

src/rpc/masternode.cpp

@@ -458,7 +458,7 @@ static RPCHelpMan masternode_payments()
         const MnRewardEra mn_reward_era{GetMnRewardEraAfter(pindex->pprev, chainman)};
         if (!GetMasternodePayments(node.dmnman->GetListForBlock(pindex->pprev), pindex->pprev, blockSubsidy, nBlockFees,
                                    mn_reward_era, Params().GetConsensus(), voutMasternodePayments)) {
-            LogPrint(BCLog::MNPAYMENTS, "%s -- No masternode to pay (MN list probably empty)\n", __func__);
+            LogPrint(BCLog::MNPAYMENTS, "%s -- No masternode to pay (MN list probably empty)\n", self.m_name);
         }
 
         UniValue blockObj(UniValue::VOBJ);

src/rpc/mempool.cpp

@@ -462,7 +462,7 @@ static RPCHelpMan getmempoolancestors()
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool");
     }
 
-    auto ancestors{mempool.AssumeCalculateMemPoolAncestors(__func__, *it, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)};
+    auto ancestors{mempool.AssumeCalculateMemPoolAncestors(self.m_name, *it, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)};
 
     if (!fVerbose) {
         UniValue o(UniValue::VARR);