Interrupt rescan on shutdown request #12507

Fixes #10987.

Here are the steps to test the feature:

1. start bitcoind, generate a couple of transactions and then stop:

bitcoind -regtest -printtoconsole
bitcoin-cli -regtest generate 100

2. apply the following patch

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2478d67ce..8f8cea40c 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1671,6 +1671,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
         }
         while (pindex && !fAbortRescan && !ShutdownRequested())
         {
+            MilliSleep(500);
             if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0) {
                 double gvp = 0;
                 {

3. start bitcoind with rescan flag, interrupt with CTRL+C and the output should look like:

bitcoind -regtest -printtoconsole -rescan
...
^C2018-02-22 01:00:55 AddToWallet e8bfb4501b630ad2acb91e88ab0112a779766536d2c564b04faae45ae90e18f7
2018-02-22 01:00:55 Rescan interrupted by shutdown request at block 5. Progress=1.000000
2018-02-22 01:00:55  rescan                 1774ms
2018-02-22 01:00:55 setKeyPool.size() = 1995
2018-02-22 01:00:55 mapWallet.size() = 10145
2018-02-22 01:00:55 mapAddressBook.size() = 3
2018-02-22 01:00:55 Shutdown: In progress...
2018-02-22 01:00:55 scheduler thread interrupt
2018-02-22 01:00:55 Shutdown: done

Seems not urgently important, but maybe worth to do. My only concern is to include init.h in wallet.cpp (since the long term goal is to make the wallet more independent from the sublayers).

I thought the concern was having the init depending on the wallet.

Rebased after #12287.

I wonder if the rescan should resume in the next run? The problem is the same if abortrescan is called - keys are stored but transactions can be missed.

I guess it fixes #12596.

utACK 3c647844d960c80f45f0359bcb311638f4a733b2

Am I reading this correctly, that the rescan would be aborted instead of paused (and resumed on the next run), similar to reindex? This might be confusing to users.

@MarcoFalke right, but if abortrescan is called, the rescan is also not resumed. Also, not interrupting might lead the user to force kill which is not desirable.

I can implement the "resume rescan" option here, just waiting for more feedback.

@MarcoFalke In what way do you think this is not expected behavior?

Since rescanning might take several hours, some users might prefer to shut down the node temporarily and then continue the rescan process whenever they start the node again. (Without having to manually specify rescanblockchain with a block height.)

Valid use cases are probably

• (1) abort rescan, (2) shut down (3) restart and don't resume rescan
• (1) shut down, (2) restart and resume rescan

Since the rescan in the gui is specifically advertised to be non-resumable, the following code can be used to fix the gui:

--- a/src/qt/splashscreen.cpp
+++ b/src/qt/splashscreen.cpp
@@ -225,8 +225,16 @@ void SplashScreen::paintEvent(QPaintEvent *event)
     painter.drawText(r, curAlignment, curMessage);
 }
 
-void SplashScreen::closeEvent(QCloseEvent *event)
+void SplashScreen::closeEvent(QCloseEvent* event)
 {
+#ifdef ENABLE_WALLET
+    for (CWallet* wallet : connectedWallets) {
+        if (wallet->IsScanning()) {
+            // We don't resume the ongoing rescan
+            wallet->AbortRescan();
+        }
+    }
+#endif
     StartShutdown(); // allows an "emergency" shutdown during startup
     event->ignore();
 }

I am not sure how to abort an ongoing rescan in bitcoind on startup, since the rpc will be started later.

I think the issue about interrupted re-scans should be tracked in a separate issue elsewhere. That's something I'm interested in working on at some point, i.e. in general if you shut down you node in the middle of a rescan it should make a best-effort to restart at the same point it was interrupted at.

Needs rebase utACK 3c647844d960c80f45f0359bcb311638f4a733b2 Agree with @eklitzke that interruptible rescans could be next and can be independent from this PR.

Rebased.