refactor: rename CCoinJoinBaseManager to CoinJoinQueueManager and make it non-virtual

Author: knst

contrib/auto_gdb/simple_class_obj.py

@@ -14,7 +14,7 @@
                 "CMasternodeBroadcast", "CMasternodePing",
                 "CMasternodeMan", "CDarksendQueue", "CDarkSendEntry",
                 "CTransaction", "CMutableTransaction", "CCoinJoinBaseSession",
-                "CCoinJoinBaseManager", "CCoinJoinClientSession",
+                "CoinJoinQueueManager", "CCoinJoinClientSession",
                 "CCoinJoinClientManager", "CCoinJoinServer", "CMasternodePayments",
                 "CMasternodePaymentVote", "CMasternodeBlockPayees",
                 "CMasternodePayee", "CInstantSend", "CTxLockRequest",

src/Makefile.test.include

@@ -122,7 +122,6 @@ BITCOIN_TESTS =\
   test/governance_validators_tests.cpp \
   test/coinjoin_inouts_tests.cpp \
   test/coinjoin_dstxmanager_tests.cpp \
-  test/coinjoin_basemanager_tests.cpp \
   test/coinjoin_queue_tests.cpp \
   test/hash_tests.cpp \
   test/httpserver_tests.cpp \

src/coinjoin/client.cpp

@@ -43,7 +43,7 @@ using wallet::ReserveDestination;
 CCoinJoinClientManager::CCoinJoinClientManager(const std::shared_ptr<wallet::CWallet>& wallet,
                                                CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
                                                const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
-                                               CCoinJoinBaseManager* queueman) :
+                                               CoinJoinQueueManager* queueman) :
     m_wallet{wallet},
     m_dmnman{dmnman},
     m_mn_metaman{mn_metaman},

src/coinjoin/client.h

@@ -163,7 +163,7 @@ class CCoinJoinClientManager
     const CMasternodeSync& m_mn_sync;
     const llmq::CInstantSendManager& m_isman;
     //! Non-owning pointer; null when relay_txes is disabled (no queue processing).
-    CCoinJoinBaseManager* const m_queueman;
+    CoinJoinQueueManager* const m_queueman;
 
     mutable Mutex cs_deqsessions;
     // TODO: or map<denom, CCoinJoinClientSession> ??
@@ -192,7 +192,7 @@ class CCoinJoinClientManager
     CCoinJoinClientManager& operator=(const CCoinJoinClientManager&) = delete;
     explicit CCoinJoinClientManager(const std::shared_ptr<wallet::CWallet>& wallet, CDeterministicMNManager& dmnman,
                                     CMasternodeMetaMan& mn_metaman, const CMasternodeSync& mn_sync,
-                                    const llmq::CInstantSendManager& isman, CCoinJoinBaseManager* queueman);
+                                    const llmq::CInstantSendManager& isman, CoinJoinQueueManager* queueman);
     ~CCoinJoinClientManager();
 
     void ProcessMessage(CNode& peer, CChainState& active_chainstate, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);

src/coinjoin/coinjoin.cpp

@@ -115,17 +115,13 @@ void CCoinJoinBaseSession::SetNull()
     nTimeLastSuccessfulStep = GetTime();
 }
 
-CCoinJoinBaseManager::CCoinJoinBaseManager() = default;
-
-CCoinJoinBaseManager::~CCoinJoinBaseManager() = default;
-
-void CCoinJoinBaseManager::SetNull()
+void CoinJoinQueueManager::SetNull()
 {
     LOCK(cs_vecqueue);
     vecCoinJoinQueue.clear();
 }
 
-void CCoinJoinBaseManager::CheckQueue()
+void CoinJoinQueueManager::CheckQueue()
 {
     TRY_LOCK(cs_vecqueue, lockDS);
     if (!lockDS) return; // it's ok to fail here, we run this quite frequently
@@ -134,22 +130,22 @@ void CCoinJoinBaseManager::CheckQueue()
     auto it = vecCoinJoinQueue.begin();
     while (it != vecCoinJoinQueue.end()) {
         if (it->IsTimeOutOfBounds()) {
-            LogPrint(BCLog::COINJOIN, "CCoinJoinBaseManager::%s -- Removing a queue (%s)\n", __func__, it->ToString());
+            LogPrint(BCLog::COINJOIN, "CoinJoinQueueManager::%s -- Removing a queue (%s)\n", __func__, it->ToString());
             it = vecCoinJoinQueue.erase(it);
         } else {
             ++it;
         }
     }
 }
 
-std::optional<bool> CCoinJoinBaseManager::TryHasQueueFromMasternode(const COutPoint& outpoint) const
+std::optional<bool> CoinJoinQueueManager::TryHasQueueFromMasternode(const COutPoint& outpoint) const
 {
     TRY_LOCK(cs_vecqueue, lockDS);
     if (!lockDS) return std::nullopt;
     return std::ranges::any_of(vecCoinJoinQueue, [&outpoint](const auto& q) { return q.masternodeOutpoint == outpoint; });
 }
 
