Skip to content

Commit

Permalink
Don't use boost range adaptors in CDeterministicMNList (#2327)
Browse files Browse the repository at this point in the history 
These cause crashes when used in for loops. Reason is very likely that
temporary CDeterministicMNList objects are destroyed before entering the
loop while the adaptors rely on these objects.

Discovered in one of our devnets while calling "protx list"
  • Loading branch information
@codablock @UdjinM6
codablock authored and UdjinM6 committed Oct 2, 2018
1 parent 07208a4 commit eaef902
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 81 deletions.
15 changes: 8 additions & 7 deletions src/evo/deterministicmns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,11 @@ CDeterministicMNCPtr CDeterministicMNList::GetMNPayee() const
return nullptr;

CDeterministicMNCPtr best;
for (const auto& dmn : valid_range()) {
if (!best || CompareByLastPaid(dmn, best))
ForEachMN(true, [&](const CDeterministicMNCPtr& dmn) {
if (!best || CompareByLastPaid(dmn, best)) {
best = dmn;
}
}
});

return best;
}
Expand Down Expand Up @@ -310,8 +311,8 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde
evoDb.Write(std::make_pair(DB_LIST_DIFF, diff.blockHash), diff);
if ((nHeight % SNAPSHOT_LIST_PERIOD) == 0) {
evoDb.Write(std::make_pair(DB_LIST_SNAPSHOT, diff.blockHash), newList);
LogPrintf("CDeterministicMNManager::%s -- Wrote snapshot. nHeight=%d, mapCurMNs.size=%d\n",
__func__, nHeight, newList.size());
LogPrintf("CDeterministicMNManager::%s -- Wrote snapshot. nHeight=%d, mapCurMNs.allMNsCount=%d\n",
__func__, nHeight, newList.GetAllMNsCount());
}

if (nHeight == GetSpork15Value()) {
Expand Down Expand Up @@ -373,8 +374,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C
if (dmn && dmn->nCollateralIndex == in.prevout.n) {
newList.RemoveMN(proTxHash);

LogPrintf("CDeterministicMNManager::%s -- MN %s removed from list because collateral was spent. nHeight=%d, mapCurMNs.size=%d\n",
__func__, proTxHash.ToString(), nHeight, newList.size());
LogPrintf("CDeterministicMNManager::%s -- MN %s removed from list because collateral was spent. nHeight=%d, mapCurMNs.allMNsCount=%d\n",
__func__, proTxHash.ToString(), nHeight, newList.GetAllMNsCount());
}
}

Expand Down
36 changes: 5 additions & 31 deletions src/evo/deterministicmns.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@

#include <map>

#include <boost/range/adaptors.hpp>
#include <boost/range/any_range.hpp>

class CBlock;
class CBlockIndex;
class CValidationState;
Expand Down Expand Up @@ -220,41 +217,18 @@ class CDeterministicMNList

public:

size_t size() const
size_t GetAllMNsCount() const
{
return mnMap.size();
}

typedef boost::any_range<const CDeterministicMNCPtr&, boost::forward_traversal_tag> range_type;

range_type all_range() const
{
return boost::adaptors::transform(mnMap, [] (const MnMap::value_type& p) -> const CDeterministicMNCPtr& {
return p.second;
});
}

range_type valid_range() const
{
return boost::adaptors::filter(all_range(), [&] (const CDeterministicMNCPtr& dmn) -> bool {
return IsMNValid(dmn);
});
}

size_t all_count() const
{
return mnMap.size();
}

