refactor: trim and document assumptions for GetQuorum and friends

Some portions of the codebase make implicit assumptions that `GetQuorum`
will not return a `nullptr` by not performing checking for it or make
explicit assumptions by `assert`ing not-`nullptr`.

Let's document explicit assumptions, document implicit assumptions and
soften some hard assumptions where softening is possible.

src/evo/assetlocktx.cpp

@@ -140,7 +140,8 @@ bool CAssetUnlockPayload::VerifySig(const llmq::CQuorumManager& qman, const uint
     const auto quorum = qman.GetQuorum(llmqType, quorumHash);
     // quorum must be valid at this point. Let's check and throw error just in case
     if (!quorum) {
-        return state.Invalid(TxValidationResult::TX_CONSENSUS, "internal-error");
+        LogPrintf("%s: ERROR! No quorum for credit pool found for hash=%s\n", __func__, quorumHash.ToString());
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-quorum-internal-error");
     }
 
     const uint256 requestId = ::SerializeHash(std::make_pair(ASSETUNLOCK_REQUESTID_PREFIX, index));

src/evo/mnhftx.cpp

@@ -96,6 +96,10 @@ bool MNHFTx::Verify(const llmq::CQuorumManager& qman, const uint256& quorumHash,
     const Consensus::LLMQType& llmqType = Params().GetConsensus().llmqTypeMnhf;
     const auto quorum = qman.GetQuorum(llmqType, quorumHash);
 
+    if (!quorum) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-missing-quorum");
+    }
+
     const uint256 signHash = llmq::BuildSignHash(llmqType, quorum->qc->quorumHash, requestId, msgHash);
     if (!sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash)) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-invalid");

src/evo/simplifiedmns.cpp

@@ -187,14 +187,23 @@ bool CSimplifiedMNListDiff::BuildQuorumsDiff(const CBlockIndex* baseBlockIndex,
     return true;
 }
 
-void CSimplifiedMNListDiff::BuildQuorumChainlockInfo(const llmq::CQuorumManager& qman, const CBlockIndex* blockIndex)
+bool CSimplifiedMNListDiff::BuildQuorumChainlockInfo(const llmq::CQuorumManager& qman, const CBlockIndex* blockIndex)
 {
     // Group quorums (indexes corresponding to entries of newQuorums) per CBlockIndex containing the expected CL signature in CbTx.
     // We want to avoid to load CbTx now, as more than one quorum will target the same block: hence we want to load CbTxs once per block (heavy operation).
     std::multimap<const CBlockIndex*, uint16_t>  workBaseBlockIndexMap;
 
     for (const auto [idx, e] : enumerate(newQuorums)) {
+        // We assume that we have on hand, quorums that correspond to the hashes queried.
+        // If we cannot find them, something must have gone wrong and we should cease trying
+        // to build any further.
         auto quorum = qman.GetQuorum(e.llmqType, e.quorumHash);
+        if (!quorum) {
+            LogPrintf("%s: ERROR! Unexpected missing quorum with llmqType=%d, quorumHash=%s\n", __func__,
+                      ToUnderlying(e.llmqType), e.quorumHash.ToString());
+            return false;
+        }
+
         // In case of rotation, all rotated quorums rely on the CL sig expected in the cycleBlock (the block of the first DKG) - 8
         // In case of non-rotation, quorums rely on the CL sig expected in the block of the DKG - 8
         const CBlockIndex* pWorkBaseBlockIndex =
@@ -203,7 +212,7 @@ void CSimplifiedMNListDiff::BuildQuorumChainlockInfo(const llmq::CQuorumManager&
         workBaseBlockIndexMap.insert(std::make_pair(pWorkBaseBlockIndex, idx));
     }
 
-    for(auto it = workBaseBlockIndexMap.begin(); it != workBaseBlockIndexMap.end(); ) {
+    for (auto it = workBaseBlockIndexMap.begin(); it != workBaseBlockIndexMap.end();) {
         // Process each key (CBlockIndex containing the expected CL signature in CbTx) of the std::multimap once
         const CBlockIndex* pWorkBaseBlockIndex = it->first;
         const auto cbcl = GetNonNullCoinbaseChainlock(pWorkBaseBlockIndex);
@@ -224,6 +233,8 @@ void CSimplifiedMNListDiff::BuildQuorumChainlockInfo(const llmq::CQuorumManager&
             it_sig->second.insert(idx_set.begin(), idx_set.end());
         }
     }
+
+    return true;
 }
 
 UniValue CSimplifiedMNListDiff::ToJson(bool extended) const
@@ -366,7 +377,10 @@ bool BuildSimplifiedMNListDiff(CDeterministicMNManager& dmnman, const Chainstate
     }
 
     if (DeploymentActiveAfter(blockIndex, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) {
-        mnListDiffRet.BuildQuorumChainlockInfo(qman, blockIndex);
+        if (!mnListDiffRet.BuildQuorumChainlockInfo(qman, blockIndex)) {
+            errorRet = strprintf("failed to build quorum chainlock info");
+            return false;
+        }
     }
 
     // TODO store coinbase TX in CBlockIndex

src/evo/simplifiedmns.h

@@ -170,7 +170,7 @@ public:
 
     bool BuildQuorumsDiff(const CBlockIndex* baseBlockIndex, const CBlockIndex* blockIndex,
                           const llmq::CQuorumBlockProcessor& quorum_block_processor);
-    void BuildQuorumChainlockInfo(const llmq::CQuorumManager& qman, const CBlockIndex* blockIndex);
+    bool BuildQuorumChainlockInfo(const llmq::CQuorumManager& qman, const CBlockIndex* blockIndex);
 
     [[nodiscard]] UniValue ToJson(bool extended = false) const;
 };

src/llmq/quorums.cpp

@@ -602,8 +602,17 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp
         assert(pQuorumBaseBlockIndex);
         // populate cache for keepOldConnections most recent quorums only
         bool populate_cache = vecResultQuorums.size() < static_cast<size_t>(llmq_params_opt->keepOldConnections);
+
+        // We assume that every quorum asked for is available to us on hand, if this
+        // fails then we can assume that something has gone wrong and we should stop
+        // trying to process any further and return a blank.
         auto quorum = GetQuorum(llmqType, pQuorumBaseBlockIndex, populate_cache);
-        assert(quorum != nullptr);
+        if (!quorum) {
+            LogPrintf("%s: ERROR! Unexpected missing quorum with llmqType=%d, blockHash=%s, populate_cache=%s\n",
+                      __func__, ToUnderlying(llmqType), pQuorumBaseBlockIndex->GetBlockHash().ToString(),
+                      populate_cache ? "true" : "false");
+            return {};
+        }
         vecResultQuorums.emplace_back(quorum);
     }
 
@@ -742,7 +751,7 @@ PeerMsgRet CQuorumManager::ProcessMessage(CNode& pfrom, const std::string& msg_t
             return sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, request_limit_exceeded);
         }
 
