gui: Throttle GUI update pace when -reindex #18121

1. 
   hebasto commented at 8:44 PM on February 11, 2020: member

   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
   

2. fanquake added the label GUI on Feb 11, 2020
3. hebasto requested review from ryanofsky on Feb 11, 2020
4. hebasto requested review from laanwj on Feb 11, 2020
5. 
   luke-jr commented at 1:35 AM on February 12, 2020: member

   utACK

6. luke-jr referenced this in commit d097a5dd77 on Feb 12, 2020
7. 
   laanwj commented at 4:17 PM on February 12, 2020: member

   Thanks you for working on this! Concept ACK

8. in src/qt/clientmodel.cpp:246 in 56bf425167 outdated

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

   ---

   ---

   

   ryanofsky commented at 7:34 PM on February 12, 2020:

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

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

   ---

   

   hebasto commented at 10:00 PM on February 12, 2020:

   Done.

9. in src/qt/clientmodel.cpp:247 in 56bf425167 outdated

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

   ---

   ---

   

   ryanofsky commented at 7:57 PM on February 12, 2020:

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

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

   ---

   

   hebasto commented at 10:01 PM on February 12, 2020:

   Done.

10. ryanofsky approved
11. 
    ryanofsky commented at 8:13 PM on February 12, 2020: member

    Code review ACK 56bf42516722c7f05391f5ca2a5c4709486057e4. 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.

12. gui: Throttle GUI update pace when -reindex

    Co-authored-by: Barry Deeney <mxaddict@codedmaster.com>
    Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

    c9fe61291e
13. hebasto force-pushed on Feb 12, 2020
14. 
    hebasto commented at 10:00 PM on February 12, 2020: member

    Updated 56bf42516722c7f05391f5ca2a5c4709486057e4 -> c9fe61291e9b23f37cf66194c2dad28e4d4a8954 (pr18121.01 -> pr18121.02, compare). @ryanofsky Thank you for your review. All your comments have been addressed.

15. hebasto requested review from ryanofsky on Feb 12, 2020
16. in src/qt/clientmodel.cpp:246 in c9fe61291e

     241 | @@ -242,8 +242,9 @@ static void BlockTipChanged(ClientModel *clientmodel, bool initialSync, int heig
     242 |          clientmodel->cachedBestHeaderHeight = height;
     243 |          clientmodel->cachedBestHeaderTime = blockTime;
     244 |      }
     245 | -    // if we are in-sync or if we notify a header update, update the UI regardless of last update time
     246 | -    if (fHeader || !initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {
     247 | +
     248 | +    // During initial sync, block notifications, and header notifications from reindexing are both throttled.
    

    ---

    ---

    

    luke-jr commented at 10:34 PM on February 12, 2020:

    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?

    ---

    

    hebasto commented at 7:40 AM on February 13, 2020:

    IIUC, fReindex==true implies initialSync==true: https://github.com/bitcoin/bitcoin/blob/2bdc476d4d23256d8396bb9051a511f540d87392/src/validation.cpp#L1281-L1282

    Anyway, mind suggesting a better wording?

    ---

    

    ryanofsky commented at 4:33 PM on February 13, 2020:

    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#pullrequestreview-357730284

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

    ---

    

    hebasto commented at 2:57 PM on February 15, 2020:

    Addressed in #18152

17. 
    DrahtBot commented at 11:05 PM on February 12, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

18. 
    jonasschnelli commented at 7:46 AM on February 13, 2020: contributor

    utACK c9fe61291e9b23f37cf66194c2dad28e4d4a8954

19. jonasschnelli referenced this in commit 0c20809da8 on Feb 13, 2020
20. jonasschnelli merged this on Feb 13, 2020
21. jonasschnelli closed this on Feb 13, 2020

    ---

22. MarkLTZ referenced this in commit d99bc96496 on Feb 13, 2020
23. hebasto deleted the branch on Feb 13, 2020
24. jonasschnelli referenced this in commit d44dd51322 on May 19, 2020
25. jasonbcox referenced this in commit cb55c4f9b5 on Nov 9, 2020
26. PastaPastaPasta referenced this in commit e1d30656f6 on Sep 17, 2021
27. PastaPastaPasta referenced this in commit b6a1b67143 on Sep 18, 2021
28. thelazier referenced this in commit 54faad1c17 on Sep 25, 2021
29. DrahtBot locked this on Feb 15, 2022

Contributors

hebastoluke-jrlaanwjryanofskyDrahtBotjonasschnelli

Review Requested
laanwjryanofsky