Merge #6132: refactor: trim and document assumptions for GetQuorum,…

… `Get`*`MN`* and friends

a014cf3 refactor: trim and document assumptions for `Get`*`MN`* and friends (Kittywhiskers Van Gogh)
8c9f57d refactor: trim and document assumptions for `GetQuorum` and friends (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  This pull request aims to document assumptions when handling `CDeterministicMNCPtr` and `CQuorumCPtr` entities, which can be `nullptr`. In some instances, mishandling or missing validation logic can result in an assertion failure or a null pointer dereference (in both circumstances, the client will crash).

  While in other cases, assumptions are made based on prior code that affirms that the returned value will be valid. For the former, bail-out logic has been introduced and for the latter, assertions and code comments have been added.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [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:
  knst:
    utACK a014cf3
  UdjinM6:
    utACK a014cf3

Tree-SHA512: e21824d61d81c4ca4b5b4a545a833932946eb0f279d15c586bb5eae96aefcc88d1e3b3fdfa7a01d161f1650351a7cac4bc917b2d1109d77ea2eedd8408d8f37d

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/deterministicmns.cpp

@@ -428,7 +428,7 @@ CDeterministicMNList CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockI
     for (const auto& id : diff.removedMns) {
         auto dmn = result.GetMNByInternalId(id);
         if (!dmn) {
-            throw(std::runtime_error(strprintf("%s: can't find a removed masternode, id=%d", __func__, id)));
+            throw std::runtime_error(strprintf("%s: can't find a removed masternode, id=%d", __func__, id));
         }
         result.RemoveMN(dmn->proTxHash);
     }
@@ -437,6 +437,9 @@ CDeterministicMNList CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockI
     }
     for (const auto& p : diff.updatedMNs) {
         auto dmn = result.GetMNByInternalId(p.first);
+        if (!dmn) {
+            throw std::runtime_error(strprintf("%s: can't find an updated masternode, id=%d", __func__, p.first));
+        }
         result.UpdateMN(*dmn, p.second);
     }
 
@@ -818,7 +821,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-addr");
             }
 
-            CDeterministicMNCPtr dmn = newList.GetMN(opt_proTx->proTxHash);
+            auto dmn = newList.GetMN(opt_proTx->proTxHash);
             if (!dmn) {
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-hash");
             }
@@ -859,7 +862,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload");
             }
 
-            CDeterministicMNCPtr dmn = newList.GetMN(opt_proTx->proTxHash);
+            auto dmn = newList.GetMN(opt_proTx->proTxHash);
             if (!dmn) {
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-hash");
             }
@@ -887,7 +890,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload");
             }
 
-            CDeterministicMNCPtr dmn = newList.GetMN(opt_proTx->proTxHash);
+            auto dmn = newList.GetMN(opt_proTx->proTxHash);
             if (!dmn) {
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-hash");
             }
