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

furszy · 2022-12-14T14:07:01Z

First Issue Description (bitcoin-core/gui#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):

The GUI balance check prior the transaction creation process.
The GUI "useAvailableBalance" functionality.

Note 1:
As the GUI uses a balance cache since bitcoin-core/gui#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 (#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.

DrahtBot · 2022-12-14T14:07:05Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, achow101, theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/417 (Ditch wallet model juggling by promag)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#27217 (wallet: Replace use of purpose strings with an enum by achow101)
#26715 (Introduce MockableDatabase for wallet unit tests by achow101)
#25979 ([WIP] wallet: standardize change output detection process by furszy)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

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.

achow101 · 2022-12-14T17:45:25Z

While you're in this function, can you also fix #26687. It's a similar issue - we're now filtering for only spendable coins which excludes watch-only coins.

furszy · 2022-12-14T20:25:58Z

While you're in this function, can you also fix #26687. It's a similar issue - we're now filtering for only spendable coins which excludes watch-only coins.

sure @achow101, will check it. Feel free to cc me for other similar stuff next time.

furszy · 2022-12-15T01:55:31Z

Added fix and test coverage for #26687.

As we are using a cached balance on WalletModel::getAvailableBalance,
the function needs to include into the response the watch-only available
balance on wallets with private keys disabled.

Note:
Let me know if want me to decouple this two fixes into a new PR.

src/wallet/spend.cpp

furszy · 2022-12-17T02:28:57Z

Added test coverage for the GUI PSBT creation on legacy watch-only wallets.

theStack

Concept ACK

Saw this problem yesterday while I was experimenting with the GUI and noticed that trying to spend a single available UTXO via coin control failed with a "The amount exceeds your balance." error -- IIUC, this issue appears whenever the balance of the not-selected coins is smaller than the balance of the selected coins.

maflcko · 2023-01-18T16:08:16Z

Needs rebase

furszy · 2023-01-18T18:25:25Z

rebased, silent conflict solved.

Sjors · 2023-03-31T16:19:57Z

Tested that this fixes the coin selection + use available balance GUI issue for me. Haven't reviewed the code yet.

furszy · 2023-04-02T15:33:47Z

rebased, conflict solved.

Sjors

Tested and reviewed 86b43c7.

Especially happy with all the new tests. Some questions / issues:

src/qt/sendcoinsdialog.cpp

Sjors · 2023-04-03T12:40:10Z

src/wallet/interfaces.cpp

@@ -403,7 +404,24 @@ class WalletImpl : public Wallet
    CAmount getBalance() override { return GetBalance(*m_wallet).m_mine_trusted; }
    CAmount getAvailableBalance(const CCoinControl& coin_control) override
    {
-        return GetAvailableBalance(*m_wallet, &coin_control);


8cd7730: paging @ryanofsky: do you think we should move all this complexity here to the interface?

8cd7730: paging @ryanofsky: do you think we should move all this complexity here to the interface?

Right, probably it would be better to move the new code from the wallet:WalletImpl::getAvailableBalance() method in src/wallet/interfaces.cpp into the wallet::GetAvailableBalance() function in src/wallet/spend.cpp or into another function there like wallet::GetSelectedCoinsBalance. Reasoning is

src/wallet/interfaces.cpp is supposed to define a public interface to wallet code that GUI code can call without accessing wallet internals. It is just supposed to forward calls from the public interface to private functions, and not really designed to contain coin selection code.

It is easier to write unit tests for standalone spend.cpp functions than interface methods that require constructing a larger interface.

It is easier to share code between RPC and GUI interfaces right now if code is standalone

EDIT: To clarify, I think it would be better to move this code, but I don't think leaving the code here is a big problem. I wouldn't want it to delay fixing the bug.

Sure. Having a proxy class is nice for any future IPC mechanism. All the protobuf/protocan/etc messages are simpler to craft in this way.

Based on how close we are to v25 branch-off, I'm thinking on whether should do it now or later :/. Otherwise I would push it right away.

Probably, could tackle it on a quick follow-up with #26699 (comment) and few other cleanups as well.

src/wallet/interfaces.cpp

The previous behavior for getAvailableBalance when coin control has 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. This turns into a GUI-only issue for the "use available balance" button when the user manually select coins in the send screen. Reason: We missed to update the GetAvailableBalance function to include the coin control selected coins on bitcoin#25685. Context: Since bitcoin#25685 we skip the selected coins inside `AvailableCoins`, the reason is that there is no need to traverse the wallet's txes map just to get coins that can directly be fetched by their id.

The following cases were covered: Case 1: No coin control selected coins. - 'useAvailableBalance' should fill the amount edit box with the total available balance. Case 2: With coin control selected coins. - 'useAvailableBalance' should fill the amount edit box with the sum of the selected coins values.

Only for wallets with private keys disabled. The returned amount need to include the watch-only available balance too. Solves bitcoin#26687.

Prepare ground for legacy watch-only test.

So it can be reused across tests.

furszy · 2023-04-03T20:26:28Z

Feedback tackled. Thanks Sjors.

Only a comment change, diff.

Sjors · 2023-04-04T08:09:55Z

tACK 68eed5d

Remaining feedback is only about source code comments.

achow101 · 2023-04-10T13:37:00Z

ACK 68eed5d

theStack

ACK 68eed5d

Reviewed the changeset (with only light review for the qt unit tests, as I'm not too familiar in that area), verified manually that the GUI coin selection issue is fixed, tested the tests by reverting the fix commits dc1cc1c and cd98b71 each and checking that the corresponding unit tests fail. LGTM.

theStack · 2023-04-11T16:17:17Z

src/qt/sendcoinsdialog.cpp

@@ -403,7 +403,7 @@ void SendCoinsDialog::presentPSBT(PartiallySignedTransaction& psbtx)
    CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
    ssTx << psbtx;
    GUIUtil::setClipboard(EncodeBase64(ssTx.str()).c_str());
-    QMessageBox msgBox;
+    QMessageBox msgBox(this);


nit: Is there any specific reason for this change? Without it, everything seems to work still fine on my side (but obviously, it shouldn't hurt to construct the message box with a parent...)

It was because I was accessing the parent to click on the message box from the unit test (the dialog exec() method blocks the thread until the user presses any of the buttons). And, later on, changed my mind and accessed it via the QApplication::topLevelWidgets call.

It doesn't hurt to have it. This would had been a leak if the message box would had been constructed via the new call.

…elected…

hebasto · 2024-02-19T12:19:08Z

src/qt/test/wallettests.cpp

+    QTimer timer;
+    timer.setInterval(500);
+    QObject::connect(&timer, &QTimer::timeout, [&](){
+        for (QWidget* widget : QApplication::topLevelWidgets()) {
+            if (widget->inherits("QMessageBox")) {
+                QMessageBox* dialog = qobject_cast<QMessageBox*>(widget);
+                QAbstractButton* button = dialog->button(QMessageBox::Discard);
+                button->setEnabled(true);
+                button->click();
+                timer.stop();
+                break;
+            }
+        }
+    });
+    timer.start(500);


It seems all these lines of code might be replaced with ConfirmMessage(nullptr, 500ms);.

It seems all these lines of code might be replaced with ConfirmMessage(nullptr, 500ms);.

That will trigger the filesystem window to dump the psbt info to a file, which would incur in another step to cancel the dialog. The idea here is to obtain the data through the clipboard only (see few lines below), not through a file.

That being said, agree that we could move this into the utility file in the future and cleanup some code.

That will trigger the filesystem window to dump the psbt info to a file...

Why? The ConfirmMessage triggers the default button, which is QMessageBox::Discard, no?

That will trigger the filesystem window to dump the psbt info to a file...

Why? The ConfirmMessage triggers the default button, which is QMessageBox::Discard, no?

Yeah ok, I did not see the manual setDefaultButton call. That will work fine while the dialog takes less than 500ms in popping up. If it takes longer, the test will stall because ConfirmMessage runs the timer only once (QTimer::singleShot).

furszy changed the title ~~wallet, gui: getAvailableBalance skips selected coins~~ wallet, gui: bugfix, getAvailableBalance skips selected coins Dec 14, 2022

furszy force-pushed the 2022_bugfix_wallet_getavailablebalance branch from 197fbb7 to 30785b2 Compare December 15, 2022 01:46

This was referenced Dec 15, 2022

wallet: GetEffectiveBalance #26365

Closed

wallet: simplify ListCoins implementation #25659

Merged

furszy mentioned this pull request Dec 15, 2022

Unable to create PSBT for legacy watchonly wallets in the GUI #26687

Closed

achow101 reviewed Dec 16, 2022

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_bugfix_wallet_getavailablebalance branch from 30785b2 to e680c2d Compare December 16, 2022 23:10

This was referenced Dec 17, 2022

Introduce MockableDatabase for wallet unit tests #26715

Merged

bumpfee: Allow the user to choose which output is change #26467

Merged

furszy force-pushed the 2022_bugfix_wallet_getavailablebalance branch 2 times, most recently from cd28dc4 to c1c1fcb Compare December 17, 2022 12:49

This was referenced Dec 17, 2022

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

Ditch wallet model juggling bitcoin-core/gui#417

Closed

achow101 added the Wallet label Dec 20, 2022

furszy mentioned this pull request Dec 24, 2022

Selecting two coins will abort with "The amount exceeds your balance." bitcoin-core/gui#688

Closed

This was referenced Jan 11, 2023

[WIP] wallet: standardize change output detection process #25979

Closed

wallet: Use defined purposes instead of inlining #26858

Closed

theStack reviewed Jan 18, 2023

View reviewed changes

furszy force-pushed the 2022_bugfix_wallet_getavailablebalance branch 2 times, most recently from 941cdc1 to 705d44d Compare January 18, 2023 16:40

DrahtBot added the Needs rebase label Jan 18, 2023

furszy force-pushed the 2022_bugfix_wallet_getavailablebalance branch from 705d44d to 48e5173 Compare January 18, 2023 22:25

DrahtBot removed the Needs rebase label Jan 18, 2023

DrahtBot mentioned this pull request Mar 7, 2023

wallet: Replace use of purpose strings with an enum #27217

Merged

DrahtBot mentioned this pull request Mar 21, 2023

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

furszy mentioned this pull request Mar 23, 2023

wallet: unify “allow/block other inputs“ concept #25118

Merged

beirut-boop mentioned this pull request Mar 23, 2023

Coin Control is broken in 4.4.1 syscoin/syscoin#524

Closed

DrahtBot mentioned this pull request Mar 31, 2023

Update translation source file for v25.0 string freeze bitcoin-core/gui#724

Merged

DrahtBot added the Needs rebase label Apr 2, 2023

furszy force-pushed the 2022_bugfix_wallet_getavailablebalance branch from 37d8a00 to 86b43c7 Compare April 2, 2023 15:32

DrahtBot removed the Needs rebase label Apr 2, 2023

Sjors reviewed Apr 3, 2023

View reviewed changes

furszy added 6 commits April 3, 2023 17:23

gui: 'getAvailableBalance', include watch only balance

cd98b71

Only for wallets with private keys disabled. The returned amount need to include the watch-only available balance too. Solves bitcoin#26687.

test,gui: decouple chain and wallet initialization from test case

2f76ac0

Prepare ground for legacy watch-only test.

test,gui: decouple widgets and model into a MiniGui struct

306aab5

So it can be reused across tests.

test,gui: add coverage for PSBT creation on legacy watch-only wallets

68eed5d

furszy force-pushed the 2022_bugfix_wallet_getavailablebalance branch from 86b43c7 to 68eed5d Compare April 3, 2023 20:24

achow101 requested a review from theStack April 10, 2023 13:37

theStack approved these changes Apr 11, 2023

View reviewed changes

achow101 merged commit 27dcc07 into bitcoin:master Apr 11, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 12, 2023

Merge bitcoin#26699: wallet, gui: bugfix, getAvailableBalance skips s…

e4482b4

…elected…

This was referenced May 15, 2023

"Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives #27659

Closed

Watch-only, balance not "available" for psbt bitcoin-core/gui#83

Closed

furszy deleted the 2022_bugfix_wallet_getavailablebalance branch May 27, 2023 01:46

hebasto reviewed Feb 19, 2024

View reviewed changes

bitcoin locked and limited conversation to collaborators Feb 18, 2025

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

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

Uh oh!

Conversation

furszy commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

achow101 commented Dec 14, 2022

Uh oh!

furszy commented Dec 14, 2022

Uh oh!

furszy commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

furszy commented Dec 17, 2022

Uh oh!

theStack left a comment • 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!

maflcko commented Jan 18, 2023

Uh oh!

furszy commented Jan 18, 2023

Uh oh!

Sjors commented Mar 31, 2023

Uh oh!

furszy commented Apr 2, 2023

Uh oh!

Sjors left a comment • 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!

Uh oh!

Uh oh!

Sjors Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 10, 2023 • 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!

furszy Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Apr 3, 2023

Uh oh!

Sjors commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Apr 10, 2023

Uh oh!

theStack left a comment • 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!

theStack Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

furszy Apr 11, 2023 • 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!

hebasto Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

furszy Feb 19, 2024 • 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!

hebasto Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

furszy Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

furszy commented Dec 14, 2022 •

edited

Loading

DrahtBot commented Dec 14, 2022 •

edited

Loading

furszy commented Dec 15, 2022 •

edited

Loading

theStack left a comment •

edited

Loading

Sjors left a comment •

edited

Loading

ryanofsky Apr 10, 2023 •

edited

Loading

Sjors commented Apr 4, 2023 •

edited

Loading

theStack left a comment •

edited

Loading

furszy Apr 11, 2023 •

edited

Loading

furszy Feb 19, 2024 •

edited

Loading

furszy Feb 19, 2024 •

edited

Loading