diff options
Diffstat (limited to 'src')
-rw-r--r-- | src/Makefile.test.include | 2 | ||||
-rw-r--r-- | src/bench/wallet_balance.cpp | 3 | ||||
-rw-r--r-- | src/interfaces/chain.cpp | 44 | ||||
-rw-r--r-- | src/interfaces/chain.h | 2 | ||||
-rw-r--r-- | src/interfaces/wallet.cpp | 5 | ||||
-rw-r--r-- | src/interfaces/wallet.h | 7 | ||||
-rw-r--r-- | src/qt/walletcontroller.cpp | 4 | ||||
-rw-r--r-- | src/qt/walletmodel.cpp | 17 | ||||
-rw-r--r-- | src/test/README.md | 29 | ||||
-rw-r--r-- | src/test/main.cpp | 18 | ||||
-rw-r--r-- | src/validationinterface.cpp | 18 | ||||
-rw-r--r-- | src/validationinterface.h | 12 | ||||
-rw-r--r-- | src/wallet/test/wallet_test_fixture.cpp | 3 | ||||
-rw-r--r-- | src/wallet/test/wallet_test_fixture.h | 1 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 16 | ||||
-rw-r--r-- | src/wallet/wallet.h | 5 |
16 files changed, 110 insertions, 76 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; } |