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.");
nits: I don't think it should throw an error. The expected state is correctly reached at the end of the call.
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.
It's set at line 1530 above. I was thinking of setting it to false at end but it doesn't really matter as it's reset at top anyway.
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.";
returning a human readable message feel a bit weird to me.
I was sure I'd seen that elsewhere but now I'm not so sure. Revisiting that part now.
From the API consumer perspective, what you want is parsable differentiations between the possible states.
Your currently use RPC_WALLET_ERROR for fScanningWallet and fAbortRescan leading to the fact the one needs to parse string to get the correct state.
Right, good point -- I will add error codes for these two and throw those instead.
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"
nit: Usually we have the HelpExampleCli and HelpExampleRpc here.
@jonasschnelli I wasn't sure that was the case even for no-arg commands. Will add.
Added.
Nice. Concept ACK. Have you tested this together with the GUI? Are you willing to expand this to the GUI?
Accidentally closed. Reopening.
@jonasschnelli I haven't tried with the GUI. Definitely willing to add that, but wonder if that should be a separate PR?
Can be separate if this PR does not break the GUI rescan additions (the popup window).
Gotcha. I will test the GUI. If it doesn't seem overly complex I'll add support there as well while at it.
@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.
@kallewoof You can run bitcoin-qt with the -server flag, and then issue command through bitcoin-cli or any other RPC client.
@kallewoof: Great. Yes. Let's fix this in an upcoming PR. I think you can also rescan by ./bitcoin-qt -server or ./bitcoin-qt -rescan.
@sipa Ahh that let me try abortrescan from CLI while doing import from the GUI. Thanks! @jonasschnelli GUI fully tested and appears functional.
utACK 33b93f52b4452008075520239f4991440380be1a
"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;
I would turn both these flags to private standard boolean and use CWallet getters&setters to change them
For some reason I didn't think we used getters/setters but clearly we do. Fixed, thanks!
I wish we didn't need this, but we do. Concept ACK.
ACK
nice! utACK after squash
Squashed. ~~~Edit: Crap. Fixing...~~~
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 have to agree with the previous comment, throwing here is super strange. The direct API we expose is likely fine, but many API wrappers will throw a corresponding exception like we do here, and I'd consider "not currently scanning" to be a warning, not a critical error - it would be nice to just return a boolean for "we did something".
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 was referring to #10208 (review) The help throw, however, is for when you provide bad arguments, which is the correct "you fucked up" response. Throwing if not scanning is just a race in client applications, an almost-guaranteed-benign race to boot.
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.
One comment, utACK otherwise.
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
postumous utACK