Skip to content

Commit c0d07dc

Browse files
ryanofskyryanofsky
committed
wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions
This is a step toward removing the Chain::Lock class and reducing cs_main locking. This change affects behavior in a few small ways. - If there's no max_height specified, percentage progress is measured ending at wallet last processed block instead of node tip - More consistent error reporting: Early check to see if start_block is on the active chain is removed, so start_block is always read and the triggers an error if it's unavailable
1 parent 1be8ff2 commit c0d07dc

File tree

8 files changed

+65
-42
lines changed

8 files changed

+65
-42
lines changed

‎src/interfaces/chain.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,13 @@ class ChainImpl : public Chain
251251
WAIT_LOCK(cs_main, lock);
252252
return FillBlock(ChainActive().FindEarliestAtLeast(min_time, min_height), block, lock);
253253
}
254+
bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next, bool* reorg) override {
255+
WAIT_LOCK(cs_main, lock);
256+
CBlockIndex* block = ChainActive()[block_height];
257+
if (block && block->GetBlockHash() != block_hash) block = nullptr;
258+
if (reorg) *reorg = !block;
259+
return FillBlock(block ? ChainActive()[block_height + 1] : nullptr, next, lock);
260+
}
254261
bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out) override
255262
{
256263
WAIT_LOCK(cs_main, lock);

‎src/interfaces/chain.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,11 @@ class Chain
148148
//! information.
149149
virtual bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block={}) = 0;
150150

151+
//! Find next block if block is part of current chain. Also flag if
152+
//! there was a reorg and the specified block hash is no longer in the
153+
//! current chain, and optionally return block information.
154+
virtual bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next={}, bool* reorg=nullptr) = 0;
155+
151156
//! Find ancestor of block at specified height and optionally return
152157
//! ancestor information.
153158
virtual bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out={}) = 0;

‎src/qt/test/wallettests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ void TestGUI(interfaces::Node& node)
153153
{
154154
WalletRescanReserver reserver(wallet.get());
155155
reserver.reserve();
156-
CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, {} /* stop_block */, reserver, true /* fUpdate */);
156+
CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* block height */, {} /* max height */, reserver, true /* fUpdate */);
157157
QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
158158
QCOMPARE(result.last_scanned_block, ::ChainActive().Tip()->GetBlockHash());
159159
QVERIFY(result.last_failed_block.IsNull());

‎src/test/interfaces_tests.cpp‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,21 @@ BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight)
5959
BOOST_CHECK(!chain->findFirstBlockWithTimeAndHeight(/* min_time= */ active.Tip()->GetBlockTimeMax() + 1, /* min_height= */ 0));
6060
}
6161

62+
BOOST_AUTO_TEST_CASE(findNextBlock)
63+
{
64+
auto chain = interfaces::MakeChain(m_node);
65+
auto& active = ChainActive();
66+
bool reorg;
67+
uint256 hash;
68+
BOOST_CHECK(chain->findNextBlock(active[20]->GetBlockHash(), 20, FoundBlock().hash(hash), &reorg));
69+
BOOST_CHECK_EQUAL(hash, active[21]->GetBlockHash());
70+
BOOST_CHECK_EQUAL(reorg, false);
71+
BOOST_CHECK(!chain->findNextBlock(uint256(), 20, {}, &reorg));
72+
BOOST_CHECK_EQUAL(reorg, true);
73+
BOOST_CHECK(!chain->findNextBlock(active.Tip()->GetBlockHash(), active.Height(), {}, &reorg));
74+
BOOST_CHECK_EQUAL(reorg, false);
75+
}
76+
6277
BOOST_AUTO_TEST_CASE(findAncestorByHeight)
6378
{
6479
auto chain = interfaces::MakeChain(m_node);

‎src/wallet/rpcwallet.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3569,7 +3569,7 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
35693569
}
35703570

