From 1be8ff280c78c30baabae9429c53c0bebb89c44d Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 21 Jan 2020 17:08:12 -0500 Subject: [PATCH] wallet: Avoid use of Chain::Lock in rescanblockchain This is a step toward removing the Chain::Lock class and reducing cs_main locking. This change only affects behavior in the case where wallet last block processed falls behind the chain tip. The rescanblockchain error height error checking will just be stricter in this case and only accept values up to the last processed height --- src/interfaces/chain.cpp | 33 ++++++++++++++++++-------------- src/interfaces/chain.h | 9 +++++---- src/test/interfaces_tests.cpp | 36 +++++++++++++++++++++++++++++++++++ src/wallet/rpcwallet.cpp | 27 +++++++++----------------- src/wallet/wallet.cpp | 15 ++++++++------- src/wallet/wallet.h | 2 +- 6 files changed, 78 insertions(+), 44 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 0d69c33be0e..1c578b56caa 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -103,20 +103,6 @@ class LockImpl : public Chain::Lock, public UniqueLock } return nullopt; } - Optional findPruned(int start_height, Optional stop_height) override - { - LockAssertion lock(::cs_main); - if (::fPruneMode) { - CBlockIndex* block = stop_height ? ::ChainActive()[*stop_height] : ::ChainActive().Tip(); - while (block && block->nHeight >= start_height) { - if ((block->nStatus & BLOCK_HAVE_DATA) == 0) { - return block->nHeight; - } - block = block->pprev; - } - } - return nullopt; - } Optional findFork(const uint256& hash, Optional* height) override { LockAssertion lock(::cs_main); @@ -297,6 +283,25 @@ public: LOCK(cs_main); return GuessVerificationProgress(Params().TxData(), LookupBlockIndex(block_hash)); } + bool hasBlocks(const uint256& block_hash, int min_height, Optional max_height) override + { + // hasBlocks returns true if all ancestors of block_hash in specified + // range have block data (are not pruned), false if any ancestors in + // specified range are missing data. + // + // For simplicity and robustness, min_height and max_height are only + // used to limit the range, and passing min_height that's too low or + // max_height that's too high will not crash or change the result. + LOCK(::cs_main); + if (CBlockIndex* block = LookupBlockIndex(block_hash)) { + if (max_height && block->nHeight >= *max_height) block = block->GetAncestor(*max_height); + for (; block->nStatus & BLOCK_HAVE_DATA; block = block->pprev) { + // Check pprev to not segfault if min_height is too low + if (block->nHeight <= min_height || !block->pprev) return true; + } + } + return false; + } RBFTransactionState isRBFOptIn(const CTransaction& tx) override { LOCK(::mempool.cs); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 16753b7cc1d..8bc0ed824ce 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -114,10 +114,6 @@ public: //! (to avoid the cost of a second lookup in case this information is needed.) virtual Optional findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0; - //! Return height of last block in the specified range which is pruned, or - //! nullopt if no block in the range is pruned. Range is inclusive. - virtual Optional findPruned(int start_height = 0, Optional stop_height = nullopt) = 0; - //! Return height of the specified block if it is on the chain, otherwise //! return the height of the highest block on chain that's an ancestor //! of the specified block, or nullopt if there is no common ancestor. @@ -179,6 +175,11 @@ public: //! the specified block hash are verified. virtual double guessVerificationProgress(const uint256& block_hash) = 0; + //! Return true if data is available for all blocks in the specified range + //! of blocks. This checks all blocks that are ancestors of block_hash in + //! the height range from min_height to max_height, inclusive. + virtual bool hasBlocks(const uint256& block_hash, int min_height = 0, Optional max_height = {}) = 0; + //! Check if transaction is RBF opt in. virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0; diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp index b398b5819d4..5fba0e0429c 100644 --- a/src/test/interfaces_tests.cpp +++ b/src/test/interfaces_tests.cpp @@ -103,4 +103,40 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor) BOOST_CHECK_EQUAL(fork_hash, active[fork_height]->GetBlockHash()); } +BOOST_AUTO_TEST_CASE(hasBlocks) +{ + auto chain = interfaces::MakeChain(m_node); + auto& active = ChainActive(); + + // Test ranges + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90)); + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {})); + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90)); + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {})); + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000)); + active[5]->nStatus &= ~BLOCK_HAVE_DATA; + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90)); + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {})); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90)); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {})); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000)); + active[95]->nStatus &= ~BLOCK_HAVE_DATA; + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90)); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {})); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90)); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {})); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000)); + active[50]->nStatus &= ~BLOCK_HAVE_DATA; + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90)); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {})); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90)); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {})); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000)); + + // Test edge cases + BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 6, 49)); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 5, 49)); + BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 6, 50)); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 14e2066c903..6bda33c6c9e 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3536,22 +3536,23 @@ UniValue rescanblockchain(const JSONRPCRequest& request) } int start_height = 0; - uint256 start_block, stop_block; + Optional stop_height; + uint256 start_block; { auto locked_chain = pwallet->chain().lock(); - Optional tip_height = locked_chain->getHeight(); + LOCK(pwallet->cs_wallet); + int tip_height = pwallet->GetLastBlockHeight(); if (!request.params[0].isNull()) { start_height = request.params[0].get_int(); - if (start_height < 0 || !tip_height || start_height > *tip_height) { + if (start_height < 0 || start_height > tip_height) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height"); } } - Optional stop_height; if (!request.params[1].isNull()) { stop_height = request.params[1].get_int(); - if (*stop_height < 0 || !tip_height || *stop_height > *tip_height) { + if (*stop_height < 0 || *stop_height > tip_height) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stop_height"); } else if (*stop_height < start_height) { @@ -3560,25 +3561,15 @@ UniValue rescanblockchain(const JSONRPCRequest& request) } // We can't rescan beyond non-pruned blocks, stop and throw an error - if (locked_chain->findPruned(start_height, stop_height)) { + if (!pwallet->chain().hasBlocks(pwallet->GetLastBlockHash(), start_height, stop_height)) { throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height."); } - if (tip_height) { - start_block = locked_chain->getBlockHash(start_height); - // If called with a stop_height, set the stop_height here to - // trigger a rescan to that height. - // If called without a stop height, leave stop_height as null here - // so rescan continues to the tip (even if the tip advances during - // rescan). - if (stop_height) { - stop_block = locked_chain->getBlockHash(*stop_height); - } - } + CHECK_NONFATAL(pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), start_height, FoundBlock().hash(start_block))); } CWallet::ScanResult result = - pwallet->ScanForWalletTransactions(start_block, stop_block, reserver, true /* fUpdate */); + pwallet->ScanForWalletTransactions(start_block, stop_height, reserver, true /* fUpdate */); switch (result.status) { case CWallet::ScanResult::SUCCESS: break; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9ffb810ee7d..70d39a8fc58 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1615,9 +1615,8 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r * * @param[in] start_block Scan starting block. If block is not on the active * chain, the scan will return SUCCESS immediately. - * @param[in] stop_block Scan ending block. If block is not on the active - * chain, the scan will continue until it reaches the - * chain tip. + * @param[in] max_height Optional max scanning height. If unset there is + * no maximum and scanning can continue to the tip * * @return ScanResult returning scan information and indicating success or * failure. Return status will be set to SUCCESS if scan was @@ -1629,7 +1628,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 uint256& start_block, const uint256& stop_block, const WalletRescanReserver& reserver, bool fUpdate) +CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, Optional max_height, const WalletRescanReserver& reserver, bool fUpdate) { int64_t nNow = GetTime(); int64_t start_time = GetTimeMillis(); @@ -1654,8 +1653,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc tip_hash = locked_chain->getBlockHash(*tip_height); } block_height = locked_chain->getBlockHeight(block_hash); + uint256 end_hash = tip_hash; + if (max_height) chain().findAncestorByHeight(tip_hash, *max_height, FoundBlock().hash(end_hash)); progress_begin = chain().guessVerificationProgress(block_hash); - progress_end = chain().guessVerificationProgress(stop_block.IsNull() ? tip_hash : stop_block); + progress_end = chain().guessVerificationProgress(end_hash); } double progress_current = progress_begin; while (block_height && !fAbortRescan && !chain().shutdownRequested()) { @@ -1693,7 +1694,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc result.last_failed_block = block_hash; result.status = ScanResult::FAILURE; } - if (block_hash == stop_block) { + if (max_height && *block_height >= *max_height) { break; } { @@ -1712,7 +1713,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc // handle updated tip hash const uint256 prev_tip_hash = tip_hash; tip_hash = locked_chain->getBlockHash(*tip_height); - if (stop_block.IsNull() && prev_tip_hash != tip_hash) { + if (!max_height && prev_tip_hash != tip_hash) { // in case the tip has changed, update progress max progress_end = chain().guessVerificationProgress(tip_hash); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 26824946c69..f105ccd178c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -894,7 +894,7 @@ public: //! USER_ABORT. uint256 last_failed_block; }; - ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate); + ScanResult ScanForWalletTransactions(const uint256& first_block, Optional max_height, const WalletRescanReserver& reserver, bool fUpdate); void transactionRemovedFromMempool(const CTransactionRef &ptx) override; void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void ResendWalletTransactions();