From 2c8dff00cca70966733e837a6ae91bcda7fb1a70 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 20 Nov 2019 12:52:58 +0000 Subject: [PATCH 01/10] ThreadNotifyRecentlyAdded -> ThreadNotifyWallets --- src/init.cpp | 21 +++------------------ src/validationinterface.cpp | 25 +++++++++++++++++++++++++ src/validationinterface.h | 2 ++ 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index cc1cfeb5a..1cb86ff45 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -675,22 +675,6 @@ void ThreadImport(std::vector vImportFiles) } } -void ThreadNotifyRecentlyAdded() -{ - while (true) { - // Run the notifier on an integer second in the steady clock. - auto now = std::chrono::steady_clock::now().time_since_epoch(); - auto nextFire = std::chrono::duration_cast( - now + std::chrono::seconds(1)); - std::this_thread::sleep_until( - std::chrono::time_point(nextFire)); - - boost::this_thread::interruption_point(); - - mempool.NotifyRecentlyAdded(); - } -} - bool InitExperimentalMode() { fExperimentalMode = GetBoolArg("-experimentalfeatures", false); @@ -1867,8 +1851,9 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) #endif // Start the thread that notifies listeners of transactions that have been - // recently added to the mempool. - threadGroup.create_thread(boost::bind(&TraceThread, "txnotify", &ThreadNotifyRecentlyAdded)); + // recently added to the mempool, or have been added to or removed from the + // chain. + threadGroup.create_thread(boost::bind(&TraceThread, "txnotify", &ThreadNotifyWallets)); if (GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) StartTorControl(threadGroup, scheduler); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 54da44773..1c16988a2 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -5,6 +5,13 @@ #include "validationinterface.h" +#include "txmempool.h" + +#include + +#include +#include + static CMainSignals g_signals; CMainSignals& GetMainSignals() @@ -57,3 +64,21 @@ void UnregisterAllValidationInterfaces() { void SyncWithWallets(const CTransaction &tx, const CBlock *pblock) { g_signals.SyncTransaction(tx, pblock); } + +extern CTxMemPool mempool; + +void ThreadNotifyWallets() +{ + while (true) { + // Run the notifier on an integer second in the steady clock. + auto now = std::chrono::steady_clock::now().time_since_epoch(); + auto nextFire = std::chrono::duration_cast( + now + std::chrono::seconds(1)); + std::this_thread::sleep_until( + std::chrono::time_point(nextFire)); + + boost::this_thread::interruption_point(); + + mempool.NotifyRecentlyAdded(); + } +} diff --git a/src/validationinterface.h b/src/validationinterface.h index 1b5a426de..96307c373 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -76,4 +76,6 @@ struct CMainSignals { CMainSignals& GetMainSignals(); +void ThreadNotifyWallets(); + #endif // BITCOIN_VALIDATIONINTERFACE_H From a28916d273cf8cbd5daa19ba77d5f7d528d59b06 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 20 Nov 2019 14:16:58 +0000 Subject: [PATCH 02/10] Move mempool tx notifying logic out of CTxMemPool ThreadNotifyWallets now collects all notification-related state at once, and then executes wallet logic based on the collected state after dropping any locks it is holding. --- src/txmempool.cpp | 29 +++++++---------------------- src/txmempool.h | 3 ++- src/validationinterface.cpp | 30 +++++++++++++++++++++++++++++- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 9b60ef911..568d437fc 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -734,7 +734,7 @@ bool CTxMemPool::nullifierExists(const uint256& nullifier, ShieldedType type) co } } -void CTxMemPool::NotifyRecentlyAdded() +std::pair, uint64_t> CTxMemPool::DrainRecentlyAdded() { uint64_t recentlyAddedSequence; std::vector txs; @@ -747,28 +747,13 @@ void CTxMemPool::NotifyRecentlyAdded() mapRecentlyAddedTx.clear(); } - // A race condition can occur here between these SyncWithWallets calls, and - // the ones triggered by block logic (in ConnectTip and DisconnectTip). It - // is harmless because calling SyncWithWallets(_, NULL) does not alter the - // wallet transaction's block information. - for (auto tx : txs) { - try { - SyncWithWallets(tx, NULL); - } catch (const boost::thread_interrupted&) { - throw; - } catch (const std::exception& e) { - PrintExceptionContinue(&e, "CTxMemPool::NotifyRecentlyAdded()"); - } catch (...) { - PrintExceptionContinue(NULL, "CTxMemPool::NotifyRecentlyAdded()"); - } - } + return std::make_pair(txs, recentlyAddedSequence); +} - // Update the notified sequence number. We only need this in regtest mode, - // and should not lock on cs after calling SyncWithWallets otherwise. - if (Params().NetworkIDString() == "regtest") { - LOCK(cs); - nNotifiedSequence = recentlyAddedSequence; - } +void CTxMemPool::SetNotifiedSequence(uint64_t recentlyAddedSequence) { + assert(Params().NetworkIDString() == "regtest"); + LOCK(cs); + nNotifiedSequence = recentlyAddedSequence; } bool CTxMemPool::IsFullyNotified() { diff --git a/src/txmempool.h b/src/txmempool.h index 630b132a6..d66176436 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -224,7 +224,8 @@ public: bool nullifierExists(const uint256& nullifier, ShieldedType type) const; - void NotifyRecentlyAdded(); + std::pair, uint64_t> DrainRecentlyAdded(); + void SetNotifiedSequence(uint64_t recentlyAddedSequence); bool IsFullyNotified(); unsigned long size() diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 1c16988a2..f6b691cd6 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -5,6 +5,7 @@ #include "validationinterface.h" +#include "chainparams.h" #include "txmempool.h" #include @@ -79,6 +80,33 @@ void ThreadNotifyWallets() boost::this_thread::interruption_point(); - mempool.NotifyRecentlyAdded(); + // Collect all the state we require + + auto recentlyAdded = mempool.DrainRecentlyAdded(); + + // Execute wallet logic based on the collected state. We MUST NOT take + // the cs_main or mempool.cs locks again until after the next sleep. + + // A race condition can occur here between these SyncWithWallets calls, and + // the ones triggered by block logic (in ConnectTip and DisconnectTip). It + // is harmless because calling SyncWithWallets(_, NULL) does not alter the + // wallet transaction's block information. + for (auto tx : recentlyAdded.first) { + try { + SyncWithWallets(tx, NULL); + } catch (const boost::thread_interrupted&) { + throw; + } catch (const std::exception& e) { + PrintExceptionContinue(&e, "ThreadNotifyWallets()"); + } catch (...) { + PrintExceptionContinue(NULL, "ThreadNotifyWallets()"); + } + } + + // Update the notified sequence number. We only need this in regtest mode, + // and should not lock on cs between calls to DrainRecentlyAdded otherwise. + if (Params().NetworkIDString() == "regtest") { + mempool.SetNotifiedSequence(recentlyAdded.second); + } } } From a71a7341b9dd3603c95e5b8c0a86c357cf85f78b Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 21 Nov 2019 17:00:22 +0000 Subject: [PATCH 03/10] Merge tree and boolean fields in ChainTip API The trees were being unnecessarily fetched during DisconnectTip. --- src/main.cpp | 9 ++------- src/validationinterface.cpp | 4 ++-- src/validationinterface.h | 4 ++-- src/wallet/wallet.cpp | 6 ++---- src/wallet/wallet.h | 5 ++++- src/zcbenchmarks.cpp | 8 ++++---- 6 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 74b5cdcb1..3adf9b082 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3014,18 +3014,13 @@ bool static DisconnectTip(CValidationState &state, const CChainParams& chainpara // Update chainActive and related variables. UpdateTip(pindexDelete->pprev, chainparams); - // Get the current commitment tree - SproutMerkleTree newSproutTree; - SaplingMerkleTree newSaplingTree; - assert(pcoinsTip->GetSproutAnchorAt(pcoinsTip->GetBestAnchor(SPROUT), newSproutTree)); - assert(pcoinsTip->GetSaplingAnchorAt(pcoinsTip->GetBestAnchor(SAPLING), newSaplingTree)); // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: BOOST_FOREACH(const CTransaction &tx, block.vtx) { SyncWithWallets(tx, NULL); } // Update cached incremental witnesses - GetMainSignals().ChainTip(pindexDelete, &block, newSproutTree, newSaplingTree, false); + GetMainSignals().ChainTip(pindexDelete, &block, boost::none); return true; } @@ -3100,7 +3095,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, SyncWithWallets(tx, pblock); } // Update cached incremental witnesses - GetMainSignals().ChainTip(pindexNew, pblock, oldSproutTree, oldSaplingTree, true); + GetMainSignals().ChainTip(pindexNew, pblock, std::make_pair(oldSproutTree, oldSaplingTree)); EnforceNodeDeprecation(pindexNew->nHeight); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index f6b691cd6..07da61122 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -25,7 +25,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.SyncTransaction.connect(boost::bind(&CValidationInterface::SyncTransaction, pwalletIn, _1, _2)); g_signals.EraseTransaction.connect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); g_signals.UpdatedTransaction.connect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); - g_signals.ChainTip.connect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4, _5)); + g_signals.ChainTip.connect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3)); g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); @@ -40,7 +40,7 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); - g_signals.ChainTip.disconnect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4, _5)); + g_signals.ChainTip.disconnect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3)); g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.EraseTransaction.disconnect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); diff --git a/src/validationinterface.h b/src/validationinterface.h index 96307c373..1fbfe3837 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -36,7 +36,7 @@ protected: virtual void UpdatedBlockTip(const CBlockIndex *pindex) {} virtual void SyncTransaction(const CTransaction &tx, const CBlock *pblock) {} virtual void EraseFromWallet(const uint256 &hash) {} - virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, SaplingMerkleTree saplingTree, bool added) {} + virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, boost::optional> added) {} virtual void SetBestChain(const CBlockLocator &locator) {} virtual void UpdatedTransaction(const uint256 &hash) {} virtual void Inventory(const uint256 &hash) {} @@ -59,7 +59,7 @@ struct CMainSignals { /** Notifies listeners of an updated transaction without new data (for now: a coinbase potentially becoming visible). */ boost::signals2::signal UpdatedTransaction; /** Notifies listeners of a change to the tip of the active block chain. */ - boost::signals2::signal ChainTip; + boost::signals2::signal>)> ChainTip; /** Notifies listeners of a new active block chain. */ boost::signals2::signal SetBestChain; /** Notifies listeners about an inventory item being seen on the network. */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4d8a0e1f9..f09894db4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -574,12 +574,10 @@ void CWallet::ChainTipAdded(const CBlockIndex *pindex, void CWallet::ChainTip(const CBlockIndex *pindex, const CBlock *pblock, - SproutMerkleTree sproutTree, - SaplingMerkleTree saplingTree, - bool added) + boost::optional> added) { if (added) { - ChainTipAdded(pindex, pblock, sproutTree, saplingTree); + ChainTipAdded(pindex, pblock, added->first, added->second); // Prevent migration transactions from being created when node is syncing after launch, // and also when node wakes up from suspension/hibernation and incoming blocks are old. if (!IsInitialBlockDownload(Params()) && diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bfacdc494..78ac5632c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1195,7 +1195,10 @@ public: CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const; CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const; CAmount GetChange(const CTransaction& tx) const; - void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, SaplingMerkleTree saplingTree, bool added); + void ChainTip( + const CBlockIndex *pindex, + const CBlock *pblock, + boost::optional> added); void RunSaplingMigration(int blockHeight); void AddPendingSaplingMigrationTx(const CTransaction& tx); /** Saves witness caches and best block locator to disk. */ diff --git a/src/zcbenchmarks.cpp b/src/zcbenchmarks.cpp index e064dd586..391bf5847 100644 --- a/src/zcbenchmarks.cpp +++ b/src/zcbenchmarks.cpp @@ -350,7 +350,7 @@ double benchmark_increment_sprout_note_witnesses(size_t nTxs) index1.nHeight = 1; // Increment to get transactions witnessed - wallet.ChainTip(&index1, &block1, sproutTree, saplingTree, true); + wallet.ChainTip(&index1, &block1, std::make_pair(sproutTree, saplingTree)); // Second block CBlock block2; @@ -366,7 +366,7 @@ double benchmark_increment_sprout_note_witnesses(size_t nTxs) struct timeval tv_start; timer_start(tv_start); - wallet.ChainTip(&index2, &block2, sproutTree, saplingTree, true); + wallet.ChainTip(&index2, &block2, std::make_pair(sproutTree, saplingTree)); return timer_stop(tv_start); } @@ -412,7 +412,7 @@ double benchmark_increment_sapling_note_witnesses(size_t nTxs) index1.nHeight = 1; // Increment to get transactions witnessed - wallet.ChainTip(&index1, &block1, sproutTree, saplingTree, true); + wallet.ChainTip(&index1, &block1, std::make_pair(sproutTree, saplingTree)); // Second block CBlock block2; @@ -428,7 +428,7 @@ double benchmark_increment_sapling_note_witnesses(size_t nTxs) struct timeval tv_start; timer_start(tv_start); - wallet.ChainTip(&index2, &block2, sproutTree, saplingTree, true); + wallet.ChainTip(&index2, &block2, std::make_pair(sproutTree, saplingTree)); return timer_stop(tv_start); } From 9771506acd03180b1e11d1112d4dc20755340c96 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 21 Nov 2019 20:09:06 +0000 Subject: [PATCH 04/10] Move block-notifying logic into ThreadNotifyWallets --- src/main.cpp | 46 ++++++----- src/main.h | 2 + src/validationinterface.cpp | 148 +++++++++++++++++++++++++++++++++--- src/validationinterface.h | 2 - 4 files changed, 163 insertions(+), 35 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 3adf9b082..d2726623e 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3014,13 +3014,9 @@ bool static DisconnectTip(CValidationState &state, const CChainParams& chainpara // Update chainActive and related variables. UpdateTip(pindexDelete->pprev, chainparams); - // Let wallets know transactions went from 1-confirmed to - // 0-confirmed or conflicted: - BOOST_FOREACH(const CTransaction &tx, block.vtx) { - SyncWithWallets(tx, NULL); - } - // Update cached incremental witnesses - GetMainSignals().ChainTip(pindexDelete, &block, boost::none); + + // Updates to connected wallets are triggered by ThreadNotifyWallets + return true; } @@ -3030,6 +3026,9 @@ static int64_t nTimeFlush = 0; static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; +// Protected by cs_main +std::map> recentlyConflictedTxs; + /** * 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. @@ -3046,11 +3045,6 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, return AbortNode(state, "Failed to read block"); pblock = █ } - // Get the current commitment tree - SproutMerkleTree oldSproutTree; - SaplingMerkleTree oldSaplingTree; - assert(pcoinsTip->GetSproutAnchorAt(pcoinsTip->GetBestAnchor(SPROUT), oldSproutTree)); - assert(pcoinsTip->GetSaplingAnchorAt(pcoinsTip->GetBestAnchor(SAPLING), oldSaplingTree)); // Apply the block atomically to the chain state. int64_t nTime2 = GetTimeMicros(); nTimeReadFromDisk += nTime2 - nTime1; int64_t nTime3; @@ -3077,7 +3071,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4; LogPrint("bench", " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); // Remove conflicting transactions from the mempool. - list txConflicted; + std::list txConflicted; mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload(chainparams)); // Remove transactions that expire at new block height from mempool @@ -3085,17 +3079,10 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, // Update chainActive & related variables. UpdateTip(pindexNew, chainparams); - // Tell wallet about transactions that went from mempool - // to conflicted: - BOOST_FOREACH(const CTransaction &tx, txConflicted) { - SyncWithWallets(tx, NULL); - } - // ... and about transactions that got confirmed: - BOOST_FOREACH(const CTransaction &tx, pblock->vtx) { - SyncWithWallets(tx, pblock); - } - // Update cached incremental witnesses - GetMainSignals().ChainTip(pindexNew, pblock, std::make_pair(oldSproutTree, oldSaplingTree)); + + // Cache the conflicted transactions for subsequent notification. + // Updates to connected wallets are triggered by ThreadNotifyWallets + recentlyConflictedTxs.insert(std::make_pair(pindexNew, txConflicted)); EnforceNodeDeprecation(pindexNew->nHeight); @@ -3105,6 +3092,17 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, return true; } +std::map> DrainRecentlyConflicted() +{ + std::map> txs; + { + LOCK(cs_main); + txs.swap(recentlyConflictedTxs); + } + + return txs; +} + /** * Return the tip of the chain with the most work in it, that isn't * known to be invalid (it's however far from certain to be valid). diff --git a/src/main.h b/src/main.h index 5db32904e..9f64f6921 100644 --- a/src/main.h +++ b/src/main.h @@ -512,4 +512,6 @@ uint64_t CalculateCurrentUsage(); */ CMutableTransaction CreateNewContextualCMutableTransaction(const Consensus::Params& consensusParams, int nHeight); +std::map> DrainRecentlyConflicted(); + #endif // BITCOIN_MAIN_H diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 07da61122..15699665c 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -6,7 +6,10 @@ #include "validationinterface.h" #include "chainparams.h" +#include "init.h" +#include "main.h" #include "txmempool.h" +#include "ui_interface.h" #include @@ -66,10 +69,26 @@ void SyncWithWallets(const CTransaction &tx, const CBlock *pblock) { g_signals.SyncTransaction(tx, pblock); } -extern CTxMemPool mempool; +struct CachedBlockData { + CBlockIndex *pindex; + std::pair oldTrees; + std::list txConflicted; + + CachedBlockData( + CBlockIndex *pindex, + std::pair oldTrees, + std::list txConflicted): + pindex(pindex), oldTrees(oldTrees), txConflicted(txConflicted) {} +}; void ThreadNotifyWallets() { + CBlockIndex *pindexLastTip = nullptr; + { + LOCK(cs_main); + pindexLastTip = chainActive.Tip(); + } + while (true) { // Run the notifier on an integer second in the steady clock. auto now = std::chrono::steady_clock::now().time_since_epoch(); @@ -80,17 +99,128 @@ void ThreadNotifyWallets() boost::this_thread::interruption_point(); + auto chainParams = Params(); + + // // Collect all the state we require + // - auto recentlyAdded = mempool.DrainRecentlyAdded(); + // The common ancestor between the last chain tip we notified and the + // current chain tip. + const CBlockIndex *pindexFork; + // The stack of blocks we will notify as having been connected. + // Pushed in reverse, popped in order. + std::vector blockStack; + // Transactions that have been recently conflicted out of the mempool. + std::map> recentlyConflicted; + // Transactions that have been recently added to the mempool. + std::pair, uint64_t> recentlyAdded; + { + LOCK(cs_main); + + // Figure out the path from the last block we notified to the + // current chain tip. + CBlockIndex *pindex = chainActive.Tip(); + pindexFork = chainActive.FindFork(pindexLastTip); + + // Fetch recently-conflicted transactions. These will include any + // block that has been connected since the last cycle, but we only + // notify for the conflicts created by the current active chain. + recentlyConflicted = DrainRecentlyConflicted(); + + // Iterate backwards over the connected blocks we need to notify. + while (pindex && pindex != pindexFork) { + // Get the Sprout commitment tree as of the start of this block. + SproutMerkleTree oldSproutTree; + assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, oldSproutTree)); + + // Get the Sapling commitment tree as of the start of this block. + // We can get this from the `hashFinalSaplingRoot` of the last block + // However, this is only reliable if the last block was on or after + // the Sapling activation height. Otherwise, the last anchor was the + // empty root. + SaplingMerkleTree oldSaplingTree; + if (chainParams.GetConsensus().NetworkUpgradeActive( + pindex->pprev->nHeight, Consensus::UPGRADE_SAPLING)) { + assert(pcoinsTip->GetSaplingAnchorAt( + pindex->pprev->hashFinalSaplingRoot, oldSaplingTree)); + } else { + assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldSaplingTree)); + } + + blockStack.emplace_back( + pindex, + std::make_pair(oldSproutTree, oldSaplingTree), + recentlyConflicted.at(pindex)); + + pindex = pindex->pprev; + } + + recentlyAdded = mempool.DrainRecentlyAdded(); + } + + // // Execute wallet logic based on the collected state. We MUST NOT take // the cs_main or mempool.cs locks again until after the next sleep. + // - // A race condition can occur here between these SyncWithWallets calls, and - // the ones triggered by block logic (in ConnectTip and DisconnectTip). It - // is harmless because calling SyncWithWallets(_, NULL) does not alter the - // wallet transaction's block information. + // Notify block disconnects + while (pindexLastTip && pindexLastTip != pindexFork) { + // Read block from disk. + CBlock block; + if (!ReadBlockFromDisk(block, pindexLastTip, chainParams.GetConsensus())) { + LogPrintf("*** %s\n", "Failed to read block while notifying wallets of block disconnects"); + uiInterface.ThreadSafeMessageBox( + _("Error: A fatal internal error occurred, see debug.log for details"), + "", CClientUIInterface::MSG_ERROR); + StartShutdown(); + } + + // Let wallets know transactions went from 1-confirmed to + // 0-confirmed or conflicted: + for (const CTransaction &tx : block.vtx) { + SyncWithWallets(tx, NULL); + } + // Update cached incremental witnesses + GetMainSignals().ChainTip(pindexLastTip, &block, boost::none); + + // On to the next block! + pindexLastTip = pindexLastTip->pprev; + } + + // Notify block connections + while (!blockStack.empty()) { + auto blockData = blockStack.back(); + blockStack.pop_back(); + + // Read block from disk. + CBlock block; + if (!ReadBlockFromDisk(block, blockData.pindex, chainParams.GetConsensus())) { + LogPrintf("*** %s\n", "Failed to read block while notifying wallets of block connects"); + uiInterface.ThreadSafeMessageBox( + _("Error: A fatal internal error occurred, see debug.log for details"), + "", CClientUIInterface::MSG_ERROR); + StartShutdown(); + } + + // Tell wallet about transactions that went from mempool + // to conflicted: + for (const CTransaction &tx : blockData.txConflicted) { + SyncWithWallets(tx, NULL); + } + // ... and about transactions that got confirmed: + for (const CTransaction &tx : block.vtx) { + SyncWithWallets(tx, &block); + } + // Update cached incremental witnesses + GetMainSignals().ChainTip(blockData.pindex, &block, blockData.oldTrees); + + // This block is done! + pindexLastTip = blockData.pindex; + } + + // Notify transactions in the mempool for (auto tx : recentlyAdded.first) { try { SyncWithWallets(tx, NULL); @@ -103,9 +233,9 @@ void ThreadNotifyWallets() } } - // Update the notified sequence number. We only need this in regtest mode, - // and should not lock on cs between calls to DrainRecentlyAdded otherwise. - if (Params().NetworkIDString() == "regtest") { + // Update the notified sequence numbers. We only need this in regtest mode, + // and should not lock on cs or cs_main here otherwise. + if (chainParams.NetworkIDString() == "regtest") { mempool.SetNotifiedSequence(recentlyAdded.second); } } diff --git a/src/validationinterface.h b/src/validationinterface.h index 1fbfe3837..55a7f5f44 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -28,8 +28,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn); void UnregisterValidationInterface(CValidationInterface* pwalletIn); /** Unregister all wallets from core */ void UnregisterAllValidationInterfaces(); -/** Push an updated transaction to all registered wallets */ -void SyncWithWallets(const CTransaction& tx, const CBlock* pblock = NULL); class CValidationInterface { protected: From 03db5c8ca35db978b6fc0f8675aa5ceacdde6b7d Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 21 Nov 2019 20:46:00 +0000 Subject: [PATCH 05/10] Tie sync_blocks in RPC tests to notifier thread --- qa/rpc-tests/test_framework/util.py | 11 ++++++++++- src/main.cpp | 21 +++++++++++++++++++-- src/main.h | 4 +++- src/rpc/blockchain.cpp | 5 +++++ src/validationinterface.cpp | 5 +++-- 5 files changed, 40 insertions(+), 6 deletions(-) diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index 604bc9e28..11bfc08e1 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -46,7 +46,8 @@ def str_to_b64str(string): def sync_blocks(rpc_connections, wait=1): """ - Wait until everybody has the same block count + Wait until everybody has the same block count, and has notified + all internal listeners of them """ while True: counts = [ x.getblockcount() for x in rpc_connections ] @@ -54,6 +55,14 @@ def sync_blocks(rpc_connections, wait=1): break time.sleep(wait) + # Now that the block counts are in sync, wait for the internal + # notifications to finish + while True: + notified = [ x.getblockchaininfo()['fullyNotified'] for x in rpc_connections ] + if notified == [ True ] * len(notified): + break + time.sleep(wait) + def sync_mempools(rpc_connections, wait=1): """ Wait until everybody has the same transactions in their memory diff --git a/src/main.cpp b/src/main.cpp index d2726623e..6493c83d3 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3028,6 +3028,8 @@ static int64_t nTimePostConnect = 0; // Protected by cs_main std::map> recentlyConflictedTxs; +uint64_t nRecentlyConflictedSequence = 0; +uint64_t nNotifiedSequence = 0; /** * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock @@ -3083,6 +3085,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, // Cache the conflicted transactions for subsequent notification. // Updates to connected wallets are triggered by ThreadNotifyWallets recentlyConflictedTxs.insert(std::make_pair(pindexNew, txConflicted)); + nRecentlyConflictedSequence += 1; EnforceNodeDeprecation(pindexNew->nHeight); @@ -3092,15 +3095,29 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, return true; } -std::map> DrainRecentlyConflicted() +std::pair>, uint64_t> DrainRecentlyConflicted() { + uint64_t recentlyConflictedSequence; std::map> txs; { LOCK(cs_main); + recentlyConflictedSequence = nRecentlyConflictedSequence; txs.swap(recentlyConflictedTxs); } - return txs; + return std::make_pair(txs, recentlyConflictedSequence); +} + +void SetChainNotifiedSequence(uint64_t recentlyConflictedSequence) { + assert(Params().NetworkIDString() == "regtest"); + LOCK(cs_main); + nNotifiedSequence = recentlyConflictedSequence; +} + +bool ChainIsFullyNotified() { + assert(Params().NetworkIDString() == "regtest"); + LOCK(cs_main); + return nRecentlyConflictedSequence == nNotifiedSequence; } /** diff --git a/src/main.h b/src/main.h index 9f64f6921..e5a680b54 100644 --- a/src/main.h +++ b/src/main.h @@ -512,6 +512,8 @@ uint64_t CalculateCurrentUsage(); */ CMutableTransaction CreateNewContextualCMutableTransaction(const Consensus::Params& consensusParams, int nHeight); -std::map> DrainRecentlyConflicted(); +std::pair>, uint64_t> DrainRecentlyConflicted(); +void SetChainNotifiedSequence(uint64_t recentlyConflictedSequence); +bool ChainIsFullyNotified(); #endif // BITCOIN_MAIN_H diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 595d61e2b..0f6c6c760 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1070,6 +1070,11 @@ UniValue getblockchaininfo(const UniValue& params, bool fHelp) obj.push_back(Pair("pruneheight", block->nHeight)); } + + if (Params().NetworkIDString() == "regtest") { + obj.push_back(Pair("fullyNotified", ChainIsFullyNotified())); + } + return obj; } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 15699665c..ab593634c 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -112,7 +112,7 @@ void ThreadNotifyWallets() // Pushed in reverse, popped in order. std::vector blockStack; // Transactions that have been recently conflicted out of the mempool. - std::map> recentlyConflicted; + std::pair>, uint64_t> recentlyConflicted; // Transactions that have been recently added to the mempool. std::pair, uint64_t> recentlyAdded; @@ -152,7 +152,7 @@ void ThreadNotifyWallets() blockStack.emplace_back( pindex, std::make_pair(oldSproutTree, oldSaplingTree), - recentlyConflicted.at(pindex)); + recentlyConflicted.first.at(pindex)); pindex = pindex->pprev; } @@ -236,6 +236,7 @@ void ThreadNotifyWallets() // Update the notified sequence numbers. We only need this in regtest mode, // and should not lock on cs or cs_main here otherwise. if (chainParams.NetworkIDString() == "regtest") { + SetChainNotifiedSequence(recentlyConflicted.second); mempool.SetNotifiedSequence(recentlyAdded.second); } } From 8af85a0d10e8e2ca1689128e234a02cba5c8a8a0 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 17 Dec 2019 11:11:05 -0600 Subject: [PATCH 06/10] Extend comment with reason for taking care with locks --- src/validationinterface.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index ab593634c..405359347 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -162,7 +162,9 @@ void ThreadNotifyWallets() // // Execute wallet logic based on the collected state. We MUST NOT take - // the cs_main or mempool.cs locks again until after the next sleep. + // the cs_main or mempool.cs locks again until after the next sleep; + // doing so introduces a locking side-channel between this code and the + // network message processing thread. // // Notify block disconnects From f7eaf921bcb5811aa9a5dac662835e2856bd2036 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 18 Dec 2019 14:34:39 -0600 Subject: [PATCH 07/10] test: Add sync_all points after block generation to RPC tests Previously we only required synchronization points where blocks were sent between nodes; now we need them between action and query operations on the same node, because wallet notification of mined blocks no longer occurs in real-time. --- qa/rpc-tests/mempool_resurrect_test.py | 3 +++ qa/rpc-tests/mergetoaddress_helper.py | 1 + qa/rpc-tests/paymentdisclosure.py | 1 + qa/rpc-tests/wallet.py | 1 + qa/rpc-tests/wallet_1941.py | 2 ++ qa/rpc-tests/wallet_anchorfork.py | 1 + qa/rpc-tests/wallet_shieldcoinbase.py | 1 + qa/rpc-tests/walletbackup.py | 1 + qa/rpc-tests/zcjoinsplit.py | 3 +++ 9 files changed, 14 insertions(+) diff --git a/qa/rpc-tests/mempool_resurrect_test.py b/qa/rpc-tests/mempool_resurrect_test.py index 2d67bbb9a..5a512f322 100755 --- a/qa/rpc-tests/mempool_resurrect_test.py +++ b/qa/rpc-tests/mempool_resurrect_test.py @@ -56,6 +56,7 @@ class MempoolCoinbaseTest(BitcoinTestFramework): spends2_id = [ self.nodes[0].sendrawtransaction(tx) for tx in spends2_raw ] blocks.extend(self.nodes[0].generate(1)) + self.sync_all() # mempool should be empty, all txns confirmed assert_equal(set(self.nodes[0].getrawmempool()), set()) @@ -76,6 +77,8 @@ class MempoolCoinbaseTest(BitcoinTestFramework): # Generate another block, they should all get mined self.nodes[0].generate(1) + self.sync_all() + # mempool should be empty, all txns confirmed assert_equal(set(self.nodes[0].getrawmempool()), set()) for txid in spends1_id+spends2_id: diff --git a/qa/rpc-tests/mergetoaddress_helper.py b/qa/rpc-tests/mergetoaddress_helper.py index d93454e7e..7231f24e4 100755 --- a/qa/rpc-tests/mergetoaddress_helper.py +++ b/qa/rpc-tests/mergetoaddress_helper.py @@ -62,6 +62,7 @@ class MergeToAddressHelper: do_not_shield_taddr = test.nodes[0].getnewaddress() test.nodes[0].generate(4) + test.sync_all() walletinfo = test.nodes[0].getwalletinfo() assert_equal(walletinfo['immature_balance'], 50) assert_equal(walletinfo['balance'], 0) diff --git a/qa/rpc-tests/paymentdisclosure.py b/qa/rpc-tests/paymentdisclosure.py index 308fbc0ca..ccb33eaac 100755 --- a/qa/rpc-tests/paymentdisclosure.py +++ b/qa/rpc-tests/paymentdisclosure.py @@ -37,6 +37,7 @@ class PaymentDisclosureTest (BitcoinTestFramework): print "Mining blocks..." self.nodes[0].generate(4) + self.sync_all() walletinfo = self.nodes[0].getwalletinfo() assert_equal(walletinfo['immature_balance'], 40) assert_equal(walletinfo['balance'], 0) diff --git a/qa/rpc-tests/wallet.py b/qa/rpc-tests/wallet.py index 8d6cab67a..8af271ce3 100755 --- a/qa/rpc-tests/wallet.py +++ b/qa/rpc-tests/wallet.py @@ -32,6 +32,7 @@ class WalletTest (BitcoinTestFramework): print "Mining blocks..." self.nodes[0].generate(4) + self.sync_all() walletinfo = self.nodes[0].getwalletinfo() assert_equal(walletinfo['immature_balance'], 40) diff --git a/qa/rpc-tests/wallet_1941.py b/qa/rpc-tests/wallet_1941.py index a41d672a7..688c5852a 100755 --- a/qa/rpc-tests/wallet_1941.py +++ b/qa/rpc-tests/wallet_1941.py @@ -48,6 +48,7 @@ class Wallet1941RegressionTest (BitcoinTestFramework): self.nodes[0].setmocktime(starttime) self.nodes[0].generate(101) + self.sync_all() mytaddr = get_coinbase_address(self.nodes[0]) myzaddr = self.nodes[0].z_getnewaddress('sprout') @@ -66,6 +67,7 @@ class Wallet1941RegressionTest (BitcoinTestFramework): self.nodes[0].generate(1) self.nodes[0].setmocktime(starttime + 9000) self.nodes[0].generate(1) + self.sync_all() # Confirm the balance on node 0. resp = self.nodes[0].z_getbalance(myzaddr) diff --git a/qa/rpc-tests/wallet_anchorfork.py b/qa/rpc-tests/wallet_anchorfork.py index 8f1ea5e9f..9333c54b9 100755 --- a/qa/rpc-tests/wallet_anchorfork.py +++ b/qa/rpc-tests/wallet_anchorfork.py @@ -29,6 +29,7 @@ class WalletAnchorForkTest (BitcoinTestFramework): def run_test (self): print "Mining blocks..." self.nodes[0].generate(4) + self.sync_all() walletinfo = self.nodes[0].getwalletinfo() assert_equal(walletinfo['immature_balance'], 40) diff --git a/qa/rpc-tests/wallet_shieldcoinbase.py b/qa/rpc-tests/wallet_shieldcoinbase.py index 212f09619..cd691e870 100755 --- a/qa/rpc-tests/wallet_shieldcoinbase.py +++ b/qa/rpc-tests/wallet_shieldcoinbase.py @@ -39,6 +39,7 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): self.nodes[0].generate(1) self.nodes[0].generate(4) + self.sync_all() walletinfo = self.nodes[0].getwalletinfo() assert_equal(walletinfo['immature_balance'], 50) assert_equal(walletinfo['balance'], 0) diff --git a/qa/rpc-tests/walletbackup.py b/qa/rpc-tests/walletbackup.py index 820308646..eb148f327 100755 --- a/qa/rpc-tests/walletbackup.py +++ b/qa/rpc-tests/walletbackup.py @@ -93,6 +93,7 @@ class WalletBackupTest(BitcoinTestFramework): # Must sync mempools before mining. sync_mempools(self.nodes) self.nodes[3].generate(1) + self.sync_all() # As above, this mirrors the original bash test. def start_three(self): diff --git a/qa/rpc-tests/zcjoinsplit.py b/qa/rpc-tests/zcjoinsplit.py index 6edb1d1f4..b52d5cd94 100755 --- a/qa/rpc-tests/zcjoinsplit.py +++ b/qa/rpc-tests/zcjoinsplit.py @@ -32,6 +32,7 @@ class JoinSplitTest(BitcoinTestFramework): shield_tx = self.nodes[0].signrawtransaction(joinsplit_result["rawtxn"]) self.nodes[0].sendrawtransaction(shield_tx["hex"]) self.nodes[0].generate(1) + self.sync_all() receive_result = self.nodes[0].zcrawreceive(zcsecretkey, joinsplit_result["encryptednote1"]) assert_equal(receive_result["exists"], True) @@ -41,6 +42,7 @@ class JoinSplitTest(BitcoinTestFramework): addrtest = self.nodes[0].getnewaddress() for xx in range(0,10): self.nodes[0].generate(1) + self.sync_all() for x in range(0,50): self.nodes[0].sendtoaddress(addrtest, 0.01); @@ -49,6 +51,7 @@ class JoinSplitTest(BitcoinTestFramework): self.nodes[0].sendrawtransaction(joinsplit_result["rawtxn"]) self.nodes[0].generate(1) + self.sync_all() print "Done!" receive_result = self.nodes[0].zcrawreceive(zcsecretkey, joinsplit_result["encryptednote1"]) From d57af958029c24a1a57588e9765855282da512d4 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 18 Dec 2019 14:36:37 -0600 Subject: [PATCH 08/10] test: Remove genesis-block Sapling activation from shorter_block_times Sapling can't be activated in the genesis block without recomputing it, and we already activate Sapling by default in block 1, which is fine for this RPC test. --- qa/rpc-tests/shorter_block_times.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/qa/rpc-tests/shorter_block_times.py b/qa/rpc-tests/shorter_block_times.py index 3141a7e77..c74dae672 100755 --- a/qa/rpc-tests/shorter_block_times.py +++ b/qa/rpc-tests/shorter_block_times.py @@ -19,8 +19,6 @@ from test_framework.util import ( class ShorterBlockTimes(BitcoinTestFramework): def setup_nodes(self): return start_nodes(4, self.options.tmpdir, [[ - '-nuparams=5ba81b19:0', # Overwinter - '-nuparams=76b809bb:0', # Sapling '-nuparams=2bb40e60:106', # Blossom ]] * 4) From 4f6b477fb6c98f91bc6a9f85132019ee8f799ef7 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 18 Dec 2019 14:50:38 -0600 Subject: [PATCH 09/10] test: Reverse hashtx and hashblock ordering at start of ZMQ RPC test The "hashtx" message was previously triggered synchronously inside ConnectBlock, while the "hashblock" message is triggered in ConnectTip immediately after ConnectBlock returns. Thus ZMQ would see a "hashtx" for every relevant transaction, followed by a "hashblock". Now, "hashtx" is triggered asynchronously once the cs_main lock is dropped, which does not occur until after ConnectTip returns. Thus ZMQ will see a "hashblock" immediately after the block is connected, and then a "hashtx" for relevant transactions at some point afterwards. --- qa/rpc-tests/zmq_test.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/qa/rpc-tests/zmq_test.py b/qa/rpc-tests/zmq_test.py index 69b754240..b4a98fc61 100755 --- a/qa/rpc-tests/zmq_test.py +++ b/qa/rpc-tests/zmq_test.py @@ -39,15 +39,6 @@ class ZMQTest(BitcoinTestFramework): self.sync_all() print "listen..." - msg = self.zmqSubSocket.recv_multipart() - topic = msg[0] - assert_equal(topic, b"hashtx") - body = msg[1] - nseq = msg[2] - [nseq] # hush pyflakes - msgSequence = struct.unpack(' Date: Wed, 18 Dec 2019 15:05:04 -0600 Subject: [PATCH 10/10] test: Add missing sync_all point This file was renamed in master, so it was missed in the first commit. --- qa/rpc-tests/wallet_shieldingcoinbase.py | 1 + 1 file changed, 1 insertion(+) diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 5dd5be95b..e8d7a84ed 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/qa/rpc-tests/wallet_shieldingcoinbase.py @@ -47,6 +47,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): print "Mining blocks..." self.nodes[0].generate(4) + self.sync_all() walletinfo = self.nodes[0].getwalletinfo() assert_equal(walletinfo['immature_balance'], 40)