Add locking

This commit adds locks in a few places that I am concerned
could be a problem if the underlying global map changes in
another thread, plus makes a few other cleenups.

Author: jamescowens

src/scraper/fwd.h

@@ -67,8 +67,14 @@ typedef std::map<ScraperID, mCSManifest> mmCSManifestsBinnedByScraper;
 // -------------- Project -- Converged Part Pointer
 typedef std::map<std::string, CSplitBlob::CPart*> mConvergedManifestPart_ptrs;
 
+// The below type is still used for situations where a long global lock would be held during stats processing
+// primarily in GetScraperStatsByConvergedManifest because of the intense work done in LoadProjectObjectToStatsByCPID.
+// -------------- Project ---- Converged Part
+typedef std::map<std::string, CSerializeData> mConvergedManifestParts;
+
 struct ConvergedManifest
 {
+    // Empty converged manifest constructor
     ConvergedManifest()
     {
         nContentHash = {};
@@ -96,9 +102,9 @@ struct ConvergedManifest
         vExcludedProjects = {};
     }
 
+    // Constructor of a passed in ConvergedManifest (effectively a copy)
     ConvergedManifest(const ConvergedManifest& in)
     {
-        // We can use the content hash from the specified converged manifest. We do not need to recompute it.
         nContentHash = in.nContentHash;
 
         ConsensusBlock = in.ConsensusBlock;
@@ -107,7 +113,7 @@ struct ConvergedManifest
 
         CScraperConvergedManifest_ptr = in.CScraperConvergedManifest_ptr;
 
-        PopulateConvergedManifestPartPtrsMap();
+        ConvergedManifestPartPtrsMap = in.ConvergedManifestPartPtrsMap;
 
         mIncludedScraperManifests = in.mIncludedScraperManifests;
 
@@ -142,6 +148,7 @@ struct ConvergedManifest
     }
 
 
+    // Call operator to update an already initialized ConvergedManifest with a passed in ConvergedManifest
     void operator()(ConvergedManifest& in)
     {
         // We can use the content hash from the specified converged manifest. We do not need to recompute it.
@@ -153,9 +160,7 @@ struct ConvergedManifest
 
         CScraperConvergedManifest_ptr = in.CScraperConvergedManifest_ptr;
 
-        PopulateConvergedManifestPartPtrsMap();
-
-        //ConvergedManifestPartsMap = in.ConvergedManifestPartsMap;
+        ConvergedManifestPartPtrsMap = in.ConvergedManifestPartPtrsMap;
 
         mIncludedScraperManifests = in.mIncludedScraperManifests;
 
@@ -173,6 +178,7 @@ struct ConvergedManifest
         vExcludedProjects = in.vExcludedProjects;
     }
 
+    // Call operator to update an already initialized ConvergedManifest with a passed in CScraperManifest
     bool operator()(const CScraperManifest& in)
     {
         ConsensusBlock = in.ConsensusBlock;
@@ -276,6 +282,8 @@ struct ConvergedManifest
             else
                 sProject = CScraperConvergedManifest_ptr->projects[iPartNum-1].project;
 
+            //if (sProject.empty()) return false;
+
             // Copy the pointer to the CPart into the map. This is ok, because the parts will be held
             // until the CScraperManifest in this object is destroyed and all of the manifest refs to the part
             // are gone.

src/scraper/scraper.cpp

@@ -3457,26 +3457,42 @@ ScraperStatsAndVerifiedBeacons GetScraperStatsByConvergedManifest(const Converge
 
     double dMagnitudePerProject = NEURALNETWORKMULTIPLIER / nActiveProjects;
 
+    mConvergedManifestParts ConvergedManifestPartsMap;
     ScraperStats mScraperStats;
 
-    for (auto entry = StructConvergedManifest.ConvergedManifestPartPtrsMap.begin(); entry != StructConvergedManifest.ConvergedManifestPartPtrsMap.end(); ++entry)
+    // This is a lock and copy block specifically because LoadProjectObjectToStatsByCPID is very expensive and in
+    // this particular case it will reduce lock time to make a copy of the parts temporarily for processing. We have
+    // to take these locks now because of the conversion of the StructConvergedManifest parts map to pointers.
     {
-        std::string project = entry->first;
-        ScraperStats mProjectScraperStats;
+        LOCK2(CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts);
+        _log(logattribute::INFO, "LOCK2", "CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts");
 
-        // Do not process the BeaconList or VerifiedBeacons as a project stats file.
-        if (project != "BeaconList" && project != "VerifiedBeacons")
+        for (auto entry = StructConvergedManifest.ConvergedManifestPartPtrsMap.begin(); entry != StructConvergedManifest.ConvergedManifestPartPtrsMap.end(); ++entry)
         {
-            _log(logattribute::INFO, "GetScraperStatsByConvergedManifest", "Processing stats for project: " + project);
-
-            LoadProjectObjectToStatsByCPID(project, entry->second->data, dMagnitudePerProject, mProjectScraperStats);
-
-            // Insert into overall map.
-            for (auto const& entry2 : mProjectScraperStats)
+            // Do not process the BeaconList or VerifiedBeacons as a project stats file.
+            if (entry->first != "BeaconList" && entry->first != "VerifiedBeacons")
             {
-                mScraperStats[entry2.first] = entry2.second;
+                ConvergedManifestPartsMap[entry->first] = entry->second->data;
             }
         }
+
+        _log(logattribute::INFO, "ENDLOCK2", "CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts");
+    }
+
+    // Now run through the copy and do the stats processing.
+    for (const auto& entry : ConvergedManifestPartsMap)
+    {
+        _log(logattribute::INFO, "GetScraperStatsByConvergedManifest", "Processing stats for project: " + entry.first);
+
+        ScraperStats mProjectScraperStats;
+
+        LoadProjectObjectToStatsByCPID(entry.first, entry.second, dMagnitudePerProject, mProjectScraperStats);
+
+        // Insert into overall map.
+        for (auto const& entry2 : mProjectScraperStats)
+        {
+            mScraperStats[entry2.first] = entry2.second;
+        }
     }
 
     ProcessNetworkWideFromProjectStats(mScraperStats);
@@ -3499,52 +3515,6 @@ ScraperStatsAndVerifiedBeacons GetScraperStatsFromSingleManifest(CScraperManifes
 
     ScraperStatsAndVerifiedBeacons stats_and_verified_beacons {};
 
-    /*
-    // Fill out the dummy ConvergedManifest structure. Note this assumes one-to-one part to project statistics BLOB.
-    // Needs to be fixed for more than one part per BLOB. This is easy in this case, because it is all from/referring to
-    // one manifest.
-    int iPartNum = 0;
-    CDataStream ss(SER_NETWORK,1);
-    WriteCompactSize(ss, manifest.vParts.size());
-    uint256 nContentHashCheck;
-
-    for (const auto& iter : manifest.vParts)
-    {
-        std::string sProject;
-
-        if (iPartNum == 0)
-            sProject = "BeaconList";
-        else
-            sProject = manifest.projects[iPartNum-1].project;
-
-        // Serialize the hash to doublecheck the content hash.
-        ss << iter->hash;
-
-        iPartNum++;
-    }
-    ss << StructDummyConvergedManifest.ConsensusBlock;
-
-    nContentHashCheck = Hash(ss.begin(), ss.end());
-
-    if (nContentHashCheck != manifest.nContentHash)
-    {
-        _log(logattribute::ERR, "GetScraperStatsFromSingleManifest", "Selected Manifest content hash check failed! nContentHashCheck = "
-             + nContentHashCheck.GetHex() + " and nContentHash = " + manifest.nContentHash.GetHex());
-        // Content hash check failed. Return empty mScraperStats
-        return stats_and_verified_beacons;
-    }
-    else // Content matches.
-    {
-        // The DummyConvergedManifest content hash is NOT the same as the hash above from the CScraper::manifest, because it needs to be in the order of the
-        // map key and on the data, not the order of vParts by the part hash. So, unfortunately, we have to walk through the map again to hash it correctly.
-        CDataStream ss2(SER_NETWORK,1);
-        for (const auto& iter : StructDummyConvergedManifest.ConvergedManifestPartsMap)
-            ss2 << iter.second;
-
-        StructDummyConvergedManifest.nContentHash = Hash(ss2.begin(), ss2.end());
-    }
-    */
-
     // Enumerate the count of active projects from the dummy converged manifest. One of the parts
     // is the beacon list, is not a project, which is why that should not be included in the count.
     // Populate the verified beacons map, and if it is don't count that either.
@@ -4371,8 +4341,8 @@ bool ScraperConstructConvergedManifest(ConvergedManifest& StructConvergedManifes
 
     if (bConvergenceSuccessful)
     {
-        LOCK(CScraperManifest::cs_mapManifest);
-        _log(logattribute::INFO, "LOCK", "CScraperManifest::cs_mapManifest");
+        LOCK2(CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts);
+        _log(logattribute::INFO, "LOCK2", "CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts");
 
         // Select agreed upon (converged) CScraper manifest based on converged hash.
         auto pair = CScraperManifest::mapManifest.find(convergence->second.second);
@@ -4382,40 +4352,6 @@ bool ScraperConstructConvergedManifest(ConvergedManifest& StructConvergedManifes
         // be fixed for more than one part per BLOB. This is easy in this case, because it is all from/referring to one manifest.
         bool bConvergedContentHashMatches = StructConvergedManifest(manifest);
 
-        /*
-        StructConvergedManifest.ConsensusBlock = manifest.ConsensusBlock;
-        StructConvergedManifest.timestamp = GetAdjustedTime();
-        StructConvergedManifest.bByParts = false;
-
-        int iPartNum = 0;
-        CDataStream ss(SER_NETWORK,1);
-        WriteCompactSize(ss, manifest.vParts.size());
-        uint256 nContentHashCheck;
-
-        for (const auto& iter : manifest.vParts)
-        {
-            std::string sProject;
-
-            if (iPartNum == 0)
-                sProject = "BeaconList";
-            else
-                sProject = manifest.projects[iPartNum-1].project;
-
-            // Copy the parts data into the map keyed by project. This will also pick up the
-            // VerifiedBeacons "project" if present.
-            StructConvergedManifest.ConvergedManifestPartsMap.insert(std::make_pair(sProject, iter->data));
-
-            // Serialize the hash to doublecheck the content hash.
-            ss << iter->hash;
-
-            iPartNum++;
-        }
-        ss << StructConvergedManifest.ConsensusBlock;
-
-        nContentHashCheck = Hash(ss.begin(), ss.end());
-
-        */
-
         if (!bConvergedContentHashMatches)
         {
             bConvergenceSuccessful = false;
@@ -4425,19 +4361,6 @@ bool ScraperConstructConvergedManifest(ConvergedManifest& StructConvergedManifes
         }
         else // Content matches so we have a confirmed convergence.
         {
-            /*
-            // Copy the MANIFEST content hash into the ConvergedManifest.
-            StructConvergedManifest.nUnderlyingManifestContentHash = convergence->first;
-
-            // The ConvergedManifest content hash is NOT the same as the hash above from the CScraper::manifest, because it needs to be in the order of the
-            // map key and on the data, not the order of vParts by the part hash. So, unfortunately, we have to walk through the map again to hash it correctly.
-            CDataStream ss2(SER_NETWORK,1);
-            for (const auto& iter : StructConvergedManifest.ConvergedManifestPartsMap)
-                ss2 << iter.second;
-
-            StructConvergedManifest.nContentHash = Hash(ss2.begin(), ss2.end());
-            */
-
             // Determine if there is an excluded project. If so, set convergence back to false and drop back to project level to try and recover project by project.
             for (const auto& iProjects : projectWhitelist)
             {
@@ -4467,7 +4390,7 @@ bool ScraperConstructConvergedManifest(ConvergedManifest& StructConvergedManifes
             }
         }
 
-        _log(logattribute::INFO, "ENDLOCK", "CScraperManifest::cs_mapManifest");
+        _log(logattribute::INFO, "ENDLOCK2", "CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts");
     }
 
     if (!bConvergenceSuccessful)
@@ -4898,7 +4821,7 @@ mmCSManifestsBinnedByScraper ScraperCullAndBinCScraperManifests()
 
     _log(logattribute::INFO, "ScraperDeleteCScraperManifests", "Deleting old CScraperManifests.");
 
-    LOCK(CScraperManifest::cs_mapManifest);
+    LOCK2(CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts);
     _log(logattribute::INFO, "LOCK", "CScraperManifest::cs_mapManifest");
 
     // First check for unauthorized manifests just in case a scraper has been deauthorized.
@@ -4979,7 +4902,7 @@ mmCSManifestsBinnedByScraper ScraperCullAndBinCScraperManifests()
     // that large. (The lock on CScraperManifest::cs_mapManifest is still held from above.)
     mMapCSManifestsBinnedByScraper = BinCScraperManifestsByScraper();
 
-    _log(logattribute::INFO, "ENDLOCK", "CScraperManifest::cs_mapManifest");
+    _log(logattribute::INFO, "ENDLOCK2", "CScraperManifest::cs_mapManifest, CSplitBlob::cs_mapParts");
 
     return mMapCSManifestsBinnedByScraper;
 }
@@ -4989,21 +4912,29 @@ mmCSManifestsBinnedByScraper ScraperCullAndBinCScraperManifests()
 // ---------------------------------------------- In ---------------------------------------- Out
 bool LoadBeaconListFromConvergedManifest(const ConvergedManifest& StructConvergedManifest, ScraperBeaconMap& mBeaconMap)
 {
-    // Find the beacon list.
-    auto iter = StructConvergedManifest.ConvergedManifestPartPtrsMap.find("BeaconList");
+    boostio::filtering_istream in;
 
-    // Bail if the beacon list is not found, or the part is zero size (missing referenced part)
-    if (iter == StructConvergedManifest.ConvergedManifestPartPtrsMap.end() || iter->second->data.size() == 0)
     {
-        return false;
-    }
+        LOCK(CSplitBlob::cs_mapParts);
+        _log(logattribute::INFO, "LOCK", "CSplitBlob::cs_mapParts");
 
-    boostio::basic_array_source<char> input_source(&iter->second->data[0], iter->second->data.size());
-    boostio::stream<boostio::basic_array_source<char>> ingzss(input_source);
+        // Find the beacon list.
+        auto iter = StructConvergedManifest.ConvergedManifestPartPtrsMap.find("BeaconList");
 
-    boostio::filtering_istream in;
-    in.push(boostio::gzip_decompressor());
-    in.push(ingzss);
+        // Bail if the beacon list is not found, or the part is zero size (missing referenced part)
+        if (iter == StructConvergedManifest.ConvergedManifestPartPtrsMap.end() || iter->second->data.size() == 0)
+        {
+            return false;
+        }
+
+        boostio::basic_array_source<char> input_source(&iter->second->data[0], iter->second->data.size());
+        boostio::stream<boostio::basic_array_source<char>> ingzss(input_source);
+
+        in.push(boostio::gzip_decompressor());
+        in.push(ingzss);
+
+        _log(logattribute::INFO, "ENDLOCK", "CSplitBlob::cs_mapParts");
+    }
 
     std::string line;
 
@@ -5869,9 +5800,10 @@ UniValue scraperreport(const UniValue& params, bool fHelp)
 
             total_convergences_part_unique_pointer_maps_size = global_cache_unique_parts.size();
 
-            part_objects_reduced = total_convergences_part_pointer_maps_size
-                                   - total_convergences_part_unique_pointer_maps_size;
-
+            // The part objects reduced is EQUAL to the total size of all of the convergence part pointers
+            // added up, because those would be the additional parts needed in the old parts map. The
+            // new maps are all pointers to the already existing parts in the global map.
+            part_objects_reduced = total_convergences_part_pointer_maps_size;
 
             converged_scraper_stats_cache.pushKV("current_convergence_publishing_scrapers",
                                                  current_convergence_publishing_scrapers);