Skip to content

Commit 1be8ff2

Browse files
ryanofskyryanofsky
committed
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
1 parent 3cb85ac commit 1be8ff2

File tree

6 files changed

+78
-44
lines changed

6 files changed

+78
-44
lines changed

‎src/interfaces/chain.cpp

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -103,20 +103,6 @@ class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
103103
}
104104
return nullopt;
105105
}
106-
Optional<int> findPruned(int start_height, Optional<int> stop_height) override
107-
{
108-
LockAssertion lock(::cs_main);
109-
if (::fPruneMode) {
110-
CBlockIndex* block = stop_height ? ::ChainActive()[*stop_height] : ::ChainActive().Tip();
111-
while (block && block->nHeight >= start_height) {
112-
if ((block->nStatus & BLOCK_HAVE_DATA) == 0) {
113-
return block->nHeight;
114-
}
115-
block = block->pprev;
116-
}
117-
}
118-
return nullopt;
119-
}
120106
Optional<int> findFork(const uint256& hash, Optional<int>* height) override
121107
{
122108
LockAssertion lock(::cs_main);
@@ -297,6 +283,25 @@ class ChainImpl : public Chain
297283
LOCK(cs_main);
298284
return GuessVerificationProgress(Params().TxData(), LookupBlockIndex(block_hash));
299285
}
286+
bool hasBlocks(const uint256& block_hash, int min_height, Optional<int> max_height) override
287+
{
288+
// hasBlocks returns true if all ancestors of block_hash in specified
289+
// range have block data (are not pruned), false if any ancestors in
290+
// specified range are missing data.
291+
//
292+
// For simplicity and robustness, min_height and max_height are only
293+
// used to limit the range, and passing min_height that's too low or
294+
// max_height that's too high will not crash or change the result.
295+
LOCK(::cs_main);
296+
if (CBlockIndex* block = LookupBlockIndex(block_hash)) {
297+
if (max_height && block->nHeight >= *max_height) block = block->GetAncestor(*max_height);
298+
for (; block->nStatus & BLOCK_HAVE_DATA; block = block->pprev) {
299+
// Check pprev to not segfault if min_height is too low
300+
if (block->nHeight <= min_height || !block->pprev) return true;
301+
}
302+
}
303+
return false;
304+
}
300305
RBFTransactionState isRBFOptIn(const CTransaction& tx) override
301306
{
302307
LOCK(::mempool.cs);

‎src/interfaces/chain.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,6 @@ class Chain
114114
//! (to avoid the cost of a second lookup in case this information is needed.)
115115
virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0;
116116

117-
//! Return height of last block in the specified range which is pruned, or
118-
//! nullopt if no block in the range is pruned. Range is inclusive.
119-
virtual Optional<int> findPruned(int start_height = 0, Optional<int> stop_height = nullopt) = 0;
120-
121117
//! Return height of the specified block if it is on the chain, otherwise
122118
//! return the height of the highest block on chain that's an ancestor
123119
//! of the specified block, or nullopt if there is no common ancestor.
@@ -179,6 +175,11 @@ class Chain
179175
//! the specified block hash are verified.
180176
virtual double guessVerificationProgress(const uint256& block_hash) = 0;
181177

178+
//! Return true if data is available for all blocks in the specified range
179+
//! of blocks. This checks all blocks that are ancestors of block_hash in
180+
//! the height range from min_height to max_height, inclusive.
181+
virtual bool hasBlocks(const uint256& block_hash, int min_height = 0, Optional<int> max_height = {}) = 0;
182+
182183
//! Check if transaction is RBF opt in.
183184
virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;
184185

‎src/test/interfaces_tests.cpp

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,40 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
103103
BOOST_CHECK_EQUAL(fork_hash, active[fork_height]->GetBlockHash());
104104
}
105105

106+
BOOST_AUTO_TEST_CASE(hasBlocks)
107+
{
108+
auto chain = interfaces::MakeChain(m_node);
109+
auto& active = ChainActive();
110+
111+
// Test ranges
112+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90));
113+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {}));
114+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90));
115+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {}));
116+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000));
117+
active[5]->nStatus &= ~BLOCK_HAVE_DATA;
118+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90));
119+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {}));
120+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90));
121+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {}));
122+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000));
123+
active[95]->nStatus &= ~BLOCK_HAVE_DATA;
124+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90));
125+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {}));
126+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90));
127+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {}));
128+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000));
129+
active[50]->nStatus &= ~BLOCK_HAVE_DATA;
130+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90));
131+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 10, {}));
132+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, 90));
133+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 0, {}));
134+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), -1000, 1000));
135+
136+
// Test edge cases
137+
BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 6, 49));
138+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 5, 49));
139+
BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 6, 50));
140+
}
141+
106142
BOOST_AUTO_TEST_SUITE_END()

‎src/wallet/rpcwallet.cpp

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3536,22 +3536,23 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
35363536
}
35373537

