From 27d328030cf8e57092951f00817d78dd9c578965 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Jan=C3=ADk?= Date: Sat, 6 Aug 2016 10:54:29 +0200 Subject: [PATCH 01/20] Do not shadow LOCK's criticalblock variable for LOCK inside LOCK zcash: cherry picked from commit 33d15a3a76d073986337adcd62646d93e7ba223f zcash: https://github.com/bitcoin/bitcoin/pull/8472 --- src/sync.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/sync.h b/src/sync.h index 75021ee4b..f8cd9fd73 100644 --- a/src/sync.h +++ b/src/sync.h @@ -171,7 +171,10 @@ public: typedef CMutexLock CCriticalBlock; -#define LOCK(cs) CCriticalBlock criticalblock(cs, #cs, __FILE__, __LINE__) +#define PASTE(x, y) x ## y +#define PASTE2(x, y) PASTE(x, y) + +#define LOCK(cs) CCriticalBlock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__) #define LOCK2(cs1, cs2) CCriticalBlock criticalblock1(cs1, #cs1, __FILE__, __LINE__), criticalblock2(cs2, #cs2, __FILE__, __LINE__) #define TRY_LOCK(cs, name) CCriticalBlock name(cs, #cs, __FILE__, __LINE__, true) From 451600b9a680ae7a7a9b4382754e2341b1db0b24 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 26 Aug 2016 21:07:05 +0200 Subject: [PATCH 02/20] Fix some locks This makes sure that cs_filter is never held while taking cs_main or CNode::cs_vSend. zcash: cherry picked from commit 144ed76ea0b38b7c21b224501964f8a2c65c4bb6 zcash: https://github.com/bitcoin/bitcoin/pull/8606 --- src/main.cpp | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 57147bb8e..f47a2a5ca 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5594,10 +5594,16 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam pfrom->PushMessage("block", block); else // MSG_FILTERED_BLOCK) { - LOCK(pfrom->cs_filter); - if (pfrom->pfilter) + bool send = false; + CMerkleBlock merkleBlock; { - CMerkleBlock merkleBlock(block, *pfrom->pfilter); + LOCK(pfrom->cs_filter); + if (pfrom->pfilter) { + send = true; + merkleBlock = CMerkleBlock(block, *pfrom->pfilter); + } + } + if (send) { pfrom->PushMessage("merkleblock", merkleBlock); // CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see // This avoids hurting performance by pointlessly requiring a round-trip @@ -6578,8 +6584,8 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string delete pfrom->pfilter; pfrom->pfilter = new CBloomFilter(filter); pfrom->pfilter->UpdateEmptyFull(); + pfrom->fRelayTxes = true; } - pfrom->fRelayTxes = true; } @@ -6590,20 +6596,21 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string // Nodes must NEVER send a data item > 520 bytes (the max size for a script data object, // and thus, the maximum size any matched object can have) in a filteradd message - if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) - { - LOCK(cs_main); - Misbehaving(pfrom->GetId(), 100); + bool bad = false; + if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) { + bad = true; } else { LOCK(pfrom->cs_filter); - if (pfrom->pfilter) + if (pfrom->pfilter) { pfrom->pfilter->insert(vData); - else - { - LOCK(cs_main); - Misbehaving(pfrom->GetId(), 100); + } else { + bad = true; } } + if (bad) { + LOCK(cs_main); + Misbehaving(pfrom->GetId(), 100); + } } From cafc3f845d0fee013513c8feed5491c558fe4ada Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 27 Nov 2016 15:11:49 -0800 Subject: [PATCH 03/20] Fix race when accessing std::locale::classic() See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78552 zcash: cherry picked from commit 507145d78595e052ce13368e122f72c85093992c zcash: https://github.com/bitcoin/bitcoin/pull/9230 --- src/utiltime.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utiltime.cpp b/src/utiltime.cpp index 6e819e7af..17d51a2d6 100644 --- a/src/utiltime.cpp +++ b/src/utiltime.cpp @@ -50,8 +50,9 @@ void MilliSleep(int64_t n) std::string DateTimeStrFormat(const char* pszFormat, int64_t nTime) { + static std::locale classic(std::locale::classic()); // std::locale takes ownership of the pointer - std::locale loc(std::locale::classic(), new boost::posix_time::time_facet(pszFormat)); + std::locale loc(classic, new boost::posix_time::time_facet(pszFormat)); std::stringstream ss; ss.imbue(loc); ss << boost::posix_time::from_time_t(nTime); From f60964c374e88eef3e712372e9cffb5743bb1eab Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 29 Nov 2016 18:52:44 -0800 Subject: [PATCH 04/20] Lock mapArgs/mapMultiArgs access in util zcash: cherry picked from commit 4e048142a5e45d622355dad92ade192ad4769ca3 zcash: https://github.com/bitcoin/bitcoin/pull/9243 --- src/util.cpp | 44 +++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/src/util.cpp b/src/util.cpp index f7c6233c6..2663b8d1b 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -87,6 +87,7 @@ using namespace std; const char * const BITCOIN_CONF_FILENAME = "zcash.conf"; const char * const BITCOIN_PID_FILENAME = "zcashd.pid"; +CCriticalSection cs_args; map mapArgs; map > mapMultiArgs; bool fDebug = false; @@ -113,6 +114,7 @@ static void InterpretNegativeSetting(std::string& strKey, std::string& strValue) void ParseParameters(int argc, const char* const argv[]) { + LOCK(cs_args); mapArgs.clear(); mapMultiArgs.clear(); @@ -148,6 +150,7 @@ void ParseParameters(int argc, const char* const argv[]) std::string GetArg(const std::string& strArg, const std::string& strDefault) { + LOCK(cs_args); if (mapArgs.count(strArg)) return mapArgs[strArg]; return strDefault; @@ -155,6 +158,7 @@ std::string GetArg(const std::string& strArg, const std::string& strDefault) int64_t GetArg(const std::string& strArg, int64_t nDefault) { + LOCK(cs_args); if (mapArgs.count(strArg)) return atoi64(mapArgs[strArg]); return nDefault; @@ -162,6 +166,7 @@ int64_t GetArg(const std::string& strArg, int64_t nDefault) bool GetBoolArg(const std::string& strArg, bool fDefault) { + LOCK(cs_args); if (mapArgs.count(strArg)) return InterpretBool(mapArgs[strArg]); return fDefault; @@ -169,6 +174,7 @@ bool GetBoolArg(const std::string& strArg, bool fDefault) bool SoftSetArg(const std::string& strArg, const std::string& strValue) { + LOCK(cs_args); if (mapArgs.count(strArg)) return false; mapArgs[strArg] = strValue; @@ -286,7 +292,7 @@ static fs::path ZC_GetDefaultBaseParamsDir() const fs::path &ZC_GetParamsDir() { - LOCK(csPathCached); // Reuse the same lock as upstream. + LOCK2(cs_args, csPathCached); fs::path &path = zc_paramsPathCached; @@ -313,6 +319,7 @@ const fs::path &ZC_GetParamsDir() const fs::path GetExportDir() { fs::path path; + LOCK(cs_args); if (mapArgs.count("-exportdir")) { path = fs::system_complete(mapArgs["-exportdir"]); if (fs::exists(path) && !fs::is_directory(path)) { @@ -328,8 +335,7 @@ const fs::path GetExportDir() const fs::path &GetDataDir(bool fNetSpecific) { - - LOCK(csPathCached); + LOCK2(cs_args, csPathCached); fs::path &path = fNetSpecific ? pathCachedNetSpecific : pathCached; @@ -357,6 +363,7 @@ const fs::path &GetDataDir(bool fNetSpecific) void ClearDatadirCache() { + LOCK(csPathCached); pathCached = fs::path(); pathCachedNetSpecific = fs::path(); } @@ -402,23 +409,26 @@ void ReadConfigFile(const std::string& confPath, }; set unique_options; - for (boost::program_options::detail::config_file_iterator it(streamConfig, setOptions), end; it != end; ++it) { - string strKey = string("-") + it->string_key; - string strValue = it->value[0]; - - if (find(allowed_duplicates.begin(), allowed_duplicates.end(), it->string_key) == allowed_duplicates.end()) + LOCK(cs_args); + for (boost::program_options::detail::config_file_iterator it(streamConfig, setOptions), end; it != end; ++it) { - if (!unique_options.insert(strKey).second) { - throw std::runtime_error(strprintf("Option '%s' is duplicated, which is not allowed.", strKey)); - } - } + string strKey = string("-") + it->string_key; + string strValue = it->value[0]; - InterpretNegativeSetting(strKey, strValue); - // Don't overwrite existing settings so command line settings override zcash.conf - if (mapSettingsRet.count(strKey) == 0) - mapSettingsRet[strKey] = strValue; - mapMultiSettingsRet[strKey].push_back(strValue); + if (find(allowed_duplicates.begin(), allowed_duplicates.end(), it->string_key) == allowed_duplicates.end()) + { + if (!unique_options.insert(strKey).second) { + throw std::runtime_error(strprintf("Option '%s' is duplicated, which is not allowed.", strKey)); + } + } + + InterpretNegativeSetting(strKey, strValue); + // Don't overwrite existing settings so command line settings override zcash.conf + if (mapSettingsRet.count(strKey) == 0) + mapSettingsRet[strKey] = strValue; + mapMultiSettingsRet[strKey].push_back(strValue); + } } // If datadir is changed in .conf file: ClearDatadirCache(); From 024a94663cac35d4bdedc15d493b3eb322afb4d3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 24 Jan 2017 16:50:27 -0500 Subject: [PATCH 05/20] Ensure cs_vNodes is held when using the return value from FindNode zcash: cherry picked from commit 3c37dc40d39e1a1e56b6b0d3e660626a78656d4f zcash: https://github.com/bitcoin/bitcoin/pull/9626 --- src/net.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/net.cpp b/src/net.cpp index 07cc82682..95442f500 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -372,6 +372,7 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest) return NULL; // Look for an existing connection + LOCK(cs_vNodes); CNode* pnode = FindNode((CService)addrConnect); if (pnode) { From 6cc2c45eb9dbbdaa58d962df4400db59dd87add5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 3 Feb 2017 13:48:48 -0500 Subject: [PATCH 06/20] Access WorkQueue::running only within the cs lock. This removes a "race" between Interrupt() and Run(), though it should not effect any of our supported platforms. zcash: cherry picked from commit 7b2d96b634f9fd283480caf3bece56138d0587e3 zcash: https://github.com/bitcoin/bitcoin/pull/9679 --- src/httpserver.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 134057d51..677ab8190 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -119,7 +119,7 @@ public: void Run() { ThreadCounter count(*this); - while (running) { + while (true) { std::unique_ptr i; { std::unique_lock lock(cs); From 0658e2d0d393609ba01df525069aaef5b7ba5bdd Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Sat, 26 Nov 2016 19:32:30 -0800 Subject: [PATCH 07/20] Make nWalletDBUpdated atomic to avoid a potential race. zcash: cherry picked from commit d63ff6265b0c6ae30efcbb9120d4db419606198a zcash: https://github.com/bitcoin/bitcoin/pull/9227 --- src/wallet/db.cpp | 3 -- src/wallet/db.h | 2 - src/wallet/wallet.cpp | 2 +- src/wallet/walletdb.cpp | 89 +++++++++++++++++++++++------------------ src/wallet/walletdb.h | 2 + 5 files changed, 54 insertions(+), 44 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index d45f3662f..088a74784 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -24,9 +24,6 @@ using namespace std; -unsigned int nWalletDBUpdated; - - // // CDB // diff --git a/src/wallet/db.h b/src/wallet/db.h index 1b0da7882..1aa88f90b 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -24,8 +24,6 @@ static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100; static const bool DEFAULT_WALLET_PRIVDB = true; -extern unsigned int nWalletDBUpdated; - class CDBEnv { private: diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ff0e30058..c8ce6117b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4835,7 +4835,7 @@ bool CWallet::InitLoadWallet(bool clearWitnessCaches) walletInstance->ScanForWalletTransactions(pindexRescan, true); LogPrintf(" rescan %15dms\n", GetTimeMillis() - nStart); walletInstance->SetBestChain(chainActive.GetLocator()); - nWalletDBUpdated++; + CWalletDB::IncrementUpdateCounter(); // Restore wallet transaction metadata after -zapwallettxes=1 if (GetBoolArg("-zapwallettxes", false) && GetArg("-zapwallettxes", "1") != "2") diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 4b8f0ee92..642dbf7a6 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -20,18 +20,21 @@ #include #include +#include using namespace std; static uint64_t nAccountingEntryNumber = 0; +static std::atomic nWalletDBUpdateCounter; + // // CWalletDB // bool CWalletDB::WriteName(const string& strAddress, const string& strName) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(make_pair(string("name"), strAddress), strName); } @@ -39,37 +42,37 @@ bool CWalletDB::EraseName(const string& strAddress) { // This should only be used for sending addresses, never for receiving addresses, // receiving addresses must always have an address book entry if they're not change return. - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(make_pair(string("name"), strAddress)); } bool CWalletDB::WritePurpose(const string& strAddress, const string& strPurpose) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(make_pair(string("purpose"), strAddress), strPurpose); } bool CWalletDB::ErasePurpose(const string& strPurpose) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(make_pair(string("purpose"), strPurpose)); } bool CWalletDB::WriteTx(uint256 hash, const CWalletTx& wtx) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("tx"), hash), wtx); } bool CWalletDB::EraseTx(uint256 hash) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(std::make_pair(std::string("tx"), hash)); } bool CWalletDB::WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata& keyMeta) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; if (!Write(std::make_pair(std::string("keymeta"), vchPubKey), keyMeta, false)) @@ -89,7 +92,7 @@ bool CWalletDB::WriteCryptedKey(const CPubKey& vchPubKey, const CKeyMetadata &keyMeta) { const bool fEraseUnencryptedKey = true; - nWalletDBUpdated++; + nWalletDBUpdateCounter++; if (!Write(std::make_pair(std::string("keymeta"), vchPubKey), keyMeta)) @@ -111,7 +114,7 @@ bool CWalletDB::WriteCryptedZKey(const libzcash::SproutPaymentAddress & addr, const CKeyMetadata &keyMeta) { const bool fEraseUnencryptedKey = true; - nWalletDBUpdated++; + nWalletDBUpdateCounter++; if (!Write(std::make_pair(std::string("zkeymeta"), addr), keyMeta)) return false; @@ -131,7 +134,7 @@ bool CWalletDB::WriteCryptedSaplingZKey( const CKeyMetadata &keyMeta) { const bool fEraseUnencryptedKey = true; - nWalletDBUpdated++; + nWalletDBUpdateCounter++; auto ivk = extfvk.fvk.in_viewing_key(); if (!Write(std::make_pair(std::string("sapzkeymeta"), ivk), keyMeta)) @@ -149,13 +152,13 @@ bool CWalletDB::WriteCryptedSaplingZKey( bool CWalletDB::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("mkey"), nID), kMasterKey, true); } bool CWalletDB::WriteZKey(const libzcash::SproutPaymentAddress& addr, const libzcash::SproutSpendingKey& key, const CKeyMetadata &keyMeta) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; if (!Write(std::make_pair(std::string("zkeymeta"), addr), keyMeta)) return false; @@ -167,7 +170,7 @@ bool CWalletDB::WriteSaplingZKey(const libzcash::SaplingIncomingViewingKey &ivk, const libzcash::SaplingExtendedSpendingKey &key, const CKeyMetadata &keyMeta) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; if (!Write(std::make_pair(std::string("sapzkeymeta"), ivk), keyMeta)) return false; @@ -179,58 +182,58 @@ bool CWalletDB::WriteSaplingPaymentAddress( const libzcash::SaplingPaymentAddress &addr, const libzcash::SaplingIncomingViewingKey &ivk) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("sapzaddr"), addr), ivk, false); } bool CWalletDB::WriteSproutViewingKey(const libzcash::SproutViewingKey &vk) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("vkey"), vk), '1'); } bool CWalletDB::EraseSproutViewingKey(const libzcash::SproutViewingKey &vk) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(std::make_pair(std::string("vkey"), vk)); } bool CWalletDB::WriteSaplingExtendedFullViewingKey( const libzcash::SaplingExtendedFullViewingKey &extfvk) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("sapextfvk"), extfvk), '1'); } bool CWalletDB::EraseSaplingExtendedFullViewingKey( const libzcash::SaplingExtendedFullViewingKey &extfvk) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(std::make_pair(std::string("sapextfvk"), extfvk)); } bool CWalletDB::WriteCScript(const uint160& hash, const CScript& redeemScript) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("cscript"), hash), *(const CScriptBase*)(&redeemScript), false); } bool CWalletDB::WriteWatchOnly(const CScript &dest) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("watchs"), *(const CScriptBase*)(&dest)), '1'); } bool CWalletDB::EraseWatchOnly(const CScript &dest) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(std::make_pair(std::string("watchs"), *(const CScriptBase*)(&dest))); } bool CWalletDB::WriteBestBlock(const CBlockLocator& locator) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::string("bestblock"), locator); } @@ -241,19 +244,19 @@ bool CWalletDB::ReadBestBlock(CBlockLocator& locator) bool CWalletDB::WriteOrderPosNext(int64_t nOrderPosNext) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::string("orderposnext"), nOrderPosNext); } bool CWalletDB::WriteDefaultKey(const CPubKey& vchPubKey) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::string("defaultkey"), vchPubKey); } bool CWalletDB::WriteWitnessCacheSize(int64_t nWitnessCacheSize) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::string("witnesscachesize"), nWitnessCacheSize); } @@ -264,13 +267,13 @@ bool CWalletDB::ReadPool(int64_t nPool, CKeyPool& keypool) bool CWalletDB::WritePool(int64_t nPool, const CKeyPool& keypool) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("pool"), nPool), keypool); } bool CWalletDB::ErasePool(int64_t nPool) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(std::make_pair(std::string("pool"), nPool)); } @@ -1105,20 +1108,20 @@ void ThreadFlushWalletDB(const string& strFile) if (!GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) return; - unsigned int nLastSeen = nWalletDBUpdated; - unsigned int nLastFlushed = nWalletDBUpdated; + unsigned int nLastSeen = CWalletDB::GetUpdateCounter(); + unsigned int nLastFlushed = CWalletDB::GetUpdateCounter(); int64_t nLastWalletUpdate = GetTime(); while (true) { MilliSleep(500); - if (nLastSeen != nWalletDBUpdated) + if (nLastSeen != CWalletDB::GetUpdateCounter()) { - nLastSeen = nWalletDBUpdated; + nLastSeen = CWalletDB::GetUpdateCounter(); nLastWalletUpdate = GetTime(); } - if (nLastFlushed != nWalletDBUpdated && GetTime() - nLastWalletUpdate >= 2) + if (nLastFlushed != CWalletDB::GetUpdateCounter() && GetTime() - nLastWalletUpdate >= 2) { TRY_LOCK(bitdb.cs_db,lockDb); if (lockDb) @@ -1139,7 +1142,7 @@ void ThreadFlushWalletDB(const string& strFile) if (mi != bitdb.mapFileUseCount.end()) { LogPrint("db", "Flushing %s\n", strFile); - nLastFlushed = nWalletDBUpdated; + nLastFlushed = CWalletDB::GetUpdateCounter(); int64_t nStart = GetTimeMillis(); // Flush wallet file so it's self contained @@ -1282,31 +1285,41 @@ bool CWalletDB::Recover(CDBEnv& dbenv, const std::string& filename) bool CWalletDB::WriteDestData(const std::string &address, const std::string &key, const std::string &value) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("destdata"), std::make_pair(address, key)), value); } bool CWalletDB::EraseDestData(const std::string &address, const std::string &key) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Erase(std::make_pair(std::string("destdata"), std::make_pair(address, key))); } bool CWalletDB::WriteHDSeed(const HDSeed& seed) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("hdseed"), seed.Fingerprint()), seed.RawSeed()); } bool CWalletDB::WriteCryptedHDSeed(const uint256& seedFp, const std::vector& vchCryptedSecret) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::make_pair(std::string("chdseed"), seedFp), vchCryptedSecret); } bool CWalletDB::WriteHDChain(const CHDChain& chain) { - nWalletDBUpdated++; + nWalletDBUpdateCounter++; return Write(std::string("hdchain"), chain); } + +void CWalletDB::IncrementUpdateCounter() +{ + nWalletDBUpdateCounter++; +} + +unsigned int CWalletDB::GetUpdateCounter() +{ + return nWalletDBUpdateCounter; +} diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 087fda714..0a5281fce 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -203,6 +203,8 @@ public: bool WriteSaplingExtendedFullViewingKey(const libzcash::SaplingExtendedFullViewingKey &extfvk); bool EraseSaplingExtendedFullViewingKey(const libzcash::SaplingExtendedFullViewingKey &extfvk); + static void IncrementUpdateCounter(); + static unsigned int GetUpdateCounter(); private: CWalletDB(const CWalletDB&); void operator=(const CWalletDB&); From 8a26f4b8000740f92baebf8ae7ed04f1c72b1c17 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 6 Feb 2017 13:47:24 -0500 Subject: [PATCH 08/20] net: rearrange so that socket accesses can be grouped together zcash: cherry picked from commit 45e2e085612463dd9cca9f1b221733afa6d52991 zcash: https://github.com/bitcoin/bitcoin/pull/9698 --- src/net.cpp | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 95442f500..f82fb6dac 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1145,9 +1145,6 @@ void ThreadSocketHandler() { if (pnode->hSocket == INVALID_SOCKET) continue; - FD_SET(pnode->hSocket, &fdsetError); - hSocketMax = max(hSocketMax, pnode->hSocket); - have_fds = true; // Implement the following logic: // * If there is data to send, select() for sending data. As this only @@ -1164,19 +1161,30 @@ void ThreadSocketHandler() // * We send some data. // * We wait for data to be received (and disconnect after timeout). // * We process a message in the buffer (message handler thread). + + bool select_send; { TRY_LOCK(pnode->cs_vSend, lockSend); - if (lockSend && !pnode->vSendMsg.empty()) { - FD_SET(pnode->hSocket, &fdsetSend); - continue; - } + select_send = lockSend && !pnode->vSendMsg.empty(); } + + FD_SET(pnode->hSocket, &fdsetError); + hSocketMax = max(hSocketMax, pnode->hSocket); + have_fds = true; + + if (select_send) { + FD_SET(pnode->hSocket, &fdsetSend); + continue; + } + bool select_recv; { TRY_LOCK(pnode->cs_vRecvMsg, lockRecv); - if (lockRecv && ( + select_recv = lockRecv && ( pnode->vRecvMsg.empty() || !pnode->vRecvMsg.front().complete() || - pnode->GetTotalRecvSize() <= ReceiveFloodSize())) - FD_SET(pnode->hSocket, &fdsetRecv); + pnode->GetTotalRecvSize() <= ReceiveFloodSize()); + } + if (select_recv) { + FD_SET(pnode->hSocket, &fdsetRecv); } } } @@ -1229,9 +1237,15 @@ void ThreadSocketHandler() // // Receive // + bool recvSet = false; + bool sendSet = false; + bool errorSet = false; if (pnode->hSocket == INVALID_SOCKET) continue; - if (FD_ISSET(pnode->hSocket, &fdsetRecv) || FD_ISSET(pnode->hSocket, &fdsetError)) + recvSet = FD_ISSET(pnode->hSocket, &fdsetRecv); + sendSet = FD_ISSET(pnode->hSocket, &fdsetSend); + errorSet = FD_ISSET(pnode->hSocket, &fdsetError); + if (recvSet || errorSet) { TRY_LOCK(pnode->cs_vRecvMsg, lockRecv); if (lockRecv) @@ -1273,9 +1287,7 @@ void ThreadSocketHandler() // // Send // - if (pnode->hSocket == INVALID_SOCKET) - continue; - if (FD_ISSET(pnode->hSocket, &fdsetSend)) + if (sendSet) { TRY_LOCK(pnode->cs_vSend, lockSend); if (lockSend) From 9825103a10375ae9c8494d1d70d4cd8fd9b3a4ae Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 6 Feb 2017 14:05:45 -0500 Subject: [PATCH 09/20] net: add a lock around hSocket zcash: cherry picked from commit 9a0b784deaab6b9fffcab227d928987b981d0572 zcash: https://github.com/bitcoin/bitcoin/pull/9698 --- src/net.cpp | 63 +++++++++++++++++++++++++++++++++++------------------ src/net.h | 1 + 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index f82fb6dac..1362a8d86 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -444,10 +444,12 @@ static void DumpBanlist() void CNode::CloseSocketDisconnect() { fDisconnect = true; - if (hSocket != INVALID_SOCKET) { - LogPrint("net", "disconnecting peer=%d\n", id); - CloseSocket(hSocket); + LOCK(cs_hSocket); + if (hSocket != INVALID_SOCKET) { + LogPrint("net", "disconnecting peer=%d\n", id); + CloseSocket(hSocket); + } } // in case this fails, we'll empty the recv buffer when the CNode is deleted @@ -781,7 +783,13 @@ void SocketSendData(CNode *pnode) while (it != pnode->vSendMsg.end()) { const CSerializeData &data = *it; assert(data.size() > pnode->nSendOffset); - int nBytes = send(pnode->hSocket, &data[pnode->nSendOffset], data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); + int nBytes = 0; + { + LOCK(pnode->cs_hSocket); + if (pnode->hSocket == INVALID_SOCKET) + break; + nBytes = send(pnode->hSocket, &data[pnode->nSendOffset], data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); + } if (nBytes > 0) { pnode->nLastSend = GetTime(); pnode->nSendBytes += nBytes; @@ -1143,9 +1151,6 @@ void ThreadSocketHandler() LOCK(cs_vNodes); for (CNode* pnode : vNodes) { - if (pnode->hSocket == INVALID_SOCKET) - continue; - // Implement the following logic: // * If there is data to send, select() for sending data. As this only // happens when optimistic write failed, we choose to first drain the @@ -1168,6 +1173,18 @@ void ThreadSocketHandler() select_send = lockSend && !pnode->vSendMsg.empty(); } + bool select_recv; + { + TRY_LOCK(pnode->cs_vRecvMsg, lockRecv); + select_recv = lockRecv && ( + pnode->vRecvMsg.empty() || !pnode->vRecvMsg.front().complete() || + pnode->GetTotalRecvSize() <= ReceiveFloodSize()); + } + + LOCK(pnode->cs_hSocket); + if (pnode->hSocket == INVALID_SOCKET) + continue; + FD_SET(pnode->hSocket, &fdsetError); hSocketMax = max(hSocketMax, pnode->hSocket); have_fds = true; @@ -1176,13 +1193,6 @@ void ThreadSocketHandler() FD_SET(pnode->hSocket, &fdsetSend); continue; } - bool select_recv; - { - TRY_LOCK(pnode->cs_vRecvMsg, lockRecv); - select_recv = lockRecv && ( - pnode->vRecvMsg.empty() || !pnode->vRecvMsg.front().complete() || - pnode->GetTotalRecvSize() <= ReceiveFloodSize()); - } if (select_recv) { FD_SET(pnode->hSocket, &fdsetRecv); } @@ -1240,11 +1250,14 @@ void ThreadSocketHandler() bool recvSet = false; bool sendSet = false; bool errorSet = false; - if (pnode->hSocket == INVALID_SOCKET) - continue; - recvSet = FD_ISSET(pnode->hSocket, &fdsetRecv); - sendSet = FD_ISSET(pnode->hSocket, &fdsetSend); - errorSet = FD_ISSET(pnode->hSocket, &fdsetError); + { + LOCK(pnode->cs_hSocket); + if (pnode->hSocket == INVALID_SOCKET) + continue; + recvSet = FD_ISSET(pnode->hSocket, &fdsetRecv); + sendSet = FD_ISSET(pnode->hSocket, &fdsetSend); + errorSet = FD_ISSET(pnode->hSocket, &fdsetError); + } if (recvSet || errorSet) { TRY_LOCK(pnode->cs_vRecvMsg, lockRecv); @@ -1253,7 +1266,13 @@ void ThreadSocketHandler() { // typical socket buffer is 8K-64K char pchBuf[0x10000]; - int nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT); + int nBytes = 0; + { + LOCK(pnode->cs_hSocket); + if (pnode->hSocket == INVALID_SOCKET) + continue; + nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT); + } if (nBytes > 0) { if (!pnode->ReceiveMsgBytes(pchBuf, nBytes)) @@ -1944,9 +1963,11 @@ public: ~CNetCleanup() { // Close sockets - for (CNode* pnode : vNodes) + for (CNode* pnode : vNodes) { + LOCK(pnode->cs_hSocket); if (pnode->hSocket != INVALID_SOCKET) CloseSocket(pnode->hSocket); + } for (ListenSocket& hListenSocket : vhListenSocket) if (hListenSocket.socket != INVALID_SOCKET) if (!CloseSocket(hListenSocket.socket)) diff --git a/src/net.h b/src/net.h index 1d3c154d2..3db69a035 100644 --- a/src/net.h +++ b/src/net.h @@ -265,6 +265,7 @@ public: uint64_t nSendBytes; std::deque vSendMsg; CCriticalSection cs_vSend; + CCriticalSection cs_hSocket; std::deque vRecvGetData; std::deque vRecvMsg; From 51fb0415c9f62a6c1cdbf3789a3fe13298117ebb Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 6 Feb 2017 02:34:57 -0500 Subject: [PATCH 10/20] net: fix a few races. Credit @TheBlueMatt These are (afaik) all long-standing races or concurrent accesses. Going forward, we can clean these up so that they're not all individual atomic accesses. - Reintroduce cs_vRecv to guard receive-specific vars - Lock vRecv/vSend for CNodeStats - Make some vars atomic. - Only set the connection time in CNode's constructor so that it doesn't change zcash: cherry picked from commit 321d0fc6b6624c65508f8b9059418cb936f0bbbe zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/main.cpp | 2 +- src/net.cpp | 20 ++++++++++++++++---- src/net.h | 19 ++++++++++--------- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index f47a2a5ca..d15310189 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6482,7 +6482,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string if (pingUsecTime > 0) { // Successful ping time measurement, replace previous pfrom->nPingUsecTime = pingUsecTime; - pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime, pingUsecTime); + pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime.load(), pingUsecTime); } else { // This should never happen sProblem = "Timing mishap"; diff --git a/src/net.cpp b/src/net.cpp index 1362a8d86..6d75c2a37 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -653,8 +653,14 @@ void CNode::copyStats(CNodeStats &stats) stats.cleanSubVer = cleanSubVer; stats.fInbound = fInbound; stats.nStartingHeight = nStartingHeight; - stats.nSendBytes = nSendBytes; - stats.nRecvBytes = nRecvBytes; + { + LOCK(cs_vSend); + stats.nSendBytes = nSendBytes; + } + { + LOCK(cs_vRecv); + stats.nRecvBytes = nRecvBytes; + } stats.fWhitelisted = fWhitelisted; // It is common for nodes with good ping times to suddenly become lagged, @@ -792,7 +798,10 @@ void SocketSendData(CNode *pnode) } if (nBytes > 0) { pnode->nLastSend = GetTime(); - pnode->nSendBytes += nBytes; + { + LOCK(pnode->cs_vSend); + pnode->nSendBytes += nBytes; + } pnode->nSendOffset += nBytes; pnode->RecordBytesSent(nBytes); if (pnode->nSendOffset == data.size()) { @@ -1278,7 +1287,10 @@ void ThreadSocketHandler() if (!pnode->ReceiveMsgBytes(pchBuf, nBytes)) pnode->CloseSocketDisconnect(); pnode->nLastRecv = GetTime(); - pnode->nRecvBytes += nBytes; + { + LOCK(pnode->cs_vRecv); + pnode->nRecvBytes += nBytes; + } pnode->RecordBytesRecv(nBytes); } else if (nBytes == 0) diff --git a/src/net.h b/src/net.h index 3db69a035..c1207661d 100644 --- a/src/net.h +++ b/src/net.h @@ -266,6 +266,7 @@ public: std::deque vSendMsg; CCriticalSection cs_vSend; CCriticalSection cs_hSocket; + CCriticalSection cs_vRecv; std::deque vRecvGetData; std::deque vRecvMsg; @@ -273,10 +274,10 @@ public: uint64_t nRecvBytes; int nRecvVersion; - int64_t nLastSend; - int64_t nLastRecv; + std::atomic nLastSend; + std::atomic nLastRecv; int64_t nTimeConnected; - int64_t nTimeOffset; + std::atomic nTimeOffset; const CAddress addr; std::string addrName; CService addrLocal; @@ -302,8 +303,8 @@ public: CSemaphoreGrant grantOutbound; CCriticalSection cs_filter; CBloomFilter* pfilter; - int nRefCount; NodeId id; + std::atomic nRefCount; const uint64_t nKeyedNetGroup; @@ -346,15 +347,15 @@ public: // Ping time measurement: // The pong reply we're expecting, or 0 if no pong expected. - uint64_t nPingNonceSent; + std::atomic nPingNonceSent; // Time (in usec) the last ping was sent, or 0 if no ping was ever sent. - int64_t nPingUsecStart; + std::atomic nPingUsecStart; // Last measured round-trip time. - int64_t nPingUsecTime; + std::atomic nPingUsecTime; // Best measured round-trip time. - int64_t nMinPingUsecTime; + std::atomic nMinPingUsecTime; // Whether a ping is requested. - bool fPingQueued; + std::atomic fPingQueued; CNode(SOCKET hSocketIn, const CAddress &addrIn, const std::string &addrNameIn = "", bool fInboundIn = false); ~CNode(); From 208a1a5867c2b98aa3825fb1a96089f5196d5caf Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:42:49 -0500 Subject: [PATCH 11/20] Make nTimeConnected const in CNode zcash: cherry picked from commit 644f1234e22626a7b5618a1dae60a8457a4063b1 zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/net.cpp | 4 +--- src/net.h | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 6d75c2a37..386cabb42 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -409,8 +409,6 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest) vNodes.push_back(pnode); } - pnode->nTimeConnected = GetTime(); - return pnode; } else if (!proxyConnectionFailed) { // If connecting to the node failed, and failure is not caused by a problem connecting to @@ -2204,6 +2202,7 @@ unsigned int SendBufferSize() { return 1000*GetArg("-maxsendbuffer", DEFAULT_MAX CNode::CNode(SOCKET hSocketIn, const CAddress& addrIn, const std::string& addrNameIn, bool fInboundIn) : ssSend(SER_NETWORK, INIT_PROTO_VERSION), + nTimeConnected(GetTime()), addr(addrIn), nKeyedNetGroup(CalculateKeyedNetGroup(addrIn)), addrKnown(5000, 0.001), @@ -2216,7 +2215,6 @@ CNode::CNode(SOCKET hSocketIn, const CAddress& addrIn, const std::string& addrNa nLastRecv = 0; nSendBytes = 0; nRecvBytes = 0; - nTimeConnected = GetTime(); nTimeOffset = 0; addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn; nVersion = 0; diff --git a/src/net.h b/src/net.h index c1207661d..fffbe070e 100644 --- a/src/net.h +++ b/src/net.h @@ -276,7 +276,7 @@ public: std::atomic nLastSend; std::atomic nLastRecv; - int64_t nTimeConnected; + const int64_t nTimeConnected; std::atomic nTimeOffset; const CAddress addr; std::string addrName; From 5d3586f707d9c787fa5ab0c1b34ac46ec77ce6f7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:44:38 -0500 Subject: [PATCH 12/20] Avoid copying CNodeStats to make helgrind OK with buggy std::string zcash: cherry picked from commit ae683c1b1960b32134f5a5a29504691c91f39cf3 zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/rpc/net.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index da2154e6a..00832acfa 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -68,9 +68,8 @@ static void CopyNodeStats(std::vector& vstats) LOCK(cs_vNodes); vstats.reserve(vNodes.size()); for (CNode* pnode : vNodes) { - CNodeStats stats; - pnode->copyStats(stats); - vstats.push_back(stats); + vstats.emplace_back(); + pnode->copyStats(vstats.back()); } } From c2c43e39b22c020123a07fcc802be49c0c4775d0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:53:34 -0500 Subject: [PATCH 13/20] Access fRelayTxes with cs_filter lock in copyStats zcash: cherry picked from commit 512731bed0782f10092de35a960153b17ecc11eb zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/net.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index 386cabb42..aaf22de2c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -641,7 +641,10 @@ void CNode::copyStats(CNodeStats &stats) { stats.nodeid = this->GetId(); stats.nServices = nServices; - stats.fRelayTxes = fRelayTxes; + { + LOCK(cs_filter); + stats.fRelayTxes = fRelayTxes; + } stats.nLastSend = nLastSend; stats.nLastRecv = nLastRecv; stats.nTimeConnected = nTimeConnected; From 651c46a4b329680eaa7fa51680ea24f7c5f89fc8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:15:30 -0500 Subject: [PATCH 14/20] Make nStartingHeight atomic zcash: cherry picked from commit 96f42d8a12871b8d5c4e31fd27d8135f97c6b3e0 zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/main.cpp | 7 +++++-- src/net.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index d15310189..a646af146 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5754,8 +5754,11 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string vRecv >> LIMITED_STRING(pfrom->strSubVer, MAX_SUBVERSION_LENGTH); pfrom->cleanSubVer = SanitizeString(pfrom->strSubVer, SAFE_CHARS_SUBVERSION); } - if (!vRecv.empty()) - vRecv >> pfrom->nStartingHeight; + if (!vRecv.empty()) { + int nStartingHeight; + vRecv >> nStartingHeight; + pfrom->nStartingHeight = nStartingHeight; + } if (!vRecv.empty()) vRecv >> pfrom->fRelayTxes; // set to true after we get the first filter* message else diff --git a/src/net.h b/src/net.h index fffbe070e..da2514b3a 100644 --- a/src/net.h +++ b/src/net.h @@ -330,7 +330,7 @@ protected: public: uint256 hashContinue; - int nStartingHeight; + std::atomic nStartingHeight; // flood relay std::vector vAddrToSend; From 676e7814f3498e3d8e59397546e373425300a839 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:20:16 -0500 Subject: [PATCH 15/20] Make nServices atomic zcash: cherry picked from commit 0f3187261519c7568ef4211ce12b9740a3c1200f zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/main.cpp | 4 +++- src/net.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index a646af146..076075f26 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5718,7 +5718,9 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string CAddress addrMe; CAddress addrFrom; uint64_t nNonce = 1; - vRecv >> pfrom->nVersion >> pfrom->nServices >> nTime >> addrMe; + uint64_t nServices; + vRecv >> pfrom->nVersion >> nServices >> nTime >> addrMe; + pfrom->nServices = nServices; if (pfrom->nVersion < MIN_PEER_PROTO_VERSION) { // disconnect from peers older than this proto version diff --git a/src/net.h b/src/net.h index da2514b3a..ebee5b83e 100644 --- a/src/net.h +++ b/src/net.h @@ -256,7 +256,7 @@ class CNode { public: // socket - uint64_t nServices; + std::atomic nServices; SOCKET hSocket; CDataStream ssSend; std::string strSendCommand; // Current command being assembled in ssSend From 78e0b89750b6aaccce6547f99d05a99fe778c15e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:08:31 -0500 Subject: [PATCH 16/20] Move [clean|str]SubVer writes/copyStats into a lock zcash: cherry picked from commit 22b4966a29501c4f3f2e970ac5008fbd91e665a9 zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/main.cpp | 13 ++++++++++--- src/net.cpp | 5 ++++- src/net.h | 1 + 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 076075f26..51b719128 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5718,6 +5718,8 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string CAddress addrMe; CAddress addrFrom; uint64_t nNonce = 1; + std::string strSubVer; + std::string cleanSubVer; uint64_t nServices; vRecv >> pfrom->nVersion >> nServices >> nTime >> addrMe; pfrom->nServices = nServices; @@ -5753,8 +5755,8 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string if (!vRecv.empty()) vRecv >> addrFrom >> nNonce; if (!vRecv.empty()) { - vRecv >> LIMITED_STRING(pfrom->strSubVer, MAX_SUBVERSION_LENGTH); - pfrom->cleanSubVer = SanitizeString(pfrom->strSubVer, SAFE_CHARS_SUBVERSION); + vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); + cleanSubVer = SanitizeString(strSubVer, SAFE_CHARS_SUBVERSION); } if (!vRecv.empty()) { int nStartingHeight; @@ -5784,6 +5786,11 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string if (pfrom->fInbound) pfrom->PushVersion(); + { + LOCK(pfrom->cs_SubVer); + pfrom->strSubVer = strSubVer; + pfrom->cleanSubVer = cleanSubVer; + } pfrom->fClient = !(pfrom->nServices & NODE_NETWORK); // Potentially mark this peer as a preferred download peer. @@ -5843,7 +5850,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string remoteAddr = ", peeraddr=" + pfrom->addr.ToString(); LogPrintf("receive version message: %s: version %d, blocks=%d, us=%s, peer=%d%s\n", - pfrom->cleanSubVer, pfrom->nVersion, + cleanSubVer, pfrom->nVersion, pfrom->nStartingHeight, addrMe.ToString(), pfrom->id, remoteAddr); diff --git a/src/net.cpp b/src/net.cpp index aaf22de2c..dc9b8669f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -651,7 +651,10 @@ void CNode::copyStats(CNodeStats &stats) stats.nTimeOffset = nTimeOffset; stats.addrName = addrName; stats.nVersion = nVersion; - stats.cleanSubVer = cleanSubVer; + { + LOCK(cs_SubVer); + stats.cleanSubVer = cleanSubVer; + } stats.fInbound = fInbound; stats.nStartingHeight = nStartingHeight; { diff --git a/src/net.h b/src/net.h index ebee5b83e..f62fa7567 100644 --- a/src/net.h +++ b/src/net.h @@ -287,6 +287,7 @@ public: // store the sanitized version in cleanSubVer. The original should be used when dealing with // the network or wire types and the cleaned string used when displayed or logged. std::string strSubVer, cleanSubVer; + CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer bool fWhitelisted; // This peer can bypass DoS banning. bool fOneShot; bool fClient; From e6eac19062ebafaafdbf5b881f8195dfbc98d575 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 17:38:57 -0500 Subject: [PATCH 17/20] Make nTimeBestReceived atomic zcash: cherry picked from commit d8f2b8a8c032b83a3bd90750e58abaeece7e34e7 zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 51b719128..ec52f1a6d 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -65,7 +65,7 @@ CCriticalSection cs_main; BlockMap mapBlockIndex; CChain chainActive; CBlockIndex *pindexBestHeader = NULL; -static int64_t nTimeBestReceived = 0; +static std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block CWaitableCriticalSection csBestBlock; CConditionVariable cvBlockChange; int nScriptCheckThreads = 0; From 116deeec5b6aa01dcafd7847c786b6d74b893011 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:04:34 -0500 Subject: [PATCH 18/20] Move CNode::addrName accesses behind locked accessors zcash: cherry picked from commit 036073bf87c07f8d69e39168dd93a52f1aafe85c zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/main.cpp | 2 +- src/net.cpp | 21 ++++++++++++++++++--- src/net.h | 7 ++++++- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index ec52f1a6d..452aae823 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -362,7 +362,7 @@ int64_t GetBlockTimeout(int64_t nTime, int nValidatedQueuedBefore, const Consens void InitializeNode(NodeId nodeid, const CNode *pnode) { LOCK(cs_main); CNodeState &state = mapNodeState.insert(std::make_pair(nodeid, CNodeState())).first->second; - state.name = pnode->addrName; + state.name = pnode->GetAddrName(); state.address = pnode->addr; } diff --git a/src/net.cpp b/src/net.cpp index dc9b8669f..77e212ad0 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -350,9 +350,11 @@ CNode* FindNode(const CSubNet& subNet) CNode* FindNode(const std::string& addrName) { LOCK(cs_vNodes); - for (CNode* pnode : vNodes) - if (pnode->addrName == addrName) + for (CNode* pnode : vNodes) { + if (pnode->GetAddrName() == addrName) { return (pnode); + } + } return NULL; } @@ -637,6 +639,19 @@ void CNode::AddWhitelistedRange(const CSubNet &subnet) { vWhitelistedRange.push_back(subnet); } + +std::string CNode::GetAddrName() const { + LOCK(cs_addrName); + return addrName; +} + +void CNode::MaybeSetAddrName(const std::string& addrNameIn) { + LOCK(cs_addrName); + if (addrName.empty()) { + addrName = addrNameIn; + } +} + void CNode::copyStats(CNodeStats &stats) { stats.nodeid = this->GetId(); @@ -649,7 +664,7 @@ void CNode::copyStats(CNodeStats &stats) stats.nLastRecv = nLastRecv; stats.nTimeConnected = nTimeConnected; stats.nTimeOffset = nTimeOffset; - stats.addrName = addrName; + stats.addrName = GetAddrName(); stats.nVersion = nVersion; { LOCK(cs_SubVer); diff --git a/src/net.h b/src/net.h index f62fa7567..260ef6666 100644 --- a/src/net.h +++ b/src/net.h @@ -279,7 +279,6 @@ public: const int64_t nTimeConnected; std::atomic nTimeOffset; const CAddress addr; - std::string addrName; CService addrLocal; int nVersion; // strSubVer is whatever byte array we read from the wire. However, this field is intended @@ -379,6 +378,9 @@ private: static uint64_t CalculateKeyedNetGroup(const CAddress& ad); + + mutable CCriticalSection cs_addrName; + std::string addrName; public: // Regenerate the span for this CNode. This re-queries the log filter to see @@ -703,6 +705,9 @@ public: //!response the time in seconds left in the current max outbound cycle // in case of no limit, it will always respond with 0 static uint64_t GetMaxOutboundTimeLeftInCycle(); + std::string GetAddrName() const; + //! Sets the addrName only if it was not previously set + void MaybeSetAddrName(const std::string& addrNameIn); }; From c9e2172eb693597c00c778d24d0ba98c01c81ac3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:18:51 -0500 Subject: [PATCH 19/20] Move CNode::addrLocal access behind locked accessors zcash: cherry picked from commit db2dc7a58cb0a3df58188b748df8e0d04ba76f00 zcash: https://github.com/bitcoin/bitcoin/pull/9708 --- src/main.cpp | 4 ++-- src/net.cpp | 24 ++++++++++++++++++++---- src/net.h | 8 +++++++- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 452aae823..b5e28a3ee 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5776,7 +5776,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string return true; } - pfrom->addrLocal = addrMe; + pfrom->SetAddrLocal(addrMe); if (pfrom->fInbound && addrMe.IsRoutable()) { SeenLocal(addrMe); @@ -5815,7 +5815,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string LogPrintf("ProcessMessages: advertizing address %s\n", addr.ToString()); pfrom->PushAddress(addr, insecure_rand); } else if (IsPeerAddrLocalGood(pfrom)) { - addr.SetIP(pfrom->addrLocal); + addr.SetIP(addrMe); LogPrintf("ProcessMessages: advertizing address %s\n", addr.ToString()); pfrom->PushAddress(addr, insecure_rand); } diff --git a/src/net.cpp b/src/net.cpp index 77e212ad0..c35596b91 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -192,8 +192,9 @@ int GetnScore(const CService& addr) // Is our peer's addrLocal potentially useful as an external IP source? bool IsPeerAddrLocalGood(CNode *pnode) { - return fDiscover && pnode->addr.IsRoutable() && pnode->addrLocal.IsRoutable() && - !IsLimited(pnode->addrLocal.GetNetwork()); + CService addrLocal = pnode->GetAddrLocal(); + return fDiscover && pnode->addr.IsRoutable() && addrLocal.IsRoutable() && + !IsLimited(addrLocal.GetNetwork()); } // pushes our own address to a peer @@ -208,7 +209,7 @@ void AdvertizeLocal(CNode *pnode) if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || GetRand((GetnScore(addrLocal) > LOCAL_MANUAL) ? 8:2) == 0)) { - addrLocal.SetIP(pnode->addrLocal); + addrLocal.SetIP(pnode->GetAddrLocal()); } if (addrLocal.IsRoutable()) { @@ -652,6 +653,20 @@ void CNode::MaybeSetAddrName(const std::string& addrNameIn) { } } +CService CNode::GetAddrLocal() const { + LOCK(cs_addrLocal); + return addrLocal; +} + +void CNode::SetAddrLocal(const CService& addrLocalIn) { + LOCK(cs_addrLocal); + if (addrLocal.IsValid()) { + error("Addr local already set for node: %i. Refusing to change from %s to %s", id, addrLocal.ToString(), addrLocalIn.ToString()); + } else { + addrLocal = addrLocalIn; + } +} + void CNode::copyStats(CNodeStats &stats) { stats.nodeid = this->GetId(); @@ -698,7 +713,8 @@ void CNode::copyStats(CNodeStats &stats) stats.dPingWait = (((double)nPingUsecWait) / 1e6); // Leave string empty if addrLocal invalid (not filled in yet) - stats.addrLocal = addrLocal.IsValid() ? addrLocal.ToString() : ""; + CService addrLocalUnlocked = GetAddrLocal(); + stats.addrLocal = addrLocalUnlocked.IsValid() ? addrLocalUnlocked.ToString() : ""; } // requires LOCK(cs_vRecvMsg) diff --git a/src/net.h b/src/net.h index 260ef6666..0dd4d9bc4 100644 --- a/src/net.h +++ b/src/net.h @@ -279,7 +279,6 @@ public: const int64_t nTimeConnected; std::atomic nTimeOffset; const CAddress addr; - CService addrLocal; int nVersion; // strSubVer is whatever byte array we read from the wire. However, this field is intended // to be printed out, displayed to humans in various forms and so on. So we sanitize it and @@ -381,6 +380,9 @@ private: mutable CCriticalSection cs_addrName; std::string addrName; + + CService addrLocal; + mutable CCriticalSection cs_addrLocal; public: // Regenerate the span for this CNode. This re-queries the log filter to see @@ -417,6 +419,10 @@ public: msg.SetVersion(nVersionIn); } + CService GetAddrLocal() const; + //! May not be called more than once + void SetAddrLocal(const CService& addrLocalIn); + CNode* AddRef() { nRefCount++; From 4fd7387cef7ca7bb0a774ee8947aab47ebe6cc7e Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 15 Feb 2017 17:01:30 -0500 Subject: [PATCH 20/20] Add missing cs_wallet lock that triggers new lock held assertion A new AssertLockHeld(cs_wallet) call was added in commit a58370e "Dedup nTimeFirstKey update logic" (part of PR #9108). The lock held assertion will fail when loading prexisting wallets files from before the #9108 merge that have watch-only keys. zcash: cherry picked from commit 07afcd6379bb46ace5856f6a47a9188cf9aed2ea zcash: https://github.com/bitcoin/bitcoin/pull/9771 --- src/wallet/walletdb.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 642dbf7a6..c1d64919e 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -907,8 +907,8 @@ DBErrors CWalletDB::LoadWallet(CWallet* pwallet) bool fNoncriticalErrors = false; DBErrors result = DB_LOAD_OK; + LOCK(pwallet->cs_wallet); try { - LOCK(pwallet->cs_wallet); int nMinVersion = 0; if (Read((string)"minversion", nMinVersion)) {