refactor: subsume CoinJoin objects under CJContext, deglobalize coinJoin{ClientQueueManager,Server}

[kwvg]

Motivation

CoinJoin's subsystems are initialized by variables and managers that occupy the global context. The extent to which these subsystems entrench themselves into the codebase is difficult to assess and moving them out of the global context forces us to enumerate the subsystems in the codebase that rely on CoinJoin logic and enumerate the order in which components are initialized and destroyed.

Keeping this in mind, the scope of this pull request aims to:

• Reduce the amount of CoinJoin-specific entities present in the global scope
• Make the remaining usage of these entities in the global scope explicit and easily searchable

Additional Information

• The initialization of CCoinJoinClientQueueManager is dependent on blocks-only mode being disabled (which can be alternatively interpreted as enabling the relay of transactions). The same applies to CBlockPolicyEstimator, which CCoinJoinClientQueueManager depends.

  Therefore, CCoinJoinClientQueueManager is only initialized if transaction relaying is enabled and so is its scheduled maintenance task. This can be found by looking at init.cpp here, here and here.

  For this reason, CBlockPolicyEstimator is not a member of CJContext and its usage is fulfilled by passing it as a reference when initializing the scheduling task.

• CJClientManager has not used CConnman or CTxMemPool as const as existing code that is outside the scope of this PR would cast away constness, which would be unacceptable. Furthermore, some logical paths are taken that will grind to a halt if they are stored as const.

  Examples of such a call chains would be:

  • CJClientManager::DoMaintenance > CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating > CCoinJoinClientSession::DoAutomaticDenominating > CCoinJoinClientSession::StartNewQueue > CConnman::AddPendingMasternode which modifies CConnman::vPendingMasternodes, which is non-const behaviour

  • CJClientManager::DoMaintenance > CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating > CCoinJoin::IsCollateralValid > AcceptToMemoryPool which adds a transaction to the memory pool, which is non-const behaviour

• There were cppcheck linter failures that seemed to be caused by the usage of Assert in coinjoin/client.h. This seems to be resolved by backporting bitcoin#24714. (Thanks @knst!)

  • Depends on backport: merge bitcoin#23842, #24714, #21059, #23994, #19866, #19935, #23756, #19064, #23411, #20080 (util backports) #5546

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[PastaPastaPasta]

Please take a look at this patch

