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 1/2] 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"); From a014cf3703aa4d33aef88a9a03c5e69b262c2c5b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 14 Nov 2024 10:08:05 +0000 Subject: [PATCH 2/2] refactor: trim and document assumptions for `Get`*`MN`* and friends --- src/evo/deterministicmns.cpp | 16 ++++++++++++---- src/governance/governance.cpp | 6 ++++++ src/masternode/node.cpp | 5 ++++- src/qt/masternodelist.cpp | 3 ++- src/rpc/evo.cpp | 6 ++++++ src/rpc/masternode.cpp | 10 ++++++---- src/test/evo_deterministicmns_tests.cpp | 4 +++- 7 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 0b969d1f25..96fbe4c02c 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -428,7 +428,7 @@ CDeterministicMNList CDeterministicMNList::ApplyDiff(gsl::not_nullproTxHash); } @@ -437,6 +437,9 @@ CDeterministicMNList CDeterministicMNList::ApplyDiff(gsl::not_nullproTxHash); + 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(*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); } diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 6b02357540..deea85ce26 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -1601,6 +1601,9 @@ void CGovernanceManager::RemoveInvalidVotes() std::vector 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); } diff --git a/src/masternode/node.cpp b/src/masternode/node.cpp index 8ffe2c2f9a..0a0b51deef 100644 --- a/src/masternode/node.cpp +++ b/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); diff --git a/src/qt/masternodelist.cpp b/src/qt/masternodelist.cpp index 6bf6816e4b..7e3eabe410 100644 --- a/src/qt/masternodelist.cpp +++ b/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() diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index f6e96b58b5..a7d040a059 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -1635,6 +1635,9 @@ 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); @@ -1642,6 +1645,9 @@ static RPCHelpMan protx_listdiff() 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); diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index 0c1cd33c86..b18f7ef06d 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -229,7 +229,7 @@ static RPCHelpMan masternode_status() // keep compatibility with legacy status for now (might get deprecated/removed later) mnObj.pushKV("outpoint", node.mn_activeman->GetOutPoint().ToStringShort()); mnObj.pushKV("service", node.mn_activeman->GetService().ToStringAddrPort()); - CDeterministicMNCPtr dmn = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash()); + auto dmn = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash()); if (dmn) { mnObj.pushKV("proTxHash", dmn->proTxHash.ToString()); mnObj.pushKV("type", std::string(GetMnType(dmn->nType).description)); @@ -322,9 +322,11 @@ static RPCHelpMan masternode_winners() for (int h = nStartHeight; h <= nChainTipHeight; h++) { const CBlockIndex* pIndex = pindexTip->GetAncestor(h - 1); auto payee = node.dmnman->GetListForBlock(pIndex).GetMNPayee(pIndex); - std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.govman), tip_mn_list, h, payee); - if (strFilter != "" && strPayments.find(strFilter) == std::string::npos) continue; - obj.pushKV(strprintf("%d", h), strPayments); + if (payee) { + std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.govman), tip_mn_list, h, payee); + if (strFilter != "" && strPayments.find(strFilter) == std::string::npos) continue; + obj.pushKV(strprintf("%d", h), strPayments); + } } auto projection = node.dmnman->GetListForBlock(pindexTip).GetProjectedMNPayees(pindexTip, 20); diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 5677b71a48..ae82700ad2 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -467,6 +467,7 @@ void FuncDIP3Protx(TestChainSetup& setup) // check MN reward payments for (size_t i = 0; i < 20; i++) { auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); + BOOST_ASSERT(dmnExpectedPayee); CBlock block = setup.CreateAndProcessBlock({}, setup.coinbaseKey); dmnman.UpdatedBlockTip(chainman.ActiveChain().Tip()); @@ -525,7 +526,7 @@ void FuncDIP3Protx(TestChainSetup& setup) // test that the revoked MN does not get paid anymore for (size_t i = 0; i < 20; i++) { auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); - BOOST_REQUIRE(dmnExpectedPayee->proTxHash != dmnHashes[0]); + BOOST_REQUIRE(dmnExpectedPayee && dmnExpectedPayee->proTxHash != dmnHashes[0]); CBlock block = setup.CreateAndProcessBlock({}, setup.coinbaseKey); dmnman.UpdatedBlockTip(chainman.ActiveChain().Tip()); @@ -575,6 +576,7 @@ void FuncDIP3Protx(TestChainSetup& setup) bool foundRevived = false; for (size_t i = 0; i < 20; i++) { auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); + BOOST_ASSERT(dmnExpectedPayee); if (dmnExpectedPayee->proTxHash == dmnHashes[0]) { foundRevived = true; }