From 9e5ee6ac529d69df732c00f380431993a44966e4 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 5 Nov 2020 18:45:03 +0100 Subject: [PATCH 1/6] Merge #20316: test: Fix wallet_multiwallet test issue on Windows fa00ff0399fe54f42d80daa04140d19bcfe0e2d8 test: Fix wallet_multiwallet test issue on Windows (MarcoFalke) Pull request description: Fixes: ``` Traceback (most recent call last): File "test\functional\test_framework\test_framework.py", line 126, in main self.run_test() File "test/functional/wallet_multiwallet.py", line 120, in run_test assert_equal(sorted(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), sorted(in_wallet_dir)) File "test\functional\test_framework\util.py", line 49, in assert_equal raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) AssertionError: not(['', 'sub\\w5', 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'] == ['', 'sub/w5', 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8']) ACKs for top commit: promag: ACK fa00ff0399fe54f42d80daa04140d19bcfe0e2d8. Tree-SHA512: 7a809a352677a216465cef59e866e4881272e302e897cebf7d9645bf87aebeaf54435bb0692bb5c1381c2dd680e8a34e640ea18ca6e2a4087e3233cd9c24ed04 --- test/functional/wallet_multiwallet.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index cb0dde6625..ca1deb0de2 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -100,14 +100,14 @@ class MultiWalletTest(BitcoinTestFramework): # w8 - to verify existing wallet file is loaded correctly. Not tested for SQLite wallets as this is a deprecated BDB behavior. # '' - to verify default wallet file is created correctly to_create = ['w1', 'w2', 'w3', 'w', 'sub/w5', 'w7_symlink'] - in_wallet_dir = to_create.copy() # Wallets in the wallet dir - in_wallet_dir.append('w7') # w7 is not loaded or created, but will be listed by listwalletdir because w7_symlink - to_create.append(os.path.join(self.options.tmpdir, 'extern/w6')) # External, not in the wallet dir, so we need to avoid adding it to in_wallet_dir + in_wallet_dir = [w.replace('/', os.path.sep) for w in to_create] # Wallets in the wallet dir + in_wallet_dir.append('w7') # w7 is not loaded or created, but will be listed by listwalletdir because w7_symlink + to_create.append(os.path.join(self.options.tmpdir, 'extern/w6')) # External, not in the wallet dir, so we need to avoid adding it to in_wallet_dir to_load = [self.default_wallet_name] if not self.options.is_sqlite_only: to_load.append('w8') - wallet_names = to_create + to_load # Wallet names loaded in the wallet - in_wallet_dir += to_load # The loaded wallets are also in the wallet dir + wallet_names = to_create + to_load # Wallet names loaded in the wallet + in_wallet_dir += to_load # The loaded wallets are also in the wallet dir self.start_node(0) for wallet_name in to_create: @@ -398,5 +398,6 @@ class MultiWalletTest(BitcoinTestFramework): self.nodes[0].unloadwallet(wallet) self.nodes[1].loadwallet(wallet) + if __name__ == '__main__': MultiWalletTest().main() From d2b9631a9056c8c01bd0ba2aa34ce6e11efdb624 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 20 Jul 2021 15:04:01 +0200 Subject: [PATCH 2/6] Merge bitcoin/bitcoin#22492: wallet: Reorder locks in dumpwallet to avoid lock order assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9b85a5e2f7e003ca8621feaac9bdd304d19081b4 tests: Test for dumpwallet lock order issue (Andrew Chow) 25d99e6511d8c43b2025a89bcd8295de755346a7 Reorder dumpwallet so that cs_main functions go first (Andrew Chow) Pull request description: When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue. Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`. Fixes #22489 ACKs for top commit: MarcoFalke: review ACK 9b85a5e2f7e003ca8621feaac9bdd304d19081b4 🎰 ryanofsky: Code review ACK 9b85a5e2f7e003ca8621feaac9bdd304d19081b4. Nice to reduce lock scope, and good test! prayank23: tACK https://github.com/bitcoin/bitcoin/pull/22492/commits/9b85a5e2f7e003ca8621feaac9bdd304d19081b4 lsilva01: Tested ACK https://github.com/bitcoin/bitcoin/pull/22492/commits/9b85a5e2f7e003ca8621feaac9bdd304d19081b4 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully. Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755 --- src/wallet/rpcdump.cpp | 13 ++++--- src/wallet/wallet.cpp | 66 ++++++++++++++++++---------------- test/functional/wallet_dump.py | 9 +++++ 3 files changed, 53 insertions(+), 35 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 1cf4e4bbe1..78330ebfba 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -917,7 +917,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore); + LOCK(wallet.cs_wallet); EnsureWalletIsUnlocked(&wallet); @@ -939,9 +939,16 @@ UniValue dumpwallet(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file"); std::map mapKeyBirth; - const std::map& mapKeyPool = spk_man.GetAllReserveKeys(); wallet.GetKeyBirthTimes(mapKeyBirth); + int64_t block_time = 0; + CHECK_NONFATAL(wallet.chain().findBlock(wallet.GetLastBlockHash(), FoundBlock().time(block_time))); + + // Note: To avoid a lock order issue, access to cs_main must be locked before cs_KeyStore. + // So we do the two things in this function that lock cs_main first: GetKeyBirthTimes, and findBlock. + LOCK(spk_man.cs_KeyStore); + + const std::map& mapKeyPool = spk_man.GetAllReserveKeys(); std::set scripts = spk_man.GetCScripts(); // sort time/key pairs @@ -956,8 +963,6 @@ UniValue dumpwallet(const JSONRPCRequest& request) file << strprintf("# Wallet dump created by Dash Core %s\n", CLIENT_BUILD); file << strprintf("# * Created on %s\n", FormatISO8601DateTime(GetTime())); file << strprintf("# * Best block at time of backup was %i (%s),\n", wallet.GetLastBlockHeight(), wallet.GetLastBlockHash().ToString()); - int64_t block_time = 0; - CHECK_NONFATAL(wallet.chain().findBlock(wallet.GetLastBlockHash(), FoundBlock().time(block_time))); file << strprintf("# mined on %s\n", FormatISO8601DateTime(block_time)); file << "\n"; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c67cb20248..ec79a82bd2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4321,44 +4321,48 @@ void CWallet::GetKeyBirthTimes(std::map& mapKeyBirth) const { AssertLockHeld(cs_wallet); mapKeyBirth.clear(); - LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); - assert(spk_man != nullptr); - LOCK(spk_man->cs_KeyStore); - - // get birth times for keys with metadata - for (const auto& entry : spk_man->mapKeyMetadata) { - if (entry.second.nCreateTime) { - mapKeyBirth[entry.first] = entry.second.nCreateTime; - } - } - // map in which we'll infer heights of other keys std::map mapKeyFirstBlock; CWalletTx::Confirmation max_confirm; max_confirm.block_height = GetLastBlockHeight() > 144 ? GetLastBlockHeight() - 144 : 0; // the tip can be reorganized; use a 144-block safety margin CHECK_NONFATAL(chain().findAncestorByHeight(GetLastBlockHash(), max_confirm.block_height, FoundBlock().hash(max_confirm.hashBlock))); - for (const CKeyID &keyid : spk_man->GetKeys()) { - if (mapKeyBirth.count(keyid) == 0) - mapKeyFirstBlock[keyid] = &max_confirm; - } - // if there are no such keys, we're done - if (mapKeyFirstBlock.empty()) - return; + { + LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); + assert(spk_man != nullptr); + LOCK(spk_man->cs_KeyStore); - // find first block that affects those keys, if there are any left - for (const auto& entry : mapWallet) { - // iterate over all wallet transactions... - const CWalletTx &wtx = entry.second; - if (wtx.m_confirm.status == CWalletTx::CONFIRMED) { - // ... which are already in a block - for (const CTxOut &txout : wtx.tx->vout) { - // iterate over all their outputs - for (const auto &keyid : GetAffectedKeys(txout.scriptPubKey, *spk_man)) { - // ... and all their affected keys - auto rit = mapKeyFirstBlock.find(keyid); - if (rit != mapKeyFirstBlock.end() && wtx.m_confirm.block_height < rit->second->block_height) { - rit->second = &wtx.m_confirm; + // get birth times for keys with metadata + for (const auto& entry : spk_man->mapKeyMetadata) { + if (entry.second.nCreateTime) { + mapKeyBirth[entry.first] = entry.second.nCreateTime; + } + } + + // Prepare to infer birth heights for keys without metadata + for (const CKeyID &keyid : spk_man->GetKeys()) { + if (mapKeyBirth.count(keyid) == 0) + mapKeyFirstBlock[keyid] = &max_confirm; + } + + // if there are no such keys, we're done + if (mapKeyFirstBlock.empty()) + return; + + // find first block that affects those keys, if there are any left + for (const auto& entry : mapWallet) { + // iterate over all wallet transactions... + const CWalletTx &wtx = entry.second; + if (wtx.m_confirm.status == CWalletTx::CONFIRMED) { + // ... which are already in a block + for (const CTxOut &txout : wtx.tx->vout) { + // iterate over all their outputs + for (const auto &keyid : GetAffectedKeys(txout.scriptPubKey, *spk_man)) { + // ... and all their affected keys + auto rit = mapKeyFirstBlock.find(keyid); + if (rit != mapKeyFirstBlock.end() && wtx.m_confirm.block_height < rit->second->block_height) { + rit->second = &wtx.m_confirm; + } } } } diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py index 2808d3561a..05f0773779 100755 --- a/test/functional/wallet_dump.py +++ b/test/functional/wallet_dump.py @@ -195,6 +195,15 @@ class WalletDumpTest(BitcoinTestFramework): with self.nodes[0].assert_debug_log(['Flushing wallet.dat'], timeout=20): self.nodes[0].getnewaddress() + # Make sure that dumpwallet doesn't have a lock order issue when there is an unconfirmed tx and it is reloaded + # See https://github.com/bitcoin/bitcoin/issues/22489 + self.nodes[0].createwallet("w3") + w3 = self.nodes[0].get_wallet_rpc("w3") + w3.importprivkey(privkey=self.nodes[0].get_deterministic_priv_key().key, label="coinbase_import") + w3.sendtoaddress(w3.getnewaddress(), 10) + w3.unloadwallet() + self.nodes[0].loadwallet("w3") + w3.dumpwallet(os.path.join(self.nodes[0].datadir, "w3.dump")) if __name__ == '__main__': WalletDumpTest().main() From b28f25774a0a8f84cf9a342126c1b821340f13ed Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Thu, 22 Jul 2021 14:53:43 +0200 Subject: [PATCH 3/6] Merge bitcoin/bitcoin#22481: mempool: apply rule of 5 to epochguard.h, fix compiler warnings 7b3a20b2602f902c344a615f23f8f0280b6f6bcc mempool: apply rule of 5 to epochguard.h, fix compiler warnings (Jon Atack) Pull request description: Apply the rule of five to `src/util/epochguard.h::{Epoch, Marker}` for safety, which also nicely fixes the `-Wdeprecated-copy` compiler warnings with Clang 13. References: - https://en.cppreference.com/w/cpp/language/rule_of_three - https://www.stroustrup.com/C++11FAQ.html#default - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-five
Compiler warnings fixed

