gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged #18587

ryanofsky · 2020-04-10T21:33:22Z

Main commit gui: Avoid wallet tryGetBalances calls is one-line change to WalletModel::pollBalanceChanged that returns early if there hasn't been a new TransactionChanged or BlockTip notification since the previous poll call. This is the same behavior that was implemented in #18160, now implemented in a simpler way.

The other commits are a straight revert of #18160, and two tweaks to avoid relying on WalletModel::m_client_model lifetime which were causing travis failures with earlier versions of this PR.

Motivation for this change is to be able to revert #18160 and cut down on unnecessary cross-process calls that happen when #18160 is combined with #10102

This PR is part of the process separation project.

promag · 2020-04-10T21:52:06Z

Worth mentioning that #17905 made this easy (#18160 (review)) and that #18160 was merged to fix #15015 in 0.19 and 0.20.

promag · 2020-04-10T22:00:39Z

src/qt/walletmodel.cpp

@@ -79,6 +79,10 @@ void WalletModel::updateStatus()

 void WalletModel::pollBalanceChanged()
 {
+    // Avoid recomputing wallet balances unless a TransactionChanged or
+    // BlockTip notification was received.
+    if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model.getNumBlocks()) return;


Maybe this should be >=? Can't tryGetBalances return a block ahead of m_client_model.getNumBlocks()?

Alternatively drop 2nd arg from tryGetBalances and use numBlocks = m_client_model.getNumBlocks()?

Maybe this should be >=? Can't tryGetBalances return a block ahead of m_client_model.getNumBlocks()?

Not sure what goal of this suggestion is. >= would make the check less sensitive in case of a rollback. This code is already not sensitive enough because it is assuming chains the same height are identical.

Alternatively drop 2nd arg from tryGetBalances and use numBlocks = m_client_model.getNumBlocks()?

Again not sure reason for this suggestion. It would make the PR no longer a direct revert of 0933a37 and also no longer be correct when combined with #17954 and when the node tip runs ahead of the wallet last block processed.

Goal of this PR is just to make a one-line change that prevents unnecessary cross-process calls and allows #18160 to be reverted. There's room for improvement on top of PR, including removing the polling logic altogether, but by itself this PR should be an improvement on a few fronts

This code is already not sensitive enough because it is assuming chains the same height are identical

Indeed! 🤦

promag

Code review ACK 6dbc52b.

promag · 2020-04-10T22:35:52Z

src/qt/walletmodel.cpp

@@ -79,6 +79,10 @@ void WalletModel::updateStatus()

 void WalletModel::pollBalanceChanged()
 {
+    // Avoid recomputing wallet balances unless a TransactionChanged or
+    // BlockTip notification was received.
+    if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model.getNumBlocks()) return;


This code is already not sensitive enough because it is assuming chains the same height are identical

Indeed! 🤦

ryanofsky · 2020-04-15T15:50:18Z

Rebased 6dbc52b -> 605c3f1 (pr/ipc-bal.1 -> pr/ipc-bal.2, compare) due to silent conflict with #17954

ryanofsky · 2020-04-15T21:04:06Z

Travis failure in wallet_resendwallettransactions seems like it must be caused by a problem in master because this PR is just a gui change which shouldn't be affecting that test at all:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367626#L13955

Travis failure in address sanitizer build is probably caused this PR, and I'm going to try to debug and fix it locally. https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367629#L4240. Hopefully it's just a test bug not a code bug

meshcollider · 2020-04-17T11:21:37Z

Concept ACK

DrahtBot · 2020-04-23T05:25:34Z

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

Conflicts

No conflicts as of last run.

ryanofsky · 2020-04-27T20:23:33Z

Updated 605c3f1 -> 6de7f9e (pr/ipc-bal.2 -> pr/ipc-bal.3, compare) to fix ClientModel use-after-free when shutdown requested https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367629#L4240 mentioned #18587 (comment)

Tweak of bitcoin#17905 to make gui display of transactions and balances more consistent. This change shouldn't cause visible effects in normal cases, just make GUI wallet code more internally correct and consistent.

This doesn't fix any current problem, but it makes balance checking code less fragile, and prevents use-after free travis error in next commit: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367629#L4240

…lockTip notifications interfaces::Wallet::tryGetBalances was recently updated in bitcoin#18160 to avoid computing balances internally, but this not efficient as it could be with bitcoin#10102 because tryGetBalances is an interprocess call. Implementing the TransactionChanged / BlockTip check outside of tryGetBalances also allows tryGetBalances to be simplified in next commit 'Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged"'.

…ged" This reverts commit 0933a37 from bitcoin#18160 which no longer an optimization since commit "gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications".

ryanofsky · 2020-05-01T15:49:14Z

Rebased 6de7f9e -> d3a56be (pr/ipc-bal.3 -> pr/ipc-bal.4, compare) due to silent conflict with #16426

jonasschnelli · 2020-05-20T08:59:01Z

utACK d3a56be

