Scraper enhanced thread safety

This implements qualifiers required for advanced static thread safety
analysis by Clang and other advanced compilers. Note that a few minor
thread safety issues were caught and fixed preparing this commit.

Also, thread safety was implemented around the scraper globals and the
AppCache reads. Even though these are seldom changed and there is little
chance of an issue, they should be completely thread-safe as they
are accessed by multiple threads.

Note that some lambdas were used to do quick locked reads against the
scraper globals in some of the scraper functions to facilitate use of
the global in existing if statements without further change. This is
a little ugly and some of the lambdas are repeated. This will be cleaned
up when the scraper is restructured into classes later.

Author: jamescowens

src/gridcoin/gridcoin.cpp

@@ -20,6 +20,7 @@
 
 using namespace GRC;
 
+extern CCriticalSection cs_ScraperGlobals;
 extern bool fExplorer;
 extern unsigned int nScraperSleep;
 extern unsigned int nActiveBeforeSB;
@@ -276,10 +277,14 @@ void ThreadScraperSubscriber(void* parg)
 //!
 void InitializeScraper(ThreadHandlerPtr threads)
 {
-    // Default to 300 sec (5 min), clamp to 60 minimum, 600 maximum - converted to milliseconds.
-    nScraperSleep = std::clamp<int64_t>(gArgs.GetArg("-scrapersleep", 300), 60, 600) * 1000;
-    // Default to 14400 sec (4 hrs), clamp to 300 minimum, 86400 maximum (meaning active all of the time).
-    nActiveBeforeSB = std::clamp<int64_t>(gArgs.GetArg("-activebeforesb", 14400), 300, 86400);
+    {
+        LOCK(cs_ScraperGlobals);
+
+        // Default to 300 sec (5 min), clamp to 60 minimum, 600 maximum - converted to milliseconds.
+        nScraperSleep = std::clamp<int64_t>(gArgs.GetArg("-scrapersleep", 300), 60, 600) * 1000;
+        // Default to 14400 sec (4 hrs), clamp to 300 minimum, 86400 maximum (meaning active all of the time).
+        nActiveBeforeSB = std::clamp<int64_t>(gArgs.GetArg("-activebeforesb", 14400), 300, 86400);
+    }
 
     // Run the scraper or subscriber housekeeping thread, but not both. The
     // subscriber housekeeping thread checks if the flag for the scraper thread
@@ -311,6 +316,8 @@ void InitializeScraper(ThreadHandlerPtr threads)
 //!
 void InitializeExplorerFeatures()
 {
+    LOCK(cs_ScraperGlobals);
+
     fExplorer = gArgs.GetBoolArg("-scraper", false) && gArgs.GetBoolArg("-explorer", false);
 }
 

src/gridcoin/quorum.cpp

@@ -29,6 +29,7 @@ Superblock ScraperGetSuperblockContract(
     bool bContractDirectFromStatsUpdate = false,
     bool bFromHousekeeping = false);
 
+extern CCriticalSection cs_ScraperGlobals;
 extern CCriticalSection cs_ConvergedScraperStatsCache;
 extern ConvergedScraperStats ConvergedScraperStatsCache;
 
@@ -609,8 +610,12 @@ class SuperblockValidator
             return Result::UNKNOWN;
         }
 
