Merge #6972: fix: move more_work calculation into CollectPendingRecoveredSigsToVerify replacing incorrect logic in ProcessPendingRecoveredSigs

3b948d2 fix: update CollectPendingSigSharesToVerify to return work status instead of a constant true (pasta)
72c3efc fix: move `more_work` calculation into CollectPendingRecoveredSigsToVerify replacing incorrect logic in ProcessPendingRecoveredSigs (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  I believe the current "moreWork" logic in ProcessPendingRecoveredSigs is improper, and always or almost always returns false.

  I believe the check `recSigsByNode.size() >= nMaxBatchSize` will only report as "needs more work" if the **number of nodes** we are verifying regSigs from is more than 32; **rather than** performing aggregation / verification on 32 recSigs.

  ## What was done?
  Move moreWork logic into ProcessPendingRecoveredSigs

  ## How Has This Been Tested?
  Builds

  ## Breaking Changes
    _Please describe any breaking changes your code introduces_

  ## 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:
  UdjinM6:
    utACK 3b948d2
  kwvg:
    utACK 3b948d2

Tree-SHA512: 8fba0d0364b15af618d46d752d218a61d5c19f26d3d8d401724ad84c4cf16f85b49e5da4f8d4b4056291e17864e5d0d440e7e0cc0c18c4ab6e43258be4438d1e

Author: PastaPastaPasta

src/llmq/signing.cpp

@@ -435,15 +435,17 @@ MessageProcessingResult CSigningManager::ProcessMessage(NodeId from, std::string
     return ret;
 }
 
-void CSigningManager::CollectPendingRecoveredSigsToVerify(
+bool CSigningManager::CollectPendingRecoveredSigsToVerify(
         size_t maxUniqueSessions,
         std::unordered_map<NodeId, std::list<std::shared_ptr<const CRecoveredSig>>>& retSigShares,
         std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
 {
+    bool more_work{false};
+
     {
         LOCK(cs_pending);
         if (pendingRecoveredSigs.empty()) {
-            return;
+            return false;
         }
 
         // TODO: refactor it to remove duplicated code with `CSigSharesManager::CollectPendingSigSharesToVerify`
@@ -466,8 +468,12 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
         }, rnd);
 
         if (retSigShares.empty()) {
-            return;
+            return false;
         }
+
+        more_work = std::any_of(pendingRecoveredSigs.begin(), pendingRecoveredSigs.end(),
+                                [](const auto& p) { return !p.second.empty(); }) ||
+                    !pendingReconstructedRecoveredSigs.empty();
     }
 
     for (auto& p : retSigShares) {
@@ -500,6 +506,8 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
             ++it;
         }
     }
+
+    return more_work;
 }
 
 void CSigningManager::ProcessPendingReconstructedRecoveredSigs(PeerManager& peerman)
@@ -520,7 +528,7 @@ bool CSigningManager::ProcessPendingRecoveredSigs(PeerManager& peerman)
     ProcessPendingReconstructedRecoveredSigs(peerman);
 
     const size_t nMaxBatchSize{32};
-    CollectPendingRecoveredSigsToVerify(nMaxBatchSize, recSigsByNode, quorums);
+    bool more_work = CollectPendingRecoveredSigsToVerify(nMaxBatchSize, recSigsByNode, quorums);
     if (recSigsByNode.empty()) {
         return false;
     }
@@ -574,7 +582,7 @@ bool CSigningManager::ProcessPendingRecoveredSigs(PeerManager& peerman)
         }
     }
 
-    return recSigsByNode.size() >= nMaxBatchSize;
+    return more_work;
 }
 
 // signature must be verified already

src/llmq/signing.h

@@ -209,7 +209,7 @@ class CSigningManager
     void TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
 
 private:
-    void CollectPendingRecoveredSigsToVerify(
+    bool CollectPendingRecoveredSigsToVerify(
         size_t maxUniqueSessions, std::unordered_map<NodeId, std::list<std::shared_ptr<const CRecoveredSig>>>& retSigShares,
         std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_pending);

src/llmq/signing_shares.cpp

@@ -568,6 +568,8 @@ bool CSigSharesManager::CollectPendingSigSharesToVerify(
     size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
     std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
 {
+    bool more_work{false};
+
     {
         LOCK(cs);
         if (nodeStates.empty()) {
@@ -606,6 +608,13 @@ bool CSigSharesManager::CollectPendingSigSharesToVerify(
         if (retSigShares.empty()) {
             return false;
         }
+
+        // Determine if there is still work left in any node state after pulling this batch
+        more_work = std::any_of(nodeStates.begin(), nodeStates.end(),
+                                [](const auto& entry) {
+                                    const auto& ns = entry.second;
+                                    return !ns.pendingIncomingSigShares.Empty();
+                                });
     }
 
     // For the convenience of the caller, also build a map of quorumHash -> quorum
@@ -632,7 +641,7 @@ bool CSigSharesManager::CollectPendingSigSharesToVerify(
         }
     }
 
-    return true;
+    return more_work;
 }
 
 bool CSigSharesManager::ProcessPendingSigShares()
@@ -641,8 +650,8 @@ bool CSigSharesManager::ProcessPendingSigShares()
     std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher> quorums;
 
     const size_t nMaxBatchSize{32};
-    bool collect_status = CollectPendingSigSharesToVerify(nMaxBatchSize, sigSharesByNodes, quorums);
-    if (!collect_status || sigSharesByNodes.empty()) {
+    bool more_work = CollectPendingSigSharesToVerify(nMaxBatchSize, sigSharesByNodes, quorums);
+    if (sigSharesByNodes.empty()) {
         return false;
     }
 
@@ -703,7 +712,7 @@ bool CSigSharesManager::ProcessPendingSigShares()
         ProcessPendingSigShares(v, quorums);
     }
 
-    return sigSharesByNodes.size() >= nMaxBatchSize;
+    return more_work;
 }
 
 // It's ensured that no duplicates are passed to this method