-std::optional<bool> CCoinJoinBaseManager::TryCheckDuplicate(const CCoinJoinQueue& dsq) const
+std::optional<bool> CoinJoinQueueManager::TryCheckDuplicate(const CCoinJoinQueue& dsq) const
 {
     TRY_LOCK(cs_vecqueue, lockDS);
     if (!lockDS) return std::nullopt;
@@ -160,15 +156,15 @@ std::optional<bool> CCoinJoinBaseManager::TryCheckDuplicate(const CCoinJoinQueue
     return false;
 }
 
-bool CCoinJoinBaseManager::TryAddQueue(CCoinJoinQueue dsq)
+bool CoinJoinQueueManager::TryAddQueue(CCoinJoinQueue dsq)
 {
     TRY_LOCK(cs_vecqueue, lockDS);
     if (!lockDS) return false;
     vecCoinJoinQueue.push_back(std::move(dsq));
     return true;
 }
 
-bool CCoinJoinBaseManager::GetQueueItemAndTry(CCoinJoinQueue& dsqRet)
+bool CoinJoinQueueManager::GetQueueItemAndTry(CCoinJoinQueue& dsqRet)
 {
     TRY_LOCK(cs_vecqueue, lockDS);
     if (!lockDS) return false; // it's ok to fail here, we run this quite frequently

src/coinjoin/coinjoin.h

@@ -321,19 +321,15 @@ class CCoinJoinBaseSession
     int GetEntriesCountLocked() const EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin) { return vecEntries.size(); }
 };
 
-// base class
-class CCoinJoinBaseManager
+class CoinJoinQueueManager
 {
-protected:
+private:
     mutable Mutex cs_vecqueue;
 
     // The current mixing sessions in progress on the network
     std::vector<CCoinJoinQueue> vecCoinJoinQueue GUARDED_BY(cs_vecqueue);
 
 public:
-    CCoinJoinBaseManager();
-    virtual ~CCoinJoinBaseManager();
-
     void SetNull() EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);
 
     //! Remove timed-out queue entries. Call periodically (e.g. every second).

src/coinjoin/server.h