-        if (m_superblock.Age(GetAdjustedTime()) > SCRAPER_CMANIFEST_RETENTION_TIME) {
-            return Result::HISTORICAL;
+        {
+            LOCK(cs_ScraperGlobals);
+
+            if (m_superblock.Age(GetAdjustedTime()) > SCRAPER_CMANIFEST_RETENTION_TIME) {
+                return Result::HISTORICAL;
+            }
         }
 
         if (use_cache) {
@@ -1037,12 +1042,19 @@ class SuperblockValidator
             // If the manifest for the beacon list is now empty, we cannot
             // proceed, but ProjectResolver should always select manifests
             // with a beacon list part:
-            if (manifest->vParts.empty()) {
-                LogPrintf("ValidateSuperblock(): beacon list part missing.");
-                return;
-            }
 
-            convergence.AddPart("BeaconList", manifest->vParts[0]);
+            // Note using fine-grained locking here to avoid lock-order issues and
+            // also to deal with Clang potential false positive below.
+            {
+                LOCK(manifest->cs_manifest);
+
+                if (manifest->vParts.empty()) {
+                    LogPrintf("ValidateSuperblock(): beacon list part missing.");
+                    return;
+                }
+
+                convergence.AddPart("BeaconList", manifest->vParts[0]);
+            }
 
             // Find the offset of the verified beacons project part. Typically
             // this exists at vParts offset 1 when a scraper verified at least
@@ -1051,10 +1063,14 @@ class SuperblockValidator
             const auto verified_beacons_entry_iter = std::find_if(
                 manifest->projects.begin(),
                 manifest->projects.end(),
-                [](const CScraperManifest::dentry& entry) {
+                [manifest](const CScraperManifest::dentry& entry) {
+                    // Clang was giving a false positive on thread safety without this.
+                    LOCK(manifest->cs_manifest);
                     return entry.project == "VerifiedBeacons";
                 });
 
+            LOCK2(CSplitBlob::cs_mapParts, manifest->cs_manifest);
+
             if (verified_beacons_entry_iter == manifest->projects.end()) {
                 LogPrintf("ValidateSuperblock(): verified beacon project missing.");
                 return;
@@ -1229,6 +1245,8 @@ class SuperblockValidator
 
             const CScraperManifest_shared_ptr manifest = iter->second;
 
+            LOCK(manifest->cs_manifest);
+
             for (const auto& entry : manifest->projects) {
                 auto project_option = TallyProject(entry.project, scraper_id);
 

src/gridcoin/researcher.cpp

@@ -29,6 +29,7 @@
 using namespace GRC;
 
 extern CCriticalSection cs_main;
+extern CCriticalSection cs_ScraperGlobals;
 extern std::string msMiningErrors;
 extern unsigned int nActiveBeforeSB;
 
@@ -1086,6 +1087,8 @@ void Researcher::RunRenewBeaconJob()
     // window begins nActiveBeforeSB seconds before the next superblock.
     // This is four hours by default unless overridden by protocol entry.
     //
+    LOCK(cs_ScraperGlobals);
+
     if (!Quorum::SuperblockNeeded(pindexBest->nTime + nActiveBeforeSB)) {
         TRY_LOCK(pwalletMain->cs_wallet, locked_wallet);
 

src/gridcoin/scraper/fwd.h

@@ -46,12 +46,6 @@ enum class scraperSBvalidationtype
     ProjectLevelConvergence
 };
 
-
-/*********************
-* Global Vars        *
-*********************/
-
-
 typedef std::string ScraperID;
 // The inner map is sorted in descending order of time. The pair is manifest hash, content hash.
 typedef std::multimap<int64_t, std::pair<uint256, uint256>, std::greater <int64_t>> mCSManifest;
@@ -74,66 +68,13 @@ typedef std::map<std::string, CSplitBlob::CPart*> mConvergedManifestPart_ptrs;
 struct ConvergedManifest
 {
     // Empty converged manifest constructor
-    ConvergedManifest()
-    {
-        nContentHash = {};
-        ConsensusBlock = {};
-        timestamp = 0;
-        bByParts = false;
-
-        CScraperConvergedManifest_ptr = nullptr;
-
-        ConvergedManifestPartPtrsMap = {};
-
-        mIncludedScraperManifests = {};
-
-        nUnderlyingManifestContentHash = {};
-
-        vIncludedScrapers = {};
-        vExcludedScrapers = {};
-        vScrapersNotPublishing = {};
-
-        mIncludedScrapersbyProject = {};
-        mIncludedProjectsbyScraper = {};
-
-        mScraperConvergenceCountbyProject = {};
-
-        vExcludedProjects = {};
-    }
+    ConvergedManifest();
 
     // For constructing a dummy converged manifest from a single manifest
-    ConvergedManifest(CScraperManifest_shared_ptr& in)
-    {
-        ConsensusBlock = in->ConsensusBlock;
-        timestamp = GetAdjustedTime();
-        bByParts = false;
-
-        CScraperConvergedManifest_ptr = in;
-
-        PopulateConvergedManifestPartPtrsMap();
-
-        ComputeConvergedContentHash();
-
-        nUnderlyingManifestContentHash = in->nContentHash;
-    }
+    ConvergedManifest(CScraperManifest_shared_ptr& in);
 
     // Call operator to update an already initialized ConvergedManifest with a passed in CScraperManifest
-    bool operator()(const CScraperManifest_shared_ptr& in)
-    {
-        ConsensusBlock = in->ConsensusBlock;
-        timestamp = GetAdjustedTime();
-        bByParts = false;
-
-        CScraperConvergedManifest_ptr = in;
-
-        bool bConvergedContentHashMatches = PopulateConvergedManifestPartPtrsMap();
-
-        ComputeConvergedContentHash();
-
-        nUnderlyingManifestContentHash = in->nContentHash;
-
-        return bConvergedContentHashMatches;
-    }
+    bool operator()(const CScraperManifest_shared_ptr& in);
 
     // IMPORTANT... nContentHash is NOT the hash of part hashes in the order of vParts unlike CScraper::manifest.
     // It is the hash of the data in the ConvergedManifestPartsMap in the order of the key. It represents
@@ -173,61 +114,9 @@ struct ConvergedManifest
     // --------- project
     std::vector<std::string> vExcludedProjects;
 
-    bool PopulateConvergedManifestPartPtrsMap()
-    {
-        if (CScraperConvergedManifest_ptr == nullptr) return false;
-
-        int iPartNum = 0;
-        CDataStream ss(SER_NETWORK,1);
-        WriteCompactSize(ss, CScraperConvergedManifest_ptr->vParts.size());
-        uint256 nContentHashCheck;
-
-        for (const auto& iter : CScraperConvergedManifest_ptr->vParts)
-        {
-            std::string sProject;
-
-            if (iPartNum == 0)
-                sProject = "BeaconList";
-            else
-                sProject = CScraperConvergedManifest_ptr->projects[iPartNum-1].project;
-
-            // 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.
-            ConvergedManifestPartPtrsMap.insert(std::make_pair(sProject, iter));
+    bool PopulateConvergedManifestPartPtrsMap();
 
-            // Serialize the hash to doublecheck the content hash.
-            ss << iter->hash;
-
-            iPartNum++;
-        }
-
-        ss << CScraperConvergedManifest_ptr->ConsensusBlock;
-
-        nContentHashCheck = Hash(ss.begin(), ss.end());
-
-        if (nContentHashCheck != CScraperConvergedManifest_ptr->nContentHash)
-        {
-            LogPrintf("ERROR: PopulateConvergedManifestPartPtrsMap(): Selected Manifest content hash check failed! "
-                      "nContentHashCheck = %s and nContentHash = %s.",
-                      nContentHashCheck.GetHex(), CScraperConvergedManifest_ptr->nContentHash.GetHex());
-            return false;
-        }
-
-        return true;
-    }
-
-    void ComputeConvergedContentHash()
-    {
-        CDataStream ss(SER_NETWORK,1);
-
-        for (const auto& iter : ConvergedManifestPartPtrsMap)
-        {
-            ss << iter.second->data;
-        }
-
-        nContentHash = Hash(ss.begin(), ss.end());
-    }
+    void ComputeConvergedContentHash();
 };