From 8f3865792a7f272cef9664b452a0f98eb6b1bde4 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 5 Feb 2021 12:04:19 -0300 Subject: [PATCH 01/12] make ConnectBlock static in validation.cpp --- src/validation.cpp | 5 ++++- src/validation.h | 3 --- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b9dcc4459a73..e9d150ba2ad7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1388,7 +1388,10 @@ static int64_t nTimeIndex = 0; static int64_t nTimeCallbacks = 0; static int64_t nTimeTotal = 0; -bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck, bool fAlreadyChecked) +/** Apply the effects of this block (with given index) on the UTXO set represented by coins. + * Validity checks that depend on the UTXO set are also done; ConnectBlock() + * can fail if those validity checks fail (among other reasons). */ +static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck = false, bool fAlreadyChecked = false) { AssertLockHeld(cs_main); // Check it again in case a previous version let a bad block in diff --git a/src/validation.h b/src/validation.h index 6f6804ac39c3..be6d571c935c 100644 --- a/src/validation.h +++ b/src/validation.h @@ -322,9 +322,6 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex); /** Functions for validating blocks and updating the block tree */ -/** Apply the effects of this block (with given index) on the UTXO set represented by coins */ -bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& coins, bool fJustCheck, bool fAlreadyChecked = false); - /** Context-independent validity checks */ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW = true, bool fCheckMerkleRoot = true, bool fCheckSig = true); bool CheckWork(const CBlock& block, const CBlockIndex* const pindexPrev); From 2c9de457c10a0270c47966b5259f72f6939778e9 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 5 Feb 2021 12:24:21 -0300 Subject: [PATCH 02/12] Remove unused UpdatedTransaction signal and connected slot. This was only used to show coinbase transactions differently (with two confirmations of delay) in upstream and we removed such difference a long time ago. --- src/validation.cpp | 5 ----- src/validationinterface.cpp | 8 -------- src/validationinterface.h | 2 -- src/wallet/wallet.cpp | 14 -------------- src/wallet/wallet.h | 2 -- 5 files changed, 31 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e9d150ba2ad7..8537b51469b1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1707,11 +1707,6 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd nTimeIndex += nTime3 - nTime2; LogPrint(BCLog::BENCH, " - Index writing: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeIndex * 0.000001); - // Watch for changes to the previous coinbase transaction. - static uint256 hashPrevBestCoinBase; - GetMainSignals().UpdatedTransaction(hashPrevBestCoinBase); - hashPrevBestCoinBase = block.vtx[0]->GetHash(); - int64_t nTime4 = GetTimeMicros(); nTimeCallbacks += nTime4 - nTime3; LogPrint(BCLog::BENCH, " - Callbacks: %.2fms [%.2fs]\n", 0.001 * (nTime4 - nTime3), nTimeCallbacks * 0.000001); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 9fa5029fe023..760ff4bdd9db 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -15,7 +15,6 @@ struct ValidationInterfaceConnections { boost::signals2::scoped_connection BlockConnected; boost::signals2::scoped_connection BlockDisconnected; boost::signals2::scoped_connection NotifyTransactionLock; - boost::signals2::scoped_connection UpdatedTransaction; boost::signals2::scoped_connection SetBestChain; boost::signals2::scoped_connection Broadcast; boost::signals2::scoped_connection BlockChecked; @@ -36,8 +35,6 @@ struct MainSignalsInstance { boost::signals2::signal &, int nBlockHeight)> BlockDisconnected; /** Notifies listeners of an updated transaction lock without new data. */ boost::signals2::signal NotifyTransactionLock; - /** Notifies listeners of an updated transaction without new data (for now: a coinbase potentially becoming visible). */ - boost::signals2::signal UpdatedTransaction; /** Notifies listeners of a new active block chain. */ boost::signals2::signal SetBestChain; /** Tells listeners to broadcast their data. */ @@ -67,7 +64,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) conns.BlockConnected = g_signals.m_internals->BlockConnected.connect(std::bind(&CValidationInterface::BlockConnected, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.NotifyTransactionLock = g_signals.m_internals->NotifyTransactionLock.connect(std::bind(&CValidationInterface::NotifyTransactionLock, pwalletIn, std::placeholders::_1)); - conns.UpdatedTransaction = g_signals.m_internals->UpdatedTransaction.connect(std::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, std::placeholders::_1)); conns.SetBestChain = g_signals.m_internals->SetBestChain.connect(std::bind(&CValidationInterface::SetBestChain, pwalletIn, std::placeholders::_1)); conns.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, std::placeholders::_1)); conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2)); @@ -108,10 +104,6 @@ void CMainSignals::NotifyTransactionLock(const CTransaction& tx) { m_internals->NotifyTransactionLock(tx); } -void CMainSignals::UpdatedTransaction(const uint256& hash) { - m_internals->UpdatedTransaction(hash); -} - void CMainSignals::SetBestChain(const CBlockLocator& locator) { m_internals->SetBestChain(locator); } diff --git a/src/validationinterface.h b/src/validationinterface.h index af980450a250..786d51a3add3 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -38,7 +38,6 @@ class CValidationInterface { virtual void NotifyTransactionLock(const CTransaction &tx) {} /** Notifies listeners of the new active block chain on-disk. */ virtual void SetBestChain(const CBlockLocator &locator) {} - virtual bool UpdatedTransaction(const uint256 &hash) { return false;} /** Tells listeners to broadcast their data. */ virtual void ResendWalletTransactions(CConnman* connman) {} virtual void BlockChecked(const CBlock&, const CValidationState&) {} @@ -63,7 +62,6 @@ class CMainSignals { void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted); void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight); void NotifyTransactionLock(const CTransaction&); - void UpdatedTransaction(const uint256 &); void SetBestChain(const CBlockLocator &); void Broadcast(CConnman* connman); void BlockChecked(const CBlock&, const CValidationState&); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0e9f2b526f18..56c343844210 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3552,20 +3552,6 @@ void CReserveKey::ReturnKey() vchPubKey = CPubKey(); } -bool CWallet::UpdatedTransaction(const uint256& hashTx) -{ - { - LOCK(cs_wallet); - // Only notify UI if this transaction is in this wallet - std::map::const_iterator mi = mapWallet.find(hashTx); - if (mi != mapWallet.end()) { - NotifyTransactionChanged(this, hashTx, CT_UPDATED); - return true; - } - } - return false; -} - void CWallet::LockCoin(const COutPoint& output) { AssertLockHeld(cs_wallet); // setLockedCoins diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 6a9e589cb700..d6aa1e2fa43f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -748,8 +748,6 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void LoadAddressBookName(const CWDestination& dest, const std::string& strName); void LoadAddressBookPurpose(const CWDestination& dest, const std::string& strPurpose); - bool UpdatedTransaction(const uint256& hashTx) override; - unsigned int GetKeyPoolSize(); unsigned int GetStakingKeyPoolSize(); From c26d18cb2022ab1228986f213006c9a2ccf086d1 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 5 Feb 2021 12:32:11 -0300 Subject: [PATCH 03/12] Remove unused NotifyTransactionLock signal. --- src/validationinterface.cpp | 8 -------- src/validationinterface.h | 2 -- src/zmq/zmqabstractnotifier.cpp | 4 ---- src/zmq/zmqabstractnotifier.h | 1 - src/zmq/zmqnotificationinterface.cpp | 19 ------------------- src/zmq/zmqnotificationinterface.h | 1 - src/zmq/zmqpublishnotifier.cpp | 23 ----------------------- src/zmq/zmqpublishnotifier.h | 12 ------------ 8 files changed, 70 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 760ff4bdd9db..fafa1841367e 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -14,7 +14,6 @@ struct ValidationInterfaceConnections { boost::signals2::scoped_connection TransactionAddedToMempool; boost::signals2::scoped_connection BlockConnected; boost::signals2::scoped_connection BlockDisconnected; - boost::signals2::scoped_connection NotifyTransactionLock; boost::signals2::scoped_connection SetBestChain; boost::signals2::scoped_connection Broadcast; boost::signals2::scoped_connection BlockChecked; @@ -33,8 +32,6 @@ struct MainSignalsInstance { boost::signals2::signal &, const CBlockIndex *pindex, const std::vector &)> BlockConnected; /** Notifies listeners of a block being disconnected */ boost::signals2::signal &, int nBlockHeight)> BlockDisconnected; - /** Notifies listeners of an updated transaction lock without new data. */ - boost::signals2::signal NotifyTransactionLock; /** Notifies listeners of a new active block chain. */ boost::signals2::signal SetBestChain; /** Tells listeners to broadcast their data. */ @@ -63,7 +60,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) 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, std::placeholders::_3)); conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); - conns.NotifyTransactionLock = g_signals.m_internals->NotifyTransactionLock.connect(std::bind(&CValidationInterface::NotifyTransactionLock, pwalletIn, std::placeholders::_1)); conns.SetBestChain = g_signals.m_internals->SetBestChain.connect(std::bind(&CValidationInterface::SetBestChain, pwalletIn, std::placeholders::_1)); conns.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, std::placeholders::_1)); conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2)); @@ -100,10 +96,6 @@ void CMainSignals::BlockDisconnected(const std::shared_ptr &block, m_internals->BlockDisconnected(block, nBlockHeight); } -void CMainSignals::NotifyTransactionLock(const CTransaction& tx) { - m_internals->NotifyTransactionLock(tx); -} - void CMainSignals::SetBestChain(const CBlockLocator& locator) { m_internals->SetBestChain(locator); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 786d51a3add3..436531679bb0 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -35,7 +35,6 @@ class CValidationInterface { 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, int nBlockHeight) {} - virtual void NotifyTransactionLock(const CTransaction &tx) {} /** Notifies listeners of the new active block chain on-disk. */ virtual void SetBestChain(const CBlockLocator &locator) {} /** Tells listeners to broadcast their data. */ @@ -61,7 +60,6 @@ class CMainSignals { void TransactionAddedToMempool(const CTransactionRef &ptxn); void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted); void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight); - void NotifyTransactionLock(const CTransaction&); void SetBestChain(const CBlockLocator &); void Broadcast(CConnman* connman); void BlockChecked(const CBlock&, const CValidationState&); diff --git a/src/zmq/zmqabstractnotifier.cpp b/src/zmq/zmqabstractnotifier.cpp index 76543a4a797a..53dac44aebc8 100644 --- a/src/zmq/zmqabstractnotifier.cpp +++ b/src/zmq/zmqabstractnotifier.cpp @@ -21,7 +21,3 @@ bool CZMQAbstractNotifier::NotifyTransaction(const CTransaction &/*transaction*/ return true; } -bool CZMQAbstractNotifier::NotifyTransactionLock(const CTransaction &/*transaction*/) -{ - return true; -} diff --git a/src/zmq/zmqabstractnotifier.h b/src/zmq/zmqabstractnotifier.h index 0d3f1fcd4e41..77cf5141e27b 100644 --- a/src/zmq/zmqabstractnotifier.h +++ b/src/zmq/zmqabstractnotifier.h @@ -34,7 +34,6 @@ class CZMQAbstractNotifier virtual bool NotifyBlock(const CBlockIndex *pindex); virtual bool NotifyTransaction(const CTransaction &transaction); - virtual bool NotifyTransactionLock(const CTransaction &transaction); protected: void *psocket; diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index fffa3bbb023f..19ad4a0eadd7 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -36,10 +36,8 @@ CZMQNotificationInterface* CZMQNotificationInterface::Create() factories["pubhashblock"] = CZMQAbstractNotifier::Create; factories["pubhashtx"] = CZMQAbstractNotifier::Create; - factories["pubhashtxlock"] = CZMQAbstractNotifier::Create; factories["pubrawblock"] = CZMQAbstractNotifier::Create; factories["pubrawtx"] = CZMQAbstractNotifier::Create; - factories["pubrawtxlock"] = CZMQAbstractNotifier::Create; for (std::map::const_iterator i=factories.begin(); i!=factories.end(); ++i) { @@ -181,20 +179,3 @@ void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr::iterator i = notifiers.begin(); i!=notifiers.end(); ) - { - CZMQAbstractNotifier *notifier = *i; - if (notifier->NotifyTransactionLock(tx)) - { - i++; - } - else - { - notifier->Shutdown(); - i = notifiers.erase(i); - } - } -} diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 136724e102c3..b4df973d9781 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -28,7 +28,6 @@ class CZMQNotificationInterface : public CValidationInterface void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) override; void BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) override; - void NotifyTransactionLock(const CTransaction &tx) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; private: diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index 7541bcf726fa..ee08858fe18e 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -13,10 +13,8 @@ static std::multimap mapPublishNotifi static const char *MSG_HASHBLOCK = "hashblock"; static const char *MSG_HASHTX = "hashtx"; -static const char *MSG_HASHTXLOCK = "hashtxlock"; static const char *MSG_RAWBLOCK = "rawblock"; static const char *MSG_RAWTX = "rawtx"; -static const char *MSG_RAWTXLOCK = "rawtxlock"; // Internal function to send multipart message static int zmq_send_multipart(void *sock, const void* data, size_t size, ...) @@ -164,26 +162,14 @@ bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &t return SendMessage(MSG_HASHTX, data, 32); } -bool CZMQPublishHashTransactionLockNotifier::NotifyTransactionLock(const CTransaction &transaction) -{ - uint256 hash = transaction.GetHash(); - LogPrint(BCLog::ZMQ, "Publish hashtxlock %s\n", hash.GetHex()); - char data[32]; - for (unsigned int i = 0; i < 32; i++) - data[31 - i] = hash.begin()[i]; - return SendMessage(MSG_HASHTXLOCK, data, 32); -} - bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex) { LogPrint(BCLog::ZMQ, "Publish rawblock %s\n", pindex->GetBlockHash().GetHex()); -// XX42 const Consensus::Params& consensusParams = Params().GetConsensus(); CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); { LOCK(cs_main); CBlock block; -// XX42 if(!ReadBlockFromDisk(block, pindex, consensusParams)) if(!ReadBlockFromDisk(block, pindex)) { zmqError("Can't read block from disk"); @@ -204,12 +190,3 @@ bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &tr ss << transaction; return SendMessage(MSG_RAWTX, &(*ss.begin()), ss.size()); } - -bool CZMQPublishRawTransactionLockNotifier::NotifyTransactionLock(const CTransaction &transaction) -{ - uint256 hash = transaction.GetHash(); - LogPrint(BCLog::ZMQ, "Publish rawtxlock %s\n", hash.GetHex()); - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss << transaction; - return SendMessage(MSG_RAWTXLOCK, &(*ss.begin()), ss.size()); -} diff --git a/src/zmq/zmqpublishnotifier.h b/src/zmq/zmqpublishnotifier.h index d6a52e902262..75d0bcba991f 100644 --- a/src/zmq/zmqpublishnotifier.h +++ b/src/zmq/zmqpublishnotifier.h @@ -40,12 +40,6 @@ class CZMQPublishHashTransactionNotifier : public CZMQAbstractPublishNotifier bool NotifyTransaction(const CTransaction &transaction); }; -class CZMQPublishHashTransactionLockNotifier : public CZMQAbstractPublishNotifier -{ -public: - bool NotifyTransactionLock(const CTransaction &transaction); -}; - class CZMQPublishRawBlockNotifier : public CZMQAbstractPublishNotifier { public: @@ -58,10 +52,4 @@ class CZMQPublishRawTransactionNotifier : public CZMQAbstractPublishNotifier bool NotifyTransaction(const CTransaction &transaction); }; -class CZMQPublishRawTransactionLockNotifier : public CZMQAbstractPublishNotifier -{ -public: - bool NotifyTransactionLock(const CTransaction &transaction); -}; - #endif // BITCOIN_ZMQ_ZMQPUBLISHNOTIFIER_H From d583d125296db57a44f6b4e6f949df9df156c6bd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 5 Feb 2021 13:11:18 -0300 Subject: [PATCH 04/12] 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_pivx.cpp | 7 +++++++ src/test/test_pivx.h | 2 ++ src/validationinterface.cpp | 12 ++++++++++++ src/validationinterface.h | 7 +++++++ 5 files changed, 31 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 28044ab55243..7b19bcdea272 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -301,6 +301,7 @@ void PrepareShutdown() // Disconnect all slots UnregisterAllValidationInterfaces(); + GetMainSignals().UnregisterBackgroundSignalScheduler(); #ifndef WIN32 try { @@ -1243,6 +1244,8 @@ bool AppInitMain() CScheduler::Function serviceLoop = std::bind(&CScheduler::serviceQueue, &scheduler); threadGroup.create_thread(std::bind(&TraceThread, "scheduler", serviceLoop)); + GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); + // Initialize Sapling circuit parameters LoadSaplingParams(); diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index d803be76aace..5ec8a5c0bbd9 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -53,6 +53,12 @@ TestingSetup::TestingSetup() pathTemp = GetTempPath() / strprintf("test_pivx_%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(100000))); fs::create_directories(pathTemp); gArgs.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); + // Ideally we'd move all the RPC tests to the functional testing framework // instead of unit tests, but for now we need these here. RegisterAllCoreRPCCommands(tableRPC); @@ -80,6 +86,7 @@ TestingSetup::~TestingSetup() UnregisterNodeSignals(GetNodeSignals()); threadGroup.interrupt_all(); threadGroup.join_all(); + GetMainSignals().UnregisterBackgroundSignalScheduler(); UnloadBlockIndex(); delete pcoinsTip; delete pcoinsdbview; diff --git a/src/test/test_pivx.h b/src/test/test_pivx.h index b56e18a80376..d22da8ecd1bd 100644 --- a/src/test/test_pivx.h +++ b/src/test/test_pivx.h @@ -6,6 +6,7 @@ #define PIVX_TEST_TEST_PIVX_H #include "fs.h" +#include "scheduler.h" #include "txdb.h" #include @@ -48,6 +49,7 @@ struct TestingSetup: public BasicTestingSetup { fs::path pathTemp; boost::thread_group threadGroup; CConnman* connman; + CScheduler scheduler; ECCVerifyHandle globalVerifyHandle; TestingSetup(); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index fafa1841367e..202819c37bf0 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -5,6 +5,8 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "validationinterface.h" +#include "init.h" +#include "scheduler.h" #include #include @@ -39,6 +41,7 @@ struct MainSignalsInstance { /** Notifies listeners of a block validation result */ boost::signals2::signal BlockChecked; + CScheduler *m_scheduler = nullptr; std::unordered_map m_connMainSignals; }; @@ -48,6 +51,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 = nullptr; +} + CMainSignals& GetMainSignals() { return g_signals; diff --git a/src/validationinterface.h b/src/validationinterface.h index 436531679bb0..26f8fda7f90d 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -18,6 +18,7 @@ class CConnman; class CValidationInterface; class CValidationState; class uint256; +class CScheduler; // These functions dispatch to one or all registered wallets @@ -53,9 +54,15 @@ class CMainSignals { 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 &ptxn); void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted); From 6532fcaaa631a94f350e70aa94a2717116915d0f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 20 Jan 2017 15:10:43 -0500 Subject: [PATCH 05/12] 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 9f1b703dd8f0..f81e65e62f5d 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -42,7 +42,7 @@ class CScheduler 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 deltaMilliSeconds from now void scheduleFromNow(Function f, int64_t deltaMilliSeconds); From 8022463c45930c311137ecfe2a68b159f6c801f1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 5 Feb 2021 13:15:26 -0300 Subject: [PATCH 06/12] 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 | 51 +++++++++++++++++++++++++++++++++++++ src/scheduler.h | 24 +++++++++++++++++ src/validationinterface.cpp | 22 ++++++++++------ src/validationinterface.h | 2 -- 4 files changed, 89 insertions(+), 10 deletions(-) diff --git a/src/scheduler.cpp b/src/scheduler.cpp index bb671645b360..150c92028348 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -120,3 +120,54 @@ 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 f81e65e62f5d..00a511bac7d2 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -15,6 +15,8 @@ #include #include +#include "sync.h" + // // Simple class for background tasks that should be run // periodically or once "after a while" @@ -80,4 +82,26 @@ class CScheduler 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; + + RecursiveMutex 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 202819c37bf0..5c2741dbcf9f 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -7,7 +7,11 @@ #include "validationinterface.h" #include "init.h" #include "scheduler.h" +#include "sync.h" +#include "util.h" +#include +#include #include #include @@ -41,23 +45,25 @@ struct MainSignalsInstance { /** Notifies listeners of a block validation result */ boost::signals2::signal BlockChecked; - CScheduler *m_scheduler = nullptr; std::unordered_map m_connMainSignals; + + // 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 = nullptr; + m_internals.reset(nullptr); } CMainSignals& GetMainSignals() diff --git a/src/validationinterface.h b/src/validationinterface.h index 26f8fda7f90d..5ccdfc7e35c9 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -56,8 +56,6 @@ class CMainSignals { 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 64c525b8d697028b02e2735c55a788e518dac6ec Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 5 Feb 2021 13:20:14 -0300 Subject: [PATCH 07/12] Otherwise threads will try to continue modifying data structures while the dump process is being executed. 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_pivx.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 7b19bcdea272..66f858d86c93 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -265,6 +265,19 @@ void PrepareShutdown() 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 150c92028348..0a79f603b4f4 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -171,3 +171,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 00a511bac7d2..ada6370fce4f 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -102,6 +102,9 @@ class SingleThreadedSchedulerClient { 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_pivx.cpp b/src/test/test_pivx.cpp index 5ec8a5c0bbd9..8537114b1285 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -86,6 +86,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 5c2741dbcf9f..1af671456db4 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -66,6 +66,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 5ccdfc7e35c9..687c17222bf8 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -60,6 +60,8 @@ class CMainSignals { 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 &ptxn); From 207c17f3fc7131f63cd7f3c94ff7ca4514b35d1c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Jul 2017 21:08:19 -0400 Subject: [PATCH 08/12] 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 0a79f603b4f4..0d21f3eab0a1 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -121,6 +121,10 @@ size_t CScheduler::getQueueInfo(boost::chrono::system_clock::time_point &first, return result; } +bool CScheduler::AreThreadsServicingQueue() const { + return nThreadsServicingQueue; +} + void SingleThreadedSchedulerClient::MaybeScheduleProcessQueue() { { LOCK(m_cs_callbacks_pending); @@ -173,6 +177,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 ada6370fce4f..f578ee2db980 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -72,6 +72,9 @@ class CScheduler 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; @@ -104,6 +107,7 @@ class SingleThreadedSchedulerClient { 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(); }; From 9e523a7b3e022efebe27565cd61f72267fa0ec79 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 5 Feb 2021 17:12:43 -0300 Subject: [PATCH 09/12] Fix shutdown in case of errors during initialization PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization. Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's harden against this in any case. --- src/validationinterface.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 1af671456db4..3b3731595500 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -67,7 +67,9 @@ void CMainSignals::UnregisterBackgroundSignalScheduler() { } void CMainSignals::FlushBackgroundCallbacks() { - m_internals->m_schedulerClient.EmptyQueue(); + if (m_internals) { + m_internals->m_schedulerClient.EmptyQueue(); + } } CMainSignals& GetMainSignals() From 6d1a60e0b7b25c60dce71be93d2115ac2ff15741 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 5 Feb 2021 17:16:14 -0300 Subject: [PATCH 10/12] ValidationInterface compiler warning fix delete called on non-final 'PeerLogicValidation' that has virtual functions but non-virtual destructor Plus some small headers cleanup --- src/scheduler.cpp | 2 +- src/scheduler.h | 4 ++-- src/validationinterface.cpp | 6 +----- src/validationinterface.h | 2 ++ 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 0d21f3eab0a1..65c118c8e5c0 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -153,7 +153,7 @@ void SingleThreadedSchedulerClient::ProcessQueue() { // to ensure both happen safely even if callback() throws. struct RAIICallbacksRunning { SingleThreadedSchedulerClient* instance; - RAIICallbacksRunning(SingleThreadedSchedulerClient* _instance) : instance(_instance) {} + explicit RAIICallbacksRunning(SingleThreadedSchedulerClient* _instance) : instance(_instance) {} ~RAIICallbacksRunning() { { LOCK(instance->m_cs_callbacks_pending); diff --git a/src/scheduler.h b/src/scheduler.h index f578ee2db980..71ca40f4fb83 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -82,7 +82,7 @@ class CScheduler int nThreadsServicingQueue; bool stopRequested; bool stopWhenEmpty; - bool shouldStop() { return stopRequested || (stopWhenEmpty && taskQueue.empty()); } + bool shouldStop() const { return stopRequested || (stopWhenEmpty && taskQueue.empty()); } }; /** @@ -103,7 +103,7 @@ class SingleThreadedSchedulerClient { void ProcessQueue(); public: - SingleThreadedSchedulerClient(CScheduler *pschedulerIn) : m_pscheduler(pschedulerIn) {} + explicit 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 diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 3b3731595500..d38e04bf1f42 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -5,12 +5,8 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "validationinterface.h" -#include "init.h" #include "scheduler.h" -#include "sync.h" -#include "util.h" -#include #include #include #include @@ -52,7 +48,7 @@ struct MainSignalsInstance { // our own queue here :( SingleThreadedSchedulerClient m_schedulerClient; - MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {} + explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {} }; static CMainSignals g_signals; diff --git a/src/validationinterface.h b/src/validationinterface.h index 687c17222bf8..0a4343070426 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -30,6 +30,8 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn); void UnregisterAllValidationInterfaces(); class CValidationInterface { +public: + virtual ~CValidationInterface() = default; protected: /** Notifies listeners of updated block chain tip */ virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} From 73499ff3736aa29b0de7900894107551dda73ca5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 5 Feb 2021 17:30:27 -0300 Subject: [PATCH 11/12] Add missing lock in CScheduler::AreThreadsServicingQueue() Not an actual bug as this is only used in asserts right now, but nice to not have a missing lock. --- src/scheduler.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 65c118c8e5c0..0232ad2794ef 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -122,6 +122,7 @@ size_t CScheduler::getQueueInfo(boost::chrono::system_clock::time_point &first, } bool CScheduler::AreThreadsServicingQueue() const { + boost::unique_lock lock(newTaskMutex); return nThreadsServicingQueue; } From 6cecb7ba79927df09895c0ed44535bcba855f914 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 6 Feb 2021 11:31:13 -0300 Subject: [PATCH 12/12] AddToWalletIfInvolvingMe should test pIndex, not posInBlock Adaptation of btc@714e4ad13d805d45210d390fae83790360f47d9a --- src/wallet/wallet.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 56c343844210..0b084b0a3c33 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1037,9 +1037,9 @@ void CWallet::AddExternalNotesDataToTx(CWalletTx& wtx) const /** * Add a transaction to the wallet, or update it. pIndex and posInBlock should * be set when the transaction was known to be included in a block. When - * posInBlock = SYNC_TRANSACTION_NOT_IN_BLOCK (-1) , then wallet state is not - * updated in AddToWallet, but notifications happen and cached balances are - * marked dirty. + * pIndex == NULL, then wallet state is not updated in AddToWallet, but + * notifications happen and cached balances are marked dirty. + * * If fUpdate is true, existing transactions will be updated. * TODO: One exception to this is that the abandoned state is cleared under the * assumption that any further notification of a transaction that was considered @@ -1053,7 +1053,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256 { AssertLockHeld(cs_wallet); - if (posInBlock != -1 && !tx.HasZerocoinSpendInputs() && !tx.IsCoinBase()) { + if (!blockHash.IsNull() && !tx.HasZerocoinSpendInputs() && !tx.IsCoinBase()) { for (const CTxIn& txin : tx.vin) { std::pair range = mapTxSpends.equal_range(txin.prevout); while (range.first != range.second) { @@ -1102,8 +1102,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256 } // Get merkle branch if transaction was found in a block - if (posInBlock != -1) + if (!blockHash.IsNull()) { wtx.SetMerkleBranch(blockHash, posInBlock); + } return AddToWallet(wtx, false); }