diff options
27 files changed, 186 insertions, 104 deletions
diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 059876bec8..93c5973a21 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -1011,7 +1011,7 @@ endif %.cpp.test: %.cpp @echo Running tests: `cat $< | grep -E "(BOOST_FIXTURE_TEST_SUITE\\(|BOOST_AUTO_TEST_SUITE\\()" | cut -d '(' -f 2 | cut -d ',' -f 1 | cut -d ')' -f 1` from $< - $(AM_V_at)$(TEST_BINARY) --catch_system_errors=no -l test_suite -t "`cat $< | grep -E "(BOOST_FIXTURE_TEST_SUITE\\(|BOOST_AUTO_TEST_SUITE\\()" | cut -d '(' -f 2 | cut -d ',' -f 1 | cut -d ')' -f 1`" > $<.log 2>&1 || (cat $<.log && false) + $(AM_V_at)$(TEST_BINARY) --catch_system_errors=no -l test_suite -t "`cat $< | grep -E "(BOOST_FIXTURE_TEST_SUITE\\(|BOOST_AUTO_TEST_SUITE\\()" | cut -d '(' -f 2 | cut -d ',' -f 1 | cut -d ')' -f 1`" -- DEBUG_LOG_OUT > $<.log 2>&1 || (cat $<.log && false) %.json.h: %.json @$(MKDIR_P) $(@D) diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index 62568a9da5..0381369218 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -23,9 +23,8 @@ static void WalletBalance(benchmark::State& state, const bool set_dirty, const b wallet.SetupLegacyScriptPubKeyMan(); bool first_run; if (wallet.LoadWallet(first_run) != DBErrors::LOAD_OK) assert(false); - wallet.handleNotifications(); } - + auto handler = chain->handleNotifications({ &wallet, [](CWallet*) {} }); const Optional<std::string> address_mine{add_mine ? Optional<std::string>{getnewaddress(wallet)} : nullopt}; if (add_watchonly) importaddress(wallet, ADDRESS_WATCHONLY); diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 775a89f4cf..0b3cd08e22 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -148,22 +148,12 @@ class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex> using UniqueLock::UniqueLock; }; -class NotificationsHandlerImpl : public Handler, CValidationInterface +class NotificationsProxy : public CValidationInterface { public: - explicit NotificationsHandlerImpl(Chain& chain, Chain::Notifications& notifications) - : m_chain(chain), m_notifications(¬ifications) - { - RegisterValidationInterface(this); - } - ~NotificationsHandlerImpl() override { disconnect(); } - void disconnect() override - { - if (m_notifications) { - m_notifications = nullptr; - UnregisterValidationInterface(this); - } - } + explicit NotificationsProxy(std::shared_ptr<Chain::Notifications> notifications) + : m_notifications(std::move(notifications)) {} + virtual ~NotificationsProxy() = default; void TransactionAddedToMempool(const CTransactionRef& tx) override { m_notifications->transactionAddedToMempool(tx); @@ -185,8 +175,26 @@ public: m_notifications->updatedBlockTip(); } void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->chainStateFlushed(locator); } - Chain& m_chain; - Chain::Notifications* m_notifications; + std::shared_ptr<Chain::Notifications> m_notifications; +}; + +class NotificationsHandlerImpl : public Handler +{ +public: + explicit NotificationsHandlerImpl(std::shared_ptr<Chain::Notifications> notifications) + : m_proxy(std::make_shared<NotificationsProxy>(std::move(notifications))) + { + RegisterSharedValidationInterface(m_proxy); + } + ~NotificationsHandlerImpl() override { disconnect(); } + void disconnect() override + { + if (m_proxy) { + UnregisterSharedValidationInterface(m_proxy); + m_proxy.reset(); + } + } + std::shared_ptr<NotificationsProxy> m_proxy; }; class RpcHandlerImpl : public Handler @@ -342,9 +350,9 @@ public: { ::uiInterface.ShowProgress(title, progress, resume_possible); } - std::unique_ptr<Handler> handleNotifications(Notifications& notifications) override + std::unique_ptr<Handler> handleNotifications(std::shared_ptr<Notifications> notifications) override { - return MakeUnique<NotificationsHandlerImpl>(*this, notifications); + return MakeUnique<NotificationsHandlerImpl>(std::move(notifications)); } void waitForNotificationsIfTipChanged(const uint256& old_tip) override { diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index caefa87e11..e1bc9bbbf3 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -229,7 +229,7 @@ public: }; //! Register handler for notifications. - virtual std::unique_ptr<Handler> handleNotifications(Notifications& notifications) = 0; + virtual std::unique_ptr<Handler> handleNotifications(std::shared_ptr<Notifications> notifications) = 0; //! Wait for pending notifications to be processed unless block hash points to the current //! chain tip. diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index bb6bb4923f..645a8709d2 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -368,16 +368,17 @@ public: } return result; } - bool tryGetBalances(WalletBalances& balances, int& num_blocks) override + bool tryGetBalances(WalletBalances& balances, int& num_blocks, bool force, int cached_num_blocks) override { auto locked_chain = m_wallet->chain().lock(true /* try_lock */); if (!locked_chain) return false; + num_blocks = locked_chain->getHeight().get_value_or(-1); + if (!force && num_blocks == cached_num_blocks) return false; TRY_LOCK(m_wallet->cs_wallet, locked_wallet); if (!locked_wallet) { return false; } balances = getBalances(); - num_blocks = locked_chain->getHeight().get_value_or(-1); return true; } CAmount getBalance() override { return m_wallet->GetBalance().m_mine_trusted; } diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 0e551b0a96..dbb0912230 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -201,8 +201,11 @@ public: //! Get balances. virtual WalletBalances getBalances() = 0; - //! Get balances if possible without blocking. - virtual bool tryGetBalances(WalletBalances& balances, int& num_blocks) = 0; + //! Get balances if possible without waiting for chain and wallet locks. + virtual bool tryGetBalances(WalletBalances& balances, + int& num_blocks, + bool force, + int cached_num_blocks) = 0; //! Get balance. virtual CAmount getBalance() = 0; diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 233c0ab6be..88c694567e 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -116,7 +116,7 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal const bool called = QMetaObject::invokeMethod(wallet_model, "startPollBalance"); assert(called); - connect(wallet_model, &WalletModel::unload, [this, wallet_model] { + connect(wallet_model, &WalletModel::unload, this, [this, wallet_model] { // Defer removeAndDeleteWallet when no modal widget is active. // TODO: remove this workaround by removing usage of QDiallog::exec. if (QApplication::activeModalWidget()) { @@ -128,7 +128,7 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal } else { removeAndDeleteWallet(wallet_model); } - }); + }, Qt::QueuedConnection); // Re-emit coinsSent signal from wallet model. connect(wallet_model, &WalletModel::coinsSent, this, &WalletController::coinsSent); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 41876f7119..608797d6ad 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -78,21 +78,18 @@ void WalletModel::pollBalanceChanged() // rescan. interfaces::WalletBalances new_balances; int numBlocks = -1; - if (!m_wallet->tryGetBalances(new_balances, numBlocks)) { + if (!m_wallet->tryGetBalances(new_balances, numBlocks, fForceCheckBalanceChanged, cachedNumBlocks)) { return; } - if(fForceCheckBalanceChanged || numBlocks != cachedNumBlocks) - { - fForceCheckBalanceChanged = false; + fForceCheckBalanceChanged = false; - // Balance and number of transactions might have changed - cachedNumBlocks = numBlocks; + // Balance and number of transactions might have changed + cachedNumBlocks = numBlocks; - checkBalanceChanged(new_balances); - if(transactionTableModel) - transactionTableModel->updateConfirmations(); - } + checkBalanceChanged(new_balances); + if(transactionTableModel) + transactionTableModel->updateConfirmations(); } void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances) diff --git a/src/test/README.md b/src/test/README.md index 731720f654..57cda26d7c 100644 --- a/src/test/README.md +++ b/src/test/README.md @@ -17,26 +17,31 @@ and tests weren't explicitly disabled. After configuring, they can be run with `make check`. -To run the bitcoind tests manually, launch `src/test/test_bitcoin`. To recompile +To run the unit tests manually, launch `src/test/test_bitcoin`. To recompile after a test file was modified, run `make` and then run the test again. If you modify a non-test file, use `make -C src/test` to recompile only what's needed -to run the bitcoind tests. +to run the unit tests. -To add more bitcoind tests, add `BOOST_AUTO_TEST_CASE` functions to the existing +To add more unit tests, add `BOOST_AUTO_TEST_CASE` functions to the existing .cpp files in the `test/` directory or add new .cpp files that implement new `BOOST_AUTO_TEST_SUITE` sections. -To run the bitcoin-qt tests manually, launch `src/qt/test/test_bitcoin-qt` +To run the GUI unit tests manually, launch `src/qt/test/test_bitcoin-qt` -To add more bitcoin-qt tests, add them to the `src/qt/test/` directory and +To add more GUI unit tests, add them to the `src/qt/test/` directory and the `src/qt/test/test_main.cpp` file. ### Running individual tests -test_bitcoin has some built-in command-line arguments; for -example, to run just the getarg_tests verbosely: +`test_bitcoin` has some built-in command-line arguments; for +example, to run just the `getarg_tests` verbosely: - test_bitcoin --log_level=all --run_test=getarg_tests + test_bitcoin --log_level=all --run_test=getarg_tests -- DEBUG_LOG_OUT + +`log_level` controls the verbosity of the test framework, which logs when a +test case is entered, for example. The `DEBUG_LOG_OUT` after the two dashes +redirects the debug log, which would normally go to a file in the test datadir +(`BasicTestingSetup::m_path_root`), to the standard terminal output. ... or to run just the doubledash test: @@ -56,11 +61,15 @@ see `uint256_tests.cpp`. ### Logging and debugging in unit tests +`make check` will write to a log file `foo_tests.cpp.log` and display this file +on failure. For running individual tests verbosely, refer to the section +[above](#running-individual-tests). + To write to logs from unit tests you need to use specific message methods provided by Boost. The simplest is `BOOST_TEST_MESSAGE`. -For debugging you can launch the test_bitcoin executable with `gdb`or `lldb` and -start debugging, just like you would with bitcoind: +For debugging you can launch the `test_bitcoin` executable with `gdb`or `lldb` and +start debugging, just like you would with any other program: ```bash gdb src/test/test_bitcoin diff --git a/src/test/main.cpp b/src/test/main.cpp index e6529949e2..f32243d1d3 100644 --- a/src/test/main.cpp +++ b/src/test/main.cpp @@ -11,12 +11,16 @@ #include <test/util/setup_common.h> -/** Redirect debug log to boost log */ +#include <iostream> + +/** Redirect debug log to unit_test.log files */ const std::function<void(const std::string&)> G_TEST_LOG_FUN = [](const std::string& s) { - if (s.back() == '\n') { - // boost will insert the new line - BOOST_TEST_MESSAGE(s.substr(0, s.size() - 1)); - } else { - BOOST_TEST_MESSAGE(s); - } + static const bool should_log{std::any_of( + &boost::unit_test::framework::master_test_suite().argv[1], + &boost::unit_test::framework::master_test_suite().argv[boost::unit_test::framework::master_test_suite().argc], + [](const char* arg) { + return std::string{"DEBUG_LOG_OUT"} == arg; + })}; + if (!should_log) return; + std::cout << s; }; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index c895904b19..f9f61e8a09 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -75,8 +75,10 @@ CMainSignals& GetMainSignals() return g_signals; } -void RegisterValidationInterface(CValidationInterface* pwalletIn) { - ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn]; +void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn) { + // Each connection captures pwalletIn to ensure that each callback is + // executed before pwalletIn is destroyed. For more details see #18338. + ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn.get()]; conns.UpdatedBlockTip = g_signals.m_internals->UpdatedBlockTip.connect(std::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); conns.TransactionAddedToMempool = g_signals.m_internals->TransactionAddedToMempool.connect(std::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, std::placeholders::_1)); conns.BlockConnected = g_signals.m_internals->BlockConnected.connect(std::bind(&CValidationInterface::BlockConnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); @@ -87,6 +89,18 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { conns.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, std::placeholders::_1, std::placeholders::_2)); } +void RegisterValidationInterface(CValidationInterface* callbacks) +{ + // Create a shared_ptr with a no-op deleter - CValidationInterface lifecycle + // is managed by the caller. + RegisterSharedValidationInterface({callbacks, [](CValidationInterface*){}}); +} + +void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> callbacks) +{ + UnregisterValidationInterface(callbacks.get()); +} + void UnregisterValidationInterface(CValidationInterface* pwalletIn) { if (g_signals.m_internals) { g_signals.m_internals->m_connMainSignals.erase(pwalletIn); diff --git a/src/validationinterface.h b/src/validationinterface.h index 5707422635..f9a359b7ad 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -30,6 +30,14 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn); void UnregisterValidationInterface(CValidationInterface* pwalletIn); /** Unregister all wallets from core */ void UnregisterAllValidationInterfaces(); + +// Alternate registration functions that release a shared_ptr after the last +// notification is sent. These are useful for race-free cleanup, since +// unregistration is nonblocking and can return before the last notification is +// processed. +void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> callbacks); +void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> callbacks); + /** * Pushes a function to callback onto the notification queue, guaranteeing any * callbacks generated prior to now are finished when the function is called. @@ -163,7 +171,7 @@ protected: * Notifies listeners that a block which builds directly on our current tip * has been received and connected to the headers tree, though not validated yet */ virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) {}; - friend void ::RegisterValidationInterface(CValidationInterface*); + friend void ::RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); }; @@ -173,7 +181,7 @@ class CMainSignals { private: std::unique_ptr<MainSignalsInstance> m_internals; - friend void ::RegisterValidationInterface(CValidationInterface*); + friend void ::RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); friend void ::CallFunctionInValidationInterfaceQueue(std::function<void ()> func); diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index ba0843f352..b9e714946d 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -10,7 +10,6 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName) { bool fFirstRun; m_wallet.LoadWallet(fFirstRun); - m_wallet.handleNotifications(); - + m_chain_notifications_handler = m_chain->handleNotifications({ &m_wallet, [](CWallet*) {} }); m_chain_client->registerRpcs(); } diff --git a/src/wallet/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h index 4e4129fb2c..81d8a60b8a 100644 --- a/src/wallet/test/wallet_test_fixture.h +++ b/src/wallet/test/wallet_test_fixture.h @@ -23,6 +23,7 @@ struct WalletTestingSetup: public TestingSetup { std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain(m_node); std::unique_ptr<interfaces::ChainClient> m_chain_client = interfaces::MakeWalletClient(*m_chain, {}); CWallet m_wallet; + std::unique_ptr<interfaces::Handler> m_chain_notifications_handler; }; #endif // BITCOIN_WALLET_TEST_WALLET_TEST_FIXTURE_H diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9a972febab..98f308f927 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -62,8 +62,10 @@ bool AddWallet(const std::shared_ptr<CWallet>& wallet) bool RemoveWallet(const std::shared_ptr<CWallet>& wallet) { - LOCK(cs_wallets); assert(wallet); + // Unregister with the validation interface which also drops shared ponters. + wallet->m_chain_notifications_handler.reset(); + LOCK(cs_wallets); std::vector<std::shared_ptr<CWallet>>::iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet); if (i == vpwallets.end()) return false; vpwallets.erase(i); @@ -105,13 +107,9 @@ static std::set<std::string> g_unloading_wallet_set; // Custom deleter for shared_ptr<CWallet>. static void ReleaseWallet(CWallet* wallet) { - // Unregister and delete the wallet right after BlockUntilSyncedToCurrentChain - // so that it's in sync with the current chainstate. const std::string name = wallet->GetName(); wallet->WalletLogPrintf("Releasing wallet\n"); - wallet->BlockUntilSyncedToCurrentChain(); wallet->Flush(); - wallet->m_chain_notifications_handler.reset(); delete wallet; // Wallet is now released, notify UnloadWallet, if any. { @@ -137,6 +135,7 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet) // Notify the unload intent so that all remaining shared pointers are // released. wallet->NotifyUnload(); + // Time to ditch our shared_ptr and wait for ReleaseWallet call. wallet.reset(); { @@ -4092,7 +4091,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, } // Register with the validation interface. It's ok to do this after rescan since we're still holding locked_chain. - walletInstance->handleNotifications(); + walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance); walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); @@ -4105,11 +4104,6 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, return walletInstance; } -void CWallet::handleNotifications() -{ - m_chain_notifications_handler = m_chain->handleNotifications(*this); -} - void CWallet::postInitProcess() { auto locked_chain = chain().lock(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9a83c84fc4..fe59773488 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -606,7 +606,7 @@ class WalletRescanReserver; //forward declarations for ScanForWalletTransactions /** * A CWallet maintains a set of transactions and balances, and provides the ability to create new transactions. */ -class CWallet final : public WalletStorage, private interfaces::Chain::Notifications +class CWallet final : public WalletStorage, public interfaces::Chain::Notifications { private: CKeyingMaterial vMasterKey GUARDED_BY(cs_wallet); @@ -782,9 +782,6 @@ public: /** Registered interfaces::Chain::Notifications handler. */ std::unique_ptr<interfaces::Handler> m_chain_notifications_handler; - /** Register the wallet for chain notifications */ - void handleNotifications(); - /** Interface for accessing chain state. */ interfaces::Chain& chain() const { assert(m_chain); return *m_chain; } diff --git a/test/functional/p2p_dos_header_tree.py b/test/functional/p2p_dos_header_tree.py index 6676b84e54..28dd809ca5 100755 --- a/test/functional/p2p_dos_header_tree.py +++ b/test/functional/p2p_dos_header_tree.py @@ -47,8 +47,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework): self.log.info("Feed all non-fork headers, including and up to the first checkpoint") self.nodes[0].add_p2p_connection(P2PInterface()) - self.nodes[0].p2p.send_message(msg_headers(self.headers)) - self.nodes[0].p2p.sync_with_ping() + self.nodes[0].p2p.send_and_ping(msg_headers(self.headers)) assert { 'height': 546, 'hash': '000000002a936ca763904c3c35fce2f3556c559c0214345d31b1bcebf76acb70', @@ -65,8 +64,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework): # On node 0 it succeeds because checkpoints are disabled self.restart_node(0, extra_args=['-nocheckpoints']) self.nodes[0].add_p2p_connection(P2PInterface()) - self.nodes[0].p2p.send_message(msg_headers(self.headers_fork)) - self.nodes[0].p2p.sync_with_ping() + self.nodes[0].p2p.send_and_ping(msg_headers(self.headers_fork)) assert { "height": 2, "hash": "00000000b0494bd6c3d5ff79c497cfce40831871cbf39b1bc28bd1dac817dc39", @@ -76,8 +74,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework): # On node 1 it succeeds because no checkpoint has been reached yet by a chain tip self.nodes[1].add_p2p_connection(P2PInterface()) - self.nodes[1].p2p.send_message(msg_headers(self.headers_fork)) - self.nodes[1].p2p.sync_with_ping() + self.nodes[1].p2p.send_and_ping(msg_headers(self.headers_fork)) assert { "height": 2, "hash": "00000000b0494bd6c3d5ff79c497cfce40831871cbf39b1bc28bd1dac817dc39", diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py index a22ee91483..ad7a9dcf6e 100755 --- a/test/functional/p2p_filter.py +++ b/test/functional/p2p_filter.py @@ -11,6 +11,7 @@ from test_framework.messages import ( MSG_FILTERED_BLOCK, msg_getdata, msg_filterload, + msg_filterclear, ) from test_framework.mininode import ( P2PInterface, @@ -97,6 +98,13 @@ class FilterTest(BitcoinTestFramework): filter_node.wait_for_tx(txid) assert not filter_node.merkleblock_received + self.log.info('Check that after deleting filter all txs get relayed again') + filter_node.send_message(msg_filterclear()) + filter_node.sync_with_ping() + for _ in range(5): + txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7) + filter_node.wait_for_tx(txid) + if __name__ == '__main__': FilterTest().main() diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 9876d749ff..759f111e69 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -140,7 +140,6 @@ class InvalidMessagesTest(BitcoinTestFramework): # Node is still up. conn = node.add_p2p_connection(P2PDataStore()) - conn.sync_with_ping() def test_magic_bytes(self): conn = self.nodes[0].add_p2p_connection(P2PDataStore()) diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py index 06049db54c..2751ae6a5b 100755 --- a/test/functional/p2p_leak.py +++ b/test/functional/p2p_leak.py @@ -19,6 +19,7 @@ from test_framework.util import wait_until banscore = 10 + class CLazyNode(P2PInterface): def __init__(self): super().__init__() @@ -88,6 +89,7 @@ class CNodeNoVerackIdle(CLazyNode): self.send_message(msg_ping()) self.send_message(msg_getaddr()) + class P2PLeakTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 @@ -96,7 +98,11 @@ class P2PLeakTest(BitcoinTestFramework): def run_test(self): no_version_bannode = self.nodes[0].add_p2p_connection(CNodeNoVersionBan(), send_version=False, wait_for_verack=False) no_version_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVersionIdle(), send_version=False, wait_for_verack=False) - no_verack_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVerackIdle()) + no_verack_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVerackIdle(), wait_for_verack=False) + + # Wait until we got the verack in response to the version. Though, don't wait for the other node to receive the + # verack, since we never sent one + no_verack_idlenode.wait_for_verack() wait_until(lambda: no_version_bannode.ever_connected, timeout=10, lock=mininode_lock) wait_until(lambda: no_version_idlenode.ever_connected, timeout=10, lock=mininode_lock) diff --git a/test/functional/p2p_timeouts.py b/test/functional/p2p_timeouts.py index 02ceec3dc1..c1235f8a6b 100755 --- a/test/functional/p2p_timeouts.py +++ b/test/functional/p2p_timeouts.py @@ -27,11 +27,13 @@ from test_framework.messages import msg_ping from test_framework.mininode import P2PInterface from test_framework.test_framework import BitcoinTestFramework + class TestP2PConn(P2PInterface): def on_version(self, message): # Don't send a verack in response pass + class TimeoutsTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True @@ -41,10 +43,14 @@ class TimeoutsTest(BitcoinTestFramework): def run_test(self): # Setup the p2p connections - no_verack_node = self.nodes[0].add_p2p_connection(TestP2PConn()) + no_verack_node = self.nodes[0].add_p2p_connection(TestP2PConn(), wait_for_verack=False) no_version_node = self.nodes[0].add_p2p_connection(TestP2PConn(), send_version=False, wait_for_verack=False) no_send_node = self.nodes[0].add_p2p_connection(TestP2PConn(), send_version=False, wait_for_verack=False) + # Wait until we got the verack in response to the version. Though, don't wait for the other node to receive the + # verack, since we never sent one + no_verack_node.wait_for_verack() + sleep(1) assert no_verack_node.is_connected @@ -81,5 +87,6 @@ class TimeoutsTest(BitcoinTestFramework): assert not no_version_node.is_connected assert not no_send_node.is_connected + if __name__ == '__main__': TimeoutsTest().main() diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 9e87ad62a1..ff0c763b72 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -1356,6 +1356,23 @@ class msg_filterload: self.data, self.nHashFuncs, self.nTweak, self.nFlags) +class msg_filterclear: + __slots__ = () + command = b"filterclear" + + def __init__(self): + pass + + def deserialize(self, f): + pass + + def serialize(self): + return b"" + + def __repr__(self): + return "msg_filterclear()" + + class msg_feefilter: __slots__ = ("feerate",) command = b"feefilter" diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py index b760e7e1f3..ce51513ce9 100755 --- a/test/functional/test_framework/mininode.py +++ b/test/functional/test_framework/mininode.py @@ -30,6 +30,7 @@ from test_framework.messages import ( msg_blocktxn, msg_cmpctblock, msg_feefilter, + msg_filterclear, msg_filterload, msg_getaddr, msg_getblocks, @@ -64,6 +65,7 @@ MESSAGEMAP = { b"blocktxn": msg_blocktxn, b"cmpctblock": msg_cmpctblock, b"feefilter": msg_feefilter, + b"filterclear": msg_filterclear, b"filterload": msg_filterload, b"getaddr": msg_getaddr, b"getblocks": msg_getblocks, @@ -322,6 +324,7 @@ class P2PInterface(P2PConnection): def on_blocktxn(self, message): pass def on_cmpctblock(self, message): pass def on_feefilter(self, message): pass + def on_filterclear(self, message): pass def on_filterload(self, message): pass def on_getaddr(self, message): pass def on_getblocks(self, message): pass diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index c7559ac7c8..8260c917fe 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -468,7 +468,19 @@ class TestNode(): p2p_conn.peer_connect(**kwargs, net=self.chain)() self.p2ps.append(p2p_conn) if wait_for_verack: + # Wait for the node to send us the version and verack p2p_conn.wait_for_verack() + # At this point we have sent our version message and received the version and verack, however the full node + # has not yet received the verack from us (in reply to their version). So, the connection is not yet fully + # established (fSuccessfullyConnected). + # + # This shouldn't lead to any issues when sending messages, since the verack will be in-flight before the + # message we send. However, it might lead to races where we are expecting to receive a message. E.g. a + # transaction that will be added to the mempool as soon as we return here. + # + # So syncing here is redundant when we only want to send a message, but the cost is low (a few milliseconds) + # in comparision to the upside of making tests less fragile and unexpected intermittent errors less likely. + p2p_conn.sync_with_ping() return p2p_conn @@ -487,6 +499,7 @@ class TestNode(): p.peer_disconnect() del self.p2ps[:] + class TestNodeCLIAttr: def __init__(self, cli, command): self.cli = cli @@ -498,6 +511,7 @@ class TestNodeCLIAttr: def get_request(self, *args, **kwargs): return lambda: self(*args, **kwargs) + def arg_to_cli(arg): if isinstance(arg, bool): return str(arg).lower() @@ -506,6 +520,7 @@ def arg_to_cli(arg): else: return str(arg) + class TestNodeCLI(): """Interface to bitcoin-cli for an individual node""" diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 5bb73aee7e..e89b4e9879 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -393,6 +393,7 @@ def connect_nodes(from_connection, node_num): # with transaction relaying wait_until(lambda: all(peer['version'] != 0 for peer in from_connection.getpeerinfo())) + def sync_blocks(rpc_connections, *, wait=1, timeout=60): """ Wait until everybody has the same tip. @@ -406,9 +407,12 @@ def sync_blocks(rpc_connections, *, wait=1, timeout=60): best_hash = [x.getbestblockhash() for x in rpc_connections] if best_hash.count(best_hash[0]) == len(rpc_connections): return + # Check that each peer has at least one connection + assert (all([len(x.getpeerinfo()) for x in rpc_connections])) time.sleep(wait) raise AssertionError("Block sync timed out:{}".format("".join("\n {!r}".format(b) for b in best_hash))) + def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True): """ Wait until everybody has the same transactions in their memory @@ -422,6 +426,8 @@ def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True): for r in rpc_connections: r.syncwithvalidationinterfacequeue() return + # Check that each peer has at least one connection + assert (all([len(x.getpeerinfo()) for x in rpc_connections])) time.sleep(wait) raise AssertionError("Mempool sync timed out:{}".format("".join("\n {!r}".format(m) for m in pool))) diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py index 229eda9806..6d51ca6c93 100755 --- a/test/functional/wallet_listsinceblock.py +++ b/test/functional/wallet_listsinceblock.py @@ -111,23 +111,21 @@ class ListSinceBlockTest(BitcoinTestFramework): senttx = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1) # generate on both sides - lastblockhash = self.nodes[1].generate(6)[5] - self.nodes[2].generate(7) - self.log.debug('lastblockhash={}'.format(lastblockhash)) + nodes1_last_blockhash = self.nodes[1].generate(6)[-1] + nodes2_first_blockhash = self.nodes[2].generate(7)[0] + self.log.debug("nodes[1] last blockhash = {}".format(nodes1_last_blockhash)) + self.log.debug("nodes[2] first blockhash = {}".format(nodes2_first_blockhash)) self.sync_all(self.nodes[:2]) self.sync_all(self.nodes[2:]) self.join_network() - # listsinceblock(lastblockhash) should now include tx, as seen from nodes[0] - lsbres = self.nodes[0].listsinceblock(lastblockhash) - found = False - for tx in lsbres['transactions']: - if tx['txid'] == senttx: - found = True - break - assert found + # listsinceblock(nodes1_last_blockhash) should now include tx as seen from nodes[0] + # and return the block height which listsinceblock now exposes since a5e7795. + transactions = self.nodes[0].listsinceblock(nodes1_last_blockhash)['transactions'] + found = next(tx for tx in transactions if tx['txid'] == senttx) + assert_equal(found['blockheight'], self.nodes[0].getblockheader(nodes2_first_blockhash)['height']) def test_double_spend(self): ''' diff --git a/test/sanitizer_suppressions/tsan b/test/sanitizer_suppressions/tsan index b9c5c038d0..70eea34363 100644 --- a/test/sanitizer_suppressions/tsan +++ b/test/sanitizer_suppressions/tsan @@ -7,14 +7,6 @@ deadlock:WalletBatch # Intentional deadlock in tests deadlock:TestPotentialDeadLockDetected -# Race due to unprotected calls to thread-unsafe BOOST_TEST_MESSAGE from different threads: -# * G_TEST_LOG_FUN in the index thread -# * boost test case invoker (entering a test case) in the main thread -# TODO: get rid of BOOST_ macros, see also https://github.com/bitcoin/bitcoin/issues/8670 -race:blockfilter_index_initial_sync_invoker -race:txindex_initial_sync_invoker -race:validation_block_tests::TestSubscriber - # Wildcard for all gui tests, should be replaced with non-wildcard suppressions race:src/qt/test/* deadlock:src/qt/test/* |