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).
1131- progressDialog->setCancelButton(0);
1132 progressDialog->setAutoClose(false);
1133 progressDialog->setValue(0);
1134+
1135+ if (cancel) {
1136+ progressDialog->setCancelButtonText("Cancel");
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+ }
} else {
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)
& are next to the type. Same for remaining changes.
Should it be possible to abort rescans caused by the RPC calls?
I’m not very fond of callbacks. Have you thought in alternatives?
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.
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)>)));
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.
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)>());
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.
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();
In commit “Add cancel button to rescan progress dialog”
Maybe assert(cancel)
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) {
In commit “Give an error when rescan is aborted by the user”
s/TIMESTAMP_MIN/nTimeBegin/ here to fix test failures
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.
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
std::bind (and below)
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;
nullptr as default parameter for cancel to avoid adding nullptr to all non-callback ShowProgress calls?
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
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)>)));
void() instead of void(void) everywhere
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));
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.)
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 }
@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?
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);
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()) {
RescanFromTime uses ScanForWalletTransactions internally. And the interface of ScanForWalletTransactions is changed in this pull request.
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:
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;
ScanForWalletTransactions directly
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()) {
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.");
resume_possible argument to showProgress.
AbortRescans() function that Qt code could call more directly. Would this make everybody happy?
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.
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
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 */);
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.
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
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.
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();
m_wallet->AbortRescan()?
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.
203@@ -204,6 +204,8 @@ class WalletModel : public QObject
204 QString getWalletName() const;
205
206 bool isMultiwallet();
207+
208+ void AbortRescan() const;
void abortRescan() const.
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.
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).
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
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.
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.
Adds a cancel button to the rescan progress dialog. When it is clicked,
AbortRescan is called to abort a rescan