Merge #14384: Fire TransactionRemovedFromMempool callbacks from mempool

laanwj · laanwj · commit bb862d7864cc · 2019-11-22T16:35:08.000+01:00
e20c72f Fire TransactionRemovedFromMempool from mempool (251) Pull request description: This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. It also resolves the `txmempool -> validation -> validationinterface -> txmempool` circular dependency. Ideally, `validationinterface` is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving `txmempool` specific code out of `validationinterface` to `txmempool` where it belongs. ACKs for top commit: jnewbery: ACK e20c72f Tree-SHA512: 354c3ff1113b21a0b511d80d604edfe3846dddae3355e43d1387f68906e54bf5dc01e7c029edc0b8e635b500b2ab97ee50362e2486eb4319f7347ee9a9e6cef3
diff --git a/src/init.cpp b/src/init.cpp
@@ -281,7 +281,6 @@ void Shutdown(NodeContext& node)
     node.chain_clients.clear();
     UnregisterAllValidationInterfaces();
     GetMainSignals().UnregisterBackgroundSignalScheduler();
-    GetMainSignals().UnregisterWithMempoolSignals(mempool);
     globalVerifyHandle.reset();
     ECC_Stop();
     if (node.mempool) node.mempool = nullptr;
@@ -1263,7 +1262,6 @@ bool AppInitMain(NodeContext& node)
     }, 60000);
 
     GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);
-    GetMainSignals().RegisterWithMempoolSignals(mempool);
 
     // Create client interfaces for wallets that are supposed to be loaded
     // according to -wallet and -disablewallet options. This only constructs
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
@@ -17,6 +17,7 @@
 #include <util/system.h>
 #include <util/moneystr.h>
 #include <util/time.h>
+#include <validationinterface.h>
 
 CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee,
                                  int64_t _nTime, unsigned int _entryHeight,
@@ -403,7 +404,12 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces
 
 void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
 {
-    NotifyEntryRemoved(it->GetSharedTx(), reason);
+    CTransactionRef ptx = it->GetSharedTx();
+    NotifyEntryRemoved(ptx, reason);
+    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
+        GetMainSignals().TransactionRemovedFromMempool(ptx);
+    }
+
     const uint256 hash = it->GetTx().GetHash();
     for (const CTxIn& txin : it->GetTx().vin)
         mapNextTx.erase(txin.prevout);
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
@@ -7,9 +7,9 @@
 
 #include <primitives/block.h>
 #include <scheduler.h>
-#include <txmempool.h>
 
 #include <future>
+#include <unordered_map>
 #include <utility>
 
 #include <boost/signals2/signal.hpp>
@@ -46,11 +46,6 @@ struct MainSignalsInstance {
 
 static CMainSignals g_signals;
 
-// This map has to a separate global instead of a member of MainSignalsInstance,
-// because RegisterWithMempoolSignals is currently called before RegisterBackgroundSignalScheduler,
-// so MainSignalsInstance hasn't been created yet.
-static std::unordered_map<CTxMemPool*, boost::signals2::scoped_connection> g_connNotifyEntryRemoved;
-
 void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) {
     assert(!m_internals);
     m_internals.reset(new MainSignalsInstance(&scheduler));
@@ -71,17 +66,6 @@ size_t CMainSignals::CallbacksPending() {
     return m_internals->m_schedulerClient.CallbacksPending();
 }
 
-void CMainSignals::RegisterWithMempoolSignals(CTxMemPool& pool) {
-    g_connNotifyEntryRemoved.emplace(std::piecewise_construct,
-        std::forward_as_tuple(&pool),
-        std::forward_as_tuple(pool.NotifyEntryRemoved.connect(std::bind(&CMainSignals::MempoolEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)))
-    );
-}
-
-void CMainSignals::UnregisterWithMempoolSignals(CTxMemPool& pool) {
-    g_connNotifyEntryRemoved.erase(&pool);
-}
-
 CMainSignals& GetMainSignals()
 {
     return g_signals;
@@ -126,13 +110,6 @@ void SyncWithValidationInterfaceQueue() {
     promise.get_future().wait();
 }
 
-void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason) {
-    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
-        m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] {
-            m_internals->TransactionRemovedFromMempool(ptx);
-        });
-    }
-}
 
 void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {
     // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which
@@ -150,6 +127,12 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
     });
 }
 
+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) {
+    m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] {
+        m_internals->TransactionRemovedFromMempool(ptx);
+    });
+}
+
 void CMainSignals::BlockConnected(const std::shared_ptr<const CBlock> &pblock, const CBlockIndex *pindex, const std::shared_ptr<const std::vector<CTransactionRef>>& pvtxConflicted) {
     m_internals->m_schedulerClient.AddToProcessQueue([pblock, pindex, pvtxConflicted, this] {
         m_internals->BlockConnected(pblock, pindex, *pvtxConflicted);
diff --git a/src/validationinterface.h b/src/validationinterface.h
@@ -21,8 +21,6 @@ class CConnman;
 class CValidationInterface;
 class uint256;
 class CScheduler;
-class CTxMemPool;
-enum class MemPoolRemovalReason;
 
 // These functions dispatch to one or all registered wallets
 
@@ -158,8 +156,6 @@ class CMainSignals {
     friend void ::UnregisterAllValidationInterfaces();
     friend void ::CallFunctionInValidationInterfaceQueue(std::function<void ()> func);
 
-    void MempoolEntryRemoved(CTransactionRef tx, MemPoolRemovalReason reason);
-
 public:
     /** Register a CScheduler to give callbacks which should run in the background (may only be called once) */
     void RegisterBackgroundSignalScheduler(CScheduler& scheduler);
@@ -170,13 +166,10 @@ class CMainSignals {
 
     size_t CallbacksPending();
 
-    /** Register with mempool to call TransactionRemovedFromMempool callbacks */
-    void RegisterWithMempoolSignals(CTxMemPool& pool);
-    /** Unregister with mempool */
-    void UnregisterWithMempoolSignals(CTxMemPool& pool);
 
     void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload);
     void TransactionAddedToMempool(const CTransactionRef &);
+    void TransactionRemovedFromMempool(const CTransactionRef &);
     void BlockConnected(const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex, const std::shared_ptr<const std::vector<CTransactionRef>> &);
     void BlockDisconnected(const std::shared_ptr<const CBlock> &, const CBlockIndex* pindex);
     void ChainStateFlushed(const CBlockLocator &);
diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh
@@ -26,7 +26,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
     "wallet/fees -> wallet/wallet -> wallet/fees"
     "wallet/wallet -> wallet/walletdb -> wallet/wallet"
     "policy/fees -> txmempool -> validation -> policy/fees"
-    "txmempool -> validation -> validationinterface -> txmempool"
     "wallet/scriptpubkeyman -> wallet/wallet -> wallet/scriptpubkeyman"
 )
 

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(`
`26`	`26`	`"wallet/fees -> wallet/wallet -> wallet/fees"`
`27`	`27`	`"wallet/wallet -> wallet/walletdb -> wallet/wallet"`
`28`	`28`	`"policy/fees -> txmempool -> validation -> policy/fees"`
`29`		`- "txmempool -> validation -> validationinterface -> txmempool"`
`30`	`29`	`"wallet/scriptpubkeyman -> wallet/wallet -> wallet/scriptpubkeyman"`
`31`	`30`	`)`
`32`	`31`