wallet, gui: bugfix, getAvailableBalance skips selected coins

furszy commented at 2:07 pm on December 14, 2022: member

Fixes https://github.com/bitcoin-core/gui/issues/688 and #26687.

First Issue Description (https://github.com/bitcoin-core/gui/issues/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 https://github.com/bitcoin-core/gui/pull/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 (https://github.com/bitcoin/bitcoin/issues/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 commented at 2:07 pm on December 14, 2022: contributor

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.

furszy renamed this:
~~wallet, gui: getAvailableBalance skips selected coins~~
wallet, gui: bugfix, getAvailableBalance skips selected coins
on Dec 14, 2022

achow101 commented at 5:45 pm on December 14, 2022: member

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 commented at 8:25 pm on December 14, 2022: member

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 force-pushed on Dec 15, 2022

furszy commented at 1:55 am on December 15, 2022: member

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.

in src/wallet/spend.cpp:901 in 30785b280e outdated

896@@ -897,7 +897,9 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
897     // allowed (coins automatically selected by the wallet)
898     CoinsResult available_coins;
899     if (coin_control.m_allow_other_inputs) {
900-        available_coins = AvailableCoins(wallet, &coin_control, coin_selection_params.m_effective_feerate);
901+        CoinFilterParams params; // include non-spendable coins if this is a watch-only wallet
902+        params.only_spendable = !wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);

achow101 commented at 9:06 pm on December 16, 2022:

Can we test for this?

furszy commented at 10:12 pm on December 16, 2022:

Actually, this change is not needed here. Will just drop it. I was using it to add support for creating PSBTs on legacy wallets that only know the pubkey hash and not the pubkey (in other terms, use importaddress instead of importpubkey). Which has never being possible.

furszy force-pushed on Dec 16, 2022

furszy commented at 2:28 am on December 17, 2022: member

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

furszy force-pushed on Dec 17, 2022

achow101 added the label Wallet on Dec 20, 2022

theStack commented at 3:56 pm on January 18, 2023: contributor

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 commented at 4:08 pm on January 18, 2023: member

Needs rebase

furszy force-pushed on Jan 18, 2023

furszy commented at 6:25 pm on January 18, 2023: member

rebased, silent conflict solved.

DrahtBot added the label Needs rebase on Jan 18, 2023

furszy force-pushed on Jan 18, 2023

DrahtBot removed the label Needs rebase on Jan 18, 2023

achow101 added this to the milestone 25.0 on Jan 27, 2023

DrahtBot added the label Needs rebase on Feb 17, 2023

furszy force-pushed on Feb 20, 2023

DrahtBot removed the label Needs rebase on Feb 20, 2023

furszy commented at 2:06 pm on February 20, 2023: member

rebased, conflicts solved.

Sjors commented at 4:19 pm on March 31, 2023: member

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

DrahtBot added the label Needs rebase on Apr 2, 2023

furszy force-pushed on Apr 2, 2023

furszy commented at 3:33 pm on April 2, 2023: member

rebased, conflict solved.

DrahtBot removed the label Needs rebase on Apr 2, 2023

in src/qt/sendcoinsdialog.cpp:795 in 8cd7730230 outdated

790@@ -791,8 +791,13 @@ void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry)
791     // Include watch-only for wallets without private key
792     m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner();
793 
794+    // Same behavior as send: if we have selected coins, only obtain their available balance.
795+    // future, introduce a checkbox to customize this value.

Sjors commented at 12:21 pm on April 3, 2023:

8cd77302305a2387368aabe8744b1859caf9956a: what do you mean by this?

furszy commented at 3:49 pm on April 3, 2023:

Basically to follow the same procedure as the send process (SendCoinsDialog::PrepareSendText), it’s is at line 288-290.

Currently, the GUI disables the automatic selection of “other inputs” when the user manually selected coins (the process will only use those). But, in the future, that could also be customizable. The user might want to let the wallet add new inputs, even when the user had selected some, if there aren’t enough funds to cover for the tx target amount.

Sjors commented at 4:24 pm on April 3, 2023:

I see. I think it’s better to drop that comment and instead open a GUI issue. The checkbox could say “Add more coins as needed” to match the send RPC behaviour. The issue can point to the line !coin_control.HasSelected(); that needs to be changed to implement this.

furszy commented at 8:23 pm on April 3, 2023:

np about creating a gui issue for this.

still, I’m not fan of the link to this particular line because this isn’t the only place that has to be modified to introduce the feature. Plus, code changes over time, so the issue could easily get behind, which would mean more maintenance burden for us.

furszy commented at 8:25 pm on April 3, 2023:

pushed

Sjors commented at 8:07 am on April 4, 2023:

pushed

I don’t see a change.

My main point was to clarify the comment “future, introduce a checkbox to customize this value”. It could be done as source code comments or a Github issue.

in src/qt/sendcoinsdialog.cpp:797 in 8cd7730230 outdated

790@@ -791,8 +791,13 @@ void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry)
791     // Include watch-only for wallets without private key
792     m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner();
793 
794+    // Same behavior as send: if we have selected coins, only obtain their available balance.
795+    // future, introduce a checkbox to customize this value.
796+    CCoinControl coin_control = *m_coin_control;
797+    coin_control.m_allow_other_inputs = !coin_control.HasSelected();

