[wallet] Rescan abortability #10208

kallewoof · 2017-04-14T06:48:01Z

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.

NicolasDorier · 2017-04-14T06:53:09Z

src/wallet/rpcdump.cpp

+        );
+
+    if (!pwallet->fScanningWallet) throw JSONRPCError(RPC_WALLET_ERROR, "No wallet rescans in progress.");
+    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.

NicolasDorier · 2017-04-14T06:54:23Z

src/wallet/wallet.cpp

@@ -1558,7 +1560,12 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
                LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, GuessVerificationProgress(chainParams.TxData(), pindex));
            }
        }
+        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.

NicolasDorier · 2017-04-14T06:58:18Z

src/wallet/rpcdump.cpp

+    if (!pwallet->fScanningWallet) throw JSONRPCError(RPC_WALLET_ERROR, "No wallet rescans in progress.");
+    if (pwallet->fAbortRescan) throw JSONRPCError(RPC_WALLET_ERROR, "Rescan abort already in progress.");
+    pwallet->fAbortRescan = true;
+    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.

jonasschnelli · 2017-04-14T07:05:11Z

src/wallet/rpcdump.cpp

+    if (request.fHelp || request.params.size() > 0)
+        throw std::runtime_error(
+            "abortrescan\n"
+            "\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.

jonasschnelli · 2017-04-14T07:09:05Z

Nice. Concept ACK.
Have you tested this together with the GUI? Are you willing to expand this to the GUI?

jonasschnelli · 2017-04-14T07:09:23Z

Accidentally closed. Reopening.

kallewoof · 2017-04-14T07:11:57Z

@jonasschnelli I haven't tried with the GUI. Definitely willing to add that, but wonder if that should be a separate PR?

jonasschnelli · 2017-04-14T07:13:02Z

Can be separate if this PR does not break the GUI rescan additions (the popup window).

kallewoof · 2017-04-14T07:14:19Z

Gotcha. I will test the GUI. If it doesn't seem overly complex I'll add support there as well while at it.

kallewoof · 2017-04-14T07:35:12Z

@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.

sipa · 2017-04-14T07:36:23Z

@kallewoof You can run bitcoin-qt with the -server flag, and then issue command through bitcoin-cli or any other RPC client.

jonasschnelli · 2017-04-14T07:36:45Z

@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.

kallewoof · 2017-04-14T07:41:51Z

@sipa Ahh that let me try abortrescan from CLI while doing import from the GUI. Thanks!

@jonasschnelli GUI fully tested and appears functional.

NicolasDorier · 2017-04-14T10:04:06Z

utACK 33b93f52b4452008075520239f4991440380be1a

KibbledJiveElkZoo · 2017-04-14T19:45:48Z

"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.

rawodb · 2017-04-14T20:18:14Z

src/wallet/wallet.h

@@ -723,6 +723,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     */
    mutable CCriticalSection cs_wallet;

+    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!

gmaxwell

I wish we didn't need this, but we do. Concept ACK.

rawodb · 2017-04-17T08:39:03Z

ACK

laanwj · 2017-04-17T13:12:41Z

nice! utACK after squash

kallewoof · 2017-04-17T14:27:04Z

Squashed. ~~~Edit: Crap. Fixing...~~~

TheBlueMatt

One comment, utACK otherwise.

TheBlueMatt · 2017-04-17T19:14:37Z

src/wallet/rpcdump.cpp

+            + HelpExampleRpc("abortrescan", "")
+        );
+
+    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 (comment) 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 (comment) 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.

kallewoof · 2017-04-18T02:55:29Z

Per #10208 (comment) I have replaced the RPC throws with true/false for "did something" / "did nothing" return values in abortrescan.

Unsquashed history: 1 → 2 → 3∈2

9141622 [rpc] Add abortrescan command to RPC interface. (Kalle Alm) 75a08e7 [wallet] Add support for aborting wallet transaction rescans. (Kalle Alm) Tree-SHA512: 18545a1dc48c6dc112993f971f3adc7c0341fa621186c6d70bef1052e1d127ca116c5769595d721a209d502ca2019f2ad33876fe35d2b17216393404607a6c76

laanwj · 2017-04-18T06:06:17Z

This still needs (in follow-up PR):

a test in test/functional
mention in the release notes

TheBlueMatt · 2017-04-18T19:17:44Z

postumous utACK

9141622 [rpc] Add abortrescan command to RPC interface. (Kalle Alm) 75a08e7 [wallet] Add support for aborting wallet transaction rescans. (Kalle Alm) Tree-SHA512: 18545a1dc48c6dc112993f971f3adc7c0341fa621186c6d70bef1052e1d127ca116c5769595d721a209d502ca2019f2ad33876fe35d2b17216393404607a6c76

