From 00f36ea5e1d85f0a7e0b94433e06b3ce953e202a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 19 Jan 2021 12:32:28 -0300 Subject: [PATCH 01/27] Use CScheduler for wallet flushing, remove ThreadFlushWalletDB --- src/init.cpp | 2 +- src/wallet/wallet.cpp | 9 +++++---- src/wallet/wallet.h | 7 +++---- src/wallet/walletdb.cpp | 38 +++++++++++++++++--------------------- src/wallet/walletdb.h | 2 +- 5 files changed, 27 insertions(+), 31 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index cd98e6f568d89..0c40d23905ef3 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1908,7 +1908,7 @@ bool AppInitMain() #ifdef ENABLE_WALLET if (pwalletMain) { - pwalletMain->postInitProcess(threadGroup); + pwalletMain->postInitProcess(scheduler); // StakeMiner thread disabled by default on regtest if (gArgs.GetBoolArg("-staking", !Params().IsRegTestNet() && DEFAULT_STAKING)) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3b80076a2bf71..5a99dc3980e23 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -15,6 +15,7 @@ #include "policy/policy.h" #include "sapling/key_io_sapling.h" #include "script/sign.h" +#include "scheduler.h" #include "spork.h" #include "util.h" #include "utilmoneystr.h" @@ -4128,16 +4129,16 @@ bool CWallet::InitLoadWallet() return true; } -std::atomic CWallet::fFlushThreadRunning(false); +std::atomic CWallet::fFlushScheduled(false); -void CWallet::postInitProcess(boost::thread_group& threadGroup) +void CWallet::postInitProcess(CScheduler& scheduler) { // Add wallet transactions that aren't already in a block to mapTransactions ReacceptWalletTransactions(/*fFirstLoad*/true); // Run a thread to flush wallet periodically - if (!CWallet::fFlushThreadRunning.exchange(true)) { - threadGroup.create_thread(ThreadFlushWalletDB); + if (!CWallet::fFlushScheduled.exchange(true)) { + scheduler.scheduleEvery(MaybeFlushWalletDB, 500); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 92c56abda32f8..4cd5f6fa8af50 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -44,8 +44,6 @@ #include #include -#include - extern CWallet* pwalletMain; /** @@ -94,6 +92,7 @@ class COutput; class CStakeableOutput; class CReserveKey; class CScript; +class CScheduler; class CWalletTx; class ScriptPubKeyMan; class SaplingScriptPubKeyMan; @@ -265,7 +264,7 @@ typedef std::map mapSaplingNoteData_t; class CWallet : public CCryptoKeyStore, public CValidationInterface { private: - static std::atomic fFlushThreadRunning; + static std::atomic fFlushScheduled; //! keeps track of whether Unlock has run a thorough check before bool fDecryptionThoroughlyChecked{false}; @@ -786,7 +785,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface * Wallet post-init setup * Gives the wallet a chance to register repetitive tasks and complete post-init tasks */ - void postInitProcess(boost::thread_group& threadGroup); + void postInitProcess(CScheduler& scheduler); /** * Address book entry changed. diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c871c3868b900..726a49fdce58e 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -891,35 +891,31 @@ DBErrors CWalletDB::ZapWalletTx(CWallet* pwallet, std::vector& vWtx) return DB_LOAD_OK; } -void ThreadFlushWalletDB() +void MaybeFlushWalletDB() { - // Make this thread recognisable as the wallet flushing thread - util::ThreadRename("pivx-wallet"); - - static bool fOneThread; - if (fOneThread) + static std::atomic fOneThread; + if (fOneThread.exchange(true)) { return; - fOneThread = true; - if (!gArgs.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) + } + if (!gArgs.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) { return; + } - unsigned int nLastSeen = CWalletDB::GetUpdateCounter(); - unsigned int nLastFlushed = CWalletDB::GetUpdateCounter(); - int64_t nLastWalletUpdate = GetTime(); - while (true) { - MilliSleep(500); + static unsigned int nLastSeen = CWalletDB::GetUpdateCounter(); + static unsigned int nLastFlushed = CWalletDB::GetUpdateCounter(); + static int64_t nLastWalletUpdate = GetTime(); - if (nLastSeen != CWalletDB::GetUpdateCounter()) { - nLastSeen = CWalletDB::GetUpdateCounter(); - nLastWalletUpdate = GetTime(); - } + if (nLastSeen != CWalletDB::GetUpdateCounter()) { + nLastSeen = CWalletDB::GetUpdateCounter(); + nLastWalletUpdate = GetTime(); + } - if (nLastFlushed != CWalletDB::GetUpdateCounter() && GetTime() - nLastWalletUpdate >= 2) { - if (CDB::PeriodicFlush(pwalletMain->GetDBHandle())) { - nLastFlushed = CWalletDB::GetUpdateCounter(); - } + if (nLastFlushed != CWalletDB::GetUpdateCounter() && GetTime() - nLastWalletUpdate >= 2) { + if (CDB::PeriodicFlush(pwalletMain->GetDBHandle())) { + nLastFlushed = CWalletDB::GetUpdateCounter(); } } + fOneThread = false; } void NotifyBacked(const CWallet& wallet, bool fSuccess, std::string strMessage) diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 73ad1b4842f53..4072b1a3bff77 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -230,6 +230,6 @@ void NotifyBacked(const CWallet& wallet, bool fSuccess, std::string strMessage); bool BackupWallet(const CWallet& wallet, const fs::path& strDest); bool AttemptBackupWallet(const CWallet& wallet, const fs::path& pathSrc, const fs::path& pathDest); -void ThreadFlushWalletDB(); +void MaybeFlushWalletDB(); #endif // BITCOIN_WALLETDB_H From 85e18f0c9c5cae076bdccee49928708c4f795ae4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 23 Jan 2017 09:27:59 -0500 Subject: [PATCH 02/27] Rename FlushWalletDB -> CompactWalletDB, add function description --- src/wallet/wallet.cpp | 2 +- src/wallet/walletdb.cpp | 2 +- src/wallet/walletdb.h | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5a99dc3980e23..64591a21cd212 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4138,7 +4138,7 @@ void CWallet::postInitProcess(CScheduler& scheduler) // Run a thread to flush wallet periodically if (!CWallet::fFlushScheduled.exchange(true)) { - scheduler.scheduleEvery(MaybeFlushWalletDB, 500); + scheduler.scheduleEvery(MaybeCompactWalletDB, 500); } } diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 726a49fdce58e..d59523a4c5ad1 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -891,7 +891,7 @@ DBErrors CWalletDB::ZapWalletTx(CWallet* pwallet, std::vector& vWtx) return DB_LOAD_OK; } -void MaybeFlushWalletDB() +void MaybeCompactWalletDB() { static std::atomic fOneThread; if (fOneThread.exchange(true)) { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 4072b1a3bff77..f7cb0b0593e20 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -230,6 +230,7 @@ void NotifyBacked(const CWallet& wallet, bool fSuccess, std::string strMessage); bool BackupWallet(const CWallet& wallet, const fs::path& strDest); bool AttemptBackupWallet(const CWallet& wallet, const fs::path& pathSrc, const fs::path& pathDest); -void MaybeFlushWalletDB(); +//! Compacts BDB state so that wallet.dat is self-contained (if there are changes) +void MaybeCompactWalletDB(); #endif // BITCOIN_WALLETDB_H From 466e97a81069dcd868688ca19a8d77b1023c1c30 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 19 Jan 2021 13:39:06 -0300 Subject: [PATCH 03/27] [Wallet] Simplify InMempool --- src/wallet/wallet.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 64591a21cd212..691c83ac26c2b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1780,10 +1780,7 @@ void CWallet::ReacceptWalletTransactions(bool fFirstLoad) bool CWalletTx::InMempool() const { LOCK(mempool.cs); - if (mempool.exists(GetHash())) { - return true; - } - return false; + return mempool.exists(GetHash()); } void CWalletTx::RelayWalletTransaction(CConnman* connman) From 15a21c2a4fffc3882b309d677d0640a1386b3e14 Mon Sep 17 00:00:00 2001 From: Chun Kuan Lee Date: Thu, 20 Sep 2018 15:33:07 +0800 Subject: [PATCH 04/27] tests: write the notification to different files to avoid race condition --- test/functional/feature_notifications.py | 58 ++++++++++++------------ 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index 708e42abc2f7a..2d63ed45842b4 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -18,17 +18,20 @@ def set_test_params(self): self.setup_clean_chain = True def setup_network(self): - self.alert_filename = os.path.join(self.options.tmpdir, "alert.txt") - self.block_filename = os.path.join(self.options.tmpdir, "blocks.txt") - self.tx_filename = os.path.join(self.options.tmpdir, "transactions.txt") + self.alertnotify_dir = os.path.join(self.options.tmpdir, "alertnotify") + self.blocknotify_dir = os.path.join(self.options.tmpdir, "blocknotify") + self.walletnotify_dir = os.path.join(self.options.tmpdir, "walletnotify") + os.mkdir(self.alertnotify_dir) + os.mkdir(self.blocknotify_dir) + os.mkdir(self.walletnotify_dir) # -alertnotify and -blocknotify on node0, walletnotify on node1 - self.extra_args = [["-blockversion=4", - "-alertnotify=echo %%s >> %s" % self.alert_filename, - "-blocknotify=echo %%s >> %s" % self.block_filename], + self.extra_args = [["-blockversion=8", + "-alertnotify=echo > {}".format(os.path.join(self.alertnotify_dir, '%s')), + "-blocknotify=echo > {}".format(os.path.join(self.blocknotify_dir, '%s'))], ["-blockversion=211", "-rescan", - "-walletnotify=echo %%s >> %s" % self.tx_filename]] + "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, '%s'))]] super().setup_network() def run_test(self): @@ -36,55 +39,50 @@ def run_test(self): block_count = 10 blocks = self.nodes[1].generate(block_count) - # wait at most 10 seconds for expected file size before reading the content - wait_until(lambda: os.path.isfile(self.block_filename) and os.stat(self.block_filename).st_size >= (block_count * 65), timeout=10) + # wait at most 10 seconds for expected number of files before reading the content + wait_until(lambda: len(os.listdir(self.blocknotify_dir)) == block_count, timeout=10) - # file content should equal the generated blocks hashes - with open(self.block_filename, 'r') as f: - assert_equal(sorted(blocks), sorted(f.read().splitlines())) + # 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 file size before reading the content - wait_until(lambda: os.path.isfile(self.tx_filename) and os.stat(self.tx_filename).st_size >= (block_count * 65), timeout=10) + # 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) - # file content should equal the generated transaction hashes + # directory content should equal the generated transaction hashes txids_rpc = list(map(lambda t: t['txid'], self.nodes[1].listtransactions("*", block_count))) - with open(self.tx_filename, 'r') as f: - assert_equal(sorted(txids_rpc), sorted(f.read().splitlines())) - os.remove(self.tx_filename) + assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir))) + 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.restart_node(1) connect_nodes(self.nodes[0], 1) - wait_until(lambda: os.path.isfile(self.tx_filename) and os.stat(self.tx_filename).st_size >= (block_count * 65), timeout=10) + wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) - # file content should equal the generated transaction hashes + # directory content should equal the generated transaction hashes txids_rpc = list(map(lambda t: t['txid'], self.nodes[1].listtransactions("*", block_count))) - with open(self.tx_filename, 'r') as f: - assert_equal(sorted(txids_rpc), sorted(f.read().splitlines())) + 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") self.nodes[1].generate(51) self.sync_all() - # Give pivxd 10 seconds to write the alert notification - wait_until(lambda: os.path.isfile(self.alert_filename) and os.path.getsize(self.alert_filename), timeout=10) + # Give bitcoind 10 seconds to write the alert notification + wait_until(lambda: len(os.listdir(self.alertnotify_dir)), timeout=10) - with open(self.alert_filename, 'r', encoding='utf8') as f: - alert_text = f.read() + for notify_file in os.listdir(self.alertnotify_dir): + os.remove(os.path.join(self.alertnotify_dir, notify_file)) # Mine more up-version blocks, should not get more alerts: self.nodes[1].generate(2) self.sync_all() - with open(self.alert_filename, 'r', encoding='utf8') as f: - alert_text2 = f.read() - self.log.info("-alertnotify should not continue notifying for more unknown version blocks") - assert_equal(alert_text, alert_text2) + assert_equal(len(os.listdir(self.alertnotify_dir)), 0) if __name__ == '__main__': NotificationsTest().main() From ef429af141557ac68ad7d23a15061fdf1c83c063 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 19 Jan 2021 15:49:19 -0300 Subject: [PATCH 05/27] tests: Stop node before removing the notification file --- test/functional/feature_notifications.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index 2d63ed45842b4..109c2114c851c 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -52,12 +52,13 @@ def run_test(self): # 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.restart_node(1) + self.start_node(1) connect_nodes(self.nodes[0], 1) wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) From 76c72c6b01dcbc7a8375718f54211241a2485a42 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 19 Jan 2021 18:24:00 -0300 Subject: [PATCH 06/27] remove external usage of mempool conflict tracking --- src/test/mempool_tests.cpp | 2 +- src/txmempool.cpp | 6 ++---- src/txmempool.h | 2 +- src/validation.cpp | 11 +++-------- 4 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 9ed326af740a2..a493681340f1e 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -415,7 +415,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) /* after tx6 is mined, tx7 should move up in the sort */ std::vector vtx; vtx.emplace_back(MakeTransactionRef(tx6)); - pool.removeForBlock(vtx, 1, nullptr, false); + pool.removeForBlock(vtx, 1); sortedOrder.erase(sortedOrder.begin()+1); // Ties are broken by hash diff --git a/src/txmempool.cpp b/src/txmempool.cpp index cc1797a63a79c..e43139682c70c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -368,13 +368,11 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n) nTransactionsUpdated += n; } - bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate) { // Add to memory pool without checking anything. // Used by AcceptToMemoryPool(), which DOES do all the appropriate checks. LOCK(cs); - indexed_transaction_set::iterator newit = mapTx.insert(entry).first; mapLinks.insert(make_pair(newit, TxLinks())); @@ -606,7 +604,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::vector& vtx, unsigned int nBlockHeight, - std::vector* conflicts, bool fCurrentEstimate) + bool fCurrentEstimate) { LOCK(cs); std::vector entries; @@ -623,7 +621,7 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne stage.insert(it); RemoveStaged(stage, true); } - removeConflicts(*tx, conflicts); + removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } // After the txs in the new block have been removed from the mempool, update policy estimates diff --git a/src/txmempool.h b/src/txmempool.h index 027aa3c0d91e7..38f0d672f816e 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -527,7 +527,7 @@ class CTxMemPool void removeWithAnchor(const uint256& invalidRoot); void removeConflicts(const CTransaction& tx, std::vector* removed = nullptr); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, - std::vector* conflicts = nullptr, bool fCurrentEstimate = true); + bool fCurrentEstimate = true); void clear(); void _clear(); // lock-free bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb); diff --git a/src/validation.cpp b/src/validation.cpp index b2cb3df078ec3..a4e8903e748e5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2023,11 +2023,10 @@ static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; /** - * Used to track conflicted transactions removed from mempool and transactions - * applied to the UTXO state as a part of a single ActivateBestChainStep call. + * Used to track blocks whose transactions were applied to the UTXO state as a + * part of a single ActivateBestChainStep call. */ struct ConnectTrace { - std::vector txConflicted; std::vector > > blocksConnected; }; @@ -2092,7 +2091,7 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const st LogPrint(BCLog::BENCH, " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); // Remove conflicting transactions from the mempool. - mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, &connectTrace.txConflicted, !IsInitialBlockDownload()); + mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, !IsInitialBlockDownload()); // Update chainActive & related variables. UpdateTip(pindexNew); // Update MN manager cache @@ -2305,10 +2304,6 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb fInitialDownload = IsInitialBlockDownload(); // throw all transactions though the signal-interface - for (const auto &tx : connectTrace.txConflicted) { - GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } - // ... and about transactions that got confirmed: for (const auto& pair : connectTrace.blocksConnected) { assert(pair.second); const CBlock& block = *(pair.second); From e7db9ffc444b4eb2a6baf1e1f32ab5a9a845a29f Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 19 Jan 2021 18:27:11 -0300 Subject: [PATCH 07/27] remove internal tracking of mempool conflicts for reporting to wallet --- src/test/mempool_tests.cpp | 42 ++++++++++++++++++++------------------ src/txmempool.cpp | 11 +++------- src/txmempool.h | 4 ++-- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index a493681340f1e..0ca5a76ae982e 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -53,18 +53,18 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) CTxMemPool testPool(CFeeRate(0)); - std::vector removed; // Nothing in pool, remove should do nothing: - testPool.removeRecursive(txParent, &removed); - BOOST_CHECK_EQUAL(removed.size(), 0); + unsigned int poolSize = testPool.size(); + testPool.removeRecursive(txParent); + BOOST_CHECK_EQUAL(testPool.size(), poolSize); // Just the parent: testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent)); - testPool.removeRecursive(txParent, &removed); - BOOST_CHECK_EQUAL(removed.size(), 1); - removed.clear(); - + poolSize = testPool.size(); + testPool.removeRecursive(txParent); + BOOST_CHECK_EQUAL(testPool.size(), poolSize - 1); + // Parent, children, grandchildren: testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent)); for (int i = 0; i < 3; i++) @@ -73,19 +73,21 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) testPool.addUnchecked(txGrandChild[i].GetHash(), entry.FromTx(txGrandChild[i])); } // Remove Child[0], GrandChild[0] should be removed: - testPool.removeRecursive(txChild[0], &removed); - BOOST_CHECK_EQUAL(removed.size(), 2); - removed.clear(); + poolSize = testPool.size(); + testPool.removeRecursive(txChild[0]); + BOOST_CHECK_EQUAL(testPool.size(), poolSize - 2); // ... make sure grandchild and child are gone: - testPool.removeRecursive(txGrandChild[0], &removed); - BOOST_CHECK_EQUAL(removed.size(), 0); - testPool.removeRecursive(txChild[0], &removed); - BOOST_CHECK_EQUAL(removed.size(), 0); + poolSize = testPool.size(); + testPool.removeRecursive(txGrandChild[0]); + BOOST_CHECK_EQUAL(testPool.size(), poolSize); + poolSize = testPool.size(); + testPool.removeRecursive(txChild[0]); + BOOST_CHECK_EQUAL(testPool.size(), poolSize); // Remove parent, all children/grandchildren should go: - testPool.removeRecursive(txParent, &removed); - BOOST_CHECK_EQUAL(removed.size(), 5); + poolSize = testPool.size(); + testPool.removeRecursive(txParent); + BOOST_CHECK_EQUAL(testPool.size(), poolSize - 5); BOOST_CHECK_EQUAL(testPool.size(), 0); - removed.clear(); // Add children and grandchildren, but NOT the parent (simulate the parent being in a block) for (int i = 0; i < 3; i++) @@ -95,10 +97,10 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) } // Now remove the parent, as might happen if a block-re-org occurs but the parent cannot be // put into the mempool (maybe because it is non-standard): - testPool.removeRecursive(txParent, &removed); - BOOST_CHECK_EQUAL(removed.size(), 6); + poolSize = testPool.size(); + testPool.removeRecursive(txParent); + BOOST_CHECK_EQUAL(testPool.size(), poolSize - 6); BOOST_CHECK_EQUAL(testPool.size(), 0); - removed.clear(); } template diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e43139682c70c..0093ed968bcae 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -481,7 +481,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants } } -void CTxMemPool::removeRecursive(const CTransaction& origTx, std::vector* removed) +void CTxMemPool::removeRecursive(const CTransaction& origTx) { // Remove transaction from memory pool { @@ -508,11 +508,6 @@ void CTxMemPool::removeRecursive(const CTransaction& origTx, std::vectoremplace_back(it->GetSharedTx()); - } - } RemoveStaged(setAllRemoves, false); } } @@ -572,7 +567,7 @@ void CTxMemPool::removeWithAnchor(const uint256& invalidRoot) } } -void CTxMemPool::removeConflicts(const CTransaction& tx, std::vector* removed) +void CTxMemPool::removeConflicts(const CTransaction& tx) { // Remove transactions which depend on inputs of tx, recursively std::list result; @@ -582,7 +577,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::vectorsecond; if (txConflict != tx) { - removeRecursive(txConflict, removed); + removeRecursive(txConflict); } } } diff --git a/src/txmempool.h b/src/txmempool.h index 38f0d672f816e..3b111bb04ba2d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -522,10 +522,10 @@ class CTxMemPool // then invoke the second version. bool addUnchecked(const uint256& hash, const CTxMemPoolEntry& entry, bool fCurrentEstimate = true); bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); - void removeRecursive(const CTransaction& tx, std::vector* removed = nullptr); + void removeRecursive(const CTransaction& tx); void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags); void removeWithAnchor(const uint256& invalidRoot); - void removeConflicts(const CTransaction& tx, std::vector* removed = nullptr); + void removeConflicts(const CTransaction& tx); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, bool fCurrentEstimate = true); void clear(); From a8605d2f28e53061ca49d32a0a2e1ee8db7a9e6d Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Mon, 3 Aug 2015 15:49:01 -0400 Subject: [PATCH 08/27] Clean up tx prioritization when conflict mined --- src/txmempool.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 0093ed968bcae..8d3e84237bed5 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -578,6 +578,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx) const CTransaction& txConflict = *it->second; if (txConflict != tx) { removeRecursive(txConflict); + ClearPrioritisation(txConflict.GetHash()); } } } @@ -588,7 +589,8 @@ void CTxMemPool::removeConflicts(const CTransaction& tx) if (it != mapSaplingNullifiers.end()) { const CTransaction& txConflict = *it->second; if (txConflict != tx) { - removeRecursive(txConflict, removed); + removeRecursive(txConflict); + ClearPrioritisation(txConflict.GetHash()); } } } From 7326acb8050ed30b5630535b0d757079ebf05153 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 19 Jan 2021 18:55:05 -0300 Subject: [PATCH 09/27] mempool: add notification for added/removed entries Add notification signals to make it possible to subscribe to mempool changes: - NotifyEntryAdded(CTransactionRef)> - NotifyEntryRemoved(CTransactionRef, MemPoolRemovalReason)> Also add a mempool removal reason enumeration, which is passed to the removed notification based on why the transaction was removed from the mempool. --- src/txmempool.cpp | 25 ++++++++++++++----------- src/txmempool.h | 26 +++++++++++++++++++++++--- src/validation.cpp | 2 +- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 8d3e84237bed5..2e9987e9c2284 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -370,6 +370,7 @@ 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); @@ -431,8 +432,9 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, return true; } -void CTxMemPool::removeUnchecked(txiter it) +void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) { + NotifyEntryRemoved(it->GetSharedTx(), reason); AssertLockHeld(cs); const CTransaction& tx = it->GetTx(); for (const CTxIn& txin : tx.vin) @@ -481,7 +483,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants } } -void CTxMemPool::removeRecursive(const CTransaction& origTx) +void CTxMemPool::removeRecursive(const CTransaction& origTx, MemPoolRemovalReason reason) { // Remove transaction from memory pool { @@ -508,7 +510,8 @@ void CTxMemPool::removeRecursive(const CTransaction& origTx) for (const txiter& it : txToRemove) { CalculateDescendants(it, setAllRemoves); } - RemoveStaged(setAllRemoves, false); + + RemoveStaged(setAllRemoves, false, reason); } } @@ -540,7 +543,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem for (txiter it : txToRemove) { CalculateDescendants(it, setAllRemoves); } - RemoveStaged(setAllRemoves, false); + RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG); } void CTxMemPool::removeWithAnchor(const uint256& invalidRoot) @@ -577,7 +580,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx) if (it != mapNextTx.end()) { const CTransaction& txConflict = *it->second; if (txConflict != tx) { - removeRecursive(txConflict); + removeRecursive(txConflict, MemPoolRemovalReason::CONFLICT); ClearPrioritisation(txConflict.GetHash()); } } @@ -589,7 +592,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx) if (it != mapSaplingNullifiers.end()) { const CTransaction& txConflict = *it->second; if (txConflict != tx) { - removeRecursive(txConflict); + removeRecursive(txConflict, MemPoolRemovalReason::CONFLICT); ClearPrioritisation(txConflict.GetHash()); } } @@ -616,7 +619,7 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne if (it != mapTx.end()) { setEntries stage; stage.insert(it); - RemoveStaged(stage, true); + RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK); } removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); @@ -1059,12 +1062,12 @@ size_t CTxMemPool::DynamicMemoryUsage() const memusage::DynamicUsage(mapSaplingNullifiers); } -void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants) +void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants, MemPoolRemovalReason reason) { AssertLockHeld(cs); UpdateForRemoveFromMempool(stage, updateDescendants); for (const txiter& it : stage) { - removeUnchecked(it); + removeUnchecked(it, reason); } } @@ -1081,7 +1084,7 @@ int CTxMemPool::Expire(int64_t time) for (const txiter& removeit : toremove) { CalculateDescendants(removeit, stage); } - RemoveStaged(stage, false); + RemoveStaged(stage, false, MemPoolRemovalReason::EXPIRY); return stage.size(); } @@ -1192,7 +1195,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends for (txiter it: stage) txn.push_back(it->GetTx()); } - RemoveStaged(stage, false); + RemoveStaged(stage, false, MemPoolRemovalReason::SIZELIMIT); if (pvNoSpendsRemaining) { for (const CTransaction& tx: txn) { for (const CTxIn& txin: tx.vin) { diff --git a/src/txmempool.h b/src/txmempool.h index 3b111bb04ba2d..6e29cd7965073 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -23,6 +23,8 @@ #include "boost/multi_index/ordered_index.hpp" #include "boost/multi_index/hashed_index.hpp" +#include + class CAutoFile; inline double AllowFreeThreshold() @@ -303,6 +305,19 @@ struct TxMempoolInfo int64_t nFeeDelta; }; +/** Reason why a transaction was removed from the mempool, + * this is passed to the notification signal. + */ +enum class MemPoolRemovalReason { + UNKNOWN = 0, //! Manually removed or unknown reason + EXPIRY, //! Expired from mempool + SIZELIMIT, //! Removed in size limiting + REORG, //! Removed for reorganization + BLOCK, //! Removed for block + CONFLICT, //! Removed for conflict with in-block transaction + REPLACED //! Removed for replacement +}; + /** * CTxMemPool stores valid-according-to-the-current-best-chain * transactions that may be included in the next block. @@ -522,12 +537,14 @@ class CTxMemPool // then invoke the second version. bool addUnchecked(const uint256& hash, const CTxMemPoolEntry& entry, bool fCurrentEstimate = true); bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); - void removeRecursive(const CTransaction& tx); + + void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN); void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags); void removeWithAnchor(const uint256& invalidRoot); void removeConflicts(const CTransaction& tx); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, bool fCurrentEstimate = true); + void clear(); void _clear(); // lock-free bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb); @@ -556,7 +573,7 @@ class CTxMemPool * Set updateDescendants to true when removing a tx that was in a block, so * that any in-mempool descendants have their ancestor state updated. */ - void RemoveStaged(setEntries &stage, bool updateDescendants); + void RemoveStaged(setEntries &stage, bool updateDescendants, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN); /** When adding transactions from a disconnected block back to the mempool, * new mempool entries may have children in the mempool (which is generally @@ -650,6 +667,9 @@ 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 @@ -690,7 +710,7 @@ class CTxMemPool * transactions in a chain before we've updated all the state for the * removal. */ - void removeUnchecked(txiter entry); + void removeUnchecked(txiter entry, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN); }; /** diff --git a/src/validation.cpp b/src/validation.cpp index a4e8903e748e5..b6bf5f501fdad 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1975,7 +1975,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara // ignore validation errors in resurrected transactions CValidationState stateDummy; if (tx->IsCoinBase() || tx->IsCoinStake() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, nullptr, true)) { - mempool.removeRecursive(*tx); + mempool.removeRecursive(*tx, MemPoolRemovalReason::REORG); } else if (mempool.exists(tx->GetHash())) { vHashUpdate.push_back(tx->GetHash()); } From 21be4e28809b65b3dbca982a5c401e4817810075 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 19 Jan 2021 19:02:41 -0300 Subject: [PATCH 10/27] Introduce MemPoolConflictRemovalTracker Analogue to ConnectTrace that tracks transactions that have been removed from the mempool due to conflicts and then passes them through SyncTransaction at the end of its scope. --- src/validation.cpp | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index b6bf5f501fdad..fdcf92b447500 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -174,6 +174,38 @@ std::set setDirtyBlockIndex; std::set setDirtyFileInfo; } // anon namespace +/* Use this class to start tracking transactions that are removed from the + * mempool and pass all those transactions through SyncTransaction when the + * object goes out of scope. This is currently only used to call SyncTransaction + * on conflicts removed from the mempool during block connection. Applied in + * ActivateBestChain around ActivateBestStep which in turn calls: + * ConnectTip->removeForBlock->removeConflicts + */ +class MemPoolConflictRemovalTracker +{ +private: + std::vector conflictedTxs; + CTxMemPool &pool; + +public: + MemPoolConflictRemovalTracker(CTxMemPool &_pool) : pool(_pool) { + pool.NotifyEntryRemoved.connect(std::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)); + } + + void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { + if (reason == MemPoolRemovalReason::CONFLICT) { + conflictedTxs.push_back(txRemoved); + } + } + + ~MemPoolConflictRemovalTracker() { + pool.NotifyEntryRemoved.disconnect(std::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)); + for (const auto& tx : conflictedTxs) { + GetMainSignals().SyncTransaction(*tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + } + conflictedTxs.clear(); + } +}; CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) { @@ -2287,7 +2319,14 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb MilliSleep(50); continue; } - + { // TODO: Temporarily ensure that mempool removals are notified before + // connected transactions. This shouldn't matter, but the abandoned + // state of transactions in our wallet is currently cleared when we + // receive another notification and there is a race condition where + // notification of a connected conflict might cause an outside process + // to abandon a transaction and then have it inadvertently cleared by + // the notification that the conflicted transaction was evicted. + MemPoolConflictRemovalTracker mrt(mempool); CBlockIndex *pindexOldTip = chainActive.Tip(); pindexMostWork = FindMostWorkChain(); @@ -2304,6 +2343,10 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb fInitialDownload = IsInitialBlockDownload(); // throw all transactions though the signal-interface + + } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified + + // Transactions in the connnected block are notified for (const auto& pair : connectTrace.blocksConnected) { assert(pair.second); const CBlock& block = *(pair.second); From 4cb5820ca5315b06d0524d09bdf292c2fd24b7c4 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 19 Jan 2021 19:07:23 -0300 Subject: [PATCH 11/27] Better document usage of SyncTransaction --- src/validationinterface.cpp | 7 ++++++- src/validationinterface.h | 4 +++- src/wallet/wallet.cpp | 12 ++++++++++-- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index addcc91f29fa3..377eb9820ea4e 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -27,7 +27,12 @@ struct MainSignalsInstance { boost::signals2::signal UpdatedBlockTip; /** A posInBlock value for SyncTransaction which indicates the transaction was conflicted, disconnected, or not in a block */ static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1; - /** Notifies listeners of updated transaction data (transaction, and optionally the block it is found in. */ + /** Notifies listeners of updated transaction data (transaction, and + * optionally the block it is found in). Called with block data when + * transaction is included in a connected block, and without block data when + * transaction was accepted to mempool, removed from mempool (only when + * removal was due to conflict from connected block), or appeared in a + * disconnected block.*/ boost::signals2::signal SyncTransaction; /** Notifies listeners of an updated transaction lock without new data. */ boost::signals2::signal NotifyTransactionLock; diff --git a/src/validationinterface.h b/src/validationinterface.h index 3519791c74041..3049944ff00ae 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -59,7 +59,9 @@ class CMainSignals { public: CMainSignals(); - /** A posInBlock value for SyncTransaction which indicates the transaction was conflicted, disconnected, or not in a block */ + /** A posInBlock value for SyncTransaction calls for transactions not + * included in connected blocks such as transactions removed from mempool, + * accepted to mempool or appearing in disconnected blocks.*/ static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1; void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 691c83ac26c2b..76a7142138962 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1048,9 +1048,17 @@ void CWallet::AddExternalNotesDataToTx(CWalletTx& wtx) const } /** - * Add a transaction to the wallet, or update it. - * pblock is optional, but should be provided if the transaction is known to be in a block. + * 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. * 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 + * abandoned is an indication that it is not safe to be considered abandoned. + * Abandoned state should probably be more carefully tracked via different + * posInBlock signals or by checking mempool presence when necessary. */ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const uint256& blockHash, int posInBlock, bool fUpdate) { From 1b396b87c7dca0cd9456450478a4125fe8d6d9f9 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 19 Jan 2021 19:50:41 -0300 Subject: [PATCH 12/27] Move `NotifyEntryRemoved` to use the general interfaces::MakeHandler --- src/validation.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index fdcf92b447500..53ad53488661a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -26,8 +26,8 @@ #include "guiinterface.h" #include "init.h" #include "invalid.h" +#include "interfaces/handler.h" #include "legacy/validation_zerocoin_legacy.h" -#include "libzerocoin/Denominations.h" #include "kernel.h" #include "masternode-payments.h" #include "masternode-sync.h" @@ -186,10 +186,11 @@ class MemPoolConflictRemovalTracker private: std::vector conflictedTxs; CTxMemPool &pool; + std::unique_ptr m_handler_notify_entry_removed; public: MemPoolConflictRemovalTracker(CTxMemPool &_pool) : pool(_pool) { - pool.NotifyEntryRemoved.connect(std::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)); + m_handler_notify_entry_removed = interfaces::MakeHandler(pool.NotifyEntryRemoved.connect(std::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2))); } void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { @@ -199,7 +200,7 @@ class MemPoolConflictRemovalTracker } ~MemPoolConflictRemovalTracker() { - pool.NotifyEntryRemoved.disconnect(std::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)); + m_handler_notify_entry_removed->disconnect(); for (const auto& tx : conflictedTxs) { GetMainSignals().SyncTransaction(*tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); } From e7c2789db5f4b4ba0fee37aad6d85fca7181c0cb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 20 Jan 2017 18:27:13 -0500 Subject: [PATCH 13/27] Include missing #include in zmqnotificationinterface.h --- src/zmq/zmqnotificationinterface.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index eb7f7e7cef587..49525b1d67d5c 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -8,6 +8,7 @@ #include "validationinterface.h" #include #include +#include class CBlockIndex; class CZMQAbstractNotifier; From 60329da672425042d1bba6d630031519dd6b678d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 19 Jan 2021 13:45:09 -0300 Subject: [PATCH 14/27] Add pblock to connectTrace at the end of ConnectTip, not start This makes ConnectTip responsible for the ConnectTrace instead of splitting the logic between ActivateBestChainStep and ConnectTip --- src/validation.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 53ad53488661a..97228d23417af 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2067,9 +2067,7 @@ struct ConnectTrace { * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. * - * The block is always added to connectTrace (either after loading from disk or by copying - * pblock) - if that is not intended, care must be taken to remove the last entry in - * blocksConnected in case of failure. + * The block is added to connectTrace if connection succeeds. */ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr& pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) { @@ -2080,15 +2078,16 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const st // Read block from disk. int64_t nTime1 = GetTimeMicros(); + std::shared_ptr pthisBlock; if (!pblock) { std::shared_ptr pblockNew = std::make_shared(); - connectTrace.blocksConnected.emplace_back(pindexNew, pblockNew); if (!ReadBlockFromDisk(*pblockNew, pindexNew)) return AbortNode(state, "Failed to read block"); + pthisBlock = pblockNew; } else { - connectTrace.blocksConnected.emplace_back(pindexNew, pblock); + pthisBlock = pblock; } - const CBlock& blockConnecting = *connectTrace.blocksConnected.back().second; + const CBlock& blockConnecting = *pthisBlock; // Apply the block atomically to the chain state. int64_t nTime2 = GetTimeMicros(); @@ -2136,6 +2135,8 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const st nTimeTotal += nTime6 - nTime1; LogPrint(BCLog::BENCH, " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001); + + connectTrace.blocksConnected.emplace_back(pindexNew, std::move(pthisBlock)); return true; } @@ -2259,8 +2260,6 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo state = CValidationState(); fInvalidFound = true; fContinue = false; - // If we didn't actually connect the block, don't notify listeners about it - connectTrace.blocksConnected.pop_back(); break; } else { // A system error occurred (disk space, database error, ...). From 27fb8970f4ddf6d79c039c1e65c43aa3160e4412 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 20 Jan 2021 00:17:23 -0300 Subject: [PATCH 15/27] Make ConnectTrace::blocksConnected private, hide behind accessors --- src/validation.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 97228d23417af..67adcf576cdb3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2060,7 +2060,17 @@ static int64_t nTimePostConnect = 0; * part of a single ActivateBestChainStep call. */ struct ConnectTrace { +private: std::vector > > blocksConnected; + +public: + void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { + blocksConnected.emplace_back(pindex, std::move(pblock)); + } + + std::vector > >& GetBlocksConnected() { + return blocksConnected; + } }; /** @@ -2136,7 +2146,7 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const st LogPrint(BCLog::BENCH, " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001); - connectTrace.blocksConnected.emplace_back(pindexNew, std::move(pthisBlock)); + connectTrace.BlockConnected(pindexNew, std::move(pthisBlock)); return true; } @@ -2347,7 +2357,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified // Transactions in the connnected block are notified - for (const auto& pair : connectTrace.blocksConnected) { + for (const auto& pair : connectTrace.GetBlocksConnected()) { assert(pair.second); const CBlock& block = *(pair.second); for (unsigned int i = 0; i < block.vtx.size(); i++) { From 50d3e0e103b123d9913e4a43676f81433f4ff253 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 20 Jan 2021 00:25:08 -0300 Subject: [PATCH 16/27] Handle conflicted transactions directly in ConnectTrace Adapted version of btc@d3167ba9bbefc2e5b7062f81c481547f21c5e44b --- src/validation.cpp | 84 +++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 45 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 67adcf576cdb3..bb48ff4ad36e6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -174,40 +174,6 @@ std::set setDirtyBlockIndex; std::set setDirtyFileInfo; } // anon namespace -/* Use this class to start tracking transactions that are removed from the - * mempool and pass all those transactions through SyncTransaction when the - * object goes out of scope. This is currently only used to call SyncTransaction - * on conflicts removed from the mempool during block connection. Applied in - * ActivateBestChain around ActivateBestStep which in turn calls: - * ConnectTip->removeForBlock->removeConflicts - */ -class MemPoolConflictRemovalTracker -{ -private: - std::vector conflictedTxs; - CTxMemPool &pool; - std::unique_ptr m_handler_notify_entry_removed; - -public: - MemPoolConflictRemovalTracker(CTxMemPool &_pool) : pool(_pool) { - m_handler_notify_entry_removed = interfaces::MakeHandler(pool.NotifyEntryRemoved.connect(std::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2))); - } - - void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { - if (reason == MemPoolRemovalReason::CONFLICT) { - conflictedTxs.push_back(txRemoved); - } - } - - ~MemPoolConflictRemovalTracker() { - m_handler_notify_entry_removed->disconnect(); - for (const auto& tx : conflictedTxs) { - GetMainSignals().SyncTransaction(*tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } - conflictedTxs.clear(); - } -}; - CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) { // Find the first block the caller has in the main chain @@ -2058,12 +2024,27 @@ static int64_t nTimePostConnect = 0; /** * 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 and can be used to pass all those transactions through + * SyncTransaction. */ -struct ConnectTrace { +class ConnectTrace { private: std::vector > > blocksConnected; + std::vector conflictedTxs; + CTxMemPool &pool; + std::unique_ptr m_handler_notify_entry_removed; public: + ConnectTrace(CTxMemPool &_pool) : pool(_pool) { + m_handler_notify_entry_removed = interfaces::MakeHandler(pool.NotifyEntryRemoved.connect(std::bind(&ConnectTrace::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2))); + } + + ~ConnectTrace() { + m_handler_notify_entry_removed->disconnect(); + } + void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { blocksConnected.emplace_back(pindex, std::move(pblock)); } @@ -2071,6 +2052,19 @@ struct ConnectTrace { std::vector > >& GetBlocksConnected() { return blocksConnected; } + + void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { + if (reason == MemPoolRemovalReason::CONFLICT) { + conflictedTxs.emplace_back(txRemoved); + } + } + + void CallSyncTransactionOnConflictedTransactions() { + for (const auto& tx : conflictedTxs) { + GetMainSignals().SyncTransaction(*tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + } + conflictedTxs.clear(); + } }; /** @@ -2321,7 +2315,6 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb boost::this_thread::interruption_point(); const CBlockIndex *pindexFork; - ConnectTrace connectTrace; bool fInitialDownload; while (true) { TRY_LOCK(cs_main, lockMain); @@ -2329,14 +2322,8 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb MilliSleep(50); continue; } - { // TODO: Temporarily ensure that mempool removals are notified before - // connected transactions. This shouldn't matter, but the abandoned - // state of transactions in our wallet is currently cleared when we - // receive another notification and there is a race condition where - // notification of a connected conflict might cause an outside process - // to abandon a transaction and then have it inadvertently cleared by - // the notification that the conflicted transaction was evicted. - MemPoolConflictRemovalTracker mrt(mempool); + ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked + CBlockIndex *pindexOldTip = chainActive.Tip(); pindexMostWork = FindMostWorkChain(); @@ -2353,8 +2340,15 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb fInitialDownload = IsInitialBlockDownload(); // throw all transactions though the signal-interface + connectTrace.CallSyncTransactionOnConflictedTransactions(); - } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified + // TODO: Temporarily ensure that mempool removals are notified before + // connected transactions. This shouldn't matter, but the abandoned + // state of transactions in our wallet is currently cleared when we + // receive another notification and there is a race condition where + // notification of a connected conflict might cause an outside process + // to abandon a transaction and then have it inadvertently cleared by + // the notification that the conflicted transaction was evicted. // Transactions in the connnected block are notified for (const auto& pair : connectTrace.GetBlocksConnected()) { From 6dcb6b0f9ba870b0ab3226a70fb1be772ac68cf3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Mar 2017 17:22:50 -0500 Subject: [PATCH 17/27] Keep conflictedTxs in ConnectTrace per-block --- src/validation.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index bb48ff4ad36e6..572f81eec7f6b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2026,18 +2026,18 @@ static int64_t nTimePostConnect = 0; * part of a single ActivateBestChainStep call. * * This class also tracks transactions that are removed from the mempool as - * conflicts and can be used to pass all those transactions through - * SyncTransaction. + * conflicts (per block) and can be used to pass all those transactions + * through SyncTransaction. */ class ConnectTrace { private: std::vector > > blocksConnected; - std::vector conflictedTxs; + std::vector > conflictedTxs; CTxMemPool &pool; std::unique_ptr m_handler_notify_entry_removed; public: - ConnectTrace(CTxMemPool &_pool) : pool(_pool) { + ConnectTrace(CTxMemPool &_pool) : conflictedTxs(1), pool(_pool) { m_handler_notify_entry_removed = interfaces::MakeHandler(pool.NotifyEntryRemoved.connect(std::bind(&ConnectTrace::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2))); } @@ -2047,6 +2047,7 @@ class ConnectTrace { void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { blocksConnected.emplace_back(pindex, std::move(pblock)); + conflictedTxs.emplace_back(); } std::vector > >& GetBlocksConnected() { @@ -2055,15 +2056,18 @@ class ConnectTrace { void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { if (reason == MemPoolRemovalReason::CONFLICT) { - conflictedTxs.emplace_back(txRemoved); + conflictedTxs.back().emplace_back(txRemoved); } } void CallSyncTransactionOnConflictedTransactions() { - for (const auto& tx : conflictedTxs) { - GetMainSignals().SyncTransaction(*tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + for (const auto& txRemovedForBlock : conflictedTxs) { + for (const auto& tx : txRemovedForBlock) { + GetMainSignals().SyncTransaction(*tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + } } conflictedTxs.clear(); + conflictedTxs.emplace_back(); } }; From 8704d9d5253bfa660561b9de5dbb58b3b5939e32 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 20 Jan 2021 00:39:47 -0300 Subject: [PATCH 18/27] Handle SyncTransaction in ActivateBestChain instead of ConnectTrace This makes a later change to move it all into one per-block callback simpler. --- src/validation.cpp | 74 ++++++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 572f81eec7f6b..9720c103f3643 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2021,6 +2021,12 @@ static int64_t nTimeFlush = 0; static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; +struct PerBlockConnectTrace { + CBlockIndex* pindex = nullptr; + std::shared_ptr pblock; + std::shared_ptr> conflictedTxs; + PerBlockConnectTrace() : conflictedTxs(std::make_shared>()) {} +}; /** * Used to track blocks whose transactions were applied to the UTXO state as a * part of a single ActivateBestChainStep call. @@ -2028,16 +2034,23 @@ static int64_t nTimePostConnect = 0; * 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. */ class ConnectTrace { private: - std::vector > > blocksConnected; - std::vector > conflictedTxs; + std::vector blocksConnected; CTxMemPool &pool; std::unique_ptr m_handler_notify_entry_removed; public: - ConnectTrace(CTxMemPool &_pool) : conflictedTxs(1), pool(_pool) { + 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))); } @@ -2046,29 +2059,32 @@ class ConnectTrace { } void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { - blocksConnected.emplace_back(pindex, std::move(pblock)); - conflictedTxs.emplace_back(); - } - - std::vector > >& GetBlocksConnected() { + assert(!blocksConnected.back().pindex); + assert(pindex); + assert(pblock); + blocksConnected.back().pindex = pindex; + blocksConnected.back().pblock = std::move(pblock); + blocksConnected.emplace_back(); + } + + std::vector& GetBlocksConnected() { + // We always keep one extra block at the end of our list because + // blocks are added after all the conflicted transactions have + // been filled in. Thus, the last entry should always be an empty + // 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) { - conflictedTxs.back().emplace_back(txRemoved); + blocksConnected.back().conflictedTxs->emplace_back(std::move(txRemoved)); } } - - void CallSyncTransactionOnConflictedTransactions() { - for (const auto& txRemovedForBlock : conflictedTxs) { - for (const auto& tx : txRemovedForBlock) { - GetMainSignals().SyncTransaction(*tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } - } - conflictedTxs.clear(); - conflictedTxs.emplace_back(); - } }; /** @@ -2343,9 +2359,6 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb pindexFork = chainActive.FindFork(pindexOldTip); fInitialDownload = IsInitialBlockDownload(); - // throw all transactions though the signal-interface - connectTrace.CallSyncTransactionOnConflictedTransactions(); - // TODO: Temporarily ensure that mempool removals are notified before // connected transactions. This shouldn't matter, but the abandoned // state of transactions in our wallet is currently cleared when we @@ -2354,12 +2367,21 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb // to abandon a transaction and then have it inadvertently cleared by // the notification that the conflicted transaction was evicted. + // throw all transactions though the signal-interface + auto blocksConnected = connectTrace.GetBlocksConnected(); + for (const PerBlockConnectTrace& trace : blocksConnected) { + assert(trace.conflictedTxs); + for (const auto& tx : *trace.conflictedTxs) { + GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + } + } + // Transactions in the connnected block are notified - for (const auto& pair : connectTrace.GetBlocksConnected()) { - assert(pair.second); - const CBlock& block = *(pair.second); + for (const PerBlockConnectTrace& trace : blocksConnected) { + assert(trace.pblock && trace.pindex); + const CBlock& block = *(trace.pblock); for (unsigned int i = 0; i < block.vtx.size(); i++) { - GetMainSignals().SyncTransaction(*block.vtx[i], pair.first, i); + GetMainSignals().SyncTransaction(*block.vtx[i], trace.pindex, i); } // Sapling: notify wallet about the connected blocks ordered From f5ac648a01d9f6310637d9e3df159d2c96674b4a Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 20 Jan 2021 00:51:26 -0300 Subject: [PATCH 19/27] [Refactor] Move Sapling ChainTip signal notification loop to use the latest connectTrace class structure --- src/validation.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 9720c103f3643..82e3039f550fb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2386,7 +2386,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb // Sapling: notify wallet about the connected blocks ordered // Get prev block tree anchor - CBlockIndex* pprev = pair.first->pprev; + CBlockIndex* pprev = trace.pindex->pprev; SaplingMerkleTree oldSaplingTree; bool isSaplingActive = (pprev) != nullptr && Params().GetConsensus().NetworkUpgradeActive(pprev->nHeight, @@ -2398,7 +2398,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb } // Sapling: Update cached incremental witnesses - GetMainSignals().ChainTip(pair.first, &block, oldSaplingTree); + GetMainSignals().ChainTip(trace.pindex, &block, oldSaplingTree); } break; From d3867a751d3de2d29b59f5d1a87bbb9ef62c70a1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 20 Jan 2021 17:10:33 -0300 Subject: [PATCH 20/27] An adaptation of Corallo's btc@461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected This simplifies fixing the wallet-returns-stale-info issue as we can now hold cs_wallet across an entire block instead of only per-tx (though we only actually do so in the next commit). This change also removes the NOT_IN_BLOCK constant in favor of only passing the CBlockIndex* parameter to SyncTransactions when a new block is being connected, instead of also when a block is being disconnected. This change adds a parameter to BlockConnectedDisconnected which lists the transactions which were removed from mempool due to confliction as a result of this operation. While its somewhat of a shame to make block-validation-logic generate a list of mempool changes to be included in its generated callbacks, fixing this isnt too hard. Further in this change-set, CValidationInterface starts listening to mempool directly, placing it in the middle and giving it a bit of logic to know how to route notifications from block-validation, mempool, etc (though not listening for conflicted-removals yet). --- src/validation.cpp | 36 +++++------------------ src/validationinterface.cpp | 38 ++++++++++++++++-------- src/validationinterface.h | 16 +++++----- src/wallet/wallet.cpp | 44 ++++++++++++++++++++++++++-- src/wallet/wallet.h | 7 ++++- src/zmq/zmqnotificationinterface.cpp | 22 +++++++++++++- src/zmq/zmqnotificationinterface.h | 4 ++- 7 files changed, 110 insertions(+), 57 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 82e3039f550fb..10ac81259de8f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -605,7 +605,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "mempool full"); } - GetMainSignals().SyncTransaction(tx, nullptr, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + GetMainSignals().TransactionAddedToMempool(_tx); return true; } @@ -1951,7 +1951,8 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara CBlockIndex* pindexDelete = chainActive.Tip(); assert(pindexDelete); // Read block from disk. - CBlock block; + std::shared_ptr pblock = std::make_shared(); + CBlock& block = *pblock; if (!ReadBlockFromDisk(block, pindexDelete)) return AbortNode(state, "Failed to read block"); // Apply the block atomically to the chain state. @@ -2003,9 +2004,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara UpdateTip(pindexDelete->pprev); // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: - for (const auto& tx : block.vtx) { - GetMainSignals().SyncTransaction(*tx, pindexDelete->pprev, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } + GetMainSignals().BlockDisconnected(pblock); if (chainparams.GetConsensus().NetworkUpgradeActive(pindexDelete->nHeight, Consensus::UPGRADE_V5_0)) { // Update Sapling cached incremental witnesses @@ -2359,30 +2358,9 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb pindexFork = chainActive.FindFork(pindexOldTip); fInitialDownload = IsInitialBlockDownload(); - // TODO: Temporarily ensure that mempool removals are notified before - // connected transactions. This shouldn't matter, but the abandoned - // state of transactions in our wallet is currently cleared when we - // receive another notification and there is a race condition where - // notification of a connected conflict might cause an outside process - // to abandon a transaction and then have it inadvertently cleared by - // the notification that the conflicted transaction was evicted. - - // throw all transactions though the signal-interface - auto blocksConnected = connectTrace.GetBlocksConnected(); - for (const PerBlockConnectTrace& trace : blocksConnected) { - assert(trace.conflictedTxs); - for (const auto& tx : *trace.conflictedTxs) { - GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } - } - - // Transactions in the connnected block are notified - for (const PerBlockConnectTrace& trace : blocksConnected) { + for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { assert(trace.pblock && trace.pindex); - const CBlock& block = *(trace.pblock); - for (unsigned int i = 0; i < block.vtx.size(); i++) { - GetMainSignals().SyncTransaction(*block.vtx[i], trace.pindex, i); - } + GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs); // Sapling: notify wallet about the connected blocks ordered // Get prev block tree anchor @@ -2398,7 +2376,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb } // Sapling: Update cached incremental witnesses - GetMainSignals().ChainTip(trace.pindex, &block, oldSaplingTree); + GetMainSignals().ChainTip(trace.pindex, trace.pblock.get(), oldSaplingTree); } break; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 377eb9820ea4e..3c84f96fa01fe 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -11,7 +11,9 @@ struct ValidationInterfaceConnections { boost::signals2::scoped_connection UpdatedBlockTip; - boost::signals2::scoped_connection SyncTransaction; + boost::signals2::scoped_connection TransactionAddedToMempool; + 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; @@ -25,15 +27,15 @@ struct MainSignalsInstance { // XX42 boost::signals2::signal EraseTransaction; /** Notifies listeners of updated block chain tip */ boost::signals2::signal UpdatedBlockTip; - /** A posInBlock value for SyncTransaction which indicates the transaction was conflicted, disconnected, or not in a block */ - static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1; - /** Notifies listeners of updated transaction data (transaction, and - * optionally the block it is found in). Called with block data when - * transaction is included in a connected block, and without block data when - * transaction was accepted to mempool, removed from mempool (only when - * removal was due to conflict from connected block), or appeared in a - * disconnected block.*/ - boost::signals2::signal SyncTransaction; + /** Notifies listeners of a transaction having been added to mempool. */ + boost::signals2::signal TransactionAddedToMempool; + /** + * 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; + /** Notifies listeners of a block being disconnected */ + boost::signals2::signal &)> 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). */ @@ -68,7 +70,9 @@ 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.SyncTransaction = g_signals.m_internals->SyncTransaction.connect(std::bind(&CValidationInterface::SyncTransaction, 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.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1)); conns.ChainTip = g_signals.m_internals->ChainTip.connect(std::bind(&CValidationInterface::ChainTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); 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)); @@ -97,8 +101,16 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockInd m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); } -void CMainSignals::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock) { - m_internals->SyncTransaction(tx, pindex, posInBlock); +void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptxn) { + m_internals->TransactionAddedToMempool(ptxn); +} + +void CMainSignals::BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) { + m_internals->BlockConnected(block, pindex, txnConflicted); +} + +void CMainSignals::BlockDisconnected(const std::shared_ptr &block) { + m_internals->BlockDisconnected(block); } void CMainSignals::NotifyTransactionLock(const CTransaction& tx) { diff --git a/src/validationinterface.h b/src/validationinterface.h index 3049944ff00ae..83e5ff8dd3968 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -9,13 +9,12 @@ #include "optional.h" #include "sapling/incrementalmerkletree.h" +#include "primitives/transaction.h" class CBlock; struct CBlockLocator; class CBlockIndex; class CConnman; -class CReserveScript; -class CTransaction; class CValidationInterface; class CValidationState; class uint256; @@ -33,7 +32,9 @@ class CValidationInterface { protected: /** Notifies listeners of updated block chain tip */ virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} - virtual void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) {} + 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 ChainTip(const CBlockIndex *pindex, const CBlock *pblock, Optional added) {} virtual void NotifyTransactionLock(const CTransaction &tx) {} /** Notifies listeners of the new active block chain on-disk. */ @@ -59,13 +60,10 @@ class CMainSignals { public: CMainSignals(); - /** A posInBlock value for SyncTransaction calls for transactions not - * included in connected blocks such as transactions removed from mempool, - * accepted to mempool or appearing in disconnected blocks.*/ - static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1; - void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); - void SyncTransaction(const CTransaction &, const CBlockIndex *pindex, int posInBlock); + 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); void NotifyTransactionLock(const CTransaction&); void UpdatedTransaction(const uint256 &); void SetBestChain(const CBlockLocator &); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 76a7142138962..e64c5c897841a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1240,15 +1240,53 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) } } -void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock) +void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock) { - LOCK(cs_wallet); - if (!AddToWalletIfInvolvingMe(tx, (pindex) ? pindex->GetBlockHash() : uint256(), posInBlock, true)) + const CTransaction& tx = *ptx; + if (!AddToWalletIfInvolvingMe(tx, + (pindexBlockConnected) ? pindexBlockConnected->GetBlockHash() : uint256(), + posInBlock, + true)) { return; // Not one of ours + } MarkAffectedTransactionsDirty(tx); } +void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) +{ + LOCK2(cs_main, cs_wallet); + SyncTransaction(ptx, NULL, -1); +} + +void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) +{ + // TODO: Tempoarily ensure that mempool removals are notified before + // connected transactions. This shouldn't matter, but the abandoned + // state of transactions in our wallet is currently cleared when we + // receive another notification and there is a race condition where + // notification of a connected conflict might cause an outside process + // to abandon a transaction and then have it inadvertantly cleared by + // the notification that the conflicted transaction was evicted. + + for (const CTransactionRef& ptx : vtxConflicted) { + LOCK2(cs_main, cs_wallet); + SyncTransaction(ptx, NULL, -1); + } + for (size_t i = 0; i < pblock->vtx.size(); i++) { + LOCK2(cs_main, cs_wallet); + SyncTransaction(pblock->vtx[i], pindex, i); + } +} + +void CWallet::BlockDisconnected(const std::shared_ptr& pblock) +{ + for (const CTransactionRef& ptx : pblock->vtx) { + LOCK2(cs_main, cs_wallet); + SyncTransaction(ptx, NULL, -1); + } +} + void CWallet::MarkAffectedTransactionsDirty(const CTransaction& tx) { // If a transaction changes 'conflicted' state, that changes the balance diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4cd5f6fa8af50..de9c760c249a2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -301,6 +301,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void SyncMetaData(std::pair::iterator, typename TxSpendMap::iterator> range); void ChainTipAdded(const CBlockIndex *pindex, const CBlock *pblock, SaplingMerkleTree saplingTree); + /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected */ + void SyncTransaction(const CTransactionRef& tx, const CBlockIndex *pindexBlockConnected, int posInBlock); + bool IsKeyUsed(const CPubKey& vchPubKey); struct OutputAvailabilityResult @@ -598,7 +601,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void MarkDirty(); bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose = true); bool LoadToWallet(const CWalletTx& wtxIn); - void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock); + void TransactionAddedToMempool(const CTransactionRef& tx) override; + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; + void BlockDisconnected(const std::shared_ptr& pblock) override; bool AddToWalletIfInvolvingMe(const CTransaction& tx, const uint256& blockHash, int posInBlock, bool fUpdate); void EraseFromWallet(const uint256& hash); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 9c1b798d2fd94..57b59208c2138 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -145,8 +145,12 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co } } -void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock) +void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx) { + // Used by BlockConnected and BlockDisconnected as well, because they're + // all the same external callback. + const CTransaction& tx = *ptx; + for (std::list::iterator i = notifiers.begin(); i!=notifiers.end(); ) { CZMQAbstractNotifier *notifier = *i; @@ -162,6 +166,22 @@ void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CB } } +void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) +{ + for (const CTransactionRef& ptx : pblock->vtx) { + // Do a normal notify for each transaction added in the block + TransactionAddedToMempool(ptx); + } +} + +void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr& pblock) +{ + for (const CTransactionRef& ptx : pblock->vtx) { + // Do a normal notify for each transaction removed in block disconnection + TransactionAddedToMempool(ptx); + } +} + void CZMQNotificationInterface::NotifyTransactionLock(const CTransaction &tx) { for (std::list::iterator i = notifiers.begin(); i!=notifiers.end(); ) diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 49525b1d67d5c..ce8b12189903e 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -25,7 +25,9 @@ class CZMQNotificationInterface : public CValidationInterface void Shutdown(); // CValidationInterface - void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock); + void TransactionAddedToMempool(const CTransactionRef& tx); + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted); + void BlockDisconnected(const std::shared_ptr& pblock); void NotifyTransactionLock(const CTransaction &tx); void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); From 1a450369e267609f58fb085938a7cead43c30c11 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 20 Jan 2021 17:12:05 -0300 Subject: [PATCH 21/27] fix compiler warning member functions not marked as 'override' --- src/net_processing.h | 4 ++-- src/wallet/wallet.h | 18 +++++++++--------- src/zmq/zmqnotificationinterface.h | 10 +++++----- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/net_processing.h b/src/net_processing.h index 9924cf704d00f..bc7abeb5ebdfb 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -41,8 +41,8 @@ class PeerLogicValidation : public CValidationInterface { PeerLogicValidation(CConnman* connmanIn); ~PeerLogicValidation() = default; - virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); - virtual void BlockChecked(const CBlock& block, const CValidationState& state); + void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; + void BlockChecked(const CBlock& block, const CValidationState& state) override; }; struct CNodeStateStats { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index de9c760c249a2..200f10ffb0095 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -554,7 +554,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface //////////// End Sapling ////////////// //! Adds a key to the store, and saves it to disk. - bool AddKeyPubKey(const CKey& key, const CPubKey& pubkey); + bool AddKeyPubKey(const CKey& key, const CPubKey& pubkey) override; //! Adds a key to the store, without saving it to disk (used by LoadWallet) bool LoadKey(const CKey& key, const CPubKey& pubkey) { return CCryptoKeyStore::AddKeyPubKey(key, pubkey); } //! Load metadata (used by LoadWallet) @@ -563,10 +563,10 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool LoadMinVersion(int nVersion); //! Adds an encrypted key to the store, and saves it to disk. - bool AddCryptedKey(const CPubKey& vchPubKey, const std::vector& vchCryptedSecret); + bool AddCryptedKey(const CPubKey& vchPubKey, const std::vector& vchCryptedSecret) override; //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) bool LoadCryptedKey(const CPubKey& vchPubKey, const std::vector& vchCryptedSecret); - bool AddCScript(const CScript& redeemScript); + bool AddCScript(const CScript& redeemScript) override; bool LoadCScript(const CScript& redeemScript); //! Adds a destination data tuple to the store, and saves it to disk @@ -577,8 +577,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value); //! Adds a watch-only address to the store, and saves it to disk. - bool AddWatchOnly(const CScript& dest); - bool RemoveWatchOnly(const CScript& dest); + bool AddWatchOnly(const CScript& dest) override; + bool RemoveWatchOnly(const CScript& dest) override; //! Adds a watch-only address to the store, without saving it to disk (used by LoadWallet) bool LoadWatchOnly(const CScript& dest); @@ -615,7 +615,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface int ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false, bool fromStartup = false); void ReacceptWalletTransactions(bool fFirstLoad = false); - void ResendWalletTransactions(CConnman* connman); + void ResendWalletTransactions(CConnman* connman) override; CAmount loopTxsBalance(std::functionmethod) const; CAmount GetAvailableBalance(bool fIncludeDelegated = true, bool fIncludeShielded = true) const; @@ -727,9 +727,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface //! Sapling merkle tree update void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, - Optional added); + Optional added) override; - void SetBestChain(const CBlockLocator& loc); + void SetBestChain(const CBlockLocator& loc) override; void SetBestChainInternal(CWalletDB& walletdb, const CBlockLocator& loc); // only public for testing purposes, must never be called directly in any other situation // Force balance recomputation if any transaction got conflicted void MarkAffectedTransactionsDirty(const CTransaction& tx); // only public for testing purposes, must never be called directly in any other situation @@ -753,7 +753,7 @@ 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); + bool UpdatedTransaction(const uint256& hashTx) override; unsigned int GetKeyPoolSize(); unsigned int GetStakingKeyPoolSize(); diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index ce8b12189903e..6a32cdef0010c 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -25,11 +25,11 @@ class CZMQNotificationInterface : public CValidationInterface void Shutdown(); // CValidationInterface - void TransactionAddedToMempool(const CTransactionRef& tx); - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted); - void BlockDisconnected(const std::shared_ptr& pblock); - void NotifyTransactionLock(const CTransaction &tx); - void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); + 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) override; + void NotifyTransactionLock(const CTransaction &tx) override; + void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; private: CZMQNotificationInterface(); From 10ccbbfdaa508e94cedebdd473faf42deac9f8ba Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 20 Jan 2021 17:13:24 -0300 Subject: [PATCH 22/27] Hold cs_wallet for whole block [dis]connection processing This simplifies fixing the wallet-returns-stale-info issue as we now hold cs_wallet across an entire block instead of only per-tx. --- src/wallet/wallet.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e64c5c897841a..977d243b55ce4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1261,6 +1261,7 @@ void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { + LOCK2(cs_main, cs_wallet); // TODO: Tempoarily ensure that mempool removals are notified before // connected transactions. This shouldn't matter, but the abandoned // state of transactions in our wallet is currently cleared when we @@ -1270,19 +1271,17 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const // the notification that the conflicted transaction was evicted. for (const CTransactionRef& ptx : vtxConflicted) { - LOCK2(cs_main, cs_wallet); SyncTransaction(ptx, NULL, -1); } for (size_t i = 0; i < pblock->vtx.size(); i++) { - LOCK2(cs_main, cs_wallet); SyncTransaction(pblock->vtx[i], pindex, i); } } void CWallet::BlockDisconnected(const std::shared_ptr& pblock) { + LOCK2(cs_main, cs_wallet); for (const CTransactionRef& ptx : pblock->vtx) { - LOCK2(cs_main, cs_wallet); SyncTransaction(ptx, NULL, -1); } } From d77244c3edd9d77fbfe9c257280306d6503ffd71 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 20 Jan 2021 17:18:32 -0300 Subject: [PATCH 23/27] Remove dead-code tracking of requests for blocks we generated --- src/miner.cpp | 3 --- src/validationinterface.cpp | 8 -------- src/validationinterface.h | 2 -- 3 files changed, 13 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 650541bb4fc90..89628ce6e9221 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -521,9 +521,6 @@ bool ProcessBlockFound(const std::shared_ptr& pblock, CWallet& wal if (reservekey) reservekey->KeepKey(); - // Inform about the new block - GetMainSignals().BlockFound(pblock->GetHash()); - // Process this block the same as if we had received it from another node CValidationState state; if (!ProcessNewBlock(state, nullptr, pblock, nullptr)) { diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 3c84f96fa01fe..f1f30d80e5a1c 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -19,7 +19,6 @@ struct ValidationInterfaceConnections { boost::signals2::scoped_connection SetBestChain; boost::signals2::scoped_connection Broadcast; boost::signals2::scoped_connection BlockChecked; - boost::signals2::scoped_connection BlockFound; boost::signals2::scoped_connection ChainTip; }; @@ -46,8 +45,6 @@ struct MainSignalsInstance { boost::signals2::signal Broadcast; /** Notifies listeners of a block validation result */ boost::signals2::signal BlockChecked; - /** Notifies listeners that a block has been successfully mined */ - boost::signals2::signal BlockFound; /** Notifies listeners of a change to the tip of the active block chain. */ boost::signals2::signal)> ChainTip; @@ -79,7 +76,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) 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)); - conns.BlockFound = g_signals.m_internals->BlockFound.connect(std::bind(&CValidationInterface::ResetRequestCount, pwalletIn, std::placeholders::_1)); } void UnregisterValidationInterface(CValidationInterface* pwalletIn) @@ -133,10 +129,6 @@ void CMainSignals::BlockChecked(const CBlock& block, const CValidationState& sta m_internals->BlockChecked(block, state); } -void CMainSignals::BlockFound(const uint256& hash) { - m_internals->BlockFound(hash); -} - void CMainSignals::ChainTip(const CBlockIndex* pindex, const CBlock* block, Optional tree) { m_internals->ChainTip(pindex, block, tree); } \ No newline at end of file diff --git a/src/validationinterface.h b/src/validationinterface.h index 83e5ff8dd3968..08f19b45a84db 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -43,7 +43,6 @@ class CValidationInterface { /** Tells listeners to broadcast their data. */ virtual void ResendWalletTransactions(CConnman* connman) {} virtual void BlockChecked(const CBlock&, const CValidationState&) {} - virtual void ResetRequestCount(const uint256 &hash) {}; friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); @@ -69,7 +68,6 @@ class CMainSignals { void SetBestChain(const CBlockLocator &); void Broadcast(CConnman* connman); void BlockChecked(const CBlock&, const CValidationState&); - void BlockFound(const uint256&); void ChainTip(const CBlockIndex *, const CBlock *, Optional); }; From b7990707430f880d2e4151e186bef4ee21125b4e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 20 Jan 2021 17:22:26 -0300 Subject: [PATCH 24/27] Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy --- src/wallet/wallet.cpp | 10 +++++----- src/wallet/wallet.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 977d243b55ce4..976cf1365efcd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1060,8 +1060,9 @@ void CWallet::AddExternalNotesDataToTx(CWalletTx& wtx) const * Abandoned state should probably be more carefully tracked via different * posInBlock signals or by checking mempool presence when necessary. */ -bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const uint256& blockHash, int posInBlock, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256& blockHash, int posInBlock, bool fUpdate) { + const CTransaction& tx = *ptx; { AssertLockHeld(cs_wallet); @@ -1242,15 +1243,14 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock) { - const CTransaction& tx = *ptx; - if (!AddToWalletIfInvolvingMe(tx, + if (!AddToWalletIfInvolvingMe(ptx, (pindexBlockConnected) ? pindexBlockConnected->GetBlockHash() : uint256(), posInBlock, true)) { return; // Not one of ours } - MarkAffectedTransactionsDirty(tx); + MarkAffectedTransactionsDirty(*ptx); } void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) @@ -1747,7 +1747,7 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b int posInBlock; for (posInBlock = 0; posInBlock < (int)block.vtx.size(); posInBlock++) { const auto& tx = block.vtx[posInBlock]; - if (AddToWalletIfInvolvingMe(*tx, pindex->GetBlockHash(), posInBlock, fUpdate)) { + if (AddToWalletIfInvolvingMe(tx, pindex->GetBlockHash(), posInBlock, fUpdate)) { myTxHashes.push_back(tx->GetHash()); ret++; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 200f10ffb0095..4e5788493ffec 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -604,7 +604,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; void BlockDisconnected(const std::shared_ptr& pblock) override; - bool AddToWalletIfInvolvingMe(const CTransaction& tx, const uint256& blockHash, int posInBlock, bool fUpdate); + bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const uint256& blockHash, int posInBlock, bool fUpdate); void EraseFromWallet(const uint256& hash); /** From bcdd3e9d7c73519c20ff56772c0ea01bbfa08fd0 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 20 Jan 2021 23:12:41 -0300 Subject: [PATCH 25/27] Move ChainTip sapling update witnesses and nullifiers to BlockConnected/BlockDisconnected. --- src/sapling/saplingscriptpubkeyman.cpp | 4 ++-- src/sapling/saplingscriptpubkeyman.h | 4 ++-- src/validation.cpp | 23 +------------------- src/validationinterface.cpp | 8 +++---- src/validationinterface.h | 4 ++-- src/wallet/wallet.cpp | 30 ++++++++++++++++++++++---- src/wallet/wallet.h | 2 +- src/zmq/zmqnotificationinterface.cpp | 2 +- src/zmq/zmqnotificationinterface.h | 2 +- 9 files changed, 40 insertions(+), 39 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index 196d212429318..562bb5d2f6123 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -300,11 +300,11 @@ void DecrementNoteWitnesses(NoteDataMap& noteDataMap, int indexHeight, int64_t n } } -void SaplingScriptPubKeyMan::DecrementNoteWitnesses(const CBlockIndex* pindex) +void SaplingScriptPubKeyMan::DecrementNoteWitnesses(int nChainHeight) { LOCK(wallet->cs_wallet); for (std::pair& wtxItem : wallet->mapWallet) { - ::DecrementNoteWitnesses(wtxItem.second.mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize); + ::DecrementNoteWitnesses(wtxItem.second.mapSaplingNoteData, nChainHeight, nWitnessCacheSize); } nWitnessCacheSize -= 1; nWitnessCacheNeedsUpdate = true; diff --git a/src/sapling/saplingscriptpubkeyman.h b/src/sapling/saplingscriptpubkeyman.h index dec4b7b3abaca..02e228b3204d7 100644 --- a/src/sapling/saplingscriptpubkeyman.h +++ b/src/sapling/saplingscriptpubkeyman.h @@ -163,9 +163,9 @@ class SaplingScriptPubKeyMan { const CBlock* pblock, SaplingMerkleTree& saplingTree); /** - * pindex is the old tip being disconnected. + * nChainHeight is the old tip height being disconnected. */ - void DecrementNoteWitnesses(const CBlockIndex* pindex); + void DecrementNoteWitnesses(int nChainHeight); /** * Update mapSaplingNullifiersToNotes diff --git a/src/validation.cpp b/src/validation.cpp index 10ac81259de8f..97a2bf7b9af7a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2004,12 +2004,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara UpdateTip(pindexDelete->pprev); // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: - GetMainSignals().BlockDisconnected(pblock); - - if (chainparams.GetConsensus().NetworkUpgradeActive(pindexDelete->nHeight, Consensus::UPGRADE_V5_0)) { - // Update Sapling cached incremental witnesses - GetMainSignals().ChainTip(pindexDelete, &block, nullopt); - } + GetMainSignals().BlockDisconnected(pblock, pindexDelete->nHeight); return true; } @@ -2361,22 +2356,6 @@ 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); - - // Sapling: notify wallet about the connected blocks ordered - // Get prev block tree anchor - CBlockIndex* pprev = trace.pindex->pprev; - SaplingMerkleTree oldSaplingTree; - bool isSaplingActive = (pprev) != nullptr && - Params().GetConsensus().NetworkUpgradeActive(pprev->nHeight, - Consensus::UPGRADE_V5_0); - if (isSaplingActive) { - assert(pcoinsTip->GetSaplingAnchorAt(pprev->hashFinalSaplingRoot, oldSaplingTree)); - } else { - assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldSaplingTree)); - } - - // Sapling: Update cached incremental witnesses - GetMainSignals().ChainTip(trace.pindex, trace.pblock.get(), oldSaplingTree); } break; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index f1f30d80e5a1c..3aac3c1df13be 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -34,7 +34,7 @@ struct MainSignalsInstance { */ boost::signals2::signal &, const CBlockIndex *pindex, const std::vector &)> BlockConnected; /** Notifies listeners of a block being disconnected */ - boost::signals2::signal &)> BlockDisconnected; + 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). */ @@ -69,7 +69,7 @@ void RegisterValidationInterface(CValidationInterface* 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.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1)); + conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.ChainTip = g_signals.m_internals->ChainTip.connect(std::bind(&CValidationInterface::ChainTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); 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)); @@ -105,8 +105,8 @@ void CMainSignals::BlockConnected(const std::shared_ptr &block, co m_internals->BlockConnected(block, pindex, txnConflicted); } -void CMainSignals::BlockDisconnected(const std::shared_ptr &block) { - m_internals->BlockDisconnected(block); +void CMainSignals::BlockDisconnected(const std::shared_ptr &block, int nBlockHeight) { + m_internals->BlockDisconnected(block, nBlockHeight); } void CMainSignals::NotifyTransactionLock(const CTransaction& tx) { diff --git a/src/validationinterface.h b/src/validationinterface.h index 08f19b45a84db..8bcfcff03c579 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -34,7 +34,7 @@ class CValidationInterface { 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 BlockDisconnected(const std::shared_ptr &block, int nBlockHeight) {} virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, Optional added) {} virtual void NotifyTransactionLock(const CTransaction &tx) {} /** Notifies listeners of the new active block chain on-disk. */ @@ -62,7 +62,7 @@ class CMainSignals { 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); - void BlockDisconnected(const std::shared_ptr &block); + void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight); void NotifyTransactionLock(const CTransaction&); void UpdatedTransaction(const uint256 &); void SetBestChain(const CBlockLocator &); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 976cf1365efcd..eaeaa3d10a2f0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1267,23 +1267,45 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const // state of transactions in our wallet is currently cleared when we // receive another notification and there is a race condition where // notification of a connected conflict might cause an outside process - // to abandon a transaction and then have it inadvertantly cleared by + // to abandon a transaction and then have it inadvertently cleared by // the notification that the conflicted transaction was evicted. for (const CTransactionRef& ptx : vtxConflicted) { - SyncTransaction(ptx, NULL, -1); + SyncTransaction(ptx, nullptr, -1); } for (size_t i = 0; i < pblock->vtx.size(); i++) { SyncTransaction(pblock->vtx[i], pindex, i); } + + // Sapling: notify about the connected block + // Get prev block tree anchor + CBlockIndex* pprev = pindex->pprev; + SaplingMerkleTree oldSaplingTree; + bool isSaplingActive = (pprev) != nullptr && + Params().GetConsensus().NetworkUpgradeActive(pprev->nHeight, + Consensus::UPGRADE_V5_0); + if (isSaplingActive) { + assert(pcoinsTip->GetSaplingAnchorAt(pprev->hashFinalSaplingRoot, oldSaplingTree)); + } else { + assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldSaplingTree)); + } + + // Sapling: Update cached incremental witnesses + ChainTipAdded(pindex, pblock.get(), oldSaplingTree); } -void CWallet::BlockDisconnected(const std::shared_ptr& pblock) +void CWallet::BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) { LOCK2(cs_main, cs_wallet); for (const CTransactionRef& ptx : pblock->vtx) { SyncTransaction(ptx, NULL, -1); } + + if (Params().GetConsensus().NetworkUpgradeActive(nBlockHeight, Consensus::UPGRADE_V5_0)) { + // Update Sapling cached incremental witnesses + m_sspk_man->DecrementNoteWitnesses(nBlockHeight); + m_sspk_man->UpdateSaplingNullifierNoteMapForBlock(pblock.get()); + } } void CWallet::MarkAffectedTransactionsDirty(const CTransaction& tx) @@ -4490,7 +4512,7 @@ void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex, const CBlock* pblock, SaplingMerkleTree& saplingTree) { m_sspk_man->IncrementNoteWitnesses(pindex, pblock, saplingTree); } -void CWallet::DecrementNoteWitnesses(const CBlockIndex* pindex) { m_sspk_man->DecrementNoteWitnesses(pindex); } +void CWallet::DecrementNoteWitnesses(const CBlockIndex* pindex) { m_sspk_man->DecrementNoteWitnesses(pindex->nHeight); } bool CWallet::AddSaplingZKey(const libzcash::SaplingExtendedSpendingKey &key) { return m_sspk_man->AddSaplingZKey(key); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4e5788493ffec..7332adeeda180 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -603,7 +603,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool LoadToWallet(const CWalletTx& wtxIn); void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; - void BlockDisconnected(const std::shared_ptr& pblock) override; + void BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) override; bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const uint256& blockHash, int posInBlock, bool fUpdate); void EraseFromWallet(const uint256& hash); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 57b59208c2138..fffa3bbb023ff 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -174,7 +174,7 @@ void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock) +void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) { for (const CTransactionRef& ptx : pblock->vtx) { // Do a normal notify for each transaction removed in block disconnection diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 6a32cdef0010c..136724e102c3b 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -27,7 +27,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 BlockDisconnected(const std::shared_ptr& pblock) 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; From 67e068c397d9a4f8c404825a1dd1239071f4bc3a Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 20 Jan 2021 23:18:28 -0300 Subject: [PATCH 26/27] Remove now unneeded ChainTip signal --- src/sapling/saplingscriptpubkeyman.h | 2 +- src/validationinterface.cpp | 11 +---------- src/validationinterface.h | 2 -- src/wallet/test/wallet_shielded_balances_tests.cpp | 4 +++- src/wallet/wallet.cpp | 12 ------------ src/wallet/wallet.h | 5 ----- 6 files changed, 5 insertions(+), 31 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.h b/src/sapling/saplingscriptpubkeyman.h index 02e228b3204d7..ae2f800c7951f 100644 --- a/src/sapling/saplingscriptpubkeyman.h +++ b/src/sapling/saplingscriptpubkeyman.h @@ -72,7 +72,7 @@ class SaplingNoteData * Block height corresponding to the most current witness. * * When we first create a SaplingNoteData in SaplingScriptPubKeyMan::FindMySaplingNotes, this is set to - * -1 as a placeholder. The next time CWallet::ChainTip is called, we can + * -1 as a placeholder. The next time CWallet::BlockConnected/CWallet::BlockDisconnected is called, we can * determine what height the witness cache for this note is valid for (even * if no witnesses were cached), and so can set the correct value in * SaplingScriptPubKeyMan::IncrementNoteWitnesses and SaplingScriptPubKeyMan::DecrementNoteWitnesses. diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 3aac3c1df13be..9fa5029fe023e 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -19,11 +19,10 @@ struct ValidationInterfaceConnections { boost::signals2::scoped_connection SetBestChain; boost::signals2::scoped_connection Broadcast; boost::signals2::scoped_connection BlockChecked; - boost::signals2::scoped_connection ChainTip; }; struct MainSignalsInstance { -// XX42 boost::signals2::signal EraseTransaction; + /** Notifies listeners of updated block chain tip */ boost::signals2::signal UpdatedBlockTip; /** Notifies listeners of a transaction having been added to mempool. */ @@ -46,9 +45,6 @@ struct MainSignalsInstance { /** Notifies listeners of a block validation result */ boost::signals2::signal BlockChecked; - /** Notifies listeners of a change to the tip of the active block chain. */ - boost::signals2::signal)> ChainTip; - std::unordered_map m_connMainSignals; }; @@ -70,7 +66,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.ChainTip = g_signals.m_internals->ChainTip.connect(std::bind(&CValidationInterface::ChainTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); 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)); @@ -128,7 +123,3 @@ void CMainSignals::Broadcast(CConnman* connman) { void CMainSignals::BlockChecked(const CBlock& block, const CValidationState& state) { m_internals->BlockChecked(block, state); } - -void CMainSignals::ChainTip(const CBlockIndex* pindex, const CBlock* block, Optional tree) { - m_internals->ChainTip(pindex, block, tree); -} \ No newline at end of file diff --git a/src/validationinterface.h b/src/validationinterface.h index 8bcfcff03c579..af980450a2503 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 ChainTip(const CBlockIndex *pindex, const CBlock *pblock, Optional added) {} virtual void NotifyTransactionLock(const CTransaction &tx) {} /** Notifies listeners of the new active block chain on-disk. */ virtual void SetBestChain(const CBlockLocator &locator) {} @@ -68,7 +67,6 @@ class CMainSignals { void SetBestChain(const CBlockLocator &); void Broadcast(CConnman* connman); void BlockChecked(const CBlock&, const CValidationState&); - void ChainTip(const CBlockIndex *, const CBlock *, Optional); }; CMainSignals& GetMainSignals(); diff --git a/src/wallet/test/wallet_shielded_balances_tests.cpp b/src/wallet/test/wallet_shielded_balances_tests.cpp index a6dff16969f97..2fc4bcdcc169f 100644 --- a/src/wallet/test/wallet_shielded_balances_tests.cpp +++ b/src/wallet/test/wallet_shielded_balances_tests.cpp @@ -348,7 +348,9 @@ BOOST_AUTO_TEST_CASE(GetShieldedAvailableCredit) // 2) Confirm the tx SaplingMerkleTree tree; FakeBlock fakeBlock = SimpleFakeMine(wtxUpdated, tree); - wallet.ChainTip(fakeBlock.pindex, &fakeBlock.block, tree); + // Simulate receiving a new block and updating the witnesses/nullifiers + wallet.IncrementNoteWitnesses(fakeBlock.pindex, &fakeBlock.block, tree); + wallet.GetSaplingScriptPubKeyMan()->UpdateSaplingNullifierNoteMapForBlock(&fakeBlock.block); wtxUpdated = wallet.mapWallet[wtxUpdated.GetHash()]; // 3) Now can spend one output and recalculate the shielded credit. diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index eaeaa3d10a2f0..a0832e61d5270 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -445,18 +445,6 @@ void CWallet::ChainTipAdded(const CBlockIndex *pindex, m_sspk_man->UpdateSaplingNullifierNoteMapForBlock(pblock); } -void CWallet::ChainTip(const CBlockIndex *pindex, - const CBlock *pblock, - Optional added) -{ - if (added) { - ChainTipAdded(pindex, pblock, added.get()); - } else { - DecrementNoteWitnesses(pindex); - m_sspk_man->UpdateSaplingNullifierNoteMapForBlock(pblock); - } -} - void CWallet::SetBestChain(const CBlockLocator& loc) { CWalletDB walletdb(*dbw); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7332adeeda180..b8453670bf5c3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -724,11 +724,6 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount GetCredit(const CWalletTx& tx, const isminefilter& filter) const; CAmount GetChange(const CTransaction& tx) const; - //! Sapling merkle tree update - void ChainTip(const CBlockIndex *pindex, - const CBlock *pblock, - Optional added) override; - void SetBestChain(const CBlockLocator& loc) override; void SetBestChainInternal(CWalletDB& walletdb, const CBlockLocator& loc); // only public for testing purposes, must never be called directly in any other situation // Force balance recomputation if any transaction got conflicted From 98d770f818f848985523d4787dab148747cb858c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 20 Jan 2017 15:36:13 -0500 Subject: [PATCH 27/27] CScheduler boost->std::function, use millisecs for times, not secs --- src/net.cpp | 2 +- src/scheduler.cpp | 12 ++++++------ src/scheduler.h | 10 +++++----- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 792ff0a05ed48..63eb1f31ea69c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2142,7 +2142,7 @@ bool CConnman::Start(CScheduler& scheduler, std::string& strNodeError, Options c threadMessageHandler = std::thread(&TraceThread >, "msghand", std::function(std::bind(&CConnman::ThreadMessageHandler, this))); // Dump network addresses - scheduler.scheduleEvery(std::bind(&CConnman::DumpData, this), DUMP_ADDRESSES_INTERVAL); + scheduler.scheduleEvery(std::bind(&CConnman::DumpData, this), DUMP_ADDRESSES_INTERVAL * 1000); return true; } diff --git a/src/scheduler.cpp b/src/scheduler.cpp index e757e9ffa8d91..bb671645b3604 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -93,20 +93,20 @@ void CScheduler::schedule(CScheduler::Function f, boost::chrono::system_clock::t newTaskScheduled.notify_one(); } -void CScheduler::scheduleFromNow(CScheduler::Function f, int64_t deltaSeconds) +void CScheduler::scheduleFromNow(CScheduler::Function f, int64_t deltaMilliSeconds) { - schedule(f, boost::chrono::system_clock::now() + boost::chrono::seconds(deltaSeconds)); + schedule(f, boost::chrono::system_clock::now() + boost::chrono::milliseconds(deltaMilliSeconds)); } -static void Repeat(CScheduler* s, CScheduler::Function f, int64_t deltaSeconds) +static void Repeat(CScheduler* s, CScheduler::Function f, int64_t deltaMilliSeconds) { f(); - s->scheduleFromNow(std::bind(&Repeat, s, f, deltaSeconds), deltaSeconds); + s->scheduleFromNow(std::bind(&Repeat, s, f, deltaMilliSeconds), deltaMilliSeconds); } -void CScheduler::scheduleEvery(CScheduler::Function f, int64_t deltaSeconds) +void CScheduler::scheduleEvery(CScheduler::Function f, int64_t deltaMilliSeconds) { - scheduleFromNow(std::bind(&Repeat, this, f, deltaSeconds), deltaSeconds); + scheduleFromNow(std::bind(&Repeat, this, f, deltaMilliSeconds), deltaMilliSeconds); } size_t CScheduler::getQueueInfo(boost::chrono::system_clock::time_point &first, diff --git a/src/scheduler.h b/src/scheduler.h index ced6098958f4c..9f1b703dd8f03 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -44,15 +44,15 @@ class CScheduler // Call func at/after time t void schedule(Function f, boost::chrono::system_clock::time_point t); - // Convenience method: call f once deltaSeconds from now - void scheduleFromNow(Function f, int64_t deltaSeconds); + // Convenience method: call f once deltaMilliSeconds from now + void scheduleFromNow(Function f, int64_t deltaMilliSeconds); // Another convenience method: call f approximately - // every deltaSeconds forever, starting deltaSeconds from now. + // every deltaMilliSeconds forever, starting deltaMilliSeconds from now. // To be more precise: every time f is finished, it - // is rescheduled to run deltaSeconds later. If you + // is rescheduled to run deltaMilliSeconds later. If you // need more accurate scheduling, don't use this method. - void scheduleEvery(Function f, int64_t deltaSeconds); + void scheduleEvery(Function f, int64_t deltaMilliSeconds); // To keep things as simple as possible, there is no unschedule.