From 4993f6e8940ff82339873a758f80c1015fe86882 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 6 Jan 2021 07:07:32 +0100 Subject: [PATCH 1/5] Merge #20816: net: Move RecordBytesSent() call out of cs_vSend lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 378aedc45248cea82d9a3e6dc1038d6828008a76 [net] Add cs_vSend lock annotations (John Newbery) 673254515a2f97e53dd8c7335c836b083ba7e31a [net] Move RecordBytesSent() call out of cs_vSend lock (John Newbery) Pull request description: RecordBytesSent() does not require cs_vSend to be locked, so reduce the scope of cs_vSend. Also correctly annotate the CNode data members that are guarded by cs_vSend. This is a simpler alternative to #19673. ACKs for top commit: jnewbery: ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs. troygiorshev: ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 theStack: re-ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 MarcoFalke: review ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 🔌 Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07 --- src/net.cpp | 23 +++++++++++++---------- src/net.h | 8 +++++--- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index b48efb533d390..ed0154e8c356e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1399,11 +1399,16 @@ void CConnman::CalculateNumConnectionsChangedStats() mapSentBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0; auto vNodesCopy = CopyNodeVector(CConnman::FullyConnectedOnly); for (auto pnode : vNodesCopy) { - LOCK(pnode->cs_vRecv); - for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd) - mapRecvBytesMsgStats[i.first] += i.second; - for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd) - mapSentBytesMsgStats[i.first] += i.second; + { + LOCK(pnode->cs_vRecv); + for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd) + mapRecvBytesMsgStats[i.first] += i.second; + } + { + LOCK(pnode->cs_vSend); + for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd) + mapSentBytesMsgStats[i.first] += i.second; + } if(pnode->fClient) spvNodes++; else @@ -1877,11 +1882,9 @@ void CConnman::SocketHandler() break; } - LOCK(pnode->cs_vSend); - size_t nBytes = SocketSendData(pnode); - if (nBytes) { - RecordBytesSent(nBytes); - } + // Send data + size_t bytes_sent = WITH_LOCK(pnode->cs_vSend, return SocketSendData(pnode)); + if (bytes_sent) RecordBytesSent(bytes_sent); } ReleaseNodeVector(vErrorNodes); diff --git a/src/net.h b/src/net.h index 65058c0b65b2d..b7be35ae20d55 100644 --- a/src/net.h +++ b/src/net.h @@ -1026,8 +1026,10 @@ class CNode NetPermissionFlags m_permissionFlags{ PF_NONE }; std::atomic nServices{NODE_NONE}; SOCKET hSocket GUARDED_BY(cs_hSocket); - size_t nSendSize{0}; // total size of all vSendMsg entries - size_t nSendOffset{0}; // offset inside the first vSendMsg already sent + /** Total size of all vSendMsg entries */ + size_t nSendSize GUARDED_BY(cs_vSend){0}; + /** Offset inside the first vSendMsg already sent */ + size_t nSendOffset GUARDED_BY(cs_vSend){0}; uint64_t nSendBytes GUARDED_BY(cs_vSend){0}; std::list> vSendMsg GUARDED_BY(cs_vSend); std::atomic nSendMsgSize{0}; @@ -1119,7 +1121,7 @@ class CNode Network ConnectedThroughNetwork() const; protected: - mapMsgCmdSize mapSendBytesPerMsgCmd; + mapMsgCmdSize mapSendBytesPerMsgCmd GUARDED_BY(cs_vSend); mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); public: From 069624dd622bbec90b6a12ea0e65015cfc8145d0 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 6 Jan 2021 08:35:53 +0100 Subject: [PATCH 2/5] Merge #20844: test: Add sanitizer suppressions for AMD EPYC CPUs fa6c114ae604571435e8c4d25906a8b6d5b9984c test: Add sanitizer suppressions for AMD EPYC CPUs (MarcoFalke) Pull request description: Currently the ci system only runs on intel cpus (and some arm devices), but it won't run on CPUs `Using the 'shani(1way,2way)' SHA256 implementation` (excerpt from debug log). For reference, google cloud CPUs (which is what Cirrus CI uses) print `Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation` The traceback I got: ``` crypto/sha256_shani.cpp:87:18: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long') #0 0x55c0000e95ec in sha256_shani::Transform(unsigned int*, unsigned char const*, unsigned long) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256_shani.cpp:87:18 #1 0x55bfffb926f8 in (anonymous namespace)::SelfTest() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:517:9 #2 0x55bfffb906ed in SHA256AutoDetect[abi:cxx11]() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:626:5 #3 0x55bfff87ab97 in BasicTestingSetup::BasicTestingSetup(std::__cxx11::basic_string, std::allocator > const&, std::vector > const&) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:104:5 #4 0x55bffe885877 in main /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_main.cpp:52:27 #5 0x7f20c3bf60b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2) #6 0x55bffe7a5f6d in _start (/root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x1d00f6d) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow crypto/sha256_shani.cpp:87:18 in ACKs for top commit: laanwj: Anyhow ACK fa6c114ae604571435e8c4d25906a8b6d5b9984c Tree-SHA512: 968a1d28eedec58c337b1323862f583cb1bcd78c5f03396940b9ab53ded12f8c6652877909aba05ee5586532137418fd817ff979bd7bef6e07856094f9d7f9b1 --- test/sanitizer_suppressions/ubsan | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index d69e3e8af8960..b181d1e97fea2 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -16,12 +16,7 @@ unsigned-integer-overflow:chain.cpp unsigned-integer-overflow:chain.h unsigned-integer-overflow:coded_stream.h unsigned-integer-overflow:core_write.cpp -unsigned-integer-overflow:crypto/chacha20.cpp -unsigned-integer-overflow:crypto/ctaes/ctaes.c -unsigned-integer-overflow:crypto/ripemd160.cpp -unsigned-integer-overflow:crypto/sha1.cpp -unsigned-integer-overflow:crypto/sha256.cpp -unsigned-integer-overflow:crypto/sha512.cpp +unsigned-integer-overflow:crypto/* unsigned-integer-overflow:hash.cpp unsigned-integer-overflow:leveldb/db/log_reader.cc unsigned-integer-overflow:leveldb/util/bloom.cc From e554d3a02e58a298c9f2b10a8bee443cbfc8ed54 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 26 Apr 2020 19:57:33 -0400 Subject: [PATCH 3/5] Merge #18669: log: Use Join() helper when listing log categories faec0638872798b58b9882ee079014555bc8393e log: Use Join() helper when listing log categories (MarcoFalke) Pull request description: This removes the global `ListLogCategories` and replaces it with a one-line member function `LogCategoriesString`, which just calls `Join`. Should be a straightforward refactor to get rid of a few LOC. ACKs for top commit: laanwj: ACK faec0638872798b58b9882ee079014555bc8393e promag: ACK faec0638872798b58b9882ee079014555bc8393e, I also think it's fine as it is (re https://github.com/bitcoin/bitcoin/pull/18669#discussion_r412944724). Tree-SHA512: 2f51f9ce1246eda5630015f3a869e36953c7eb34f311baad576b92d7829e4e88051c6189436271cd0a13732a49698506345b446b98fd28e58edfb5b62169f1c9 --- src/init.cpp | 2 +- src/logging.cpp | 43 ++++--------------------------------------- src/logging.h | 20 +++++++++----------- src/rpc/misc.cpp | 9 ++++----- 4 files changed, 18 insertions(+), 56 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0d10dc13ca488..92b7cb64ffd5b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -723,7 +723,7 @@ void SetupServerArgs(NodeContext& node) argsman.AddArg("-addrmantest", "Allows to test address relay on localhost", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-debug=", "Output debugging information (default: -nodebug, supplying is optional). " - "If is not supplied or if = 1, output all debugging information. can be: " + ListLogCategories() + ".", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); + "If is not supplied or if = 1, output all debugging information. can be: " + LogInstance().LogCategoriesString() + ".", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-debugexclude=", strprintf("Exclude debugging information for a category. Can be used in conjunction with -debug=1 to output debug logs for all categories except one or more specified categories."), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-disablegovernance", strprintf("Disable governance validation (0-1, default: %u)", 0), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-logips", strprintf("Include IP addresses in debug output (default: %u)", DEFAULT_LOGIPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); diff --git a/src/logging.cpp b/src/logging.cpp index f35407a513bdc..7b4a5956c75c8 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -197,56 +197,21 @@ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str) return false; } -std::string ListLogCategories() +std::vector BCLog::Logger::LogCategoriesList() const { - std::string ret; - int outcount = 0; + std::vector ret; for (const CLogCategoryDesc& category_desc : LogCategories) { // Omit the special cases. if (category_desc.flag != BCLog::NONE && category_desc.flag != BCLog::ALL && category_desc.flag != BCLog::DASH) { - if (outcount != 0) ret += ", "; - ret += category_desc.category; - outcount++; - } - } - return ret; -} - -std::vector ListActiveLogCategories() -{ - std::vector ret; - for (const CLogCategoryDesc& category_desc : LogCategories) { - // Omit the special cases. - if (category_desc.flag != BCLog::NONE && category_desc.flag != BCLog::ALL && category_desc.flag != BCLog::DASH) { - CLogCategoryActive catActive; + LogCategory catActive; catActive.category = category_desc.category; - catActive.active = LogAcceptCategory(category_desc.flag); + catActive.active = WillLogCategory(category_desc.flag); ret.push_back(catActive); } } return ret; } -std::string ListActiveLogCategoriesString() -{ - if (LogInstance().GetCategoryMask() == BCLog::NONE) - return "0"; - if (LogInstance().GetCategoryMask() == BCLog::ALL) - return "1"; - - std::string ret; - int outcount = 0; - for (const CLogCategoryDesc& category_desc : LogCategories) { - // Omit the special cases. - if (category_desc.flag != BCLog::NONE && category_desc.flag != BCLog::ALL && category_desc.flag != BCLog::DASH && LogAcceptCategory(category_desc.flag)) { - if (outcount != 0) ret += ", "; - ret += category_desc.category; - outcount++; - } - } - return ret; -} - std::string BCLog::Logger::LogTimestampStr(const std::string& str) { std::string strStamped; diff --git a/src/logging.h b/src/logging.h index 9a5476d8057ec..657fe54cf357e 100644 --- a/src/logging.h +++ b/src/logging.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -26,8 +27,7 @@ extern const char * const DEFAULT_DEBUGLOGFILE; extern bool fLogThreadNames; extern bool fLogIPs; -struct CLogCategoryActive -{ +struct LogCategory { std::string category; bool active; }; @@ -157,6 +157,13 @@ namespace BCLog { bool DisableCategory(const std::string& str); bool WillLogCategory(LogFlags category) const; + /** Returns a vector of the log categories */ + std::vector LogCategoriesList(); + /** Returns a string with the log categories */ + std::string LogCategoriesString() + { + return Join(LogCategoriesList(), ", ", [&](const LogCategory& i) { return i.category; }); + }; bool DefaultShrinkDebugFile() const; }; @@ -171,15 +178,6 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category) return LogInstance().WillLogCategory(category); } -/** Returns a string with the log categories. */ -std::string ListLogCategories(); - -/** Returns a string with the list of active log categories */ -std::string ListActiveLogCategoriesString(); - -/** Returns a vector of the active log categories. */ -std::vector ListActiveLogCategories(); - /** Return true if str parses as a log category and set the flag */ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str); diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 7f6f7f9978760..76a3913ae5833 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -44,7 +44,7 @@ static UniValue debug(const JSONRPCRequest& request) { RPCHelpMan{"debug", "Change debug category on the fly. Specify single category or use '+' to specify many.\n" - "The valid debug categories are: " + ListLogCategories() + ".\n" + "The valid logging categories are: " + LogInstance().LogCategoriesString() + ".\n" "libevent logging is configured on startup and cannot be modified by this RPC during runtime.\n" "There are also a few meta-categories:\n" " - \"all\", \"1\" and \"\" activate all categories at once;\n" @@ -72,7 +72,7 @@ static UniValue debug(const JSONRPCRequest& request) } } - return "Debug mode: " + ListActiveLogCategoriesString(); + return "Debug mode: " + LogInstance().LogCategoriesString(); } static UniValue mnsync(const JSONRPCRequest& request) @@ -1228,7 +1228,7 @@ static UniValue logging(const JSONRPCRequest& request) "When called with arguments, adds or removes categories from debug logging and return the lists above.\n" "The arguments are evaluated in order \"include\", \"exclude\".\n" "If an item is both included and excluded, it will thus end up being excluded.\n" - "The valid logging categories are: " + ListLogCategories() + "\n" + "The valid logging categories are: " + LogInstance().LogCategoriesString() + "\n" "In addition, the following are available as category names with special meanings:\n" " - \"all\", \"1\" : represent all logging categories.\n" " - \"dash\" activates all Dash-specific categories at once.\n" @@ -1282,8 +1282,7 @@ static UniValue logging(const JSONRPCRequest& request) } UniValue result(UniValue::VOBJ); - std::vector vLogCatActive = ListActiveLogCategories(); - for (const auto& logCatActive : vLogCatActive) { + for (const auto& logCatActive : LogInstance().LogCategoriesList()) { result.pushKV(logCatActive.category, logCatActive.active); } From a298eb2b934044edef11ba6118c688e2dec940e2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 7 Jan 2021 09:03:35 +0100 Subject: [PATCH 4/5] Merge #20584: Declare de facto const reference variables/member functions as const 31b136e5802e1b1e5f9a9589736afe0652f34da2 Don't declare de facto const reference variables as non-const (practicalswift) 1c65c075ee4c7f98d9c1fac5ed7576b96374d4e9 Don't declare de facto const member functions as non-const (practicalswift) Pull request description: _Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_ Changes in this PR: * Don't declare de facto const member functions as non-const * Don't declare de facto const reference variables as non-const Awards for finding candidates for the above changes go to: * `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html)) * `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/)) See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`. ACKs for top commit: ajtowns: ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 jonatack: ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 theStack: ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 :snowflake: Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe --- src/addrman.cpp | 2 +- src/logging.h | 4 ++-- src/miner.cpp | 2 +- src/miner.h | 2 +- src/net.cpp | 2 +- src/net.h | 2 +- src/rpc/blockchain.cpp | 2 +- src/script/interpreter.cpp | 4 ++-- src/script/sign.cpp | 2 +- src/test/checkqueue_tests.cpp | 8 ++++---- src/test/util/setup_common.cpp | 4 ++-- src/test/util/setup_common.h | 4 ++-- src/validation.cpp | 2 +- src/wallet/wallet.cpp | 4 ++-- 14 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 50ec471981f39..b850488869a74 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -725,7 +725,7 @@ CAddrInfo CAddrMan::SelectTriedCollision_() return CAddrInfo(); } - CAddrInfo& newInfo = mapInfo[id_new]; + const CAddrInfo& newInfo = mapInfo[id_new]; // which tried bucket to move the entry to int tried_bucket = newInfo.GetTriedBucket(nKey, m_asmap); diff --git a/src/logging.h b/src/logging.h index 657fe54cf357e..7810e318cebe0 100644 --- a/src/logging.h +++ b/src/logging.h @@ -158,9 +158,9 @@ namespace BCLog { bool WillLogCategory(LogFlags category) const; /** Returns a vector of the log categories */ - std::vector LogCategoriesList(); + std::vector LogCategoriesList() const; /** Returns a string with the log categories */ - std::string LogCategoriesString() + std::string LogCategoriesString() const { return Join(LogCategoriesList(), ", ", [&](const LogCategory& i) { return i.category; }); }; diff --git a/src/miner.cpp b/src/miner.cpp index 47c6de81df5cc..059c0781d72fc 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -290,7 +290,7 @@ bool BlockAssembler::TestPackage(uint64_t packageSize, unsigned int packageSigOp // Perform transaction-level checks before adding to block: // - transaction finality (locktime) // - safe TXs in regard to ChainLocks -bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& package) +bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& package) const { for (CTxMemPool::txiter it : package) { if (!IsFinalTx(it->GetTx(), nHeight, nLockTimeCutoff)) diff --git a/src/miner.h b/src/miner.h index e321c132783e9..18a3c022c0fd6 100644 --- a/src/miner.h +++ b/src/miner.h @@ -205,7 +205,7 @@ class BlockAssembler * locktime * These checks should always succeed, and they're here * only as an extra check in case of suboptimal node configuration */ - bool TestPackageTransactions(const CTxMemPool::setEntries& package); + bool TestPackageTransactions(const CTxMemPool::setEntries& package) const; /** Return true if given transaction from mapTx has already been evaluated, * or if the transaction's cached data in mapTx is incorrect. */ bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs); diff --git a/src/net.cpp b/src/net.cpp index ed0154e8c356e..ecd96534e0e46 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1441,7 +1441,7 @@ void CConnman::CalculateNumConnectionsChangedStats() statsClient.gauge("peers.torConnections", torNodes, 1.0f); } -void CConnman::InactivityCheck(CNode *pnode) +void CConnman::InactivityCheck(CNode *pnode) const { int64_t nTime = GetSystemTimeInSeconds(); if (nTime - pnode->nTimeConnected > m_peer_connect_timeout) diff --git a/src/net.h b/src/net.h index b7be35ae20d55..08716e5a4a2d1 100644 --- a/src/net.h +++ b/src/net.h @@ -532,7 +532,7 @@ friend class CNode; void DisconnectNodes(); void NotifyNumConnectionsChanged(); void CalculateNumConnectionsChangedStats(); - void InactivityCheck(CNode *pnode); + void InactivityCheck(CNode *pnode) const; bool GenerateSelectSet(std::set &recv_set, std::set &send_set, std::set &error_set); #ifdef USE_KQUEUE void SocketEventsKqueue(std::set &recv_set, std::set &send_set, std::set &error_set, bool fOnlyPoll); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 8416a1655c81f..f18a588d322e9 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -68,7 +68,7 @@ extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, CTxMemPool NodeContext& EnsureAnyNodeContext(const CoreContext& context) { - auto* node_context = GetContext(context); + auto* const node_context = GetContext(context); if (!node_context) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Node context not found"); } diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index d48debdb33feb..abcd1b7e919d4 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -301,8 +301,8 @@ class ConditionStack { uint32_t m_first_false_pos = NO_FALSE; public: - bool empty() { return m_stack_size == 0; } - bool all_true() { return m_first_false_pos == NO_FALSE; } + bool empty() const { return m_stack_size == 0; } + bool all_true() const { return m_first_false_pos == NO_FALSE; } void push_back(bool f) { if (m_first_false_pos == NO_FALSE && !f) { diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 063c560660d8f..89bf363c657dd 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -327,7 +327,7 @@ bool SignSignature(const SigningProvider &provider, const CScript& fromPubKey, C bool SignSignature(const SigningProvider &provider, const CTransaction& txFrom, CMutableTransaction& txTo, unsigned int nIn, int nHashType) { assert(nIn < txTo.vin.size()); - CTxIn& txin = txTo.vin[nIn]; + const CTxIn& txin = txTo.vin[nIn]; assert(txin.prevout.n < txFrom.vout.size()); const CTxOut& txout = txFrom.vout[txin.prevout.n]; diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index bce35b3d7b89e..bb34715832d89 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -24,7 +24,7 @@ static const unsigned int QUEUE_BATCH_SIZE = 128; static const int SCRIPT_CHECK_THREADS = 3; struct FakeCheck { - bool operator()() + bool operator()() const { return true; } @@ -45,7 +45,7 @@ struct FailingCheck { bool fails; FailingCheck(bool _fails) : fails(_fails){}; FailingCheck() : fails(true){}; - bool operator()() + bool operator()() const { return !fails; } @@ -74,7 +74,7 @@ struct UniqueCheck { struct MemoryCheck { static std::atomic fake_allocated_memory; bool b {false}; - bool operator()() + bool operator()() const { return true; } @@ -105,7 +105,7 @@ struct FrozenCleanupCheck { // Freezing can't be the default initialized behavior given how the queue // swaps in default initialized Checks. bool should_freeze {false}; - bool operator()() + bool operator()() const { return true; } diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 11a0defcffdac..96eca1f50e799 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -430,12 +430,12 @@ TestChainSetup::~TestChainSetup() SetMockTime(0); } -CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) +CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) const { return FromTx(MakeTransactionRef(tx)); } -CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) +CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const { return CTxMemPoolEntry(tx, nFee, nTime, nHeight, spendsCoinbase, sigOpCount, lp); diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 14b49ea2e8ca0..bf5ebb2b7354b 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -181,8 +181,8 @@ struct TestMemPoolEntryHelper nFee(0), nTime(0), nHeight(1), spendsCoinbase(false), sigOpCount(1) { } - CTxMemPoolEntry FromTx(const CMutableTransaction& tx); - CTxMemPoolEntry FromTx(const CTransactionRef& tx); + CTxMemPoolEntry FromTx(const CMutableTransaction& tx) const; + CTxMemPoolEntry FromTx(const CTransactionRef& tx) const; // Change the default value TestMemPoolEntryHelper &Fee(CAmount _fee) { nFee = _fee; return *this; } diff --git a/src/validation.cpp b/src/validation.cpp index be691495ed31f..54621f3c93bdf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -717,7 +717,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) m_view.SetBackend(m_viewmempool); assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(m_active_chainstate.CoinsTip())); - CCoinsViewCache& coins_cache = m_active_chainstate.CoinsTip(); + const CCoinsViewCache& coins_cache = m_active_chainstate.CoinsTip(); // do all inputs exist? for (const CTxIn& txin : tx.vin) { if (!coins_cache.HaveCoinInCache(txin.prevout)) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 74a4080191668..de5230bc068a8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -592,7 +592,7 @@ void CWallet::AddToSpends(const uint256& wtxid) { auto it = mapWallet.find(wtxid); assert(it != mapWallet.end()); - CWalletTx& thisTx = it->second; + const CWalletTx& thisTx = it->second; if (thisTx.IsCoinBase()) // Coinbases don't spend anything! return; @@ -1093,7 +1093,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) // Can't mark abandoned if confirmed or in mempool auto it = mapWallet.find(hashTx); assert(it != mapWallet.end()); - CWalletTx& origtx = it->second; + const CWalletTx& origtx = it->second; if (origtx.GetDepthInMainChain() != 0 || origtx.InMempool() || origtx.IsLockedByInstantSend()) { return false; } From 2bacbcf1fd0597234f99d33922572ccabb7f35c9 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 7 Jan 2021 21:56:42 +0100 Subject: [PATCH 5/5] Merge #14501: Fix possible data race when committing block files ef712298c3f8bc2afdad783f05080443b72b3f77 util: Check for file being NULL in DirectoryCommit (Luke Dashjr) 457490403853321d308c6ca6aaa90d6f8f29b4cf Fix possible data race when committing block files (Evan Klitzke) 220bb16cbee5b91d0bc0fcc6c71560d631295fa5 util: Introduce DirectoryCommit commit function to sync a directory (Evan Klitzke) ce5cbaea63ad4ea78e533bdb14f47f414061ae7f util.h: Document FileCommit function (Evan Klitzke) 844d650eea3bd809884cc5dd996a388bdc58314e util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit (Evan Klitzke) f6cec0bcaf560fa310853ad3fe17022602b63d5f util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif (Evan Klitzke) Pull request description: Reviving #12696 ACKs for top commit: laanwj: Code review ACK ef712298c3f8bc2afdad783f05080443b72b3f77 Tree-SHA512: 07d650990ef4c18d645dee3f9a199a940683ad17557d79d93979a76c4e710d8d70e6eae01d1a5991494a24a7654eb7db868be0c34a31e70b2509945d95bc9cce --- configure.ac | 2 +- src/flatfile.cpp | 1 + src/util/system.cpp | 27 ++++++++++++++++++--------- src/util/system.h | 12 ++++++++++++ 4 files changed, 32 insertions(+), 10 deletions(-) diff --git a/configure.ac b/configure.ac index cf8c21a7166dd..1ddd7f51f8c78 100644 --- a/configure.ac +++ b/configure.ac @@ -1164,13 +1164,13 @@ if test x$TARGET_OS != xwindows; then fi fi -dnl LevelDB platform checks AC_MSG_CHECKING(for fdatasync) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include ]], [[ fdatasync(0); ]])], [ AC_MSG_RESULT(yes); HAVE_FDATASYNC=1 ], [ AC_MSG_RESULT(no); HAVE_FDATASYNC=0 ] ) +AC_DEFINE_UNQUOTED([HAVE_FDATASYNC], [$HAVE_FDATASYNC], [Define to 1 if fdatasync is available.]) AC_MSG_CHECKING(for F_FULLFSYNC) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include ]], diff --git a/src/flatfile.cpp b/src/flatfile.cpp index 8a8f7b681c520..11cf357f3dd79 100644 --- a/src/flatfile.cpp +++ b/src/flatfile.cpp @@ -92,6 +92,7 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) fclose(file); return error("%s: failed to commit file %d", __func__, pos.nFile); } + DirectoryCommit(m_dir); fclose(file); return true; diff --git a/src/util/system.cpp b/src/util/system.cpp index 078e53c5937c6..d3e2841f96f24 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1089,27 +1089,36 @@ bool FileCommit(FILE *file) LogPrintf("%s: FlushFileBuffers failed: %d\n", __func__, GetLastError()); return false; } -#else - #if defined(HAVE_FDATASYNC) - if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync - LogPrintf("%s: fdatasync failed: %d\n", __func__, errno); - return false; - } - #elif defined(MAC_OSX) && defined(F_FULLFSYNC) +#elif defined(MAC_OSX) && defined(F_FULLFSYNC) if (fcntl(fileno(file), F_FULLFSYNC, 0) == -1) { // Manpage says "value other than -1" is returned on success LogPrintf("%s: fcntl F_FULLFSYNC failed: %d\n", __func__, errno); return false; } - #else +#elif HAVE_FDATASYNC + if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync + LogPrintf("%s: fdatasync failed: %d\n", __func__, errno); + return false; + } +#else if (fsync(fileno(file)) != 0 && errno != EINVAL) { LogPrintf("%s: fsync failed: %d\n", __func__, errno); return false; } - #endif #endif return true; } +void DirectoryCommit(const fs::path &dirname) +{ +#ifndef WIN32 + FILE* file = fsbridge::fopen(dirname, "r"); + if (file) { + fsync(fileno(file)); + fclose(file); + } +#endif +} + bool TruncateFile(FILE *file, unsigned int length) { #if defined(WIN32) return _chsize(_fileno(file), length) == 0; diff --git a/src/util/system.h b/src/util/system.h index 60d75aeb028ad..48035de2a6606 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -71,7 +71,19 @@ bool error(const char* fmt, const Args&... args) } void PrintExceptionContinue(const std::exception_ptr pex, const char* pszExceptionOrigin); + +/** + * Ensure file contents are fully committed to disk, using a platform-specific + * feature analogous to fsync(). + */ bool FileCommit(FILE *file); + +/** + * Sync directory contents. This is required on some environments to ensure that + * newly created files are committed to disk. + */ +void DirectoryCommit(const fs::path &dirname); + bool TruncateFile(FILE *file, unsigned int length); int RaiseFileDescriptorLimit(int nMinFD); void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);