size_t valid_count() const
{
size_t c = 0;
template<typename Callback>
void ForEachMN(bool onlyValid, Callback&& cb) const {
for (const auto& p : mnMap) {
if (IsMNValid(p.second)) {
c++;
if (!onlyValid || IsMNValid(p.second)) {
cb(p.second);
}
}
return c;
}

public:
Expand Down
6 changes: 3 additions & 3 deletions src/evo/simplifiedmns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ CSimplifiedMNList::CSimplifiedMNList(const std::vector<CSimplifiedMNListEntry>&

CSimplifiedMNList::CSimplifiedMNList(const CDeterministicMNList& dmnList)
{
mnList.reserve(dmnList.all_count());
mnList.reserve(dmnList.GetAllMNsCount());

for (const auto& dmn : dmnList.all_range()) {
dmnList.ForEachMN(false, [this](const CDeterministicMNCPtr& dmn) {
mnList.emplace_back(*dmn);
}
});

std::sort(mnList.begin(), mnList.end(), [&](const CSimplifiedMNListEntry& a, const CSimplifiedMNListEntry& b) {
return a.proRegTxHash.Compare(b.proRegTxHash) < 0;
Expand Down
19 changes: 10 additions & 9 deletions src/masternodeman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ void CMasternodeMan::AddDeterministicMasternodes()
unsigned int oldMnCount = mapMasternodes.size();

auto mnList = deterministicMNManager->GetListAtChainTip();
for (const auto& dmn : mnList.valid_range()) {
mnList.ForEachMN(true, [this](const CDeterministicMNCPtr& dmn) {
// call Find() on each deterministic MN to force creation of CMasternode object
auto mn = Find(COutPoint(dmn->proTxHash, dmn->nCollateralIndex));
assert(mn);
Expand All @@ -395,7 +395,7 @@ void CMasternodeMan::AddDeterministicMasternodes()

// If it appeared in the valid list, it is enabled no matter what
mn->nActiveState = CMasternode::MASTERNODE_ENABLED;
}
});

added = oldMnCount != mapMasternodes.size();
}
Expand All @@ -415,9 +415,9 @@ void CMasternodeMan::RemoveNonDeterministicMasternodes()
LOCK(cs);
std::set<COutPoint> mnSet;
auto mnList = deterministicMNManager->GetListAtChainTip();
for (const auto& dmn : mnList.valid_range()) {
mnList.ForEachMN(true, [&](const CDeterministicMNCPtr& dmn) {
mnSet.insert(COutPoint(dmn->proTxHash, dmn->nCollateralIndex));
}
});
auto it = mapMasternodes.begin();
while (it != mapMasternodes.end()) {
if (!mnSet.count(it->second.outpoint)) {
Expand Down Expand Up @@ -455,10 +455,11 @@ int CMasternodeMan::CountMasternodes(int nProtocolVersion)

if (deterministicMNManager->IsDeterministicMNsSporkActive()) {
auto mnList = deterministicMNManager->GetListAtChainTip();
for (const auto& dmn : mnList.valid_range()) {
if (dmn->pdmnState->nProtocolVersion < nProtocolVersion) continue;
nCount++;
}
mnList.ForEachMN(true, [&](const CDeterministicMNCPtr& dmn) {
if (dmn->pdmnState->nProtocolVersion >= nProtocolVersion) {
nCount++;
}
});
} else {
for (const auto& mnpair : mapMasternodes) {
if(mnpair.second.nProtocolVersion < nProtocolVersion) continue;
Expand Down Expand Up @@ -1693,7 +1694,7 @@ std::string CMasternodeMan::ToString() const

if (deterministicMNManager->IsDeterministicMNsSporkActive()) {
info << "Masternodes: masternode object count: " << (int)mapMasternodes.size() <<
", deterministic masternode count: " << deterministicMNManager->GetListAtChainTip().size() <<
", deterministic masternode count: " << deterministicMNManager->GetListAtChainTip().GetAllMNsCount() <<
", nDsqCount: " << (int)nDsqCount;
} else {
info << "Masternodes: " << (int)mapMasternodes.size() <<
Expand Down
20 changes: 9 additions & 11 deletions src/rpc/governance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ UniValue gobject_vote_many(const JSONRPCRequest& request)
// management
if (deterministicMNManager->IsDeterministicMNsSporkActive()) {
auto mnList = deterministicMNManager->GetListAtChainTip();
for (const auto &dmn : mnList.valid_range()) {
mnList.ForEachMN(true, [&](const CDeterministicMNCPtr& dmn) {
bool found = false;
for (const auto &mne : entries) {
uint256 nTxHash;
Expand All @@ -595,17 +595,15 @@ UniValue gobject_vote_many(const JSONRPCRequest& request)
break;
}
}
if (found) {
continue;
}

CKey ownerKey;
if (pwalletMain->GetKey(dmn->pdmnState->keyIDVoting, ownerKey)) {
CBitcoinSecret secret(ownerKey);
CMasternodeConfig::CMasternodeEntry mne(dmn->proTxHash.ToString(), dmn->pdmnState->addr.ToStringIPPort(false), secret.ToString(), dmn->proTxHash.ToString(), itostr(dmn->nCollateralIndex));
entries.push_back(mne);
if (!found) {
CKey ownerKey;
if (pwalletMain->GetKey(dmn->pdmnState->keyIDVoting, ownerKey)) {
CBitcoinSecret secret(ownerKey);
CMasternodeConfig::CMasternodeEntry mne(dmn->proTxHash.ToString(), dmn->pdmnState->addr.ToStringIPPort(false), secret.ToString(), dmn->proTxHash.ToString(), itostr(dmn->nCollateralIndex));
entries.push_back(mne);
}
}
}
});
}
#endif

Expand Down
26 changes: 10 additions & 16 deletions src/rpc/rpcevo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,16 +519,16 @@ UniValue protx_list(const JSONRPCRequest& request)
setOutpts.emplace(outpt.hash);
}

for (const auto& dmn : deterministicMNManager->GetListAtChainTip().all_range()) {
deterministicMNManager->GetListAtChainTip().ForEachMN(false, [&](const CDeterministicMNCPtr& dmn) {
if (setOutpts.count(dmn->proTxHash) ||
pwalletMain->HaveKey(dmn->pdmnState->keyIDOwner) ||
pwalletMain->HaveKey(dmn->pdmnState->keyIDOperator) ||
pwalletMain->HaveKey(dmn->pdmnState->keyIDVoting) ||
CheckWalletOwnsScript(dmn->pdmnState->scriptPayout) ||
CheckWalletOwnsScript(dmn->pdmnState->scriptOperatorPayout)) {
pwalletMain->HaveKey(dmn->pdmnState->keyIDOwner) ||
pwalletMain->HaveKey(dmn->pdmnState->keyIDOperator) ||
pwalletMain->HaveKey(dmn->pdmnState->keyIDVoting) ||
CheckWalletOwnsScript(dmn->pdmnState->scriptPayout) ||
CheckWalletOwnsScript(dmn->pdmnState->scriptOperatorPayout)) {
ret.push_back(BuildDMNListEntry(dmn, detailed));
}
}
});
} else if (type == "valid" || type == "registered") {
if (request.params.size() > 4)
protx_list_help();
Expand All @@ -542,16 +542,10 @@ UniValue protx_list(const JSONRPCRequest& request)
bool detailed = request.params.size() > 3 ? ParseBoolV(request.params[3], "detailed") : false;

CDeterministicMNList mnList = deterministicMNManager->GetListForBlock(chainActive[height]->GetBlockHash());
CDeterministicMNList::range_type range;

if (type == "valid") {
range = mnList.valid_range();
} else if (type == "registered") {
range = mnList.all_range();
}
for (const auto& dmn : range) {
bool onlyValid = type == "valid";
mnList.ForEachMN(onlyValid, [&](const CDeterministicMNCPtr& dmn) {
ret.push_back(BuildDMNListEntry(dmn, detailed));
}
});
} else {
throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid type specified");
}
Expand Down
12 changes: 8 additions & 4 deletions src/test/evo_deterministicmns_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,15 @@ static CDeterministicMNCPtr FindPayoutDmn(const CBlock& block)
{
auto dmnList = deterministicMNManager->GetListAtChainTip();

for (auto txout : block.vtx[0]->vout) {
for (auto& dmn : dmnList.valid_range()) {
if (txout.scriptPubKey == dmn->pdmnState->scriptPayout) {
return dmn;
for (const auto& txout : block.vtx[0]->vout) {
CDeterministicMNCPtr found;
dmnList.ForEachMN(true, [&](const CDeterministicMNCPtr& dmn) {
if (found == nullptr && txout.scriptPubKey == dmn->pdmnState->scriptPayout) {
found = dmn;
}
});
if (found != nullptr) {
return found;
}
}
return nullptr;
Expand Down

0 comments on commit eaef902

Please sign in to comment.