Avoid recalculating the wallet balance - use model cache #598

furszy · 2022-05-09T15:14:38Z

As per the title says, we are recalculating the entire wallet balance on different situations calling to wallet().getBalances(), when should instead make use of the wallet model cached balance.

This has the benefits of (1) not spending resources calculating a balance that we already have cached, and (2) avoid blocking the main thread for a long time, in case of big wallets, walking through the entire wallet's tx map more than what it's really needed.

Changes:

Fix: SendCoinsDialog was calling wallet().getBalances() twice during setModel.
Use the cached balance if the user did not select any UTXO manually inside the wallet model getAvailableBalance call.

As an extra note, this work born in #25005 but grew out of scope of it.

src/qt/walletmodel.cpp

achow101 · 2022-05-16T20:44:47Z

src/qt/test/wallettests.cpp

-        QCOMPARE(balanceText, balanceComparison);
-    }
+    // Update walletModel cached balance which will trigger an update for the 'labelBalance' QLabel.
+    walletModel.pollBalanceChanged();


In 7345a04 "test: GUI WalletTests, trigger walletModel balance changed manually."

It seems potentially bad that the cached balance might be out of sync with the real balance and that pollBalanceChanged needs to be called here in the tests?

It's out of sync merely because we are not starting the polling timer at all in the test. Same as we aren't initializing several parts of the GUI objects. All the objects (wallet, models, views, etc) that are used on this test are manually created instead of using the WalletController class flow.

Which I think that is actually what we are looking for. It's an unit test that is focused on verifying that GUI widgets/views are behaving as expected: updating the presented information, etc. when they receive different signals and/or function calls from outside (in other words, focus is on the signal slots/receiver side). It's not about whether the wallet balance polling timer is functioning as expected or not (which we definitely can create a new test case for it in a follow-up work).

furszy · 2022-05-20T13:50:24Z

thanks achow101 for the review!

typo fixed 👍🏼.

hebasto · 2022-05-30T18:31:31Z

The following part of the PR description

2. Guarded WalletModel::m_cached_balances with its own mutex, so it can be used from the widgets as well. (the OverviewPage and SendCoinsDialog now are making use of it instead of calling wallet().getBalances() by their own).

looks outdated now.

hebasto

It seems wrong to me when the commit 696683d "GUI: use cached balance in overviewpage and sendcoinsdialog." breaks GUI WalletTests, then another commit fixes the broken test.

If a change is a refactoring then all tests must pass. Otherwise, if a commit introduces a new feature/functionality, it should change both main code and its tests.

src/qt/walletmodel.cpp

src/qt/overviewpage.cpp

furszy · 2022-05-31T15:04:57Z

thanks for the review hebasto!

It seems wrong to me when the commit 696683d "GUI: use cached balance in overviewpage and sendcoinsdialog." breaks GUI WalletTests, then another commit fixes the broken test.

If a change is a refactoring then all tests must pass. Otherwise, if a commit introduces a new feature/functionality, it should change both main code and its tests.

I decoupled it to explain the rationale properly but sure. Squashed 👍🏼 .

Feedback tackled.

DrahtBot · 2022-06-12T21:45:13Z

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

Conflicts

No conflicts as of last run.

jarolrod

could break off 20d1b90 into it's own PR in order to make this focused on refactoring to use the model cache

furszy · 2022-06-27T13:45:43Z

could break off 20d1b90 into it's own PR in order to make this focused on refactoring to use the model cache

Hey @jarolrod, I honestly don't think that separating that easy reviewable fix would make any difference.

Would rather friendly ping/dm some possible reviewers directly. This set of changes will be quite beneficial for large wallets.

shaavan

Concept ACK

I agree with using cached balances whenever possible to save time on wallet balances more than necessary.
I would like to briefly summarise my understanding of each commit, along with a few points I would like to discuss.
If some of my observations are erroneous or incomplete, please do correct me.

Notes:

