Merge #7248: refactor: simplify CoinJoin class hierarchy

eb2142d fix: added missing thread annotation for CJWalletManagerImpl (Konstantin Akimov)
40cd08d refactor: rename CCoinJoinBaseManager to CoinJoinQueueManager and make it non-virtual (Konstantin Akimov)
6fae09a refactor: remove inheritance from CCoinJoinBaseManager in src/test/coinjoin_basemanager_tests.cpp (Konstantin Akimov)
a0f39f2 refactor: removed CCoinJoinBaseManager from inheritance of CCoinJoinServer and make it a member (Konstantin Akimov)
72229ce refactor: inline class CCoinJoinClientQueueManager to CJWalletManagerImpl (Konstantin Akimov)
3b1c2da refactor: inline the middleman class CoinJoinWalletManager to CJWalletManagerImpl (Konstantin Akimov)
e2d8247 refactor: pass directly reference to CCoinJoinClientManager& instead unique_ptr inside ForEachCJClientMan (Konstantin Akimov)
689b4ec refactor: remove passing pointer to unique_ptr to more clear sequence of object initializtion in CJ (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  Current coinjoin subsystem have multiple classes that inherits from each other but some of them doesn't provide any real abstraction level yet complicate architecture.

  CJWalletManagerImpl, CCoinJoinClientManager, and CCoinJoinClientQueueManager are 2 separate pieces of implementation of CJWalletManager while CJWalletManagerImpl is already private and could keep all implementation inside.

  ## What was done?

  This PR simplify CJ architecture by removing boiler code and unneeded abstraction levels to make code easier to support, read and improve.

  The changes for this PR has been spotted and half-done during #7234 but it's not a direct dependency of final solution, so far as it was possible to fully cut CJ from dash-chainstate implementation.

   - replaces all references to unique_ptr to just a reference or just unique_ptr to more clear sequence of object initialization in CJ and ownership
   - inline the middleman class CoinJoinWalletManager to CJWalletManagerImpl which is already private and hidden
   - remove class CCoinJoinClientQueueManager; logic inlined into CJWalletManagerImpl
   - remove interface CoinJoinQueueNotify - que notification no longer needed as a separate abstraction
   - replaced inheritance of CCoinJoinServer : public CCoinJoinBaseManager to composition (m_queueman member)
   - remove inheritance of regression tests from CCoinJoinBaseManager
   - rename CCoinJoinBaseManager to CoinJoinQueueManager and make it non-virtual

   ## CoinJoin Class Diagram — Before vs After [by claude]

      BEFORE

      CCoinJoinBaseManager          CoinJoinQueueNotify
       (virtual ctor/dtor)           (interface)
       - vecCoinJoinQueue             - OnQueueUpdate() [pure]
       - cs_vecqueue
              ▲                              ▲
              │ inherits              │ implements
              ├─────────────────┐     │
      CCoinJoinClientQueueManager  CCoinJoinClientManager
       (owned by CJWalletManagerImpl)  (map entry per wallet)
              │
              │ inherits
              │
       CCoinJoinServer ──also inherits──▶ CCoinJoinBaseSession
                                               ▲
                                               │ inherits
                                         CCoinJoinClientSession

      CJWalletManager (abstract)
       └── CJWalletManagerImpl
            ├── unique_ptr<CCoinJoinClientQueueManager>  m_basequeueman
            └── map<wallet* → CCoinJoinClientManager>

      ---
      AFTER

      CoinJoinQueueManager           ← renamed (no C prefix), no virtual ctor/dtor
       - vecCoinJoinQueue               no longer a base class at all
       - cs_vecqueue
       + TryHasQueueFromMasternode()    new TRY_LOCK helpers
       + TryCheckDuplicate()
       + TryAddQueue()

      CCoinJoinServer  ──inherits──▶  CCoinJoinBaseSession
       - m_queueman: CoinJoinQueueManager   ← OWNED by value (composition)
                                               ▲
                                               │ inherits
                                         CCoinJoinClientSession

      CJWalletManager (abstract)
       └── CJWalletManagerImpl
            ├── unique_ptr<CoinJoinQueueManager>  m_queueman  ← nullptr if !relay_txes
            └── map<wallet* → CCoinJoinClientManager>
                     │
                     │  CCoinJoinClientManager
                     │   - m_queueman: CoinJoinQueueManager*   ← non-owning ptr
                     │                 (points into CJWalletManagerImpl::m_queueman)

  ## How Has This Been Tested?
  Run unit & functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK eb2142d

Tree-SHA512: 03a648885d86c6e251296b3315ddf2ffb0ab3839526cf10a52bb36f53dc7ff1888bc45f69bbdcddd8a32bd940955cf6e8487e84062d0dde9daa32317721eeb5a

Author: PastaPastaPasta

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

@@ -40,122 +40,10 @@ using wallet::CoinType;
 using wallet::CWallet;
 using wallet::ReserveDestination;
 
-CCoinJoinClientQueueManager::CCoinJoinClientQueueManager(CoinJoinWalletManager& walletman, CDeterministicMNManager& dmnman,
-                                                         CMasternodeMetaMan& mn_metaman, const CMasternodeSync& mn_sync) :
-    m_walletman{walletman},
-    m_dmnman{dmnman},
-    m_mn_metaman{mn_metaman},
-    m_mn_sync{mn_sync}
-{
-}
-
-CCoinJoinClientQueueManager::~CCoinJoinClientQueueManager() = default;
-
-MessageProcessingResult CCoinJoinClientQueueManager::ProcessMessage(NodeId from, CConnman& connman,
-                                                                    std::string_view msg_type, CDataStream& vRecv)
-{
-    if (msg_type != NetMsgType::DSQUEUE) {
-        return {};
-    }
-    if (!m_mn_sync.IsBlockchainSynced()) return {};
-
-    assert(m_mn_metaman.IsValid());
-
-    CCoinJoinQueue dsq;
-    vRecv >> dsq;
-
-    MessageProcessingResult ret{};
-    ret.m_to_erase = CInv{MSG_DSQ, dsq.GetHash()};
-
-    if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
-        ret.m_error = MisbehavingError{100};
-        return ret;
-    }
-
-    const auto tip_mn_list = m_dmnman.GetListAtChainTip();
-    if (dsq.masternodeOutpoint.IsNull()) {
-        if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) {
-            dsq.masternodeOutpoint = dmn->collateralOutpoint;
-        } else {
-            ret.m_error = MisbehavingError{10};
-            return ret;
-        }
-    }
-
-    {
-        LOCK(cs_ProcessDSQueue);
-
-        {
-            LOCK(cs_vecqueue);
-            // process every dsq only once
-            for (const auto &q: vecCoinJoinQueue) {
-                if (q == dsq) {
-                    return ret;
-                }
-                if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) {
-                    // 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",
-                             from, dsq.masternodeOutpoint.ToStringShort());
-                    return ret;
-                }
-            }
-        } // cs_vecqueue
-
-        LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());
-
-        if (dsq.IsTimeOutOfBounds()) return ret;
-
-        auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);
-        if (!dmn) return ret;
-
-        if (dsq.m_protxHash.IsNull()) {
-            dsq.m_protxHash = dmn->proTxHash;
-        }
-
-        if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
-            ret.m_error = MisbehavingError{10};
-            return ret;
-        }
-
-        // if the queue is ready, submit if we can
-        if (dsq.fReady &&
-            m_walletman.ForAnyCJClientMan([&connman, &dmn](std::unique_ptr<CCoinJoinClientManager>& clientman) {
-                return clientman->TrySubmitDenominate(dmn->proTxHash, connman);
-            })) {
-            LogPrint(BCLog::COINJOIN, "DSQUEUE -- CoinJoin queue is ready, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());
-            return ret;
-        } else {
-            if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, tip_mn_list.GetCounts().enabled())) {
-                LogPrint(BCLog::COINJOIN, "DSQUEUE -- Masternode %s is sending too many dsq messages\n",
-                         dmn->proTxHash.ToString());
-                return ret;
-            }
-            m_mn_metaman.AllowMixing(dmn->proTxHash);
-
-            LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());
-
-            m_walletman.ForAnyCJClientMan([&dsq](const std::unique_ptr<CCoinJoinClientManager>& clientman) {
-                return clientman->MarkAlreadyJoinedQueueAsTried(dsq);
-            });
-
-            WITH_LOCK(cs_vecqueue, vecCoinJoinQueue.push_back(dsq));
-        }
-    } // cs_ProcessDSQueue
-    return ret;
-}
-
-void CCoinJoinClientQueueManager::DoMaintenance()
-{
-    if (!m_mn_sync.IsBlockchainSynced() || ShutdownRequested()) return;
-
-    CheckQueue();
-}
-
 CCoinJoinClientManager::CCoinJoinClientManager(const std::shared_ptr<wallet::CWallet>& wallet,
                                                CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
                                                const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
-                                               const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
+                                               CoinJoinQueueManager* queueman) :
     m_wallet{wallet},
     m_dmnman{dmnman},
     m_mn_metaman{mn_metaman},
