Merge #1930: [RPC] Speeding up listmasternodes command

872da91 Speedup listmasternodes removing a redundant for loop over every MN on the mnnodeman. (furszy)
ea434a1 CMasternodeMan::GetMasternodeRanks removed unneeded MN extra validations and totally unnecessary extra vector load. (furszy)

Pull request description:

  Built on top of #1904 . PR starting on 9123d22.

  `listmasternodes` RPC command was taking a considerable amount of time, checked the flow and discovered a lot of redundancies in the process. This PR aims to correct them.

   1) `CMasternodeMan::GetMasternodeRanks` is only used for an RPC call and it was checking and validating each Masternode instead of just retrieve them as it suppose to be doing (the tier two thread is the only one responsible of check and update the masternodes list).

  2) `CMasternodeMan::GetMasternodeRanks` had a totally unneeded second vector load. Looping over the entire masternodes list one more time after getting it.

  3) `listmasternodes` after calling `GetMasternodeRanks` was looking for each of the retrieved masternodes one more time using the retrieved masternode vin (yes..).

ACKs for top commit:
  random-zebra:
    ACK 872da91
  Fuzzbawls:
    ACK 872da91

Tree-SHA512: 52db4488ec429594a8fa6cacceaaf494ec0da179929b7de30f8698cbbb2d940c0d859f8a0082dce33ead5227066079011f30480434ab935906e12aa80b4199a3

src/masternodeman.cpp

@@ -616,41 +616,27 @@ int CMasternodeMan::GetMasternodeRank(const CTxIn& vin, int64_t nBlockHeight, in
     return -1;
 }
 
-std::vector<std::pair<int, CMasternode> > CMasternodeMan::GetMasternodeRanks(int64_t nBlockHeight, int minProtocol)
+std::vector<std::pair<int64_t, CMasternode>> CMasternodeMan::GetMasternodeRanks(int nBlockHeight)
 {
-    std::vector<std::pair<int64_t, CMasternode> > vecMasternodeScores;
-    std::vector<std::pair<int, CMasternode> > vecMasternodeRanks;
-
+    std::vector<std::pair<int64_t, CMasternode>> vecMasternodeScores;
     const uint256& hash = GetHashAtHeight(nBlockHeight - 1);
     // height outside range
-    if (!hash) return vecMasternodeRanks;
-
-    // scan for winner
-    for (CMasternode& mn : vMasternodes) {
-        mn.Check();
-
-        if (mn.protocolVersion < minProtocol) continue;
+    if (!hash) return vecMasternodeScores;
+    {
+        LOCK(cs);
+        // scan for winner
+        for (CMasternode& mn : vMasternodes) {
+            if (!mn.IsEnabled()) {
+                vecMasternodeScores.emplace_back(9999, mn);
+                continue;
+            }
 
-        if (!mn.IsEnabled()) {
-            vecMasternodeScores.emplace_back(9999, mn);
-            continue;
+            int64_t n2 = mn.CalculateScore(hash).GetCompact(false);
+            vecMasternodeScores.emplace_back(n2, mn);
         }
-
-        uint256 n = mn.CalculateScore(hash);
-        int64_t n2 = n.GetCompact(false);
-
-        vecMasternodeScores.emplace_back(n2, mn);
     }
-
     sort(vecMasternodeScores.rbegin(), vecMasternodeScores.rend(), CompareScoreMN());
-
-    int rank = 0;
-    for (PAIRTYPE(int64_t, CMasternode) & s : vecMasternodeScores) {
-        rank++;
-        vecMasternodeRanks.emplace_back(rank, s.second);
-    }
-
-    return vecMasternodeRanks;
+    return vecMasternodeScores;
 }
 
 void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataStream& vRecv)

src/masternodeman.h

@@ -146,8 +146,8 @@ class CMasternodeMan
         Check();
         return vMasternodes;
     }
-
-    std::vector<std::pair<int, CMasternode> > GetMasternodeRanks(int64_t nBlockHeight, int minProtocol = 0);
+    // Retrieve the known masternodes ordered by scoring without checking them. (Only used for listmasternodes RPC call)
+    std::vector<std::pair<int64_t, CMasternode>> GetMasternodeRanks(int nBlockHeight);
     int GetMasternodeRank(const CTxIn& vin, int64_t nBlockHeight, int minProtocol = 0, bool fOnlyActive = true);
 
     void ProcessMessage(CNode* pfrom, std::string& strCommand, CDataStream& vRecv);