Commit-1
1. Doubt: It is claimed that setBalance is called twice. But it is so only when displayUnitChanged signal is emitted. So, as far as I understood, this will lead to not updating the balance in case displayUnit is not changed.
2. Renaming the updateDisplayUnit function is technically a refactoring change, so ideally, it should be separate from the code changes.
Commit-2:
1. Introduces the getCachedBalance function.
2. I think I understand @hebasto’s reasoning here to rename the function to CachedBalance(). m_cached_balances should be something that is updated only by the internal functions of the class. Since they can directly access the variable, they don’t need a setter.
3. And having a function named CachedBalance() clearly points to what value the function will return; hence the get suffix seems redundant.
Commit-3:
1. Makes the code changes to use the cached balance.
2. And make relevant test changes to allow them to pass.
3. The relevant change includes doing manual balance polling updates.
4. commit ACK.
Commit-4:
1. Adds a function that checks if the coins are manually selected; if so, they are passed through the previously used getAvailableBalance() function, otherwise, the cachedBalance is used.
2. This makes sense as the cached balance is created with the GetBalance(*m_wallet) function.
3. commit ACK.
Commit-5:
1. Removes a redundant global variable.
2. On line 308, of src/qt/overviewpage.cpp, the m_balances variable should be renamed to balances as the m_ prefix is reserved for global variables.

furszy · 2022-07-06T17:15:02Z

Thanks @shaavan for the review :).

Commit-1

Doubt: It is claimed that setBalance is called twice. But it is so only when displayUnitChanged signal is emitted. So, as far as I understood, this will lead to not updating the balance in case displayUnit is not changed.

At startup, when the widgets are initialized, SendCoinsDialog::setModel is called. Which calculates and sets the initial balance prior receiving any signal.

To be specific, in master:

First calculation:
Inside SendCoinsDialog::setModel, at line 167 and 168 getBalances and setBalance are called.
Second calculation:  Inside SendCoinsDialog::setModel, at line 171 updateDisplayUnit is called which internally calls to setBalance(model->wallet().getBalances());.

And having a function named CachedBalance() clearly points to what value the function will return; hence the get suffix seems redundant.

