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

furszy · 2020-01-23T18:39:06Z

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, whatever is presented to fix it, this should use the same flow too.

[Edit - 24/01/2020]

Have added #17905 comment fix here too.

ryanofsky · 2020-01-23T19:17:17Z

Concept ACK. Will look more closely but I think this change seems good. It would also be good to follow up with #17905 (comment) and switch to hashes instead of heights, so GUI is updated reliably if there are reorgs

DrahtBot · 2020-01-23T19:32:13Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: remove ::vpwallets and related global variables #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
refactor: Move wallet methods out of chain.h and node.h #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)

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.

furszy · 2020-01-23T19:38:47Z

agree @ryanofsky . Once you cache the best tip hash in clientModel, I can just code a getter there and replace the cachedNumBlocks here for the hash.

ryanofsky · 2020-01-23T19:51:36Z

Once you cache the best tip hash in clientModel

To be sure, I didn't start to implement this at all. It'd be a welcome followup to this PR and #17905, or something that could be done alongside

furszy · 2020-01-23T20:10:35Z

k. if you didn't start it (and want me doing it), i can tackle it over night. Just give me green light to not end up with both doing the same work.

ryanofsky · 2020-01-23T20:17:48Z

k. if you didn't start it, i can tackle it over night. Just give me green light to not end up with both doing the same work.

🚥

Feel free to go ahead! I don't have immediate plans to work on this, and have no problem closing/rebasing/replacing my other PR if your changes supercede or conflict with it

furszy · 2020-01-24T00:57:45Z

Done.
The last commit is not really needed, it's just wrapping the tip data. I looked for a more manageable structure than continue adding arguments to the functions.

[Edit: seems that travis is still failing, will check it tomorrow]

ryanofsky

Conditional code review ACK cacb52feb81b22801a2747c3ae40369972825f5d if std::atomic<uint256> errors are fixed. The caching logic in the GUI all seems correct though.

src/interfaces/node.cpp

src/interfaces/node.h

src/qt/clientmodel.cpp

src/qt/clientmodel.h

src/qt/transactionrecord.cpp

src/qt/walletmodel.cpp

src/qt/walletmodel.h

src/interfaces/node.h

promag · 2020-01-26T19:14:35Z

Concept ACK.

src/qt/walletmodel.cpp

src/interfaces/node.cpp

hebasto · 2020-04-22T13:49:54Z

@furszy Mind rebasing if you are still working on it?

furszy · 2020-04-22T22:20:42Z

Yep sure, will check the PRs that have been merged over the area and update/rebase this one. If my memory doesn't fail me, the first commit will not be needed anymore and this PR will be mainly focused on moving the best tip cached height to the block hash.

[ClientModel] best header/block hash cached.

…ockTip signals.

furszy · 2020-05-23T23:08:54Z

Thanks for the quick feedback @hebasto. PR updated per repo's conventions suggestions 👍 .

hebasto

re-ACK a06e845, suggested style changes implemented since the previous 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

ryanofsky · 2020-05-26T19:31:59Z

