The wallet importprivkey command triggers a rescan of the entire block chain unless explicitly asked not to. This oftentimes is not intentional, and there has so far not existed a way to abort a rescan in progress.
This PR adds support for a new RPC command abortrescan which will prompt ScanForWalletTransactions to stop scanning.
166+ "abortrescan\n"
167+ "\nStops current wallet rescan triggered e.g. by an importprivkey call.\n"
168+ );
169+
170+ if (!pwallet->fScanningWallet) throw JSONRPCError(RPC_WALLET_ERROR, "No wallet rescans in progress.");
171+ if (pwallet->fAbortRescan) throw JSONRPCError(RPC_WALLET_ERROR, "Rescan abort already in progress.");
It’s a response to the RPC caller, so it’s more like feedback and less than an actual error. Compare this to the std::runtime_error thrown for the help message.
Ultimately, if the scan is not in progress and/or if the rescan flag is already set, the end results will be the same even without throwing.
1559@@ -1558,7 +1560,12 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
1560 LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, GuessVerificationProgress(chainParams.TxData(), pindex));
1561 }
1562 }
1563+ if (pindex && fAbortRescan) {
need to set fAbortRescan to false again.
EDIT: Actually it should be fine… I think it is best that the end state of the variable is known though.
168+ );
169+
170+ if (!pwallet->fScanningWallet) throw JSONRPCError(RPC_WALLET_ERROR, "No wallet rescans in progress.");
171+ if (pwallet->fAbortRescan) throw JSONRPCError(RPC_WALLET_ERROR, "Rescan abort already in progress.");
172+ pwallet->fAbortRescan = true;
173+ return "Wallet rescan aborted.";
RPC_WALLET_ERROR for fScanningWallet and fAbortRescan leading to the fact the one needs to parse string to get the correct state.
162+ }
163+
164+ if (request.fHelp || request.params.size() > 0)
165+ throw std::runtime_error(
166+ "abortrescan\n"
167+ "\nStops current wallet rescan triggered e.g. by an importprivkey call.\n"
HelpExampleCli and HelpExampleRpc here.
@jonasschnelli I tested out the GUI. The only way I could find importing keys was the Help > Debug > Console > importprivkey … route. It brings up the “Rescanning…” popup with the progress bar, and locks input so I can’t abort the rescan to test that part. But the actual GUI works fine.
Adding an “Abort” button to the progress popup seems sensible, but I’m going to declare that as a separate PR for now as I don’t immediately see a neat solution to doing so.
./bitcoin-qt -server or ./bitcoin-qt -rescan.
“Wallet is being asked to abort rescan despite already pending rescan abort”
I would change “abort” to “abortion”; I believe it may be more grammatically correct, and we need the word abortion in the core code.
722@@ -723,6 +723,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
723 */
724 mutable CCriticalSection cs_wallet;
725
726+ std::atomic<bool> fAbortRescan;
172+ + HelpExampleCli("abortrescan", "") +
173+ "\nAs a JSON-RPC call\n"
174+ + HelpExampleRpc("abortrescan", "")
175+ );
176+
177+ if (!pwallet->IsScanning()) throw JSONRPCError(RPC_WALLET_NOT_SCANNING, "No wallet rescans in progress.");
I can’t find the previous comment you are referring to? (Did you get this and #9622 (review) mixed up, by any chance?)
Since we’re throwing when a user types the word help, it conceptually feels like a throw is simply a result with a code and a message, rather than a critical error. It’s a way to distinguish from “ACK” and “NACK” even for trivial reasons. Compare e.g. the addnode RPC which throws an error when a node being added is already there, or when a node being removed is not in the list. That feels similarly non-critical to me – end result is the same – but could be harder to code for without error codes if we changed to returning a success bool or something.
I guess the worst thing that happens is that you accidentally target the wrong node and one node ends up scanning the entire wallet even though you asked it not to. That’s not a biggie, really, especially if you don’t even realize it yourself.
I’ll switch to returning true/false for did/didn’t do something as you suggest.
Per #10208 (review) I have replaced the RPC throws with true/false for “did something” / “did nothing” return values in abortrescan.
This still needs (in follow-up PR):
test/functional