Skip to content

Commit 8470e64

Browse files
laanwjlaanwj
committed
Merge #11281: Avoid permanent cs_main/cs_wallet lock during RescanFromTime
7f81250 Mention that other RPC calls report keys as "imported" while txns are still missing (Jonas Schnelli) ccd8ef6 Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock (Jonas Schnelli) bc356b4 Make sure WalletRescanReserver has successfully reserved the rescan (Jonas Schnelli) dbf8556 Add RAII wallet rescan reserver (Jonas Schnelli) 8d0b610 Avoid pemanent cs_main/cs_wallet lock during wallet rescans (Jonas Schnelli) Pull request description: Right now, we are holding `cs_main`/`cs_wallet` during the whole rescan process (which can take a couple of hours). This was probably only done because of laziness and it is an important show-stopper for #11200 (GUI rescan abort). Tree-SHA512: 0fc3f82d0ee9b2f013e6bacba8d59f7334306660cd676cd64c47bb305c4cb7c7a36219d6a6f76023b74e5fe87f3ab9fc7fd2439e939f71aef653fddb0a1e23b1
2 parents b5e4b9b + 7f81250 commit 8470e64

File tree

7 files changed

+349
-215
lines changed

7 files changed

+349
-215
lines changed

‎src/qt/test/wallettests.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,9 @@ void TestGUI()
169169
}
170170
{
171171
LOCK(cs_main);
172-
wallet.ScanForWalletTransactions(chainActive.Genesis(), nullptr, true);
172+
WalletRescanReserver reserver(&wallet);
173+
reserver.reserve();
174+
wallet.ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, true);
173175
}
174176
wallet.SetBroadcastTransactions(true);
175177

‎src/validation.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1121,7 +1121,13 @@ bool ReadBlockFromDisk(CBlock& block, const CDiskBlockPos& pos, const Consensus:
11211121

11221122
bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
11231123
{
1124-
if (!ReadBlockFromDisk(block, pindex->GetBlockPos(), consensusParams))
1124+
CDiskBlockPos blockPos;
1125+
{
1126+
LOCK(cs_main);
1127+
blockPos = pindex->GetBlockPos();
1128+
}
1129+
1130+
if (!ReadBlockFromDisk(block, blockPos, consensusParams))
11251131
return false;
11261132
if (block.GetHash() != pindex->GetBlockHash())
11271133
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",

‎src/wallet/rpcdump.cpp

Lines changed: 204 additions & 170 deletions
Large diffs are not rendered by default.

‎src/wallet/rpcwallet.cpp

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3416,30 +3416,41 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
34163416
);
34173417
}
34183418

3419-
LOCK2(cs_main, pwallet->cs_wallet);
3419+
WalletRescanReserver reserver(pwallet);
3420+
if (!reserver.reserve()) {
3421+
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
3422+
}
34203423