@@ -947,6 +950,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
     // current block. We still pay that MN one last time, however.
     if (payee && newList.HasMN(payee->proTxHash)) {
         auto dmn = newList.GetMN(payee->proTxHash);
+        // HasMN has reported that GetMN should succeed, enforce that.
+        assert(dmn);
         auto newState = std::make_shared<CDeterministicMNState>(*dmn->pdmnState);
         newState->nLastPaidHeight = nHeight;
         // Starting from v19 and until MNRewardReallocation, EvoNodes will be paid 4 blocks in a row
@@ -962,6 +967,9 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
         newList.UpdateMN(payee->proTxHash, newState);
         if (debugLogs) {
             dmn = newList.GetMN(payee->proTxHash);
+            // Since the previous GetMN query returned a value, after an update, querying the same
+            // hash *must* give us a result. If it doesn't, that would be a potential logic bug.
+            assert(dmn);
             LogPrint(BCLog::MNPAYMENTS, "CDeterministicMNManager::%s -- MN %s, nConsecutivePayments=%d\n",
                       __func__, dmn->proTxHash.ToString(), dmn->pdmnState->nConsecutivePayments);
         }

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 @@ class CSimplifiedMNListDiff
 
     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/governance/governance.cpp

@@ -1601,6 +1601,9 @@ void CGovernanceManager::RemoveInvalidVotes()
     std::vector<COutPoint> changedKeyMNs;
     for (const auto& p : diff.updatedMNs) {
         auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(p.first);
+        // BuildDiff will construct itself with MNs that we already have knowledge
+        // of, meaning that fetch operations should never fail.
+        assert(oldDmn);
         if ((p.second.fields & CDeterministicMNStateDiff::Field_keyIDVoting) && p.second.state.keyIDVoting != oldDmn->pdmnState->keyIDVoting) {
             changedKeyMNs.emplace_back(oldDmn->collateralOutpoint);
         } else if ((p.second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && p.second.state.pubKeyOperator != oldDmn->pdmnState->pubKeyOperator) {
@@ -1609,6 +1612,9 @@ void CGovernanceManager::RemoveInvalidVotes()
     }
     for (const auto& id : diff.removedMns) {
         auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(id);
+        // BuildDiff will construct itself with MNs that we already have knowledge
+        // of, meaning that fetch operations should never fail.
+        assert(oldDmn);
         changedKeyMNs.emplace_back(oldDmn->collateralOutpoint);
     }
 

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(
-        size_t maxUniqueSessions,
-        std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
-        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
+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)
 {
     {
         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());
-            assert(quorum != nullptr);
+            auto quorum = qman.GetQuorum(llmqType, sigShare.getQuorumHash());
+            // 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);
-    if (sigSharesByNodes.empty()) {
+    bool collect_status = CollectPendingSigSharesToVerify(nMaxBatchSize, sigSharesByNodes, quorums);
+    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);
-            ++it;
-        } else {
-            it = quorums.erase(it);
+            auto quorum = qman.GetQuorum(it->first.first, it->first.second);
+            if (quorum) {
+                it->second = quorum;
+                ++it;
+                continue;
+            }
         }
+        it = quorums.erase(it);
     }
 
     {

src/llmq/signing_shares.h

@@ -452,9 +452,9 @@ class CSigSharesManager : public CRecoveredSigsListener
     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,
-            std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
-            std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
+    bool CollectPendingSigSharesToVerify(
+        size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
+        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
     bool ProcessPendingSigShares(const CConnman& connman);
 
     void ProcessPendingSigShares(const std::vector<CSigShare>& sigSharesToProcess,

src/masternode/node.cpp

@@ -129,7 +129,7 @@ void CActiveMasternodeManager::InitInternal(const CBlockIndex* pindex)
 
     CDeterministicMNList mnList = Assert(m_dmnman)->GetListForBlock(pindex);
 
-    CDeterministicMNCPtr dmn = mnList.GetMNByOperatorKey(m_info.blsPubKeyOperator);
+    auto dmn = mnList.GetMNByOperatorKey(m_info.blsPubKeyOperator);
     if (!dmn) {
         // MN not appeared on the chain yet
         return;
@@ -202,6 +202,9 @@ void CActiveMasternodeManager::UpdatedBlockTip(const CBlockIndex* pindexNew, con
 
         auto oldDmn = oldMNList.GetMN(cur_protx_hash);
         auto newDmn = newMNList.GetMN(cur_protx_hash);
+        if (!oldDmn || !newDmn) {
+            return reset(MasternodeState::SOME_ERROR);
+        }
         if (newDmn->pdmnState->pubKeyOperator != oldDmn->pdmnState->pubKeyOperator) {
             // MN operator key changed or revoked
             return reset(MasternodeState::OPERATOR_KEY_CHANGED);

src/qt/masternodelist.cpp

@@ -354,7 +354,8 @@ CDeterministicMNCPtr MasternodeList::GetSelectedDIP3MN()
     uint256 proTxHash;
     proTxHash.SetHex(strProTxHash);
 
-    return clientModel->getMasternodeList().first.GetMN(proTxHash);;
+    // Caller is responsible for nullptr checking return value
+    return clientModel->getMasternodeList().first.GetMN(proTxHash);
 }
 
 void MasternodeList::extraInfoDIP3_clicked()

src/rpc/evo.cpp

@@ -1635,13 +1635,19 @@ static RPCHelpMan protx_listdiff()
     UniValue jremovedMNs(UniValue::VARR);
     for(const auto& internal_id : mnDiff.removedMns) {
         auto dmn = baseBlockMNList.GetMNByInternalId(internal_id);
+        // BuildDiff will construct itself with MNs that we already have knowledge
+        // of, meaning that fetch operations should never fail.
+        CHECK_NONFATAL(dmn);
         jremovedMNs.push_back(dmn->proTxHash.ToString());
     }
     ret.pushKV("removedMNs", jremovedMNs);
 
     UniValue jupdatedMNs(UniValue::VARR);
     for(const auto& [internal_id, stateDiff] : mnDiff.updatedMNs) {
         auto dmn = baseBlockMNList.GetMNByInternalId(internal_id);
+        // BuildDiff will construct itself with MNs that we already have knowledge
+        // of, meaning that fetch operations should never fail.
+        CHECK_NONFATAL(dmn);
         UniValue obj(UniValue::VOBJ);
         obj.pushKV(dmn->proTxHash.ToString(), stateDiff.ToJson(dmn->nType));
         jupdatedMNs.push_back(obj);