Merge dashpay#6004: backport: merge bitcoin#21167, bitcoin#22782, bitcoin#21943, bitcoin#22829, bitcoin#24079, bitcoin#24108, bitcoin#24157, bitcoin#25109 (network backports: part 5)

5dde8e7 merge bitcoin#25109: Strengthen AssertLockNotHeld assertions (Kittywhiskers Van Gogh)
a1f005e merge bitcoin#24157: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it (Kittywhiskers Van Gogh)
de4b4bf merge bitcoin#24108: Replace RecursiveMutex cs_addrLocal with Mutex, and rename it (Kittywhiskers Van Gogh)
2f7a138 merge bitcoin#24079: replace RecursiveMutex cs_SubVer with Mutex (and rename) (Kittywhiskers Van Gogh)
23b152c merge bitcoin#22829: various RecursiveMutex replacements in CConnman (Kittywhiskers Van Gogh)
362e310 merge bitcoin#21943: Dedup and RAII-fy the creation of a copy of CConnman::vNodes (Kittywhiskers Van Gogh)
bf98ad6 merge bitcoin#22782: Remove unused MaybeSetAddrName (Kittywhiskers Van Gogh)
2b65526 merge bitcoin#21167: make CNode::m_inbound_onion public, initialize explicitly (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on dashpay#6001
  * Dependency for dashpay#6018
  * Partially reverts ff69e0d from dashpay#5336 due to `Span<CNode*>`'s incompatibility with `CConnman::NodesSnapshot::Snap()` (returning `const std::vector<CNode*>&`)

    ```
    masternode/sync.cpp:147:18: error: no matching member function for call to 'RequestGovernanceObjectVotes'
            m_govman.RequestGovernanceObjectVotes(snap.Nodes(), connman);
            ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ./governance/governance.h:360:9: note: candidate function not viable: no known conversion from 'const
    std::vector<CNode *>' to 'CNode &' for 1st argument
        int RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const;
          ^
    ./governance/governance.h:361:9: note: candidate function not viable: no known conversion from 'const std::vector<CNode *>' to 'Span<CNode *>' for 1st argument
        int RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const;
          ^
    1 error generated.
    ```
  * Dash already implements its own `CNode*` iteration logic in [dash#1382](dashpay#1382) and implemented additional capabilities in [dash#1575](dashpay#1575), which meant backporting [bitcoin#21943](bitcoin#21943) involved migrating Dash-specific code to upstream logic that needed to be modified to implement expected functionality.

  * Unlike Bitcoin, Dash maintains a map of every raw `SOCKET` corresponding to a pointer of their `CNode` instance and uses it to translate socket sets to their corresponding `CNode*` sets. This is done to accommodate for edge-triggered modes which have an event-socket relationship, as opposed to level-triggered modes, which have a socket-event relationship.

    This means that `CConnman::SocketHandlerConnected()` doesn't require access to a vector of all `CNode` pointers and therefore, the argument `nodes` has been omitted.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 5dde8e7

Tree-SHA512: 5685d8ebb4fa1f10d018e60d9b0efc3100ea13ac437e7892a09ad3f86d6ac6756e4b5a08ebe70de2eabb27740678e10b975d319f2d553ae5b27dafa71dba0a9f

Author: PastaPastaPasta

src/checkqueue.h

@@ -65,7 +65,7 @@ class CCheckQueue
     bool m_request_stop GUARDED_BY(m_mutex){false};
 
     /** Internal function that does bulk of the verification work. */
-    bool Loop(bool fMaster)
+    bool Loop(bool fMaster) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         std::condition_variable& cond = fMaster ? m_master_cv : m_worker_cv;
         std::vector<T> vChecks;
@@ -139,7 +139,7 @@ class CCheckQueue
     }
 
     //! Create a pool of new worker threads.
-    void StartWorkerThreads(const int threads_num)
+    void StartWorkerThreads(const int threads_num) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         {
             LOCK(m_mutex);
@@ -157,13 +157,13 @@ class CCheckQueue
     }
 
     //! Wait until execution finishes, and return whether all evaluations were successful.
-    bool Wait()
+    bool Wait() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         return Loop(true /* master thread */);
     }
 
     //! Add a batch of checks to the queue