Subject: [PATCH] patch
---
Index: src/coinjoin/client.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp
--- a/src/coinjoin/client.cpp	(revision 15722d75cff7105725d2ce54d8f21eac14e26797)
+++ b/src/coinjoin/client.cpp	(date 1694453117373)
@@ -1050,14 +1050,13 @@
 bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman)
 {
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
-    if (m_queueman == nullptr) return false;
 
     const auto mnList = deterministicMNManager->GetListAtChainTip();
     const int nWeightedMnCount = mnList.GetValidWeightedMNsCount();
 
     // Look through the queues and see if anything matches
     CCoinJoinQueue dsq;
-    while (m_queueman->GetQueueItemAndTry(dsq)) {
+    while (m_queueman.GetQueueItemAndTry(dsq)) {
         auto dmn = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint);
 
         if (!dmn) {
@@ -1880,14 +1879,14 @@
 }
 
 void CJClientManager::Add(CWallet& wallet) {
-    m_wallet_manager_map.emplace(
+    m_wallet_manager_map.try_emplace(
         wallet.GetName(),
         std::make_unique<CCoinJoinClientManager>(wallet, *this, m_mn_sync, m_queueman)
     );
 }
 
 void CJClientManager::DoMaintenance(CBlockPolicyEstimator& fee_estimator) {
-    for (auto& pair : m_wallet_manager_map) {
-        pair.second->DoMaintenance(m_connman, fee_estimator, m_mempool);
+    for (auto& [_, cj_man] : m_wallet_manager_map) {
+        cj_man->DoMaintenance(m_connman, fee_estimator, m_mempool);
     }
 }
Index: src/coinjoin/context.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/coinjoin/context.cpp b/src/coinjoin/context.cpp
--- a/src/coinjoin/context.cpp	(revision 15722d75cff7105725d2ce54d8f21eac14e26797)
+++ b/src/coinjoin/context.cpp	(date 1694444524652)
@@ -18,7 +18,7 @@
     clientman {
         [&]() -> CJClientManager* const {
             assert(::coinJoinClientManagers == nullptr);
-            ::coinJoinClientManagers = std::make_unique<CJClientManager>(connman, mempool, mn_sync, queueman);
+            ::coinJoinClientManagers = std::make_unique<CJClientManager>(connman, mempool, mn_sync, *queueman);
             return ::coinJoinClientManagers.get();
         }()
     },
Index: src/coinjoin/client.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h
--- a/src/coinjoin/client.h	(revision 15722d75cff7105725d2ce54d8f21eac14e26797)
+++ b/src/coinjoin/client.h	(date 1694453552985)
@@ -73,11 +73,11 @@
 class CJClientManager {
 public:
     CJClientManager(CConnman& connman, CTxMemPool& mempool, const CMasternodeSync& mn_sync,
-                    const std::unique_ptr<CCoinJoinClientQueueManager>& queueman)
+                    CCoinJoinClientQueueManager& queueman)
         : m_connman(connman), m_mempool(mempool), m_mn_sync(mn_sync), m_queueman(queueman) {}
     ~CJClientManager() {
-        for (auto& pair : m_wallet_manager_map) {
-            pair.second.reset();
+        for (auto& [wallet_name, cj_man] : m_wallet_manager_map) {
+            cj_man.reset();
         }
     }
 
@@ -85,8 +85,7 @@
     void DoMaintenance(CBlockPolicyEstimator& fee_estimator);
 
     void Remove(const std::string& name) {
-        auto it = m_wallet_manager_map.find(name);
-        if (it != m_wallet_manager_map.end()) { m_wallet_manager_map.erase(it); }
+        m_wallet_manager_map.erase(name);
     }
 
     CCoinJoinClientManager* Get(const CWallet& wallet) const {
@@ -94,27 +93,27 @@
         return (it != m_wallet_manager_map.end()) ? it->second.get() : nullptr;
     }
 
-    const std::map<const std::string, std::unique_ptr<CCoinJoinClientManager>>& raw() const { return m_wallet_manager_map; }
+    using wallet_name_cjman_map = std::map<const std::string, std::unique_ptr<CCoinJoinClientManager>>;
+    const wallet_name_cjman_map& raw() const { return m_wallet_manager_map; }
 
 private:
     CConnman& m_connman;
     CTxMemPool& m_mempool;
 
     const CMasternodeSync& m_mn_sync;
-    const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman;
+    CCoinJoinClientQueueManager& m_queueman;
 
-    std::map<const std::string, std::unique_ptr<CCoinJoinClientManager>> m_wallet_manager_map;
+    wallet_name_cjman_map m_wallet_manager_map;
 };
 
 class CCoinJoinClientSession : public CCoinJoinBaseSession
 {
 private:
     CWallet& m_wallet;
-    CJClientManager& m_clientman;
     CCoinJoinClientManager& m_manager;
 
     const CMasternodeSync& m_mn_sync;
-    const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman;
+    CCoinJoinClientQueueManager& m_queueman;
 
     std::vector<COutPoint> vecOutPointLocked;
 
@@ -163,8 +162,8 @@
 
 public:
     explicit CCoinJoinClientSession(CWallet& pwallet, CJClientManager& clientman, const CMasternodeSync& mn_sync,
-                                    const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
-        m_wallet(pwallet), m_clientman(clientman), m_manager(*Assert(clientman.Get(pwallet))), m_mn_sync(mn_sync), m_queueman(queueman) {}
+                                    CCoinJoinClientQueueManager& queueman) :
+        m_wallet(pwallet), m_manager(*Assert(clientman.Get(pwallet))), m_mn_sync(mn_sync), m_queueman(queueman) {}
 
     void ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv);
 
@@ -217,7 +216,7 @@
     CJClientManager& m_clientman;
 
     const CMasternodeSync& m_mn_sync;
-    const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman;
+    CCoinJoinClientQueueManager& m_queueman;
 
     // Keep track of the used Masternodes
     std::vector<COutPoint> vecMasternodesUsed;
@@ -249,7 +248,7 @@
     CCoinJoinClientManager& operator=(CCoinJoinClientManager const&) = delete;
 
     explicit CCoinJoinClientManager(CWallet& wallet, CJClientManager& clientman, const CMasternodeSync& mn_sync,
-                                    const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
+                                    CCoinJoinClientQueueManager& queueman) :
         m_wallet(wallet), m_clientman(clientman), m_mn_sync(mn_sync), m_queueman(queueman) {}
 
     void ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv) LOCKS_EXCLUDED(cs_deqsessions);

[kwvg]

Dropped 954bbcf due to test failures (pipeline)

[knst]

Dropped 954bbcf due to test failures (pipeline)

   clientman {
        [&]() -> CJClientManager* const {
            assert(::coinJoinClientManagers == nullptr);
            ::coinJoinClientManagers = std::make_unique<CJClientManager>(connman, mempool, mn_sync, *queueman);
            return ::coinJoinClientManagers.get();
        }()
    },
queueman {relay_txes ? std::make_unique<CCoinJoinClientQueueManager>(connman, *clientman, mn_sync) : nullptr},

queueman requires clientman for init; clientman requires queueman for init. unique_ptr is required at the moment for an implementation indeed, 954bbcf can't be applied as it is implemented now.

[kwvg]

queueman requires clientman for init; clientman requires queueman for init. unique_ptr is required at the moment for an implementation indeed

This was initially why I used unique_ptr, The patch seemed to indicate that perhaps in between the time I started working on this PR and the time it's being reviewed at, circumstances have changed (or that I had missed something). Tests passed successfully on my local machine so it was added in.

CI has shown that the patch caused problems (the exact same I found early on when working on this PR) and so I've reversed it.

[PastaPastaPasta]

utACK for squash merge

[knst]

Dropped 954bbcf due to test failures (pipeline)

btw, check this one: knst@be2494f

[kwvg]

btw, check this one: knst@be2494f

It is a clever fix, though, this mildly obscures the circular dependency. Part of the deglobalization effort is to make the web of dependencies more self-documenting.

I'm a bit uncertain about adding this, at least as part of this PR but would be fine if it was applied consistently for circular dependencies and then documented explicitly. Would be easier to document the origin of this practice.

[knst]

IMO reference is [almost] always better than pointer because you never need to think can this object be nullptr? Should I check for nullptr?. There's still question who own it? but it is common between pointer and reference. if you want to show circular dependency clear as possible, may be need to add extra comment here?

btw, I am ok with knst@be2494f or whevener without it to approve PR because this PR looks as complete and whole anyway and my commit is just one more minor improvement that can be part of this or any next PR of coinjoin's refactorings.

[UdjinM6]

utACK