src/qt/transactiontablemodel.cpp

 {
    Q_UNUSED(parent);
-    TransactionRecord *data = priv->index(walletModel->wallet(), walletModel->getNumBlocks(), row);
+    TransactionRecord* data = priv->index(walletModel->wallet(), walletModel->clientModel().getBestBlockHash(), row);


In commit "Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals." (2f86720)

Note: there is a change in behavior here. Previously the transaction would be updated when WalletModel::cachedNumBlocks changed (when the wallet model balance polling timer detected the wallet processed a new block). Now the transaction will get updated ClientModel::m_cached_tip_blocks changes (when there is a new node BlockTipChanged event). This means transaction display should now get updated a little faster since it won't depend on the wallet balance timer, but now it will do some wasted work during the time it takes for the wallet to update its last processed block after the node tip changes.

Ideally the wallet model would have its own wallet last block processed member similar CWallet::m_last_block_processed, that could be used here, and which would get updated from a wallet notification as described #16874 (comment), not requiring polling or locking cs_wallet to get an up to date value.

EDIT: To be clear, this is all good for the current PR, just wanted to document what changed here and what future steps could be

jonasschnelli · 2020-05-29T08:36:19Z

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

…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

Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up in a deadlock. `ClientModel::m_cached_tip_blocks` is protected by `ClientModel::m_cached_tip_mutex`. There are two access paths that lock the two mutexes in opposite order: ``` validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip() ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost ... qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex ``` and ``` qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash() interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main ``` From `debug.log`: ``` POTENTIAL DEADLOCK DETECTED Previous lock order was: m_cs_chainstate validation.cpp:2851 (1) cs_main validation.cpp:2868 ::mempool.cs validation.cpp:2868 (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 Current lock order is: (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (1) ::cs_main interfaces/node.cpp:200 ``` The possible deadlock was introduced in bitcoin#17993

f46b678 qt: lock cs_main, m_cached_tip_mutex in that order (Vasil Dimov) Pull request description: Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up in a deadlock. `ClientModel::m_cached_tip_blocks` is protected by `ClientModel::m_cached_tip_mutex`. There are two access paths that lock the two mutexes in opposite order: ``` validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip() ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost ... qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex ``` and ``` qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash() interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main ``` From `debug.log`: ``` POTENTIAL DEADLOCK DETECTED Previous lock order was: m_cs_chainstate validation.cpp:2851 (1) cs_main validation.cpp:2868 ::mempool.cs validation.cpp:2868 (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 Current lock order is: (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (1) ::cs_main interfaces/node.cpp:200 ``` The possible deadlock was introduced in #17993 ACKs for top commit: jonasschnelli: Tested ACK f46b678 Tree-SHA512: 904f24b39bdc97c4d0ecb897a6980d8d479814535eb167e23105238800ea2f1f85273e3370cf894db58bc597f94c4f2e81fb68d0ff3362d468c16af5ce8f5d78

f46b678 qt: lock cs_main, m_cached_tip_mutex in that order (Vasil Dimov) Pull request description: Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up in a deadlock. `ClientModel::m_cached_tip_blocks` is protected by `ClientModel::m_cached_tip_mutex`. There are two access paths that lock the two mutexes in opposite order: ``` validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip() ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost ... qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex ``` and ``` qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash() interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main ``` From `debug.log`: ``` POTENTIAL DEADLOCK DETECTED Previous lock order was: m_cs_chainstate validation.cpp:2851 (1) cs_main validation.cpp:2868 ::mempool.cs validation.cpp:2868 (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 Current lock order is: (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (1) ::cs_main interfaces/node.cpp:200 ``` The possible deadlock was introduced in bitcoin#17993 ACKs for top commit: jonasschnelli: Tested ACK f46b678 Tree-SHA512: 904f24b39bdc97c4d0ecb897a6980d8d479814535eb167e23105238800ea2f1f85273e3370cf894db58bc597f94c4f2e81fb68d0ff3362d468c16af5ce8f5d78

hebasto · 2020-06-19T13:16:28Z

The regression, that was supposedly introduced by this PR, has been detected in bitcoin-core/gui#7.

d906aaa qt: Fix regression in TransactionTableModel (Hennadii Stepanov) Pull request description: Since bitcoin/bitcoin#17993 a crash is possible on exit. Steps to reproduce: - precondition: the old chain - start `bitcoin-qt` - wait until sync - on main window: Menu -> File -> Quit - crash This PR is based on ryanofsky's [suggestion](#7 (comment)). Fixes #7. ACKs for top commit: promag: Code review ACK d906aaa. ryanofsky: Code review ACK d906aaa. Only changes are squashing, adding assert and adding const vasild: ACK d906aaa Tree-SHA512: 99a475fd90dff50407a58537fdc6099a2a074018e9078452bf86defc1a4b9e546aa94f916d242355900b21638c6cfef845598a5282661a9343556c4514eb155f

Since bitcoin#17993 a crash is possible on exit. Co-authored-by: Russell Yanofsky <[email protected]>

Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up in a deadlock. `ClientModel::m_cached_tip_blocks` is protected by `ClientModel::m_cached_tip_mutex`. There are two access paths that lock the two mutexes in opposite order: ``` validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip() ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost ... qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex ``` and ``` qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash() interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main ``` From `debug.log`: ``` POTENTIAL DEADLOCK DETECTED Previous lock order was: m_cs_chainstate validation.cpp:2851 (1) cs_main validation.cpp:2868 ::mempool.cs validation.cpp:2868 (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 Current lock order is: (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (1) ::cs_main interfaces/node.cpp:200 ``` The possible deadlock was introduced in bitcoin/bitcoin#17993

Summary: PR 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. bitcoin/bitcoin@2f86720 > [ClientModel] best header/block hash cached. bitcoin/bitcoin@f46b678 > qt: lock cs_main, m_cached_tip_mutex in that order > > Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in > the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up > in a deadlock. > > `ClientModel::m_cached_tip_blocks` is protected by > `ClientModel::m_cached_tip_mutex`. There are two access paths that > lock the two mutexes in opposite order: > > ``` > validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main > validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip() > ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost > ... > qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex > ``` > > and > > ``` > qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex > qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash() > interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main > ``` > > From `debug.log`: > > ``` > POTENTIAL DEADLOCK DETECTED > Previous lock order was: > m_cs_chainstate validation.cpp:2851 > (1) cs_main validation.cpp:2868 > ::mempool.cs validation.cpp:2868 > (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 > Current lock order is: > (2) m_cached_tip_mutex qt/clientmodel.cpp:119 > (1) ::cs_main interfaces/node.cpp:200 > ``` > > The possible deadlock was introduced in > bitcoin/bitcoin#17993 This is a backport of Core [[bitcoin/bitcoin#17993 | PR17993]] [1/2] and [[bitcoin/bitcoin#19132 | PR19132]] Test Plan: `ninja && src/qt/bitcoin-qt` Send a transaction, wait for a few block and check that the tx status is correctly updated in the interface. Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9253

…ockTip signals. Summary: This is a backport of Core [[bitcoin/bitcoin#17993 | PR17993]] [2/2] bitcoin/bitcoin@a06e845 Depends on D9253 Test Plan: `ninja && src/qt/bitcoin-qt` Send a transaction, wait for a few block and check that the tx status is correctly updated in the interface. Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9254

furszy requested a review from ryanofsky January 23, 2020 18:40

DrahtBot added GUI Tests labels Jan 23, 2020

fanquake removed the Tests label Jan 23, 2020

furszy force-pushed the 2020_avoid_unnecessary_lock_in_balance_polling branch 2 times, most recently from 901d7d2 to cacb52f Compare January 24, 2020 17:10

ryanofsky approved these changes Jan 24, 2020

View reviewed changes

furszy force-pushed the 2020_avoid_unnecessary_lock_in_balance_polling branch 2 times, most recently from 67c2730 to d851606 Compare January 25, 2020 04:28

promag reviewed Jan 30, 2020

View reviewed changes

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

furszy force-pushed the 2020_avoid_unnecessary_lock_in_balance_polling branch from d851606 to 3d03bac Compare January 30, 2020 18:03

promag reviewed Feb 3, 2020

View reviewed changes

src/interfaces/node.cpp Outdated Show resolved Hide resolved

src/interfaces/node.cpp Outdated Show resolved Hide resolved

ryanofsky mentioned this pull request Feb 5, 2020

wallet: Remove calls to Chain::Lock methods #17954

Merged

DrahtBot added the Needs rebase label Feb 13, 2020

ryanofsky mentioned this pull request Feb 25, 2020

gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged #18160

Merged

furszy added 2 commits May 23, 2020 20:02

Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.

2f86720

[ClientModel] best header/block hash cached.

BlockTip struct created and connected to notifyHeaderTip and notifyBl…

a06e845

…ockTip signals.

furszy force-pushed the 2020_avoid_unnecessary_lock_in_balance_polling branch from 4062b9e to a06e845 Compare May 23, 2020 23:03

hebasto approved these changes May 24, 2020

View reviewed changes

ryanofsky approved these changes May 26, 2020

View reviewed changes

fanquake mentioned this pull request May 27, 2020

doc: Separate repository for the gui #19071

Merged

This was referenced May 29, 2020

refactor: remove ::vpwallets and related global variables #19101

Merged

refactor: Move wallet methods out of chain.h and node.h #19099

Merged

jonasschnelli merged commit f4b603c into bitcoin:master May 29, 2020

vasild mentioned this pull request Jun 1, 2020

qt: lock cs_main, m_cached_tip_mutex in that order #19132

Merged

hebasto mentioned this pull request Jun 19, 2020

Crash on exit bitcoin-core/gui#7

Closed

hebasto mentioned this pull request Jun 19, 2020

Fix regression in TransactionTableModel bitcoin-core/gui#8

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 26, 2020

qt: Fix regression in TransactionTableModel

d906aaa

Since bitcoin#17993 a crash is possible on exit. Co-authored-by: Russell Yanofsky <[email protected]>

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 7, 2020

qt: Fix regression in TransactionTableModel

4dfbb17

Since bitcoin#17993 a crash is possible on exit. Co-authored-by: Russell Yanofsky <[email protected]>

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

furszy deleted the 2020_avoid_unnecessary_lock_in_balance_polling branch November 29, 2022 14:09

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

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

Uh oh!

Conversation

furszy commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jan 23, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

furszy commented Jan 23, 2020

Uh oh!

ryanofsky commented Jan 23, 2020

Uh oh!

furszy commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jan 23, 2020

Uh oh!

furszy commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag commented Jan 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Apr 22, 2020

Uh oh!

furszy commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented May 23, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 26, 2020 • 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!

jonasschnelli commented May 29, 2020

Uh oh!

hebasto commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

furszy commented Jan 23, 2020 •

edited

Loading

DrahtBot commented Jan 23, 2020 •

edited

Loading

furszy commented Jan 23, 2020 •

edited

Loading

furszy commented Jan 24, 2020 •

edited

Loading

furszy commented Apr 22, 2020 •

edited

Loading

ryanofsky May 26, 2020 •

edited

Loading