…Model::pollBalanceChanged d3a56be Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" (Russell Yanofsky) bf0a510 gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications (Russell Yanofsky) 2bc9b92 Cancel wallet balance timer when shutdown requested (Russell Yanofsky) 83f69fa Switch transaction table to use wallet height not node height (Russell Yanofsky) Pull request description: Main commit `gui: Avoid wallet tryGetBalances calls` is one-line change to `WalletModel::pollBalanceChanged` that returns early if there hasn't been a new `TransactionChanged` or `BlockTip` notification since the previous poll call. This is the same behavior that was implemented in bitcoin#18160, now implemented in a simpler way. The other commits are a straight revert of bitcoin#18160, and two tweaks to avoid relying on `WalletModel::m_client_model` lifetime which were causing travis failures with earlier versions of this PR. Motivation for this change is to be able to revert bitcoin#18160 and cut down on unnecessary cross-process calls that happen when bitcoin#18160 is combined with bitcoin#10102 This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). ACKs for top commit: jonasschnelli: utACK d3a56be Tree-SHA512: 3cd31ca515e77c3bd7160d3f1ea0dce5050d4038b2aa441b6f66b8599bd413d81ca5542a197806e773d6092dd1d26830932b1cecbc95298b1f1ab41099e2f12f

…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

…l::pollBalanceChanged Summary: d3a56be77a9d112cde4baef4314882170b9f228f Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" (Russell Yanofsky) bf0a510981ddc28c754881ca21c50ab18e5f2b59 gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications (Russell Yanofsky) 2bc9b92ed8b7736ad67876398a0bb8287f57e9b3 Cancel wallet balance timer when shutdown requested (Russell Yanofsky) 83f69fab3a1ae97c5cff8ba1e6fd191b0fa264bb Switch transaction table to use wallet height not node height (Russell Yanofsky) Pull request description: Main commit `gui: Avoid wallet tryGetBalances calls` is one-line change to `WalletModel::pollBalanceChanged` that returns early if there hasn't been a new `TransactionChanged` or `BlockTip` notification since the previous poll call. This is the same behavior that was implemented in #18160, now implemented in a simpler way. The other commits are a straight revert of #18160, and two tweaks to avoid relying on `WalletModel::m_client_model` lifetime which were causing travis failures with earlier versions of this PR. Motivation for this change is to be able to revert #18160 and cut down on unnecessary cross-process calls that happen when #18160 is combined with #10102 This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). --- Backport of Core [[bitcoin/bitcoin#18587 | PR18587]] Test Plan: ninja all check-check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9168

promag reviewed Apr 10, 2020

View reviewed changes

DrahtBot added GUI Wallet labels Apr 10, 2020

fanquake mentioned this pull request Apr 10, 2020

0.19: gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged #18542

Closed

ryanofsky force-pushed the pr/ipc-bal branch from 6dbc52b to 605c3f1 Compare April 15, 2020 15:50

ryanofsky mentioned this pull request Apr 15, 2020

Multiprocess bitcoin #10102

Draft

DrahtBot mentioned this pull request Apr 23, 2020

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

Merged

ryanofsky force-pushed the pr/ipc-bal branch from 605c3f1 to 6de7f9e Compare April 25, 2020 19:04

ryanofsky added 4 commits May 1, 2020 06:59

Switch transaction table to use wallet height not node height

83f69fa

Tweak of bitcoin#17905 to make gui display of transactions and balances more consistent. This change shouldn't cause visible effects in normal cases, just make GUI wallet code more internally correct and consistent.

Cancel wallet balance timer when shutdown requested

2bc9b92

This doesn't fix any current problem, but it makes balance checking code less fragile, and prevents use-after free travis error in next commit: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367629#L4240

Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChan…

d3a56be

…ged" This reverts commit 0933a37 from bitcoin#18160 which no longer an optimization since commit "gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications".

ryanofsky force-pushed the pr/ipc-bal branch from 6de7f9e to d3a56be Compare May 1, 2020 15:48

ryanofsky mentioned this pull request May 18, 2020

gui: Long or blocking calls should be async #16874

Closed

jonasschnelli merged commit a587f85 into bitcoin:master May 20, 2020

jarolrod mentioned this pull request Mar 25, 2021

Qt: Only call tryGetBalances in pollBalanceChanged if the result will be used. #13612

Closed

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

gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged #18587

gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged #18587

ryanofsky commented Apr 10, 2020 •

edited

Loading

promag commented Apr 10, 2020

promag Apr 10, 2020

ryanofsky Apr 10, 2020

promag Apr 10, 2020

promag left a comment

promag Apr 10, 2020

ryanofsky commented Apr 15, 2020

ryanofsky commented Apr 15, 2020

meshcollider commented Apr 17, 2020

DrahtBot commented Apr 23, 2020 •

edited

Loading

ryanofsky commented Apr 27, 2020

ryanofsky commented May 1, 2020

jonasschnelli commented May 20, 2020 •

edited

Loading

gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged #18587

gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged #18587

Conversation

ryanofsky commented Apr 10, 2020 • edited Loading

promag commented Apr 10, 2020

promag Apr 10, 2020

Choose a reason for hiding this comment

ryanofsky Apr 10, 2020

Choose a reason for hiding this comment

promag Apr 10, 2020

Choose a reason for hiding this comment

promag left a comment

Choose a reason for hiding this comment

promag Apr 10, 2020

Choose a reason for hiding this comment

ryanofsky commented Apr 15, 2020

ryanofsky commented Apr 15, 2020

meshcollider commented Apr 17, 2020

DrahtBot commented Apr 23, 2020 • edited Loading

Conflicts

ryanofsky commented Apr 27, 2020

ryanofsky commented May 1, 2020

jonasschnelli commented May 20, 2020 • edited Loading

ryanofsky commented Apr 10, 2020 •

edited

Loading

DrahtBot commented Apr 23, 2020 •

edited

Loading

jonasschnelli commented May 20, 2020 •

edited

Loading