Merge #6875: feat: share vecMasternodesUsed across all wallets, improve its handling

PastaPastaPasta · PastaPastaPasta · commit e4fa575ac463 · 2025-10-28T09:15:05.000-05:00
eb3be09 refactor: code style/log text adjustments (UdjinM6) f50d09a fix: update `MasternodeMetaStore::ToString()` (UdjinM6) 13a28fa perf: optimize CoinJoin masternode tracking with hybrid data structure (UdjinM6) 2c82532 feat: share `vecMasternodesUsed` across all wallets, improve its handling (UdjinM6) Pull request description: ## Issue being fixed or feature implemented `vecMasternodesUsed` has a few issues: - it's shared only across mixing sessions in a single wallet, wallets don't share this data so it's possible they are going to waste tries in `StartNewQueue()` and fail to start mixing because of that - we start with a fresh vector each time which also can result in failed mixing attempts - there are two threads (scheduler and net) where it can be accessed but it's not protected by any mutex atm - it still stores masternode outpoints while most of our codebase uses protxhash-es as masternode ids ## What was done? - moved it to `MasternodeMetaStore` and renamed it to `m_used_masternodes` - changed to `std::vector<uint256>` (using `proTxHash` now) - implemented proper encapsulation through accessor methods - `m_used_masternodes` is no longer accessed via multiple threads (because it's not cleared in `ResetPool()`) but I made it thread-safe anyway just in case (using existing `Mutex cs`) - made it persistent via serialization (with version bump 4 -> 5) ## How Has This Been Tested? Mixing in multiple wallets at once ## Breaking Changes n/a ## Checklist: - [ ] 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 - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK eb3be09 kwvg: utACK eb3be09 Tree-SHA512: 68527f4edb255baf879055d622d72e3aff9807ee37c59dca3b7f4436df8bba67fddba9331b5d8af50ca849fdcfbb57ef003bf093856fc1ba210d43deecd8048c
diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp
@@ -248,7 +248,6 @@ void CCoinJoinClientSession::ResetPool()
 void CCoinJoinClientManager::ResetPool()
 {
     nCachedLastSuccessBlock = 0;
-    vecMasternodesUsed.clear();
     AssertLockNotHeld(cs_deqsessions);
     LOCK(cs_deqsessions);
     for (auto& session : deqSessions) {
@@ -967,11 +966,14 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(ChainstateManager& chainman
     // If we've used 90% of the Masternode list then drop the oldest first ~30%
     int nThreshold_high = nMnCountEnabled * 0.9;
     int nThreshold_low = nThreshold_high * 0.7;
-    WalletCJLogPrint(m_wallet, "Checking vecMasternodesUsed: size: %d, threshold: %d\n", (int)vecMasternodesUsed.size(), nThreshold_high);
+    size_t used_count{m_mn_metaman.GetUsedMasternodesCount()};
 
-    if ((int)vecMasternodesUsed.size() > nThreshold_high) {
-        vecMasternodesUsed.erase(vecMasternodesUsed.begin(), vecMasternodesUsed.begin() + vecMasternodesUsed.size() - nThreshold_low);
-        WalletCJLogPrint(m_wallet, "  vecMasternodesUsed: new size: %d, threshold: %d\n", (int)vecMasternodesUsed.size(), nThreshold_high);
+    WalletCJLogPrint(m_wallet, "Checking threshold - used: %d, threshold: %d\n", (int)used_count, nThreshold_high);
+
+    if ((int)used_count > nThreshold_high) {
+        m_mn_metaman.RemoveUsedMasternodes(used_count - nThreshold_low);
+        WalletCJLogPrint(m_wallet, "  new used: %d, threshold: %d\n", (int)m_mn_metaman.GetUsedMasternodesCount(),
+                         nThreshold_high);
     }
 
     bool fResult = true;
@@ -995,17 +997,17 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(ChainstateManager& chainman
     return fResult;
 }
 
-void CCoinJoinClientManager::AddUsedMasternode(const COutPoint& outpointMn)
+void CCoinJoinClientManager::AddUsedMasternode(const uint256& proTxHash)
 {
-    vecMasternodesUsed.push_back(outpointMn);
+    m_mn_metaman.AddUsedMasternode(proTxHash);
 }
 
 CDeterministicMNCPtr CCoinJoinClientManager::GetRandomNotUsedMasternode()
 {
     auto mnList = m_dmnman.GetListAtChainTip();
 
     size_t nCountEnabled = mnList.GetValidMNsCount();
-    size_t nCountNotExcluded = nCountEnabled - vecMasternodesUsed.size();
+    size_t nCountNotExcluded{nCountEnabled - m_mn_metaman.GetUsedMasternodesCount()};
 
     WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %d enabled masternodes, %d masternodes to choose from\n", __func__, nCountEnabled, nCountNotExcluded);
     if (nCountNotExcluded < 1) {
@@ -1022,15 +1024,14 @@ CDeterministicMNCPtr CCoinJoinClientManager::GetRandomNotUsedMasternode()
     // shuffle pointers
     Shuffle(vpMasternodesShuffled.begin(), vpMasternodesShuffled.end(), FastRandomContext());
 
-    std::set<COutPoint> excludeSet(vecMasternodesUsed.begin(), vecMasternodesUsed.end());
-
-    // loop through
+    // loop through - using direct O(1) lookup instead of creating a set copy
     for (const auto& dmn : vpMasternodesShuffled) {
-        if (excludeSet.count(dmn->collateralOutpoint)) {
+        if (m_mn_metaman.IsUsedMasternode(dmn->proTxHash)) {
             continue;
         }
 
-        WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- found, masternode=%s\n", __func__, dmn->collateralOutpoint.ToStringShort());
+        WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- found, masternode=%s\n", __func__,
+                         dmn->proTxHash.ToString());
         return dmn;
     }
 
@@ -1083,7 +1084,7 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized,
             continue;
         }
 
-        m_clientman.AddUsedMasternode(dsq.masternodeOutpoint);
+        m_clientman.AddUsedMasternode(dmn->proTxHash);
 
         if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) {
             WalletCJLogPrint(m_wallet, /* Continued */
@@ -1137,7 +1138,7 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon
             return false;
         }
 
-        m_clientman.AddUsedMasternode(dmn->collateralOutpoint);
+        m_clientman.AddUsedMasternode(dmn->proTxHash);
 
         // skip next mn payments winners
         if (dmn->pdmnState->nLastPaidHeight + nWeightedMnCount < mnList.GetHeight() + WinnersToSkip()) {
diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h
@@ -262,9 +262,6 @@ class CCoinJoinClientManager
     const llmq::CInstantSendManager& m_isman;
     const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman;
 
-    // Keep track of the used Masternodes
-    std::vector<COutPoint> vecMasternodesUsed;
-
     mutable Mutex cs_deqsessions;
     // TODO: or map<denom, CCoinJoinClientSession> ??
     std::deque<CCoinJoinClientSession> deqSessions GUARDED_BY(cs_deqsessions);
@@ -327,7 +324,7 @@ class CCoinJoinClientManager
 
     void ProcessPendingDsaRequest(CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
 
-    void AddUsedMasternode(const COutPoint& outpointMn);
+    void AddUsedMasternode(const uint256& proTxHash);
     CDeterministicMNCPtr GetRandomNotUsedMasternode();
 
     void UpdatedSuccessBlock();
diff --git a/src/masternode/meta.cpp b/src/masternode/meta.cpp
@@ -8,7 +8,7 @@
 #include <univalue.h>
 #include <util/time.h>
 
-const std::string MasternodeMetaStore::SERIALIZATION_VERSION_STRING = "CMasternodeMetaMan-Version-4";
+const std::string MasternodeMetaStore::SERIALIZATION_VERSION_STRING = "CMasternodeMetaMan-Version-5";
 
 CMasternodeMetaMan::CMasternodeMetaMan() :
     m_db{std::make_unique<db_type>("mncache.dat", "magicMasternodeCache")}
@@ -153,10 +153,44 @@ void CMasternodeMetaMan::RememberPlatformBan(const uint256& inv_hash, PlatformBa
     m_seen_platform_bans.insert(inv_hash, std::move(msg));
 }
 
+void CMasternodeMetaMan::AddUsedMasternode(const uint256& proTxHash)
+{
+    LOCK(cs);
+    // Only add if not already present (prevents duplicates)
+    if (m_used_masternodes_set.insert(proTxHash).second) {
+        m_used_masternodes.push_back(proTxHash);
+    }
+}
+
+void CMasternodeMetaMan::RemoveUsedMasternodes(size_t count)
+{
+    LOCK(cs);
+    size_t removed = 0;
+    while (removed < count && !m_used_masternodes.empty()) {
+        // Remove from both the set and the deque
+        m_used_masternodes_set.erase(m_used_masternodes.front());
+        m_used_masternodes.pop_front();
+        ++removed;
+    }
+}
+
+size_t CMasternodeMetaMan::GetUsedMasternodesCount() const
+{
+    LOCK(cs);
+    return m_used_masternodes.size();
+}
+
+bool CMasternodeMetaMan::IsUsedMasternode(const uint256& proTxHash) const
+{
+    LOCK(cs);
+    return m_used_masternodes_set.find(proTxHash) != m_used_masternodes_set.end();
+}
+
 std::string MasternodeMetaStore::ToString() const
 {
     LOCK(cs);
-    return strprintf("Masternodes: meta infos object count: %d, nDsqCount: %d", metaInfos.size(), nDsqCount);
+    return strprintf("Masternodes: meta infos object count: %d, nDsqCount: %d, used masternodes count: %d",
+                     metaInfos.size(), nDsqCount, m_used_masternodes.size());
 }
 
 uint256 PlatformBanMessage::GetHash() const { return ::SerializeHash(*this); }
diff --git a/src/masternode/meta.h b/src/masternode/meta.h
@@ -14,6 +14,7 @@
 #include <unordered_lru_cache.h>
 
 #include <atomic>
+#include <deque>
 #include <map>
 #include <memory>
 #include <optional>
@@ -139,6 +140,10 @@ class MasternodeMetaStore
     std::map<uint256, CMasternodeMetaInfoPtr> metaInfos GUARDED_BY(cs);
     // keep track of dsq count to prevent masternodes from gaming coinjoin queue
     std::atomic<int64_t> nDsqCount{0};
+    // keep track of the used Masternodes for CoinJoin across all wallets
+    // Using deque for efficient FIFO removal and unordered_set for O(1) lookups
+    std::deque<uint256> m_used_masternodes GUARDED_BY(cs);
+    Uint256HashSet m_used_masternodes_set GUARDED_BY(cs);
 
 public:
     template<typename Stream>
@@ -149,7 +154,9 @@ class MasternodeMetaStore
         for (const auto& p : metaInfos) {
             tmpMetaInfo.emplace_back(*p.second);
         }
-        s << SERIALIZATION_VERSION_STRING << tmpMetaInfo << nDsqCount;
+        // Convert deque to vector for serialization - unordered_set will be rebuilt on deserialization
+        std::vector<uint256> tmpUsedMasternodes(m_used_masternodes.begin(), m_used_masternodes.end());
+        s << SERIALIZATION_VERSION_STRING << tmpMetaInfo << nDsqCount << tmpUsedMasternodes;
     }
 
     template<typename Stream>
@@ -164,18 +171,27 @@ class MasternodeMetaStore
             return;
         }
         std::vector<CMasternodeMetaInfo> tmpMetaInfo;
-        s >> tmpMetaInfo >> nDsqCount;
+        std::vector<uint256> tmpUsedMasternodes;
+        s >> tmpMetaInfo >> nDsqCount >> tmpUsedMasternodes;
+
         metaInfos.clear();
         for (auto& mm : tmpMetaInfo) {
             metaInfos.emplace(mm.GetProTxHash(), std::make_shared<CMasternodeMetaInfo>(std::move(mm)));
         }
+
+        // Convert vector to deque and build unordered_set for O(1) lookups
+        m_used_masternodes.assign(tmpUsedMasternodes.begin(), tmpUsedMasternodes.end());
+        m_used_masternodes_set.clear();
+        m_used_masternodes_set.insert(tmpUsedMasternodes.begin(), tmpUsedMasternodes.end());
     }
 
     void Clear() EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
 
         metaInfos.clear();
+        m_used_masternodes.clear();
+        m_used_masternodes_set.clear();
     }
 
     std::string ToString() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
@@ -257,6 +273,12 @@ class CMasternodeMetaMan : public MasternodeMetaStore
     bool AlreadyHavePlatformBan(const uint256& inv_hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
     std::optional<PlatformBanMessage> GetPlatformBan(const uint256& inv_hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
     void RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg) EXCLUSIVE_LOCKS_REQUIRED(!cs);
+
+    // CoinJoin masternode tracking
+    void AddUsedMasternode(const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    void RemoveUsedMasternodes(size_t count) EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    size_t GetUsedMasternodesCount() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    bool IsUsedMasternode(const uint256& proTxHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
 };
 
 #endif // BITCOIN_MASTERNODE_META_H