35713571
CWallet::ScanResult result =
3572-
pwallet->ScanForWalletTransactions(start_block, stop_height, reserver, true /* fUpdate */);
3572+
pwallet->ScanForWalletTransactions(start_block, start_height, stop_height, reserver, true /* fUpdate */);
35733573
switch (result.status) {
35743574
case CWallet::ScanResult::SUCCESS:
35753575
break;

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
4646
auto locked_chain = chain->lock();
4747
LockAssertion lock(::cs_main);
4848

49-
// Verify ScanForWalletTransactions accommodates a null start block.
49+
// Verify ScanForWalletTransactions fails to read an unknown start block.
5050
{
5151
CWallet wallet(chain.get(), WalletLocation(), WalletDatabase::CreateDummy());
5252
{
@@ -56,8 +56,8 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
5656
AddKey(wallet, coinbaseKey);
5757
WalletRescanReserver reserver(&wallet);
5858
reserver.reserve();
59-
CWallet::ScanResult result = wallet.ScanForWalletTransactions({} /* start_block */, {} /* stop_block */, reserver, false /* update */);
60-
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
59+
CWallet::ScanResult result = wallet.ScanForWalletTransactions({} /* start_block */, 0 /* start_height */, {} /* max_height */, reserver, false /* update */);
60+
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
6161
BOOST_CHECK(result.last_failed_block.IsNull());
6262
BOOST_CHECK(result.last_scanned_block.IsNull());
6363
BOOST_CHECK(!result.last_scanned_height);
@@ -75,7 +75,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
7575
AddKey(wallet, coinbaseKey);
7676
WalletRescanReserver reserver(&wallet);
7777
reserver.reserve();
78-
CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
78+
CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */);
7979
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
8080
BOOST_CHECK(result.last_failed_block.IsNull());
8181
BOOST_CHECK_EQUAL(result.last_scanned_block, newTip->GetBlockHash());
@@ -98,7 +98,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
9898
AddKey(wallet, coinbaseKey);
9999
WalletRescanReserver reserver(&wallet);
100100
reserver.reserve();
101-
CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
101+
CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */);
102102
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
103103
BOOST_CHECK_EQUAL(result.last_failed_block, oldTip->GetBlockHash());
104104
BOOST_CHECK_EQUAL(result.last_scanned_block, newTip->GetBlockHash());
@@ -120,7 +120,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
120120
AddKey(wallet, coinbaseKey);
121121
WalletRescanReserver reserver(&wallet);
122122
reserver.reserve();
123-
CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
123+
CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */);
124124
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
125125
BOOST_CHECK_EQUAL(result.last_failed_block, newTip->GetBlockHash());
126126
BOOST_CHECK(result.last_scanned_block.IsNull());
@@ -465,7 +465,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
465465
AddKey(*wallet, coinbaseKey);
466466
WalletRescanReserver reserver(wallet.get());
467467
reserver.reserve();
468-
CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
468+
CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, false /* update */);
469469
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
470470
BOOST_CHECK_EQUAL(result.last_scanned_block, ::ChainActive().Tip()->GetBlockHash());
471471
BOOST_CHECK_EQUAL(*result.last_scanned_height, ::ChainActive().Height());

‎src/wallet/wallet.cpp‎

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1598,7 +1598,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
15981598

