Skip to content

Avoid permanent cs_main/cs_wallet lock during RescanFromTime #11281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 24, 2018
Merged

Avoid permanent cs_main/cs_wallet lock during RescanFromTime #11281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d0b610
Avoid pemanent cs_main/cs_wallet lock during wallet rescans
jonasschnelli Sep 7, 2017
dbf8556
Add RAII wallet rescan reserver
jonasschnelli Dec 8, 2017
bc356b4
Make sure WalletRescanReserver has successfully reserved the rescan
jonasschnelli Dec 12, 2017
ccd8ef6
Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under…
jonasschnelli Dec 13, 2017
7f81250
Mention that other RPC calls report keys as "imported" while txns are…
jonasschnelli Jan 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/qt/test/wallettests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,9 @@ void TestGUI()
}
{
LOCK(cs_main);
wallet.ScanForWalletTransactions(chainActive.Genesis(), nullptr, true);
WalletRescanReserver reserver(&wallet);
reserver.reserve();
wallet.ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, true);
}
wallet.SetBroadcastTransactions(true);

Expand Down
8 changes: 7 additions & 1 deletion src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,7 +1121,13 @@ bool ReadBlockFromDisk(CBlock& block, const CDiskBlockPos& pos, const Consensus:

bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
{
if (!ReadBlockFromDisk(block, pindex->GetBlockPos(), consensusParams))
CDiskBlockPos blockPos;
{
LOCK(cs_main);
blockPos = pindex->GetBlockPos();
}

if (!ReadBlockFromDisk(block, blockPos, consensusParams))
return false;
if (block.GetHash() != pindex->GetBlockHash())
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
Expand Down
90 changes: 62 additions & 28 deletions src/wallet/rpcdump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ UniValue importprivkey(const JSONRPCRequest& request)
"1. \"privkey\" (string, required) The private key (see dumpprivkey)\n"
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
"\nNote: This call can take minutes to complete if rescan is true.\n"
"\nNote: This call can take minutes to complete if rescan is true, during that time, other rpc calls\n"
"may report that the imported key exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
"\nExamples:\n"
"\nDump a private key\n"
+ HelpExampleCli("dumpprivkey", "\"myaddress\"") +
Expand All @@ -101,6 +102,9 @@ UniValue importprivkey(const JSONRPCRequest& request)
);


WalletRescanReserver reserver(pwallet);
bool fRescan = true;
{
LOCK2(cs_main, pwallet->cs_wallet);

EnsureWalletIsUnlocked(pwallet);
Expand All @@ -111,13 +115,16 @@ UniValue importprivkey(const JSONRPCRequest& request)
strLabel = request.params[1].get_str();

// Whether to perform rescan after import
bool fRescan = true;
if (!request.params[2].isNull())
fRescan = request.params[2].get_bool();

if (fRescan && fPruneMode)
throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");

if (fRescan && !reserver.reserve()) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
}

CBitcoinSecret vchSecret;
bool fGood = vchSecret.SetString(strSecret);

Expand All @@ -131,7 +138,6 @@ UniValue importprivkey(const JSONRPCRequest& request)
CKeyID vchAddress = pubkey.GetID();
{
pwallet->MarkDirty();

// We don't know which corresponding address will be used; label them all
for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
pwallet->SetAddressBook(dest, strLabel, "receive");
Expand All @@ -142,20 +148,19 @@ UniValue importprivkey(const JSONRPCRequest& request)
return NullUniValue;
}

// whenever a key is imported, we need to scan the whole chain
pwallet->UpdateTimeFirstKey(1);
pwallet->mapKeyMetadata[vchAddress].nCreateTime = 1;

if (!pwallet->AddKeyPubKey(key, pubkey)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
}
pwallet->LearnAllRelatedScripts(pubkey);

// whenever a key is imported, we need to scan the whole chain
pwallet->UpdateTimeFirstKey(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line didn't make it into the new rebase, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Yes. Thanks Will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Why'd you move it up in the function instead of keeping its position?


if (fRescan) {
pwallet->RescanFromTime(TIMESTAMP_MIN, true /* update */);
}
}
if (fRescan) {
pwallet->RescanFromTime(TIMESTAMP_MIN, reserver, true /* update */);
}

return NullUniValue;
}
Expand Down Expand Up @@ -237,7 +242,8 @@ UniValue importaddress(const JSONRPCRequest& request)
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
"4. p2sh (boolean, optional, default=false) Add the P2SH version of the script as well\n"
"\nNote: This call can take minutes to complete if rescan is true.\n"
"\nNote: This call can take minutes to complete if rescan is true, during that time, other rpc calls\n"
"may report that the imported address exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
"If you have the full public key, you should call importpubkey instead of this.\n"
"\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n"
"as change, and not show up in many RPCs.\n"
Expand All @@ -263,11 +269,17 @@ UniValue importaddress(const JSONRPCRequest& request)
if (fRescan && fPruneMode)
throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");