I tried to explain the rationale in a previous comment:
Generally speaking, if the method is a getter (it returns the value of an object's private field), then why not just name it getSomething by convention as it’s done, by convention, in plenty of projects out there?

So any dev using the model interface (or any interface actually) can find it right away without even need to think twice about it.
Personally, I think that those three characters (same as other coding styling conventions) make searches across large interfaces/projects simpler and more organized than having to know/remember how things are/were called in the first place (or require to remember each project specific conventions).

At the end, It’s more a general code styling conversation than something for this PR. But.. I’m in favor of not making code harder to read/follow with local/own conventions where is possible.

shaavan · 2022-08-04T12:52:11Z

Thanks for the explanation @furszy!

I understand now how setBalance is being called twice.

Also, after some thinking, I agree with your rationale for adding the get prefix. If small enough would make it easier to understand the use-case of this function, then I am OK with that!

Just one last nit suggestion.

On line 308, of src/qt/overviewpage.cpp, the m_balances variable should be renamed to balances as the m_ prefix is reserved for global variables.

furszy · 2022-08-04T15:58:08Z

Thanks @shaavan

On line 308, of src/qt/overviewpage.cpp, the m_balances variable should be renamed to balances as the m_ prefix is reserved for global variables.

nit pushed in 42059fb 👍🏼

shaavan

ACK 42059fb

Changes since my last review:

Renamed m_balances -> balances

Apologize for the late reply. I missed your notification earlier when the PR was updated. Thanks for your patience!

w0xlt

ACK 42059fb

furszy · 2022-08-12T12:14:58Z

@hebasto maybe merge?

hebasto

ACK 42059fb, I have reviewed the code and it looks OK.

src/qt/walletmodel.cpp

src/qt/sendcoinsdialog.cpp

Inside setModel, we call 'wallet().getBalances()', to set the view balance, right before calling 'updateDisplayUnit' which calls 'wallet().getBalances()' internally and re-sets the view balance again.

No need to guard it as it is/will only be accessed from the main thread for now

Plus, calculate the cached balance right when the wallet model, so the wallet widgets don't need to redo the same balance calculation multiple times when they are waiting for the model balance polling timer. ---------------------------------------------------------------------- test wise: `WalletTests` now need to trigger the walletModel balance changed manually. So the model updates its internal state and can be used by the widgets. This is because the test does not start the balance polling timer, in the same way as does not initialize several parts of the GUI workflow. All the objects (wallet, models, views, etc) that are used on this test are manually created instead of using the `WalletController` class flow. Rationale is that this unit test is focused on verifying the GUI widgets/views behavior only: update the presented information, etc. when they receive different signals and/or function calls from outside (in other words, focus is on the signal slots/receiver side). It's not about whether the wallet balance polling timer is functioning as expected or not (which we definitely create a new test case for it in a follow-up work).

…lect UTXO manually No need to walk through the entire wallet's tx map. Used for 'walletModel::prepareTransaction' and 'useAvailable' flow in sendcoinsdialog.

furszy · 2022-08-12T16:07:50Z

nits tackled.

hebasto

re-ACK 4584d30, only suggested changes and commit message formatting since my recent review.

hebasto · 2022-08-12T17:53:54Z

@achow101 @jarolrod @shaavan @w0xlt
Mind having another (final?) look at this PR?

jarolrod · 2022-08-15T17:15:09Z

ACK 4584d30

I tested this with a script i wrote to generate a bunch of blocks on regtest and generate 10K transactions for a wallet. I tested the wallet with this PR and Master. I didn't visually perceive a difference between master and this PR; probably due to the power of my desktop, but obviously the code shows that this is an improvement.

furszy · 2022-08-16T13:20:18Z

ACK 4584d30

I tested this with a script i wrote to generate a bunch of blocks on regtest and generate 10K transactions for a wallet. I tested the wallet with this PR and Master. I didn't visually perceive a difference between master and this PR; probably due to the power of my desktop, but obviously the code shows that this is an improvement.

10k txes isn't that much (otherwise the wallet would be pretty unusable). 100k should be enough to notice it even on powerful machines. Will see a longer loading time for the wallet at startup (in the worker thread).

Still.. have to say that this is just the tip of the iceberg, and we do have other stuff that can be improved to decrease the wallet startup time.

…… …use model cache

… skips selected coins 68eed5d test,gui: add coverage for PSBT creation on legacy watch-only wallets (furszy) 306aab5 test,gui: decouple widgets and model into a MiniGui struct (furszy) 2f76ac0 test,gui: decouple chain and wallet initialization from test case (furszy) cd98b71 gui: 'getAvailableBalance', include watch only balance (furszy) 74eac3a test: add coverage for 'useAvailableBalance' functionality (furszy) dc1cc1c gui: bugfix, getAvailableBalance skips selected coins (furszy) Pull request description: Fixes #688 and bitcoin/bitcoin#26687. First Issue Description (#688): The previous behavior for `getAvailableBalance`, when the coin control had selected coins, was to return the sum of them. Instead, we are currently returning the wallet's available total balance minus the selected coins total amount. Reason: Missed to update the `GetAvailableBalance` function to include the coin control selected coins on #25685. Context: Since #25685 we skip the selected coins inside `AvailableCoins`, the reason is that there is no need to waste resources walking through the entire wallet's txes map just to get coins that could have gotten by just doing a simple `mapWallet.find`). Places Where This Generates Issues (only when the user manually select coins via coin control): 1) The GUI balance check prior the transaction creation process. 2) The GUI "useAvailableBalance" functionality. Note 1: As the GUI uses a balance cache since #598, this issue does not affect the regular spending process. Only arises when the user manually select coins. Note 2: Added test coverage for the `useAvailableBalance` functionality. ---------------------------------- Second Issue Description (bitcoin/bitcoin#26687): As we are using a cached balance on `WalletModel::getAvailableBalance`, the function needs to include the watch-only available balance for wallets with private keys disabled. ACKs for top commit: Sjors: tACK 68eed5d achow101: ACK 68eed5d theStack: ACK 68eed5d Tree-SHA512: 674f3e050024dabda2ff4a04b9ed3750cf54a040527204c920e1e38bd3d7f5fd4d096e4fd08a0fea84ee6abb5070f022b5c0d450c58fd30202ef05ebfd7af6d3

promag reviewed May 9, 2022

View reviewed changes

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

furszy force-pushed the 2022_GUI_use_model_cached_balance branch 3 times, most recently from a8d9b7d to 7345a04 Compare May 9, 2022 18:19

furszy mentioned this pull request May 9, 2022

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. bitcoin/bitcoin#25005

Merged

achow101 reviewed May 16, 2022

View reviewed changes

furszy force-pushed the 2022_GUI_use_model_cached_balance branch from a95c81f to 061c6ee Compare May 20, 2022 13:49

hebasto added the Wallet label May 30, 2022

hebasto reviewed May 30, 2022

View reviewed changes

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

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

src/qt/walletmodel.cpp Show resolved Hide resolved

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

furszy force-pushed the 2022_GUI_use_model_cached_balance branch from 061c6ee to f6084d0 Compare May 31, 2022 15:03

furszy requested a review from hebasto June 12, 2022 14:38

jarolrod reviewed Jun 26, 2022

View reviewed changes

shaavan reviewed Jul 4, 2022

View reviewed changes

furszy force-pushed the 2022_GUI_use_model_cached_balance branch from f6084d0 to 42059fb Compare August 4, 2022 15:57

shaavan approved these changes Aug 9, 2022

View reviewed changes

w0xlt approved these changes Aug 10, 2022

View reviewed changes

hebasto approved these changes Aug 12, 2022

View reviewed changes

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

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

furszy added 4 commits August 12, 2022 13:05

GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call

e62958d

Inside setModel, we call 'wallet().getBalances()', to set the view balance, right before calling 'updateDisplayUnit' which calls 'wallet().getBalances()' internally and re-sets the view balance again.

GUI: add getter for WalletModel::m_cached_balances field

321335b

No need to guard it as it is/will only be accessed from the main thread for now

GUI: 'getAvailableBalance', use cached balance if the user did not se…

050e8b1

…lect UTXO manually No need to walk through the entire wallet's tx map. Used for 'walletModel::prepareTransaction' and 'useAvailable' flow in sendcoinsdialog.

GUI: remove now unneeded 'm_balances' field from overviewpage

4584d30

furszy force-pushed the 2022_GUI_use_model_cached_balance branch from 42059fb to 4584d30 Compare August 12, 2022 16:07

hebasto approved these changes Aug 12, 2022

View reviewed changes

hebasto merged commit 6d4889a into bitcoin-core:master Aug 15, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 16, 2022

Merge bitcoin-core/gui#598: Avoid recalculating the wallet balance - …

4d5fd73

…… …use model cache

furszy mentioned this pull request Dec 14, 2022

wallet, gui: bugfix, getAvailableBalance skips selected coins bitcoin/bitcoin#26699

Merged

bitcoin-core locked and limited conversation to collaborators Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid recalculating the wallet balance - use model cache #598

Avoid recalculating the wallet balance - use model cache #598

furszy commented May 9, 2022 •

edited

Loading

achow101 May 16, 2022

furszy May 20, 2022 •

edited

Loading

furszy commented May 20, 2022

hebasto commented May 30, 2022

hebasto left a comment

furszy commented May 31, 2022

DrahtBot commented Jun 12, 2022 •

edited

Loading

jarolrod left a comment

furszy commented Jun 27, 2022 •

edited

Loading

shaavan left a comment

furszy commented Jul 6, 2022 •

edited

Loading

shaavan commented Aug 4, 2022

furszy commented Aug 4, 2022

shaavan left a comment

w0xlt left a comment

furszy commented Aug 12, 2022

hebasto left a comment

furszy commented Aug 12, 2022

hebasto left a comment

hebasto commented Aug 12, 2022

jarolrod commented Aug 15, 2022

furszy commented Aug 16, 2022

Avoid recalculating the wallet balance - use model cache #598

Avoid recalculating the wallet balance - use model cache #598

Conversation

furszy commented May 9, 2022 • edited Loading

achow101 May 16, 2022

Choose a reason for hiding this comment

furszy May 20, 2022 • edited Loading

Choose a reason for hiding this comment

furszy commented May 20, 2022

hebasto commented May 30, 2022

hebasto left a comment

Choose a reason for hiding this comment

furszy commented May 31, 2022

DrahtBot commented Jun 12, 2022 • edited Loading

Conflicts

jarolrod left a comment

Choose a reason for hiding this comment

furszy commented Jun 27, 2022 • edited Loading

shaavan left a comment

Choose a reason for hiding this comment

furszy commented Jul 6, 2022 • edited Loading

shaavan commented Aug 4, 2022

furszy commented Aug 4, 2022

shaavan left a comment

Choose a reason for hiding this comment

w0xlt left a comment

Choose a reason for hiding this comment

furszy commented Aug 12, 2022

hebasto left a comment

Choose a reason for hiding this comment

furszy commented Aug 12, 2022

hebasto left a comment

Choose a reason for hiding this comment

hebasto commented Aug 12, 2022

jarolrod commented Aug 15, 2022

furszy commented Aug 16, 2022

furszy commented May 9, 2022 •

edited

Loading

furszy May 20, 2022 •

edited

Loading

DrahtBot commented Jun 12, 2022 •

edited

Loading

furszy commented Jun 27, 2022 •

edited

Loading

furszy commented Jul 6, 2022 •

edited

Loading