From a0c8c9f0a56e39877a6747413bbecb42efb143c7 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sat, 28 Oct 2023 07:53:27 +0700 Subject: [PATCH] fix: possible assert call if nHeight in CDeterministicMNList is higher then Tip (#5590) ## Issue being fixed or feature implemented fix: possible assert call if nHeight in CDeterministicMNListDiff is higher than Tip Example of new log: ``` 2023-09-28T17:35:50Z GetProjectedMNPayeesAtChainTip WARNING pindex is nullptr due to height=914160 chain height=914159 ``` instead assert call: ``` ... #6 0x00007ffff7a33b86 in __assert_fail (assertion=0x55555783afd2 "pindex", file=0x5555577f2ed8 "llmq/utils.cpp", line=730, function=0x5555577f2448 "bool llmq::utils::IsMNRewardReallocationActive(const CBlockIndex*)") at ./assert/assert.c:101 #7 0x0000555555ab7daf in llmq::utils::IsMNRewardReallocationActive (pindex=) at llmq/utils.cpp:730 #8 0x00005555559458ad in CDeterministicMNList::GetProjectedMNPayees (this=this@entry=0x7fffffffc690, pindex=0x0, nCount=, nCount@entry=2147483647) at evo/deterministicmns.cpp:231 #9 0x000055555594614f in CDeterministicMNList::GetProjectedMNPayeesAtChainTip (this=this@entry=0x7fffffffc690, nCount=nCount@entry=2147483647) at evo/deterministicmns.cpp:216 #10 0x00005555558c9f51 in MasternodeList::updateDIP3List (this=this@entry=0x55555908cfd0) at qt/masternodelist.cpp:194 #11 0x00005555558ca9a0 in MasternodeList::updateDIP3ListScheduled (this=0x55555908cfd0) at qt/masternodelist.cpp:157 #12 0x000055555684a60f in void doActivate(QObject*, int, void**) () #13 0x00005555568525b1 in QTimer::timerEvent(QTimerEvent*) () #14 0x0000555556844ce5 in QObject::event(QEvent*) () #15 0x0000555556ac3252 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () #16 0x000055555681e6b8 in QCoreApplication::sendEvent(QObject*, QEvent*) () #17 0x000055555686de2a in QTimerInfoList::activateTimers() () #18 0x000055555686be84 in QEventDispatcherUNIX::processEvents(QFlags) () #19 0x00005555569bf8a2 in QXcbUnixEventDispatcher::processEvents(QFlags) () #20 0x000055555681caf6 in QEventLoop::exec(QFlags) () #21 0x0000555556825f8a in QCoreApplication::exec() () ... ``` ## What was done? ClientModel returns now a pair: MNList and CBlockIndex; so, we always know the which one has been used even if current chain is switched. ## How Has This Been Tested? Run on my localhost from `c034ff0c2606142ba3e8894bc74f693b87374e5c` - aborted with backtrace like above. With both of commit - no assert more. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone --------- Co-authored-by: UdjinM6 --- src/evo/deterministicmns.cpp | 45 ++++++++++++++++------------------- src/evo/deterministicmns.h | 1 - src/governance/governance.cpp | 6 +++-- src/interfaces/node.h | 6 +++-- src/node/interfaces.cpp | 13 ++++++---- src/qt/clientmodel.cpp | 17 +++++++------ src/qt/clientmodel.h | 5 ++-- src/qt/governancelist.cpp | 2 +- src/qt/masternodelist.cpp | 16 +++++++++---- src/qt/rpcconsole.cpp | 4 ++-- src/rpc/masternode.cpp | 5 ++-- src/ui_interface.cpp | 2 +- src/ui_interface.h | 2 +- 13 files changed, 70 insertions(+), 54 deletions(-) diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 9b86418bcea15..3dd9f6e879f43 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -214,38 +214,35 @@ CDeterministicMNCPtr CDeterministicMNList::GetMNPayee(const CBlockIndex* pIndex) return best; } -std::vector CDeterministicMNList::GetProjectedMNPayeesAtChainTip(int nCount) const -{ - return GetProjectedMNPayees(::ChainActive()[nHeight], nCount); -} - std::vector CDeterministicMNList::GetProjectedMNPayees(const CBlockIndex* const pindex, int nCount) const { if (nCount < 0 ) { return {}; } - nCount = std::min(nCount, int(GetValidWeightedMNsCount())); + const auto weighted_count = GetValidWeightedMNsCount(); + nCount = std::min(nCount, int(weighted_count)); std::vector result; - result.reserve(nCount); + result.reserve(weighted_count); - auto remaining_evo_payments = 0; - CDeterministicMNCPtr evo_to_be_skipped = nullptr; - bool isMNRewardReallocation = llmq::utils::IsMNRewardReallocationActive(pindex); - ForEachMNShared(true, [&](const CDeterministicMNCPtr& dmn) { - if (dmn->pdmnState->nLastPaidHeight == nHeight) { - // We found the last MN Payee. - // If the last payee is an EvoNode, we need to check its consecutive payments and pay him again if needed - if (!isMNRewardReallocation && dmn->nType == MnType::Evo && dmn->pdmnState->nConsecutivePayments < dmn_types::Evo.voting_weight) { - remaining_evo_payments = dmn_types::Evo.voting_weight - dmn->pdmnState->nConsecutivePayments; - for ([[maybe_unused]] auto _ : irange::range(remaining_evo_payments)) { - result.emplace_back(dmn); - evo_to_be_skipped = dmn; + int remaining_evo_payments{0}; + CDeterministicMNCPtr evo_to_be_skipped{nullptr}; + const bool isMNRewardReallocation = llmq::utils::IsMNRewardReallocationActive(pindex); + if (!isMNRewardReallocation) { + ForEachMNShared(true, [&](const CDeterministicMNCPtr& dmn) { + if (dmn->pdmnState->nLastPaidHeight == nHeight) { + // We found the last MN Payee. + // If the last payee is an EvoNode, we need to check its consecutive payments and pay him again if needed + if (dmn->nType == MnType::Evo && dmn->pdmnState->nConsecutivePayments < dmn_types::Evo.voting_weight) { + remaining_evo_payments = dmn_types::Evo.voting_weight - dmn->pdmnState->nConsecutivePayments; + for ([[maybe_unused]] auto _ : irange::range(remaining_evo_payments)) { + result.emplace_back(dmn); + evo_to_be_skipped = dmn; + } } } - } - return; - }); + }); + } ForEachMNShared(true, [&](const CDeterministicMNCPtr& dmn) { if (dmn == evo_to_be_skipped) return; @@ -647,7 +644,7 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde // Don't hold cs while calling signals if (diff.HasChanges()) { GetMainSignals().NotifyMasternodeListChanged(false, oldList, diff, connman); - uiInterface.NotifyMasternodeListChanged(newList); + uiInterface.NotifyMasternodeListChanged(newList, pindex); } if (nHeight == consensusParams.DIP0003EnforcementHeight) { @@ -688,7 +685,7 @@ bool CDeterministicMNManager::UndoBlock(const CBlockIndex* pindex) if (diff.HasChanges()) { auto inversedDiff = curList.BuildDiff(prevList); GetMainSignals().NotifyMasternodeListChanged(true, curList, inversedDiff, connman); - uiInterface.NotifyMasternodeListChanged(prevList); + uiInterface.NotifyMasternodeListChanged(prevList, pindex->pprev); } const auto& consensusParams = Params().GetConsensus(); diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 7d56e3d10331f..91d1192d8f35a 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -348,7 +348,6 @@ class CDeterministicMNList * @return */ [[nodiscard]] std::vector GetProjectedMNPayees(const CBlockIndex* const pindex, int nCount = std::numeric_limits::max()) const; - [[nodiscard]] std::vector GetProjectedMNPayeesAtChainTip(int nCount = std::numeric_limits::max()) const; /** * Calculate a quorum based on the modifier. The resulting list is deterministically sorted by score diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 0c4952309b07d..3e42b6a09965c 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -676,8 +676,10 @@ void CGovernanceManager::CreateGovernanceTrigger(const CSuperblock& sb, CConnman if (identical_sb == nullptr) { // Nobody submitted a trigger we'd like to see, // so let's do it but only if we are the payee - auto mnList = deterministicMNManager->GetListAtChainTip(); - auto mn_payees = mnList.GetProjectedMNPayeesAtChainTip(); + + const CBlockIndex *tip = WITH_LOCK(::cs_main, return ::ChainActive().Tip()); + auto mnList = deterministicMNManager->GetListForBlock(tip); + auto mn_payees = mnList.GetProjectedMNPayees(tip); if (mn_payees.empty()) return; { LOCK(activeMasternodeInfoCs); diff --git a/src/interfaces/node.h b/src/interfaces/node.h index cd50480b07089..720a5c97a4339 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -21,6 +21,7 @@ #include class BanMan; +class CBlockIndex; class CCoinControl; class CDeterministicMNList; class CFeeRate; @@ -45,7 +46,7 @@ class EVO { public: virtual ~EVO() {} - virtual CDeterministicMNList getListAtChainTip() = 0; + virtual std::pair getListAtChainTip() = 0; }; //! Interface for the src/governance part of a dash node (dashd process). @@ -355,7 +356,8 @@ class Node //! Register handler for masternode list update messages. using NotifyMasternodeListChangedFn = - std::function; + std::function; virtual std::unique_ptr handleNotifyMasternodeListChanged(NotifyMasternodeListChangedFn fn) = 0; //! Register handler for additional data sync progress update messages. diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 874c6e9f50316..cb94992a27abb 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -79,9 +79,14 @@ namespace { class EVOImpl : public EVO { public: - CDeterministicMNList getListAtChainTip() override + std::pair getListAtChainTip() override { - return deterministicMNManager == nullptr ? CDeterministicMNList() : deterministicMNManager->GetListAtChainTip(); + const CBlockIndex *tip = WITH_LOCK(::cs_main, return ::ChainActive().Tip()); + CDeterministicMNList mnList{}; + if (tip != nullptr && deterministicMNManager != nullptr) { + mnList = deterministicMNManager->GetListForBlock(tip); + } + return {std::move(mnList), tip}; } }; @@ -499,8 +504,8 @@ class NodeImpl : public Node std::unique_ptr handleNotifyMasternodeListChanged(NotifyMasternodeListChangedFn fn) override { return MakeHandler( - ::uiInterface.NotifyMasternodeListChanged_connect([fn](const CDeterministicMNList& newList) { - fn(newList); + ::uiInterface.NotifyMasternodeListChanged_connect([fn](const CDeterministicMNList& newList, const CBlockIndex* pindex) { + fn(newList, pindex); })); } std::unique_ptr handleNotifyAdditionalDataSyncProgressChanged(NotifyAdditionalDataSyncProgressChangedFn fn) override diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 24a805852444a..8491b64170615 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -82,26 +82,29 @@ int ClientModel::getNumConnections(unsigned int flags) const return m_node.getNodeCount(connections); } -void ClientModel::setMasternodeList(const CDeterministicMNList& mnList) +void ClientModel::setMasternodeList(const CDeterministicMNList& mnList, const CBlockIndex* tip) { LOCK(cs_mnlinst); if (mnListCached->GetBlockHash() == mnList.GetBlockHash()) { return; } mnListCached = std::make_shared(mnList); + mnListTip = tip; Q_EMIT masternodeListChanged(); } -CDeterministicMNList ClientModel::getMasternodeList() const +std::pair ClientModel::getMasternodeList() const { LOCK(cs_mnlinst); - return *mnListCached; + return {*mnListCached, mnListTip}; } void ClientModel::refreshMasternodeList() { + auto [mnList, tip] = m_node.evo().getListAtChainTip(); + LOCK(cs_mnlinst); - setMasternodeList(m_node.evo().getListAtChainTip()); + setMasternodeList(mnList, tip); } int ClientModel::getHeaderTipHeight() const @@ -332,9 +335,9 @@ static void NotifyChainLock(ClientModel *clientmodel, const std::string& bestCha assert(invoked); } -static void NotifyMasternodeListChanged(ClientModel *clientmodel, const CDeterministicMNList& newList) +static void NotifyMasternodeListChanged(ClientModel *clientmodel, const CDeterministicMNList& newList, const CBlockIndex* pindex) { - clientmodel->setMasternodeList(newList); + clientmodel->setMasternodeList(newList, pindex); } static void NotifyAdditionalDataSyncProgressChanged(ClientModel *clientmodel, double nSyncProgress) @@ -355,7 +358,7 @@ void ClientModel::subscribeToCoreSignals() m_handler_notify_block_tip = m_node.handleNotifyBlockTip(std::bind(BlockTipChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, false)); m_handler_notify_chainlock = m_node.handleNotifyChainLock(std::bind(NotifyChainLock, this, std::placeholders::_1, std::placeholders::_2)); m_handler_notify_header_tip = m_node.handleNotifyHeaderTip(std::bind(BlockTipChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, true)); - m_handler_notify_masternodelist_changed = m_node.handleNotifyMasternodeListChanged(std::bind(NotifyMasternodeListChanged, this, std::placeholders::_1)); + m_handler_notify_masternodelist_changed = m_node.handleNotifyMasternodeListChanged(std::bind(NotifyMasternodeListChanged, this, std::placeholders::_1, std::placeholders::_2)); m_handler_notify_additional_data_sync_progess_changed = m_node.handleNotifyAdditionalDataSyncProgressChanged(std::bind(NotifyAdditionalDataSyncProgressChanged, this, std::placeholders::_1)); } diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index 9af75010351ac..01238cba36969 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -67,8 +67,8 @@ class ClientModel : public QObject int getHeaderTipHeight() const; int64_t getHeaderTipTime() const; - void setMasternodeList(const CDeterministicMNList& mnList); - CDeterministicMNList getMasternodeList() const; + void setMasternodeList(const CDeterministicMNList& mnList, const CBlockIndex* tip); + std::pair getMasternodeList() const; void refreshMasternodeList(); void getAllGovernanceObjects(std::vector &obj); @@ -119,6 +119,7 @@ class ClientModel : public QObject // representation of the list in UI during initial sync/reindex, so we cache it here too. mutable RecursiveMutex cs_mnlinst; // protects mnListCached CDeterministicMNListPtr mnListCached; + const CBlockIndex* mnListTip; void subscribeToCoreSignals(); void unsubscribeFromCoreSignals(); diff --git a/src/qt/governancelist.cpp b/src/qt/governancelist.cpp index 0ec47888b4364..f196bfd3325dd 100644 --- a/src/qt/governancelist.cpp +++ b/src/qt/governancelist.cpp @@ -348,7 +348,7 @@ void GovernanceList::updateProposalList() // A proposal is considered passing if (YES votes - NO votes) >= (Total Weight of Masternodes / 10), // count total valid (ENABLED) masternodes to determine passing threshold. // Need to query number of masternodes here with access to clientModel. - const int nWeightedMnCount = clientModel->getMasternodeList().GetValidWeightedMNsCount(); + const int nWeightedMnCount = clientModel->getMasternodeList().first.GetValidWeightedMNsCount(); const int nAbsVoteReq = std::max(Params().GetConsensus().nGovernanceMinQuorum, nWeightedMnCount / 10); proposalModel->setVotingParams(nAbsVoteReq); diff --git a/src/qt/masternodelist.cpp b/src/qt/masternodelist.cpp index 2eb27c69b060d..36ca8ae1de486 100644 --- a/src/qt/masternodelist.cpp +++ b/src/qt/masternodelist.cpp @@ -166,7 +166,15 @@ void MasternodeList::updateDIP3List() return; } - auto mnList = clientModel->getMasternodeList(); + auto [mnList, pindex] = clientModel->getMasternodeList(); + auto projectedPayees = mnList.GetProjectedMNPayees(pindex); + + if (projectedPayees.empty() && mnList.GetValidMNsCount() > 0) { + // GetProjectedMNPayees failed to provide results for a list with valid mns. + // Keep current list and let it try again later. + return; + } + std::map mapCollateralDests; { @@ -191,7 +199,6 @@ void MasternodeList::updateDIP3List() nTimeUpdatedDIP3 = GetTime(); - auto projectedPayees = mnList.GetProjectedMNPayeesAtChainTip(); std::map nextPayments; for (size_t i = 0; i < projectedPayees.size(); i++) { const auto& dmn = projectedPayees[i]; @@ -222,7 +229,7 @@ void MasternodeList::updateDIP3List() QByteArray addr_ba(reinterpret_cast(addr_key.data()), addr_key.size()); QTableWidgetItem* addressItem = new CMasternodeListWidgetItem(QString::fromStdString(dmn.pdmnState->addr.ToString()), addr_ba); QTableWidgetItem* typeItem = new QTableWidgetItem(QString::fromStdString(std::string(GetMnType(dmn.nType).description))); - QTableWidgetItem* statusItem = new QTableWidgetItem(mnList.IsMNValid(dmn) ? tr("ENABLED") : (mnList.IsMNPoSeBanned(dmn) ? tr("POSE_BANNED") : tr("UNKNOWN"))); + QTableWidgetItem* statusItem = new QTableWidgetItem(dmn.pdmnState->IsBanned() ? tr("POSE_BANNED") : tr("ENABLED")); QTableWidgetItem* PoSeScoreItem = new CMasternodeListWidgetItem(QString::number(dmn.pdmnState->nPoSePenalty), dmn.pdmnState->nPoSePenalty); QTableWidgetItem* registeredItem = new CMasternodeListWidgetItem(QString::number(dmn.pdmnState->nRegisteredHeight), dmn.pdmnState->nRegisteredHeight); QTableWidgetItem* lastPaidItem = new CMasternodeListWidgetItem(QString::number(dmn.pdmnState->nLastPaidHeight), dmn.pdmnState->nLastPaidHeight); @@ -349,8 +356,7 @@ CDeterministicMNCPtr MasternodeList::GetSelectedDIP3MN() uint256 proTxHash; proTxHash.SetHex(strProTxHash); - auto mnList = clientModel->getMasternodeList(); - return mnList.GetMN(proTxHash); + return clientModel->getMasternodeList().first.GetMN(proTxHash);; } void MasternodeList::extraInfoDIP3_clicked() diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 4c1514874c130..7eab9f8457b3c 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -967,7 +967,7 @@ void RPCConsole::updateMasternodeCount() if (!clientModel) { return; } - auto mnList = clientModel->getMasternodeList(); + auto mnList = clientModel->getMasternodeList().first; size_t total_mn_count = mnList.GetAllMNsCount(); size_t total_enabled_mn_count = mnList.GetValidMNsCount(); size_t total_evo_count = mnList.GetAllEvoCount(); @@ -1247,7 +1247,7 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) ui->peerHeight->setText(QString::number(stats->nodeStats.nStartingHeight)); ui->peerWhitelisted->setText(stats->nodeStats.m_legacyWhitelisted ? tr("Yes") : tr("No")); ui->peerMappedAS->setText(stats->nodeStats.m_mapped_as != 0 ? QString::number(stats->nodeStats.m_mapped_as) : tr("N/A")); - auto dmn = clientModel->getMasternodeList().GetMNByService(stats->nodeStats.addr); + auto dmn = clientModel->getMasternodeList().first.GetMNByService(stats->nodeStats.addr); if (dmn == nullptr) { ui->peerNodeType->setText(tr("Regular")); ui->peerPoSeScore->setText(tr("N/A")); diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index d3182caa22312..e3403dc90a14c 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -135,8 +135,9 @@ static UniValue masternode_count(const JSONRPCRequest& request) static UniValue GetNextMasternodeForPayment(int heightShift) { - auto mnList = deterministicMNManager->GetListAtChainTip(); - auto payees = mnList.GetProjectedMNPayeesAtChainTip(heightShift); + const CBlockIndex *tip = WITH_LOCK(::cs_main, return ::ChainActive().Tip()); + auto mnList = deterministicMNManager->GetListForBlock(tip); + auto payees = mnList.GetProjectedMNPayees(tip, heightShift); if (payees.empty()) return "unknown"; auto payee = payees.back(); diff --git a/src/ui_interface.cpp b/src/ui_interface.cpp index c702698818942..c53bedf2b9707 100644 --- a/src/ui_interface.cpp +++ b/src/ui_interface.cpp @@ -58,7 +58,7 @@ void CClientUIInterface::ShowProgress(const std::string& title, int nProgress, b void CClientUIInterface::NotifyBlockTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyBlockTip(b, i); } void CClientUIInterface::NotifyChainLock(const std::string& bestChainLockHash, int bestChainLockHeight) { return g_ui_signals.NotifyChainLock(bestChainLockHash, bestChainLockHeight); } void CClientUIInterface::NotifyHeaderTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyHeaderTip(b, i); } -void CClientUIInterface::NotifyMasternodeListChanged(const CDeterministicMNList& list) { return g_ui_signals.NotifyMasternodeListChanged(list); } +void CClientUIInterface::NotifyMasternodeListChanged(const CDeterministicMNList& list, const CBlockIndex* i) { return g_ui_signals.NotifyMasternodeListChanged(list, i); } void CClientUIInterface::NotifyAdditionalDataSyncProgressChanged(double nSyncProgress) { return g_ui_signals.NotifyAdditionalDataSyncProgressChanged(nSyncProgress); } void CClientUIInterface::BannedListChanged() { return g_ui_signals.BannedListChanged(); } diff --git a/src/ui_interface.h b/src/ui_interface.h index c0e8aadcf4e26..0e6226986942f 100644 --- a/src/ui_interface.h +++ b/src/ui_interface.h @@ -118,7 +118,7 @@ class CClientUIInterface ADD_SIGNALS_DECL_WRAPPER(NotifyHeaderTip, void, bool, const CBlockIndex*); /** Masternode list has changed */ - ADD_SIGNALS_DECL_WRAPPER(NotifyMasternodeListChanged, void, const CDeterministicMNList&); + ADD_SIGNALS_DECL_WRAPPER(NotifyMasternodeListChanged, void, const CDeterministicMNList&, const CBlockIndex*); /** Additional data sync progress changed */ ADD_SIGNALS_DECL_WRAPPER(NotifyAdditionalDataSyncProgressChanged, void, double nSyncProgress);