Merge #6894: refactor: drop mutex and atomic from CMasternodeMetaInfo, access to object directly without shared_ptr

572bafd refactor: rename IsDsqOver to IsMixingThresholdExceeded (Konstantin Akimov)
16915ff fixup: lock annotation for CMasternodeMetaMan::nDsqCount (Konstantin Akimov)
982b68e fix: usage of ResetPlatformBan in CDeterministicMNList (Konstantin Akimov)
f860031 refactoring: apply code review suggestions (Konstantin Akimov)
fe3966d fix: removed leftover annotation, as follow-up conflict resolving with #6880 (Konstantin Akimov)
969b841 refactor: drop return bool from AddGovernanceVote which is always true (Konstantin Akimov)
04ab976 refactor: remove unused CConnMan from meta.h (Konstantin Akimov)
ed27d90 refactor: use GetMetaInfoOrDefault widely, final (Konstantin Akimov)
b79dd90 refactor: use a helper GetMetaInfo and GetMetaInfoOrDefault internally (Konstantin Akimov)
69ed5f5 refactor: remove useless helpers of CMasternodeMetaInfo (Konstantin Akimov)
1bfd6ff refactor: drop mutex and atomics from CMasternodeMetaInfo (Konstantin Akimov)
d5e693f refactor: drop shared_ptr from CMasternodeMetaMan and make GetMetaInfo a private member (Konstantin Akimov)
c0e146f refactor: hide direct usages of GetMetaInfo for net_processing (Konstantin Akimov)
b1a03e6 refactor: hide direct usages of GetMetaInfo from net module (Konstantin Akimov)
a40c418 refactor: hide direct calls of GetMetaInfo from llmq/utils (Konstantin Akimov)
7ba51ec refactor: remove direct usages of GetMetaInfo for dkgsession (Konstantin Akimov)
7ec239b refactor: replace direct usages of GetMetaInfo when platform unban node in mn-list (Konstantin Akimov)
16abbed refactor: replace direct usages of GetMetaInfo while calculating dsq threshold (Konstantin Akimov)
6cb2344 refactor: replace direct call of MetaInfo from rpc/evo code to helper (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Current implementation:
   1. every access to `CMasternodeMetaInfo` is done by accessing `GetMetaInfo` under mutex `CMasternodeMetaMan::cs`
   2. `GetMetaInfo` returns shared_ptr to object
   3. every read or write of `GetMetaInfo` is protected by atomic, second mutex `CMasternodeMetaInfo::cs`; access to object is done indirectly by one more memory hop (because shared_ptr)

  It is not efficient, because `GetMetaInfo` is spread all over code base and not a single case need shared_ptr here; all accesses are brief and short (single read or single write; except RPC where all structure is serialized).

  ## What was done?
  This PR is follow-up for PR #6868 with further improvements and simplification of code.

  Instead returning shared_ptr with `CMasternodeMetaInfo` directly access to members of `CMasternodeMetaMan` under its mutex; all mutexes, atomic and shared_ptr itself are removed from `CMasternodeMetaInfo` accesses.

  It simplified implementation significantly, removed big amount of thread-safety annotations.
  Performance is probably improved, but improvement is unmeasurable, because `CMasternodeMetaInfo` and `CMasternodeMetaMan` has not been spotted as hot spot in profiler neither hot spot in lock contentions logs.

  Also this PR removes duplicated code between coinjoin/client and coinjoin/server by creating a helper in `CMasternodeMetaMan`

  ## 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 572bafd

Tree-SHA512: a9a955a6d2d5d9c18f35bd900a4d2064217974c44277572cdaf10d729cafa4e937b4fc361d64a5132439227fcc7fd3fff98358e20aa7adcbfbbe2a399f9e5052

Author: PastaPastaPasta

src/coinjoin/client.cpp

@@ -124,17 +124,11 @@ MessageProcessingResult CCoinJoinClientQueueManager::ProcessMessage(NodeId from,
             LogPrint(BCLog::COINJOIN, "DSQUEUE -- CoinJoin queue is ready, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());
             return ret;
         } else {
-            int64_t nLastDsq = m_mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-            int64_t nDsqThreshold = m_mn_metaman.GetDsqThreshold(dmn->proTxHash, tip_mn_list.GetValidMNsCount());
-            LogPrint(BCLog::COINJOIN, "DSQUEUE -- nLastDsq: %d  nDsqThreshold: %d  nDsqCount: %d\n", nLastDsq,
-                     nDsqThreshold, m_mn_metaman.GetDsqCount());
-            // don't allow a few nodes to dominate the queuing process
-            if (nLastDsq != 0 && nDsqThreshold > m_mn_metaman.GetDsqCount()) {
+            if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, tip_mn_list.GetValidMNsCount())) {
                 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());
@@ -1180,13 +1174,10 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon
             continue;
         }
 
-        int64_t nLastDsq = m_mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-        int64_t nDsqThreshold = m_mn_metaman.GetDsqThreshold(dmn->proTxHash, nMnCount);
-        if (nLastDsq != 0 && nDsqThreshold > m_mn_metaman.GetDsqCount()) {
+        if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, nMnCount)) {
             WalletCJLogPrint(m_wallet, /* Continued */
-                             "CCoinJoinClientSession::StartNewQueue -- too early to mix with node," /* Continued */
-                             " masternode=%s, nLastDsq=%d, nDsqThreshold=%d, nDsqCount=%d\n",
-                             dmn->proTxHash.ToString(), nLastDsq, nDsqThreshold, m_mn_metaman.GetDsqCount());
+                             "CCoinJoinClientSession::StartNewQueue -- too early to mix with node masternode=%s\n",
+                             dmn->proTxHash.ToString());
             nTries++;
             continue;
         }

