gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged #18160
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@MarcoFalke considering #18123 this should be backported too (if change is correct). |
|
Concept ACK about not wasting resources. |
hebasto
approved these changes
Feb 16, 2020
Comment on lines
+91
to
+92
| if(transactionTableModel) | ||
| transactionTableModel->updateConfirmations(); |
There was a problem hiding this comment.
nit: While these lines are touched already, why not apply correct format style?
There was a problem hiding this comment.
Forgot to mention to review with &w=1.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Might fix #15015. |
|
See also #10251 (could the discussion there be relevant here?) |
|
The goal here is to avoid unnecessary calls to Refactoring to make it asynchronous can also be done but I don't think it invalidates this change. |
instagibbs
approved these changes
Feb 25, 2020
There was a problem hiding this comment.
ACK 0933a37
Unrelated to this PR: Checking for staleness should be done via checking total chain work, not height changes. A re-org across a difficulty change threshold could result in balances not updating when they should.
| @@ -201,8 +201,11 @@ class Wallet | |||
| //! Get balances. | |||
| virtual WalletBalances getBalances() = 0; | |||
|
|
|||
| //! Get balances if possible without blocking. | |||
| virtual bool tryGetBalances(WalletBalances& balances, int& num_blocks) = 0; | |||
| //! Get balances if possible without waiting for chain and wallet locks. | |||
There was a problem hiding this comment.
This should mention that it won't attempt to get balances if height is same.
|
Can someone explain to me how this is supposed to work? Why does the balance not change when the block height stays the same? Can't we send out funds or receive them in the meantime (they might be untrusted, but still, the balance should change)? |
|
@MarcoFalke look at |
|
In that case it seems easier to just call it with the force parameter on every connected block and then remove the polling? |
|
The real deal should be async balance computation to not block the GUI thread. What you suggest is a bigger change and requires locks instead of try locks. |
|
I think this PR would be equivalent to adding: if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model.getNumBlocks()) return;to the top of the Assuming #17905 is a change we want to make anyway, keeping the changed detection in
|
|
@ryanofsky If we call So this PR tunes |
ryanofsky
approved these changes
Feb 26, 2020
There was a problem hiding this comment.
Code review ACK 0933a37, but I would prefer (not strongly) for #17905 to be merged first. This PR can be simpler if it is based on #17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out.
|
@ryanofsky is the alternative easy to backport? |
The suggested alternative makes this PR smaller and probably easier to backport, but it builds on #17905 which is bigger and does have minor conflicts with 0.18 and 0.19. If the goal is to backport #18160 without #17905, I'd suggest leaving this PR in its current form so it is easier to backport. If the goal is to backport both PRs, then adopting the suggested alternative won't have an impact on backporting. The alternative would just make this PR simpler and let these changes be entirely contained in src/qt/ instead of requiring a complication to src/interfaces/ that will be unnecessary and confusing after #17905 But again no strong preference on what order things are merged. I am just saying there will be less to clean up if #17905 is merged first and src/interfaces can be left alone. |
|
Well we could tag this for backport to 0.19 and wait for progress on #17905 or 0.19.1. |
|
IMO this should go into 0.20. @ryanofsky @jonasschnelli @laanwj |
|
@promag Too late for any non-fixes... |
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 10, 2020
…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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 15, 2020
…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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 15, 2020
…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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 15, 2020
…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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 15, 2020
…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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 15, 2020
…anged or BlockTip notifications gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip 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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 15, 2020
…alanceChanged" Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" 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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 25, 2020
…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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 25, 2020
…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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 1, 2020
…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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 1, 2020
…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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 1, 2020
…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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 1, 2020
…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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 1, 2020
…anged or BlockTip notifications gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip 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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 1, 2020
…alanceChanged" Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" 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
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 8, 2020
…anged or BlockTip notifications gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip 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"'.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 8, 2020
…alanceChanged" Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" This reverts commit 0933a37 from bitcoin#18160 which no longer an optimization since commit "gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications".
jonasschnelli
added a commit
that referenced
this pull request
May 20, 2020
…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 #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). ACKs for top commit: jonasschnelli: utACK d3a56be Tree-SHA512: 3cd31ca515e77c3bd7160d3f1ea0dce5050d4038b2aa441b6f66b8599bd413d81ca5542a197806e773d6092dd1d26830932b1cecbc95298b1f1ab41099e2f12f
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 20, 2020
…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
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Jun 9, 2020
Github-Pull: bitcoin#18160 Rebased-From: 0933a37
Merged
maflcko
pushed a commit
that referenced
this pull request
Aug 11, 2020
be95147 Updated appveyor job to checkout a specific vcpkg commit ID. (Aaron Clauson) 1fd9cd2 appveyor: Remove clcache (MarcoFalke) 8c0a959 Remove cached directories and associated script blocks from appveyor CI configuration. (Aaron Clauson) d70f700 lint: fix shellcheck URL in CI install (fanquake) f8f7d91 test: remove Cirrus CI FreeBSD job (fanquake) b7e16a8 Add missing QPainterPath include (Andrew Chow) 30a2814 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged (João Barbosa) 0d87a5b QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr) bde6a5a Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr) e422f65 build: Set libevent minimum version to 2.0.21 (Hennadii Stepanov) 0d0dd6a Update with new Windows code signing certificate (Andrew Chow) Pull request description: Backports the following to the 0.19 branch: * #17946 - Fix GBT: Restore "!segwit" and "csv" to "rules" key * #18160 - gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged * #18425 - releases: Update with new Windows code signing certificate * #18676 - build: Check libevent minimum version in configure script * #19097 - qt: Add missing QPainterPath include (as per #19510) * #18640 - appveyor: Remove clcache * #19444 - test: Remove cached directories and associated script blocks from appveyor config * #19612 - lint: fix shellcheck URL in CI install * #18001 - Updated appveyor job to checkout a specific vcpkg commit ID Closes: #19510. ACKs for top commit: jnewbery: ACK be95147 MarcoFalke: cherry-pick ACK be95147 🌎 Tree-SHA512: 2ec7e3ae1da99799ff6f8cfe26095d6885cffe6952b18a7e236dc5e657b3918225c2601b8c8e17cdff5319c40cb0a214d9fad49b0ff2f54af1db7c81d83a1df5
janus
pushed a commit
to janus/bitgesell
that referenced
this pull request
Nov 15, 2020
…lockTip notifications interfaces::Wallet::tryGetBalances was recently updated in bitcoin/bitcoin#18160 to avoid computing balances internally, but this not efficient as it could be with #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"'.
janus
pushed a commit
to janus/bitgesell
that referenced
this pull request
Nov 15, 2020
…ged" This reverts commit 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37 from bitcoin/bitcoin#18160 which no longer an optimization since commit "gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications".
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jan 13, 2021
Summary: > Each 250ms the slot WalletModel::pollBalanceChanged is called which, at worst case, calls Wallet::GetBalance. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not. This is a backport of Core [[bitcoin/bitcoin#18160 | PR18160]] See D7938 (//Remove locked_chain from CWallet//) for why the `src/interfaces/wallet.cpp` diff looks different Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8896
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Each 250ms the slot
WalletModel::pollBalanceChangedis called which, at worst case, callsWallet::GetBalance. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not.The actual balance computation can still hang the GUI thread but that is tracked in #16874 and should be fixed with a solution similar to #17135.