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