From ce694802d99e42a087595ae132ebf2fa1d9a8f91 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 25 Jan 2023 14:26:08 -0700 Subject: [PATCH 1/4] Fetch recently conflicted transactions incrementally in ThreadNotifyWallet. We no longer fetch updates from the mempool unless we have fetched all updates from the chain, as we would otherwise notify the wallet of mempool changes for which they have not observed parent transactions in the chain. Co-authored-by: Jack Grigg --- src/main.cpp | 37 +++++++++++++++++------------ src/main.h | 3 ++- src/validationinterface.cpp | 46 ++++++++++++++++++++++++++++--------- 3 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 5a5bd9e95..4e1d816dd 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4062,8 +4062,8 @@ static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; // Protected by cs_main -std::map> recentlyConflictedTxs; -uint64_t nRecentlyConflictedSequence = 0; +std::map> recentlyConflictedTxs; +uint64_t nConnectedSequence = 0; uint64_t nNotifiedSequence = 0; /** @@ -4115,7 +4115,9 @@ 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; + + // Increment the count of `ConnectTip` calls. + nConnectedSequence += 1; EnforceNodeDeprecation(pindexNew->nHeight); @@ -4126,29 +4128,34 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, return true; } -std::pair>, uint64_t> DrainRecentlyConflicted() +std::pair, std::optional> TakeRecentlyConflicted(const CBlockIndex* pindex) { - uint64_t recentlyConflictedSequence; - std::map> txs; - { - LOCK(cs_main); - recentlyConflictedSequence = nRecentlyConflictedSequence; - txs.swap(recentlyConflictedTxs); - } + AssertLockHeld(cs_main); - return std::make_pair(txs, recentlyConflictedSequence); + std::list conflictedTxs = recentlyConflictedTxs.at(pindex); + recentlyConflictedTxs.erase(pindex); + if (recentlyConflictedTxs.empty()) { + return std::make_pair(conflictedTxs, nConnectedSequence); + } else { + return std::make_pair(conflictedTxs, std::nullopt); + } } -void SetChainNotifiedSequence(const CChainParams& chainparams, uint64_t recentlyConflictedSequence) { +uint64_t GetChainConnectedSequence() { + LOCK(cs_main); + return nConnectedSequence; +} + +void SetChainNotifiedSequence(const CChainParams& chainparams, uint64_t connectedSequence) { assert(chainparams.NetworkIDString() == "regtest"); LOCK(cs_main); - nNotifiedSequence = recentlyConflictedSequence; + nNotifiedSequence = connectedSequence; } bool ChainIsFullyNotified(const CChainParams& chainparams) { assert(chainparams.NetworkIDString() == "regtest"); LOCK(cs_main); - return nRecentlyConflictedSequence == nNotifiedSequence; + return nConnectedSequence == nNotifiedSequence; } /** diff --git a/src/main.h b/src/main.h index 6cc4df786..dabba715d 100644 --- a/src/main.h +++ b/src/main.h @@ -668,7 +668,8 @@ CMutableTransaction CreateNewContextualCMutableTransaction( int nHeight, bool requireV4); -std::pair>, uint64_t> DrainRecentlyConflicted(); +std::pair, std::optional> TakeRecentlyConflicted(const CBlockIndex* pindex); +uint64_t GetChainConnectedSequence(); void SetChainNotifiedSequence(const CChainParams& chainparams, uint64_t recentlyConflictedSequence); bool ChainIsFullyNotified(const CChainParams& chainparams); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 476dab819..446f9ee53 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -163,8 +163,9 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // 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::pair>, uint64_t> recentlyConflicted; + // Sequence number indicating that we have notified wallets of transactions up to + // the ConnectBlock() call that generated this sequence number. + std::optional chainNotifiedSequence; // Transactions that have been recently added to the mempool. std::pair, uint64_t> recentlyAdded; @@ -176,12 +177,8 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) 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. + bool originalTipAtFork = pindex && pindex == pindexFork; while (pindex && pindex != pindexFork) { MerkleFrontiers oldFrontiers; // Get the Sprout commitment tree as of the start of this block. @@ -214,15 +211,38 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) OrchardMerkleFrontier::empty_root(), oldFrontiers.orchard)); } + // 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. + auto recentlyConflicted = TakeRecentlyConflicted(pindex); + blockStack.emplace_back( pindex, oldFrontiers, - recentlyConflicted.first[pindex]); + recentlyConflicted.first); + + chainNotifiedSequence = recentlyConflicted.second; pindex = pindex->pprev; } - recentlyAdded = mempool.DrainRecentlyAdded(); + // This conditional can be true in the case that in the interval + // since the last second-boundary, two reorgs occurred: one that + // shifted over to a different chain history, and then a second + // that returned the chain to the original pre-reorg tip. This + // should never occur unless a caller has manually used + // `invalidateblock` to force the second reorg or we have a long + // persistent set of dueling chains. In such a case, wallets may + // not be fully notified of conflicted transactions, but they will + // still have a correct view of the current main chain, and they + // will still be notified properly of the current state of + // transactions in the mempool. + if (originalTipAtFork) { + chainNotifiedSequence = GetChainConnectedSequence(); + } + if (chainNotifiedSequence.has_value()) { + recentlyAdded = mempool.DrainRecentlyAdded(); + } } // @@ -540,8 +560,12 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // 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(chainParams, recentlyConflicted.second); - mempool.SetNotifiedSequence(recentlyAdded.second); + if (chainNotifiedSequence.has_value()) { + SetChainNotifiedSequence(chainParams, chainNotifiedSequence.value()); + } + if (recentlyAdded.second > 0) { + mempool.SetNotifiedSequence(recentlyAdded.second); + } } } } From aab58d308fb993b12e3591f631ae813d62bf1e71 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 25 Jan 2023 17:37:58 -0700 Subject: [PATCH 2/4] Bound wallet batch scanner size to 1000 blocks instead of 100 MiB 1000 blocks was selected as a balance between limiting the likely maximum memory usage of the batch scanner, and avoiding artificially restricting scanning throughput of small/fast blocks due to the second-boundary lock synchronization point. This also removes the `zcashd.wallet.batchscanner.usage.bytes` gague value that was previously made available when `-prometheusport` was specified. Co-authored-by: Jack Grigg --- doc/release-notes.md | 12 ++++++++++++ src/rust/src/wallet_scanner.rs | 14 -------------- src/validationinterface.cpp | 28 +++++++++------------------- src/validationinterface.h | 7 ++----- src/wallet/wallet.cpp | 14 +++++++++----- src/wallet/wallet.h | 2 -- 6 files changed, 32 insertions(+), 45 deletions(-) diff --git a/doc/release-notes.md b/doc/release-notes.md index 562420b55..cc9ef769d 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -42,6 +42,18 @@ heights prior to this will not have any information recorded. To track changes from genesis, and thus monitor the total transparent pool size and chain supply, you would need to restart your node with the `-reindex` option. +Wallet Performance Fixes +------------------------ + +The 100MiB memory limit for the batch scanner has been replaced by a 1000-block +limit. This eliminates an expensive call to determine the current memory usage +of the batch scanner. + +The following associated metric has been removed from the set of metrics +reported when `-prometheusport` is set: + +- (gauge) `zcashd.wallet.batchscanner.usage.bytes` + RPC Changes ----------- diff --git a/src/rust/src/wallet_scanner.rs b/src/rust/src/wallet_scanner.rs index 2dcb1ef53..ddc77c3b5 100644 --- a/src/rust/src/wallet_scanner.rs +++ b/src/rust/src/wallet_scanner.rs @@ -55,7 +55,6 @@ mod ffi { network: &Network, sapling_ivks: &[[u8; 32]], ) -> Result>; - fn get_dynamic_usage(self: &BatchScanner) -> usize; fn add_transaction( self: &mut BatchScanner, block_tag: [u8; 32], @@ -82,7 +81,6 @@ const METRIC_OUTPUTS_SCANNED: &str = "zcashd.wallet.batchscanner.outputs.scanned const METRIC_LABEL_KIND: &str = "kind"; const METRIC_SIZE_TXS: &str = "zcashd.wallet.batchscanner.size.transactions"; -const METRIC_USAGE_BYTES: &str = "zcashd.wallet.batchscanner.usage.bytes"; /// Chain parameters for the networks supported by `zcashd`. #[derive(Clone, Copy)] @@ -787,16 +785,6 @@ fn init_batch_scanner( } impl BatchScanner { - /// FFI helper to access the `DynamicUsage` trait. - fn get_dynamic_usage(&self) -> usize { - let usage = self.dynamic_usage(); - - // Since we've measured it, we may as well update the metric. - metrics::gauge!(METRIC_USAGE_BYTES, usage as f64); - - usage - } - /// Adds the given transaction's shielded outputs to the various batch runners. /// /// `block_tag` is the hash of the block that triggered this txid being added to the @@ -833,7 +821,6 @@ impl BatchScanner { // Update the size of the batch scanner. metrics::increment_gauge!(METRIC_SIZE_TXS, 1.0); - metrics::gauge!(METRIC_USAGE_BYTES, self.dynamic_usage() as f64); Ok(()) } @@ -866,7 +853,6 @@ impl BatchScanner { // Update the size of the batch scanner. metrics::decrement_gauge!(METRIC_SIZE_TXS, 1.0); - metrics::gauge!(METRIC_USAGE_BYTES, self.dynamic_usage() as f64); Box::new(BatchResult { sapling }) } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 446f9ee53..1240e0d7c 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -72,16 +72,6 @@ void UnregisterAllValidationInterfaces() { g_signals.UpdatedBlockTip.disconnect_all_slots(); } -size_t RecursiveDynamicUsage( - std::vector &batchScanners) -{ - size_t usage = 0; - for (auto& batchScanner : batchScanners) { - usage += batchScanner->RecursiveDynamicUsage(); - } - return usage; -} - void AddTxToBatches( std::vector &batchScanners, const CTransaction &tx, @@ -177,6 +167,12 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) CBlockIndex *pindex = chainActive.Tip(); pindexFork = chainActive.FindFork(pindexLastTip); + // Iterate backwards over the connected blocks until we have at + // most WALLET_NOTIFY_MAX_BLOCKS to process. + while (pindex && pindex->nHeight > pindexFork->nHeight + WALLET_NOTIFY_MAX_BLOCKS) { + pindex = pindex->pprev; + } + // Iterate backwards over the connected blocks we need to notify. bool originalTipAtFork = pindex && pindex == pindexFork; while (pindex && pindex != pindexFork) { @@ -327,12 +323,6 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // for example to add new incoming viewing keys. auto batchScanners = GetMainSignals().GetBatchScanner(); - // Closure that returns true if batchScanners is using less memory than - // the desired limit. - auto belowBatchMemoryLimit = [&]() { - return RecursiveDynamicUsage(batchScanners) < nBatchScannerMemLimit; - }; - // Closure that will add a block from blockStack to batchScanners. auto batchScanConnectedBlock = [&](const CachedBlockData& blockData) { // Read block from disk. @@ -423,7 +413,7 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // // We process blockStack in the same order we do below, so batched // work can be completed in roughly the order we need it. - for (; blockStackScanned != blockStack.rend() && belowBatchMemoryLimit(); ++blockStackScanned) { + for (; blockStackScanned != blockStack.rend(); ++blockStackScanned) { const auto& blockData = *blockStackScanned; batchScanConnectedBlock(blockData); } @@ -480,7 +470,7 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // added the rest of blockStack, or have reached the memory limit // again. At this point, we know that blockStackScanned has not been // invalidated by mutations to blockStack, and can be dereferenced. - for (; blockStackScanned != blockStack.rend() && belowBatchMemoryLimit(); ++blockStackScanned) { + for (; blockStackScanned != blockStack.rend(); ++blockStackScanned) { const auto& blockData = *blockStackScanned; batchScanConnectedBlock(blockData); } @@ -498,7 +488,7 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) while (!( blockStack.empty() || blockStack.rbegin() == blockStackScanned || - (blockStackScanned != blockStack.rend() && belowBatchMemoryLimit()) + (blockStackScanned != blockStack.rend()) )) { auto& blockData = blockStack.back(); diff --git a/src/validationinterface.h b/src/validationinterface.h index ff9d59fde..d4177e9bc 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -17,6 +17,8 @@ /** Default limit on batch scanner memory usage in MiB. */ static const size_t DEFAULT_BATCHSCANNERMEMLIMIT = 100; +/** Limit on batch scanner memory usage in MiB. */ +static const size_t WALLET_NOTIFY_MAX_BLOCKS = 1000; class CBlock; class CBlockIndex; @@ -29,11 +31,6 @@ class uint256; class BatchScanner { public: - /** - * Returns the current dynamic memory usage of this batch scanner. - */ - virtual size_t RecursiveDynamicUsage() = 0; - /** * Adds a transaction to the batch scanner. * diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e32210d2a..3425efffa 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1455,6 +1455,15 @@ void CWallet::ChainTip(const CBlockIndex *pindex, DecrementNoteWitnesses(consensus, pindex); UpdateSaplingNullifierNoteMapForBlock(pblock); } + + auto hash = tfm::format("%s", pindex->GetBlockHash().ToString()); + auto height = tfm::format("%d", pindex->nHeight); + auto kind = tfm::format("%s", added.has_value() ? "connect" : "disconnect"); + + TracingInfo("wallet", "CWallet::ChainTip: processed block", + "hash", hash.c_str(), + "height", height.c_str(), + "kind", kind.c_str()); } void CWallet::RunSaplingMigration(int blockHeight) { @@ -3597,11 +3606,6 @@ bool WalletBatchScanner::AddToWalletIfInvolvingMe( // BatchScanner APIs // -size_t WalletBatchScanner::RecursiveDynamicUsage() -{ - return inner->get_dynamic_usage(); -} - void WalletBatchScanner::AddTransaction( const CTransaction &tx, const std::vector &txBytes, diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 71e9195d1..7a4946e31 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1148,8 +1148,6 @@ public: // BatchScanner APIs // - size_t RecursiveDynamicUsage(); - void AddTransaction( const CTransaction &tx, const std::vector &txBytes, From 887b2688df0475fe3ee24a16ce49200e54abcb87 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 26 Jan 2023 12:33:55 -0700 Subject: [PATCH 3/4] Remove unused DEFAULT_BATCHSCANNERMEMLIMIT constant. --- src/validationinterface.cpp | 2 -- src/validationinterface.h | 7 ++++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 1240e0d7c..21de7514c 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -118,8 +118,6 @@ struct CachedBlockData { void ThreadNotifyWallets(CBlockIndex *pindexLastTip) { - size_t nBatchScannerMemLimit = DEFAULT_BATCHSCANNERMEMLIMIT * 1024 * 1024; - // If pindexLastTip == nullptr, the wallet is at genesis. // However, the genesis block is not loaded synchronously. // We need to wait for ThreadImport to finish. diff --git a/src/validationinterface.h b/src/validationinterface.h index d4177e9bc..0d6f395c0 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -15,9 +15,10 @@ #include "miner.h" #include "zcash/IncrementalMerkleTree.hpp" -/** Default limit on batch scanner memory usage in MiB. */ -static const size_t DEFAULT_BATCHSCANNERMEMLIMIT = 100; -/** Limit on batch scanner memory usage in MiB. */ +/** + * Limit on the maximum number of blocks that will be staged for + * scanning before an interrupt will be handled. + */ static const size_t WALLET_NOTIFY_MAX_BLOCKS = 1000; class CBlock; From 29e65bf536d26abdab1f85fab415e51a25fa758a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 26 Jan 2023 16:51:23 -0700 Subject: [PATCH 4/4] Tolerate missing cached conflict data in ThreadNotifyWallets --- src/main.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 4e1d816dd..cbcaa83c2 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4132,7 +4132,13 @@ std::pair, std::optional> TakeRecentlyConflict { AssertLockHeld(cs_main); - std::list conflictedTxs = recentlyConflictedTxs.at(pindex); + // We use bracket notation for retrieving conflict data from recentlyConflictedTxs + // here because when a node restarts, the wallet may be behind the node's view of + // the current chain tip. The node may continue reindexing from the chain tip, but + // no entries will exist in `recentlyConflictedTxs` until the next block after the + // node's chain tip at the point of shutdown. In these cases, the wallet cannot learn + // about conflicts in those blocks (which should be fine). + std::list conflictedTxs = recentlyConflictedTxs[pindex]; recentlyConflictedTxs.erase(pindex); if (recentlyConflictedTxs.empty()) { return std::make_pair(conflictedTxs, nConnectedSequence);