Skip to content

Commit 4d2d604

Browse files
ryanofskyryanofsky
committed
Fix importmulti failure to return rescan errors
An off-by-one-block bug in importmulti rescan logic could cause it to return success in an edge case even when a rescan was not successful. The case where this would happen is if there were multiple blocks in a row with the same GetBlockTimeMax() value, and the last block was scanned successfully, but one or more of the earlier blocks was not readable.
1 parent 41987aa commit 4d2d604

File tree

3 files changed

+34
-29
lines changed

3 files changed

+34
-29
lines changed

‎src/wallet/rpcdump.cpp‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,13 +1121,13 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
11211121

11221122
if (fRescan && fRunScan && requests.size()) {
11231123
CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(std::max<int64_t>(nLowestTimestamp - TIMESTAMP_WINDOW, 0)) : chainActive.Genesis();
1124-
CBlockIndex* scannedRange = nullptr;
1124+
CBlockIndex* scanFailed = nullptr;
11251125
if (pindex) {
1126-
scannedRange = pwallet->ScanForWalletTransactions(pindex, true);
1126+
scanFailed = pwallet->ScanForWalletTransactions(pindex, true);
11271127
pwallet->ReacceptWalletTransactions();
11281128
}
11291129

1130-
if (!scannedRange || scannedRange->nHeight > pindex->nHeight) {
1130+
if (scanFailed) {
11311131
std::vector<UniValue> results = response.getValues();
11321132
response.clear();
11331133
response.setArray();
@@ -1137,12 +1137,23 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
11371137
// range, or if the import result already has an error set, let
11381138
// the result stand unmodified. Otherwise replace the result
11391139
// with an error message.
1140-
if (GetImportTimestamp(request, now) - TIMESTAMP_WINDOW >= scannedRange->GetBlockTimeMax() || results.at(i).exists("error")) {
1140+
if (GetImportTimestamp(request, now) - TIMESTAMP_WINDOW > scanFailed->GetBlockTimeMax() || results.at(i).exists("error")) {
11411141
response.push_back(results.at(i));
11421142
} else {
11431143
UniValue result = UniValue(UniValue::VOBJ);
11441144
result.pushKV("success", UniValue(false));
1145-
result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, strprintf("Failed to rescan before time %d, transactions may be missing.", scannedRange->GetBlockTimeMax())));
1145+
result.pushKV(
1146+
"error",
1147+
JSONRPCError(
1148+
RPC_MISC_ERROR,
1149+
strprintf("Rescan failed for key with creation timestamp %d. There was an error reading a "
1150+
"block from time %d, which is after or within %d seconds of key creation, and "
1151+
"could contain transactions pertaining to the key. As a result, transactions "
1152+
"and coins using this key may not appear in the wallet. This error could be "
1153+
"caused by pruning or data corruption (see bitcoind log for details) and could "
1154+
"be dealt with by downloading and rescanning the relevant blocks (see -reindex "
1155+
"and -rescan options).",
1156+
GetImportTimestamp(request, now), scanFailed->GetBlockTimeMax(), TIMESTAMP_WINDOW)));
11461157
response.push_back(std::move(result));
11471158
}
11481159
++i;

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
364364
LOCK(cs_main);
365365

366366
// Cap last block file size, and mine new block in a new block file.
367+
CBlockIndex* const nullBlock = nullptr;
367368
CBlockIndex* oldTip = chainActive.Tip();
368369
GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE;
369370
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
@@ -375,7 +376,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
375376
CWallet wallet;
376377
LOCK(wallet.cs_wallet);
377378
wallet.AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
378-
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip));
379+
BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip));
379380
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN);
380381
}
381382

@@ -389,7 +390,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
389390
CWallet wallet;
390391
LOCK(wallet.cs_wallet);
391392
wallet.AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
392-
BOOST_CHECK_EQUAL(newTip, wallet.ScanForWalletTransactions(oldTip));
393+
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip));
393394
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
394395
}
395396

