Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Various fixes for GetProjectedMNPayees #5638

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 47 additions & 21 deletions src/evo/deterministicmns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,38 +214,64 @@ CDeterministicMNCPtr CDeterministicMNList::GetMNPayee(const CBlockIndex* pIndex)
return best;
}

std::vector<CDeterministicMNCPtr> CDeterministicMNList::GetProjectedMNPayeesAtChainTip(int nCount) const
{
return GetProjectedMNPayees(::ChainActive()[nHeight], nCount);
}

std::vector<CDeterministicMNCPtr> CDeterministicMNList::GetProjectedMNPayees(const CBlockIndex* const pindex, int nCount) const
std::vector<CDeterministicMNCPtr> CDeterministicMNList::GetProjectedMNPayees(int nCount) const
{
if (nCount < 0 ) {
return {};
}
nCount = std::min(nCount, int(GetValidWeightedMNsCount()));

bool isMNRewardReallocation{false};
if (auto pindex = ::ChainActive()[nHeight]; pindex == nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like using here global variable ::ChainActive().Tip().
Also there can be 2 different chain (2 forks on same time) and extracting pindex by [nHeight] can be incorrect value. if after mnList is generated the chain would be switched to other one; the value of ::ChainActive()[nHeight] would be not expected and list would be wrong.

For example call below:

auto projection = deterministicMNManager->GetListForBlock(pindexTip).GetProjectedMNPayees(20);

pindexTip is known but we lost this information.

btw, GetProjecctedMNPayees is just estimation list; is not used in consensus rules; so, I don't think that we need to consider case of LOCKED_IN to make calculation 100% exact for case that happens only once in chain - during MN_RR activation, I vote for simplicity here.

Copy link
Collaborator

@knst knst Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, if we will change exact known pindex by using ::ChainActive()[nHeight] it may cause similar bug not only in GUI code but in all usages of GetProjecctedMNPayees (that are non-GUI only), so in this case it can cause this issue getting bigger rather than beeing solved and we won't see it anymore because no crash -> seems works (but it does not). So, now I think that call g_chainman.m_blockman.LookupBlockIndex(blockHash)) is a proper solution until the BIP9 fork is buried (and we can use hardened height and drop g_chainman usage from here)

// Something went wrong, probably a race condition in tip and mnlist updates,
// try recovering...
pindex = ::ChainActive().Tip();
const auto mn_rr_state = llmq::utils::GetMNRewardReallocationState(pindex);
isMNRewardReallocation = (mn_rr_state == ThresholdState::ACTIVE);
if (!isMNRewardReallocation) {
const auto w_size = static_cast<int>(Params().GetConsensus().vDeployments[Consensus::DEPLOYMENT_MN_RR].nWindowSize);
// Check if mn_rr is going to be active at nHeight
if (mn_rr_state == ThresholdState::LOCKED_IN) {
int activation_height = llmq::utils::GetMNRewardReallocationSince(pindex) + w_size;
if (nHeight >= activation_height) {
isMNRewardReallocation = true;
}
} else {
if (nHeight - pindex->nHeight > w_size) {
// Should never happen, can't do anything
return {};
}
// No way we reach mn_rr activation if it's not locked in yet
// and height diff is less than or equal w_size, so isMNRewardReallocation == false
// but it's safe to continue nevertheless.
}
}
} else {
isMNRewardReallocation = llmq::utils::IsMNRewardReallocationActive(pindex);
}

const auto weighted_count = GetValidWeightedMNsCount();
nCount = std::min(nCount, int(weighted_count));

std::vector<CDeterministicMNCPtr> 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;
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;
Expand Down
3 changes: 1 addition & 2 deletions src/evo/deterministicmns.h
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,7 @@ class CDeterministicMNList
* @param nCount the number of payees to return. "nCount = max()"" means "all", use it to avoid calling GetValidWeightedMNsCount twice.
* @return
*/
[[nodiscard]] std::vector<CDeterministicMNCPtr> GetProjectedMNPayees(const CBlockIndex* const pindex, int nCount = std::numeric_limits<int>::max()) const;
[[nodiscard]] std::vector<CDeterministicMNCPtr> GetProjectedMNPayeesAtChainTip(int nCount = std::numeric_limits<int>::max()) const;
[[nodiscard]] std::vector<CDeterministicMNCPtr> GetProjectedMNPayees(int nCount = std::numeric_limits<int>::max()) const;

/**
* Calculate a quorum based on the modifier. The resulting list is deterministically sorted by score
Expand Down
2 changes: 1 addition & 1 deletion src/governance/governance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ void CGovernanceManager::CreateGovernanceTrigger(const CSuperblock& sb, CConnman
// 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();
auto mn_payees = mnList.GetProjectedMNPayees();
if (mn_payees.empty()) return;
{
LOCK(activeMasternodeInfoCs);
Expand Down
9 changes: 8 additions & 1 deletion src/qt/masternodelist.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,14 @@ void MasternodeList::updateDIP3List()
}

auto mnList = clientModel->getMasternodeList();
auto projectedPayees = mnList.GetProjectedMNPayees();

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<uint256, CTxDestination> mapCollateralDests;

{
Expand All @@ -192,7 +200,6 @@ void MasternodeList::updateDIP3List()

nTimeUpdatedDIP3 = GetTime();

auto projectedPayees = mnList.GetProjectedMNPayeesAtChainTip();
std::map<uint256, int> nextPayments;
for (size_t i = 0; i < projectedPayees.size(); i++) {
const auto& dmn = projectedPayees[i];
Expand Down
4 changes: 2 additions & 2 deletions src/rpc/masternode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ static UniValue masternode_count(const JSONRPCRequest& request)
static UniValue GetNextMasternodeForPayment(int heightShift)
{
auto mnList = deterministicMNManager->GetListAtChainTip();
auto payees = mnList.GetProjectedMNPayeesAtChainTip(heightShift);
auto payees = mnList.GetProjectedMNPayees(heightShift);
if (payees.empty())
return "unknown";
auto payee = payees.back();
Expand Down Expand Up @@ -360,7 +360,7 @@ static UniValue masternode_winners(const JSONRPCRequest& request, const Chainsta
obj.pushKV(strprintf("%d", h), strPayments);
}

auto projection = deterministicMNManager->GetListForBlock(pindexTip).GetProjectedMNPayees(pindexTip, 20);
auto projection = deterministicMNManager->GetListForBlock(pindexTip).GetProjectedMNPayees(20);
for (size_t i = 0; i < projection.size(); i++) {
int h = nChainTipHeight + 1 + i;
std::string strPayments = GetRequiredPaymentsString(h, projection[i]);
Expand Down