@@ -192,15 +80,13 @@ void CCoinJoinClientManager::ProcessMessage(CNode& peer, CChainState& active_cha
 
 CCoinJoinClientSession::CCoinJoinClientSession(const std::shared_ptr<CWallet>& wallet, CCoinJoinClientManager& clientman,
                                                CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
-                                               const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
-                                               const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
+                                               const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman) :
     m_wallet(wallet),
     m_clientman(clientman),
     m_dmnman(dmnman),
     m_mn_metaman(mn_metaman),
     m_mn_sync(mn_sync),
-    m_isman{isman},
-    m_queueman(queueman)
+    m_isman{isman}
 {}
 
 void CCoinJoinClientSession::ProcessMessage(CNode& peer, CChainState& active_chainstate, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv)
@@ -1007,7 +893,7 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(ChainstateManager& chainman
     AssertLockNotHeld(cs_deqsessions);
     LOCK(cs_deqsessions);
     if (int(deqSessions.size()) < CCoinJoinClientOptions::GetSessions()) {
-        deqSessions.emplace_back(m_wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_isman, m_queueman);
+        deqSessions.emplace_back(m_wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_isman);
     }
     for (auto& session : deqSessions) {
         if (!CheckAutomaticBackup()) return false;
@@ -1075,14 +961,13 @@ static int WinnersToSkip()
 bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman)
 {
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
-    if (m_queueman == nullptr) return false;
 
     const auto mnList = m_dmnman.GetListAtChainTip();
     const int nWeightedMnCount = mnList.GetCounts().m_valid_weighted;
 
     // Look through the queues and see if anything matches
     CCoinJoinQueue dsq;
-    while (m_queueman->GetQueueItemAndTry(dsq)) {
+    while (m_clientman.GetQueueItemAndTry(dsq)) {
         auto dmn = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint);
 
         if (!dmn) {
@@ -1288,6 +1173,11 @@ bool CCoinJoinClientManager::MarkAlreadyJoinedQueueAsTried(CCoinJoinQueue& dsq)
     return false;
 }
 
+bool CCoinJoinClientManager::GetQueueItemAndTry(CCoinJoinQueue& dsq) const
+{
+    return m_queueman && m_queueman->GetQueueItemAndTry(dsq);
+}
+
 bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman)
 {
     LOCK(m_wallet->cs_wallet);
@@ -1893,59 +1783,3 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
     obj.pushKV("sessions", arrSessions);
 }
 
-CoinJoinWalletManager::CoinJoinWalletManager(ChainstateManager& chainman, CDeterministicMNManager& dmnman,
-                                             CMasternodeMetaMan& mn_metaman, const CTxMemPool& mempool,
-                                             const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
-                                             const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
-    m_chainman{chainman},
-    m_dmnman{dmnman},
-    m_mn_metaman{mn_metaman},
-    m_mempool{mempool},
-    m_mn_sync{mn_sync},
-    m_isman{isman},
-    m_queueman{queueman}
-{
-}
-
-CoinJoinWalletManager::~CoinJoinWalletManager()
-{
-    LOCK(cs_wallet_manager_map);
-    for (auto& [wallet_name, cj_man] : m_wallet_manager_map) {
-        cj_man.reset();
-    }
-}
-
-void CoinJoinWalletManager::Add(const std::shared_ptr<CWallet>& wallet)
-{
-    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_queueman));
-}
-
-void CoinJoinWalletManager::DoMaintenance(CConnman& connman)
-{
-    LOCK(cs_wallet_manager_map);
-    for (auto& [_, clientman] : m_wallet_manager_map) {
-        clientman->DoMaintenance(m_chainman, connman, m_mempool);
-    }
-}
-
-void CoinJoinWalletManager::Remove(const std::string& name) {
-    LOCK(cs_wallet_manager_map);
-    m_wallet_manager_map.erase(name);
-}
-
-void CoinJoinWalletManager::Flush(const std::string& name)
-{
-    auto clientman = Assert(Get(name));
-    clientman->ResetPool();
-    clientman->StopMixing();
-}
-
-CCoinJoinClientManager* CoinJoinWalletManager::Get(const std::string& name) const
-{
-    LOCK(cs_wallet_manager_map);
-    auto it = m_wallet_manager_map.find(name);
-    return (it != m_wallet_manager_map.end()) ? it->second.get() : nullptr;
-}