Allow for aborting rescans in the GUI #11200

achow101 · 2017-08-31T00:41:00Z

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

meshcollider

utACK modulo Travis issues

meshcollider · 2017-08-31T06:39:33Z

src/qt/bitcoingui.cpp

Does this "Cancel" need a translation?

Yes, it does. Done

promag

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

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

promag · 2017-08-31T08:13:47Z

src/qt/bitcoingui.cpp

promag · 2017-08-31T08:15:17Z

src/qt/bitcoingui.cpp

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

achow101 · 2017-08-31T13:52:47Z

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.

ryanofsky

utACK e6e8b0c259594b09e405fc9123d1f8473e42b0e4 with RPC bugfixes

ryanofsky · 2017-08-31T15:26:02Z

src/qt/bitcoingui.cpp

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.

ryanofsky · 2017-08-31T16:05:28Z

src/txdb.cpp

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.

ryanofsky · 2017-08-31T16:07:19Z

src/qt/bitcoingui.cpp

In commit "Add cancel button to rescan progress dialog"

Maybe assert(cancel)

ryanofsky · 2017-08-31T16:53:27Z

src/wallet/rpcdump.cpp

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

s/TIMESTAMP_MIN/nTimeBegin/ here to fix test failures

ryanofsky

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.

jonasschnelli

Concept ACK

jonasschnelli · 2017-09-03T17:10:26Z

src/wallet/wallet.cpp

nit: use std::bind (and below)

jonasschnelli · 2017-09-03T17:12:14Z

src/ui_interface.h

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.

jonasschnelli · 2017-09-06T23:59:27Z

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 · 2017-09-07T17:17:42Z

Needs rebase

ryanofsky

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

ryanofsky · 2017-10-12T18:47:13Z

src/qt/bitcoingui.cpp

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

ryanofsky · 2017-10-12T19:01:21Z

src/qt/splashscreen.cpp

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

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.

promag · 2017-10-29T20:53:33Z

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

jonasschnelli · 2017-10-29T21:53:04Z

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

ryanofsky · 2017-10-31T17:45:38Z

@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 · 2017-10-31T18:08:54Z

@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 · 2017-10-31T18:43:11Z

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

ryanofsky

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

achow101 · 2018-04-08T22:08:52Z

Rebased

promag

Some comments, will test later.

promag · 2018-04-08T22:50:34Z

src/wallet/rpcdump.cpp

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.

promag · 2018-04-08T22:51:53Z

src/interfaces/wallet.h

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

promag · 2018-04-08T22:54:04Z

src/qt/walletmodel.cpp

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

promag · 2018-04-08T22:54:31Z

src/qt/walletmodel.h

Nit, void abortRescan() const.

promag · 2018-04-08T22:58:33Z

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 · 2018-04-09T16:37:09Z

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

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

ryanofsky · 2018-04-09T16:31:30Z

src/interfaces/wallet.h

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.

ryanofsky · 2018-04-09T16:39:30Z

src/qt/walletmodel.cpp

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.

ryanofsky · 2018-04-09T16:43:48Z

src/wallet/rpcdump.cpp

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.

ryanofsky

utACK 66ee39f227b28a32c6c1cb6cd68c0eee0cc8637f

ryanofsky · 2018-04-09T17:41:31Z

src/wallet/rpcdump.cpp

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.

ryanofsky

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

achow101 · 2018-04-12T21:00:51Z

Squashed the squashme. Is this ready for merging?

maflcko · 2018-04-12T21:47:14Z

utACK ae1d2b0. Going to test this later...

ae1d2b0 Give an error when rescan is aborted by the user (Andrew Chow) 69b01e6 Add cancel button to rescan progress dialog (Andrew Chow) Pull request description: 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). Tree-SHA512: 4bb14998766de686e2318fbc9805758eccf5dbe628a7257d072c9ae2fb4f61303a0876f49988d6e5eddb261969b8a307c81c0c2df0a42ae909a43d738af3dc1b

