Merge bitcoin#19556: Remove mempool global

fafb381 Remove mempool global (MarcoFalke)
fa0359c Remove mempool global from p2p (MarcoFalke)
eeee110 Remove mempool global from init (MarcoFalke)

Pull request description:

  This refactor unlocks some nice potential features, such as, but not limited to:
  * Removing the fee estimates global (would avoid slightly fragile workarounds such as bitcoin#18766)
  * Making the mempool optional for a "blocksonly" operation mode

  Even absent those features, the new code without the global should be easier to maintain, read and write tests for.

ACKs for top commit:
  jnewbery:
    utACK fafb381
  hebasto:
    ACK fafb381, I have reviewed the code and it looks OK, I agree it can be merged.
  darosior:
    ACK fafb381

Tree-SHA512: a2e696dc377e2e81eaf9c389e6d13dde4a48d81f3538df88f4da502d3012dd61078495140ab5a5854f360a06249fe0e1f6a094c4e006d8b5cc2552a946becf26

Author: MarcoFalke

src/coinjoin/client.cpp

@@ -134,7 +134,7 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream&
     }
 }
 
-void CCoinJoinClientManager::ProcessMessage(CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman)
+void CCoinJoinClientManager::ProcessMessage(const CTxMemPool& mempool, CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman)
 {
     if (fMasternodeMode) return;
     if (!CCoinJoinClientOptions::IsEnabled()) return;
@@ -153,12 +153,12 @@ void CCoinJoinClientManager::ProcessMessage(CNode& peer, std::string_view msg_ty
         AssertLockNotHeld(cs_deqsessions);
         LOCK(cs_deqsessions);
         for (auto& session : deqSessions) {
-            session.ProcessMessage(peer, msg_type, vRecv, connman);
+            session.ProcessMessage(mempool, peer, msg_type, vRecv, connman);
         }
     }
 }
 
