Merge bitcoin/bitcoin#26699: wallet, gui: bugfix, getAvailableBalance 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

Author: achow101

src/bench/wallet_create_tx.cpp

@@ -102,7 +102,7 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type
     }
 
     // Check available balance
-    auto bal = wallet::GetAvailableBalance(wallet); // Cache
+    auto bal = WITH_LOCK(wallet.cs_wallet, return wallet::AvailableCoins(wallet).GetTotalAmount()); // Cache
     assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY));
 
     wallet::CCoinControl coin_control;
@@ -161,7 +161,7 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector<OutputType
     }
 
     // Check available balance
-    auto bal = wallet::GetAvailableBalance(wallet); // Cache
+    auto bal = WITH_LOCK(wallet.cs_wallet, return wallet::AvailableCoins(wallet).GetTotalAmount()); // Cache
     assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY));
 
     bench.epochIterations(2).run([&] {

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);
     //: Caption of "PSBT has been copied" messagebox
     msgBox.setText(tr("Unsigned Transaction", "PSBT copied"));
     msgBox.setInformativeText(tr("The PSBT has been copied to the clipboard. You can also save it."));
@@ -791,8 +791,13 @@ void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry)
     // Include watch-only for wallets without private key
     m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner();
 
+    // Same behavior as send: if we have selected coins, only obtain their available balance.
+    // Copy to avoid modifying the member's data.
+    CCoinControl coin_control = *m_coin_control;
+    coin_control.m_allow_other_inputs = !coin_control.HasSelected();
+
     // Calculate available amount to send.
-    CAmount amount = model->getAvailableBalance(m_coin_control.get());
+    CAmount amount = model->getAvailableBalance(&coin_control);
     for (int i = 0; i < ui->entries->count(); ++i) {
         SendCoinsEntry* e = qobject_cast<SendCoinsEntry*>(ui->entries->itemAt(i)->widget());
         if (e && !e->isHidden() && e != entry) {

src/qt/sendcoinsdialog.h

@@ -49,6 +49,9 @@ class SendCoinsDialog : public QDialog
     void pasteEntry(const SendCoinsRecipient &rv);
     bool handlePaymentRequest(const SendCoinsRecipient &recipient);
 
+    // Only used for testing-purposes
+    wallet::CCoinControl* getCoinControl() { return m_coin_control.get(); }
+
 public Q_SLOTS:
     void clear();
     void reject() override;

src/qt/test/wallettests.cpp

@@ -613,5 +613,17 @@ uint256 WalletModel::getLastBlockProcessed() const
 
 CAmount WalletModel::getAvailableBalance(const CCoinControl* control)
 {
-    return control && control->HasSelected() ? wallet().getAvailableBalance(*control) : getCachedBalance().balance;
+    // No selected coins, return the cached balance
+    if (!control || !control->HasSelected()) {
+        const interfaces::WalletBalances& balances = getCachedBalance();
+        CAmount available_balance = balances.balance;
+        // if wallet private keys are disabled, this is a watch-only wallet
+        // so, let's include the watch-only balance.
+        if (balances.have_watch_only && m_wallet->privateKeysDisabled()) {
+            available_balance += balances.watch_only_balance;
+        }
+        return available_balance;
+    }
+    // Fetch balance from the wallet, taking into account the selected coins
+    return wallet().getAvailableBalance(*control);
 }

src/qt/walletmodel.cpp

@@ -18,6 +18,7 @@
 #include <util/system.h>
 #include <util/translation.h>
 #include <util/ui_change_type.h>
+#include <wallet/coincontrol.h>
 #include <wallet/context.h>
 #include <wallet/feebumper.h>
 #include <wallet/fees.h>
@@ -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);
+        LOCK(m_wallet->cs_wallet);
+        CAmount total_amount = 0;
+        // Fetch selected coins total amount
+        if (coin_control.HasSelected()) {
+            FastRandomContext rng{};
+            CoinSelectionParams params(rng);
+            // Note: for now, swallow any error.
+            if (auto res = FetchSelectedInputs(*m_wallet, coin_control, params)) {
+                total_amount += res->total_amount;
+            }
+        }
+
+        // And fetch the wallet available coins
+        if (coin_control.m_allow_other_inputs) {
+            total_amount += AvailableCoins(*m_wallet, &coin_control).GetTotalAmount();
+        }
+
+        return total_amount;
     }
     isminetype txinIsMine(const CTxIn& txin) override
     {

src/wallet/interfaces.cpp

@@ -356,12 +356,6 @@ CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl*
     return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, params);
 }
 
-CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl)
-{
-    LOCK(wallet.cs_wallet);
-    return AvailableCoins(wallet, coinControl).GetTotalAmount();
-}
-
 const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& outpoint)
 {
     AssertLockHeld(wallet.cs_wallet);

src/wallet/spend.cpp

@@ -94,8 +94,6 @@ CoinsResult AvailableCoins(const CWallet& wallet,
  */
 CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl = nullptr, CoinFilterParams params = {}) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 
-CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr);
-
 /**
  * Find non-change parent output.
  */

src/wallet/spend.h

@@ -581,7 +581,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
     BOOST_CHECK_EQUAL(list.begin()->second.size(), 1U);
 
     // Check initial balance from one mature coinbase transaction.
-    BOOST_CHECK_EQUAL(50 * COIN, GetAvailableBalance(*wallet));
+    BOOST_CHECK_EQUAL(50 * COIN, WITH_LOCK(wallet->cs_wallet, return AvailableCoins(*wallet).GetTotalAmount()));
 
     // Add a transaction creating a change address, and confirm ListCoins still
     // returns the coin associated with the change address underneath the