@@ -28,7 +28,7 @@ class UniValue;
 class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler
 {
 private:
-    CCoinJoinBaseManager m_queueman;
+    CoinJoinQueueManager m_queueman;
 
     ChainstateManager& m_chainman;
     CConnman& connman;

src/coinjoin/walletman.cpp

@@ -62,10 +62,10 @@ class CJWalletManagerImpl final : public CJWalletManager
     const CMasternodeSync& m_mn_sync;
     const llmq::CInstantSendManager& m_isman;
 
-    // m_basequeueman is declared before the wallet map so that it is destroyed
+    // m_queueman is declared before the wallet map so that it is destroyed
     // after all CCoinJoinClientManager instances (which hold a raw pointer to it).
     // Null when relay_txes is false (no queue processing).
-    const std::unique_ptr<CCoinJoinBaseManager> m_basequeueman;
+    const std::unique_ptr<CoinJoinQueueManager> m_queueman;
 
     mutable Mutex cs_ProcessDSQueue;
 
@@ -105,7 +105,7 @@ CJWalletManagerImpl::CJWalletManagerImpl(ChainstateManager& chainman, CDetermini
     m_mempool{mempool},
     m_mn_sync{mn_sync},
     m_isman{isman},
-    m_basequeueman{m_relay_txes ? std::make_unique<CCoinJoinBaseManager>() : nullptr}
+    m_queueman{m_relay_txes ? std::make_unique<CoinJoinQueueManager>() : nullptr}
 {
 }
 
@@ -134,8 +134,8 @@ void CJWalletManagerImpl::UpdatedBlockTip(const CBlockIndex* pindexNew, const CB
 
 bool CJWalletManagerImpl::hasQueue(const uint256& hash) const
 {
-    if (m_basequeueman) {
-        return m_basequeueman->HasQueue(hash);
+    if (m_queueman) {
+        return m_queueman->HasQueue(hash);
     }
     return false;
 }
@@ -149,16 +149,16 @@ CCoinJoinClientManager* CJWalletManagerImpl::getClient(const std::string& name)
 
 std::optional<CCoinJoinQueue> CJWalletManagerImpl::getQueueFromHash(const uint256& hash) const
 {
-    if (m_basequeueman) {
-        return m_basequeueman->GetQueueFromHash(hash);
+    if (m_queueman) {
+        return m_queueman->GetQueueFromHash(hash);
     }
     return std::nullopt;
 }
 
 std::optional<int> CJWalletManagerImpl::getQueueSize() const
 {
-    if (m_basequeueman) {
-        return m_basequeueman->GetQueueSize();
+    if (m_queueman) {
+        return m_queueman->GetQueueSize();
     }
     return std::nullopt;
 }
@@ -175,7 +175,7 @@ void CJWalletManagerImpl::addWallet(const std::shared_ptr<wallet::CWallet>& wall
     LOCK(cs_wallet_manager_map);
     m_wallet_manager_map.try_emplace(wallet->GetName(),
                                      std::make_unique<CCoinJoinClientManager>(wallet, m_dmnman, m_mn_metaman, m_mn_sync,
-                                                                              m_isman, m_basequeueman.get()));
+                                                                              m_isman, m_queueman.get()));
 }
 
 void CJWalletManagerImpl::flushWallet(const std::string& name)
@@ -193,8 +193,8 @@ void CJWalletManagerImpl::removeWallet(const std::string& name)
 
 void CJWalletManagerImpl::DoMaintenance(CConnman& connman)
 {
-    if (m_basequeueman && m_mn_sync.IsBlockchainSynced() && !ShutdownRequested()) {
-        m_basequeueman->CheckQueue();
+    if (m_queueman && m_mn_sync.IsBlockchainSynced() && !ShutdownRequested()) {
+        m_queueman->CheckQueue();
     }
     LOCK(cs_wallet_manager_map);
     for (auto& [_, clientman] : m_wallet_manager_map) {
@@ -209,7 +209,7 @@ MessageProcessingResult CJWalletManagerImpl::processMessage(CNode& pfrom, CChain
     ForEachCJClientMan([&](CCoinJoinClientManager& clientman) {
         clientman.ProcessMessage(pfrom, chainstate, connman, mempool, msg_type, vRecv);
     });
-    if (m_basequeueman) {
+    if (m_queueman) {
         return ProcessDSQueue(pfrom.GetId(), connman, msg_type, vRecv);
     }
     return {};
@@ -218,7 +218,7 @@ MessageProcessingResult CJWalletManagerImpl::processMessage(CNode& pfrom, CChain
 MessageProcessingResult CJWalletManagerImpl::ProcessDSQueue(NodeId from, CConnman& connman, std::string_view msg_type,
                                                             CDataStream& vRecv)
 {
-    assert(m_basequeueman);
+    assert(m_queueman);
 
     if (msg_type != NetMsgType::DSQUEUE) {
         return {};
@@ -251,9 +251,9 @@ MessageProcessingResult CJWalletManagerImpl::ProcessDSQueue(NodeId from, CConnma
     {
         LOCK(cs_ProcessDSQueue);
 
-        if (m_basequeueman->HasQueue(dsq.GetHash())) return ret;
+        if (m_queueman->HasQueue(dsq.GetHash())) return ret;
 
-        if (m_basequeueman->HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady)) {
+        if (m_queueman->HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady)) {
             // no way the same mn can send another dsq with the same readiness this soon
             LogPrint(BCLog::COINJOIN, /* Continued */
                      "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n",
@@ -298,7 +298,7 @@ MessageProcessingResult CJWalletManagerImpl::ProcessDSQueue(NodeId from, CConnma
             ForAnyCJClientMan(
                 [&dsq](CCoinJoinClientManager& clientman) { return clientman.MarkAlreadyJoinedQueueAsTried(dsq); });
 
-            m_basequeueman->AddQueue(dsq);
+            m_queueman->AddQueue(dsq);
         }
     } // cs_ProcessDSQueue
     return ret;

src/test/coinjoin_basemanager_tests.cpp

@@ -96,4 +96,47 @@ BOOST_AUTO_TEST_CASE(queue_timestamp_validation)
     BOOST_CHECK(q.IsTimeOutOfBounds(current_time));
 }
 
+static CCoinJoinQueue MakeQueue(int denom, int64_t nTime, bool fReady, const COutPoint& outpoint)
+{
+    CCoinJoinQueue q;
+    q.nDenom = denom;
+    q.masternodeOutpoint = outpoint;
+    q.m_protxHash = uint256::ONE;
+    q.nTime = nTime;
+    q.fReady = fReady;
+    return q;
+}
+
+BOOST_AUTO_TEST_CASE(queuemanager_checkqueue_removes_timeouts)
+{
+    CoinJoinQueueManager man;
+    const int denom = CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination());
+    const int64_t now = GetAdjustedTime();
+    // Non-expired
+    man.AddQueue(MakeQueue(denom, now, false, COutPoint(uint256S("11"), 0)));
+    // Expired (too old)
+    man.AddQueue(MakeQueue(denom, now - COINJOIN_QUEUE_TIMEOUT - 1, false, COutPoint(uint256S("12"), 0)));
+
+    BOOST_CHECK_EQUAL(man.GetQueueSize(), 2);
+    man.CheckQueue();
+    // One should be removed
+    BOOST_CHECK_EQUAL(man.GetQueueSize(), 1);
+}
+
+BOOST_AUTO_TEST_CASE(queuemanager_getqueueitem_marks_tried_once)
+{
+    CoinJoinQueueManager man;
+    const int denom = CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination());
+    const int64_t now = GetAdjustedTime();
+    CCoinJoinQueue dsq = MakeQueue(denom, now, false, COutPoint(uint256S("21"), 0));
+    man.AddQueue(dsq);
+
+    CCoinJoinQueue picked;
+    // First retrieval should succeed
+    BOOST_CHECK(man.GetQueueItemAndTry(picked));
+    // No other items left to try (picked is marked tried inside)
+    CCoinJoinQueue picked2;
+    BOOST_CHECK(!man.GetQueueItemAndTry(picked2));
+}
+
 BOOST_AUTO_TEST_SUITE_END()