```bash In file included from policy/rbf.cpp:5: In file included from ./policy/rbf.h:8: In file included from ./txmempool.h:24: ./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy] Marker& operator=(const Marker&) = delete; ^ ./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here class CTxMemPoolEntry ^ policy/rbf.cpp:29:29: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash()); ``` ```bash In file included from txmempool.cpp:6: In file included from ./txmempool.h:24: ./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy] Marker& operator=(const Marker&) = delete; ^ ./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here class CTxMemPoolEntry ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); } ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator>>>>>>>::construct' requested here __a.construct(__p, std::forward<_Args>(__args)...); ^ /usr/include/boost/multi_index_container.hpp:655:24: note: in instantiation of function template specialization 'std::allocator_traits>>>>>>>>::construct' requested here node_alloc_traits::construct( ^ /usr/include/boost/multi_index/detail/index_base.hpp:108:15: note: in instantiation of member function 'boost::multi_index::multi_index_container, boost::multi_index::hashed_unique, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>>>::construct_value' requested here final().construct_value(x,v); ^ /usr/include/boost/multi_index/detail/ord_index_impl.hpp:770:33: note: (skipping 5 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all) final_node_type* res=super::insert_(v,x,variant); ^ /usr/include/boost/multi_index_container.hpp:693:33: note: in instantiation of function template specialization 'boost::multi_index::detail::hashed_index, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by, boost::multi_index::hashed_unique, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>>, std::allocator>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert_' requested here final_node_type* res=super::insert_(v,x,variant); ^ /usr/include/boost/multi_index_container.hpp:705:12: note: in instantiation of function template specialization 'boost::multi_index::multi_index_container, boost::multi_index::hashed_unique, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>>>::insert_' requested here return insert_(v,detail::lvalue_tag()); ^ /usr/include/boost/multi_index/detail/index_base.hpp:213:21: note: in instantiation of member function 'boost::multi_index::multi_index_container, boost::multi_index::hashed_unique, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>>>::insert_' requested here {return final().insert_(x);} ^ /usr/include/boost/multi_index/hashed_index.hpp:284:46: note: in instantiation of member function 'boost::multi_index::detail::index_base, boost::multi_index::hashed_unique, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>>, std::allocator>::final_insert_' requested here std::pair p=this->final_insert_(x); ^ txmempool.cpp:363:53: note: in instantiation of member function 'boost::multi_index::detail::hashed_index, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by, boost::multi_index::hashed_unique, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>>, std::allocator>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert' requested here indexed_transaction_set::iterator newit = mapTx.insert(entry).first; ``` ```bash In file included from test/fuzz/policy_estimator.cpp:9: In file included from ./test/fuzz/util.h:27: In file included from ./txmempool.h:24: ./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy] Marker& operator=(const Marker&) = delete; ^ ./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here class CTxMemPoolEntry ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit move constructor for 'CTxMemPoolEntry' first required here { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); } ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator::construct' requested here __a.construct(__p, std::forward<_Args>(__args)...); ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/vector.tcc:115:21: note: in instantiation of function template specialization 'std::allocator_traits>::construct' requested here _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish, ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:1204:9: note: in instantiation of function template specialization 'std::vector::emplace_back' requested here { emplace_back(std::move(__x)); } ^ test/fuzz/policy_estimator.cpp:49:37: note: in instantiation of member function 'std::vector::push_back' requested here mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx)); ```

