From 8c9f57dac4a0dea2a215d8351f930f6dd2590dd4 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 14 Nov 2024 10:07:34 +0000 Subject: [PATCH] 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 | 3 ++- src/evo/mnhftx.cpp | 4 ++++ src/evo/simplifiedmns.cpp | 20 ++++++++++++++++--- src/evo/simplifiedmns.h | 2 +- src/llmq/quorums.cpp | 13 +++++++++++-- src/llmq/signing.cpp | 6 +++--- src/llmq/signing_shares.cpp | 39 ++++++++++++++++++++++++------------- src/llmq/signing_shares.h | 6 +++--- src/rpc/quorums.cpp | 18 ++++++++--------- 9 files changed, 75 insertions(+), 36 deletions(-) diff --git a/src/evo/assetlocktx.cpp b/src/evo/assetlocktx.cpp index bd3a7e7581..d8f2c502b6 100644 --- a/src/evo/assetlocktx.cpp +++ b/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)); diff --git a/src/evo/mnhftx.cpp b/src/evo/mnhftx.cpp index a7a953db58..05e24db112 100644 --- a/src/evo/mnhftx.cpp +++ b/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"); diff --git a/src/evo/simplifiedmns.cpp b/src/evo/simplifiedmns.cpp index 2946568602..4695d55c41 100644 --- a/src/evo/simplifiedmns.cpp +++ b/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 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 diff --git a/src/evo/simplifiedmns.h b/src/evo/simplifiedmns.h index 1a43e17c27..1de5effb40 100644 --- a/src/evo/simplifiedmns.h +++ b/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; }; diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index ada778def1..87aef8e791 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -602,8 +602,17 @@ std::vector CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp assert(pQuorumBaseBlockIndex); // populate cache for keepOldConnections most recent quorums only bool populate_cache = vecResultQuorums.size() < static_cast(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); } diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index c31ef78039..c43f403e18 100644 --- a/src/llmq/signing.cpp +++ b/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 diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 47a70430fa..baa31593d2 100644 --- a/src/llmq/signing_shares.cpp +++ b/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>& retSigShares, - std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums) +bool CSigSharesManager::CollectPendingSigSharesToVerify( + size_t maxUniqueSessions, std::unordered_map>& retSigShares, + std::unordered_map, 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, 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); } { diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 0715ec3050..e21b6e18ab 100644 --- a/src/llmq/signing_shares.h +++ b/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, - std::unordered_map>& retSigShares, - std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums); + bool CollectPendingSigSharesToVerify( + size_t maxUniqueSessions, std::unordered_map>& retSigShares, + std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums); bool ProcessPendingSigShares(const CConnman& connman); void ProcessPendingSigShares(const std::vector& sigSharesToProcess, diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index 3aa5de1a5a..e4c12e8b21 100644 --- a/src/rpc/quorums.cpp +++ b/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; - - if (quorumHash.IsNull()) { - pQuorum = llmq::SelectQuorumForSigning(llmq_params_opt.value(), chainman.ActiveChain(), *llmq_ctx.qman, id); - } else { - pQuorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash); - } + const auto pQuorum = [&]() { + if (quorumHash.IsNull()) { + return llmq::SelectQuorumForSigning(llmq_params_opt.value(), chainman.ActiveChain(), *llmq_ctx.qman, id); + } else { + return 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");