src/rpc/masternode.cpp

@@ -81,42 +81,41 @@ UniValue listmasternodes(const JSONRPCRequest& request)
     int nHeight = WITH_LOCK(cs_main, return chainActive.Height());
     if (nHeight < 0) return "[]";
 
-    std::vector<std::pair<int, CMasternode> > vMasternodeRanks = mnodeman.GetMasternodeRanks(nHeight);
-    for (PAIRTYPE(int, CMasternode) & s : vMasternodeRanks) {
+    std::vector<std::pair<int64_t, CMasternode>> vMasternodeRanks = mnodeman.GetMasternodeRanks(nHeight);
+    for (int pos=0; pos < (int) vMasternodeRanks.size(); pos++) {
+        const auto& s = vMasternodeRanks[pos];
         UniValue obj(UniValue::VOBJ);
-        std::string strVin = s.second.vin.prevout.ToStringShort();
-        std::string strTxHash = s.second.vin.prevout.hash.ToString();
-        uint32_t oIdx = s.second.vin.prevout.n;
-
-        CMasternode* mn = mnodeman.Find(s.second.vin);
-
-        if (mn != NULL) {
-            if (strFilter != "" && strTxHash.find(strFilter) == std::string::npos &&
-                mn->Status().find(strFilter) == std::string::npos &&
-                EncodeDestination(mn->pubKeyCollateralAddress.GetID()).find(strFilter) == std::string::npos) continue;
-
-            std::string strStatus = mn->Status();
-            std::string strHost;
-            int port;
-            SplitHostPort(mn->addr.ToString(), port, strHost);
-            CNetAddr node;
-            LookupHost(strHost.c_str(), node, false);
-            std::string strNetwork = GetNetworkName(node.GetNetwork());
-
-            obj.pushKV("rank", (strStatus == "ENABLED" ? s.first : 0));
-            obj.pushKV("network", strNetwork);
-            obj.pushKV("txhash", strTxHash);
-            obj.pushKV("outidx", (uint64_t)oIdx);
-            obj.pushKV("pubkey", HexStr(mn->pubKeyMasternode));
-            obj.pushKV("status", strStatus);
-            obj.pushKV("addr", EncodeDestination(mn->pubKeyCollateralAddress.GetID()));
-            obj.pushKV("version", mn->protocolVersion);
-            obj.pushKV("lastseen", (int64_t)mn->lastPing.sigTime);
-            obj.pushKV("activetime", (int64_t)(mn->lastPing.sigTime - mn->sigTime));
-            obj.pushKV("lastpaid", (int64_t)mn->GetLastPaid());
+        CMasternode mn = s.second;
+
+        std::string strVin = mn.vin.prevout.ToStringShort();
+        std::string strTxHash = mn.vin.prevout.hash.ToString();
+        uint32_t oIdx = mn.vin.prevout.n;
+
+        if (strFilter != "" && strTxHash.find(strFilter) == std::string::npos &&
+            mn.Status().find(strFilter) == std::string::npos &&
+            EncodeDestination(mn.pubKeyCollateralAddress.GetID()).find(strFilter) == std::string::npos) continue;
+
+        std::string strStatus = mn.Status();
+        std::string strHost;
+        int port;
+        SplitHostPort(mn.addr.ToString(), port, strHost);
+        CNetAddr node;
+        LookupHost(strHost.c_str(), node, false);
+        std::string strNetwork = GetNetworkName(node.GetNetwork());
+
+        obj.pushKV("rank", (strStatus == "ENABLED" ? pos : 0));
+        obj.pushKV("network", strNetwork);
+        obj.pushKV("txhash", strTxHash);
+        obj.pushKV("outidx", (uint64_t)oIdx);
+        obj.pushKV("pubkey", HexStr(mn.pubKeyMasternode));
+        obj.pushKV("status", strStatus);
+        obj.pushKV("addr", EncodeDestination(mn.pubKeyCollateralAddress.GetID()));
+        obj.pushKV("version", mn.protocolVersion);
+        obj.pushKV("lastseen", (int64_t)mn.lastPing.sigTime);
+        obj.pushKV("activetime", (int64_t)(mn.lastPing.sigTime - mn.sigTime));
+        obj.pushKV("lastpaid", (int64_t)mn.GetLastPaid());
 
-            ret.push_back(obj);
-        }
+        ret.push_back(obj);
     }
 
     return ret;