qt: Use SynchronizationState enum for signals to GUI #18152

pull hebasto wants to merge 6 commits into bitcoin:master from hebasto:20200215-pr18121-followup2 changing 14 files +73 −50
  1. hebasto commented at 2:56 PM on February 15, 2020: member

    This PR is a followup of #18121 and:

    • addresses confusion about GUI notification throttling conditions (luke-jr's comment, ryanofsky's comment)
    • removes isInitialBlockDownload() call from the GUI back to the node (on macOS). See: ryanofsky's comment
  2. hebasto cross-referenced this on Feb 15, 2020 from issue gui: Throttle GUI update pace when -reindex by hebasto
  3. DrahtBot added the label GUI on Feb 15, 2020
  4. DrahtBot commented at 6:37 PM on February 15, 2020: contributor

    <!--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:

    • #19011 (Reduce cs_main lock accumulation during GUI startup by jonasschnelli)
    • #19007 (relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload by jonasschnelli)
    • #18790 (gui: Improve thread naming by hebasto)
    • #17993 (gui: Balance/TxStatus polling update based on last block hash. 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.

  5. DrahtBot cross-referenced this on Feb 15, 2020 from issue qt, refactor: Make BitcoinUnits::Unit a scoped enum by hebasto
  6. DrahtBot cross-referenced this on Feb 15, 2020 from issue gui: Prevent idle sleep in macos while in IBD by promag
  7. in src/qt/clientmodel.cpp:231 in 5bdd2831ee outdated 
     235 | -    int64_t now = 0;
 236 | -    if (initialSync)
 237 | -        now = GetTimeMillis();
 238 | -
 239 | -    int64_t& nLastUpdateNotification = fHeader ? nLastHeaderTipUpdateNotification : nLastBlockTipUpdateNotification;
 240 | +    if (node.getImporting()) return ClientModel::NotificationStatus::INIT_IMPORT;
    ryanofsky commented at 6:09 PM on February 26, 2020:

    I'm not sure what motivation is exposing the import state, but it seems like you'd want the state to be INIT_REINDEX not INIT_IMPORT during reindexing (see ThreadImport) so I don't understand the way these are prioritized

    hebasto commented at 9:37 PM on March 4, 2020:

    Reworked.

  8. in src/qt/clientmodel.cpp:246 in 5bdd2831ee outdated 
     253 |  
 254 | -    // During initial sync, block notifications, and header notifications from reindexing are both throttled.
 255 | -    if (!initialSync || (fHeader && !clientmodel->node().getReindex()) || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {
 256 | -        //pass an async signal to the UI thread
 257 | +    const ClientModel::NotificationStatus status = GetNotificationStatus(clientmodel->node(), initialSync);
 258 | +    const bool throttle = (status == ClientModel::NotificationStatus::INIT_DOWNLOAD && !fHeader) ||
    ryanofsky commented at 6:13 PM on February 26, 2020:

    I don't see how this could be right. It appears to be never calling numBlockChanged at all during POST_INIT state

    Usage of throttle here is also potentially confusing. "throttle" bool should indicate when to slow down notifications, not when to send them

    hebasto commented at 9:37 PM on March 4, 2020:

    Fixed.

  9. ryanofsky commented at 6:39 PM on February 26, 2020: contributor

    See comments below, but some parts of this change seem buggy, or I'm not understanding them.

    In my comments from #18121, I was trying to suggest having the node tell the gui its state, instead of having the gui query the node. I think it's better for the node to just tell the GUI its state, instead of having the GUI piece together information and make assumptions. Here's what I was suggesting: c97298a5f9416561ab817c168c029a64090f107b (branch). Feel free to use these changes

  10. hebasto force-pushed on Mar 4, 2020
  11. hebasto force-pushed on Mar 4, 2020
  12. hebasto commented at 9:36 PM on March 4, 2020: member

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

    Also rebased due to #17399.

  13. hebasto renamed this:
    qt: Use NotificationStatus enum for signals to GUI
    qt: Use SynchronizationState enum for signals to GUI
    on Mar 4, 2020
  14. DrahtBot cross-referenced this on Mar 5, 2020 from issue gui: Bilingual GUI error messages by hebasto
  15. DrahtBot cross-referenced this on Mar 18, 2020 from issue Add ChainstateManager, remove BlockManager global by jamesob
  16. DrahtBot added the label Needs rebase on Apr 10, 2020
  17. in src/util/system.h:115 in ebc93a0d80 outdated 
     106 | @@ -107,6 +107,12 @@ inline bool IsSwitchChar(char c)
 107 |  #endif
 108 |  }
 109 |  
 110 | +enum class SynchronizationState {
 111 | +    INIT_DOWNLOAD,
 112 | +    INIT_REINDEX,
 113 | +    POST_INIT
 114 | +};
 115 | +
    ryanofsky commented at 8:35 PM on May 18, 2020:

    In commit "refactor: Pass SynchronizationState enum to GUI" (ebc93a0d80b2fa899753e97be480ff3f290e9789)

    I think it probably doesn't make sense to have this in system.h, which mostly contains lower level operating system stuff. I'd probably move it to validation.h here https://github.com/bitcoin/bitcoin/blob/dc5333d31f280e09bb1e8cdacfbe842f4ab9e69b/src/validation.h#L105 near the related reindex/reimporting variables. It could also go in a src/util/validation.h header (which existed previously but was recently deleted)

    Could also add a descriptive comment for the enum like /** Current sync state passed to tip changed callbacks. */.

    Could also reorder enum to reflect order the states occur: Reindex -> Download -> Post-Init

    hebasto commented at 12:11 AM on May 19, 2020:

    Updated.

  18. ryanofsky approved
  19. ryanofsky commented at 9:01 PM on May 18, 2020: contributor

    Code review ACK 7a2a5edc09986bcbcfd370ece644e7c31fa71b25. This looks great! Sorry I missed the previous update. I really like the way the PR is broken up now and dropping unused arguments.

    A followup to this PR would be able to remove ClientModel::getBlockSource too, but it's good to postpone that since this PR is already changing a number of things

  20. ryanofsky cross-referenced this on May 18, 2020 from issue relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload by jonasschnelli
  21. ryanofsky cross-referenced this on May 18, 2020 from issue gui: Long or blocking calls should be async by promag
  22. refactor: Remove unused bool parameter in BlockNotifyGenesisWait() 1df77014d8
  23. refactor: Remove unused bool parameter in RPCNotifyBlockChange() 2bec309ad6
  24. refactor: Pass SynchronizationState enum to GUI
    Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
    1dab574edf
  25. refactor: Remove Node::getReindex() call from GUI 3c709aa69d
  26. qt: Add SynchronizationState enum to signal parameter 06d519f0b4
  27. refactor: Remove Node:: queries from GUI a0d0f1c6c3
  28. hebasto force-pushed on May 19, 2020
  29. hebasto commented at 12:10 AM on May 19, 2020: member

    Updated 7a2a5edc09986bcbcfd370ece644e7c31fa71b25 -> a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc (pr18152.03 -> pr18152.04, diff):

  30. DrahtBot removed the label Needs rebase on May 19, 2020
  31. in src/qt/bitcoingui.cpp:934 in a0d0f1c6c3 
     931 | +void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, bool header, SynchronizationState sync_state)
 932 |  {
 933 |  // Disabling macOS App Nap on initial sync, disk and reindex operations.
 934 |  #ifdef Q_OS_MAC
 935 | -    (m_node.isInitialBlockDownload() || m_node.getReindex() || m_node.getImporting()) ? m_app_nap_inhibitor->disableAppNap() : m_app_nap_inhibitor->enableAppNap();
 936 | +    if (sync_state == SynchronizationState::POST_INIT) {
    jonasschnelli commented at 6:37 AM on May 19, 2020:

    We drop the check on fImporting here? Is that a problem?

    hebasto commented at 7:24 AM on May 19, 2020:

    In the (m_node.isInitialBlockDownload() || m_node.getReindex() || m_node.getImporting()) expression all but the first operand are redundant as in CChainState::IsInitialBlockDownload() we have: https://github.com/bitcoin/bitcoin/blob/362f9c60a54e673bb3daa8996f86d4bc7547eb13/src/validation.cpp#L1302-L1303

    Right?

    jonasschnelli commented at 7:33 AM on May 19, 2020:

    I see. It was unnecessary in the first place as it looks.

  32. jonasschnelli commented at 7:03 AM on May 19, 2020: contributor

    Core Review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc (modulo question). Lightly tested (mainnet catchup about 800 blocks). The annoying GUI blockings during normal catch-up operation are gone. Haven't tested reindex.

  33. DrahtBot cross-referenced this on May 19, 2020 from issue Reduce cs_main lock accumulation during GUI startup by jonasschnelli
  34. DrahtBot cross-referenced this on May 19, 2020 from issue gui: Improve thread naming by hebasto
  35. ryanofsky approved
  36. ryanofsky commented at 12:10 PM on May 19, 2020: contributor

    Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)

  37. jonasschnelli merged this on May 19, 2020
  38. jonasschnelli closed this on May 19, 2020
  39. jonasschnelli commented at 12:36 PM on May 19, 2020: contributor

    I'm not opposed to backport this since the Mac-Only GUI freeze is IMO an ugly bug.

  40. DrahtBot cross-referenced this on May 19, 2020 from issue gui: Balance/TxStatus polling update based on last block hash. by furszy
  41. hebasto deleted the branch on May 19, 2020
  42. promag commented at 8:50 AM on May 21, 2020: member

    Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc.

  43. jonasschnelli referenced this in commit f4b603cff6 on May 29, 2020
  44. sidhujag referenced this in commit 5b92706601 on May 31, 2020
  45. jasonbcox referenced this in commit 03c8acb87d on Nov 9, 2020
  46. bitcoin locked this on Feb 15, 2022
Contributors
hebastoDrahtBotryanofskyjonasschnellipromag
Labels
GUI
Linked (view graph)
#16874 gui: Long or blocking calls should be async#17399 validation: Templatize ValidationState instead of subclassing#18121 gui: Throttle GUI update pace when -reindex#19007 relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC