From 3a1261efda3fda9a41874713bd19fbd167657cc2 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 2 Apr 2022 00:38:26 +0000 Subject: [PATCH 1/2] wallet: Initialise ThreadNotifyWallets with wallet's best block The previous code assumed that the last chain tip notified to the wallet was equal to the node's chain tip at startup. However, this assumption fails if the node shuts down uncleanly, or if a wallet file is moved from one node to another. We now try to start notifying from the wallet's best block, and if the node doesn't have that block we fall back to the node's chain tip like before. Closes zcash/zcash#5805. --- src/init.cpp | 24 +++++++++++++++++++++--- src/validationinterface.cpp | 25 +++++++++++++++++++++++++ src/wallet/wallet.cpp | 21 +++++++++++++++++++++ src/wallet/wallet.h | 6 ++++++ 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 3f5d1d48f..cc0a4ed62 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1707,15 +1707,33 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) // Start the thread that notifies listeners of transactions that have been // recently added to the mempool, or have been added to or removed from the // chain. We perform this before step 10 (import blocks) so that the - // original value of chainActive.Tip(), which corresponds with the wallet's - // view of the chaintip, is passed to ThreadNotifyWallets before the chain - // tip changes again. + // original value of chainActive.Tip() can be passed to ThreadNotifyWallets + // before the chain tip changes again. { CBlockIndex *pindexLastTip; { LOCK(cs_main); pindexLastTip = chainActive.Tip(); } + + // However, if a wallet is enabled, we actually want to start notifying + // from the block which corresponds with the wallet's view of the chain + // tip. In particular, we want to handle the case where the node shuts + // down uncleanly, and on restart the chain's tip is potentially up to + // an hour of chain sync older than the wallet's tip. We assume here + // that there is only a single wallet connected to the validation + // interface, which is currently true. +#ifdef ENABLE_WALLET + if (pwalletMain) + { + LOCK2(cs_main, pwalletMain->cs_wallet); + const auto walletBestBlock = pwalletMain->GetBestBlock(); + if (walletBestBlock != nullptr) { + pindexLastTip = walletBestBlock; + } + } +#endif + boost::function threadnotifywallets = boost::bind(&ThreadNotifyWallets, pindexLastTip); threadGroup.create_thread( boost::bind(&TraceThread>, "txnotify", threadnotifywallets) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 61c9d7f5a..0668626c4 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -93,6 +93,31 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) MilliSleep(50); } + // We cannot progress with wallet notification until the chain tip is no + // more than 100 blocks behind pindexLastTip. This can occur if the node + // shuts down abruptly without being able to write out chainActive; the + // node writes chain data out roughly hourly, while the wallet writes it + // every 10 minutes. We need to wait for ThreadImport to catch up. + while (true) { + boost::this_thread::interruption_point(); + + { + LOCK(cs_main); + + const CBlockIndex *pindexFork = chainActive.FindFork(pindexLastTip); + // We know we have the genesis block. + assert(pindexFork != nullptr); + + if (pindexLastTip->nHeight < pindexFork->nHeight || + pindexLastTip->nHeight - pindexFork->nHeight < 100) + { + break; + } + } + + MilliSleep(50); + } + while (true) { // Run the notifier on an integer second in the steady clock. auto now = std::chrono::steady_clock::now().time_since_epoch(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 10e4d9979..d75b9a073 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1501,6 +1501,27 @@ void CWallet::SetBestChain(const CBlockLocator& loc) SetBestChainINTERNAL(walletdb, loc); } +CBlockIndex* CWallet::GetBestBlock() +{ + AssertLockHeld(cs_main); + AssertLockHeld(cs_wallet); + + CWalletDB walletdb(strWalletFile); + CBlockLocator locator; + if (walletdb.ReadBestBlock(locator)) { + if (!locator.vHave.empty()) { + BlockMap::iterator mi = mapBlockIndex.find(locator.vHave[0]); + if (mi != mapBlockIndex.end()) { + return (*mi).second; + } + } + } + + // The wallet's best block is not known to the node. This can occur when a + // wallet file is transplanted between disparate nodes. + return nullptr; +} + std::set> CWallet::GetSproutNullifiers( const std::set& addresses) { std::set> nullifierSet; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4864d3364..1f689b39b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1805,6 +1805,12 @@ public: void AddPendingSaplingMigrationTx(const CTransaction& tx); /** Saves witness caches and best block locator to disk. */ void SetBestChain(const CBlockLocator& loc); + /** + * Returns the block index corresponding to the wallet's best block, or + * nullptr if the wallet's best block is not known to this node (e.g. if the + * wallet was transplanted from another node). + */ + CBlockIndex* GetBestBlock(); std::set> GetSproutNullifiers( const std::set& addresses); From 098a70ed896e1f5f8e5496de5828ddd5ea80c0df Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 4 Apr 2022 17:39:53 +0000 Subject: [PATCH 2/2] wallet: Rename `CWallet::GetBestBlock` to `GetPersistedBestBlock` This more accurately reflects its meaning, as it corresponds to the most recently persisted best chain (i.e. the chain tip that the wallet will return to on restart), rather than the chain tip to which the in-memory wallet state has been synced. --- src/init.cpp | 2 +- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 12 ++++++++---- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index cc0a4ed62..b200a3448 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1727,7 +1727,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) if (pwalletMain) { LOCK2(cs_main, pwalletMain->cs_wallet); - const auto walletBestBlock = pwalletMain->GetBestBlock(); + const auto walletBestBlock = pwalletMain->GetPersistedBestBlock(); if (walletBestBlock != nullptr) { pindexLastTip = walletBestBlock; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d75b9a073..238689a5b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1501,7 +1501,7 @@ void CWallet::SetBestChain(const CBlockLocator& loc) SetBestChainINTERNAL(walletdb, loc); } -CBlockIndex* CWallet::GetBestBlock() +CBlockIndex* CWallet::GetPersistedBestBlock() { AssertLockHeld(cs_main); AssertLockHeld(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1f689b39b..aff585c3f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1806,11 +1806,15 @@ public: /** Saves witness caches and best block locator to disk. */ void SetBestChain(const CBlockLocator& loc); /** - * Returns the block index corresponding to the wallet's best block, or - * nullptr if the wallet's best block is not known to this node (e.g. if the - * wallet was transplanted from another node). + * Returns the block index corresponding to the wallet's most recently + * persisted best block. This is the state to which the wallet will revert + * if restarted immediately, and does not necessarily match the current + * in-memory state. + * + * Returns nullptr if the wallet's best block is not known to this node + * (e.g. if the wallet was transplanted from another node). */ - CBlockIndex* GetBestBlock(); + CBlockIndex* GetPersistedBestBlock(); std::set> GetSproutNullifiers( const std::set& addresses);