Sjors commented at 12:22 pm on April 3, 2023:

8cd77302305a2387368aabe8744b1859caf9956a: This seems more consistent with the style above:

0m_coin_control->m_allow_other_inputs = !m_coin_control->HasSelected();

Alternatively you could do *m_coin_control at the top of the function.

Maybe also add an assert(m_coin_control) at the top.

furszy commented at 4:09 pm on April 3, 2023:

The m_allow_other_inputs flag is different to the fAllowWatchOnly flag.

fAllowWatchOnly set is actually redundant here as privateKeysDisabled() and hasExternalSigner() are wallet constants. It can be set only once, when the wallet is set in the GUI and don’t be re-set again.

And, at the contraire, the m_allow_other_inputs flag can change as it depends on whether the user manually selected coins or not, which could be modified at any time. Thus why I would try to not set the coin control member here.

The best path would be to set m_allow_other_inputs at the same moment when the user manually selects/deselect coins in the coin control dialog. But.. I think that it’s better to do that in a follow-up to not expand this PR much.

Sjors commented at 4:25 pm on April 3, 2023:

Ah, maybe add a comment:

0// Copy to avoid modifying m_coin_control

furszy commented at 8:10 pm on April 3, 2023:

Sure. Doesn’t hurt to add a comment.

furszy commented at 8:24 pm on April 3, 2023:

pushed

in src/wallet/interfaces.cpp:406 in 8cd7730230 outdated

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

Sjors commented at 12:40 pm on April 3, 2023:

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

ryanofsky commented at 2:32 pm on April 10, 2023:

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.

furszy commented at 2:29 pm on April 11, 2023:

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 (review) and few other cleanups as well.

in src/wallet/interfaces.cpp:413 in 8cd7730230 outdated

409+        CAmount total_amount = 0;
410+        // Fetch selected coins total amount
411+        if (coin_control.HasSelected()) {
412+            FastRandomContext rng{};
413+            CoinSelectionParams params(rng);
414+            // Note: for now, swallow any error.

Sjors commented at 2:09 pm on April 3, 2023:

Here’s a commit to handle the error: https://github.com/Sjors/bitcoin/commit/5711f8274b91a278dd9307972e6985dc96ba56cb

furszy commented at 8:06 pm on April 3, 2023:

Was remembering why I left it unhandled: And it was because this cannot happen. FetchSelectedInputs fails only if any of the selected coins are not solvable or invalid. And we set the selected coins via the coin control dialog, which is loaded from the wallet’s available UTXOs, which are always valid.

Sjors commented at 8:05 am on April 4, 2023:

I see. In that case, just add what you just wrote as a comment.

furszy commented at 12:46 pm on April 4, 2023:

Ok, sounds good. Will do it in the next push.

Sjors commented at 3:08 pm on April 3, 2023: member

Tested and reviewed 86b43c78ecd5a68b5569b1c2a7b2696b1f70ef21.

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

gui: bugfix, getAvailableBalance skips selected coins

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 #25685.

Context:
Since #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.

dc1cc1c359

test: add coverage for 'useAvailableBalance' functionality

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.

74eac3a82f

gui: 'getAvailableBalance', include watch only balance

Only for wallets with private keys disabled.

The returned amount need to include the watch-only
available balance too.

Solves #26687.

cd98b71739

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

Prepare ground for legacy watch-only test.

2f76ac0383

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

So it can be reused across tests.

306aab5bb4

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

furszy force-pushed on Apr 3, 2023

furszy commented at 8:26 pm on April 3, 2023: member

Feedback tackled. Thanks Sjors.

Only a comment change, diff.

Sjors commented at 8:09 am on April 4, 2023: member

tACK 68eed5df8656bed1be6526b014e58d3123102b03

Remaining feedback is only about source code comments.

achow101 commented at 1:37 pm on April 10, 2023: member

ACK 68eed5df8656bed1be6526b014e58d3123102b03

achow101 requested review from theStack on Apr 10, 2023

in src/qt/sendcoinsdialog.cpp:406 in 68eed5df86

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

theStack commented at 4:17 pm on April 11, 2023:

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

furszy commented at 6:07 pm on April 11, 2023:

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.

theStack approved

theStack commented at 4:25 pm on April 11, 2023: contributor

ACK 68eed5df8656bed1be6526b014e58d3123102b03

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 dc1cc1c35995dc09085b3d9270c445b7923fdb51 and cd98b717398f7b13ace91ea9efac9ce1e60b4d62 each and checking that the corresponding unit tests fail. LGTM.

achow101 merged this on Apr 11, 2023

achow101 closed this on Apr 11, 2023

sidhujag referenced this in commit e4482b4ee7 on Apr 12, 2023

furszy deleted the branch on May 27, 2023

in src/qt/test/wallettests.cpp:429 in 68eed5df86

424+                timer.stop();
425+                break;
426+            }
427+        }
428+    });
429+    timer.start(500);

hebasto commented at 12:19 pm on February 19, 2024:

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

furszy commented at 1:49 pm on February 19, 2024:

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.

hebasto commented at 2:42 pm on February 19, 2024:

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?

furszy commented at 2:53 pm on February 19, 2024:

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 executes runs the timer only once (QTimer::singleShot).

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

Reviews

Conflicts