From bc366c8547e7ebd96c8c82c3c36a7d5200c589e3 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 29 Nov 2023 17:10:28 +0000 Subject: [PATCH 1/5] Merge bitcoin/bitcoin#28486: test, bench: Initialize and terminate use of Winsock properly fd4c6a10f2285f16c5d0215eb56a3060441f3ef2 test: Setup networking globally (Hennadii Stepanov) Pull request description: On the master branch, when compiling without external signer support, the `bench_bitcoin.exe` does not initialize Winsock DLL that is required, for example, here: https://github.com/bitcoin/bitcoin/blob/459272d639b9547f68000d2b9a5a0d991d477de5/src/bench/addrman.cpp#L124 Moreover, Windows docs explicitly [state](https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsacleanup) that `WSAStartup` and `WSACleanup` must be balanced: > There must be a call to `WSACleanup` for each successful call to `WSAStartup`. Only the final `WSACleanup` function call performs the actual cleanup. The preceding calls simply decrement an internal reference count in the WS2_32.DLL. That is not the case for our unit tests because the `SetupNetworking()` call is a part of the `BasicTestingSetup` fixture and is invoked multiple times, while `~CNetCleanup()` is invoked once only, at the end of the test binary execution. This PR fixes Winsock DLL initialization and termination. More docs: - https://learn.microsoft.com/en-us/windows/win32/winsock/initializing-winsock - https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsastartup - https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsacleanup Fix https://github.com/bitcoin/bitcoin/issues/28940. ACKs for top commit: maflcko: lgtm ACK fd4c6a10f2285f16c5d0215eb56a3060441f3ef2 Tree-SHA512: d360eaf776943f7f7a35ed5a5f9f3228d9e3d18eb824e5997cdc8eadddf466abe9f2da4910ee3bb86bf5411061e758259f7e1ec344f234ef7996f1bf8781dcda --- src/test/util/setup_common.cpp | 11 ++++++++++- src/test/util_tests.cpp | 3 +++ src/util/system.h | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 9847e4b2f913..1f2edd616f02 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -156,6 +157,15 @@ void DashChainstateSetupClose(NodeContext& node) Assert(node.mempool.get())); } +struct NetworkSetup +{ + NetworkSetup() + { + Assert(SetupNetworking()); + } +}; +static NetworkSetup g_networksetup_instance; + BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::vector& extra_args) : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()}, m_args{} @@ -201,7 +211,6 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve ECC_Start(); BLSInit(); SetupEnvironment(); - SetupNetworking(); InitSignatureCache(); InitScriptExecutionCache(); diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 2da64caf6576..ef5fd3e5b316 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1232,6 +1232,9 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory) // has released the lock as we would expect by probing it. int processstatus; BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1); + // The following line invokes the ~CNetCleanup dtor without + // a paired SetupNetworking call. This is acceptable as long as + // ~CNetCleanup is a no-op for non-Windows platforms. BOOST_CHECK_EQUAL(write(fd[1], &ExitCommand, 1), 1); BOOST_CHECK_EQUAL(waitpid(pid, &processstatus, 0), pid); BOOST_CHECK_EQUAL(processstatus, 0); diff --git a/src/util/system.h b/src/util/system.h index 08722d5f57bc..d3450cee5dd1 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -47,7 +47,7 @@ extern const char * const BITCOIN_CONF_FILENAME; extern const char * const BITCOIN_SETTINGS_FILENAME; void SetupEnvironment(); -bool SetupNetworking(); +[[nodiscard]] bool SetupNetworking(); template bool error(const char* fmt, const Args&... args) From 2343f0588dafcd4cf93b0f80d6b2e6473f85a605 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 29 Nov 2023 17:14:54 +0000 Subject: [PATCH 2/5] Merge bitcoin/bitcoin#28969: fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target fab164f342ae089b3a8ccd33e6e3fd6de6e2217e fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target (MarcoFalke) Pull request description: Should avoid ``` policy/feerate.cpp:29:63: runtime error: signed integer overflow: 77600710321911316 * 149 cannot be represented in type 'int64_t' (aka 'long') #0 0x563a1775ed66 in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63 #1 0x563a15913a69 in wallet::COutput::COutput(COutPoint const&, CTxOut const&, int, int, bool, bool, bool, long, bool, std::optional) src/./wallet/coinselection.h:91:57 #2 0x563a16fa6a6d in wallet::FetchSelectedInputs(wallet::CWallet const&, wallet::CCoinControl const&, wallet::CoinSelectionParams const&) src/wallet/spend.cpp:297:17 #3 0x563a16fc4512 in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1105:33 #4 0x563a16fbec74 in wallet::CreateTransaction(wallet::CWallet&, std::vector> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1291:16 #5 0x563a16fcf6df in wallet::FundTransaction(wallet::CWallet&, CMutableTransaction&, long&, int&, bilingual_str&, bool, std::set, std::allocator> const&, wallet::CCoinControl) src/wallet/spend.cpp:1361:16 #6 0x563a1597b7b9 in wallet::(anonymous namespace)::FuzzedWallet::FundTx(FuzzedDataProvider&, CMutableTransaction) src/wallet/test/fuzz/notifications.cpp:162:15 #7 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span)::$_0::operator()() const src/wallet/test/fuzz/notifications.cpp:228:23 #8 0x563a15958240 in unsigned long CallOneOf)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span)::$_1>(FuzzedDataProvider&, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span)::$_1) src/./test/fuzz/util.h:43:27 #9 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span) src/wallet/test/fuzz/notifications.cpp:196:9 #10 0x563a15fdef0c in std::function)>::operator()(Span) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #11 0x563a15fdef0c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #12 0x563a158032a4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19822a4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #13 0x563a15802999 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1981999) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #14 0x563a15804586 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983586) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #15 0x563a15804aa7 in fuzzer::Fuzzer::Loop(std::vector>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983aa7) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #16 0x563a157f21fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19711fb) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #17 0x563a1581c766 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199b766) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #18 0x7f499e17b0cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #19 0x7f499e17b188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #20 0x563a157e70c4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19660c4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in MS: 0 ; base unit: 0000000000000000000000000000000000000000 0x3f,0x0,0x2f,0x5f,0x5f,0x5f,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0xff,0xff,0xff,0xff,0xff,0x53,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x13,0x5e,0x5f,0x5f,0x8,0x25,0x0,0x5f,0x5f,0x5f,0x5f,0x5f,0x5f,0x8,0x25,0xca,0x7f,0x5f,0x5f,0x5f,0x13,0x13,0x5f,0x5f,0x5f,0x2,0xdb,0xca,0x0,0x0,0xe7,0xe6,0x66,0x65,0x0,0x0,0x0,0x0,0x44,0x3f,0xa,0xa,0xff,0xff,0xff,0xff,0xff,0x61,0x76,0x6f,0x69,0x0,0xb5,0x15, ?\000/___}}}}}}}}}}}}}}}}}}}}\377\377\377\377\377S\377\377\377\377\377\000\000\000\000\000\000\023^__\010%\000______\010%\312\177___\023\023___\002\333\312\000\000\347\346fe\000\000\000\000D?\012\012\377\377\377\377\377avoi\000\265\025 artifact_prefix='./'; Test unit written to ./crash-4d3bac8a64d4e58b2f0943e6d28e6e1f16328d7d Base64: PwAvX19ffX19fX19fX19fX19fX19fX19fX3//////1P//////wAAAAAAABNeX18IJQBfX19fX18IJcp/X19fExNfX18C28oAAOfmZmUAAAAARD8KCv//////YXZvaQC1FQ== ACKs for top commit: dergoegge: ACK fab164f342ae089b3a8ccd33e6e3fd6de6e2217e brunoerg: ACK fab164f342ae089b3a8ccd33e6e3fd6de6e2217e Tree-SHA512: f416828f4394aa7303ee437f141e9bbd23c0e0f1b830e4ef3932338858249ba68a811b9837c5b7ad8c6ab871b6354996434183597c1a910a8d8e8d829693e4b2 --- src/wallet/test/fuzz/notifications.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index daa9d5a8aa09..62c951c7a371 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2021 The Bitcoin Core developers +// Copyright (c) 2021-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -84,7 +84,7 @@ FUZZ_TARGET(wallet_notifications, .init = initialize_setup) // The total amount, to be distributed to the wallets a and b in txs // without fee. Thus, the balance of the wallets should always equal the // total amount. - const auto total_amount{ConsumeMoney(fuzzed_data_provider)}; + const auto total_amount{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY / 100000)}; FuzzedWallet a{"fuzzed_wallet_a"}; FuzzedWallet b{"fuzzed_wallet_b"}; From 45d0b29bc46c564aecd0bb7e9234a5e1c60ccf13 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 21 Jun 2023 16:34:12 +0100 Subject: [PATCH 3/5] Merge bitcoin/bitcoin#27921: fuzz: Avoid OOM in transaction fuzz target fa31c4daac5629d14360bbe9b2cd98db4c083989 fuzz: Avoid OOM in transaction fuzz target (MarcoFalke) Pull request description: To test: `FUZZ=transaction /usr/bin/time -f '%Us %MkB' ./src/test/fuzz/fuzz ../btc_qa_assets/fuzz_seed_corpus/transaction/9dc22b51df0af05ee5a595beefb0ce291feb6b99` Before: `0.72s 249636kB` After: `0.30s 92128kB` ACKs for top commit: dergoegge: utACK fa31c4daac5629d14360bbe9b2cd98db4c083989 Tree-SHA512: 958fc54e7af31af7db3e3e1fb37553ae24de251c7fdeea3d68ec168f03db48de6aa54a96bf971f9cc804e94ff8a02fda9c56d7e85869d62962f6f020568e3a7b --- src/test/fuzz/transaction.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp index 86131de22976..8a823d21896e 100644 --- a/src/test/fuzz/transaction.cpp +++ b/src/test/fuzz/transaction.cpp @@ -93,10 +93,14 @@ FUZZ_TARGET(transaction, .init = initialize_transaction) const CCoinsViewCache coins_view_cache(&coins_view); (void)AreInputsStandard(tx, coins_view_cache); - UniValue u(UniValue::VOBJ); - TxToUniv(tx, /*block_hash=*/uint256::ZERO, /*entry=*/u); - TxToUniv(tx, /*block_hash=*/uint256::ONE, /*entry=*/u); - -// TxToUniv(tx, /*block_hash=*/uint256::ZERO, /*include_addresses=*/true, u); -// TxToUniv(tx, /*block_hash=*/uint256::ONE, /*include_addresses=*/false, u); + if (tx.GetTotalSize() < 250'000) { // Avoid high memory usage (with msan) due to json encoding + { + UniValue u{UniValue::VOBJ}; + TxToUniv(tx, /*block_hash=*/uint256::ZERO, /*entry=*/u); + } + { + UniValue u{UniValue::VOBJ}; + TxToUniv(tx, /*block_hash=*/uint256::ONE, /*entry=*/u); + } + } } From e83db89c1f605eb7ab608a88caa10aae01b4be56 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 23 May 2023 15:40:30 -0400 Subject: [PATCH 4/5] Merge bitcoin/bitcoin#27177: test: fix intermittent issue in `feature_bip68_sequence` 272eb5561667482f8226bcf98eea00689dccefb8 test: fix `include_immature_coinbase` logic in `get_utxos` (brunoerg) a951c34f179dad0c7059b04a7f1e6b0804462168 test: fix `interface_usdt_mempool` by mining a block after each test (brunoerg) 1557bf1196bc2bdf00fd32f3b5d525796b4d194c test: fix mature utxos addition to wallet in `mempool_package_limits` (brunoerg) 60ced9007d518d542ce489b91076f9bbaf3312e3 test: fix intermittent issue in `feature_bip68_sequence` (brunoerg) Pull request description: Fixes #27129 To avoid `bad-txns-premature-spend-of-coinbase` error, when getting a utxo (using `get_utxo`) to create a new transaction `get_utxo` shouldn't return (if possible) by default immature coinbase. ACKs for top commit: achow101: ACK 272eb5561667482f8226bcf98eea00689dccefb8 pinheadmz: re-ACK 272eb5561667482f8226bcf98eea00689dccefb8 Tree-SHA512: eae821c7833bf084d8b907c94876ed010a7925d2177c3013a0c61b69d9571df006da83397a19487d93b0d1fa415951152f0b8ad0de2a55d86c39f6917934f050 --- test/functional/mempool_package_limits.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/functional/mempool_package_limits.py b/test/functional/mempool_package_limits.py index 9df8899895b8..330887ec22ac 100755 --- a/test/functional/mempool_package_limits.py +++ b/test/functional/mempool_package_limits.py @@ -46,8 +46,7 @@ def set_test_params(self): def run_test(self): self.wallet = MiniWallet(self.nodes[0]) # Add enough mature utxos to the wallet so that all txs spend confirmed coins. - self.generate(self.wallet, 35) - self.generate(self.nodes[0], COINBASE_MATURITY) + self.generate(self.wallet, COINBASE_MATURITY + 35) self.test_chain_limits() self.test_desc_count_limits() From 6378033fb15a9c546a73252b398e5bb560d78cc2 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 9 Oct 2023 14:36:12 -0400 Subject: [PATCH 5/5] Merge bitcoin/bitcoin#26331: Implement `CCoinsViewErrorCatcher::HaveCoin` and check disk space periodically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ed52e71176fc97c6ed01e3eebd85acdec54b4448 Periodically check disk space to avoid corruption (Aurèle Oulès) 7fe537f7a48675b1d25542bee6f390d665547580 Implement CCoinsViewErrorCatcher::HaveCoin (Aurèle Oulès) Pull request description: Attempt to fix #26112. As suggested by sipa in https://github.com/bitcoin/bitcoin/issues/26112#issuecomment-1249683401: > CCoinsViewErrorCatcher, the wrapper class used around CCoinsViewDB that's supposed to detect these problems and forcefully exit the application, has an override for GetCoins. But in CheckTxInputs, HaveInputs is first invoked, which on its turn calls HaveCoin. HaveCoin is implemented in CCoinsViewDB, but not in CCoinsViewErrorCatcher, and thus the disk read exception escapes. > A solution may be to just add an override for HaveCoin in CCoinsViewErrorCatcher. I implemented `CCoinsViewErrorCatcher::HaveCoin` and also added a periodic disk space check that shutdowns the node if there is not enough space left on disk, the minimum here is 50MB. For reviewers, it's possible to saturate disk space to test the PR by creating large files with `fallocate -l 50G test.bin` ACKs for top commit: achow101: ACK ed52e71176fc97c6ed01e3eebd85acdec54b4448 w0xlt: Code Review ACK https://github.com/bitcoin/bitcoin/pull/26331/commits/ed52e71176fc97c6ed01e3eebd85acdec54b4448 sipa: utACK ed52e71176fc97c6ed01e3eebd85acdec54b4448 Tree-SHA512: 456aa7b996023df42b4fbb5158ee429d9abf7374b7b1ec129b21aea1188ad19be8da4ae8e0edd90b85b7a3042b8e44e17d3742e33808a4234d5ddbe9bcef1b78 --- src/coins.cpp | 16 +++++++++++++--- src/coins.h | 1 + src/init.cpp | 9 +++++++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index d43e886b7603..b152fe07dec6 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -352,11 +352,13 @@ const Coin& AccessByTxid(const CCoinsViewCache& view, const uint256& txid) return coinEmpty; } -bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) const { +template +static bool ExecuteBackedWrapper(Func func, const std::vector>& err_callbacks) +{ try { - return CCoinsViewBacked::GetCoin(outpoint, coin); + return func(); } catch(const std::runtime_error& e) { - for (const auto& f : m_err_callbacks) { + for (const auto& f : err_callbacks) { f(); } LogPrintf("Error reading from database: %s\n", e.what()); @@ -367,3 +369,11 @@ bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) cons std::abort(); } } + +bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) const { + return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks); +} + +bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint &outpoint) const { + return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks); +} diff --git a/src/coins.h b/src/coins.h index dedfeb74d1dc..9f314f1a81af 100644 --- a/src/coins.h +++ b/src/coins.h @@ -382,6 +382,7 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked } bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; + bool HaveCoin(const COutPoint &outpoint) const override; private: /** A list of callbacks to execute upon leveldb read error. */ diff --git a/src/init.cpp b/src/init.cpp index db99f54a42a3..bba44889b086 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1549,6 +1549,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) RandAddPeriodic(); }, std::chrono::minutes{1}); + // Check disk space every 5 minutes to avoid db corruption. + node.scheduler->scheduleEvery([&args]{ + constexpr uint64_t min_disk_space = 50 << 20; // 50 MB + if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) { + LogPrintf("Shutting down due to lack of disk space!\n"); + StartShutdown(); + } + }, std::chrono::minutes{5}); + GetMainSignals().RegisterBackgroundSignalScheduler(*node.scheduler); // Create client interfaces for wallets that are supposed to be loaded