ACKs for top commit: laanwj: Code review ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc vasild: ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc Tree-SHA512: 0406dfcec180152d4f9ed07cbc2f406ad739b41f9c9cb38f8c75159c15d9d8a9a5c7526765966e40d695d265c178f6a80152e7edf82da344a65e55938dddb63d --- src/util/epochguard.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/epochguard.h b/src/util/epochguard.h index 1570ec4eb4..3e63e093da 100644 --- a/src/util/epochguard.h +++ b/src/util/epochguard.h @@ -40,6 +40,9 @@ public: Epoch() = default; Epoch(const Epoch&) = delete; Epoch& operator=(const Epoch&) = delete; + Epoch(Epoch&&) = delete; + Epoch& operator=(Epoch&&) = delete; + ~Epoch() = default; bool guarded() const { return m_guarded; } @@ -51,6 +54,13 @@ public: // only allow modification via Epoch member functions friend class Epoch; Marker& operator=(const Marker&) = delete; + + public: + Marker() = default; + Marker(const Marker&) = default; + Marker(Marker&&) = delete; + Marker& operator=(Marker&&) = delete; + ~Marker() = default; }; class SCOPED_LOCKABLE Guard From 90f04217ca8386545271d88fcff7fb86f1c46188 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 28 Jul 2021 10:30:58 +0800 Subject: [PATCH 4/6] Merge bitcoin/bitcoin#22557: fuzz: silence a compiler warning about unused CBanEntry comparator 787296eb6744b15ab693c053e4030ff68dfc95e0 fuzz: silence a compiler warning about unused CBanEntry comparator (Vasil Dimov) Pull request description: ``` test/fuzz/banman.cpp:35:13: warning: unused function 'operator==' [-Wunused-function] static bool operator==(const CBanEntry& lhs, const CBanEntry& rhs) ^ 1 warning generated. ``` See https://github.com/bitcoin/bitcoin/pull/22517#issuecomment-886177699 ACKs for top commit: MarcoFalke: cr ACK 787296eb6744b15ab693c053e4030ff68dfc95e0 practicalswift: cr ACK 787296eb6744b15ab693c053e4030ff68dfc95e0 hebasto: ACK 787296eb6744b15ab693c053e4030ff68dfc95e0 Tree-SHA512: 72e483cef249170160879cf4b69b787fb6c539d61dda423f618e2c5f130bee8c42897487751e5b58e7679cdb0153eb80efcb104e8a85095daa60d47e39ce78b8 --- src/test/fuzz/banman.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/banman.cpp b/src/test/fuzz/banman.cpp index af8d3dcc61..c77c0357a4 100644 --- a/src/test/fuzz/banman.cpp +++ b/src/test/fuzz/banman.cpp @@ -107,8 +107,9 @@ FUZZ_TARGET_INIT(banman, initialize_banman) BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ 0}; banmap_t banmap_read; ban_man_read.GetBanned(banmap_read); - // Temporarily disabled to allow the remainder of the fuzz test to run while a fix is being worked on: - // assert(banmap == banmap_read); + // Assert temporarily disabled to allow the remainder of the fuzz test to run while a + // fix is being worked on. See https://github.com/bitcoin/bitcoin/pull/22517 + (void)(banmap == banmap_read); } } fs::remove(banlist_file.string() + ".json"); From 83e374f0819155b09bde5d3e78a590775112fcb6 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 3 Aug 2021 20:06:11 +0800 Subject: [PATCH 5/6] Merge bitcoin/bitcoin#22609: [GetTransaction] remove unneeded cs_main lock acquire 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner) Pull request description: This PR is a follow-up to #22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/22383#discussion_r675069128)): https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.h#L554-L558 `CTxMemPool::get()` acquires this lock: https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.cpp#L822-L829 so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked. ACKs for top commit: tryphe: Concept ACK. tested 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f but not extensively. jnewbery: Code review ACK 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c --- src/node/transaction.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 42f4eafe06..7b9520a1a6 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -96,8 +96,6 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock) { - LOCK(cs_main); - if (mempool && !block_index) { CTransactionRef ptx = mempool->get(hash); if (ptx) return ptx; From e40d67a17045569c91263e2470e241ef6fe4875a Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 7 Aug 2021 16:28:45 +0200 Subject: [PATCH 6/6] Merge bitcoin/bitcoin#22657: fuzz: Re-enable assert in banman again fabed982ad9143cddaca8346f6b4c243dd84e0c4 fuzz: Re-enable assert in banman again (MarcoFalke) Pull request description: Looks like this was accidentally fixed by removing the buggy code in commit efd6f904c78769ad2e93c1f1de43014d284e7561 ACKs for top commit: hebasto: ACK fabed982ad9143cddaca8346f6b4c243dd84e0c4 Tree-SHA512: 2dea5dad48ff2050ae7086c1c6306c40f650e9629918e79adc54164a375d777a70b29f5a480566dc6430f07ce33dfe703fc5d45a20125584b4a026c5832198a2 --- src/test/fuzz/banman.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/test/fuzz/banman.cpp b/src/test/fuzz/banman.cpp index c77c0357a4..79cfe30c17 100644 --- a/src/test/fuzz/banman.cpp +++ b/src/test/fuzz/banman.cpp @@ -107,9 +107,7 @@ FUZZ_TARGET_INIT(banman, initialize_banman) BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ 0}; banmap_t banmap_read; ban_man_read.GetBanned(banmap_read); - // Assert temporarily disabled to allow the remainder of the fuzz test to run while a - // fix is being worked on. See https://github.com/bitcoin/bitcoin/pull/22517 - (void)(banmap == banmap_read); + assert(banmap == banmap_read); } } fs::remove(banlist_file.string() + ".json");