From bd3b0361d840bff95988a048abf70ade94d80524 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Tue, 13 Nov 2018 04:57:10 +0000 Subject: [PATCH] Add stop_block out arg to ScanForWalletTransactions Accurately reports the last block successfully scanned, replacing a return of the chain tip, which represented possibly inaccurated data in a race condition. --- src/qt/test/wallettests.cpp | 7 ++++--- src/wallet/rpcwallet.cpp | 7 +++---- src/wallet/test/wallet_tests.cpp | 21 ++++++++++++--------- src/wallet/wallet.cpp | 19 ++++++++++++------- src/wallet/wallet.h | 2 +- 5 files changed, 32 insertions(+), 24 deletions(-) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 5646070a3ff29..610d83acb6fb9 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -147,11 +147,12 @@ void TestGUI() WalletRescanReserver reserver(wallet.get()); reserver.reserve(); const CBlockIndex* const null_block = nullptr; - const CBlockIndex* stop_block; + const CBlockIndex *stop_block, *failed_block; QCOMPARE( - wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, stop_block, true /* fUpdate */), + wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block, true /* fUpdate */), CWallet::ScanResult::SUCCESS); - QCOMPARE(stop_block, null_block); + QCOMPARE(stop_block, chainActive.Tip()); + QCOMPARE(failed_block, null_block); } wallet->SetBroadcastTransactions(true); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c3f12f373d8f9..3c92eb31204c8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3332,13 +3332,12 @@ UniValue rescanblockchain(const JSONRPCRequest& request) } } - const CBlockIndex* stopBlock; + const CBlockIndex *failed_block, *stopBlock; CWallet::ScanResult result = - pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, stopBlock, true); + pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, failed_block, stopBlock, true); switch (result) { case CWallet::ScanResult::SUCCESS: - stopBlock = pindexStop ? pindexStop : pChainTip; - break; + break; // stopBlock set by ScanForWalletTransactions case CWallet::ScanResult::FAILURE: throw JSONRPCError(RPC_MISC_ERROR, "Rescan failed. Potentially corrupted data files."); case CWallet::ScanResult::USER_ABORT: diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index dccda1acf1b33..46d9b5b238313 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -53,9 +53,10 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) AddKey(wallet, coinbaseKey); WalletRescanReserver reserver(&wallet); reserver.reserve(); - const CBlockIndex* stop_block; - BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, stop_block), CWallet::ScanResult::SUCCESS); - BOOST_CHECK_EQUAL(stop_block, null_block); + const CBlockIndex *stop_block, *failed_block; + BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS); + BOOST_CHECK_EQUAL(failed_block, null_block); + BOOST_CHECK_EQUAL(stop_block, newTip); BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN); } @@ -70,9 +71,10 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) AddKey(wallet, coinbaseKey); WalletRescanReserver reserver(&wallet); reserver.reserve(); - const CBlockIndex* stop_block; - BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, stop_block), CWallet::ScanResult::FAILURE); - BOOST_CHECK_EQUAL(oldTip, stop_block); + const CBlockIndex *stop_block, *failed_block; + BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE); + BOOST_CHECK_EQUAL(failed_block, oldTip); + BOOST_CHECK_EQUAL(stop_block, newTip); BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN); } @@ -291,9 +293,10 @@ class ListCoinsTestingSetup : public TestChain100Setup WalletRescanReserver reserver(wallet.get()); reserver.reserve(); const CBlockIndex* const null_block = nullptr; - const CBlockIndex* stop_block; - BOOST_CHECK_EQUAL(wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, stop_block), CWallet::ScanResult::SUCCESS); - BOOST_CHECK_EQUAL(stop_block, null_block); + const CBlockIndex *stop_block, *failed_block; + BOOST_CHECK_EQUAL(wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS); + BOOST_CHECK_EQUAL(stop_block, chainActive.Tip()); + BOOST_CHECK_EQUAL(failed_block, null_block); } ~ListCoinsTestingSetup() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 28ee32b128bb6..3c25a67fd1484 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1613,9 +1613,9 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r } if (startBlock) { - const CBlockIndex* failedBlock; + const CBlockIndex *failedBlock, *stop_block; // TODO: this should take into account failure by ScanResult::USER_ABORT - if (ScanResult::FAILURE == ScanForWalletTransactions(startBlock, nullptr, reserver, failedBlock, update)) { + if (ScanResult::FAILURE == ScanForWalletTransactions(startBlock, nullptr, reserver, failedBlock, stop_block, update)) { return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1; } } @@ -1628,8 +1628,10 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r * exist in the wallet will be updated. * * @param[in] pindexStop if not a nullptr, the scan will stop at this block-index - * @param[out] failed_block if FAILURE is returned, will be set to the most - * recent block that could not be scanned, otherwise nullptr + * @param[out] failed_block if FAILURE is returned, the most recent block + * that could not be scanned, otherwise nullptr + * @param[out] stop_block the most recent block that could be scanned, + * otherwise nullptr if no block could be scanned * * @return ScanResult indicating success or failure of the scan. SUCCESS if * scan was successful. FAILURE if a complete rescan was not possible (due to @@ -1640,7 +1642,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r * the main chain after to the addition of any new keys you want to detect * transactions for. */ -CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, bool fUpdate) +CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, const CBlockIndex*& stop_block, bool fUpdate) { int64_t nNow = GetTime(); const CChainParams& chainParams = Params(); @@ -1694,7 +1696,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) { SyncTransaction(block.vtx[posInBlock], pindex, posInBlock, fUpdate); } + // scan succeeded, record block as most recent successfully scanned + stop_block = pindex; } else { + // could not scan block, keep scanning but record this block as the most recent failure failed_block = pindex; } if (pindex == pindexStop) { @@ -4174,8 +4179,8 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, nStart = GetTimeMillis(); { WalletRescanReserver reserver(walletInstance.get()); - const CBlockIndex* stop_block; - if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, reserver, stop_block, true))) { + const CBlockIndex *stop_block, *failed_block; + if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, reserver, failed_block, stop_block, true))) { InitError(_("Failed to rescan the wallet during initialization")); return nullptr; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 08184bdc22d47..5ba5514fc2b9d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -902,7 +902,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface FAILURE, USER_ABORT }; - ScanResult ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, bool fUpdate = false); + ScanResult ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, const CBlockIndex*& stop_block, bool fUpdate = false); void TransactionRemovedFromMempool(const CTransactionRef &ptx) override; void ReacceptWalletTransactions(); void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);