-    void Add(std::vector<T>& vChecks)
+    void Add(std::vector<T>& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         if (vChecks.empty()) {
             return;
@@ -186,7 +186,7 @@ class CCheckQueue
     }
 
     //! Stop all of the worker threads.
-    void StopWorkerThreads()
+    void StopWorkerThreads() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         WITH_LOCK(m_mutex, m_request_stop = true);
         m_worker_cv.notify_all();

src/coinjoin/coinjoin.h

@@ -368,15 +368,22 @@ class CDSTXManager
     void AddDSTX(const CCoinJoinBroadcastTx& dstx) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
     CCoinJoinBroadcastTx GetDSTX(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
 
-    void UpdatedBlockTip(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler, const CMasternodeSync& mn_sync);
-    void NotifyChainLock(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler, const CMasternodeSync& mn_sync);
+    void UpdatedBlockTip(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler,
+                         const CMasternodeSync& mn_sync)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
+    void NotifyChainLock(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler,
+                         const CMasternodeSync& mn_sync)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
 
     void TransactionAddedToMempool(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
-    void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
-    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex*) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
+    void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
+    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex*)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
 
 private:
-    void CheckDSTXes(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler);
+    void CheckDSTXes(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
     void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, std::optional<int> nHeight);
 
 };

src/governance/governance.cpp