15991599
if (start) {
16001600
// TODO: this should take into account failure by ScanResult::USER_ABORT
1601-
ScanResult result = ScanForWalletTransactions(start_block, {} /* stop_block */, reserver, update);
1601+
ScanResult result = ScanForWalletTransactions(start_block, start_height, {} /* max_height */, reserver, update);
16021602
if (result.status == ScanResult::FAILURE) {
16031603
int64_t time_max;
16041604
CHECK_NONFATAL(chain().findBlock(result.last_failed_block, FoundBlock().maxTime(time_max)));
@@ -1615,6 +1615,7 @@ 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] start_height Height of start_block
16181619
* @param[in] max_height Optional max scanning height. If unset there is
16191620
* no maximum and scanning can continue to the tip
16201621
*
@@ -1628,7 +1629,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
16281629
* the main chain after to the addition of any new keys you want to detect
16291630
* transactions for.
16301631
*/
1631-
CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, Optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate)
1632+
CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, int start_height, Optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate)
16321633
{
16331634
int64_t nNow = GetTime();
16341635
int64_t start_time = GetTimeMillis();
@@ -1642,38 +1643,32 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
16421643

16431644
fAbortRescan = false;
16441645
ShowProgress(strprintf("%s " + _("Rescanning...").translated, GetDisplayName()), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup
1645-
uint256 tip_hash;
1646-
// The way the 'block_height' is initialized is just a workaround for the gcc bug #47679 since version 4.6.0.
1647-
Optional<int> block_height = MakeOptional(false, int());
1648-
double progress_begin;
1649-
double progress_end;
1650-
{
1651-
auto locked_chain = chain().lock();
1652-
if (Optional<int> tip_height = locked_chain->getHeight()) {
1653-
tip_hash = locked_chain->getBlockHash(*tip_height);
1654-
}
1655-
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));
1658-
progress_begin = chain().guessVerificationProgress(block_hash);
1659-
progress_end = chain().guessVerificationProgress(end_hash);
1660-
}
1646+
uint256 tip_hash = WITH_LOCK(cs_wallet, return GetLastBlockHash());
1647+
uint256 end_hash = tip_hash;
1648+
if (max_height) chain().findAncestorByHeight(tip_hash, *max_height, FoundBlock().hash(end_hash));
1649+
double progress_begin = chain().guessVerificationProgress(block_hash);
1650+
double progress_end = chain().guessVerificationProgress(end_hash);
16611651
double progress_current = progress_begin;
1662-
while (block_height && !fAbortRescan && !chain().shutdownRequested()) {
1652+
int block_height = start_height;
1653+
while (!fAbortRescan && !chain().shutdownRequested()) {
16631654
m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
1664-
if (*block_height % 100 == 0 && progress_end - progress_begin > 0.0) {
1655+
if (block_height % 100 == 0 && progress_end - progress_begin > 0.0) {
16651656
ShowProgress(strprintf("%s " + _("Rescanning...").translated, GetDisplayName()), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));
16661657
}
16671658
if (GetTime() >= nNow + 60) {
16681659
nNow = GetTime();
1669-
WalletLogPrintf("Still rescanning. At block %d. Progress=%f\n", *block_height, progress_current);
1660+
WalletLogPrintf("Still rescanning. At block %d. Progress=%f\n", block_height, progress_current);
16701661
}
16711662

16721663
CBlock block;
1664+
bool next_block;
1665+
uint256 next_block_hash;
1666+
bool reorg = false;
16731667
if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) {
16741668
auto locked_chain = chain().lock();
16751669
LOCK(cs_wallet);
1676-
if (!locked_chain->getBlockHeight(block_hash)) {
1670+
next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);
1671+
if (reorg) {
16771672
// Abort scan if current block is no longer active, to prevent
16781673
// marking transactions as coming from the wrong block.
16791674
// TODO: This should return success instead of failure, see
@@ -1683,36 +1678,37 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
16831678
break;
16841679
}
16851680
for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
1686-
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, *block_height, block_hash, posInBlock);
1681+
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, block_height, block_hash, posInBlock);
16871682
SyncTransaction(block.vtx[posInBlock], confirm, fUpdate);
16881683
}
16891684
// scan succeeded, record block as most recent successfully scanned
16901685
result.last_scanned_block = block_hash;
1691-
result.last_scanned_height = *block_height;
1686+
result.last_scanned_height = block_height;
16921687
} else {
16931688
// could not scan block, keep scanning but record this block as the most recent failure
16941689
result.last_failed_block = block_hash;
16951690
result.status = ScanResult::FAILURE;
1691+
next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);
16961692
}
1697-
if (max_height && *block_height >= *max_height) {
1693+
if (max_height && block_height >= *max_height) {
16981694
break;
16991695
}
17001696
{
17011697
auto locked_chain = chain().lock();
1702-
Optional<int> tip_height = locked_chain->getHeight();
1703-
if (!tip_height || *tip_height <= block_height || !locked_chain->getBlockHeight(block_hash)) {
1698+
if (!next_block || reorg) {
17041699
// break successfully when rescan has reached the tip, or
17051700
// previous block is no longer on the chain due to a reorg
17061701
break;
17071702
}
17081703

17091704
// increment block and verification progress
1710-
block_hash = locked_chain->getBlockHash(++*block_height);
1705+
block_hash = next_block_hash;
1706+
++block_height;
17111707
progress_current = chain().guessVerificationProgress(block_hash);
17121708

17131709
// handle updated tip hash
17141710
const uint256 prev_tip_hash = tip_hash;
1715-
tip_hash = locked_chain->getBlockHash(*tip_height);
1711+
tip_hash = WITH_LOCK(cs_wallet, return GetLastBlockHash());
17161712
if (!max_height && prev_tip_hash != tip_hash) {
17171713
// in case the tip has changed, update progress max
17181714
progress_end = chain().guessVerificationProgress(tip_hash);
@@ -1721,10 +1717,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
17211717
}
17221718
ShowProgress(strprintf("%s " + _("Rescanning...").translated, GetDisplayName()), 100); // hide progress dialog in GUI
17231719
if (block_height && fAbortRescan) {
1724-
WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", *block_height, progress_current);
1720+
WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height, progress_current);
17251721
result.status = ScanResult::USER_ABORT;
17261722
} else if (block_height && chain().shutdownRequested()) {
1727-
WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", *block_height, progress_current);
1723+
WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", block_height, progress_current);
17281724
result.status = ScanResult::USER_ABORT;
17291725
} else {
17301726
WalletLogPrintf("Rescan completed in %15dms\n", GetTimeMillis() - start_time);
@@ -4049,7 +4045,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
40494045

40504046
{
40514047
WalletRescanReserver reserver(walletInstance.get());
4052-
if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(locked_chain->getBlockHash(rescan_height), {} /* stop block */, reserver, true /* update */).status)) {
4048+
if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(locked_chain->getBlockHash(rescan_height), rescan_height, {} /* max height */, reserver, true /* update */).status)) {
40534049
error = _("Failed to rescan the wallet during initialization").translated;
40544050
return nullptr;
40554051
}

‎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, Optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);
897+
ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, 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)