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; }