@@ -413,28 +414,25 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
413414
CKey futureKey;
414415
futureKey.MakeNewKey(true);
415416
key.pushKV("scriptPubKey", HexStr(GetScriptForRawPubKey(futureKey.GetPubKey())));
416-
key.pushKV("timestamp", newTip->GetBlockTimeMax() + TIMESTAMP_WINDOW);
417+
key.pushKV("timestamp", newTip->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1);
417418
key.pushKV("internal", UniValue(true));
418419
keys.push_back(key);
419420
JSONRPCRequest request;
420421
request.params.setArray();
421422
request.params.push_back(keys);
422423

423424
UniValue response = importmulti(request);
424-
BOOST_CHECK_EQUAL(response.write(), strprintf("[{\"success\":false,\"error\":{\"code\":-1,\"message\":\"Failed to rescan before time %d, transactions may be missing.\"}},{\"success\":true}]", newTip->GetBlockTimeMax()));
425+
BOOST_CHECK_EQUAL(response.write(),
426+
strprintf("[{\"success\":false,\"error\":{\"code\":-1,\"message\":\"Rescan failed for key with creation "
427+
"timestamp %d. There was an error reading a block from time %d, which is after or within %d "
428+
"seconds of key creation, and could contain transactions pertaining to the key. As a result, "
429+
"transactions and coins using this key may not appear in the wallet. This error could be caused "
430+
"by pruning or data corruption (see bitcoind log for details) and could be dealt with by "
431+
"downloading and rescanning the relevant blocks (see -reindex and -rescan "
432+
"options).\"}},{\"success\":true}]",
433+
0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW));
425434
::pwalletMain = backup;
426435
}
427-
428-
// Verify ScanForWalletTransactions does not return null when the scan is
429-
// elided due to the nTimeFirstKey optimization.
430-
{
431-
CWallet wallet;
432-
{
433-
LOCK(wallet.cs_wallet);
434-
wallet.UpdateTimeFirstKey(newTip->GetBlockTime() + 7200 + 1);
435-
}
436-
BOOST_CHECK_EQUAL(newTip, wallet.ScanForWalletTransactions(newTip));
437-
}
438436
}
439437

440438
// Check that GetImmatureCredit() returns a newly calculated value instead of

‎src/wallet/wallet.cpp‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1456,18 +1456,17 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
14561456
* from or to us. If fUpdate is true, found transactions that already
14571457
* exist in the wallet will be updated.
14581458
*
1459-
* Returns pointer to the first block in the last contiguous range that was
1460-
* successfully scanned or elided (elided if pIndexStart points at a block
1461-
* before CWallet::nTimeFirstKey). Returns null if there is no such range, or
1462-
* the range doesn't include chainActive.Tip().
1459+
* Returns null if scan was successful. Otherwise, if a complete rescan was not
1460+
* possible (due to pruning or corruption), returns pointer to the most recent
1461+
* block that could not be scanned.
14631462
*/
14641463
CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
14651464
{
14661465
int64_t nNow = GetTime();
14671466
const CChainParams& chainParams = Params();
14681467

14691468
CBlockIndex* pindex = pindexStart;
1470-
CBlockIndex* ret = pindexStart;
1469+
CBlockIndex* ret = nullptr;
14711470
{
14721471
LOCK2(cs_main, cs_wallet);
14731472
fAbortRescan = false;
@@ -1495,11 +1494,8 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
14951494
for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
14961495
AddToWalletIfInvolvingMe(block.vtx[posInBlock], pindex, posInBlock, fUpdate);
14971496
}
1498-
if (!ret) {
1499-
ret = pindex;
1500-
}
15011497
} else {
1502-
ret = nullptr;
1498+
ret = pindex;
15031499
}
15041500
pindex = chainActive.Next(pindex);
15051501
}

0 commit comments

Comments
 (0)