Avoid recalculating the wallet balance

furszy commented at 3:14 pm on May 9, 2022: contributor

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.

in src/qt/walletmodel.cpp:140 in 0ac7d0cd53 outdated

133     }
134 }
135 
136+interfaces::WalletBalances WalletModel::getCachedBalance() const
137+{
138+    QMutexLocker locker(&m_cache_mutex);

promag commented at 3:42 pm on May 9, 2022:

0ac7d0cd538d79511105e0e4f69dc6e2a8f10a44

Are there concurrent accesses?

furszy commented at 4:34 pm on May 9, 2022:

Hey, not yet. It’s a glance into the future actually hehe.

At the moment it’s only accessed from the main thread. At the following locations:

From pollBalanceChanged (QTimer).
From setWalletModel -> in OverviewPage and SendCoinsDialog.
From useAvailableBalance in SendCoinsDialog.
From prepareTransaction in SendCoinsDialog.

Now the rationale: I’m locally working on a branch that moves part of the processes to a worker thread thus why the guard. And added that commit here mainly because it would let me work on different flows in different scoped PRs without having to add it to each of them. – Not a big deal for me to remove it from this one –

promag commented at 5:34 pm on May 9, 2022:

Personally, a good rule to follow is to only push stuff that is used or necessary.

furszy commented at 6:21 pm on May 9, 2022:

np 👍🏼, dropped.

furszy force-pushed on May 9, 2022

furszy cross-referenced this on May 9, 2022 from issue wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. by furszy

in src/qt/walletmodel.cpp:122 in 0da6316856 outdated

118@@ -120,12 +119,18 @@ void WalletModel::pollBalanceChanged()
119 
120 void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances)
121 {
122-    if(new_balances.balanceChanged(m_cached_balances)) {
123+    const auto& cached_balace = getCachedBalance();

achow101 commented at 8:31 pm on May 16, 2022:

In 0da631685626c84934735cf9ddef5cca834180a2 “GUI: add getter for WalletModel::m_cached_balances field.”

nit: typo

0    const auto& cached_balance = getCachedBalance();

in src/qt/test/wallettests.cpp:204 in 7345a04a2f outdated

208-        CAmount balance = walletModel.wallet().getBalance();
209-        QString balanceComparison = BitcoinUnits::formatWithUnit(unit, balance, false, BitcoinUnits::SeparatorStyle::ALWAYS);
210-        QCOMPARE(balanceText, balanceComparison);
211-    }
212+    // Update walletModel cached balance which will trigger an update for the 'labelBalance' QLabel.
213+    walletModel.pollBalanceChanged();

achow101 commented at 8:44 pm on May 16, 2022:

In 7345a04a2f27d407a66595708b96537f8374be14 “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?

furszy commented at 1:44 pm on May 20, 2022:

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 force-pushed on May 20, 2022

furszy commented at 1:50 pm on May 20, 2022: contributor

thanks achow101 for the review!

typo fixed 👍🏼.

hebasto added the label Wallet on May 30, 2022

hebasto commented at 6:31 pm on May 30, 2022: member

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.

in src/qt/walletmodel.cpp:21 in 061c6ee98a outdated

17@@ -18,7 +18,6 @@
18 #include <qt/sendcoinsdialog.h>
19 #include <qt/transactiontablemodel.h>
20 
21-#include <interfaces/handler.h>

hebasto commented at 6:56 pm on May 30, 2022:

08e74071e3ae30393a80d94a8286536ad7bfab3d

Plus, removed unused include of “interfaces/handler.h”.

Actually, it is used here: https://github.com/bitcoin-core/gui/blob/08e74071e3ae30393a80d94a8286536ad7bfab3d/src/qt/walletmodel.cpp#L431-L437

In general, better to use IWYU instead of random fixing of included headers.

in src/qt/walletmodel.cpp:127 in 061c6ee98a outdated

122@@ -120,12 +123,18 @@ void WalletModel::pollBalanceChanged()
123 
124 void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances)
125 {
126-    if(new_balances.balanceChanged(m_cached_balances)) {
127+    const auto& cached_balance = getCachedBalance();
128+    if (new_balances.balanceChanged(cached_balance)) {

hebasto commented at 6:56 pm on May 30, 2022:

08e74071e3ae30393a80d94a8286536ad7bfab3d

Why this change?

furszy commented at 2:43 pm on May 31, 2022:

This was because of #598 (review).

Before the removal of that commit, getCachedBalance was guarded by a mutex. Now is not needed anymore 👍🏼.

in src/qt/walletmodel.cpp:133 in 061c6ee98a outdated

129         m_cached_balances = new_balances;
130         Q_EMIT balanceChanged(new_balances);
131     }
132 }
133 
134+interfaces::WalletBalances WalletModel::getCachedBalance() const

hebasto commented at 7:04 pm on May 30, 2022:

08e74071e3ae30393a80d94a8286536ad7bfab3d

Without having a setter, maybe shorter cachedBalance() is better?

furszy commented at 3:00 pm on May 31, 2022:

At least for me, it’s cleared to always use the same naming convention even if we don’t have a setter right now.

I mean, generally speaking, if the method is a getter, then why not just name it getSomething? So any dev using the model interface (or any interface) can find it right away without even need to think twice about it.

At least for me, those three characters make searches across large interfaces/projects a bit more simple and organized.

in src/qt/overviewpage.cpp:179 in 061c6ee98a outdated

175@@ -178,6 +176,7 @@ void OverviewPage::handleTransactionClicked(const QModelIndex &index)
176 void OverviewPage::setPrivacy(bool privacy)
177 {
178     m_privacy = privacy;
179+    const auto& m_balances = walletModel->getCachedBalance();

hebasto commented at 7:08 pm on May 30, 2022:

061c6ee98af7185fa340989da8178e0149207e3f

m_ prefix is used for member variables, not for local ones.

See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

hebasto commented at 7:08 pm on May 30, 2022: member

It seems wrong to me when the commit 696683d7c330e2fa1591501aac8bb13324af68f9 “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.

furszy force-pushed on May 31, 2022

furszy commented at 3:04 pm on May 31, 2022: contributor

thanks for the review hebasto!

It seems wrong to me when the commit https://github.com/bitcoin-core/gui/commit/696683d7c330e2fa1591501aac8bb13324af68f9 “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.

furszy requested review from hebasto on Jun 12, 2022

DrahtBot commented at 9:45 pm on June 12, 2022: contributor

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

Conflicts

No conflicts as of last run.

jarolrod commented at 2:39 am on June 26, 2022: member

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

furszy commented at 1:45 pm on June 27, 2022: contributor

could break off https://github.com/bitcoin-core/gui/commit/20d1b909b4c0836068e18d8982e67edeb593245c 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 commented at 12:05 pm on July 4, 2022: contributor

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 commented at 5:15 pm on July 6, 2022: contributor

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 commented at 12:52 pm on August 4, 2022: contributor

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 force-pushed on Aug 4, 2022

furszy commented at 3:58 pm on August 4, 2022: contributor

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 approved

shaavan commented at 2:37 pm on August 9, 2022: contributor

ACK 42059fbc5cf4256c4f9e0b380164d34d962ae59a

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 approved

w0xlt commented at 0:59 am on August 10, 2022: contributor

ACK https://github.com/bitcoin-core/gui/pull/598/commits/42059fbc5cf4256c4f9e0b380164d34d962ae59a

furszy commented at 12:14 pm on August 12, 2022: contributor

@hebasto maybe merge?

in src/qt/walletmodel.cpp:614 in 42059fbc5c outdated

609@@ -599,3 +610,7 @@ uint256 WalletModel::getLastBlockProcessed() const
610 {
611     return m_client_model ? m_client_model->getBestBlockHash() : uint256{};
612 }
613+
614+CAmount WalletModel::getAvailableBalance(const CCoinControl* control) {

hebasto commented at 3:34 pm on August 12, 2022:

ed0a32ec66db47b99d7d89e8c6df94b3f405f8f0, style nit:

0CAmount WalletModel::getAvailableBalance(const CCoinControl* control)
1{

furszy commented at 4:07 pm on August 12, 2022:

done

in src/qt/sendcoinsdialog.cpp:26 in 42059fbc5c outdated

22@@ -23,7 +23,6 @@
23 #include <key_io.h>
24 #include <node/ui_interface.h>
25 #include <policy/fees.h>
26-#include <txmempool.h>

hebasto commented at 3:36 pm on August 12, 2022:

ed0a32ec66db47b99d7d89e8c6df94b3f405f8f0

This removal looks unrelated to this PR, doesn’t it?

furszy commented at 3:54 pm on August 12, 2022:

Yeah, it’s a manual removal of an unused include. (This is an old PR, prior me knowing about IWYU existence.. sorry for that, I forgot to update it)

Do you want me to revert it or we just go ahead with it?

hebasto commented at 4:00 pm on August 12, 2022:

I’ll really appreciate addressing of comments before merging :)

furszy commented at 4:07 pm on August 12, 2022:

pushed

hebasto approved

hebasto commented at 3:46 pm on August 12, 2022: member

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

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

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.

e62958dc81

GUI: add getter for WalletModel::m_cached_balances field

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

321335bf02

GUI: use cached balance in overviewpage and sendcoinsdialog

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

96e3264a82

GUI: 'getAvailableBalance', use cached balance if the user did not select UTXO manually

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

050e8b1391

GUI: remove now unneeded 'm_balances' field from overviewpage 4584d300a4

furszy force-pushed on Aug 12, 2022

furszy commented at 4:07 pm on August 12, 2022: contributor

nits tackled.

hebasto approved

hebasto commented at 5:52 pm on August 12, 2022: member

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

hebasto commented at 5:53 pm on August 12, 2022: member

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

jarolrod commented at 5:15 pm on August 15, 2022: member

ACK 4584d300a40bfd84517072f7a6eee114fb7cab08

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.

hebasto merged this on Aug 15, 2022

hebasto closed this on Aug 15, 2022

furszy commented at 1:20 pm on August 16, 2022: contributor

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.

sidhujag referenced this in commit 4d5fd73715 on Aug 16, 2022

furszy cross-referenced this on Dec 14, 2022 from issue wallet, gui: bugfix, getAvailableBalance skips selected coins by furszy

achow101 referenced this in commit 27dcc07c08 on Apr 11, 2023

bitcoin-core locked this on Aug 16, 2023

Avoid recalculating the wallet balance - use model cache #598

Conflicts