3421-
CBlockIndex *pindexStart = chainActive.Genesis();
3424+
CBlockIndex *pindexStart = nullptr;
34223425
CBlockIndex *pindexStop = nullptr;
3423-
if (!request.params[0].isNull()) {
3424-
pindexStart = chainActive[request.params[0].get_int()];
3425-
if (!pindexStart) {
3426-
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height");
3427-
}
3428-
}
3426+
CBlockIndex *pChainTip = nullptr;
3427+
{
3428+
LOCK(cs_main);
3429+
pindexStart = chainActive.Genesis();
3430+
pChainTip = chainActive.Tip();
34293431

3430-
if (!request.params[1].isNull()) {
3431-
pindexStop = chainActive[request.params[1].get_int()];
3432-
if (!pindexStop) {
3433-
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stop_height");
3432+
if (!request.params[0].isNull()) {
3433+
pindexStart = chainActive[request.params[0].get_int()];
3434+
if (!pindexStart) {
3435+
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height");
3436+
}
34343437
}
3435-
else if (pindexStop->nHeight < pindexStart->nHeight) {
3436-
throw JSONRPCError(RPC_INVALID_PARAMETER, "stop_height must be greater then start_height");
3438+
3439+
if (!request.params[1].isNull()) {
3440+
pindexStop = chainActive[request.params[1].get_int()];
3441+
if (!pindexStop) {
3442+
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stop_height");
3443+
}
3444+
else if (pindexStop->nHeight < pindexStart->nHeight) {
3445+
throw JSONRPCError(RPC_INVALID_PARAMETER, "stop_height must be greater then start_height");
3446+
}
34373447
}
34383448
}
34393449

34403450
// We can't rescan beyond non-pruned blocks, stop and throw an error
34413451
if (fPruneMode) {
3442-
CBlockIndex *block = pindexStop ? pindexStop : chainActive.Tip();
3452+
LOCK(cs_main);
3453+
CBlockIndex *block = pindexStop ? pindexStop : pChainTip;
34433454
while (block && block->nHeight >= pindexStart->nHeight) {
34443455
if (!(block->nStatus & BLOCK_HAVE_DATA)) {
34453456
throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
@@ -3448,18 +3459,17 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
34483459
}
34493460
}
34503461

3451-
CBlockIndex *stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, true);
3462+
CBlockIndex *stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, true);
34523463
if (!stopBlock) {
34533464
if (pwallet->IsAbortingRescan()) {
34543465
throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted.");
34553466
}
34563467
// if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex
3457-
stopBlock = pindexStop ? pindexStop : chainActive.Tip();
3468+
stopBlock = pindexStop ? pindexStop : pChainTip;
34583469
}
34593470
else {
34603471
throw JSONRPCError(RPC_MISC_ERROR, "Rescan failed. Potentially corrupted data files.");
34613472
}
3462-
34633473
UniValue response(UniValue::VOBJ);
34643474
response.pushKV("start_height", pindexStart->nHeight);
34653475
response.pushKV("stop_height", stopBlock->nHeight);

‎src/wallet/test/wallet_tests.cpp

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,9 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
384384
{
385385
CWallet wallet;
386386
AddKey(wallet, coinbaseKey);
387-
BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip, nullptr));
387+
WalletRescanReserver reserver(&wallet);
388+
reserver.reserve();
389+
BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip, nullptr, reserver));
388390
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN);
389391
}
390392

@@ -397,7 +399,9 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
397399
{
398400
CWallet wallet;
399401
AddKey(wallet, coinbaseKey);
400-
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip, nullptr));
402+
WalletRescanReserver reserver(&wallet);
403+
reserver.reserve();
404+
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip, nullptr, reserver));
401405
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
402406
}
403407

@@ -608,7 +612,9 @@ class ListCoinsTestingSetup : public TestChain100Setup
608612
bool firstRun;
609613
wallet->LoadWallet(firstRun);
610614
AddKey(*wallet, coinbaseKey);
611-
wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr);
615+
WalletRescanReserver reserver(wallet.get());
616+
reserver.reserve();
617+
wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver);
612618
}
613619

614620
~ListCoinsTestingSetup()

‎src/wallet/wallet.cpp