Summary: ae1d2b0 Give an error when rescan is aborted by the user (Andrew Chow) 69b01e6 Add cancel button to rescan progress dialog (Andrew Chow) Pull request description: 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). Tree-SHA512: 4bb14998766de686e2318fbc9805758eccf5dbe628a7257d072c9ae2fb4f61303a0876f49988d6e5eddb261969b8a307c81c0c2df0a42ae909a43d738af3dc1b Backport of Core PR11200 bitcoin/bitcoin#11200 Test Plan: make check ./bitcoin-qt -> help -> debug -> console -> rescanblockchain -> select cancel in the new window. Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3891

…scanFromTime 7f81250 Mention that other RPC calls report keys as "imported" while txns are still missing (Jonas Schnelli) ccd8ef6 Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock (Jonas Schnelli) bc356b4 Make sure WalletRescanReserver has successfully reserved the rescan (Jonas Schnelli) dbf8556 Add RAII wallet rescan reserver (Jonas Schnelli) 8d0b610 Avoid pemanent cs_main/cs_wallet lock during wallet rescans (Jonas Schnelli) Pull request description: Right now, we are holding `cs_main`/`cs_wallet` during the whole rescan process (which can take a couple of hours). This was probably only done because of laziness and it is an important show-stopper for bitcoin#11200 (GUI rescan abort). Tree-SHA512: 0fc3f82d0ee9b2f013e6bacba8d59f7334306660cd676cd64c47bb305c4cb7c7a36219d6a6f76023b74e5fe87f3ab9fc7fd2439e939f71aef653fddb0a1e23b1 Signed-off-by: Pasta <[email protected]> # Conflicts: # src/wallet/rpcdump.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.h

…escanFromTime (#3411) * Merge bitcoin#11281: Avoid permanent cs_main/cs_wallet lock during RescanFromTime 7f81250 Mention that other RPC calls report keys as "imported" while txns are still missing (Jonas Schnelli) ccd8ef6 Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock (Jonas Schnelli) bc356b4 Make sure WalletRescanReserver has successfully reserved the rescan (Jonas Schnelli) dbf8556 Add RAII wallet rescan reserver (Jonas Schnelli) 8d0b610 Avoid pemanent cs_main/cs_wallet lock during wallet rescans (Jonas Schnelli) Pull request description: Right now, we are holding `cs_main`/`cs_wallet` during the whole rescan process (which can take a couple of hours). This was probably only done because of laziness and it is an important show-stopper for bitcoin#11200 (GUI rescan abort). Tree-SHA512: 0fc3f82d0ee9b2f013e6bacba8d59f7334306660cd676cd64c47bb305c4cb7c7a36219d6a6f76023b74e5fe87f3ab9fc7fd2439e939f71aef653fddb0a1e23b1 Signed-off-by: Pasta <[email protected]> # Conflicts: # src/wallet/rpcdump.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.h * remove "LearnAllRelatedScripts" Signed-off-by: Pasta <[email protected]> * fix importelectrumwallet Signed-off-by: Pasta <[email protected]> Co-authored-by: Wladimir J. van der Laan <[email protected]>

ae1d2b0 Give an error when rescan is aborted by the user (Andrew Chow) 69b01e6 Add cancel button to rescan progress dialog (Andrew Chow) Pull request description: 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). Tree-SHA512: 4bb14998766de686e2318fbc9805758eccf5dbe628a7257d072c9ae2fb4f61303a0876f49988d6e5eddb261969b8a307c81c0c2df0a42ae909a43d738af3dc1b

