Allow for aborting rescans in the GUI

achow101 commented at 0:41 am on August 31, 2017: member

A cancel button is added to the showProgress dialog that is used only for rescans. When clicked, AbortRescan is called directly to cancel the rescan.

Rescans triggered from the debug console will now be cancelable by clicking the cancel button.

Rescans triggered by a command (e.g. importmulti) will now give an error indicating that the rescan was aborted by the user (either by the abortrescan command or by clicking cancel).

achow101 force-pushed on Aug 31, 2017

in src/qt/bitcoingui.cpp:1134 in e6e8b0c259 outdated

1131-        progressDialog->setCancelButton(0);
1132         progressDialog->setAutoClose(false);
1133         progressDialog->setValue(0);
1134+
1135+        if (cancel) {
1136+            progressDialog->setCancelButtonText("Cancel");

meshcollider commented at 6:39 am on August 31, 2017:

Does this “Cancel” need a translation?

achow101 commented at 5:37 pm on August 31, 2017:

Yes, it does. Done

meshcollider commented at 6:46 am on August 31, 2017: contributor

utACK modulo Travis issues

in src/qt/bitcoingui.cpp:1150 in e6e8b0c259 outdated

1143@@ -1138,8 +1144,14 @@ void BitcoinGUI::showProgress(const QString &title, int nProgress)
1144             progressDialog->deleteLater();
1145         }
1146     }
1147-    else if (progressDialog)
1148-        progressDialog->setValue(nProgress);
1149+    else if (progressDialog) {
1150+        if (progressDialog->wasCanceled()) {
1151+            cancel();
1152+        }

promag commented at 8:13 am on August 31, 2017:

} else {

achow101 commented at 5:35 pm on August 31, 2017:

Done

in src/qt/bitcoingui.cpp:1123 in e6e8b0c259 outdated

1119@@ -1119,16 +1120,21 @@ void BitcoinGUI::detectShutdown()
1120     }
1121 }
1122 
1123-void BitcoinGUI::showProgress(const QString &title, int nProgress)
1124+void BitcoinGUI::showProgress(const QString &title, int nProgress, const std::function<void(void)> &cancel)

promag commented at 8:15 am on August 31, 2017:

& are next to the type. Same for remaining changes.

achow101 commented at 5:35 pm on August 31, 2017:

Done

promag commented at 8:25 am on August 31, 2017: member

Should it be possible to abort rescans caused by the RPC calls?

I’m not very fond of callbacks. Have you thought in alternatives?

achow101 commented at 1:52 pm on August 31, 2017: member

I don’t know what is causing the travis failures.

Should it be possible to abort rescans caused by the RPC calls?

Yes. There is an abortrescan RPC call that lets you abort a rescan. But it is not accessible from the GUI.

in src/qt/bitcoingui.cpp:491 in 9fbc64546f outdated

486@@ -487,7 +487,7 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
487         connect(_clientModel, SIGNAL(message(QString,QString,unsigned int)), this, SLOT(message(QString,QString,unsigned int)));
488 
489         // Show progress dialog
490-        connect(_clientModel, SIGNAL(showProgress(QString,int)), this, SLOT(showProgress(QString,int)));
491+        connect(_clientModel, SIGNAL(showProgress(QString,int, std::function<void(void)>)), this, SLOT(showProgress(QString,int, std::function<void(void)>)));

ryanofsky commented at 3:26 pm on August 31, 2017:

In commit “Add cancel function reference to ShowProgress”

A typedef or using declaration for std::function<void(void)> might make the changes throughout this commit be more readable, and also make this code less fragile because SIGNAL and SLOT macros rely on string comparison.

in src/txdb.cpp:389 in 9fbc64546f outdated