Lines changed: 56 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1612,19 +1612,20 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
16121612
* @return Earliest timestamp that could be successfully scanned from. Timestamp
16131613
* returned will be higher than startTime if relevant blocks could not be read.
16141614
*/
1615-
int64_t CWallet::RescanFromTime(int64_t startTime, bool update)
1615+
int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update)
16161616
{
1617-
AssertLockHeld(cs_main);
1618-
AssertLockHeld(cs_wallet);
1619-
16201617
// Find starting block. May be null if nCreateTime is greater than the
16211618
// highest blockchain timestamp, in which case there is nothing that needs
16221619
// to be scanned.
1623-
CBlockIndex* const startBlock = chainActive.FindEarliestAtLeast(startTime - TIMESTAMP_WINDOW);
1624-
LogPrintf("%s: Rescanning last %i blocks\n", __func__, startBlock ? chainActive.Height() - startBlock->nHeight + 1 : 0);
1620+
CBlockIndex* startBlock = nullptr;
1621+
{
1622+
LOCK(cs_main);
1623+
startBlock = chainActive.FindEarliestAtLeast(startTime - TIMESTAMP_WINDOW);
1624+
LogPrintf("%s: Rescanning last %i blocks\n", __func__, startBlock ? chainActive.Height() - startBlock->nHeight + 1 : 0);
1625+
}
16251626

16261627
if (startBlock) {
1627-
const CBlockIndex* const failedBlock = ScanForWalletTransactions(startBlock, nullptr, update);
1628+
const CBlockIndex* const failedBlock = ScanForWalletTransactions(startBlock, nullptr, reserver, update);
16281629
if (failedBlock) {
16291630
return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1;
16301631
}
@@ -1643,37 +1644,60 @@ int64_t CWallet::RescanFromTime(int64_t startTime, bool update)
16431644
*
16441645
* If pindexStop is not a nullptr, the scan will stop at the block-index
16451646
* defined by pindexStop
1647+
*
1648+
* Caller needs to make sure pindexStop (and the optional pindexStart) are on
1649+
* the main chain after to the addition of any new keys you want to detect
1650+
* transactions for.
16461651
*/
1647-
CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, bool fUpdate)
1652+
CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver &reserver, bool fUpdate)
16481653
{
16491654
int64_t nNow = GetTime();
16501655
const CChainParams& chainParams = Params();
16511656

1657+
assert(reserver.isReserved());
16521658
if (pindexStop) {
16531659
assert(pindexStop->nHeight >= pindexStart->nHeight);
16541660
}
16551661

16561662
CBlockIndex* pindex = pindexStart;
16571663
CBlockIndex* ret = nullptr;
16581664
{
1659-
LOCK2(cs_main, cs_wallet);
16601665
fAbortRescan = false;
1661-
fScanningWallet = true;
1662-
16631666
ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup
1664-
double dProgressStart = GuessVerificationProgress(chainParams.TxData(), pindex);
1665-
double dProgressTip = GuessVerificationProgress(chainParams.TxData(), chainActive.Tip());
1667+
CBlockIndex* tip = nullptr;
1668+
double dProgressStart;
1669+
double dProgressTip;
1670+
{
1671+
LOCK(cs_main);
1672+
tip = chainActive.Tip();
1673+
dProgressStart = GuessVerificationProgress(chainParams.TxData(), pindex);
1674+
dProgressTip = GuessVerificationProgress(chainParams.TxData(), tip);
1675+
}
16661676
while (pindex && !fAbortRescan)
16671677
{
1668-
if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0)
1669-
ShowProgress(_("Rescanning..."), std::max(1, std::min(99, (int)((GuessVerificationProgress(chainParams.TxData(), pindex) - dProgressStart) / (dProgressTip - dProgressStart) * 100))));
1678+
if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0) {
1679+
double gvp = 0;
1680+
{
1681+
LOCK(cs_main);
1682+
gvp = GuessVerificationProgress(chainParams.TxData(), pindex);
1683+
}
1684+
ShowProgress(_("Rescanning..."), std::max(1, std::min(99, (int)((gvp - dProgressStart) / (dProgressTip - dProgressStart) * 100))));
1685+
}
16701686
if (GetTime() >= nNow + 60) {
16711687
nNow = GetTime();
1688+
LOCK(cs_main);
16721689
LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, GuessVerificationProgress(chainParams.TxData(), pindex));
16731690
}
16741691

16751692
CBlock block;
16761693
if (ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
1694+
LOCK2(cs_main, cs_wallet);
1695+
if (pindex && !chainActive.Contains(pindex)) {
1696+
// Abort scan if current block is no longer active, to prevent
1697+
// marking transactions as coming from the wrong block.
1698+
ret = pindex;
1699+
break;
1700+
}
16771701
for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
16781702
AddToWalletIfInvolvingMe(block.vtx[posInBlock], pindex, posInBlock, fUpdate);
16791703
}
@@ -1683,14 +1707,20 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
16831707
if (pindex == pindexStop) {
16841708
break;
16851709
}
1686-
pindex = chainActive.Next(pindex);
1710+
{
1711+
LOCK(cs_main);
1712+
pindex = chainActive.Next(pindex);
1713+
if (tip != chainActive.Tip()) {
1714+
tip = chainActive.Tip();
1715+
// in case the tip has changed, update progress max
1716+
dProgressTip = GuessVerificationProgress(chainParams.TxData(), tip);
1717+
}
1718+
}
16871719
}
16881720
if (pindex && fAbortRescan) {
16891721
LogPrintf("Rescan aborted at block %d. Progress=%f\n", pindex->nHeight, GuessVerificationProgress(chainParams.TxData(), pindex));
16901722
}
16911723
ShowProgress(_("Rescanning..."), 100); // hide progress dialog in GUI
1692-
1693-
fScanningWallet = false;
16941724
}
16951725
return ret;
16961726
}
@@ -4001,7 +4031,14 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
40014031
}
40024032