@@ -1223,11 +1223,11 @@ void CGovernanceManager::RequestGovernanceObject(CNode* pfrom, const uint256& nH
 
 int CGovernanceManager::RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const
 {
-    std::array<CNode*, 1> nodeCopy{&peer};
-    return RequestGovernanceObjectVotes(nodeCopy, connman);
+    const std::vector<CNode*> vNodeCopy{&peer};
+    return RequestGovernanceObjectVotes(vNodeCopy, connman);
 }
 
-int CGovernanceManager::RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const
+int CGovernanceManager::RequestGovernanceObjectVotes(const std::vector<CNode*>& vNodesCopy, CConnman& connman) const
 {
     static std::map<uint256, std::map<CService, int64_t> > mapAskedRecently;
 
@@ -1501,7 +1501,7 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co
 
 void CGovernanceManager::RequestOrphanObjects(CConnman& connman)
 {
-    std::vector<CNode*> vNodesCopy = connman.CopyNodeVector(CConnman::FullyConnectedOnly);
+    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
 
     std::vector<uint256> vecHashesFiltered;
     {
@@ -1517,15 +1517,13 @@ void CGovernanceManager::RequestOrphanObjects(CConnman& connman)
 
     LogPrint(BCLog::GOBJECT, "CGovernanceObject::RequestOrphanObjects -- number objects = %d\n", vecHashesFiltered.size());
     for (const uint256& nHash : vecHashesFiltered) {
-        for (CNode* pnode : vNodesCopy) {
+        for (CNode* pnode : snap.Nodes()) {
             if (!pnode->CanRelay()) {
                 continue;
             }
             RequestGovernanceObject(pnode, nHash, connman);
         }
     }
-
-    connman.ReleaseNodeVector(vNodesCopy);
 }
 
 void CGovernanceManager::CleanOrphanObjects()

src/governance/governance.h

@@ -358,7 +358,7 @@ class CGovernanceManager : public GovernanceStore
     void InitOnLoad();
 
     int RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const;
-    int RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const;
+    int RequestGovernanceObjectVotes(const std::vector<CNode*>& vNodesCopy, CConnman& connman) const;
 
     /*
      * Trigger Management (formerly CGovernanceTriggerManager)

src/httpserver.cpp

@@ -83,7 +83,7 @@ class WorkQueue
     {
     }
     /** Enqueue a work item */
-    bool Enqueue(WorkItem* item)
+    bool Enqueue(WorkItem* item) EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         if (!running || queue.size() >= maxDepth) {
@@ -94,7 +94,7 @@ class WorkQueue
         return true;
     }
     /** Thread function */
-    void Run()
+    void Run() EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         while (true) {
             std::unique_ptr<WorkItem> i;
@@ -111,7 +111,7 @@ class WorkQueue
         }
     }
     /** Interrupt and exit loops */
-    void Interrupt()
+    void Interrupt() EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         running = false;

src/i2p.h

@@ -84,7 +84,7 @@ class Session
      * to the listening socket and address.
      * @return true on success
      */
-    bool Listen(Connection& conn);
+    bool Listen(Connection& conn) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /**
      * Wait for and accept a new incoming connection.
@@ -103,7 +103,7 @@ class Session
      * it is set to `false`. Only set if `false` is returned.
      * @return true on success
      */
-    bool Connect(const CService& to, Connection& conn, bool& proxy_error);
+    bool Connect(const CService& to, Connection& conn, bool& proxy_error) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
 private:
     /**
@@ -172,7 +172,7 @@ class Session
     /**
      * Check the control socket for errors and possibly disconnect.
      */
-    void CheckControlSock();
+    void CheckControlSock() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /**
      * Generate a new destination with the SAM proxy and set `m_private_key` to it.

src/index/blockfilterindex.h

@@ -63,7 +63,7 @@ class BlockFilterIndex final : public BaseIndex
     bool LookupFilter(const CBlockIndex* block_index, BlockFilter& filter_out) const;
 
     /** Get a single filter header by block. */
-    bool LookupFilterHeader(const CBlockIndex* block_index, uint256& header_out);
+    bool LookupFilterHeader(const CBlockIndex* block_index, uint256& header_out) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_headers_cache);
 
     /** Get a range of filters between two heights on a chain. */
     bool LookupFilterRange(int start_height, const CBlockIndex* stop_index,

src/llmq/dkgsession.cpp

@@ -1336,7 +1336,7 @@ void CDKGSession::RelayInvToParticipants(const CInv& inv) const
         if (pnode->GetVerifiedProRegTxHash().IsNull()) {
             logger.Batch("node[%d:%s] not mn",
                          pnode->GetId(),
-                         pnode->GetAddrName());
+                         pnode->m_addr_name);
         } else if (relayMembers.count(pnode->GetVerifiedProRegTxHash()) == 0) {
             ss2 << pnode->GetVerifiedProRegTxHash().ToString().substr(0, 4) << " | ";
         }

src/llmq/signing_shares.cpp

@@ -1092,14 +1092,13 @@ bool CSigSharesManager::SendMessages()
         return session->sendSessionId;
     };
 
-    std::vector<CNode*> vNodesCopy = connman.CopyNodeVector(CConnman::FullyConnectedOnly);
-
+    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
     {
         LOCK(cs);
         CollectSigSharesToRequest(sigSharesToRequest);
         CollectSigSharesToSend(sigShareBatchesToSend);
         CollectSigSharesToAnnounce(sigSharesToAnnounce);
-        CollectSigSharesToSendConcentrated(sigSharesToSend, vNodesCopy);
+        CollectSigSharesToSendConcentrated(sigSharesToSend, snap.Nodes());
 
         for (auto& [nodeId, sigShareMap] : sigSharesToRequest) {
             for (auto& [hash, sigShareInv] : sigShareMap) {
@@ -1120,7 +1119,7 @@ bool CSigSharesManager::SendMessages()
 
     bool didSend = false;
 
-    for (auto& pnode : vNodesCopy) {
+    for (auto& pnode : snap.Nodes()) {
         CNetMsgMaker msgMaker(pnode->GetCommonVersion());
 
         if (const auto it1 = sigSessionAnnouncements.find(pnode->GetId()); it1 != sigSessionAnnouncements.end()) {
@@ -1222,9 +1221,6 @@ bool CSigSharesManager::SendMessages()
         }
     }
 
-    // looped through all nodes, release them
-    connman.ReleaseNodeVector(vNodesCopy);
-
     return didSend;
 }
 

src/masternode/sync.cpp

@@ -140,12 +140,11 @@ void CMasternodeSync::ProcessTick()
     }
 
     nTimeLastProcess = GetTime();
-    std::vector<CNode*> vNodesCopy = connman.CopyNodeVector(CConnman::FullyConnectedOnly);
+    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
 
     // gradually request the rest of the votes after sync finished
     if(IsSynced()) {
-        m_govman.RequestGovernanceObjectVotes(vNodesCopy, connman);
-        connman.ReleaseNodeVector(vNodesCopy);
+        m_govman.RequestGovernanceObjectVotes(snap.Nodes(), connman);
         return;
     }
 
@@ -154,7 +153,7 @@ void CMasternodeSync::ProcessTick()
     LogPrint(BCLog::MNSYNC, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d nTriedPeerCount %d nSyncProgress %f\n", nTick, nCurrentAsset, nTriedPeerCount, nSyncProgress);
     uiInterface.NotifyAdditionalDataSyncProgressChanged(nSyncProgress);
 
-    for (auto& pnode : vNodesCopy)
+    for (auto& pnode : snap.Nodes())
     {
         CNetMsgMaker msgMaker(pnode->GetCommonVersion());
 
@@ -189,7 +188,7 @@ void CMasternodeSync::ProcessTick()
             }
 
             if (nCurrentAsset == MASTERNODE_SYNC_BLOCKCHAIN) {
-                int64_t nTimeSyncTimeout = vNodesCopy.size() > 3 ? MASTERNODE_SYNC_TICK_SECONDS : MASTERNODE_SYNC_TIMEOUT_SECONDS;
+                int64_t nTimeSyncTimeout = snap.Nodes().size() > 3 ? MASTERNODE_SYNC_TICK_SECONDS : MASTERNODE_SYNC_TIMEOUT_SECONDS;
                 if (fReachedBestHeader && (GetTime() - nTimeLastBumped > nTimeSyncTimeout)) {
                     // At this point we know that:
                     // a) there are peers (because we are looping on at least one of them);
@@ -205,7 +204,7 @@ void CMasternodeSync::ProcessTick()
 
                     if (gArgs.GetBoolArg("-syncmempool", DEFAULT_SYNC_MEMPOOL)) {
                         // Now that the blockchain is synced request the mempool from the connected outbound nodes if possible
-                        for (auto pNodeTmp : vNodesCopy) {
+                        for (auto pNodeTmp : snap.Nodes()) {
                             bool fRequestedEarlier = m_netfulfilledman.HasFulfilledRequest(pNodeTmp->addr, "mempool-sync");
                             if (pNodeTmp->nVersion >= 70216 && !pNodeTmp->IsInboundConn() && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) {
                                 m_netfulfilledman.AddFulfilledRequest(pNodeTmp->addr, "mempool-sync");
@@ -222,7 +221,6 @@ void CMasternodeSync::ProcessTick()
             if(nCurrentAsset == MASTERNODE_SYNC_GOVERNANCE) {
                 if (!m_govman.IsValid()) {
                     SwitchToNextAsset();
-                    connman.ReleaseNodeVector(vNodesCopy);
                     return;
                 }
                 LogPrint(BCLog::GOBJECT, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d nTimeLastBumped %lld GetTime() %lld diff %lld\n", nTick, nCurrentAsset, nTimeLastBumped, GetTime(), GetTime() - nTimeLastBumped);
@@ -235,7 +233,6 @@ void CMasternodeSync::ProcessTick()
                         // it's kind of ok to skip this for now, hopefully we'll catch up later?
                     }
                     SwitchToNextAsset();
-                    connman.ReleaseNodeVector(vNodesCopy);
                     return;
                 }
 
@@ -259,12 +256,11 @@ void CMasternodeSync::ProcessTick()
 
     if (nCurrentAsset != MASTERNODE_SYNC_GOVERNANCE) {
         // looped through all nodes and not syncing governance yet/already, release them
-        connman.ReleaseNodeVector(vNodesCopy);
         return;
     }
 
     // request votes on per-obj basis from each node
-    for (const auto& pnode : vNodesCopy) {
+    for (const auto& pnode : snap.Nodes()) {
         if(!m_netfulfilledman.HasFulfilledRequest(pnode->addr, "governance-sync")) {
             continue; // to early for this node
         }
@@ -291,16 +287,12 @@ void CMasternodeSync::ProcessTick()
                 // reset nTimeNoObjectsLeft to be able to use the same condition on resync
                 nTimeNoObjectsLeft = 0;
                 SwitchToNextAsset();
-                connman.ReleaseNodeVector(vNodesCopy);
                 return;
             }
             nLastTick = nTick;
             nLastVotes = m_govman.GetVoteCount();
         }
     }
-
-    // looped through all nodes, release them
-    connman.ReleaseNodeVector(vNodesCopy);
 }
 
 void CMasternodeSync::SendGovernanceSyncRequest(CNode* pnode) const