qt: Use SynchronizationState enum for signals to GUI #18152

hebasto · 2020-02-15T14:56:04Z

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 · 2020-02-15T18:37:26Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

ryanofsky

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: c97298a (branch). Feel free to use these changes

src/qt/clientmodel.cpp

hebasto · 2020-03-04T21:36:52Z

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

Also rebased due to #17399.

ryanofsky

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

ryanofsky · 2020-05-18T20:35:32Z

src/util/system.h

+enum class SynchronizationState {
+    INIT_DOWNLOAD,
+    INIT_REINDEX,
+    POST_INIT
+};
+


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

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

bitcoin/src/validation.h

Line 105 in dc5333d

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

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

hebasto · 2020-05-19T00:10:58Z

Updated 7a2a5ed -> a0d0f1c (pr18152.03 -> pr18152.04, diff):

rebased due to the conflict with gui: Bilingual GUI error messages #16224 and Add ChainstateManager, remove BlockManager global #17737
addressed @ryanofsky's comment

jonasschnelli · 2020-05-19T06:37:47Z

src/qt/bitcoingui.cpp

 {
 // Disabling macOS App Nap on initial sync, disk and reindex operations.
 #ifdef Q_OS_MAC
-    (m_node.isInitialBlockDownload() || m_node.getReindex() || m_node.getImporting()) ? m_app_nap_inhibitor->disableAppNap() : m_app_nap_inhibitor->enableAppNap();
+    if (sync_state == SynchronizationState::POST_INIT) {


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

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:

bitcoin/src/validation.cpp

Lines 1302 to 1303 in 362f9c6

if (fImporting || fReindex)

return true;

Right?

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

jonasschnelli · 2020-05-19T07:03:02Z

Core Review ACK a0d0f1c (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

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

jonasschnelli · 2020-05-19T12:36:08Z

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

promag · 2020-05-21T08:50:28Z

Code review ACK a0d0f1c.

…k hash. a06e845 BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals. (furszy) 2f86720 Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals. (furszy) Pull request description: Rationale: The height based polling in the GUI is an issue on chain reorgs. Any new tip signal with the same height as the one that it's cached in the model was not triggering the GUI update (interpreting it as the same same block when it could receive a different one). Ending up with bad information presented in the GUI. This PR essentially changes the last cached height to be a last cached block hash. --- Old historical information of this PR. As the tip height is cached and updated via signaling in clientModel, there is no need to continue locking `cs_main` on every balance poll (`m_node.getNumBlocks()` method call). Extra topic: Would suggest to change the `cachedNumBlocks` field inside `walletModel` to a more understandable name, maybe `nLastBalanceUpdateHeight`. And finally, this will have the equal height reorg issue mentioned [here](#17905 (comment)), whatever is presented to fix it, this should use the same flow too. **[Edit - 24/01/2020]** Have added #[17905](#17905 (comment)) comment fix here too. ACKs for top commit: jonasschnelli: utACK a06e845 - it would be great to have QT unit tests (in this case for a reorg) that either automatically inspect the window content based on accessibility and tests for expected values or at least allow for quick manual re-testing (screenshots, automatically create UI situations). hebasto: re-ACK a06e845, suggested style changes implemented since the [previous](#17993 (review)) review. ryanofsky: Code review ACK a06e845. A lot of changes since the last review: rebase after sync_state introduction #18152 and tryGetBalances revert #18587, reverting getLastBlockTime change, fixing spacing and initializations and renaming some variables Tree-SHA512: 835e587a8296df9899cccd7b3e598a5970942b640e432e6a32de0b4eaea5b40f9271258f089ec033595311707b74a0f7187ecf8ed397c713e1153e2714072975

…st block hash. a06e845 BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals. (furszy) 2f86720 Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals. (furszy) Pull request description: Rationale: The height based polling in the GUI is an issue on chain reorgs. Any new tip signal with the same height as the one that it's cached in the model was not triggering the GUI update (interpreting it as the same same block when it could receive a different one). Ending up with bad information presented in the GUI. This PR essentially changes the last cached height to be a last cached block hash. --- Old historical information of this PR. As the tip height is cached and updated via signaling in clientModel, there is no need to continue locking `cs_main` on every balance poll (`m_node.getNumBlocks()` method call). Extra topic: Would suggest to change the `cachedNumBlocks` field inside `walletModel` to a more understandable name, maybe `nLastBalanceUpdateHeight`. And finally, this will have the equal height reorg issue mentioned [here](bitcoin#17905 (comment)), whatever is presented to fix it, this should use the same flow too. **[Edit - 24/01/2020]** Have added #[17905](bitcoin#17905 (comment)) comment fix here too. ACKs for top commit: jonasschnelli: utACK a06e845 - it would be great to have QT unit tests (in this case for a reorg) that either automatically inspect the window content based on accessibility and tests for expected values or at least allow for quick manual re-testing (screenshots, automatically create UI situations). hebasto: re-ACK a06e845, suggested style changes implemented since the [previous](bitcoin#17993 (review)) review. ryanofsky: Code review ACK a06e845. A lot of changes since the last review: rebase after sync_state introduction bitcoin#18152 and tryGetBalances revert bitcoin#18587, reverting getLastBlockTime change, fixing spacing and initializations and renaming some variables Tree-SHA512: 835e587a8296df9899cccd7b3e598a5970942b640e432e6a32de0b4eaea5b40f9271258f089ec033595311707b74a0f7187ecf8ed397c713e1153e2714072975

Summary: ``` This PR is a followup of #18121 and: addresses confusion about GUI notification throttling conditions removes isInitialBlockDownload() call from the GUI back to the node (on macOS). ``` Backport of core [[bitcoin/bitcoin#18152 | PR18152]]. Depends on D8334. Test Plan: ninja all check-all Also run the tests on OSX. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8335

hebasto mentioned this pull request Feb 15, 2020

gui: Throttle GUI update pace when -reindex #18121

Merged

DrahtBot added the GUI label Feb 15, 2020

This was referenced Feb 15, 2020

qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877

Closed

gui: Prevent idle sleep in macos while in IBD #16145

Closed

ryanofsky reviewed Feb 26, 2020

View reviewed changes

src/qt/clientmodel.cpp Outdated Show resolved Hide resolved

src/qt/clientmodel.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 20200215-pr18121-followup2 branch 2 times, most recently from f52bafc to 7a2a5ed Compare March 4, 2020 21:34

hebasto changed the title ~~qt: Use NotificationStatus enum for signals to GUI~~ qt: Use SynchronizationState enum for signals to GUI Mar 4, 2020

DrahtBot mentioned this pull request Mar 5, 2020

gui: Bilingual GUI error messages #16224

Merged

DrahtBot mentioned this pull request Mar 18, 2020

Add ChainstateManager, remove BlockManager global #17737

Merged

DrahtBot added the Needs rebase label Apr 10, 2020

ryanofsky approved these changes May 18, 2020

View reviewed changes

This was referenced May 18, 2020

relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload #19007

Closed

gui: Long or blocking calls should be async #16874

Closed

hebasto and others added 6 commits May 19, 2020 02:39

refactor: Remove unused bool parameter in BlockNotifyGenesisWait()

1df7701

refactor: Remove unused bool parameter in RPCNotifyBlockChange()

2bec309

refactor: Pass SynchronizationState enum to GUI

1dab574

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

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

3c709aa

qt: Add SynchronizationState enum to signal parameter

06d519f

refactor: Remove Node:: queries from GUI

a0d0f1c

hebasto force-pushed the 20200215-pr18121-followup2 branch from 7a2a5ed to a0d0f1c Compare May 19, 2020 00:07

DrahtBot removed the Needs rebase label May 19, 2020

jonasschnelli reviewed May 19, 2020

View reviewed changes

This was referenced May 19, 2020

Reduce cs_main lock accumulation during GUI startup #19011

Merged

gui: Improve thread naming #18790

Merged

ryanofsky approved these changes May 19, 2020

View reviewed changes

jonasschnelli merged commit d44dd51 into bitcoin:master May 19, 2020

DrahtBot mentioned this pull request May 19, 2020

gui: Balance/TxStatus polling update based on last block hash. #17993

Merged

hebasto deleted the 20200215-pr18121-followup2 branch May 19, 2020 15:49

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

qt: Use SynchronizationState enum for signals to GUI #18152

qt: Use SynchronizationState enum for signals to GUI #18152

Uh oh!

hebasto commented Feb 15, 2020

Uh oh!

DrahtBot commented Feb 15, 2020 •

edited

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented Mar 4, 2020

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

ryanofsky May 18, 2020

Uh oh!

hebasto May 19, 2020

Uh oh!

hebasto commented May 19, 2020

Uh oh!

jonasschnelli May 19, 2020

Uh oh!

hebasto May 19, 2020

Uh oh!

jonasschnelli May 19, 2020

Uh oh!

jonasschnelli commented May 19, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

jonasschnelli commented May 19, 2020

Uh oh!

promag commented May 21, 2020

Uh oh!

Uh oh!

qt: Use SynchronizationState enum for signals to GUI #18152

qt: Use SynchronizationState enum for signals to GUI #18152

Uh oh!

Conversation

hebasto commented Feb 15, 2020

Uh oh!

DrahtBot commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented Mar 4, 2020

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!

ryanofsky May 18, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto May 19, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 19, 2020

Uh oh!

jonasschnelli May 19, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto May 19, 2020

Choose a reason for hiding this comment

Uh oh!

jonasschnelli May 19, 2020

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 19, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 19, 2020

Uh oh!

promag commented May 21, 2020

Uh oh!

Uh oh!

DrahtBot commented Feb 15, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading