Interrupt rescan on shutdown request #12507

1. 
   promag commented at 1:01 am on February 22, 2018: member

   Fixes #10987.

   Here are the steps to test the feature:

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

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

   2. apply the following patch

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

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

    0bitcoind -regtest -printtoconsole -rescan
    1...
    2^C2018-02-22 01:00:55 AddToWallet e8bfb4501b630ad2acb91e88ab0112a779766536d2c564b04faae45ae90e18f7
    32018-02-22 01:00:55 Rescan interrupted by shutdown request at block 5. Progress=1.000000
    42018-02-22 01:00:55  rescan                 1774ms
    52018-02-22 01:00:55 setKeyPool.size() = 1995
    62018-02-22 01:00:55 mapWallet.size() = 10145
    72018-02-22 01:00:55 mapAddressBook.size() = 3
    82018-02-22 01:00:55 Shutdown: In progress...
    92018-02-22 01:00:55 scheduler thread interrupt
   102018-02-22 01:00:55 Shutdown: done
   

2. fanquake added the label Wallet on Feb 22, 2018
3. 
   jonasschnelli commented at 0:43 am on February 25, 2018: contributor

   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).
4. 
   promag commented at 11:25 am on February 25, 2018: member

   I thought the concern was having the init depending on the wallet.
5. promag force-pushed on Feb 25, 2018
6. 
   promag commented at 11:30 am on February 25, 2018: member

   Rebased after #12287.
7. aikkyu35 approved
8. 
   promag commented at 11:22 pm on February 25, 2018: member

   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.
9. 
   promag commented at 9:18 pm on March 7, 2018: member

   I guess it fixes #12596.
10. eklitzke approved
11. 
    eklitzke commented at 7:57 am on March 11, 2018: contributor

    utACK 3c647844d960c80f45f0359bcb311638f4a733b2
12. 
    MarcoFalke commented at 4:15 pm on March 12, 2018: member

    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.
13. 
    promag commented at 4:23 pm on March 12, 2018: member

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

14. 
    sipa commented at 0:01 am on March 17, 2018: member

    @MarcoFalke In what way do you think this is not expected behavior?
15. 
    MarcoFalke commented at 3:50 pm on March 17, 2018: member

    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
16. 
    MarcoFalke commented at 4:19 pm on March 17, 2018: member

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

     0--- a/src/qt/splashscreen.cpp
     1+++ b/src/qt/splashscreen.cpp
     2@@ -225,8 +225,16 @@ void SplashScreen::paintEvent(QPaintEvent *event)
     3     painter.drawText(r, curAlignment, curMessage);
     4 }
     5 
     6-void SplashScreen::closeEvent(QCloseEvent *event)
     7+void SplashScreen::closeEvent(QCloseEvent* event)
     8 {
     9+#ifdef ENABLE_WALLET
    10+    for (CWallet* wallet : connectedWallets) {
    11+        if (wallet->IsScanning()) {
    12+            // We don't resume the ongoing rescan
    13+            wallet->AbortRescan();
    14+        }
    15+    }
    16+#endif
    17     StartShutdown(); // allows an "emergency" shutdown during startup
    18     event->ignore();
    19 }
    

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

17. 
    eklitzke commented at 7:24 am on March 19, 2018: contributor

    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.
18. 
    jonasschnelli commented at 6:05 pm on April 15, 2018: contributor

    Needs rebase utACK 3c647844d960c80f45f0359bcb311638f4a733b2 Agree with @eklitzke that interruptible rescans could be next and can be independent from this PR.
19. promag force-pushed on Apr 27, 2018
20. 
    promag commented at 9:02 am on April 27, 2018: member

    Rebased.
21. wallet: Interrupt rescan on shutdown request c4fda7672a
22. promag force-pushed on May 2, 2018
23. laanwj merged this on May 3, 2018
24. laanwj closed this on May 3, 2018

    ---

25. laanwj referenced this in commit 2afdc29403 on May 3, 2018
26. in src/wallet/wallet.cpp:1799 in c4fda7672a

    1794@@ -1794,6 +1795,8 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
    1795         }
    1796         if (pindex && fAbortRescan) {
    1797             LogPrintf("Rescan aborted at block %d. Progress=%f\n", pindex->nHeight, gvp);
    1798+        } else if (pindex && ShutdownRequested()) {
    1799+            LogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", pindex->nHeight, gvp);
    

    ---

    ---

    

    MarcoFalke commented at 2:47 pm on May 4, 2018:

    Doesn’t interrupt mean it will continue later on? I think this should say “aborted”.
27. promag deleted the branch on May 23, 2018
28. jasonbcox referenced this in commit f3cacd5064 on Sep 13, 2019
29. PastaPastaPasta referenced this in commit 87fa33e2b3 on Jun 17, 2020
30. PastaPastaPasta referenced this in commit 18e62716f6 on Jun 27, 2020
31. PastaPastaPasta referenced this in commit bf12586f01 on Jun 28, 2020
32. MarcoFalke locked this on Sep 8, 2021

Contributors
promag jonasschnelli aikkyu35 eklitzke MarcoFalke sipa

Labels
Wallet