Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Throttle GUI update pace when -reindex #18121

Merged
merged 1 commit into from Feb 13, 2020

Conversation

@hebasto
Copy link
Member

hebasto commented Feb 11, 2020

This is grabbed from #17565.

All laanwj's and ryanofsky's suggestions are implemented.

With this PR, the GUI does not freeze when a user runs:

$ ./src/qt/bitcoin-qt -reindex
@fanquake fanquake added the GUI label Feb 11, 2020
@hebasto hebasto requested review from ryanofsky and laanwj Feb 11, 2020
@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 12, 2020

utACK

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 12, 2020
Co-authored-by: Barry Deeney <mxaddict@codedmaster.com>
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

Github-Pull: bitcoin#18121
Rebased-From: 56bf425
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 12, 2020

Thanks you for working on this!
Concept ACK

Copy link
Contributor

ryanofsky left a comment

Code review ACK 56bf425. This is a good minimal fix. It would be also be nice to have followup cleanup that passes the reindexing state directly to the handleNotifyBlockTip/handleNotifyHeaderTip handlers, so the GUI doesn't need to call back into the node in the middle of handling an event from the node. This would also make the initial sync and reindexing conditions more consistent, and maybe more efficient if there are a lot of notifications backed up in the queue, because throttling could still happen when reindexing finished but notifications from reindexing were still being processed.

@@ -243,7 +243,8 @@ static void BlockTipChanged(ClientModel *clientmodel, bool initialSync, int heig
clientmodel->cachedBestHeaderTime = blockTime;
}
// if we are in-sync or if we notify a header update, update the UI regardless of last update time
if (fHeader || !initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {
// If the command line option -reindex is specified, the GUI update pace is throttled down.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 12, 2020

Contributor

In commit "gui: Throttle GUI update pace when -reindex" (56bf425)

Comment is a little misleading because throttling doesn't happen just generally when -reindex is specified, it does stop when the reindex is over.

Would suggest changing to: "During initial sync, block notifications, and header notifications from reindexing are both throttled."

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 12, 2020

Author Member

Done.

@@ -243,7 +243,8 @@ static void BlockTipChanged(ClientModel *clientmodel, bool initialSync, int heig
clientmodel->cachedBestHeaderTime = blockTime;
}
// if we are in-sync or if we notify a header update, update the UI regardless of last update time
if (fHeader || !initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {
// If the command line option -reindex is specified, the GUI update pace is throttled down.
if ((fHeader && !clientmodel->node().getReindex()) || !initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 12, 2020

Contributor

In commit "gui: Throttle GUI update pace when -reindex" (56bf425)

I think it would be best to put !initialSync condition first to be clear throttling only happens during initial sync.

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 12, 2020

Author Member

Done.

Co-authored-by: Barry Deeney <mxaddict@codedmaster.com>
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
@hebasto hebasto force-pushed the hebasto:20200211-reindex-gui branch from 56bf425 to c9fe612 Feb 12, 2020
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Feb 12, 2020

Updated 56bf425 -> c9fe612 (pr18121.01 -> pr18121.02, compare).

@ryanofsky
Thank you for your review. All your comments have been addressed.

@hebasto hebasto requested a review from ryanofsky Feb 12, 2020
// if we are in-sync or if we notify a header update, update the UI regardless of last update time
if (fHeader || !initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {

// During initial sync, block notifications, and header notifications from reindexing are both throttled.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Feb 12, 2020

Member

I find this very confusing, and even if I look past the grammar error, probably outright wrong since header notifications are only throttled for reindex, not initial sync?

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 13, 2020

Author Member

IIUC, fReindex==true implies initialSync==true:

bitcoin/src/validation.cpp

Lines 1281 to 1282 in 2bdc476

if (fImporting || fReindex)
return true;

Anyway, mind suggesting a better wording?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Contributor

I find this very confusing, and even if I look past the grammar error, probably outright wrong since header notifications are only throttled for reindex, not initial sync?

This is pretty confusing, and I was probably trying to pack too much information in a short comment. There should be chances to clean this up in the future with followups from #18136 or #18121 (review)

Ideally the code would just be more direct and not need a comment stating what it does:

enum NotificationType { BLOCK_TIP_CHANGED, HEADER_TIP_CHANGED };
enum NotificationStatus { INIT_REINDEX, INIT_DOWNLOAD, POST_INIT };

void HandleNotification(NotificationType notification, NotificationStatus status) {
   bool throttle = status == INIT_REINDEX || (status == INIT_DOWNLOAD && notification == BLOCK_TIP_CHANGED);
   if (!throttle || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) ...

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 15, 2020

Author Member

Addressed in #18152

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Feb 12, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17993 (gui: Avoid redundant cs_main locks in balance polling. by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Feb 13, 2020

utACK c9fe612

jonasschnelli added a commit that referenced this pull request Feb 13, 2020
c9fe612 gui: Throttle GUI update pace when -reindex (Hennadii Stepanov)

Pull request description:

  This is grabbed from #17565.

  All **laanwj**'s and **ryanofsky**'s suggestions are implemented.

  With this PR,  the GUI does not freeze when a user runs:
  ```
  $ ./src/qt/bitcoin-qt -reindex
  ```

ACKs for top commit:
  jonasschnelli:
    utACK c9fe612

Tree-SHA512: c7be316cb73d3d286bdf8429a960f71777d13a73d059869a64e23ad276499252b561a3a5b9613c4c1ad58cc0de26283c1ec72be745c401f604eaa05f70bf7d64
@jonasschnelli jonasschnelli merged commit c9fe612 into bitcoin:master Feb 13, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Feb 13, 2020
- [0.19] wallet: Reset reused transactions cache bitcoin#18083
- 0.19: Backports bitcoin#17792
- psbt: handle unspendable psbts bitcoin#17524
- qt: Fix comparison function signature bitcoin#17634
- psbt: check that various indexes and amounts are within bounds bitcoin#17156
- [0.19] psbt: check that various indexes and amounts are within bounds bitcoin#18079
- [0.19] Final backports for 0.19.1 bitcoin#17988
- Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash bitcoin#17924
- qt: Fix deprecated QCharRef usage bitcoin#18101
- gui: Throttle GUI update pace when -reindex bitcoin#18121
- gui: Fix race in WalletModel::pollBalanceChanged bitcoin#18123
- gui: Fix unintialized WalletView::progressDialog bitcoin#18062
- Bugfix: GUI: Hide the HD/encrypt icons earlier so they get re-shown if another wallet is open bitcoin#18007
- bug-fix macos: give free bytes to F_PREALLOCATE bitcoin#17887
- test: add missing #include to fix compiler errors bitcoin#17980
- zmq: Fix due to invalid argument and multiple notifiers bitcoin#17445
@hebasto hebasto deleted the hebasto:20200211-reindex-gui branch Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.