WalletRescanReserver reserver(pwallet);
if (fRescan && !reserver.reserve()) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
}

// Whether to import a p2sh version, too
bool fP2SH = false;
if (!request.params[3].isNull())
fP2SH = request.params[3].get_bool();

{
LOCK2(cs_main, pwallet->cs_wallet);

CTxDestination dest = DecodeDestination(request.params[0].get_str());
Expand All @@ -282,10 +294,10 @@ UniValue importaddress(const JSONRPCRequest& request)
} else {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address or script");
}

}
if (fRescan)
{
pwallet->RescanFromTime(TIMESTAMP_MIN, true /* update */);
pwallet->RescanFromTime(TIMESTAMP_MIN, reserver, true /* update */);
pwallet->ReacceptWalletTransactions();
}

Expand Down Expand Up @@ -406,7 +418,8 @@ UniValue importpubkey(const JSONRPCRequest& request)
"1. \"pubkey\" (string, required) The hex-encoded public key\n"
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
"\nNote: This call can take minutes to complete if rescan is true.\n"
"\nNote: This call can take minutes to complete if rescan is true, during that time, other rpc calls\n"
"may report that the imported pubkey exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
"\nExamples:\n"
"\nImport a public key with rescan\n"
+ HelpExampleCli("importpubkey", "\"mypubkey\"") +
Expand All @@ -429,24 +442,30 @@ UniValue importpubkey(const JSONRPCRequest& request)
if (fRescan && fPruneMode)
throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");

WalletRescanReserver reserver(pwallet);
if (fRescan && !reserver.reserve()) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
}

if (!IsHex(request.params[0].get_str()))
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string");
std::vector<unsigned char> data(ParseHex(request.params[0].get_str()));
CPubKey pubKey(data.begin(), data.end());
if (!pubKey.IsFullyValid())
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");

{
LOCK2(cs_main, pwallet->cs_wallet);

for (const auto& dest : GetAllDestinationsForKey(pubKey)) {
ImportAddress(pwallet, dest, strLabel);
}
ImportScript(pwallet, GetScriptForRawPubKey(pubKey), strLabel, false);
pwallet->LearnAllRelatedScripts(pubKey);

}
if (fRescan)
{
pwallet->RescanFromTime(TIMESTAMP_MIN, true /* update */);
pwallet->RescanFromTime(TIMESTAMP_MIN, reserver, true /* update */);
pwallet->ReacceptWalletTransactions();
}

Expand Down Expand Up @@ -479,18 +498,24 @@ UniValue importwallet(const JSONRPCRequest& request)
if (fPruneMode)
throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled in pruned mode");

WalletRescanReserver reserver(pwallet);
if (!reserver.reserve()) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
}

int64_t nTimeBegin = 0;
bool fGood = true;
{
LOCK2(cs_main, pwallet->cs_wallet);

EnsureWalletIsUnlocked(pwallet);

std::ifstream file;
file.open(request.params[0].get_str().c_str(), std::ios::in | std::ios::ate);
if (!file.is_open())
if (!file.is_open()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");

int64_t nTimeBegin = chainActive.Tip()->GetBlockTime();

bool fGood = true;
}
nTimeBegin = chainActive.Tip()->GetBlockTime();

int64_t nFilesize = std::max((int64_t)1, (int64_t)file.tellg());
file.seekg(0, file.beg);
Expand Down Expand Up @@ -563,7 +588,8 @@ UniValue importwallet(const JSONRPCRequest& request)
file.close();
pwallet->ShowProgress("", 100); // hide progress dialog in GUI
pwallet->UpdateTimeFirstKey(nTimeBegin);
pwallet->RescanFromTime(nTimeBegin, false /* update */);
}
pwallet->RescanFromTime(nTimeBegin, reserver, false /* update */);
pwallet->MarkDirty();

if (!fGood)
Expand Down Expand Up @@ -1110,6 +1136,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
" {\n"
" \"rescan\": <false>, (boolean, optional, default: true) Stating if should rescan the blockchain after all imports\n"
" }\n"
"\nNote: This call can take minutes to complete if rescan is true, during that time, other rpc calls\n"
"may report that the imported keys, addresses or scripts exists but related transactions are still missing.\n"
"\nExamples:\n" +
HelpExampleCli("importmulti", "'[{ \"scriptPubKey\": { \"address\": \"<my address>\" }, \"timestamp\":1455191478 }, "
"{ \"scriptPubKey\": { \"address\": \"<my 2nd address>\" }, \"label\": \"example 2\", \"timestamp\": 1455191480 }]'") +
Expand All @@ -1135,27 +1163,33 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
}
}

