diff --git a/src/gridcoin/quorum.cpp b/src/gridcoin/quorum.cpp index bb4b06e506..c9aaabc693 100644 --- a/src/gridcoin/quorum.cpp +++ b/src/gridcoin/quorum.cpp @@ -1039,17 +1039,22 @@ class SuperblockValidator const CScraperManifest_shared_ptr manifest = iter->second; - LOCK(manifest->cs_manifest); - // 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 @@ -1058,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; @@ -1074,8 +1083,6 @@ class SuperblockValidator return; } - LOCK(CSplitBlob::cs_mapParts); - convergence.AddPart("VerifiedBeacons", manifest->vParts[part_offset]); } }; // ProjectCombiner diff --git a/src/gridcoin/scraper/scraper.cpp b/src/gridcoin/scraper/scraper.cpp index 2a0e15a5b6..0581c02987 100755 --- a/src/gridcoin/scraper/scraper.cpp +++ b/src/gridcoin/scraper/scraper.cpp @@ -3819,6 +3819,9 @@ bool ScraperSaveCScraperManifestToFiles(uint256 nManifestHash) // This is from the map find above. const CScraperManifest_shared_ptr manifest = pair->second; + LOCK(manifest->cs_manifest); + _log(logattribute::INFO, "LOCK", "cs_manifest"); + // Write out to files the parts. Note this assumes one-to-one part to file. Needs to // be fixed for more than one part per file. int iPartNum = 0; @@ -3859,6 +3862,7 @@ bool ScraperSaveCScraperManifestToFiles(uint256 nManifestHash) iPartNum++; } + _log(logattribute::INFO, "ENDLOCK", "cs_manifest"); _log(logattribute::INFO, "ENDLOCK", "CScraperManifest::cs_mapManifest"); return true; @@ -4178,6 +4182,9 @@ unsigned int ScraperDeleteUnauthorizedCScraperManifests() { CScraperManifest_shared_ptr manifest = iter->second; + LOCK(manifest->cs_manifest); + _log(logattribute::INFO, "LOCK", "cs_manifest"); + // We are not going to do anything with the banscore here, but it is an out parameter of IsManifestAuthorized. unsigned int banscore_out = 0; @@ -4194,6 +4201,8 @@ unsigned int ScraperDeleteUnauthorizedCScraperManifests() iter = CScraperManifest::DeleteManifest(iter, true); nDeleted++; } + + _log(logattribute::INFO, "ENDLOCK", "cs_manifest"); } // End LOCK(CScraperManifest::cs_mapManifest) @@ -4211,193 +4220,203 @@ bool ScraperSendFileManifestContents(CBitcoinAddress& Address, CKey& Key) EXCLUS auto manifest = std::shared_ptr(new CScraperManifest()); - // The manifest name is the authorized address of the scraper. - manifest->sCManifestName = Address.ToString(); - - // Also store local sCManifestName, because the manifest will be std::moved by addManifest. - std::string sCManifestName = Address.ToString(); - - manifest->nTime = StructScraperFileManifest.timestamp; - - // Also store local nTime, because the manifest will be std::moved by addManifest. - int64_t nTime = StructScraperFileManifest.timestamp; - - manifest->ConsensusBlock = StructScraperFileManifest.nConsensusBlockHash; + std::string sCManifestName; + int64_t nTime = 0; // This will have to be changed to support files bigger than 32 MB, where more than one // part per object will be required. int iPartNum = 0; - // Inject the BeaconList part. { - // Read in BeaconList - fs::path inputfile = "BeaconList.csv.gz"; - fs::path inputfilewpath = pathScraper / inputfile; + LOCK(manifest->cs_manifest); + _log(logattribute::INFO, "LOCK", "cs_manifest"); - // open input file, and associate with CAutoFile - FILE *file = fsbridge::fopen(inputfilewpath, "rb"); - CAutoFile filein(file, SER_DISK, CLIENT_VERSION); + // The manifest name is the authorized address of the scraper. + manifest->sCManifestName = Address.ToString(); - if (filein.IsNull()) - { - _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to open file (" + inputfile.string() + ")"); - return false; - } + // Also store local sCManifestName, because the manifest will be std::moved by addManifest. + sCManifestName = Address.ToString(); - // use file size to size memory buffer - int dataSize = fs::file_size(inputfilewpath); - std::vector vchData; - vchData.resize(dataSize); + manifest->nTime = StructScraperFileManifest.timestamp; - // read data from file - try - { - filein.read((char *)&vchData[0], dataSize); - } - catch (std::exception &e) + // Also store local nTime, because the manifest will be std::moved by addManifest. + nTime = StructScraperFileManifest.timestamp; + + manifest->ConsensusBlock = StructScraperFileManifest.nConsensusBlockHash; + + // Inject the BeaconList part. { - _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to read file (" + inputfile.string() + ")"); - return false; - } + // Read in BeaconList + fs::path inputfile = "BeaconList.csv.gz"; + fs::path inputfilewpath = pathScraper / inputfile; - filein.fclose(); + // open input file, and associate with CAutoFile + FILE *file = fsbridge::fopen(inputfilewpath, "rb"); + CAutoFile filein(file, SER_DISK, CLIENT_VERSION); - // The first part number will be the BeaconList. - manifest->BeaconList = iPartNum; - manifest->BeaconList_c = 0; + if (filein.IsNull()) + { + _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to open file (" + inputfile.string() + ")"); + return false; + } - CDataStream part(std::move(vchData), SER_NETWORK, 1); + // use file size to size memory buffer + int dataSize = fs::file_size(inputfilewpath); + std::vector vchData; + vchData.resize(dataSize); - LOCK(CSplitBlob::cs_mapParts); - _log(logattribute::INFO, "LOCK", "cs_mapParts"); + // read data from file + try + { + filein.read((char *)&vchData[0], dataSize); + } + catch (std::exception &e) + { + _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to read file (" + inputfile.string() + ")"); + return false; + } - manifest->addPartData(std::move(part)); + filein.fclose(); - iPartNum++; + // The first part number will be the BeaconList. + manifest->BeaconList = iPartNum; + manifest->BeaconList_c = 0; - _log(logattribute::INFO, "ENDLOCK", "cs_mapParts"); - } + CDataStream part(std::move(vchData), SER_NETWORK, 1); - // Inject the VerifiedBeaconList as a "project" called VerifiedBeacons. This is inelegant, but - // will maintain compatibility with older nodes. The older nodes will simply ignore this extra part - // because it will never match any whitelisted project. Only include it if it is not empty. - { - LOCK(cs_VerifiedBeacons); - _log(logattribute::INFO, "LOCK", "cs_VerifiedBeacons"); + LOCK(CSplitBlob::cs_mapParts); + _log(logattribute::INFO, "LOCK", "cs_mapParts"); - ScraperVerifiedBeacons& ScraperVerifiedBeacons = GetVerifiedBeacons(); + manifest->addPartData(std::move(part)); - if (!ScraperVerifiedBeacons.mVerifiedMap.empty()) + iPartNum++; + + _log(logattribute::INFO, "ENDLOCK", "cs_mapParts"); + } + + // Inject the VerifiedBeaconList as a "project" called VerifiedBeacons. This is inelegant, but + // will maintain compatibility with older nodes. The older nodes will simply ignore this extra part + // because it will never match any whitelisted project. Only include it if it is not empty. { - CScraperManifest::dentry ProjectEntry; + LOCK(cs_VerifiedBeacons); + _log(logattribute::INFO, "LOCK", "cs_VerifiedBeacons"); - ProjectEntry.project = "VerifiedBeacons"; - ProjectEntry.LastModified = ScraperVerifiedBeacons.timestamp; - ProjectEntry.current = true; + ScraperVerifiedBeacons& ScraperVerifiedBeacons = GetVerifiedBeacons(); - // For now each object will only have one part. - ProjectEntry.part1 = iPartNum; - ProjectEntry.partc = 0; - ProjectEntry.GridcoinTeamID = -1; //Not used anymore + if (!ScraperVerifiedBeacons.mVerifiedMap.empty()) + { + CScraperManifest::dentry ProjectEntry; - ProjectEntry.last = 1; + ProjectEntry.project = "VerifiedBeacons"; + ProjectEntry.LastModified = ScraperVerifiedBeacons.timestamp; + ProjectEntry.current = true; - manifest->projects.push_back(ProjectEntry); + // For now each object will only have one part. + ProjectEntry.part1 = iPartNum; + ProjectEntry.partc = 0; + ProjectEntry.GridcoinTeamID = -1; //Not used anymore - CDataStream part(SER_NETWORK, 1); + ProjectEntry.last = 1; - part << ScraperVerifiedBeacons.mVerifiedMap; + manifest->projects.push_back(ProjectEntry); - LOCK(CSplitBlob::cs_mapParts); - _log(logattribute::INFO, "LOCK", "cs_mapParts"); + CDataStream part(SER_NETWORK, 1); - manifest->addPartData(std::move(part)); + part << ScraperVerifiedBeacons.mVerifiedMap; - iPartNum++; + LOCK(CSplitBlob::cs_mapParts); + _log(logattribute::INFO, "LOCK", "cs_mapParts"); - _log(logattribute::INFO, "ENDLOCK", "cs_mapParts"); + manifest->addPartData(std::move(part)); + + iPartNum++; + + _log(logattribute::INFO, "ENDLOCK", "cs_mapParts"); + } + + _log(logattribute::INFO, "ENDLOCK", "cs_VerifiedBeacons"); } - _log(logattribute::INFO, "ENDLOCK", "cs_VerifiedBeacons"); - } + for (auto const& entry : StructScraperFileManifest.mScraperFileManifest) + { + auto scraper_cmanifest_include_noncurrent_proj_files = + []() { LOCK(cs_ScraperGlobals); return SCRAPER_CMANIFEST_INCLUDE_NONCURRENT_PROJ_FILES; }; - for (auto const& entry : StructScraperFileManifest.mScraperFileManifest) - { - auto scraper_cmanifest_include_noncurrent_proj_files = - []() { LOCK(cs_ScraperGlobals); return SCRAPER_CMANIFEST_INCLUDE_NONCURRENT_PROJ_FILES; }; + // If SCRAPER_CMANIFEST_INCLUDE_NONCURRENT_PROJ_FILES is false, only include current files to send across the network. + // Also continue (exclude) if it is a non-publishable entry (excludefromcsmanifest is true). + if ((!scraper_cmanifest_include_noncurrent_proj_files() && !entry.second.current) || entry.second.excludefromcsmanifest) + continue; - // If SCRAPER_CMANIFEST_INCLUDE_NONCURRENT_PROJ_FILES is false, only include current files to send across the network. - // Also continue (exclude) if it is a non-publishable entry (excludefromcsmanifest is true). - if ((!scraper_cmanifest_include_noncurrent_proj_files() && !entry.second.current) || entry.second.excludefromcsmanifest) - continue; + fs::path inputfile = entry.first; - fs::path inputfile = entry.first; + //_log(logattribute::INFO, "ScraperSendFileManifestContents", "Input file for CScraperManifest is " + inputfile.string()); - //_log(logattribute::INFO, "ScraperSendFileManifestContents", "Input file for CScraperManifest is " + inputfile.string()); + fs::path inputfilewpath = pathScraper / inputfile; - fs::path inputfilewpath = pathScraper / inputfile; + // open input file, and associate with CAutoFile + FILE *file = fsbridge::fopen(inputfilewpath, "rb"); + CAutoFile filein(file, SER_DISK, CLIENT_VERSION); - // open input file, and associate with CAutoFile - FILE *file = fsbridge::fopen(inputfilewpath, "rb"); - CAutoFile filein(file, SER_DISK, CLIENT_VERSION); + if (filein.IsNull()) + { + _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to open file (" + inputfile.string() + ")"); + return false; + } - if (filein.IsNull()) - { - _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to open file (" + inputfile.string() + ")"); - return false; - } + // use file size to size memory buffer + int dataSize = fs::file_size(inputfilewpath); + std::vector vchData; + vchData.resize(dataSize); - // use file size to size memory buffer - int dataSize = fs::file_size(inputfilewpath); - std::vector vchData; - vchData.resize(dataSize); + // read data from file + try + { + filein.read((char *)&vchData[0], dataSize); + } + catch (std::exception &e) + { + _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to read file (" + inputfile.string() + ")"); + return false; + } - // read data from file - try - { - filein.read((char *)&vchData[0], dataSize); - } - catch (std::exception &e) - { - _log(logattribute::ERR, "ScraperSendFileManifestContents", "Failed to read file (" + inputfile.string() + ")"); - return false; - } + filein.fclose(); - filein.fclose(); + CScraperManifest::dentry ProjectEntry; - CScraperManifest::dentry ProjectEntry; + ProjectEntry.project = entry.second.project; + std::string sProject = entry.second.project + "-"; - ProjectEntry.project = entry.second.project; - std::string sProject = entry.second.project + "-"; + std::string sinputfile = inputfile.string(); + std::string suffix = ".csv.gz"; - std::string sinputfile = inputfile.string(); - std::string suffix = ".csv.gz"; + // Remove project- + sinputfile.erase(sinputfile.find(sProject), sProject.length()); + // Remove suffix. What is left is the ETag. + ProjectEntry.ETag = sinputfile.erase(sinputfile.find(suffix), suffix.length()); - // Remove project- - sinputfile.erase(sinputfile.find(sProject), sProject.length()); - // Remove suffix. What is left is the ETag. - ProjectEntry.ETag = sinputfile.erase(sinputfile.find(suffix), suffix.length()); + ProjectEntry.LastModified = entry.second.timestamp; - ProjectEntry.LastModified = entry.second.timestamp; + // For now each object will only have one part. + ProjectEntry.part1 = iPartNum; + ProjectEntry.partc = 0; + ProjectEntry.GridcoinTeamID = -1; //Not used anymore - // For now each object will only have one part. - ProjectEntry.part1 = iPartNum; - ProjectEntry.partc = 0; - ProjectEntry.GridcoinTeamID = -1; //Not used anymore + ProjectEntry.current = entry.second.current; - ProjectEntry.current = entry.second.current; + ProjectEntry.last = 1; - ProjectEntry.last = 1; + manifest->projects.push_back(ProjectEntry); - manifest->projects.push_back(ProjectEntry); + CDataStream part(vchData, SER_NETWORK, 1); - CDataStream part(vchData, SER_NETWORK, 1); + manifest->addPartData(std::move(part)); - manifest->addPartData(std::move(part)); + iPartNum++; + } - iPartNum++; + _log(logattribute::INFO, "LOCK", "cs_manifest"); } // "Sign" and "send". @@ -4447,6 +4466,9 @@ ConvergedManifest::ConvergedManifest() ConvergedManifest::ConvergedManifest(CScraperManifest_shared_ptr& in) { + // Make Clang happy. + LOCK(in->cs_manifest); + ConsensusBlock = in->ConsensusBlock; timestamp = GetAdjustedTime(); bByParts = false; @@ -4462,6 +4484,8 @@ ConvergedManifest::ConvergedManifest(CScraperManifest_shared_ptr& in) bool ConvergedManifest::operator()(const CScraperManifest_shared_ptr& in) { + LOCK(in->cs_manifest); + ConsensusBlock = in->ConsensusBlock; timestamp = GetAdjustedTime(); bByParts = false; diff --git a/src/gridcoin/scraper/scraper_net.cpp b/src/gridcoin/scraper/scraper_net.cpp index 40695c6937..8bfe0c96eb 100644 --- a/src/gridcoin/scraper/scraper_net.cpp +++ b/src/gridcoin/scraper/scraper_net.cpp @@ -339,9 +339,11 @@ void CScraperManifest::SerializeForManifestCompare(CDataStream& ss) const EXCLUS ss << ConsensusBlock; } -// A lock needs to be taken on cs_mapManifest and cs_mapParts before calling this. -void CScraperManifest::Serialize(CDataStream& ss) const EXCLUSIVE_LOCKS_REQUIRED(cs_manifest, cs_mapParts) +// A lock needs to be taken on cs_manifest and cs_mapParts before calling this. +void CScraperManifest::Serialize(CDataStream& ss) const EXCLUSIVE_LOCKS_REQUIRED(cs_mapParts) { + LOCK(cs_manifest); + SerializeWithoutSignature(ss); ss << signature; } @@ -349,7 +351,7 @@ void CScraperManifest::Serialize(CDataStream& ss) const EXCLUSIVE_LOCKS_REQUIRED // This is the complement to IsScraperAuthorizedToBroadcastManifests in the scraper. // It is used to determine whether received manifests are authorized. -bool CScraperManifest::IsManifestAuthorized(AppCacheSection& mScrapers, int64_t& nTime, CPubKey& PubKey, unsigned int& banscore_out) +bool CScraperManifest::IsManifestAuthorized(AppCacheSection& mScrapers, int64_t& nTime, CPubKey& PubKey, unsigned int& banscore_out) EXCLUSIVE_LOCKS_REQUIRED(cs_manifest) { bool bIsValid = PubKey.IsValid(); diff --git a/src/main.cpp b/src/main.cpp index dd86e12092..5cbd6b6176 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3333,8 +3333,6 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, { CScraperManifest_shared_ptr manifest = iter->second; - LOCK(manifest->cs_manifest); - // We are not going to do anything with the banscore here, because this is the sending node, // but it is an out parameter of IsManifestAuthorized. unsigned int banscore_out = 0;