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");
Does this "Cancel" need a translation?
Yes, it does. Done
utACK modulo Travis issues
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 {
Done
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.
Done
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.
Oh, I didn't realize that that would work. Done.
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)
utACK e6e8b0c259594b09e405fc9123d1f8473e42b0e4 with RPC bugfixes
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
Done
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
nit: use std::bind (and below)
Done
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;
use nullptr as default parameter for cancel to avoid adding nullptr to all non-callback ShowProgress calls?
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.
Concept ACK
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
Needs rebase
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)>)));
Could just write void() instead of void(void) everywhere
Done
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.)
Done
Actually changing this seems to be causing a segfault, so I will leave it as is.
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.
--- a/src/qt/splashscreen.cpp
+++ b/src/qt/splashscreen.cpp
@@ -181,7 +181,7 @@ static void ShowProgress(SplashScreen *splash, const std::string &title, int nPr
#ifdef ENABLE_WALLET
void SplashScreen::ConnectWallet(CWallet* wallet)
{
- wallet->ShowProgress.connect(boost::bind(ShowProgress, this, _1, _2, false, _4));
+ wallet->ShowProgress.connect(boost::bind(ShowProgress, this, _1, _2, _3, _4));
connectedWallets.push_back(wallet);
}
#endif
@@ -203,7 +203,7 @@ void SplashScreen::unsubscribeFromCoreSignals()
uiInterface.ShowProgress.disconnect(boost::bind(ShowProgress, this, _1, _2, _3, _4));
#ifdef ENABLE_WALLET
for (CWallet* const & pwallet : connectedWallets) {
- pwallet->ShowProgress.disconnect(boost::bind(ShowProgress, this, _1, _2, false, _4));
+ pwallet->ShowProgress.disconnect(boost::bind(ShowProgress, this, _1, _2, _3, _4));
}
#endif
}
Hmm, that diff works for me. I don't remember what I did that caused a segfault. Added this change.
utACK 4657e40f1516d506095f969dc726da6f97d57a99. Only change since last review is rebase and some whitespace diffs.
utACK 4657e40. Have you thought adding tests for the new RPC errors? (might be impossible?)
Reminder: this PR currently causes a deadlock (at least on OSX): IMO it would require #11281 to work flawless.
@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?
@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.
@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?
utACK 449dd52b417d2a49b4de4440b391f6ebccb093b4. Same as before, just gets rid of spurious whitespace changes and void(void)
Rebased
Review/Merge beg
Rebased
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);
Would be nice to have nullptr as a default for cancel.
This was suggested before and I was not able to get it to work.
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()) {
This is ugly, since RescanFromTime changes without a signature change. Better to have it throw an exception to be caught from callers, when cancelled.
I don't quite understand what you mean.
RescanFromTime uses ScanForWalletTransactions internally. And the interface of ScanForWalletTransactions is changed in this pull request.
That change was reverted.
utACK 7e53d957709c0bac88cabfc0f9370b33888504db. No changes since last review in Qt code. Just conflict resolution in other parts of the code.
Review beg for reviews from people other than @ryanofsky
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:
<img src="https://user-images.githubusercontent.com/6399679/37566215-9299e7c8-2a8c-11e8-819f-bb1a7edc5403.png" alt="screenshot from 2018-03-18 09-10-58" width=90pt style="max-width:100%;">
That's strange. I'm seeing that now too, but I don't know why.
Ok, I fixed that problem and also had to rebase.
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;
Please revert this change. This breaks other rpcs that use ScanForWalletTransactions directly
Done
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:
if (pwallet->IsAbortingRescan()) throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");
if (scanned_time > TIMESTAMP_MIN) throw JSONRPCError(RPC_WALLET_ERROR, "Rescan was unable to fully rescan the blockchain. Some transactions may be missing.");
Yes, done.
utACK c18cfb8fe0e5de02471011bef0e37f0a6d5920e6, but needs feedback addressed.
re-utACK 1de7f88f4cad0f59a6684e179177707c8efd7b3a (only changes were to address the feedback)
utACK 1de7f88f4cad0f59a6684e179177707c8efd7b3a. Changes since last review were rebase, checking for abort condition even if RescanFromTime succeeds, and forwarding resume_possible argument to showProgress.
@jonasschnelli You tested this a bit last year. Mind to re-test?
Rebased
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.
@TheBlueMatt I don't see any other way to implement such cancellations in the GUI. Perhaps you could suggest something?
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.
@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?
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".
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
Rebased
Needs trivial rebase to accommodate a file rename.
Rebased
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 */);
This code is repeated 4 times, maybe it's worth moving it to a function?
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.
I think that should be part of a separate refactor for deduplicating all of the import* RPCs.
Sure.
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
Nit, period.
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.
Done (moved and added period).
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();
Nit, 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.
Removed and called directly from walletview
203 | @@ -204,6 +204,8 @@ class WalletModel : public QObject 204 | QString getWalletName() const; 205 | 206 | bool isMultiwallet(); 207 | + 208 | + void AbortRescan() const;
Nit, void abortRescan() const.
Removed
Some comments, will test later.
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).
utACK bc2003cdf7102e03710dbc19e9c397b92e124fb4. This PR has changed quite a lot since it was opened but is very simple at this point and provides useful functionality.
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.
utACK 66ee39f227b28a32c6c1cb6cd68c0eee0cc8637f
utACK deaffd812e3cdaeb7ec978c2d86f4bf688add96f, thanks for the comment!
Adds a cancel button to the rescan progress dialog. When it is clicked,
AbortRescan is called to abort a rescan
Squashed the squashme. Is this ready for merging?
utACK ae1d2b0308d7fe9df7fc18699c69e9587e6154bd. Going to test this later...