40034033
nStart = GetTimeMillis();
4004-
walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, true);
4034+
{
4035+
WalletRescanReserver reserver(walletInstance);
4036+
if (!reserver.reserve()) {
4037+
InitError(_("Failed to rescan the wallet during initialization"));
4038+
return nullptr;
4039+
}
4040+
walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, reserver, true);
4041+
}
40054042
LogPrintf(" rescan %15dms\n", GetTimeMillis() - nStart);
40064043
walletInstance->SetBestChain(chainActive.GetLocator());
40074044
walletInstance->dbw->IncrementUpdateCounter();

‎src/wallet/wallet.h

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -659,6 +659,7 @@ class CAccountingEntry
659659
};
660660

661661

662+
class WalletRescanReserver; //forward declarations for ScanForWalletTransactions/RescanFromTime
662663
/**
663664
* A CWallet is an extension of a keystore, which also maintains a set of transactions and balances,
664665
* and provides the ability to create new transactions.
@@ -668,7 +669,10 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
668669
private:
669670
static std::atomic<bool> fFlushScheduled;
670671
std::atomic<bool> fAbortRescan;
671-
std::atomic<bool> fScanningWallet;
672+
std::atomic<bool> fScanningWallet; //controlled by WalletRescanReserver
673+
std::mutex mutexScanning;
674+
friend class WalletRescanReserver;
675+
672676

673677
/**
674678
* Select a set of coins such that nValueRet >= nTargetValue and at least
@@ -945,8 +949,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
945949
void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted) override;
946950
void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) override;
947951
bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate);
948-
int64_t RescanFromTime(int64_t startTime, bool update);
949-
CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, bool fUpdate = false);
952+
int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update);
953+
CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver& reserver, bool fUpdate = false);
950954
void TransactionRemovedFromMempool(const CTransactionRef &ptx) override;
951955
void ReacceptWalletTransactions();
952956
void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override;
@@ -1263,4 +1267,39 @@ CTxDestination GetDestinationForKey(const CPubKey& key, OutputType);
12631267
/** Get all destinations (potentially) supported by the wallet for the given key. */
12641268
std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key);
12651269

1270+
/** RAII object to check and reserve a wallet rescan */
1271+
class WalletRescanReserver
1272+
{
1273+
private:
1274+
CWalletRef m_wallet;
1275+
bool m_could_reserve;
1276+
public:
1277+
explicit WalletRescanReserver(CWalletRef w) : m_wallet(w), m_could_reserve(false) {}
1278+
1279+
bool reserve()
1280+
{
1281+
assert(!m_could_reserve);
1282+
std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);
1283+
if (m_wallet->fScanningWallet) {
1284+
return false;
1285+
}
1286+
m_wallet->fScanningWallet = true;
1287+
m_could_reserve = true;
1288+
return true;
1289+
}
1290+
1291+
bool isReserved() const
1292+
{
1293+
return (m_could_reserve && m_wallet->fScanningWallet);
1294+
}
1295+
1296+
~WalletRescanReserver()
1297+
{
1298+
std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);
1299+
if (m_could_reserve) {
1300+
m_wallet->fScanningWallet = false;
1301+
}
1302+
}
1303+
};
1304+
12661305
#endif // BITCOIN_WALLET_WALLET_H

0 commit comments

Comments
 (0)