…escanFromTime (dashpay#3411) * Merge bitcoin#11281: Avoid permanent cs_main/cs_wallet lock during RescanFromTime 7f81250 Mention that other RPC calls report keys as "imported" while txns are still missing (Jonas Schnelli) ccd8ef6 Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock (Jonas Schnelli) bc356b4 Make sure WalletRescanReserver has successfully reserved the rescan (Jonas Schnelli) dbf8556 Add RAII wallet rescan reserver (Jonas Schnelli) 8d0b610 Avoid pemanent cs_main/cs_wallet lock during wallet rescans (Jonas Schnelli) Pull request description: Right now, we are holding `cs_main`/`cs_wallet` during the whole rescan process (which can take a couple of hours). This was probably only done because of laziness and it is an important show-stopper for bitcoin#11200 (GUI rescan abort). Tree-SHA512: 0fc3f82d0ee9b2f013e6bacba8d59f7334306660cd676cd64c47bb305c4cb7c7a36219d6a6f76023b74e5fe87f3ab9fc7fd2439e939f71aef653fddb0a1e23b1 Signed-off-by: Pasta <[email protected]> # Conflicts: # src/wallet/rpcdump.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.h * remove "LearnAllRelatedScripts" Signed-off-by: Pasta <[email protected]> * fix importelectrumwallet Signed-off-by: Pasta <[email protected]> Co-authored-by: Wladimir J. van der Laan <[email protected]>

achow101 force-pushed the gui-recan-abort branch 2 times, most recently from 1bb1af0 to e6e8b0c Compare August 31, 2017 00:50

meshcollider reviewed Aug 31, 2017

View reviewed changes

promag reviewed Aug 31, 2017

View reviewed changes

ryanofsky reviewed Aug 31, 2017

View reviewed changes

achow101 force-pushed the gui-recan-abort branch 2 times, most recently from b42c0a9 to c8720f0 Compare August 31, 2017 17:37

ryanofsky reviewed Aug 31, 2017

View reviewed changes

jonasschnelli reviewed Sep 3, 2017

View reviewed changes

achow101 force-pushed the gui-recan-abort branch from c8720f0 to 3498226 Compare September 5, 2017 19:51

laanwj added the GUI label Sep 6, 2017

achow101 force-pushed the gui-recan-abort branch 2 times, most recently from aca0587 to 4657e40 Compare September 7, 2017 23:11

jonasschnelli mentioned this pull request Sep 7, 2017

Avoid permanent cs_main/cs_wallet lock during RescanFromTime #11281

Merged

ryanofsky mentioned this pull request Oct 4, 2017

ScanForWalletTransactions return value is incorrectly documented #11450

Closed

ryanofsky reviewed Oct 12, 2017

View reviewed changes

achow101 force-pushed the gui-recan-abort branch 2 times, most recently from 350d235 to 70550fd Compare October 31, 2017 18:41

achow101 force-pushed the gui-recan-abort branch 2 times, most recently from 79ff4a5 to 449dd52 Compare October 31, 2017 19:00

ryanofsky reviewed Oct 31, 2017

View reviewed changes

promag reviewed Apr 8, 2018

View reviewed changes

ryanofsky reviewed Apr 9, 2018

View reviewed changes

achow101 force-pushed the gui-recan-abort branch from 071710e to 66ee39f Compare April 9, 2018 17:16

ryanofsky reviewed Apr 9, 2018

View reviewed changes

achow101 added 2 commits April 12, 2018 17:00

Add cancel button to rescan progress dialog

69b01e6

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

Give an error when rescan is aborted by the user

ae1d2b0

achow101 force-pushed the gui-recan-abort branch from deaffd8 to ae1d2b0 Compare April 12, 2018 21:00

jonasschnelli merged commit ae1d2b0 into bitcoin:master Apr 13, 2018

lucash-dev mentioned this pull request Apr 14, 2018

Overview of slow unit tests #10026

Closed

maflcko modified the milestones: 0.17.0, 0.16.1 Apr 21, 2018

maflcko added Needs backport and removed Needs backport labels Apr 21, 2018

maflcko removed this from the 0.16.1 milestone Apr 21, 2018

achow101 mentioned this pull request Jun 29, 2018

[Qt] import* rescans are not abortable and can confuse users #10992

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Allow for aborting rescans in the GUI #11200

Allow for aborting rescans in the GUI #11200

Uh oh!

Conversation

achow101 commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 31, 2017

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Sep 6, 2017

Uh oh!

jonasschnelli commented Sep 7, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 29, 2017

Uh oh!

achow101 commented Aug 31, 2017 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Apr 9, 2018 •

edited

Loading