-void CCoinJoinClientSession::ProcessMessage(CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman)
+void CCoinJoinClientSession::ProcessMessage(const CTxMemPool& mempool, CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman)
 {
     if (fMasternodeMode) return;
     if (!CCoinJoinClientOptions::IsEnabled()) return;
@@ -193,7 +193,7 @@ void CCoinJoinClientSession::ProcessMessage(CNode& peer, std::string_view msg_ty
         LogPrint(BCLog::COINJOIN, "DSFINALTX -- txNew %s", txNew.ToString()); /* Continued */
 
         // check to see if input is spent already? (and probably not confirmed)
-        SignFinalTransaction(txNew, peer, connman);
+        SignFinalTransaction(mempool, txNew, peer, connman);
 
     } else if (msg_type == NetMsgType::DSCOMPLETE) {
         if (!mixingMasternode) return;
@@ -539,7 +539,7 @@ void CCoinJoinClientSession::ProcessPoolStateUpdate(CCoinJoinStatusUpdate psssup
 // check it to make sure it's what we want, then sign it if we agree.
 // If we refuse to sign, it's possible we'll be charged collateral
 //
-bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTransactionNew, CNode& peer, CConnman& connman)
+bool CCoinJoinClientSession::SignFinalTransaction(const CTxMemPool& mempool, const CTransaction& finalTransactionNew, CNode& peer, CConnman& connman)
 {
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
 
@@ -568,7 +568,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTrans
 
     // Make sure all inputs/outputs are valid
     PoolMessage nMessageID{MSG_NOERR};
-    if (!IsValidInOuts(finalMutableTransaction.vin, finalMutableTransaction.vout, nMessageID, nullptr)) {
+    if (!IsValidInOuts(mempool, finalMutableTransaction.vin, finalMutableTransaction.vout, nMessageID, nullptr)) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CCoinJoin::GetMessageByID(nMessageID).translated);
         UnlockCoins();
         keyHolderStorage.ReturnAll();
@@ -756,7 +756,7 @@ bool CCoinJoinClientManager::CheckAutomaticBackup()
 //
 // Passively run mixing in the background to mix funds based on the given configuration.
 //
-bool CCoinJoinClientSession::DoAutomaticDenominating(CConnman& connman, bool fDryRun)
+bool CCoinJoinClientSession::DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun)
 {
     if (fMasternodeMode) return false; // no client-side mixing on masternodes
     if (nState != POOL_STATE_IDLE) return false;
@@ -909,7 +909,7 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(CConnman& connman, bool fDr
                 return false;
             }
         } else {
-            if (!CCoinJoin::IsCollateralValid(CTransaction(txMyCollateral))) {
+            if (!CCoinJoin::IsCollateralValid(mempool, CTransaction(txMyCollateral))) {
                 LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::DoAutomaticDenominating -- invalid collateral, recreating...\n");
                 if (!CreateCollateralTransaction(txMyCollateral, strReason)) {
                     LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::DoAutomaticDenominating -- create collateral error: %s\n", strReason);
@@ -936,7 +936,7 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(CConnman& connman, bool fDr
     return false;
 }
 
-bool CCoinJoinClientManager::DoAutomaticDenominating(CConnman& connman, bool fDryRun)
+bool CCoinJoinClientManager::DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun)
 {
     if (fMasternodeMode) return false; // no client-side mixing on masternodes
     if (!CCoinJoinClientOptions::IsEnabled() || !IsMixing()) return false;
@@ -978,7 +978,7 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(CConnman& connman, bool fDr
             return false;
         }
 
-        fResult &= session.DoAutomaticDenominating(connman, fDryRun);
+        fResult &= session.DoAutomaticDenominating(mempool, connman, fDryRun);
     }
 
     return fResult;
@@ -1815,7 +1815,7 @@ void CCoinJoinClientQueueManager::DoMaintenance()
     CheckQueue();
 }
 
-void CCoinJoinClientManager::DoMaintenance(CConnman& connman)
+void CCoinJoinClientManager::DoMaintenance(CTxMemPool& mempool, CConnman& connman)
 {
     if (!CCoinJoinClientOptions::IsEnabled()) return;
     if (m_mn_sync == nullptr) return;
@@ -1830,7 +1830,7 @@ void CCoinJoinClientManager::DoMaintenance(CConnman& connman)
     CheckTimeout();
     ProcessPendingDsaRequest(connman);
     if (nDoAutoNextRun == nTick) {
-        DoAutomaticDenominating(connman);
+        DoAutomaticDenominating(mempool, connman);
         nDoAutoNextRun = nTick + COINJOIN_AUTO_TIMEOUT_MIN + GetRandInt(COINJOIN_AUTO_TIMEOUT_MAX - COINJOIN_AUTO_TIMEOUT_MIN);
     }
 }
@@ -1867,14 +1867,14 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
     obj.pushKV("sessions",  arrSessions);
 }
 
-void DoCoinJoinMaintenance(CConnman& connman)
+void DoCoinJoinMaintenance(CTxMemPool& mempool, CConnman& connman)
 {
     if (coinJoinClientQueueManager != nullptr) {
         coinJoinClientQueueManager->DoMaintenance();
     }
 
     for (const auto& pair : coinJoinClientManagers) {
-        pair.second->DoMaintenance(connman);
+        pair.second->DoMaintenance(mempool, connman);
     }
 }
 

src/coinjoin/client.h

@@ -20,6 +20,7 @@ class CCoinJoinClientQueueManager;
 
 class CConnman;
 class CNode;
+class CTxMemPool;
 
 class UniValue;
 class CMasternodeSync;
@@ -114,7 +115,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
     void CompletedTransaction(PoolMessage nMessageID);
 
     /// As a client, check and sign the final transaction
-    bool SignFinalTransaction(const CTransaction& finalTransactionNew, CNode& peer, CConnman& connman) LOCKS_EXCLUDED(cs_coinjoin);
+    bool SignFinalTransaction(const CTxMemPool& mempool, const CTransaction& finalTransactionNew, CNode& peer, CConnman& connman) LOCKS_EXCLUDED(cs_coinjoin);
 
     void RelayIn(const CCoinJoinEntry& entry, CConnman& connman) const;
 
@@ -126,7 +127,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
     {
     }
 
-    void ProcessMessage(CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman);
+    void ProcessMessage(const CTxMemPool& mempool, CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman);
 
     void UnlockCoins();
 
@@ -137,7 +138,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
     bool GetMixingMasternodeInfo(CDeterministicMNCPtr& ret) const;
 
     /// Passively run mixing in the background according to the configuration in settings
-    bool DoAutomaticDenominating(CConnman& connman, bool fDryRun = false) LOCKS_EXCLUDED(cs_coinjoin);
+    bool DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun = false) LOCKS_EXCLUDED(cs_coinjoin);
 
     /// As a client, submit part of a future mixing transaction to a Masternode to start the process
     bool SubmitDenominate(CConnman& connman);
@@ -187,6 +188,7 @@ class CCoinJoinClientManager
     bilingual_str strAutoDenomResult;
 
     CWallet& mixingWallet;
+//    CTxMemPool& mempool;
 
     // Keep track of current block height
     int nCachedBlockHeight{0};
@@ -205,9 +207,10 @@ class CCoinJoinClientManager
     CCoinJoinClientManager& operator=(CCoinJoinClientManager const&) = delete;
 
     explicit CCoinJoinClientManager(CWallet& wallet, const std::unique_ptr<CMasternodeSync>& mn_sync) :
-        mixingWallet(wallet), m_mn_sync(mn_sync) {}
+        mixingWallet(wallet),
+        m_mn_sync(mn_sync) {}
 
-    void ProcessMessage(CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman) LOCKS_EXCLUDED(cs_deqsessions);
+    void ProcessMessage(const CTxMemPool& mempool, CNode& peer, std::string_view msg_type, CDataStream& vRecv, CConnman& connman) LOCKS_EXCLUDED(cs_deqsessions);
 
     bool StartMixing();
     void StopMixing();
@@ -220,7 +223,7 @@ class CCoinJoinClientManager
     bool GetMixingMasternodesInfo(std::vector<CDeterministicMNCPtr>& vecDmnsRet) const LOCKS_EXCLUDED(cs_deqsessions);
 
     /// Passively run mixing in the background according to the configuration in settings
-    bool DoAutomaticDenominating(CConnman& connman, bool fDryRun = false) LOCKS_EXCLUDED(cs_deqsessions);
+    bool DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun = false) LOCKS_EXCLUDED(cs_deqsessions);
 
     bool TrySubmitDenominate(const CService& mnAddr, CConnman& connman) LOCKS_EXCLUDED(cs_deqsessions);
     bool MarkAlreadyJoinedQueueAsTried(CCoinJoinQueue& dsq) const LOCKS_EXCLUDED(cs_deqsessions);
@@ -236,12 +239,12 @@ class CCoinJoinClientManager
 
     void UpdatedBlockTip(const CBlockIndex* pindex);
 
-    void DoMaintenance(CConnman& connman);
+    void DoMaintenance(CTxMemPool& mempool, CConnman& connman);
 
     void GetJsonInfo(UniValue& obj) const LOCKS_EXCLUDED(cs_deqsessions);
 };
 
 
