From dc1cc1c35995dc09085b3d9270c445b7923fdb51 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 6 Dec 2022 12:39:00 -0300 Subject: [PATCH] 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. --- src/bench/wallet_create_tx.cpp | 4 ++-- src/qt/sendcoinsdialog.cpp | 7 ++++++- src/wallet/interfaces.cpp | 20 +++++++++++++++++++- src/wallet/spend.cpp | 6 ------ src/wallet/spend.h | 2 -- src/wallet/test/wallet_tests.cpp | 2 +- 6 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 80d23d1e512..cb31421598c 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/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::vectorfAllowWatchOnly = 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(ui->entries->itemAt(i)->widget()); if (e && !e->isHidden() && e != entry) { diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 1a76e46c546..df1eb19a339 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -403,7 +404,24 @@ public: 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 { diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4548d5f8139..57f3785a3ab 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.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); diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 78c2c5f22b9..cc9ccf30118 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -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. */ diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 2e95a148072..edcfaa24e54 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -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