qt: Use SynchronizationState enum for signals to GUI

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

DrahtBot added the label GUI on Feb 15, 2020

DrahtBot commented at 6:37 pm on February 15, 2020: member

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

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.

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.

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.

ryanofsky commented at 6:39 pm on February 26, 2020: member

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

hebasto force-pushed on Mar 4, 2020

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.

hebasto renamed this:
~~qt: Use NotificationStatus enum for signals to GUI~~
qt: Use SynchronizationState enum for signals to GUI
on Mar 4, 2020

DrahtBot added the label Needs rebase on Apr 10, 2020

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 0:11 am on May 19, 2020:

Updated.

ryanofsky approved

ryanofsky commented at 9:01 pm on May 18, 2020: member

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

refactor: Remove unused bool parameter in BlockNotifyGenesisWait() 1df77014d8

refactor: Remove unused bool parameter in RPCNotifyBlockChange() 2bec309ad6

refactor: Pass SynchronizationState enum to GUI

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

1dab574edf

refactor: Remove Node::getReindex() call from GUI 3c709aa69d

qt: Add SynchronizationState enum to signal parameter 06d519f0b4

refactor: Remove Node:: queries from GUI a0d0f1c6c3

hebasto force-pushed on May 19, 2020

hebasto commented at 0:10 am on May 19, 2020: member

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

rebased due to the conflict with #16224 and #17737
addressed @ryanofsky’s comment

DrahtBot removed the label Needs rebase on May 19, 2020

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.

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.

ryanofsky approved

ryanofsky commented at 12:10 pm on May 19, 2020: member

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

jonasschnelli merged this on May 19, 2020

jonasschnelli closed this on May 19, 2020

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.

hebasto deleted the branch on May 19, 2020

promag commented at 8:50 am on May 21, 2020: member

Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc.

jonasschnelli referenced this in commit f4b603cff6 on May 29, 2020

sidhujag referenced this in commit 5b92706601 on May 31, 2020

jasonbcox referenced this in commit 03c8acb87d on Nov 9, 2020

DrahtBot locked this on Feb 15, 2022

qt: Use SynchronizationState enum for signals to GUI #18152

Conflicts