-void DoCoinJoinMaintenance(CConnman& connman);
+void DoCoinJoinMaintenance(CTxMemPool& mempool, CConnman& connman);
 
 #endif // BITCOIN_COINJOIN_CLIENT_H

src/coinjoin/coinjoin.cpp

@@ -217,7 +217,7 @@ std::string CCoinJoinBaseSession::GetStateString() const
     }
 }
 
-bool CCoinJoinBaseSession::IsValidInOuts(const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const
+bool CCoinJoinBaseSession::IsValidInOuts(const CTxMemPool& mempool, const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const
 {
     std::set<CScript> setScripPubKeys;
     nMessageIDRet = MSG_NOERR;
@@ -308,7 +308,7 @@ Mutex CCoinJoin::cs_mapdstx;
 std::map<uint256, CCoinJoinBroadcastTx> CCoinJoin::mapDSTX GUARDED_BY(CCoinJoin::cs_mapdstx);
 
 // check to make sure the collateral provided by the client is valid
-bool CCoinJoin::IsCollateralValid(const CTransaction& txCollateral)
+bool CCoinJoin::IsCollateralValid(CTxMemPool& mempool, const CTransaction& txCollateral)
 {
     if (txCollateral.vout.empty()) return false;
     if (txCollateral.nLockTime != 0) return false;

src/coinjoin/coinjoin.h

@@ -24,6 +24,7 @@ class CConnman;
 class CBLSPublicKey;
 class CBlockIndex;
 class CMasternodeSync;
+class CTxMemPool;
 
 namespace llmq {
 class CChainLocksHandler;
@@ -343,7 +344,7 @@ class CCoinJoinBaseSession
 
     void SetNull() EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);
 
-    bool IsValidInOuts(const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const;
+    bool IsValidInOuts(const CTxMemPool& mempool, const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const;
 
 public:
     int nSessionDenom{0}; // Users must submit a denom matching this
@@ -472,7 +473,7 @@ class CCoinJoin
     static constexpr CAmount GetMaxPoolAmount() { return COINJOIN_ENTRY_MAX_SIZE * vecStandardDenominations.front(); }
 
     /// If the collateral is valid given by a client
-    static bool IsCollateralValid(const CTransaction& txCollateral);
+    static bool IsCollateralValid(CTxMemPool& mempool, const CTransaction& txCollateral);
     static constexpr CAmount GetCollateralAmount() { return GetSmallestDenomination() / 10; }
     static constexpr CAmount GetMaxCollateralAmount() { return GetCollateralAmount() * 4; }
 

src/coinjoin/server.cpp

@@ -577,7 +577,7 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
         return false;
     }
 
-    if (!CCoinJoin::IsCollateralValid(*entry.txCollateral)) {
+    if (!CCoinJoin::IsCollateralValid(mempool, *entry.txCollateral)) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: collateral not valid!\n", __func__);
         nMessageIDRet = ERR_INVALID_COLLATERAL;
         return false;
@@ -611,7 +611,7 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
     }
 
     bool fConsumeCollateral{false};
-    if (!IsValidInOuts(vin, entry.vecTxOut, nMessageIDRet, &fConsumeCollateral)) {
+    if (!IsValidInOuts(mempool, vin, entry.vecTxOut, nMessageIDRet, &fConsumeCollateral)) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CCoinJoin::GetMessageByID(nMessageIDRet).translated);
         if (fConsumeCollateral) {
             ConsumeCollateral(entry.txCollateral);
@@ -688,7 +688,7 @@ bool CCoinJoinServer::IsAcceptableDSA(const CCoinJoinAccept& dsa, PoolMessage& n
     }
 
     // check collateral
-    if (!fUnitTest && !CCoinJoin::IsCollateralValid(CTransaction(dsa.txCollateral))) {
+    if (!fUnitTest && !CCoinJoin::IsCollateralValid(mempool, CTransaction(dsa.txCollateral))) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- collateral not valid!\n", __func__);
         nMessageIDRet = ERR_INVALID_COLLATERAL;
         return false;

src/coinjoin/server.h

@@ -9,6 +9,7 @@
 #include <net.h>
 
 class CCoinJoinServer;
+class CTxMemPool;
 class UniValue;
 
 // The main object for accessing mixing
@@ -19,6 +20,7 @@ extern std::unique_ptr<CCoinJoinServer> coinJoinServer;
 class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager
 {
 private:
+    CTxMemPool& mempool;
     CConnman& connman;
     const std::unique_ptr<CMasternodeSync>& m_mn_sync;
 
@@ -75,10 +77,11 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager
     void SetNull() EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);
 
 public:
-    explicit CCoinJoinServer(CConnman& _connman, const std::unique_ptr<CMasternodeSync>& mn_sync) :
+    explicit CCoinJoinServer(CTxMemPool& mempool, CConnman& _connman, const std::unique_ptr<CMasternodeSync>& mn_sync) :
         vecSessionCollaterals(),
         fUnitTest(false),
         connman(_connman),
+        mempool(mempool),
         m_mn_sync(mn_sync) {};
 
     void ProcessMessage(CNode& pfrom, std::string_view msg_type, CDataStream& vRecv);