src/coinjoin/server.cpp

@@ -99,9 +99,7 @@ void CCoinJoinServer::ProcessDSACCEPT(CNode& peer, CDataStream& vRecv)
             }
         }
 
-        int64_t nLastDsq = m_mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-        int64_t nDsqThreshold = m_mn_metaman.GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount());
-        if (nLastDsq != 0 && nDsqThreshold > m_mn_metaman.GetDsqCount()) {
+        if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, mnList.GetValidMNsCount())) {
             if (fLogIPs) {
                 LogPrint(BCLog::COINJOIN, "DSACCEPT -- last dsq too recent, must wait: peer=%d, addr=%s\n",
                          peer.GetId(), peer.addr.ToStringAddrPort());
@@ -194,11 +192,8 @@ MessageProcessingResult CCoinJoinServer::ProcessDSQUEUE(NodeId from, CDataStream
     }
 
     if (!dsq.fReady) {
-        int64_t nLastDsq = m_mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-        int64_t nDsqThreshold = m_mn_metaman.GetDsqThreshold(dmn->proTxHash, tip_mn_list.GetValidMNsCount());
-        LogPrint(BCLog::COINJOIN, "DSQUEUE -- nLastDsq: %d  nDsqThreshold: %d  nDsqCount: %d\n", nLastDsq, nDsqThreshold, m_mn_metaman.GetDsqCount());
         //don't allow a few nodes to dominate the queuing process
-        if (nLastDsq != 0 && nDsqThreshold > m_mn_metaman.GetDsqCount()) {
+        if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, tip_mn_list.GetValidMNsCount())) {
             LogPrint(BCLog::COINJOIN, "DSQUEUE -- node sending too many dsq messages, masternode=%s\n", dmn->proTxHash.ToString());
             return ret;
         }

src/evo/deterministicmns.cpp

@@ -677,8 +677,7 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, gsl::not_null<co
             const auto opt_proTx = GetTxPayload<CProUpServTx>(tx);
             if (!opt_proTx) continue; // should not happen but does not matter
 
-            if (auto meta_info = m_mn_metaman.GetMetaInfo(opt_proTx->proTxHash, false);
-                !meta_info || !meta_info->SetPlatformBan(false, nHeight)) {
+            if (!m_mn_metaman.ResetPlatformBan(opt_proTx->proTxHash, nHeight)) {
                 LogPrint(BCLog::LLMQ, "%s -- MN %s is failed to Platform revived at height %d\n", __func__,
                          opt_proTx->proTxHash.ToString(), nHeight);
             }

src/evo/mnauth.cpp

@@ -103,7 +103,7 @@ MessageProcessingResult CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_s
     }
 
     if (!peer.IsInboundConn()) {
-        mn_metaman.GetMetaInfo(mnauth.proRegTxHash)->SetLastOutboundSuccess(GetTime<std::chrono::seconds>().count());
+        mn_metaman.SetLastOutboundSuccess(mnauth.proRegTxHash, GetTime<std::chrono::seconds>().count());
         if (peer.m_masternode_probe_connection) {
             LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode probe successful for %s, disconnecting. peer=%d\n",
                      mnauth.proRegTxHash.ToString(), peer.GetId());

src/governance/object.cpp

@@ -147,14 +147,7 @@ bool CGovernanceObject::ProcessVote(CMasternodeMetaMan& mn_metaman, CGovernanceM
         return false;
     }
 
-    if (!mn_metaman.AddGovernanceVote(dmn->proTxHash, vote.GetParentHash())) {
-        std::string msg{strprintf("CGovernanceObject::%s -- Unable to add governance vote, MN outpoint = %s, "
-                                  "governance object hash = %s",
-            __func__, vote.GetMasternodeOutpoint().ToStringShort(), GetHash().ToString())};
-        LogPrint(BCLog::GOBJECT, "%s\n", msg);
-        exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR);
-        return false;
-    }
+    mn_metaman.AddGovernanceVote(dmn->proTxHash, vote.GetParentHash());
 
     voteInstanceRef = vote_instance_t(vote.GetOutcome(), nVoteTimeUpdate, vote.GetTimestamp());
     fileVotes.AddVote(vote);

src/llmq/dkgsession.cpp

@@ -492,12 +492,11 @@ void CDKGSession::VerifyConnectionAndMinProtoVersions(CConnman& connman) const
             m->badConnection = true;
             logger.Batch("%s does not have min proto version %d (has %d)", m->dmn->proTxHash.ToString(), MIN_MASTERNODE_PROTO_VERSION, it->second);
         }
-        const auto meta_info = m_mn_metaman.GetMetaInfo(m->dmn->proTxHash);
-        if (meta_info->OutboundFailedTooManyTimes()) {
+        if (m_mn_metaman.OutboundFailedTooManyTimes(m->dmn->proTxHash)) {
             m->badConnection = true;
             logger.Batch("%s failed to connect to it too many times", m->dmn->proTxHash.ToString());
         }
-        if (meta_info->IsPlatformBanned()) {
+        if (m_mn_metaman.IsPlatformBanned(m->dmn->proTxHash)) {
             m->badConnection = true;
             logger.Batch("%s is Platform PoSe banned", m->dmn->proTxHash.ToString());
         }

src/llmq/utils.cpp

@@ -912,7 +912,7 @@ void AddQuorumProbeConnections(const Consensus::LLMQParams& llmqParams, CConnman
         if (dmn->proTxHash == myProTxHash) {
             continue;
         }
-        auto lastOutbound = mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastOutboundSuccess();
+        auto lastOutbound = mn_metaman.GetLastOutboundSuccess(dmn->proTxHash);
         if (curTime - lastOutbound < 10 * 60) {
             // avoid re-probing nodes too often
             continue;

src/masternode/meta.cpp

@@ -10,6 +10,13 @@
 
 const std::string MasternodeMetaStore::SERIALIZATION_VERSION_STRING = "CMasternodeMetaMan-Version-5";
 
+static constexpr int MASTERNODE_MAX_FAILED_OUTBOUND_ATTEMPTS{5};
+static constexpr int MASTERNODE_MAX_MIXING_TXES{5};
+
+namespace {
+static const CMasternodeMetaInfo default_meta_info{};
+} // anonymous namespace
+
 CMasternodeMetaMan::CMasternodeMetaMan() :
     m_db{std::make_unique<db_type>("mncache.dat", "magicMasternodeCache")}
 {
@@ -30,29 +37,24 @@ bool CMasternodeMetaMan::LoadCache(bool load_cache)
 
 UniValue CMasternodeMetaInfo::ToJson() const
 {
-    UniValue ret(UniValue::VOBJ);
-
     int64_t now = GetTime<std::chrono::seconds>().count();
 
-    ret.pushKV("lastDSQ", nLastDsq.load());
-    ret.pushKV("mixingTxCount", nMixingTxCount.load());
-    ret.pushKV("outboundAttemptCount", outboundAttemptCount.load());
-    ret.pushKV("lastOutboundAttempt", lastOutboundAttempt.load());
-    ret.pushKV("lastOutboundAttemptElapsed", now - lastOutboundAttempt.load());
-    ret.pushKV("lastOutboundSuccess", lastOutboundSuccess.load());
-    ret.pushKV("lastOutboundSuccessElapsed", now - lastOutboundSuccess.load());
-    {
-        LOCK(cs);
-        ret.pushKV("is_platform_banned", m_platform_ban);
-        ret.pushKV("platform_ban_height_updated", m_platform_ban_updated);
-    }
+    UniValue ret(UniValue::VOBJ);
+    ret.pushKV("lastDSQ", m_last_dsq);
+    ret.pushKV("mixingTxCount", m_mixing_tx_count);
+    ret.pushKV("outboundAttemptCount", outboundAttemptCount);
+    ret.pushKV("lastOutboundAttempt", lastOutboundAttempt);
+    ret.pushKV("lastOutboundAttemptElapsed", now - lastOutboundAttempt);
+    ret.pushKV("lastOutboundSuccess", lastOutboundSuccess);
+    ret.pushKV("lastOutboundSuccessElapsed", now - lastOutboundSuccess);
+    ret.pushKV("is_platform_banned", m_platform_ban);
+    ret.pushKV("platform_ban_height_updated", m_platform_ban_updated);
 
     return ret;
 }
 
 void CMasternodeMetaInfo::AddGovernanceVote(const uint256& nGovernanceObjectHash)
 {
-    LOCK(cs);
     // Insert a zero value, or not. Then increment the value regardless. This
     // ensures the value is in the map.
     const auto& pair = mapGovernanceObjectsVotedOn.emplace(nGovernanceObjectHash, 0);
@@ -61,65 +63,81 @@ void CMasternodeMetaInfo::AddGovernanceVote(const uint256& nGovernanceObjectHash
 
 void CMasternodeMetaInfo::RemoveGovernanceObject(const uint256& nGovernanceObjectHash)
 {
-    LOCK(cs);
     // Whether or not the govobj hash exists in the map first is irrelevant.
     mapGovernanceObjectsVotedOn.erase(nGovernanceObjectHash);
 }
 
-CMasternodeMetaInfoPtr CMasternodeMetaMan::GetMetaInfo(const uint256& proTxHash, bool fCreate)
+const CMasternodeMetaInfo& CMasternodeMetaMan::GetMetaInfoOrDefault(const uint256& protx_hash) const
+{
+    const auto it = metaInfos.find(protx_hash);
+    if (it == metaInfos.end()) return default_meta_info;
+    return it->second;
+}
+
+CMasternodeMetaInfo CMasternodeMetaMan::GetInfo(const uint256& proTxHash) const
 {
     LOCK(cs);
+    return GetMetaInfoOrDefault(proTxHash);
+}
+
+CMasternodeMetaInfo& CMasternodeMetaMan::GetMetaInfo(const uint256& proTxHash)
+{
     auto it = metaInfos.find(proTxHash);
     if (it != metaInfos.end()) {
         return it->second;
     }
-    if (!fCreate) {
-        return nullptr;
-    }
-    it = metaInfos.emplace(proTxHash, std::make_shared<CMasternodeMetaInfo>(proTxHash)).first;
+    it = metaInfos.emplace(proTxHash, CMasternodeMetaInfo{proTxHash}).first;
     return it->second;
 }
 
-// We keep track of dsq (mixing queues) count to avoid using same masternodes for mixing too often.
-// This threshold is calculated as the last dsq count this specific masternode was used in a mixing
-// session plus a margin of 20% of masternode count. In other words we expect at least 20% of unique
-// masternodes before we ever see a masternode that we know already mixed someone's funds earlier.
-int64_t CMasternodeMetaMan::GetDsqThreshold(const uint256& proTxHash, int nMnCount)
+bool CMasternodeMetaMan::IsMixingThresholdExceeded(const uint256& protx_hash, int mn_count) const
 {
-    auto metaInfo = GetMetaInfo(proTxHash);
-    if (metaInfo == nullptr) {
-        // return a threshold which is slightly above nDsqCount i.e. a no-go
-        return nDsqCount + 1;
+    LOCK(cs);
+    auto it = metaInfos.find(protx_hash);
+    if (it == metaInfos.end()) {
+        LogPrint(BCLog::COINJOIN, "DSQUEUE -- node %s is logged\n", protx_hash.ToString());
+        return false;
     }
-    return metaInfo->GetLastDsq() + nMnCount / 5;
+    const auto& meta_info = it->second;
+    int64_t last_dsq = meta_info.m_last_dsq;
+    int64_t threshold = last_dsq + mn_count / 5;
+
+    LogPrint(BCLog::COINJOIN, "DSQUEUE -- mn: %s last_dsq: %d  dsq_threshold: %d  nDsqCount: %d\n",
+             protx_hash.ToString(), last_dsq, threshold, nDsqCount);
+    return last_dsq != 0 && threshold > nDsqCount;
 }
 
 void CMasternodeMetaMan::AllowMixing(const uint256& proTxHash)
 {
-    auto mm = GetMetaInfo(proTxHash);
-    nDsqCount++;
-    mm->nLastDsq = nDsqCount.load();
-    mm->nMixingTxCount = 0;
+    LOCK(cs);
+    auto& mm = GetMetaInfo(proTxHash);
+    mm.m_last_dsq = ++nDsqCount;
+    mm.m_mixing_tx_count = 0;
 }
 
 void CMasternodeMetaMan::DisallowMixing(const uint256& proTxHash)
 {
-    auto mm = GetMetaInfo(proTxHash);
-    mm->nMixingTxCount++;
+    LOCK(cs);
+    GetMetaInfo(proTxHash).m_mixing_tx_count++;
 }
 
-bool CMasternodeMetaMan::AddGovernanceVote(const uint256& proTxHash, const uint256& nGovernanceObjectHash)
+bool CMasternodeMetaMan::IsValidForMixingTxes(const uint256& protx_hash) const
 {
-    auto mm = GetMetaInfo(proTxHash);
-    mm->AddGovernanceVote(nGovernanceObjectHash);
-    return true;
+    LOCK(cs);
+    return GetMetaInfoOrDefault(protx_hash).m_mixing_tx_count <= MASTERNODE_MAX_MIXING_TXES;
+}
+
+void CMasternodeMetaMan::AddGovernanceVote(const uint256& proTxHash, const uint256& nGovernanceObjectHash)
+{
+    LOCK(cs);
+    GetMetaInfo(proTxHash).AddGovernanceVote(nGovernanceObjectHash);
 }
 
 void CMasternodeMetaMan::RemoveGovernanceObject(const uint256& nGovernanceObjectHash)
 {
     LOCK(cs);
-    for(const auto& p : metaInfos) {
-        p.second->RemoveGovernanceObject(nGovernanceObjectHash);
+    for (auto& [_, meta_info] : metaInfos) {
+        meta_info.RemoveGovernanceObject(nGovernanceObjectHash);
     }
 }
 
@@ -130,6 +148,65 @@ std::vector<uint256> CMasternodeMetaMan::GetAndClearDirtyGovernanceObjectHashes(
     return vecTmp;
 }
 
+void CMasternodeMetaMan::SetLastOutboundAttempt(const uint256& protx_hash, int64_t t)
+{
+    LOCK(cs);
+    GetMetaInfo(protx_hash).SetLastOutboundAttempt(t);
+}
+
+void CMasternodeMetaMan::SetLastOutboundSuccess(const uint256& protx_hash, int64_t t)
+{
+    LOCK(cs);
+    GetMetaInfo(protx_hash).SetLastOutboundSuccess(t);
+}
+
+int64_t CMasternodeMetaMan::GetLastOutboundAttempt(const uint256& protx_hash) const
+{
+    LOCK(cs);
+    return GetMetaInfoOrDefault(protx_hash).lastOutboundAttempt;
+}
+
+int64_t CMasternodeMetaMan::GetLastOutboundSuccess(const uint256& protx_hash) const
+{
+    LOCK(cs);
+    return GetMetaInfoOrDefault(protx_hash).lastOutboundSuccess;
+}
+
+bool CMasternodeMetaMan::OutboundFailedTooManyTimes(const uint256& protx_hash) const
+{
+    LOCK(cs);
+    return GetMetaInfoOrDefault(protx_hash).outboundAttemptCount > MASTERNODE_MAX_FAILED_OUTBOUND_ATTEMPTS;
+}
+
+bool CMasternodeMetaMan::IsPlatformBanned(const uint256& protx_hash) const
+{
+    LOCK(cs);
+    return GetMetaInfoOrDefault(protx_hash).m_platform_ban;
+}
+
+bool CMasternodeMetaMan::ResetPlatformBan(const uint256& protx_hash, int height)
+{
+    LOCK(cs);
+
+    auto it = metaInfos.find(protx_hash);
+    if (it == metaInfos.end()) return false;
+
+    return it->second.SetPlatformBan(false, height);
+}
+
+bool CMasternodeMetaMan::SetPlatformBan(const uint256& inv_hash, PlatformBanMessage&& ban_msg)
+{
+    LOCK(cs);
+
+    const uint256& protx_hash = ban_msg.m_protx_hash;
+
+    bool ret = GetMetaInfo(protx_hash).SetPlatformBan(true, ban_msg.m_requested_height);
+    if (ret) {
+        m_seen_platform_bans.insert(inv_hash, std::move(ban_msg));
+    }
+    return ret;
+}
+
 bool CMasternodeMetaMan::AlreadyHavePlatformBan(const uint256& inv_hash) const
 {
     LOCK(cs);
@@ -147,12 +224,6 @@ std::optional<PlatformBanMessage> CMasternodeMetaMan::GetPlatformBan(const uint2
     return ret;
 }
 
-void CMasternodeMetaMan::RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg)
-{
-    LOCK(cs);
-    m_seen_platform_bans.insert(inv_hash, std::move(msg));
-}
-
 void CMasternodeMetaMan::AddUsedMasternode(const uint256& proTxHash)
 {
     LOCK(cs);