chore: apply review suggestions

- Fast bail-out if SetSecretKeyShare() is supplied with blank ProTx hash
- Pass reference instead of copy to DataCleanupHelper()
- Refactor ProcessContribQGETDATA result processing to be more legible
- Squash down two strprintf calls into one
- Validate verification vector availability before access

Author: kwvg

src/active/quorums.cpp

@@ -137,7 +137,8 @@ MessageProcessingResult QuorumParticipant::ProcessContribQDATA(CNode& pfrom, CDa
                                                                CQuorum& quorum, CQuorumDataRequest& request)
 {
     if (request.GetDataMask() & CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS) {
-        if (WITH_LOCK(quorum.cs_vvec_shShare, return quorum.quorumVvec->size() != size_t(quorum.params.threshold))) {
+        if (WITH_LOCK(quorum.cs_vvec_shShare, return !quorum.HasVerificationVectorInternal()
+                                                     || quorum.quorumVvec->size() != size_t(quorum.params.threshold))) {
             // Don't bump score because we asked for it
             LogPrint(BCLog::LLMQ, "QuorumParticipant::%s -- %s: No valid quorum verification vector available, from peer=%d\n", __func__, NetMsgType::QDATA, pfrom.GetId());
             return {};

src/llmq/quorums.cpp

@@ -26,7 +26,7 @@ uint256 MakeQuorumKey(const CQuorum& q)
     return hw.GetHash();
 }
 
-void DataCleanupHelper(CDBWrapper& db, std::set<uint256> skip_list, bool compact)
+void DataCleanupHelper(CDBWrapper& db, const std::set<uint256>& skip_list, bool compact)
 {
     const auto prefixes = {DB_QUORUM_QUORUM_VVEC, DB_QUORUM_SK_SHARE};
 
@@ -124,7 +124,7 @@ bool CQuorum::SetVerificationVector(const std::vector<CBLSPublicKey>& quorumVecI
 
 bool CQuorum::SetSecretKeyShare(const CBLSSecretKey& secretKeyShare, const uint256& protx_hash)
 {
-    if (!secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(GetMemberIndex(protx_hash)))) {
+    if (protx_hash.IsNull() || !secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(GetMemberIndex(protx_hash)))) {
         return false;
     }
     LOCK(cs_vvec_shShare);

src/llmq/quorums.h

@@ -17,6 +17,7 @@
 #include <uint256.h>
 #include <util/time.h>
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -34,7 +35,7 @@ extern const std::string DB_QUORUM_SK_SHARE;
 extern const std::string DB_QUORUM_QUORUM_VVEC;
 
 uint256 MakeQuorumKey(const CQuorum& q);
-void DataCleanupHelper(CDBWrapper& db, std::set<uint256> skip_list, bool compact = false);
+void DataCleanupHelper(CDBWrapper& db, const std::set<uint256>& skip_list, bool compact = false);
 
 /**
  * Object used as a key to store CQuorumDataRequest

src/llmq/quorumsman.cpp

@@ -481,16 +481,22 @@ MessageProcessingResult CQuorumManager::ProcessMessage(CNode& pfrom, CConnman& c
         }
 
         // Check if request wants ENCRYPTED_CONTRIBUTIONS data
-        MessageProcessingResult ret{};
+        CQuorumDataRequest::Errors ret_err{CQuorumDataRequest::Errors::NONE};
+        MessageProcessingResult qdata_ret{}, ret{};
         if (m_handler) {
             ret = m_handler->ProcessContribQGETDATA(request_limit_exceeded, ssResponseData, *pQuorum, request, pQuorumBaseBlockIndex);
             if (auto request_err = request.GetError(); request_err != CQuorumDataRequest::Errors::NONE &&
                                                        request_err != CQuorumDataRequest::Errors::UNDEFINED) {
-                auto qdata_ret = sendQDATA(request_err, request_limit_exceeded);
-                return ret.empty() ? qdata_ret : ret;
+                ret_err = request_err;
             }
         }
-        return ret.empty() ? sendQDATA(CQuorumDataRequest::Errors::NONE, request_limit_exceeded, ssResponseData) : ret;
+        // sendQDATA also pushes a message independent of the returned value
+        if (ret_err != CQuorumDataRequest::Errors::NONE) {
+            qdata_ret = sendQDATA(ret_err, request_limit_exceeded);
+        } else {
+            qdata_ret = sendQDATA(CQuorumDataRequest::Errors::NONE, request_limit_exceeded, ssResponseData);
+        }
+        return ret.empty() ? qdata_ret : ret;
     }
 
     if (msg_type == NetMsgType::QDATA) {
@@ -518,7 +524,7 @@ MessageProcessingResult CQuorumManager::ProcessMessage(CNode& pfrom, CConnman& c
         }
 
         if (request.GetError() != CQuorumDataRequest::Errors::NONE) {
-            LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: %s, from peer=%d\n", __func__, msg_type, strprintf("Error %d (%s)", request.GetError(), request.GetErrorString()), pfrom.GetId());
+            LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: Error %d (%s), from peer=%d\n", __func__, msg_type, request.GetError(), request.GetErrorString(), pfrom.GetId());
             return {};
         }