From 139882d80e3da404574e04e2a939c266d8952b2c Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Mar 2021 12:08:05 -0300 Subject: [PATCH 01/10] Fire TransactionRemovedFromMempool from mempool This commit fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. Adaptation of btc@e20c72f9f076681def325b5b5fa53bccda2b0eab --- src/init.cpp | 2 -- src/test/test_pivx.cpp | 2 -- src/txmempool.cpp | 8 +++++++- src/validationinterface.cpp | 23 ++++++----------------- src/validationinterface.h | 10 +--------- 5 files changed, 14 insertions(+), 31 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index d7b4cbbcf53a..8fa9fafecca4 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -314,7 +314,6 @@ void PrepareShutdown() // Disconnect all slots UnregisterAllValidationInterfaces(); GetMainSignals().UnregisterBackgroundSignalScheduler(); - GetMainSignals().UnregisterWithMempoolSignals(mempool); #ifndef WIN32 try { @@ -1253,7 +1252,6 @@ bool AppInitMain() threadGroup.create_thread(std::bind(&TraceThread, "scheduler", serviceLoop)); GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); - GetMainSignals().RegisterWithMempoolSignals(mempool); // Initialize Sapling circuit parameters LoadSaplingParams(); diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index adad49eb9192..d2bc777c9ee0 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -68,7 +68,6 @@ TestingSetup::TestingSetup() // 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); - GetMainSignals().RegisterWithMempoolSignals(mempool); // Ideally we'd move all the RPC tests to the functional testing framework // instead of unit tests, but for now we need these here. @@ -102,7 +101,6 @@ TestingSetup::~TestingSetup() GetMainSignals().FlushBackgroundCallbacks(); UnregisterAllValidationInterfaces(); GetMainSignals().UnregisterBackgroundSignalScheduler(); - GetMainSignals().UnregisterWithMempoolSignals(mempool); UnloadBlockIndex(); delete pcoinsTip; delete pcoinsdbview; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 91747ba11f4e..bb800e9da976 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -16,6 +16,7 @@ #include "utiltime.h" #include "version.h" #include "validation.h" +#include "validationinterface.h" @@ -434,7 +435,12 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) { - NotifyEntryRemoved(it->GetSharedTx(), reason); + CTransactionRef ptx = it->GetSharedTx(); + NotifyEntryRemoved(ptx, reason); + if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) { + GetMainSignals().TransactionRemovedFromMempool(ptx); + } + AssertLockHeld(cs); const CTransaction& tx = it->GetTx(); for (const CTxIn& txin : tx.vin) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 76a5ff27be39..85cf3d83216a 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -6,7 +6,6 @@ #include "validationinterface.h" #include "scheduler.h" -#include "txmempool.h" #include "validation.h" #include @@ -79,14 +78,6 @@ size_t CMainSignals::CallbacksPending() { return m_internals->m_schedulerClient.CallbacksPending(); } -void CMainSignals::RegisterWithMempoolSignals(CTxMemPool& pool) { - pool.NotifyEntryRemoved.connect(std::bind(&CMainSignals::MempoolEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)); -} - -void CMainSignals::UnregisterWithMempoolSignals(CTxMemPool& pool) { - pool.NotifyEntryRemoved.disconnect_all_slots(); -} - CMainSignals& GetMainSignals() { return g_signals; @@ -137,14 +128,6 @@ void SyncWithValidationInterfaceQueue() { promise.get_future().wait(); } -void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason) { - if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) { - m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] { - m_internals->TransactionRemovedFromMempool(ptx); - }); - } -} - void CMainSignals::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) { // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which // the chain actually updates. One way to ensure this is for the caller to invoke this signal @@ -161,6 +144,12 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) { }); } +void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& ptx) { + m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] { + m_internals->TransactionRemovedFromMempool(ptx); + }); +} + void CMainSignals::BlockConnected(const std::shared_ptr &pblock, const CBlockIndex *pindex, const std::shared_ptr>& pvtxConflicted) { m_internals->m_schedulerClient.AddToProcessQueue([pblock, pindex, pvtxConflicted, this] { m_internals->BlockConnected(pblock, pindex, *pvtxConflicted); diff --git a/src/validationinterface.h b/src/validationinterface.h index 91ec2ee5b46c..7b784f680aa6 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -22,8 +22,6 @@ class CValidationInterface; class CValidationState; class uint256; class CScheduler; -class CTxMemPool; -enum class MemPoolRemovalReason; // These functions dispatch to one or all registered wallets @@ -132,8 +130,6 @@ class CMainSignals { private: std::unique_ptr m_internals; - void MempoolEntryRemoved(CTransactionRef tx, MemPoolRemovalReason reason); - friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); @@ -149,13 +145,9 @@ class CMainSignals { size_t CallbacksPending(); - /** Register with mempool to call TransactionRemovedFromMempool callbacks */ - void RegisterWithMempoolSignals(CTxMemPool& pool); - /** Unregister with mempool */ - void UnregisterWithMempoolSignals(CTxMemPool& pool); - void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef &ptxn); + void TransactionRemovedFromMempool(const CTransactionRef&); void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::shared_ptr> &); void BlockDisconnected(const std::shared_ptr &block, const uint256& blockHash, int nBlockHeight, int64_t blockTime); void SetBestChain(const CBlockLocator &); From 19c9383a1387c87163942390855fe19e0f9f4bfb Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 11 Nov 2019 10:34:31 -0500 Subject: [PATCH 02/10] [wallet] Notify conflicted transactions in TransactionRemovedFromMempool The only CValidationInterface client that cares about transactions that are removed from the mempool because of CONFLICT is the wallet. Start using the TransactionRemovedFromMempool method to notify about conflicted transactions instead of using the vtxConflicted vector in BlockConnected. --- src/txmempool.cpp | 6 +++++- src/validationinterface.h | 30 ++++++++++++++++++++++++++---- src/wallet/wallet.cpp | 3 --- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index bb800e9da976..9ba4651cd4e2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -437,7 +437,11 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) { CTransactionRef ptx = it->GetSharedTx(); NotifyEntryRemoved(ptx, reason); - if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) { + if (reason != MemPoolRemovalReason::BLOCK) { + // Notify clients that a transaction has been removed from the mempool + // for any reason except being included in a block. Clients interested + // in transactions included in blocks can subscribe to the BlockConnected + // notification. GetMainSignals().TransactionRemovedFromMempool(ptx); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 7b784f680aa6..78570dba3e86 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -90,10 +90,32 @@ class CValidationInterface { /** * Notifies listeners of a transaction leaving mempool. * - * This only fires for transactions which leave mempool because of expiry, - * size limiting, reorg (changes in lock times/coinbase/coinstake maturity), or - * replacement. This does not include any transactions which are included - * in BlockConnectedDisconnected either in block->vtx or in txnConflicted. + * This notification fires for transactions that are removed from the + * mempool for the following reasons: + * + * - EXPIRY (expired from mempool after -mempoolexpiry hours) + * - SIZELIMIT (removed in size limiting if the mempool exceeds -maxmempool megabytes) + * - REORG (removed during a reorg) + * - CONFLICT (removed because it conflicts with in-block transaction) + * - REPLACED (removed due to RBF replacement) -- not supported yet -- + * + * This does not fire for transactions that are removed from the mempool + * because they have been included in a block. Any client that is interested + * in transactions removed from the mempool for inclusion in a block can learn + * about those transactions from the BlockConnected notification. + * + * Transactions that are removed from the mempool because they conflict + * with a transaction in the new block will have + * TransactionRemovedFromMempool events fired *before* the BlockConnected + * event is fired. If multiple blocks are connected in one step, then the + * ordering could be: + * + * - TransactionRemovedFromMempool(tx1 from block A) + * - TransactionRemovedFromMempool(tx2 from block A) + * - TransactionRemovedFromMempool(tx1 from block B) + * - TransactionRemovedFromMempool(tx2 from block B) + * - BlockConnected(A) + * - BlockConnected(B) * * Called on a background thread. */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index beee18dc5e26..e2920bbf2f2b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1318,9 +1318,6 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const SyncTransaction(pblock->vtx[index], confirm); TransactionRemovedFromMempool(pblock->vtx[index]); } - for (const CTransactionRef& ptx : vtxConflicted) { - TransactionRemovedFromMempool(ptx); - } // Sapling: notify about the connected block // Get prev block tree anchor From 389680a42390a886eb6d683486ac50ba42e960c7 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Mar 2021 12:15:33 -0300 Subject: [PATCH 03/10] [validation interface] Remove vtxConflicted from BlockConnected The wallet now uses TransactionRemovedFromMempool to be notified about conflicted wallet, and no other clients use vtxConflicted. Adaptation of btc@cdb893443cc16edf974f099b8485e04b3db1b1d7 --- src/test/librust/sapling_rpc_wallet_tests.cpp | 3 +-- src/test/librust/sapling_wallet_tests.cpp | 3 +-- src/validation.cpp | 2 +- src/validationinterface.cpp | 10 +++++----- src/validationinterface.h | 4 ++-- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 2 +- src/zmq/zmqnotificationinterface.cpp | 2 +- src/zmq/zmqnotificationinterface.h | 2 +- 9 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index c3a86997c2e2..e8c74b24e923 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -434,8 +434,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) chainActive.SetTip(&fakeIndex); BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(1, chainActive.Height()); - std::vector vtxConflicted; - pwalletMain->BlockConnected(std::make_shared(block), mi->second, vtxConflicted); + pwalletMain->BlockConnected(std::make_shared(block), mi->second); BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed"); // Context that shieldsendmany requires diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index af0cc84688ae..c842f87ee07f 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -351,8 +351,7 @@ BOOST_AUTO_TEST_CASE(SaplingNullifierIsSpent) { BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(0, chainActive.Height()); - std::vector vtxConflicted; - wallet.BlockConnected(std::make_shared(block), mi->second, vtxConflicted); + wallet.BlockConnected(std::make_shared(block), mi->second); // Verify note has been spent BOOST_CHECK(wallet.GetSaplingScriptPubKeyMan()->IsSaplingSpent(nullifier)); diff --git a/src/validation.cpp b/src/validation.cpp index be53443ebdb4..bfc820e68506 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2378,7 +2378,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { assert(trace.pblock && trace.pindex); - GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); + GetMainSignals().BlockConnected(trace.pblock, trace.pindex); } } while (!chainActive.Tip() || (starting_tip && CBlockIndexWorkComparator()(chainActive.Tip(), starting_tip))); if (!blocks_connected) return true; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 85cf3d83216a..02f4e1a994ad 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -34,7 +34,7 @@ struct MainSignalsInstance { * 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; + boost::signals2::signal &, const CBlockIndex *pindex)> BlockConnected; /** Notifies listeners of a block being disconnected */ boost::signals2::signal &, const uint256& blockHash, int nBlockHeight, int64_t blockTime)> BlockDisconnected; /** Notifies listeners of a transaction removal from the mempool */ @@ -88,7 +88,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn]; 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, std::placeholders::_3)); + conns.BlockConnected = g_signals.m_internals->BlockConnected.connect(std::bind(&CValidationInterface::BlockConnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4)); conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1)); conns.SetBestChain = g_signals.m_internals->SetBestChain.connect(std::bind(&CValidationInterface::SetBestChain, pwalletIn, std::placeholders::_1)); @@ -150,9 +150,9 @@ void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& ptx) { }); } -void CMainSignals::BlockConnected(const std::shared_ptr &pblock, const CBlockIndex *pindex, const std::shared_ptr>& pvtxConflicted) { - m_internals->m_schedulerClient.AddToProcessQueue([pblock, pindex, pvtxConflicted, this] { - m_internals->BlockConnected(pblock, pindex, *pvtxConflicted); +void CMainSignals::BlockConnected(const std::shared_ptr &pblock, const CBlockIndex *pindex) { + m_internals->m_schedulerClient.AddToProcessQueue([pblock, pindex, this] { + m_internals->BlockConnected(pblock, pindex); }); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 78570dba3e86..5ad7aa5bae56 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -126,7 +126,7 @@ class CValidationInterface { * * Called on a background thread. */ - virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) {} + virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex) {} /** * Notifies listeners of a block being disconnected * @@ -170,7 +170,7 @@ class CMainSignals { void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef &ptxn); void TransactionRemovedFromMempool(const CTransactionRef&); - void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::shared_ptr> &); + void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr &block, const uint256& blockHash, int nBlockHeight, int64_t blockTime); void SetBestChain(const CBlockLocator &); void Broadcast(CConnman* connman); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e2920bbf2f2b..9b44ae5d9e2c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1304,7 +1304,7 @@ void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx) { } } -void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) +void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex) { { LOCK(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5791665acfef..a88f9b53c632 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -956,7 +956,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose = true); bool LoadToWallet(CWalletTx& wtxIn); void TransactionAddedToMempool(const CTransactionRef& tx) override; - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex) override; void BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) override; bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CWalletTx::Confirmation& confirm, bool fUpdate); void EraseFromWallet(const uint256& hash); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 30d522c3d92e..110e2df44ba4 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -164,7 +164,7 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& } } -void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) +void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) { for (const CTransactionRef& ptx : pblock->vtx) { // Do a normal notify for each transaction added in the block diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 5bb17cd6c504..7ca65bd94ac4 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -26,7 +26,7 @@ class CZMQNotificationInterface : public CValidationInterface // CValidationInterface void TransactionAddedToMempool(const CTransactionRef& tx) override; - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) override; + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; void BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; From e774bfd4269280972a48596d50cb48e62ca47306 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 9 Mar 2021 12:17:22 -0300 Subject: [PATCH 04/10] [validation] Remove conflictedTxs from PerBlockConnectTrace Since we don't add a vtxConflicted vector to BlockConnected the conflictedTxs member of PerBlockConnectTrace is no longer used. --- src/validation.cpp | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index bfc820e68506..e80507556e96 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2011,22 +2011,12 @@ static int64_t nTimePostConnect = 0; struct PerBlockConnectTrace { CBlockIndex* pindex = nullptr; std::shared_ptr pblock; - std::shared_ptr> conflictedTxs; - PerBlockConnectTrace() : conflictedTxs(std::make_shared>()) {} + PerBlockConnectTrace() {} }; /** * Used to track blocks whose transactions were applied to the UTXO state as a * part of a single ActivateBestChainStep call. * - * This class also tracks transactions that are removed from the mempool as - * conflicts (per block) and can be used to pass all those transactions - * through SyncTransaction. - * - * This class assumes (and asserts) that the conflicted transactions for a given - * block are added via mempool callbacks prior to the BlockConnected() associated - * with those transactions. If any transactions are marked conflicted, it is - * assumed that an associated block will always be added. - * * This class is single-use, once you call GetBlocksConnected() you have to throw * it away and make a new one. */ @@ -2061,16 +2051,12 @@ class ConnectTrace { // one waiting for the transactions from the next block. We pop // the last entry here to make sure the list we return is sane. assert(!blocksConnected.back().pindex); - assert(blocksConnected.back().conflictedTxs->empty()); blocksConnected.pop_back(); return blocksConnected; } void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { assert(!blocksConnected.back().pindex); - if (reason == MemPoolRemovalReason::CONFLICT) { - blocksConnected.back().conflictedTxs->emplace_back(std::move(txRemoved)); - } } }; From 1e453b70af8f21d1a89822d99fbc278e5e1aedb9 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 9 Mar 2021 12:18:54 -0300 Subject: [PATCH 05/10] [validation] Remove NotifyEntryRemoved callback from ConnectTrace ConnectTrace used to subscribe to the mempool's NotifyEntryRemoved callback to be notified of transactions removed for conflict. Since PerBlockConnectTrace no longer tracks conflicted transactions, ConnectTrace no longer requires these notifications --- src/validation.cpp | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e80507556e96..9a7a1bf04aa6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2024,16 +2024,11 @@ class ConnectTrace { private: std::vector blocksConnected; CTxMemPool &pool; - std::unique_ptr m_handler_notify_entry_removed; public: - ConnectTrace(CTxMemPool &_pool) : blocksConnected(1), pool(_pool) { - m_handler_notify_entry_removed = interfaces::MakeHandler(pool.NotifyEntryRemoved.connect(std::bind(&ConnectTrace::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2))); - } + ConnectTrace(CTxMemPool &_pool) : blocksConnected(1), pool(_pool) {} - ~ConnectTrace() { - m_handler_notify_entry_removed->disconnect(); - } + ~ConnectTrace() {} void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { assert(!blocksConnected.back().pindex); @@ -2054,10 +2049,6 @@ class ConnectTrace { blocksConnected.pop_back(); return blocksConnected; } - - void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { - assert(!blocksConnected.back().pindex); - } }; /** From 5cc619fe9d8d86ce6ae01d591ecfde3ada69dc98 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 9 Mar 2021 12:20:29 -0300 Subject: [PATCH 06/10] [validation] Remove pool member from ConnectTrace It's no longer used for anything. --- src/validation.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 9a7a1bf04aa6..628eaf19f885 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2023,12 +2023,9 @@ struct PerBlockConnectTrace { class ConnectTrace { private: std::vector blocksConnected; - CTxMemPool &pool; public: - ConnectTrace(CTxMemPool &_pool) : blocksConnected(1), pool(_pool) {} - - ~ConnectTrace() {} + ConnectTrace() : blocksConnected(1) {} void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { assert(!blocksConnected.back().pindex); @@ -2330,7 +2327,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb do { // We absolutely may not unlock cs_main until we've made forward progress // (with the exception of shutdown due to hardware issues, low disk space, etc). - ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked + ConnectTrace connectTrace; // Destructed before cs_main is unlocked if (pindexMostWork == nullptr) { pindexMostWork = FindMostWorkChain(); From 37c994461c040352f388aa4d3c82d353980d59a1 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 9 Mar 2021 12:22:57 -0300 Subject: [PATCH 07/10] [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks NotifyEntryAdded never had any subscribers so can be removed. Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are now no subscribers. The CValidationInterface TransactionAddedToMempool and TransactionRemovedFromMempool methods can now provide this functionality. There's no need for a special notifications framework for the mempool. --- src/txmempool.cpp | 5 +---- src/txmempool.h | 5 ----- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 9ba4651cd4e2..881adce77453 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -371,7 +371,6 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n) bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate) { - NotifyEntryAdded(entry.GetSharedTx()); // Add to memory pool without checking anything. // Used by AcceptToMemoryPool(), which DOES do all the appropriate checks. LOCK(cs); @@ -435,14 +434,12 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) { - CTransactionRef ptx = it->GetSharedTx(); - NotifyEntryRemoved(ptx, reason); if (reason != MemPoolRemovalReason::BLOCK) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. - GetMainSignals().TransactionRemovedFromMempool(ptx); + GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx()); } AssertLockHeld(cs); diff --git a/src/txmempool.h b/src/txmempool.h index 5212e7b538db..f4355cdbff53 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -24,8 +24,6 @@ #include "boost/multi_index/hashed_index.hpp" #include -#include - class CAutoFile; inline double AllowFreeThreshold() @@ -684,9 +682,6 @@ class CTxMemPool size_t DynamicMemoryUsage() const; - boost::signals2::signal NotifyEntryAdded; - boost::signals2::signal NotifyEntryRemoved; - private: /** UpdateForDescendants is used by UpdateTransactionsFromBlock to update * the descendants for a single transaction that has been added to the From ba9d84d9641ad0d796f8ba593804ce64443b4171 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Mar 2021 15:37:56 -0300 Subject: [PATCH 08/10] feature_notifications.py mimic upstream is_wallet_compiled() function to make future back ports easier. --- test/functional/feature_notifications.py | 39 +++++++++++++----------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index 109c2114c851..df2b42f901b8 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -34,6 +34,10 @@ def setup_network(self): "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, '%s'))]] super().setup_network() + # Hardcoded, always true, function for now to make future back ports easier. + def is_wallet_compiled(self): + return True + def run_test(self): self.log.info("test -blocknotify") block_count = 10 @@ -45,27 +49,28 @@ def run_test(self): # directory content should equal the generated blocks hashes assert_equal(sorted(blocks), sorted(os.listdir(self.blocknotify_dir))) - self.log.info("test -walletnotify") - # wait at most 10 seconds for expected number of files before reading the content - wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) + if self.is_wallet_compiled(): + self.log.info("test -walletnotify") + # wait at most 10 seconds for expected number of files before reading the content + wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) - # directory content should equal the generated transaction hashes - txids_rpc = list(map(lambda t: t['txid'], self.nodes[1].listtransactions("*", block_count))) - assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir))) - self.stop_node(1) - for tx_file in os.listdir(self.walletnotify_dir): - os.remove(os.path.join(self.walletnotify_dir, tx_file)) + # directory content should equal the generated transaction hashes + txids_rpc = list(map(lambda t: t['txid'], self.nodes[1].listtransactions("*", block_count))) + assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir))) + self.stop_node(1) + for tx_file in os.listdir(self.walletnotify_dir): + os.remove(os.path.join(self.walletnotify_dir, tx_file)) - self.log.info("test -walletnotify after rescan") - # restart node to rescan to force wallet notifications - self.start_node(1) - connect_nodes(self.nodes[0], 1) + self.log.info("test -walletnotify after rescan") + # restart node to rescan to force wallet notifications + self.start_node(1) + connect_nodes(self.nodes[0], 1) - wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) + wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) - # directory content should equal the generated transaction hashes - txids_rpc = list(map(lambda t: t['txid'], self.nodes[1].listtransactions("*", block_count))) - assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir))) + # directory content should equal the generated transaction hashes + txids_rpc = list(map(lambda t: t['txid'], self.nodes[1].listtransactions("*", block_count))) + assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir))) # Mine another 41 up-version blocks. -alertnotify should trigger on the 51st. self.log.info("test -alertnotify") From 3448bc890a7f41eb872f4bfb86f25446e11bfb54 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Wed, 10 Mar 2021 16:04:12 -0300 Subject: [PATCH 09/10] wallet: Minimal fix to restore conflicted transaction notifications --- src/txmempool.cpp | 2 +- src/validationinterface.cpp | 10 +++++----- src/validationinterface.h | 5 +++-- src/wallet/wallet.cpp | 34 ++++++++++++++++++++++++++++++++-- src/wallet/wallet.h | 2 +- 5 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 881adce77453..feb663acfb9a 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -439,7 +439,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. - GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx()); + GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason); } AssertLockHeld(cs); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 02f4e1a994ad..2b980368923a 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -38,7 +38,7 @@ struct MainSignalsInstance { /** Notifies listeners of a block being disconnected */ boost::signals2::signal &, const uint256& blockHash, int nBlockHeight, int64_t blockTime)> BlockDisconnected; /** Notifies listeners of a transaction removal from the mempool */ - boost::signals2::signal TransactionRemovedFromMempool; + boost::signals2::signal TransactionRemovedFromMempool; /** Notifies listeners of a new active block chain. */ boost::signals2::signal SetBestChain; /** Tells listeners to broadcast their data. */ @@ -90,7 +90,7 @@ 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)); conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4)); - conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1)); + conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1, std::placeholders::_2)); 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)); @@ -144,9 +144,9 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) { }); } -void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& ptx) { - m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] { - m_internals->TransactionRemovedFromMempool(ptx); +void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason) { + m_internals->m_schedulerClient.AddToProcessQueue([ptx, reason, this] { + m_internals->TransactionRemovedFromMempool(ptx, reason); }); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 5ad7aa5bae56..4dd8c54681d2 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -22,6 +22,7 @@ class CValidationInterface; class CValidationState; class uint256; class CScheduler; +enum class MemPoolRemovalReason; // These functions dispatch to one or all registered wallets @@ -119,7 +120,7 @@ class CValidationInterface { * * Called on a background thread. */ - virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {} + virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {} /** * Notifies listeners of a block being connected. * Provides a vector of transactions evicted from the mempool as a result. @@ -169,7 +170,7 @@ class CMainSignals { void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef &ptxn); - void TransactionRemovedFromMempool(const CTransactionRef&); + void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason); void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr &block, const uint256& blockHash, int nBlockHeight, int64_t blockTime); void SetBestChain(const CBlockLocator &); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9b44ae5d9e2c..913941063716 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1296,12 +1296,42 @@ void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) } } -void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx) { +void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) { LOCK(cs_wallet); auto it = mapWallet.find(ptx->GetHash()); if (it != mapWallet.end()) { it->second.fInMempool = false; } + // Handle transactions that were removed from the mempool because they + // conflict with transactions in a newly connected block. + if (reason == MemPoolRemovalReason::CONFLICT) { + // Call SyncNotifications, so external -walletnotify notifications will + // be triggered for these transactions. Set Status::UNCONFIRMED instead + // of Status::CONFLICTED for a few reasons: + // + // 1. The transactionRemovedFromMempool callback does not currently + // provide the conflicting block's hash and height, and for backwards + // compatibility reasons it may not be not safe to store conflicted + // wallet transactions with a null block hash. See + // https://github.com/bitcoin/bitcoin/pull/18600#discussion_r420195993. + // 2. For most of these transactions, the wallet's internal conflict + // detection in the blockConnected handler will subsequently call + // MarkConflicted and update them with CONFLICTED status anyway. This + // applies to any wallet transaction that has inputs spent in the + // block, or that has ancestors in the wallet with inputs spent by + // the block. + // 3. Longstanding behavior since the sync implementation in + // https://github.com/bitcoin/bitcoin/pull/9371 and the prior sync + // implementation before that was to mark these transactions + // unconfirmed rather than conflicted. + // + // Nothing described above should be seen as an unchangeable requirement + // when improving this code in the future. The wallet's heuristics for + // distinguishing between conflicted and unconfirmed transactions are + // imperfect, and could be improved in general, see + // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking + SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0}); + } } void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex) @@ -1316,7 +1346,7 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed_height, m_last_block_processed, index); SyncTransaction(pblock->vtx[index], confirm); - TransactionRemovedFromMempool(pblock->vtx[index]); + TransactionRemovedFromMempool(pblock->vtx[index], MemPoolRemovalReason::BLOCK); } // Sapling: notify about the connected block diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index a88f9b53c632..fe93df85f980 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -969,7 +969,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update); CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver& reserver, bool fUpdate = false, bool fromStartup = false); - void TransactionRemovedFromMempool(const CTransactionRef &ptx) override; + void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override; void ReacceptWalletTransactions(bool fFirstLoad = false); void ResendWalletTransactions(CConnman* connman) override; From f2734f02b35a856bb9ce7ff834cd5b4b9d905ff4 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Fri, 9 Apr 2021 14:05:54 +0200 Subject: [PATCH 10/10] [Tests] Fix policyestimator and validation_block tests --- src/test/policyestimator_tests.cpp | 2 +- src/test/validation_block_tests.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 16e7bb27b7be..9e9e57655985 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -12,7 +12,7 @@ #include -BOOST_FIXTURE_TEST_SUITE(policyestimator_tests, BasicTestingSetup) +BOOST_FIXTURE_TEST_SUITE(policyestimator_tests, TestingSetup) BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index dba3c04f1b63..94445b0a6b9f 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -32,7 +32,7 @@ struct TestSubscriber : public CValidationInterface { BOOST_CHECK_EQUAL(m_expected_tip, pindexNew->GetBlockHash()); } - void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex, const std::vector& txnConflicted) + void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex) { BOOST_CHECK_EQUAL(m_expected_tip, block->hashPrevBlock); BOOST_CHECK_EQUAL(m_expected_tip, pindex->pprev->GetBlockHash()); @@ -170,6 +170,7 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) MilliSleep(100); } + SyncWithValidationInterfaceQueue(); UnregisterValidationInterface(&sub); BOOST_CHECK_EQUAL(sub.m_expected_tip, WITH_LOCK(cs_main, return chainActive.Tip()->GetBlockHash()));