385@@ -386,7 +386,7 @@ bool CCoinsViewDB::Upgrade() {
386             if (count++ % 256 == 0) {
387                 uint32_t high = 0x100 * *key.second.begin() + *(key.second.begin() + 1);
388                 int percentageDone = (int)(high * 100.0 / 65536.0 + 0.5);
389-                uiInterface.ShowProgress(_("Upgrading UTXO database") + "\n"+ _("(press q to shutdown and continue later)") + "\n", percentageDone);
390+                uiInterface.ShowProgress(_("Upgrading UTXO database") + "\n"+ _("(press q to shutdown and continue later)") + "\n", percentageDone, std::function<void(void)>());

ryanofsky commented at 4:05 pm on August 31, 2017:

In commit “Add cancel function reference to ShowProgress”

No need to write std::function<void(void)>() here and 12 other places. You can just pass {} or nullptr.

achow101 commented at 5:35 pm on August 31, 2017:

Oh, I didn’t realize that that would work. Done.

in src/qt/bitcoingui.cpp:1187 in b9c077ff98 outdated

1143@@ -1139,8 +1144,14 @@ void BitcoinGUI::showProgress(const QString &title, int nProgress, const std::fu
1144             progressDialog->deleteLater();
1145         }
1146     }
1147-    else if (progressDialog)
1148-        progressDialog->setValue(nProgress);
1149+    else if (progressDialog) {
1150+        if (progressDialog->wasCanceled()) {
1151+            cancel();

ryanofsky commented at 4:07 pm on August 31, 2017:

In commit “Add cancel button to rescan progress dialog”

Maybe assert(cancel)

ryanofsky commented at 4:15 pm on August 31, 2017: member

utACK e6e8b0c259594b09e405fc9123d1f8473e42b0e4 with RPC bugfixes

in src/wallet/rpcdump.cpp:560 in e6e8b0c259 outdated

555@@ -535,7 +556,14 @@ UniValue importwallet(const JSONRPCRequest& request)
556     file.close();
557     pwallet->ShowProgress("", 100, std::function<void(void)>()); // hide progress dialog in GUI
558     pwallet->UpdateTimeFirstKey(nTimeBegin);
559-    pwallet->RescanFromTime(nTimeBegin, false /* update */);
560+    int64_t scanned_time = pwallet->RescanFromTime(nTimeBegin, false /* update */);
561+    if (scanned_time > TIMESTAMP_MIN) {

ryanofsky commented at 4:53 pm on August 31, 2017:

In commit “Give an error when rescan is aborted by the user”

s/TIMESTAMP_MIN/nTimeBegin/ here to fix test failures

achow101 commented at 5:35 pm on August 31, 2017:

Done

achow101 force-pushed on Aug 31, 2017

ryanofsky commented at 6:54 pm on August 31, 2017: member

utACK c8720f0a320eff0cc83a459dc87f28087d518825. Only changes were implementing various suggestions.

I’m not very fond of callbacks. Have you thought in alternatives?

Reason callback seems to be used is to avoid the need for qt code to call CWallet’s AbortRescan method or set the fAbortRescan flag directly. The alternatives would be for Qt to call a different function or set a different flag, but don’t seem like they would be much better or worse.

in src/wallet/wallet.cpp:1571 in c8720f0a32 outdated

1567@@ -1568,13 +1568,13 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
1568         fAbortRescan = false;
1569         fScanningWallet = true;
1570 
1571-        ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup
1572+        ShowProgress(_("Rescanning..."), 0, boost::bind(&CWallet::AbortRescan, this)); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup

jonasschnelli commented at 5:10 pm on September 3, 2017:

nit: use std::bind (and below)

achow101 commented at 7:51 pm on September 5, 2017:

Done

in src/ui_interface.h:98 in c8720f0a32 outdated

94@@ -95,7 +95,7 @@ class CClientUIInterface
95     boost::signals2::signal<void (CWallet* wallet)> LoadWallet;
96 
97     /** Show progress e.g. for verifychain */
98-    boost::signals2::signal<void (const std::string &title, int nProgress)> ShowProgress;
99+    boost::signals2::signal<void (const std::string &title, int nProgress, const std::function<void(void)>& cancel)> ShowProgress;

jonasschnelli commented at 5:12 pm on September 3, 2017:

use nullptr as default parameter for cancel to avoid adding nullptr to all non-callback ShowProgress calls?

achow101 commented at 5:11 pm on September 5, 2017:

I couldn’t set a default parameter in the signal, so I’m not sure it is possible to do that. Or I’m just doing something wrong.

jonasschnelli commented at 5:12 pm on September 3, 2017: contributor

Concept ACK

achow101 force-pushed on Sep 5, 2017

laanwj added the label GUI on Sep 6, 2017

jonasschnelli commented at 11:59 pm on September 6, 2017: contributor

Tested a bit and there is a concurrency issue (maybe not related to this code but since you finally can test multiple rescans during the same runtime this PR may reveal the issue):

Sometimes the progress window does not show up (happened to me only if I canceled a rescan before) and the GUI is fully in locked state (due to the rescan). In that case, the rescan is happening and the GUI is locked (without showing a progress screen) via cs_main in transactiontablemodel.cpp

jonasschnelli commented at 5:17 pm on September 7, 2017: contributor

Needs rebase

achow101 force-pushed on Sep 7, 2017

in src/qt/bitcoingui.cpp:490 in cb49c4fa5b outdated

486@@ -487,7 +487,7 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
487         connect(_clientModel, SIGNAL(message(QString,QString,unsigned int)), this, SLOT(message(QString,QString,unsigned int)));
488 
489         // Show progress dialog
490-        connect(_clientModel, SIGNAL(showProgress(QString,int)), this, SLOT(showProgress(QString,int)));
491+        connect(_clientModel, SIGNAL(showProgress(QString,int, bool, std::function<void(void)>)), this, SLOT(showProgress(QString,int, std::function<void(void)>)));

ryanofsky commented at 6:47 pm on October 12, 2017:

Could just write void() instead of void(void) everywhere

achow101 commented at 6:21 pm on October 31, 2017:

Done

in src/qt/splashscreen.cpp:184 in cb49c4fa5b outdated

180@@ -181,7 +181,7 @@ static void ShowProgress(SplashScreen *splash, const std::string &title, int nPr
181 #ifdef ENABLE_WALLET
182 void SplashScreen::ConnectWallet(CWallet* wallet)
183 {
184-    wallet->ShowProgress.connect(boost::bind(ShowProgress, this, _1, _2, false));
185+    wallet->ShowProgress.connect(boost::bind(ShowProgress, this, _1, _2, false, _4));

ryanofsky commented at 7:01 pm on October 12, 2017:

You added both resume_possible and cancel arguments to the ShowProgress signal in wallet.h so this should be _3, _4, instead of false, _4.

(Another alternative if you want to ensure wallet resume_possible value is always false, is to not add resume_possible in wallet.h, and then change this to false, _3. But adding the argument and then silently ignoring like this does now isn’t good.)

achow101 commented at 6:21 pm on October 31, 2017:

Done

achow101 commented at 6:40 pm on October 31, 2017:

Actually changing this seems to be causing a segfault, so I will leave it as is.

MarcoFalke commented at 9:20 pm on March 18, 2018:

The following diff appears to be working for me. Could you help me find the steps to reproduce the segfault? Otherwise, I think this is a sensible suggestion.

 0--- a/src/qt/splashscreen.cpp
 1+++ b/src/qt/splashscreen.cpp
 2@@ -181,7 +181,7 @@ static void ShowProgress(SplashScreen *splash, const std::string &title, int nPr
 3 #ifdef ENABLE_WALLET
 4 void SplashScreen::ConnectWallet(CWallet* wallet)
 5 {
 6-    wallet->ShowProgress.connect(boost::bind(ShowProgress, this, _1, _2, false, _4));
 7+    wallet->ShowProgress.connect(boost::bind(ShowProgress, this, _1, _2, _3, _4));
 8     connectedWallets.push_back(wallet);
 9 }
10 #endif
11@@ -203,7 +203,7 @@ void SplashScreen::unsubscribeFromCoreSignals()
12     uiInterface.ShowProgress.disconnect(boost::bind(ShowProgress, this, _1, _2, _3, _4));
13 #ifdef ENABLE_WALLET
14     for (CWallet* const & pwallet : connectedWallets) {
15-        pwallet->ShowProgress.disconnect(boost::bind(ShowProgress, this, _1, _2, false, _4));
16+        pwallet->ShowProgress.disconnect(boost::bind(ShowProgress, this, _1, _2, _3, _4));
17     }
18 #endif
19 }

achow101 commented at 11:42 pm on March 18, 2018:

Hmm, that diff works for me. I don’t remember what I did that caused a segfault. Added this change.

ryanofsky commented at 7:04 pm on October 12, 2017: member

utACK 4657e40f1516d506095f969dc726da6f97d57a99. Only change since last review is rebase and some whitespace diffs.

promag commented at 8:53 pm on October 29, 2017: member

utACK 4657e40. Have you thought adding tests for the new RPC errors? (might be impossible?)

jonasschnelli commented at 9:53 pm on October 29, 2017: contributor

Reminder: this PR currently causes a deadlock (at least on OSX): IMO it would require #11281 to work flawless.

ryanofsky commented at 5:45 pm on October 31, 2017: member

@jonasschnelli, I don’t see how this could be causing any deadlock that wasn’t occurring before. The cancel callback isn’t acquiring any locks, just updating an atomic<bool>. Are you sure there is a problem in the current version of this PR?

achow101 commented at 6:08 pm on October 31, 2017: member

@ryanofsky I think it is more that a deadlock currently exists in the rescanning code which this PR makes more obvious and easier to hit.

achow101 force-pushed on Oct 31, 2017

ryanofsky commented at 6:43 pm on October 31, 2017: member

@ryanofsky I think it is more that a deadlock currently exists in the rescanning code which this PR makes more obvious and easier to hit.

I know that’s the claim, but I don’t understand what the deadlock is. What does the rescan code have to do with transactiontablemodel and why would shorter aborted scans cause more UI problems than longer full scans?

achow101 force-pushed on Oct 31, 2017

ryanofsky commented at 8:15 pm on October 31, 2017: member

utACK 449dd52b417d2a49b4de4440b391f6ebccb093b4. Same as before, just gets rid of spurious whitespace changes and void(void)

promag commented at 7:48 am on December 5, 2017: member

Please see an alternative concept in #11826.

laanwj added this to the milestone 0.17.0 on Jan 11, 2018

laanwj referenced this in commit 8470e64724 on Jan 24, 2018

achow101 commented at 9:17 pm on February 1, 2018: member

Rebased

achow101 force-pushed on Feb 1, 2018

achow101 commented at 2:46 am on February 20, 2018: member

Review/Merge beg

promag commented at 4:35 pm on February 26, 2018: member

@achow101 needs rebase.

achow101 force-pushed on Feb 26, 2018

achow101 commented at 4:39 pm on February 26, 2018: member

Rebased

in src/qt/bitcoingui.h:247 in 7e53d95770 outdated

233@@ -232,8 +234,8 @@ private Q_SLOTS:
234     void detectShutdown();
235 
236     /** Show progress dialog e.g. for verifychain */
237-    void showProgress(const QString &title, int nProgress);
238-    
239+    void showProgress(const QString &title, int nProgress, bool resume_possible, const std::function<void()>& cancel);

luke-jr commented at 7:54 pm on March 6, 2018:

Would be nice to have nullptr as a default for cancel.

achow101 commented at 8:37 pm on March 6, 2018:

This was suggested before and I was not able to get it to work.

in src/wallet/rpcdump.cpp:628 in 7e53d95770 outdated

634     }
635-    pwallet->RescanFromTime(nTimeBegin, reserver, false /* update */);
636+    pwallet->ShowProgress("", 100, false, nullptr); // hide progress dialog in GUI
637+    int64_t scanned_time = pwallet->RescanFromTime(nTimeBegin, reserver, false /* update */);
638+    if (scanned_time > nTimeBegin) {
639+        if (pwallet->IsAbortingRescan()) {

luke-jr commented at 7:57 pm on March 6, 2018:

This is ugly, since RescanFromTime changes without a signature change. Better to have it throw an exception to be caught from callers, when cancelled.

achow101 commented at 8:31 pm on March 6, 2018:

I don’t quite understand what you mean.

sipa commented at 6:19 pm on March 14, 2018:

@luke-jr In what way does RescanFromTime change?

MarcoFalke commented at 10:53 pm on March 18, 2018:

RescanFromTime uses ScanForWalletTransactions internally. And the interface of ScanForWalletTransactions is changed in this pull request.

achow101 commented at 11:38 pm on March 18, 2018:

That change was reverted.

ryanofsky commented at 9:44 pm on March 12, 2018: member

utACK 7e53d957709c0bac88cabfc0f9370b33888504db. No changes since last review in Qt code. Just conflict resolution in other parts of the code.

achow101 commented at 6:10 pm on March 14, 2018: member

Review beg for reviews from people other than @ryanofsky

MarcoFalke commented at 1:15 pm on March 18, 2018: member

utACK, but calling rescanblockchain in the GUI console no longer gives me the tiny “Rescanning …” pop up, which prevents me from testing. Any idea?

Pop up on master:

achow101 commented at 7:29 pm on March 18, 2018: member

That’s strange. I’m seeing that now too, but I don’t know why.

achow101 force-pushed on Mar 18, 2018

achow101 commented at 8:24 pm on March 18, 2018: member

Ok, I fixed that problem and also had to rebase.

in src/wallet/wallet.cpp:1774 in c18cfb8fe0 outdated

1772@@ -1773,6 +1773,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1773         }
1774         if (pindex && fAbortRescan) {
1775             LogPrintf("Rescan aborted at block %d. Progress=%f\n", pindex->nHeight, gvp);
1776+            ret = pindex;

MarcoFalke commented at 10:54 pm on March 18, 2018:

Please revert this change. This breaks other rpcs that use ScanForWalletTransactions directly

achow101 commented at 11:38 pm on March 18, 2018:

Done

in src/wallet/rpcdump.cpp:498 in c18cfb8fe0 outdated

492@@ -479,7 +493,14 @@ UniValue importpubkey(const JSONRPCRequest& request)
493     }
494     if (fRescan)
495     {
496-        pwallet->RescanFromTime(TIMESTAMP_MIN, reserver, true /* update */);
497+        int64_t scanned_time = pwallet->RescanFromTime(TIMESTAMP_MIN, reserver, true /* update */);
498+        if (scanned_time > TIMESTAMP_MIN) {
499+            if (pwallet->IsAbortingRescan()) {

MarcoFalke commented at 10:56 pm on March 18, 2018:

Why is this check within the scope of the (scanned_time > TIMESTAMP_MIN) check? Wouldn’t the following make more sense:

0if (pwallet->IsAbortingRescan()) throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");
1if (scanned_time > TIMESTAMP_MIN) throw JSONRPCError(RPC_WALLET_ERROR, "Rescan was unable to fully rescan the blockchain. Some transactions may be missing.");

achow101 commented at 11:38 pm on March 18, 2018:

Yes, done.

MarcoFalke commented at 10:57 pm on March 18, 2018: member

utACK c18cfb8fe0e5de02471011bef0e37f0a6d5920e6, but needs feedback addressed.

achow101 force-pushed on Mar 18, 2018

MarcoFalke commented at 11:52 pm on March 18, 2018: member

re-utACK 1de7f88f4cad0f59a6684e179177707c8efd7b3a (only changes were to address the feedback)

ryanofsky commented at 3:29 pm on March 20, 2018: member

utACK 1de7f88f4cad0f59a6684e179177707c8efd7b3a. Changes since last review were rebase, checking for abort condition even if RescanFromTime succeeds, and forwarding resume_possible argument to showProgress.

MarcoFalke added the label RPC/REST/ZMQ on Mar 21, 2018

MarcoFalke commented at 1:19 pm on March 21, 2018: member

@jonasschnelli You tested this a bit last year. Mind to re-test?

achow101 force-pushed on Mar 28, 2018

achow101 commented at 5:59 pm on March 28, 2018: member

Rebased

TheBlueMatt commented at 6:26 pm on March 28, 2018: member

This effectively reverts #10770. Please, no. If you want a specific cancel function, please split out showprogress into an init version and a rescan version, passing up std::functions from everywhere (and across the uiInterface interface, which people are still talking about making a process boundary) is really, really, really gross.

achow101 commented at 6:35 pm on March 28, 2018: member

@TheBlueMatt I don’t see any other way to implement such cancellations in the GUI. Perhaps you could suggest something?

TheBlueMatt commented at 6:53 pm on March 28, 2018: member

We cancel just fine during init already (using StartShutdown()). If we want to also support cancelling via a rescan progress dialog, the rescan progress dialog should know how to make that happen. I still really don’t think we need a generic interface here - we only use the dialog for one thing (and I’m not aware of any other cancelable proposed things to show there) - and making it common between init and rescan still makes no sense to me - they are very, very different things.

ryanofsky commented at 7:09 pm on March 28, 2018: member

@TheBlueMatt, your objection seems the same as the “I’m not very fond of callbacks” criticism which came up earlier: #11200#pullrequestreview-59957173. I didn’t think this was a big deal (or really a very substantive criticism), but it would be pretty easy to address by replacing the callback with a wallet AbortRescans() function that Qt code could call more directly. Would this make everybody happy?

TheBlueMatt commented at 7:13 pm on March 28, 2018: member

Its a bit more broad, I’m a bit more than “not fond of” passing callbacks from random internal places all the way to GUI, plus we started with that and deliberately got rid of them. Adding them back would need more justification than “well, this is a way that it can be done”.

ryanofsky commented at 7:29 pm on March 28, 2018: member

I think supporting cancelling rescans is a worthwhile feature, and there are lots of ways it can be implemented. Which way to choose is mostly a question of how much much existing code you want to rewrite.

Having qt code just call an AbortRescan function when the cancel button is pressed is probably the simplest possible approach. Having the qt code call a generic cancel callback is a little more generic, and maybe a little cleaner, but does involve callbacks. Changing the progress dialog to be aware of the differences between init and rescan would be another way to approach this, and probably a good idea, but just a larger change.

achow101 force-pushed on Mar 31, 2018

achow101 commented at 8:06 pm on March 31, 2018: member

I changed the implementation to completely remove the callback. The showProgress dialog that is used for rescans just calls AbortRescan directly when cancel is clicked. This should work since the dialog is used only for rescanning.

OP and title also updated.

The original branch with callbacks can be found here: https://github.com/achow101/bitcoin/tree/gui-rescan-abort-w-callback

achow101 renamed this:
~~Allow for aborting rescans and canceling showProgress dialogs~~
Allow for aborting rescans in the GUI
on Mar 31, 2018

achow101 force-pushed on Mar 31, 2018

achow101 force-pushed on Apr 5, 2018

achow101 commented at 9:34 pm on April 5, 2018: member

Rebased

MarcoFalke commented at 9:57 pm on April 8, 2018: member

Needs trivial rebase to accommodate a file rename.

achow101 force-pushed on Apr 8, 2018

achow101 commented at 10:08 pm on April 8, 2018: member

Rebased

in src/wallet/rpcdump.cpp:175 in 071710e06c outdated

171@@ -172,7 +172,13 @@ UniValue importprivkey(const JSONRPCRequest& request)
172         }
173     }
174     if (fRescan) {
175-        pwallet->RescanFromTime(TIMESTAMP_MIN, reserver, true /* update */);
176+        int64_t scanned_time = pwallet->RescanFromTime(TIMESTAMP_MIN, reserver, true /* update */);

promag commented at 10:50 pm on April 8, 2018:

This code is repeated 4 times, maybe it’s worth moving it to a function?

ryanofsky commented at 4:43 pm on April 9, 2018:

In commit “Give an error when rescan is aborted by the user”

This code is repeated 4 times, maybe it’s worth moving it to a function?

Could be nice, though to be honest a lot of other code is duplicated between the import* RPCs too. So effort might be better spent deduping larger chunks or deprecating older RPCs in favor of importmulti.

achow101 commented at 5:09 pm on April 9, 2018:

I think that should be part of a separate refactor for deduplicating all of the import* RPCs.

promag commented at 8:58 am on April 14, 2018:

Sure.

in src/interfaces/wallet.h:251 in 071710e06c outdated

246@@ -247,6 +247,9 @@ class Wallet
247     //! Register handler for watchonly changed messages.
248     using WatchOnlyChangedFn = std::function<void(bool have_watch_only)>;
249     virtual std::unique_ptr<Handler> handleWatchOnlyChanged(WatchOnlyChangedFn fn) = 0;
250+
251+    //! Abort a rescan

promag commented at 10:51 pm on April 8, 2018:

Nit, period.

ryanofsky commented at 4:31 pm on April 9, 2018:

Would suggest renaming this method abortRescan and moving it above backupWallet (both here and in wallet.cpp). These interfaces are organized with regular methods on top and callback methods below. Also roughly with whole-wallet operations above operations on individual addresses and transactions.

achow101 commented at 5:16 pm on April 9, 2018:

Done (moved and added period).

in src/qt/walletmodel.cpp:564 in 071710e06c outdated

557@@ -558,3 +558,8 @@ bool WalletModel::isMultiwallet()
558 {
559     return m_node.getWallets().size() > 1;
560 }
561+
562+void WalletModel::AbortRescan() const
563+{
564+    wallet().AbortRescan();

promag commented at 10:54 pm on April 8, 2018:

Nit, m_wallet->AbortRescan()?

ryanofsky commented at 4:39 pm on April 9, 2018:

In commit “Add cancel button to rescan progress dialog”

Would suggest dropping this method and just calling model.wallet().abortRescan() directly. I don’t think there’s a benefit to wrapping individual Wallet calls inside WalletModel methods unless they’re doing extra work like synchronization or caching.

achow101 commented at 5:16 pm on April 9, 2018:

Removed and called directly from walletview

in src/qt/walletmodel.h:208 in 071710e06c outdated

203@@ -204,6 +204,8 @@ class WalletModel : public QObject
204     QString getWalletName() const;
205 
206     bool isMultiwallet();
207+
208+    void AbortRescan() const;

promag commented at 10:54 pm on April 8, 2018:

Nit, void abortRescan() const.

achow101 commented at 5:16 pm on April 9, 2018:

Removed

promag commented at 10:55 pm on April 8, 2018: member

Some comments, will test later.

promag commented at 10:58 pm on April 8, 2018: member

Changing the progress dialog to be aware of the differences between init and rescan would be another way to approach this, and probably a good idea, but just a larger change.

I agree with @ryanofsky here. I’ll try to push #11826 forward. Not that this shouldn’t go forward.

jonasschnelli commented at 4:37 pm on April 9, 2018: contributor

Could finally test this. I think it is an important feature because the way how users currently abort rescan is by force quitting the application (which is really not what we want).

Tested ACK bc2003cdf7102e03710dbc19e9c397b92e124fb4. I’d like to see @promag’s nits addresses (lowercase methodname in QT, interface call via class member).

ryanofsky commented at 4:47 pm on April 9, 2018: member

utACK bc2003cdf7102e03710dbc19e9c397b92e124fb4. This PR has changed quite a lot since it was opened but is very simple at this point and provides useful functionality.

achow101 force-pushed on Apr 9, 2018

in src/wallet/rpcdump.cpp:557 in 5218cd3836 outdated

533@@ -534,9 +534,9 @@ UniValue importwallet(const JSONRPCRequest& request)
534         int64_t nFilesize = std::max((int64_t)1, (int64_t)file.tellg());
535         file.seekg(0, file.beg);
536 
537-        pwallet->ShowProgress(_("Importing..."), 0); // show progress dialog in GUI
538+        uiInterface.ShowProgress(_("Importing..."), 0, false); // show progress dialog in GUI

ryanofsky commented at 5:41 pm on April 9, 2018:

In commit “Add cancel button to rescan progress dialog”

These changes from wallet to global ShowProgress calls seem unrelated. Are they intentional? It might make sense to change these if removing the wallet ShowProgress signal, but since that is still used in CWallet::ScanForWalletTransactions, this change just makes these calls inconsistent.

If this change is necessary it would be good to have a comment saying why CWallet::ShowProgress is not used here.

achow101 commented at 5:56 pm on April 9, 2018:

Because CWallet::ShowProgress has a cancel button now which is directly tied to AbortRescan (because it isn’t used for anything except showing rescan progress), I needed a different progress bar which does not have a cancel button. This change makes that ShowProgress use BitcoinGUI::ShowProgress (which is tied toe uiInterface)which does not have a cancel button (and is also not used anywhere else AFAICT). This progress bar indicates the import progress, not rescan progress which is why a different ShowProgress is needed

I added a comment.

ryanofsky commented at 5:48 pm on April 9, 2018: member

utACK 66ee39f227b28a32c6c1cb6cd68c0eee0cc8637f

ryanofsky commented at 7:10 pm on April 9, 2018: member

utACK deaffd812e3cdaeb7ec978c2d86f4bf688add96f, thanks for the comment!

Add cancel button to rescan progress dialog

Adds a cancel button to the rescan progress dialog. When it is clicked,
AbortRescan is called to abort a rescan

69b01e6f8b

Give an error when rescan is aborted by the user ae1d2b0308

achow101 force-pushed on Apr 12, 2018

achow101 commented at 9:00 pm on April 12, 2018: member

Squashed the squashme. Is this ready for merging?

MarcoFalke commented at 9:47 pm on April 12, 2018: member

utACK ae1d2b0308d7fe9df7fc18699c69e9587e6154bd. Going to test this later…

jonasschnelli merged this on Apr 13, 2018

jonasschnelli closed this on Apr 13, 2018

jonasschnelli referenced this in commit 5f2a39946f on Apr 13, 2018

MarcoFalke removed this from the milestone 0.17.0 on Apr 21, 2018

MarcoFalke added this to the milestone 0.16.1 on Apr 21, 2018

MarcoFalke added the label Needs backport on Apr 21, 2018

MarcoFalke removed the label Needs backport on Apr 21, 2018

MarcoFalke removed this from the milestone 0.16.1 on Apr 21, 2018

Fabcien referenced this in commit 737142441b on Aug 30, 2019

PastaPastaPasta referenced this in commit ea808be82d on Apr 13, 2020

UdjinM6 referenced this in commit 8ade2f7957 on Apr 14, 2020

PastaPastaPasta referenced this in commit 482c37308a on Nov 10, 2020

PastaPastaPasta referenced this in commit 5fac392841 on Nov 12, 2020

PastaPastaPasta referenced this in commit c0caa8ab32 on Nov 17, 2020

ckti referenced this in commit 448bc956f6 on Mar 28, 2021

gades referenced this in commit d0be641481 on Jun 30, 2021

MarcoFalke locked this on Sep 8, 2021

Allow for aborting rescans in the GUI #11200