…nality 896520e [rpc] Add abortrescan command to RPC interface. (kallewoof) ab41ad8 [wallet] Add support for aborting wallet transaction rescans. (furszy) 25440c4 test: wallet_hd.py enable RPC based rescan test case (furszy) d3cdc63 wallet: add rescanblockchain <start_height> <stop_height> RPC command. (furszy) 4d0fa79 wallet: ScanForWalletTransactions returning the last unsuccessfully scanned block or nullptr if scan went well. (furszy) Pull request description: Added the following changes: 1) The wallet scan chain for transactions process will now return null if scan was successful. Otherwise, if a complete rescan was not possible (due to corruption or abort), returns pointer to the most recent block that could not be scanned. 2) New `rescanblockchain` RPC command with two arguments `start_height` `end_height` to trigger a chain rescan at any time during runtime + test case (adaptation of bitcoin#7061). 3) New `abortrescan` RPC command to stop any long running rescan if needed at runtime (this is helpful for example in case of importing a private/sapling key that if the user is not aware of the extra "scan" boolean argument, it will rescan the complete chain). Coming from bitcoin#10208. ACKs for top commit: random-zebra: ACK 896520e Fuzzbawls: ACK 896520e Tree-SHA512: ae7f3a7122d9d140f840154d1cfa703e2ed4d90f37b308da3765cb5bc4760229c26b8a49e835bae056f82f789d7b3b9b0d47859f74222db8d41005c748b51f61

fanquake added RPC/REST/ZMQ Wallet labels Apr 14, 2017

kallewoof force-pushed the rescan-abortability branch 2 times, most recently from 31cb886 to a9fd7c8 Compare April 14, 2017 06:51

NicolasDorier reviewed Apr 14, 2017

View reviewed changes

jonasschnelli reviewed Apr 14, 2017

View reviewed changes

jonasschnelli closed this Apr 14, 2017

jonasschnelli reopened this Apr 14, 2017

kallewoof force-pushed the rescan-abortability branch from a9fd7c8 to 9113eb4 Compare April 14, 2017 07:10

kallewoof force-pushed the rescan-abortability branch from fd7f1c1 to 33b93f5 Compare April 14, 2017 07:46

sipa mentioned this pull request Apr 14, 2017

Abortable rescans & rpc abortrescan command #10214

Closed

rawodb approved these changes Apr 14, 2017

View reviewed changes

rawodb reviewed Apr 14, 2017

View reviewed changes

gmaxwell reviewed Apr 15, 2017

View reviewed changes

kallewoof force-pushed the rescan-abortability branch from 0c9af66 to ae68fa7 Compare April 17, 2017 13:38

kallewoof force-pushed the rescan-abortability branch from ae68fa7 to b0375a8 Compare April 17, 2017 14:32

[wallet] Add support for aborting wallet transaction rescans.

75a08e7

kallewoof force-pushed the rescan-abortability branch from b0375a8 to 153d24d Compare April 17, 2017 14:36

TheBlueMatt reviewed Apr 17, 2017

View reviewed changes

[rpc] Add abortrescan command to RPC interface.

9141622

kallewoof force-pushed the rescan-abortability branch from 153d24d to 9141622 Compare April 18, 2017 02:42

laanwj merged commit 9141622 into bitcoin:master Apr 18, 2017

laanwj mentioned this pull request Apr 18, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

kallewoof deleted the rescan-abortability branch April 18, 2017 06:38

jnewbery mentioned this pull request Oct 2, 2017

[Wallet] Add RPC call "rescanblockchain <startheight> <stopheight>" #7061

Merged

furszy mentioned this pull request Mar 10, 2021

[RPC][wallet] Add rescanblockchain RPC command functionality PIVX-Project/PIVX#2240

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[wallet] Rescan abortability #10208

[wallet] Rescan abortability #10208

Conversation

kallewoof commented Apr 14, 2017

Choose a reason for hiding this comment

kallewoof Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

NicolasDorier Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Apr 14, 2017

jonasschnelli commented Apr 14, 2017

kallewoof commented Apr 14, 2017

jonasschnelli commented Apr 14, 2017

kallewoof commented Apr 14, 2017

kallewoof commented Apr 14, 2017

sipa commented Apr 14, 2017

jonasschnelli commented Apr 14, 2017

kallewoof commented Apr 14, 2017

NicolasDorier commented Apr 14, 2017

KibbledJiveElkZoo commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell left a comment

Choose a reason for hiding this comment

rawodb commented Apr 17, 2017

laanwj commented Apr 17, 2017

kallewoof commented Apr 17, 2017 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kallewoof commented Apr 18, 2017

laanwj commented Apr 18, 2017

TheBlueMatt commented Apr 18, 2017

kallewoof Apr 14, 2017 •

edited

Loading

NicolasDorier Apr 14, 2017 •

edited

Loading

kallewoof commented Apr 17, 2017 •

edited

Loading