-        const CQuorumCPtr pQuorum = GetQuorum(request.GetLLMQType(), pQuorumBaseBlockIndex);
+        const auto pQuorum = GetQuorum(request.GetLLMQType(), pQuorumBaseBlockIndex);
         if (pQuorum == nullptr) {
             return sendQDATA(CQuorumDataRequest::Errors::QUORUM_NOT_FOUND, request_limit_exceeded);
         }

src/llmq/signing.cpp

@@ -588,7 +588,7 @@ static bool PreVerifyRecoveredSig(const CQuorumManager& quorum_manager, const CR
         return false;
     }
 
-    CQuorumCPtr quorum = quorum_manager.GetQuorum(llmqType, recoveredSig.getQuorumHash());
+    auto quorum = quorum_manager.GetQuorum(llmqType, recoveredSig.getQuorumHash());
 
     if (!quorum) {
         LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found\n", __func__,
@@ -681,7 +681,7 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
             auto llmqType = recSig->getLlmqType();
             auto quorumKey = std::make_pair(recSig->getLlmqType(), recSig->getQuorumHash());
             if (!retQuorums.count(quorumKey)) {
-                CQuorumCPtr quorum = qman.GetQuorum(llmqType, recSig->getQuorumHash());
+                auto quorum = qman.GetQuorum(llmqType, recSig->getQuorumHash());
                 if (!quorum) {
                     LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found, node=%d\n", __func__,
                               recSig->getQuorumHash().ToString(), nodeId);
@@ -881,7 +881,7 @@ bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, CSigShares
     if (m_mn_activeman == nullptr) return false;
     if (m_mn_activeman->GetProTxHash().IsNull()) return false;
 
-    const CQuorumCPtr quorum = [&]() {
+    const auto quorum = [&]() {
         if (quorumHash.IsNull()) {
             // This might end up giving different results on different members
             // This might happen when we are on the brink of confirming a new quorum

src/llmq/signing_shares.cpp

@@ -549,15 +549,14 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager
     return true;
 }
 
-void CSigSharesManager::CollectPendingSigSharesToVerify(
+bool CSigSharesManager::CollectPendingSigSharesToVerify(
-        size_t maxUniqueSessions,
+    size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
-        std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
+    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
-        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
 {
     {
         LOCK(cs);
         if (nodeStates.empty()) {
-            return;
+            return false;
         }
 
         // This will iterate node states in random order and pick one sig share at a time. This avoids processing
@@ -590,7 +589,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
             rnd);
 
         if (retSigShares.empty()) {
-            return;
+            return false;
         }
     }
 
@@ -605,11 +604,20 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
                 continue;
             }
 
-            CQuorumCPtr quorum = qman.GetQuorum(llmqType, sigShare.getQuorumHash());
+            auto quorum = qman.GetQuorum(llmqType, sigShare.getQuorumHash());
-            assert(quorum != nullptr);
+            // Despite constructing a convenience map, we assume that the quorum *must* be present.
+            // The absence of it might indicate an inconsistent internal state, so we should report
+            // nothing instead of reporting flawed data.
+            if (!quorum) {
+                LogPrintf("%s: ERROR! Unexpected missing quorum with llmqType=%d, quorumHash=%s\n", __func__,
+                          ToUnderlying(llmqType), sigShare.getQuorumHash().ToString());
+                return false;
+            }
             retQuorums.try_emplace(k, quorum);
         }
     }
+
+    return true;
 }
 
 bool CSigSharesManager::ProcessPendingSigShares(const CConnman& connman)
@@ -618,8 +626,8 @@ bool CSigSharesManager::ProcessPendingSigShares(const CConnman& connman)
     std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher> quorums;
 
     const size_t nMaxBatchSize{32};
-    CollectPendingSigSharesToVerify(nMaxBatchSize, sigSharesByNodes, quorums);
+    bool collect_status = CollectPendingSigSharesToVerify(nMaxBatchSize, sigSharesByNodes, quorums);
-    if (sigSharesByNodes.empty()) {
+    if (!collect_status || sigSharesByNodes.empty()) {
         return false;
     }
 
@@ -1269,11 +1277,14 @@ void CSigSharesManager::Cleanup()
     // Find quorums which became inactive
     for (auto it = quorums.begin(); it != quorums.end(); ) {
         if (IsQuorumActive(it->first.first, qman, it->first.second)) {
-            it->second = qman.GetQuorum(it->first.first, it->first.second);
+            auto quorum = qman.GetQuorum(it->first.first, it->first.second);
-            ++it;
+            if (quorum) {
-        } else {
+                it->second = quorum;
-            it = quorums.erase(it);
+                ++it;
+                continue;
+            }
         }
+        it = quorums.erase(it);
     }
 
     {

src/llmq/signing_shares.h

@@ -452,9 +452,9 @@ private:
     static bool PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager,
                                           const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, bool& retBan);
 
-    void CollectPendingSigSharesToVerify(size_t maxUniqueSessions,
+    bool CollectPendingSigSharesToVerify(
-            std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
+        size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
-            std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
+        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
     bool ProcessPendingSigShares(const CConnman& connman);
 
     void ProcessPendingSigShares(const std::vector<CSigShare>& sigSharesToProcess,

src/rpc/quorums.cpp

@@ -255,7 +255,7 @@ static RPCHelpMan quorum_info()
         includeSkShare = ParseBoolV(request.params[2], "includeSkShare");
     }
 
-    auto quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash);
+    const auto quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash);
     if (!quorum) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found");
     }
@@ -451,13 +451,13 @@ static UniValue quorum_sign_helper(const JSONRPCRequest& request, Consensus::LLM
     if (fSubmit) {
         return llmq_ctx.sigman->AsyncSignIfMember(llmqType, *llmq_ctx.shareman, id, msgHash, quorumHash);
     } else {
-        llmq::CQuorumCPtr pQuorum;
+        const auto pQuorum = [&]() {
-
+            if (quorumHash.IsNull()) {
-        if (quorumHash.IsNull()) {
+                return llmq::SelectQuorumForSigning(llmq_params_opt.value(), chainman.ActiveChain(), *llmq_ctx.qman, id);
-            pQuorum = llmq::SelectQuorumForSigning(llmq_params_opt.value(), chainman.ActiveChain(), *llmq_ctx.qman, id);
+            } else {
-        } else {
+                return llmq_ctx.qman->GetQuorum(llmqType, quorumHash);
-            pQuorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash);
+            }
-        }
+        }();
 
         if (pQuorum == nullptr) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found");
@@ -592,7 +592,7 @@ static RPCHelpMan quorum_verify()
     }
 
     uint256 quorumHash(ParseHashV(request.params[4], "quorumHash"));
-    llmq::CQuorumCPtr quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash);
+    const auto quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash);
 
     if (!quorum) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found");