From ff6a834fc32b25376e221fefa8c2a2565b4167cc Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 26 Apr 2017 20:13:24 -0400 Subject: [PATCH 1/7] Use TestingSetup to DRY qt rpcnestedtests --- src/qt/test/rpcnestedtests.cpp | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/src/qt/test/rpcnestedtests.cpp b/src/qt/test/rpcnestedtests.cpp index 26dec3c61..fbad9e544 100644 --- a/src/qt/test/rpcnestedtests.cpp +++ b/src/qt/test/rpcnestedtests.cpp @@ -12,6 +12,7 @@ #include "rpc/server.h" #include "rpcconsole.h" #include "test/testutil.h" +#include "test/test_bitcoin.h" #include "univalue.h" #include "util.h" @@ -35,8 +36,6 @@ void RPCNestedTests::rpcNestedTests() { // do some test setup // could be moved to a more generic place when we add more tests on QT level - const CChainParams& chainparams = Params(); - RegisterAllCoreRPCCommands(tableRPC); tableRPC.appendCommand("rpcNestedTest", &vRPCCommands[0]); ClearDatadirCache(); std::string path = QDir::tempPath().toStdString() + "/" + strprintf("test_bitcoin_qt_%lu_%i", (unsigned long)GetTime(), (int)(GetRand(100000))); @@ -44,15 +43,8 @@ void RPCNestedTests::rpcNestedTests() dir.mkpath("."); ForceSetArg("-datadir", path); //mempool.setSanityCheck(1.0); - pblocktree = new CBlockTreeDB(1 << 20, true); - pcoinsdbview = new CCoinsViewDB(1 << 23, true); - pcoinsTip = new CCoinsViewCache(pcoinsdbview); - InitBlockIndex(chainparams); - { - CValidationState state; - bool ok = ActivateBestChain(state, chainparams); - QVERIFY(ok); - } + + TestingSetup test; SetRPCWarmupFinished(); @@ -145,13 +137,5 @@ void RPCNestedTests::rpcNestedTests() QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(result, "rpcNestedTest(abc,,)"), std::runtime_error); //don't tollerate empty arguments when using , #endif - UnloadBlockIndex(); - delete pcoinsTip; - pcoinsTip = nullptr; - delete pcoinsdbview; - pcoinsdbview = nullptr; - delete pblocktree; - pblocktree = nullptr; - fs::remove_all(fs::path(path)); } From 3a19fed9db558a5f666d965b6f602f7faf74ab73 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Jan 2017 16:17:14 -0500 Subject: [PATCH 2/7] Make ValidationInterface signals-type-agnostic (by hiding boost::signals stuff in the .cpp) This allows us to give it a bit more intelligence as we move forward, including routing some signals through CScheduler. While the introduction of a "internals" pointer in the class is pretty ugly, the fact that we no longer need to include boost/signals directly from validationinterface.h is very much worth the loss. --- src/validationinterface.cpp | 108 +++++++++++++++++++++++++++--------- src/validationinterface.h | 62 ++++++++++++--------- 2 files changed, 117 insertions(+), 53 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index be2f20b86..7d0ab756e 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -5,45 +5,99 @@ #include "validationinterface.h" +#include + +struct MainSignalsInstance { + boost::signals2::signal UpdatedBlockTip; + boost::signals2::signal TransactionAddedToMempool; + boost::signals2::signal &, const CBlockIndex *pindex, const std::vector&)> BlockConnected; + boost::signals2::signal &)> BlockDisconnected; + boost::signals2::signal SetBestChain; + boost::signals2::signal Inventory; + boost::signals2::signal Broadcast; + boost::signals2::signal BlockChecked; + boost::signals2::signal&)> NewPoWValidBlock; +}; + static CMainSignals g_signals; +CMainSignals::CMainSignals() { + m_internals.reset(new MainSignalsInstance()); +} + CMainSignals& GetMainSignals() { return g_signals; } void RegisterValidationInterface(CValidationInterface* pwalletIn) { - g_signals.UpdatedBlockTip.connect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); - g_signals.TransactionAddedToMempool.connect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1)); - g_signals.BlockConnected.connect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3)); - g_signals.BlockDisconnected.connect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1)); - g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); - g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); - g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2)); - g_signals.BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); - g_signals.NewPoWValidBlock.connect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); + g_signals.m_internals->UpdatedBlockTip.connect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); + g_signals.m_internals->TransactionAddedToMempool.connect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1)); + g_signals.m_internals->BlockConnected.connect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3)); + g_signals.m_internals->BlockDisconnected.connect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1)); + g_signals.m_internals->SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); + g_signals.m_internals->Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); + g_signals.m_internals->Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2)); + g_signals.m_internals->BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); + g_signals.m_internals->NewPoWValidBlock.connect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); } void UnregisterValidationInterface(CValidationInterface* pwalletIn) { - g_signals.BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); - g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2)); - g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); - g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); - g_signals.TransactionAddedToMempool.disconnect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1)); - g_signals.BlockConnected.disconnect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3)); - g_signals.BlockDisconnected.disconnect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1)); - g_signals.UpdatedBlockTip.disconnect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); - g_signals.NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); + g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); + g_signals.m_internals->Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2)); + g_signals.m_internals->Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); + g_signals.m_internals->SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); + g_signals.m_internals->TransactionAddedToMempool.disconnect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1)); + g_signals.m_internals->BlockConnected.disconnect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3)); + g_signals.m_internals->BlockDisconnected.disconnect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1)); + g_signals.m_internals->UpdatedBlockTip.disconnect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); + g_signals.m_internals->NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); } void UnregisterAllValidationInterfaces() { - g_signals.BlockChecked.disconnect_all_slots(); - g_signals.Broadcast.disconnect_all_slots(); - g_signals.Inventory.disconnect_all_slots(); - g_signals.SetBestChain.disconnect_all_slots(); - g_signals.TransactionAddedToMempool.disconnect_all_slots(); - g_signals.BlockConnected.disconnect_all_slots(); - g_signals.BlockDisconnected.disconnect_all_slots(); - g_signals.UpdatedBlockTip.disconnect_all_slots(); - g_signals.NewPoWValidBlock.disconnect_all_slots(); + g_signals.m_internals->BlockChecked.disconnect_all_slots(); + g_signals.m_internals->Broadcast.disconnect_all_slots(); + g_signals.m_internals->Inventory.disconnect_all_slots(); + g_signals.m_internals->SetBestChain.disconnect_all_slots(); + g_signals.m_internals->TransactionAddedToMempool.disconnect_all_slots(); + g_signals.m_internals->BlockConnected.disconnect_all_slots(); + g_signals.m_internals->BlockDisconnected.disconnect_all_slots(); + g_signals.m_internals->UpdatedBlockTip.disconnect_all_slots(); + g_signals.m_internals->NewPoWValidBlock.disconnect_all_slots(); +} + +void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { + m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); +} + +void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) { + m_internals->TransactionAddedToMempool(ptx); +} + +void CMainSignals::BlockConnected(const std::shared_ptr &pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { + m_internals->BlockConnected(pblock, pindex, vtxConflicted); +} + +void CMainSignals::BlockDisconnected(const std::shared_ptr &pblock) { + m_internals->BlockDisconnected(pblock); +} + +void CMainSignals::SetBestChain(const CBlockLocator &locator) { + m_internals->SetBestChain(locator); +} + +void CMainSignals::Inventory(const uint256 &hash) { + m_internals->Inventory(hash); +} + +void CMainSignals::Broadcast(int64_t nBestBlockTime, CConnman* connman) { + m_internals->Broadcast(nBestBlockTime, connman); +} + +void CMainSignals::BlockChecked(const CBlock& block, const CValidationState& state) { + m_internals->BlockChecked(block, state); +} + +void CMainSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr &block) { + m_internals->NewPoWValidBlock(pindex, block); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 17545018d..c248781a4 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -6,7 +6,6 @@ #ifndef BITCOIN_VALIDATIONINTERFACE_H #define BITCOIN_VALIDATIONINTERFACE_H -#include #include #include "primitives/transaction.h" // CTransaction(Ref) @@ -32,49 +31,60 @@ void UnregisterAllValidationInterfaces(); class CValidationInterface { protected: - virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} - virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {} - virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) {} - virtual void BlockDisconnected(const std::shared_ptr &block) {} - virtual void SetBestChain(const CBlockLocator &locator) {} - virtual void Inventory(const uint256 &hash) {} - virtual void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) {} - virtual void BlockChecked(const CBlock&, const CValidationState&) {} - virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; - friend void ::RegisterValidationInterface(CValidationInterface*); - friend void ::UnregisterValidationInterface(CValidationInterface*); - friend void ::UnregisterAllValidationInterfaces(); -}; - -struct CMainSignals { /** Notifies listeners of updated block chain tip */ - boost::signals2::signal UpdatedBlockTip; + virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} /** Notifies listeners of a transaction having been added to mempool. */ - boost::signals2::signal TransactionAddedToMempool; + virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {} /** * Notifies listeners of a block being connected. * Provides a vector of transactions evicted from the mempool as a result. */ - boost::signals2::signal &, const CBlockIndex *pindex, const std::vector &)> BlockConnected; + virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) {} /** Notifies listeners of a block being disconnected */ - boost::signals2::signal &)> BlockDisconnected; - /** Notifies listeners of a new active block chain. */ - boost::signals2::signal SetBestChain; + virtual void BlockDisconnected(const std::shared_ptr &block) {} + /** Notifies listeners of the new active block chain on-disk. */ + virtual void SetBestChain(const CBlockLocator &locator) {} /** Notifies listeners about an inventory item being seen on the network. */ - boost::signals2::signal Inventory; + virtual void Inventory(const uint256 &hash) {} /** Tells listeners to broadcast their data. */ - boost::signals2::signal Broadcast; + virtual void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) {} /** * Notifies listeners of a block validation result. * If the provided CValidationState IsValid, the provided block * is guaranteed to be the current best block at the time the * callback was generated (not necessarily now) */ - boost::signals2::signal BlockChecked; + virtual void BlockChecked(const CBlock&, const CValidationState&) {} /** * 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 */ - boost::signals2::signal&)> NewPoWValidBlock; + virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; + friend void ::RegisterValidationInterface(CValidationInterface*); + friend void ::UnregisterValidationInterface(CValidationInterface*); + friend void ::UnregisterAllValidationInterfaces(); +}; + +struct MainSignalsInstance; +class CMainSignals { +private: + std::unique_ptr m_internals; + + friend void ::RegisterValidationInterface(CValidationInterface*); + friend void ::UnregisterValidationInterface(CValidationInterface*); + friend void ::UnregisterAllValidationInterfaces(); +public: + CMainSignals(); + + void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); + void TransactionAddedToMempool(const CTransactionRef &); + void BlockConnected(const std::shared_ptr &, const CBlockIndex *pindex, const std::vector &); + void BlockDisconnected(const std::shared_ptr &); + void UpdatedTransaction(const uint256 &); + void SetBestChain(const CBlockLocator &); + void Inventory(const uint256 &); + void Broadcast(int64_t nBestBlockTime, CConnman* connman); + void BlockChecked(const CBlock&, const CValidationState&); + void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&); }; CMainSignals& GetMainSignals(); From cda1429d5bfee129a0d1f6f1c65962b30251bafb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Jan 2017 16:49:22 -0500 Subject: [PATCH 3/7] Give CMainSignals a reference to the global scheduler ...so that it can run some signals in the background later --- src/init.cpp | 3 +++ src/test/test_bitcoin.cpp | 7 +++++++ src/test/test_bitcoin.h | 2 ++ src/validationinterface.cpp | 13 +++++++++++++ src/validationinterface.h | 7 +++++++ 5 files changed, 32 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 672ef77e8..94bba6820 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -251,6 +251,7 @@ void Shutdown() } #endif UnregisterAllValidationInterfaces(); + GetMainSignals().UnregisterBackgroundSignalScheduler(); #ifdef ENABLE_WALLET for (CWalletRef pwallet : vpwallets) { delete pwallet; @@ -1203,6 +1204,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) CScheduler::Function serviceLoop = boost::bind(&CScheduler::serviceQueue, &scheduler); threadGroup.create_thread(boost::bind(&TraceThread, "scheduler", serviceLoop)); + GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); + /* Start the RPC server already. It will be started in "warmup" mode * and not really process calls already (but it will signify connections * that the server is there and will be ready later). Warmup mode will diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 579e96524..e6a8cc779 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -62,6 +62,12 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha pathTemp = GetTempPath() / strprintf("test_bitcoin_%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(100000))); fs::create_directories(pathTemp); ForceSetArg("-datadir", pathTemp.string()); + + // Note that because we don't bother running a scheduler thread here, + // callbacks via CValidationInterface are unreliable, but that's OK, + // our unit tests aren't testing multiple parts of the code at once. + GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); + mempool.setSanityCheck(1.0); pblocktree = new CBlockTreeDB(1 << 20, true); pcoinsdbview = new CCoinsViewDB(1 << 23, true); @@ -88,6 +94,7 @@ TestingSetup::~TestingSetup() UnregisterNodeSignals(GetNodeSignals()); threadGroup.interrupt_all(); threadGroup.join_all(); + GetMainSignals().UnregisterBackgroundSignalScheduler(); UnloadBlockIndex(); delete pcoinsTip; delete pcoinsdbview; diff --git a/src/test/test_bitcoin.h b/src/test/test_bitcoin.h index c9e4a3427..dd3b13c8c 100644 --- a/src/test/test_bitcoin.h +++ b/src/test/test_bitcoin.h @@ -10,6 +10,7 @@ #include "key.h" #include "pubkey.h" #include "random.h" +#include "scheduler.h" #include "txdb.h" #include "txmempool.h" @@ -53,6 +54,7 @@ struct TestingSetup: public BasicTestingSetup { fs::path pathTemp; boost::thread_group threadGroup; CConnman* connman; + CScheduler scheduler; TestingSetup(const std::string& chainName = CBaseChainParams::MAIN); ~TestingSetup(); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 7d0ab756e..a17a08eee 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -4,6 +4,8 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "validationinterface.h" +#include "init.h" +#include "scheduler.h" #include @@ -17,6 +19,8 @@ struct MainSignalsInstance { boost::signals2::signal Broadcast; boost::signals2::signal BlockChecked; boost::signals2::signal&)> NewPoWValidBlock; + + CScheduler *m_scheduler = NULL; }; static CMainSignals g_signals; @@ -25,6 +29,15 @@ CMainSignals::CMainSignals() { m_internals.reset(new MainSignalsInstance()); } +void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) { + assert(!m_internals->m_scheduler); + m_internals->m_scheduler = &scheduler; +} + +void CMainSignals::UnregisterBackgroundSignalScheduler() { + m_internals->m_scheduler = NULL; +} + CMainSignals& GetMainSignals() { return g_signals; diff --git a/src/validationinterface.h b/src/validationinterface.h index c248781a4..8cae3c6db 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -19,6 +19,7 @@ class CReserveScript; class CValidationInterface; class CValidationState; class uint256; +class CScheduler; // These functions dispatch to one or all registered wallets @@ -72,9 +73,15 @@ private: friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); + public: CMainSignals(); + /** Register a CScheduler to give callbacks which should run in the background (may only be called once) */ + void RegisterBackgroundSignalScheduler(CScheduler& scheduler); + /** Unregister a CScheduler to give callbacks which should run in the background - these callbacks will now be dropped! */ + void UnregisterBackgroundSignalScheduler(); + void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef &); void BlockConnected(const std::shared_ptr &, const CBlockIndex *pindex, const std::vector &); From 2fbf2dbe151e135586cc1bb05b891f2c8ab6c817 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 20 Jan 2017 15:10:43 -0500 Subject: [PATCH 4/7] Add default arg to CScheduler to schedule() a callback now --- src/scheduler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scheduler.h b/src/scheduler.h index 27412a15b..5da6e6f69 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -41,7 +41,7 @@ public: typedef std::function Function; // Call func at/after time t - void schedule(Function f, boost::chrono::system_clock::time_point t); + void schedule(Function f, boost::chrono::system_clock::time_point t=boost::chrono::system_clock::now()); // Convenience method: call f once deltaSeconds from now void scheduleFromNow(Function f, int64_t deltaMilliSeconds); From 08096bbbc6d6fef86943ca8ce5e6de18744d58ea Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Apr 2017 14:55:49 -0400 Subject: [PATCH 5/7] Support more than one CScheduler thread for serial clients This will be used by CValidationInterface soon. This requires a bit of work as we need to ensure that most of our callbacks happen in-order (to avoid synchronization issues in wallet) - we keep our own internal queue and push things onto it, scheduling a queue-draining function immediately upon new callbacks. --- src/scheduler.cpp | 52 +++++++++++++++++++++++++++++++++++++ src/scheduler.h | 24 +++++++++++++++++ src/validationinterface.cpp | 22 ++++++++++------ src/validationinterface.h | 2 -- 4 files changed, 90 insertions(+), 10 deletions(-) diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 923ba2c23..a76a87e10 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -139,3 +139,55 @@ size_t CScheduler::getQueueInfo(boost::chrono::system_clock::time_point &first, } return result; } + + +void SingleThreadedSchedulerClient::MaybeScheduleProcessQueue() { + { + LOCK(m_cs_callbacks_pending); + // Try to avoid scheduling too many copies here, but if we + // accidentally have two ProcessQueue's scheduled at once its + // not a big deal. + if (m_are_callbacks_running) return; + if (m_callbacks_pending.empty()) return; + } + m_pscheduler->schedule(std::bind(&SingleThreadedSchedulerClient::ProcessQueue, this)); +} + +void SingleThreadedSchedulerClient::ProcessQueue() { + std::function callback; + { + LOCK(m_cs_callbacks_pending); + if (m_are_callbacks_running) return; + if (m_callbacks_pending.empty()) return; + m_are_callbacks_running = true; + + callback = std::move(m_callbacks_pending.front()); + m_callbacks_pending.pop_front(); + } + + // RAII the setting of fCallbacksRunning and calling MaybeScheduleProcessQueue + // to ensure both happen safely even if callback() throws. + struct RAIICallbacksRunning { + SingleThreadedSchedulerClient* instance; + RAIICallbacksRunning(SingleThreadedSchedulerClient* _instance) : instance(_instance) {} + ~RAIICallbacksRunning() { + { + LOCK(instance->m_cs_callbacks_pending); + instance->m_are_callbacks_running = false; + } + instance->MaybeScheduleProcessQueue(); + } + } raiicallbacksrunning(this); + + callback(); +} + +void SingleThreadedSchedulerClient::AddToProcessQueue(std::function func) { + assert(m_pscheduler); + + { + LOCK(m_cs_callbacks_pending); + m_callbacks_pending.emplace_back(std::move(func)); + } + MaybeScheduleProcessQueue(); +} diff --git a/src/scheduler.h b/src/scheduler.h index 5da6e6f69..82036afdf 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -14,6 +14,8 @@ #include #include +#include "sync.h" + // // Simple class for background tasks that should be run // periodically or once "after a while" @@ -79,4 +81,26 @@ private: bool shouldStop() { return stopRequested || (stopWhenEmpty && taskQueue.empty()); } }; +/** + * Class used by CScheduler clients which may schedule multiple jobs + * which are required to be run serially. Does not require such jobs + * to be executed on the same thread, but no two jobs will be executed + * at the same time. + */ +class SingleThreadedSchedulerClient { +private: + CScheduler *m_pscheduler; + + CCriticalSection m_cs_callbacks_pending; + std::list> m_callbacks_pending; + bool m_are_callbacks_running = false; + + void MaybeScheduleProcessQueue(); + void ProcessQueue(); + +public: + SingleThreadedSchedulerClient(CScheduler *pschedulerIn) : m_pscheduler(pschedulerIn) {} + void AddToProcessQueue(std::function func); +}; + #endif diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index a17a08eee..8edc7c398 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -6,6 +6,11 @@ #include "validationinterface.h" #include "init.h" #include "scheduler.h" +#include "sync.h" +#include "util.h" + +#include +#include #include @@ -20,22 +25,23 @@ struct MainSignalsInstance { boost::signals2::signal BlockChecked; boost::signals2::signal&)> NewPoWValidBlock; - CScheduler *m_scheduler = NULL; + // We are not allowed to assume the scheduler only runs in one thread, + // but must ensure all callbacks happen in-order, so we end up creating + // our own queue here :( + SingleThreadedSchedulerClient m_schedulerClient; + + MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {} }; static CMainSignals g_signals; -CMainSignals::CMainSignals() { - m_internals.reset(new MainSignalsInstance()); -} - void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) { - assert(!m_internals->m_scheduler); - m_internals->m_scheduler = &scheduler; + assert(!m_internals); + m_internals.reset(new MainSignalsInstance(&scheduler)); } void CMainSignals::UnregisterBackgroundSignalScheduler() { - m_internals->m_scheduler = NULL; + m_internals.reset(nullptr); } CMainSignals& GetMainSignals() diff --git a/src/validationinterface.h b/src/validationinterface.h index 8cae3c6db..fbfe273b1 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -75,8 +75,6 @@ private: friend void ::UnregisterAllValidationInterfaces(); public: - CMainSignals(); - /** Register a CScheduler to give callbacks which should run in the background (may only be called once) */ void RegisterBackgroundSignalScheduler(CScheduler& scheduler); /** Unregister a CScheduler to give callbacks which should run in the background - these callbacks will now be dropped! */ From 3192975f1d177aa9f0bbd823c6387cfbfa943610 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 27 Jun 2017 19:07:52 -0400 Subject: [PATCH 6/7] Flush CValidationInterface callbacks prior to destruction Note that the CScheduler thread cant be running at this point, it has already been stopped with the rest of the init threadgroup. Thus, just calling any remaining loose callbacks during Shutdown() is sane. --- src/init.cpp | 13 +++++++++++++ src/scheduler.cpp | 9 +++++++++ src/scheduler.h | 3 +++ src/test/test_bitcoin.cpp | 1 + src/validationinterface.cpp | 4 ++++ src/validationinterface.h | 2 ++ 6 files changed, 32 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 94bba6820..12f29a4ca 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -215,6 +215,19 @@ void Shutdown() fFeeEstimatesInitialized = false; } + // FlushStateToDisk generates a SetBestChain callback, which we should avoid missing + FlushStateToDisk(); + + // After there are no more peers/RPC left to give us new data which may generate + // CValidationInterface callbacks, flush them... + GetMainSignals().FlushBackgroundCallbacks(); + + // Any future callbacks will be dropped. This should absolutely be safe - if + // missing a callback results in an unrecoverable situation, unclean shutdown + // would too. The only reason to do the above flushes is to let the wallet catch + // up with our current chain to avoid any strange pruning edge cases and make + // next startup faster by avoiding rescan. + { LOCK(cs_main); if (pcoinsTip != NULL) { diff --git a/src/scheduler.cpp b/src/scheduler.cpp index a76a87e10..35bf0da4b 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -191,3 +191,12 @@ void SingleThreadedSchedulerClient::AddToProcessQueue(std::function } MaybeScheduleProcessQueue(); } + +void SingleThreadedSchedulerClient::EmptyQueue() { + bool should_continue = true; + while (should_continue) { + ProcessQueue(); + LOCK(m_cs_callbacks_pending); + should_continue = !m_callbacks_pending.empty(); + } +} diff --git a/src/scheduler.h b/src/scheduler.h index 82036afdf..6a079f774 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -101,6 +101,9 @@ private: public: SingleThreadedSchedulerClient(CScheduler *pschedulerIn) : m_pscheduler(pschedulerIn) {} void AddToProcessQueue(std::function func); + + // Processes all remaining queue members on the calling thread, blocking until queue is empty + void EmptyQueue(); }; #endif diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index e6a8cc779..3ba81ed17 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -94,6 +94,7 @@ TestingSetup::~TestingSetup() UnregisterNodeSignals(GetNodeSignals()); threadGroup.interrupt_all(); threadGroup.join_all(); + GetMainSignals().FlushBackgroundCallbacks(); GetMainSignals().UnregisterBackgroundSignalScheduler(); UnloadBlockIndex(); delete pcoinsTip; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 8edc7c398..bf20d606f 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -44,6 +44,10 @@ void CMainSignals::UnregisterBackgroundSignalScheduler() { m_internals.reset(nullptr); } +void CMainSignals::FlushBackgroundCallbacks() { + m_internals->m_schedulerClient.EmptyQueue(); +} + CMainSignals& GetMainSignals() { return g_signals; diff --git a/src/validationinterface.h b/src/validationinterface.h index fbfe273b1..568da66df 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -79,6 +79,8 @@ public: void RegisterBackgroundSignalScheduler(CScheduler& scheduler); /** Unregister a CScheduler to give callbacks which should run in the background - these callbacks will now be dropped! */ void UnregisterBackgroundSignalScheduler(); + /** Call any remaining callbacks on the calling thread */ + void FlushBackgroundCallbacks(); void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef &); From 1f668b646806f94acd851acdbd9939c24e0492d3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Jul 2017 21:08:19 -0400 Subject: [PATCH 7/7] Expose if CScheduler is being serviced, assert its not in EmptyQueue --- src/scheduler.cpp | 5 +++++ src/scheduler.h | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 35bf0da4b..36a6d5110 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -140,6 +140,10 @@ size_t CScheduler::getQueueInfo(boost::chrono::system_clock::time_point &first, return result; } +bool CScheduler::AreThreadsServicingQueue() const { + return nThreadsServicingQueue; +} + void SingleThreadedSchedulerClient::MaybeScheduleProcessQueue() { { @@ -193,6 +197,7 @@ void SingleThreadedSchedulerClient::AddToProcessQueue(std::function } void SingleThreadedSchedulerClient::EmptyQueue() { + assert(!m_pscheduler->AreThreadsServicingQueue()); bool should_continue = true; while (should_continue) { ProcessQueue(); diff --git a/src/scheduler.h b/src/scheduler.h index 6a079f774..0365d668b 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -71,6 +71,9 @@ public: size_t getQueueInfo(boost::chrono::system_clock::time_point &first, boost::chrono::system_clock::time_point &last) const; + // Returns true if there are threads actively running in serviceQueue() + bool AreThreadsServicingQueue() const; + private: std::multimap taskQueue; boost::condition_variable newTaskScheduled; @@ -103,6 +106,7 @@ public: void AddToProcessQueue(std::function func); // Processes all remaining queue members on the calling thread, blocking until queue is empty + // Must be called after the CScheduler has no remaining processing threads! void EmptyQueue(); };