35383538
int start_height = 0;
3539-
uint256 start_block, stop_block;
3539+
Optional<int> stop_height;
3540+
uint256 start_block;
35403541
{
35413542
auto locked_chain = pwallet->chain().lock();
3542-
Optional<int> tip_height = locked_chain->getHeight();
3543+
LOCK(pwallet->cs_wallet);
3544+
int tip_height = pwallet->GetLastBlockHeight();
35433545

35443546
if (!request.params[0].isNull()) {
35453547
start_height = request.params[0].get_int();
3546-
if (start_height < 0 || !tip_height || start_height > *tip_height) {
3548+
if (start_height < 0 || start_height > tip_height) {
35473549
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height");
35483550
}
35493551
}
35503552

3551-
Optional<int> stop_height;
35523553
if (!request.params[1].isNull()) {
35533554
stop_height = request.params[1].get_int();
3554-
if (*stop_height < 0 || !tip_height || *stop_height > *tip_height) {
3555+
if (*stop_height < 0 || *stop_height > tip_height) {
35553556
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stop_height");
35563557
}
35573558
else if (*stop_height < start_height) {
@@ -3560,25 +3561,15 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
35603561
}
35613562

35623563
// We can't rescan beyond non-pruned blocks, stop and throw an error
3563-
if (locked_chain->findPruned(start_height, stop_height)) {
3564+
if (!pwallet->chain().hasBlocks(pwallet->GetLastBlockHash(), start_height, stop_height)) {
35643565
throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
35653566
}
35663567

3567-
if (tip_height) {
3568-
start_block = locked_chain->getBlockHash(start_height);
3569-
// If called with a stop_height, set the stop_height here to
3570-
// trigger a rescan to that height.
3571-
// If called without a stop height, leave stop_height as null here
3572-
// so rescan continues to the tip (even if the tip advances during
3573-
// rescan).
3574-
if (stop_height) {
3575-
stop_block = locked_chain->getBlockHash(*stop_height);
3576-
}
3577-
}
3568+
CHECK_NONFATAL(pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), start_height, FoundBlock().hash(start_block)));
35783569
}
35793570

35803571
CWallet::ScanResult result =
3581-
pwallet->ScanForWalletTransactions(start_block, stop_block, reserver, true /* fUpdate */);
3572+
pwallet->ScanForWalletTransactions(start_block, stop_height, reserver, true /* fUpdate */);
35823573
switch (result.status) {
35833574
case CWallet::ScanResult::SUCCESS:
35843575
break;

‎src/wallet/wallet.cpp

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1615,9 +1615,8 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
16151615
*
16161616
* @param[in] start_block Scan starting block. If block is not on the active
16171617
* chain, the scan will return SUCCESS immediately.
1618-
* @param[in] stop_block Scan ending block. If block is not on the active
1619-
* chain, the scan will continue until it reaches the
1620-
* chain tip.
1618+
* @param[in] max_height Optional max scanning height. If unset there is
1619+
* no maximum and scanning can continue to the tip
16211620
*
16221621
* @return ScanResult returning scan information and indicating success or
16231622
* 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
16291628
* the main chain after to the addition of any new keys you want to detect
16301629
* transactions for.
16311630
*/
1632-
CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, const uint256& stop_block, const WalletRescanReserver& reserver, bool fUpdate)
1631+
CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, Optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate)
16331632
{
16341633
int64_t nNow = GetTime();
16351634
int64_t start_time = GetTimeMillis();
@@ -1654,8 +1653,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
16541653
tip_hash = locked_chain->getBlockHash(*tip_height);
16551654
}
16561655
block_height = locked_chain->getBlockHeight(block_hash);
1656+
uint256 end_hash = tip_hash;
1657+
if (max_height) chain().findAncestorByHeight(tip_hash, *max_height, FoundBlock().hash(end_hash));
16571658
progress_begin = chain().guessVerificationProgress(block_hash);
1658-
progress_end = chain().guessVerificationProgress(stop_block.IsNull() ? tip_hash : stop_block);
1659+
progress_end = chain().guessVerificationProgress(end_hash);
16591660
}
16601661
double progress_current = progress_begin;
16611662
while (block_height && !fAbortRescan && !chain().shutdownRequested()) {
@@ -1693,7 +1694,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
16931694
result.last_failed_block = block_hash;
16941695
result.status = ScanResult::FAILURE;
16951696
}
1696-
if (block_hash == stop_block) {
1697+
if (max_height && *block_height >= *max_height) {
16971698
break;
16981699
}
16991700
{
@@ -1712,7 +1713,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
17121713
// handle updated tip hash
17131714
const uint256 prev_tip_hash = tip_hash;
17141715
tip_hash = locked_chain->getBlockHash(*tip_height);
1715-
if (stop_block.IsNull() && prev_tip_hash != tip_hash) {
1716+
if (!max_height && prev_tip_hash != tip_hash) {
17161717
// in case the tip has changed, update progress max
17171718
progress_end = chain().guessVerificationProgress(tip_hash);
17181719
}

‎src/wallet/wallet.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -894,7 +894,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
894894
//! USER_ABORT.
895895
uint256 last_failed_block;
896896
};
897-
ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate);
897+
ScanResult ScanForWalletTransactions(const uint256& first_block, Optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);
898898
void transactionRemovedFromMempool(const CTransactionRef &ptx) override;
899899
void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
900900
void ResendWalletTransactions();

0 commit comments

Comments
 (0)