From 8297c8f559fa755a828c427c2b95fbf501f8d323 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 26 Jul 2024 10:10:23 +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 | 5 ++++- 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 | 16 +++++++-------- 9 files changed, 76 insertions(+), 35 deletions(-) diff --git a/src/evo/assetlocktx.cpp b/src/evo/assetlocktx.cpp index ccbb0042e023e..222c10a55baf3 100644 --- a/src/evo/assetlocktx.cpp +++ b/src/evo/assetlocktx.cpp @@ -129,7 +129,10 @@ bool CAssetUnlockPayload::VerifySig(const llmq::CQuorumManager& qman, const uint } const auto quorum = qman.GetQuorum(llmqType, quorumHash); - assert(quorum); + if (quorum == nullptr) { + LogPrint(BCLog::CREDITPOOL, "No quorum for found for hash=%s\n", quorumHash.ToString()); + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-missing-quorum"); + } const uint256 requestId = ::SerializeHash(std::make_pair(ASSETUNLOCK_REQUESTID_PREFIX, index)); diff --git a/src/evo/mnhftx.cpp b/src/evo/mnhftx.cpp index 02f1839f087fb..b29ae52c0e9c3 100644 --- a/src/evo/mnhftx.cpp +++ b/src/evo/mnhftx.cpp @@ -93,6 +93,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 == nullptr) { + 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 41c3138c66526..2414a28ace3e9 100644 --- a/src/evo/simplifiedmns.cpp +++ b/src/evo/simplifiedmns.cpp @@ -183,14 +183,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 == nullptr) { + 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 = @@ -199,7 +208,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); @@ -220,6 +229,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 @@ -362,7 +373,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 ed894f9ff227d..2d323e6b15f87 100644 --- a/src/evo/simplifiedmns.h +++ b/src/evo/simplifiedmns.h @@ -166,7 +166,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; }; diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 3f729aa1e4c55..b15e52e749d3a 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -594,8 +594,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 == nullptr) { + 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); } @@ -734,7 +743,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 e9cc76e3504c8..b0dfcbd47e3dc 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__, @@ -683,7 +683,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 3667e593bc3a9..0a084bbdfbecb 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 @@ -585,7 +584,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify( }, rnd); if (retSigShares.empty()) { - return; + return false; } } @@ -600,11 +599,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 == nullptr) { + 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) @@ -613,8 +621,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; } @@ -1261,11 +1269,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 != nullptr) { + it->second = quorum; + ++it; + continue; + } } + it = quorums.erase(it); } { diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 3725d295be806..75236abafe1de 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -448,9 +448,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>& 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 ee6c990391730..e14159d3abbdd 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -452,13 +452,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); - } + 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"); @@ -593,7 +593,7 @@ static RPCHelpMan quorum_verify() } uint256 quorumHash(ParseHashV(request.params[4], "quorumHash")); - llmq::CQuorumCPtr quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash); + auto quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash); if (!quorum) { throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found"); From d382c05b5b9cb386f4f5862bced21ad8e3ef8913 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 20 Jul 2024 11:38:45 +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 b35dab60a9804..d917c71272f12 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -426,7 +426,7 @@ CDeterministicMNList CDeterministicMNList::ApplyDiff(gsl::not_nullproTxHash); } @@ -435,6 +435,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"); } @@ -857,7 +860,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"); } @@ -885,7 +888,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"); } @@ -945,6 +948,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 @@ -960,6 +965,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 6b195ea831d44..bcdc3161b7f8d 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -1559,6 +1559,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) { @@ -1567,6 +1570,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 3964c8d195864..1ba17533a74f4 100644 --- a/src/masternode/node.cpp +++ b/src/masternode/node.cpp @@ -93,7 +93,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; @@ -166,6 +166,9 @@ void CActiveMasternodeManager::UpdatedBlockTip(const CBlockIndex* pindexNew, con auto oldDmn = oldMNList.GetMN(cur_protx_hash); auto newDmn = newMNList.GetMN(cur_protx_hash); + if (oldDmn == nullptr || newDmn == nullptr) { + return reset(MASTERNODE_ERROR); + } if (newDmn->pdmnState->pubKeyOperator != oldDmn->pdmnState->pubKeyOperator) { // MN operator key changed or revoked return reset(MASTERNODE_OPERATOR_KEY_CHANGED); diff --git a/src/qt/masternodelist.cpp b/src/qt/masternodelist.cpp index 43ba572455df0..a4d40adff0ac3 100644 --- a/src/qt/masternodelist.cpp +++ b/src/qt/masternodelist.cpp @@ -356,7 +356,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 66f435dafdc95..0a2d3c79eb0cd 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -1709,6 +1709,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 != nullptr); jremovedMNs.push_back(dmn->proTxHash.ToString()); } ret.pushKV("removedMNs", jremovedMNs); @@ -1716,6 +1719,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 != nullptr); 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 923cdc0993231..4059f7cb5c5a4 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -220,7 +220,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().ToString()); - CDeterministicMNCPtr dmn = node.dmnman->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash()); + auto dmn = 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)); @@ -316,9 +316,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(*node.govman, tip_mn_list, h, payee); - if (strFilter != "" && strPayments.find(strFilter) == std::string::npos) continue; - obj.pushKV(strprintf("%d", h), strPayments); + if (payee != nullptr) { + std::string strPayments = GetRequiredPaymentsString(*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 48b73ebf99abc..e2d44baf0eea5 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -464,6 +464,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()); @@ -522,7 +523,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_ASSERT(dmnExpectedPayee->proTxHash != dmnHashes[0]); + BOOST_ASSERT(dmnExpectedPayee && dmnExpectedPayee->proTxHash != dmnHashes[0]); CBlock block = setup.CreateAndProcessBlock({}, setup.coinbaseKey); dmnman.UpdatedBlockTip(chainman.ActiveChain().Tip()); @@ -570,6 +571,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; }