Merge #6956: refactor: use a vector; introduce instantsend::PendingISLockFromPeer

1b2bab4 chore: whitespace fix (pasta)
2980e60 perf: use vector instead of hash map for ProcessPendingInstantSendLocks (pasta)
5c6e98e refactor: replace pair with structured PendingISLockFromPeer in CInstantSendManager (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  We don't use the hash map properties, so use a vector, since we are copying everything anyway.

  Also add instantsend::PendingISLockFromPeer for improved readability

  ## What was done?

  ## How Has This Been Tested?
  Builds

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] 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 _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    utACK 1b2bab4
  UdjinM6:
    utACK 1b2bab4

Tree-SHA512: 369bdfc1d91e104402927a126477ab71a27331b56cfadef1f88279acc6f86a5a2eb592d43df1a2c33f0199072ecbf162010908088af9d4fdd6acb2398e60fd84

Author: PastaPastaPasta

src/instantsend/instantsend.cpp

@@ -168,13 +168,13 @@ MessageProcessingResult CInstantSendManager::ProcessMessage(NodeId from, std::st
     }
 
     LOCK(cs_pendingLocks);
-    pendingInstantSendLocks.emplace(hash, std::make_pair(from, islock));
+    pendingInstantSendLocks.emplace(hash, instantsend::PendingISLockFromPeer{from, islock});
     return ret;
 }
 
 instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks()
 {
-    decltype(pendingInstantSendLocks) pend;
+    std::vector<std::pair<uint256, instantsend::PendingISLockFromPeer>> pend;
     instantsend::PendingState ret;
 
     if (!IsInstantSendEnabled()) {
@@ -189,14 +189,15 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks()
         // The keys of the removed values are temporaily stored here to avoid invalidating an iterator
         std::vector<uint256> removed;
         removed.reserve(maxCount);
+        pend.reserve(maxCount);
 
         for (const auto& [islockHash, nodeid_islptr_pair] : pendingInstantSendLocks) {
             // Check if we've reached max count
             if (pend.size() >= maxCount) {
                 ret.m_pending_work = true;
                 break;
             }
-            pend.emplace(islockHash, std::move(nodeid_islptr_pair));
+            pend.emplace_back(islockHash, std::move(nodeid_islptr_pair));
             removed.emplace_back(islockHash);
         }
 
@@ -222,24 +223,25 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks()
     if (!badISLocks.empty()) {
         LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- doing verification on old active set\n", __func__);
 
-        // filter out valid IS locks from "pend"
-        for (auto it = pend.begin(); it != pend.end();) {
-            if (!badISLocks.count(it->first)) {
-                it = pend.erase(it);
-            } else {
-                ++it;
+        // filter out valid IS locks from "pend" - keep only bad ones
+        std::vector<std::pair<uint256, instantsend::PendingISLockFromPeer>> filteredPend;
+        filteredPend.reserve(badISLocks.size());
+        for (auto& p : pend) {
+            if (badISLocks.contains(p.first)) {
+                filteredPend.push_back(std::move(p));
             }
         }
+
         // Now check against the previous active set and perform banning if this fails
-        ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, pend, ret.m_peer_activity);
+        ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, filteredPend, ret.m_peer_activity);
     }
 
     return ret;
 }
 
 Uint256HashSet CInstantSendManager::ProcessPendingInstantSendLocks(
     const Consensus::LLMQParams& llmq_params, int signOffset, bool ban,
-    const Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>& pend,
+    const std::vector<std::pair<uint256, instantsend::PendingISLockFromPeer>>& pend,
     std::vector<std::pair<NodeId, MessageProcessingResult>>& peer_activity)
 {
     CBLSBatchVerifier<NodeId, uint256> batchVerifier(false, true, 8);
@@ -249,8 +251,8 @@ Uint256HashSet CInstantSendManager::ProcessPendingInstantSendLocks(
     size_t alreadyVerified = 0;
     for (const auto& p : pend) {
         const auto& hash = p.first;
-        auto nodeId = p.second.first;
-        const auto& islock = p.second.second;
+        auto nodeId = p.second.node_id;
+        const auto& islock = p.second.islock;
 
         if (batchVerifier.badSources.count(nodeId)) {
             continue;
@@ -321,8 +323,8 @@ Uint256HashSet CInstantSendManager::ProcessPendingInstantSendLocks(
     }
     for (const auto& p : pend) {
         const auto& hash = p.first;
-        auto nodeId = p.second.first;
-        const auto& islock = p.second.second;
+        auto nodeId = p.second.node_id;
+        const auto& islock = p.second.islock;
 
         if (batchVerifier.badMessages.count(hash)) {
             LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: invalid sig in islock, peer=%d\n",
@@ -399,7 +401,7 @@ MessageProcessingResult CInstantSendManager::ProcessInstantSendLock(NodeId from,
     } else {
         // put it in a separate pending map and try again later
         LOCK(cs_pendingLocks);
-        pendingNoTxInstantSendLocks.try_emplace(hash, std::make_pair(from, islock));
+        pendingNoTxInstantSendLocks.try_emplace(hash, instantsend::PendingISLockFromPeer{from, islock});
     }
 
     // This will also add children TXs to pendingRetryTxs
@@ -442,11 +444,11 @@ void CInstantSendManager::TransactionAddedToMempool(const CTransactionRef& tx)
         LOCK(cs_pendingLocks);
         auto it = pendingNoTxInstantSendLocks.begin();
         while (it != pendingNoTxInstantSendLocks.end()) {
-            if (it->second.second->txid == tx->GetHash()) {
+            if (it->second.islock->txid == tx->GetHash()) {
                 // we received an islock earlier
                 LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__,
                          tx->GetHash().ToString(), it->first.ToString());
-                islock = it->second.second;
+                islock = it->second.islock;
                 pendingInstantSendLocks.try_emplace(it->first, it->second);
                 pendingNoTxInstantSendLocks.erase(it);
                 break;
@@ -539,7 +541,7 @@ void CInstantSendManager::AddNonLockedTx(const CTransactionRef& tx, const CBlock
         LOCK(cs_pendingLocks);
         auto it = pendingNoTxInstantSendLocks.begin();
         while (it != pendingNoTxInstantSendLocks.end()) {
-            if (it->second.second->txid == tx->GetHash()) {
+            if (it->second.islock->txid == tx->GetHash()) {
                 // we received an islock earlier, let's put it back into pending and verify/lock
                 LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__,
                          tx->GetHash().ToString(), it->first.ToString());
@@ -626,7 +628,7 @@ void CInstantSendManager::TryEmplacePendingLock(const uint256& hash, const NodeI
     if (db.KnownInstantSendLock(hash)) return;
     LOCK(cs_pendingLocks);
     if (!pendingInstantSendLocks.count(hash)) {
-        pendingInstantSendLocks.emplace(hash, std::make_pair(id, islock));
+        pendingInstantSendLocks.emplace(hash, instantsend::PendingISLockFromPeer{id, islock});
     }
 }
 
@@ -848,11 +850,11 @@ bool CInstantSendManager::GetInstantSendLockByHash(const uint256& hash, instants
         LOCK(cs_pendingLocks);
         auto it = pendingInstantSendLocks.find(hash);
         if (it != pendingInstantSendLocks.end()) {
-            islock = it->second.second;
+            islock = it->second.islock;
         } else {
             auto itNoTx = pendingNoTxInstantSendLocks.find(hash);
             if (itNoTx != pendingNoTxInstantSendLocks.end()) {
-                islock = itNoTx->second.second;
+                islock = itNoTx->second.islock;
             } else {
                 return false;
             }
@@ -889,7 +891,7 @@ bool CInstantSendManager::IsWaitingForTx(const uint256& txHash) const
     LOCK(cs_pendingLocks);
     auto it = pendingNoTxInstantSendLocks.begin();
     while (it != pendingNoTxInstantSendLocks.end()) {
-        if (it->second.second->txid == txHash) {
+        if (it->second.islock->txid == txHash) {
             LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__, txHash.ToString(),
                      it->first.ToString());
             return true;

src/instantsend/instantsend.h

@@ -20,6 +20,7 @@
 #include <atomic>
 #include <unordered_map>
 #include <unordered_set>
+#include <vector>
 
 class CBlockIndex;
 class CChainState;
@@ -38,6 +39,11 @@ struct DbWrapperParams;
 namespace instantsend {
 class InstantSendSigner;
 
+struct PendingISLockFromPeer {
+    NodeId node_id;
+    InstantSendLockPtr islock;
+};
+
 struct PendingState {
     bool m_pending_work{false};
     std::vector<std::pair<NodeId, MessageProcessingResult>> m_peer_activity{};
@@ -69,9 +75,9 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent
 
     mutable Mutex cs_pendingLocks;
     // Incoming and not verified yet
-    Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>> pendingInstantSendLocks GUARDED_BY(cs_pendingLocks);
+    Uint256HashMap<instantsend::PendingISLockFromPeer> pendingInstantSendLocks GUARDED_BY(cs_pendingLocks);
     // Tried to verify but there is no tx yet
-    Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>> pendingNoTxInstantSendLocks GUARDED_BY(cs_pendingLocks);
+    Uint256HashMap<instantsend::PendingISLockFromPeer> pendingNoTxInstantSendLocks GUARDED_BY(cs_pendingLocks);
 
     // TXs which are neither IS locked nor ChainLocked. We use this to determine for which TXs we need to retry IS
     // locking of child TXs
@@ -117,7 +123,7 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent
         EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry);
 
     Uint256HashSet ProcessPendingInstantSendLocks(const Consensus::LLMQParams& llmq_params, int signOffset, bool ban,
-                                                  const Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>& pend,
+                                                  const std::vector<std::pair<uint256, instantsend::PendingISLockFromPeer>>& pend,
                                                   std::vector<std::pair<NodeId, MessageProcessingResult>>& peer_activity)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry);
     MessageProcessingResult ProcessInstantSendLock(NodeId from, const uint256& hash,