WalletRescanReserver reserver(pwallet);
if (fRescan && !reserver.reserve()) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
}

int64_t now = 0;
bool fRunScan = false;
int64_t nLowestTimestamp = 0;
UniValue response(UniValue::VARR);
{
LOCK2(cs_main, pwallet->cs_wallet);
EnsureWalletIsUnlocked(pwallet);

// Verify all timestamps are present before importing any keys.
const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetMedianTimePast() : 0;
now = chainActive.Tip() ? chainActive.Tip()->GetMedianTimePast() : 0;
for (const UniValue& data : requests.getValues()) {
GetImportTimestamp(data, now);
}

bool fRunScan = false;
const int64_t minimumTimestamp = 1;
int64_t nLowestTimestamp = 0;

if (fRescan && chainActive.Tip()) {
nLowestTimestamp = chainActive.Tip()->GetBlockTime();
} else {
fRescan = false;
}

UniValue response(UniValue::VARR);

for (const UniValue& data : requests.getValues()) {
const int64_t timestamp = std::max(GetImportTimestamp(data, now), minimumTimestamp);
const UniValue result = ProcessImport(pwallet, data, timestamp);
Expand All @@ -1175,9 +1209,9 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
nLowestTimestamp = timestamp;
}
}

}
if (fRescan && fRunScan && requests.size()) {
int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, true /* update */);
int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */);
pwallet->ReacceptWalletTransactions();

if (scannedTime > nLowestTimestamp) {
Expand Down
22 changes: 16 additions & 6 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3398,10 +3398,19 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
);
}

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

CBlockIndex *pindexStart = chainActive.Genesis();
CBlockIndex *pindexStart = nullptr;
CBlockIndex *pindexStop = nullptr;
CBlockIndex *pChainTip = nullptr;
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant to put a LOCK(cs_main) at the top of this scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Indeed! Added. Fixed.

LOCK(cs_main);
pindexStart = chainActive.Genesis();
pChainTip = chainActive.Tip();

if (!request.params[0].isNull()) {
pindexStart = chainActive[request.params[0].get_int()];
if (!pindexStart) {
Expand All @@ -3418,10 +3427,12 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
throw JSONRPCError(RPC_INVALID_PARAMETER, "stop_height must be greater then start_height");
}
}
}

// We can't rescan beyond non-pruned blocks, stop and throw an error
if (fPruneMode) {
CBlockIndex *block = pindexStop ? pindexStop : chainActive.Tip();
LOCK(cs_main);
CBlockIndex *block = pindexStop ? pindexStop : pChainTip;
while (block && block->nHeight >= pindexStart->nHeight) {
if (!(block->nStatus & BLOCK_HAVE_DATA)) {
throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
Expand All @@ -3430,18 +3441,17 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
}
}

CBlockIndex *stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, true);
CBlockIndex *stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, true);
if (!stopBlock) {
if (pwallet->IsAbortingRescan()) {
throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted.");
}
// if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex
stopBlock = pindexStop ? pindexStop : chainActive.Tip();
stopBlock = pindexStop ? pindexStop : pChainTip;
}
else {
throw JSONRPCError(RPC_MISC_ERROR, "Rescan failed. Potentially corrupted data files.");
}

UniValue response(UniValue::VOBJ);
response.pushKV("start_height", pindexStart->nHeight);
response.pushKV("stop_height", stopBlock->nHeight);
Expand Down
12 changes: 9 additions & 3 deletions src/wallet/test/wallet_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,9 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
{
CWallet wallet;
AddKey(wallet, coinbaseKey);
BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip, nullptr));
WalletRescanReserver reserver(&wallet);
reserver.reserve();
BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip, nullptr, reserver));
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN);
}

Expand All @@ -397,7 +399,9 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
{
CWallet wallet;
AddKey(wallet, coinbaseKey);
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip, nullptr));
WalletRescanReserver reserver(&wallet);
reserver.reserve();
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip, nullptr, reserver));
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
}

Expand Down Expand Up @@ -608,7 +612,9 @@ class ListCoinsTestingSetup : public TestChain100Setup
bool firstRun;
wallet->LoadWallet(firstRun);
AddKey(*wallet, coinbaseKey);
wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr);
WalletRescanReserver reserver(wallet.get());
reserver.reserve();
wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver);
}

~ListCoinsTestingSetup()
Expand Down
Loading