Fix masternode score/rank calculations #1620
Conversation
src/masternodeman.cpp
Outdated
| @@ -581,103 +581,100 @@ masternode_info_t CMasternodeMan::FindRandomNotInVec(const std::vector<COutPoint | |||
| return masternode_info_t(); | |||
| } | |||
|
|
|||
| int CMasternodeMan::GetMasternodeRank(const COutPoint& outpoint, int nBlockHeight, int nMinProtocol, bool fOnlyActive) | |||
| CMasternodeMan::score_pair_m_t CMasternodeMan::GetMasternodeScores(int nBlockHeight, uint256 blockHash, int nMinProtocol) | |||
There was a problem hiding this comment.
nBlockHeight is not used in this method
| @@ -16,7 +16,7 @@ | |||
| /** Masternode manager */ | |||
| CMasternodeMan mnodeman; | |||
|
|
|||
| const std::string CMasternodeMan::SERIALIZATION_VERSION_STRING = "CMasternodeMan-Version-6"; | |||
| const std::string CMasternodeMan::SERIALIZATION_VERSION_STRING = "CMasternodeMan-Version-7"; | |||
There was a problem hiding this comment.
0b614fe just bumped the version. Is the new bumping really required? Maybe due to people already using the current version on testnet I'd assume.
| @@ -90,6 +90,15 @@ bool CMasternode::UpdateFromNewBroadcast(CMasternodeBroadcast& mnb) | |||
| // | |||
| arith_uint256 CMasternode::CalculateScore(const uint256& blockHash) | |||
| { | |||
| if (fDIP0001LockedInAtTip) { | |||
There was a problem hiding this comment.
Is it really required to use DIP0001 activation here? IntantSend is disabled via spork at the moment and I'd assume you won't re-enable InstantSend before enough (nearly all) MNs have upgraded? This is completely independent from DIP0001 activation.
There was a problem hiding this comment.
Scores/ranks are also used in few other places and specifically, mn payments. IS spork is the last one to enable (or at least that's how we usually do). Payment enforcement spork should be kept disabled until most of MNs and miners are upgraded AND mn winners list is stable/consistent across network. Switching score algo via payment enforcement spork could be dangerous too, these things should be done separately imo.
So the idea was to use the "middle" state of DIP0001 activation (THRESHOLD_LOCKED_IN) - we should be pretty confident that majority of miners and MNs are upgraded at that point and it should be safe to switch the algo without being banned by the majority of masternodes and/or hashpower.
Alternatively we could introduce some new spork but this algo upgrade is one way only and we should never need to switch new algo off. Hopefully :)
There was a problem hiding this comment.
Ah I didn't consider the MN payments. So please ignore what I said :)
|
Regarding the new score calculation: 👍 |
src/masternodeman.cpp
Outdated
|
|
||
| // lock is required and should be acquired outside | ||
| AssertLockHeld(cs); | ||
| LOCK(cs); |
There was a problem hiding this comment.
Pulling the locking into this method (instead of forcing the caller to hold it), creates a race condition. This method returns MNs by pointer and not by value. As the caller does not hold the lock anymore, it's not guaranteed that the pointers are still valid.
There was a problem hiding this comment.
hah, I knew there was a reason why I did this in the first version :D
src/masternodeman.cpp
Outdated
| return -1; | ||
| } | ||
|
|
||
| LOCK(cs); |
There was a problem hiding this comment.
See above. The loop below may now use stall data.
src/masternodeman.cpp
Outdated
| return vecMasternodeRanks; | ||
| } | ||
|
|
||
| LOCK(cs); |
src/masternodeman.cpp
Outdated
|
|
||
| LOCK(cs); |
|
utACK |
…gered by DIP0001 lock-in
|
Rebased. No code changes, no conflicts, just to make sure IS logic is still valid here after #1592 merge. Looks good imo. |
|
While reviewing both PRs I also tried to find conflicts between both, nothing found. |
|
Looks like travis is happy too, merging |
This should fix two issues we have with scores/ranks currently:
CMasternode::CalculateScorevulnerability (2ba68b6);Speaking of mn list consensus, selecting non-enabled MNs doesn't degrade quorum quality too much because MNs should have 1000 DASH locked anyway and it's morel likely that they have some problems running MN/propagating pings rather than trying to perform a DoS attack this way. On the other hand, ignoring some live data like pings and wds should help us to stabilize mn list much quicker. Also note, these does NOT change the fact that only valid masternodes are voted for being payed (
GetNextMasternodeInQueueForPayment), it only affects (extends) the selection of masternodes who are voting.