From 900e5ed51bb9d52ae496f1ed4a3b58703de53a12 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 21 May 2024 11:18:53 -0300 Subject: [PATCH 1/4] wallet: introduce "tx amount exceeds balance when fees are included" error This was previously implemented at the GUI level but has been broken since #20640 --- src/wallet/spend.cpp | 14 +++++++++++++- test/functional/wallet_fundrawtransaction.py | 19 ++++++++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 9a7e166e689..24053a4b7b5 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1131,7 +1131,19 @@ static util::Result CreateTransactionInternal( if (!select_coins_res) { // 'SelectCoins' either returns a specific error message or, if empty, means a general "Insufficient funds". const bilingual_str& err = util::ErrorString(select_coins_res); - return util::Error{err.empty() ?_("Insufficient funds") : err}; + if (!err.empty()) return util::Error{err}; + + // Check if we have enough balance but cannot cover the fees + CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount(); + if (available_balance >= recipients_sum) { + CAmount available_effective_balance = preset_inputs.total_amount + available_coins.GetEffectiveTotalAmount().value_or(available_coins.GetTotalAmount()); + if (available_effective_balance < selection_target) { + return util::Error{_("The total transaction amount exceeds your balance when fees are included")}; + } + } + + // General failure description + return util::Error{_("Insufficient funds")}; } const SelectionResult& result = *select_coins_res; TRACE5(coin_selection, selected_coins, diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 71c883f166b..6a229c6d32a 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -150,6 +150,7 @@ def run_test(self): self.test_feerate_rounding() self.test_input_confs_control() self.test_duplicate_outputs() + self.test_cannot_cover_fees() def test_duplicate_outputs(self): self.log.info("Test deserializing and funding a transaction with duplicate outputs") @@ -1426,7 +1427,8 @@ def test_feerate_rounding(self): # To test this does not happen, we subtract 202 sats from the input value. If working correctly, this should # fail with insufficient funds rather than bitcoind asserting. rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(address_type="bech32"): 1 - 0.00000202}]) - assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, fee_rate=1.85) + expected_err_msg = "The total transaction amount exceeds your balance when fees are included" + assert_raises_rpc_error(-4, expected_err_msg, w.fundrawtransaction, rawtx, fee_rate=1.85) def test_input_confs_control(self): self.nodes[0].createwallet("minconf") @@ -1489,5 +1491,20 @@ def test_input_confs_control(self): wallet.unloadwallet() + def test_cannot_cover_fees(self): + self.log.info("Test tx amount exceeds available balance when fees are included") + + self.nodes[1].createwallet("cannot_cover_fees") + wallet = self.nodes[1].get_wallet_rpc("cannot_cover_fees") + + self.nodes[0].sendtoaddress(wallet.getnewaddress(), 0.3) + self.generate(self.nodes[0], 1) + + rawtx = wallet.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(): 0.3}]) + expected_err_msg = "The total transaction amount exceeds your balance when fees are included" + assert_raises_rpc_error(-4, expected_err_msg, wallet.fundrawtransaction, rawtx) + wallet.unloadwallet() + + if __name__ == '__main__': RawTransactionsTest().main() From a4007a447ed9a6c21812fe5b3d468cf6f7039a5b Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 22 Mar 2024 09:12:11 -0300 Subject: [PATCH 2/4] gui: remove unreachable AmountWithFeeExceedsBalance error Since #20640, the 'createTransaction' does no longer retrieve the fee if the process fails due to insufficient funds. But, since #25269, 'createTransaction' retrieves an error message indicating that the total transaction amount exceeds the wallet available balance when fees are included. So this enum is no longer needed. --- src/qt/walletmodel.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 87ad98a4cc3..ff01b3163d2 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -214,12 +214,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact if (fSubtractFeeFromAmount && newTx) transaction.reassignAmounts(nChangePosRet); - if(!newTx) - { - if(!fSubtractFeeFromAmount && (total + nFeeRequired) > nBalance) - { - return SendCoinsReturn(AmountWithFeeExceedsBalance); - } + if (!newTx) { Q_EMIT message(tr("Send Coins"), QString::fromStdString(util::ErrorString(res).translated), CClientUIInterface::MSG_ERROR); return TransactionCreationFailed; From 9cd8462a6a070be07e7de7555f3963c5bc76e54a Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 21 Mar 2024 17:57:19 -0300 Subject: [PATCH 3/4] refactor: move CreatedTransactionResult to types.h So it can be used by external modules without requiring wallet.h dependency. --- src/wallet/spend.h | 12 +----------- src/wallet/types.h | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 62a7b4e4c89..f6d0a193bd4 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -202,17 +203,6 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -struct CreatedTransactionResult -{ - CTransactionRef tx; - CAmount fee; - FeeCalculation fee_calc; - std::optional change_pos; - - CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional _change_pos, const FeeCalculation& _fee_calc) - : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {} -}; - /** * Create a new transaction paying the recipients with a set of coins * selected by SelectCoins(); Also create the change output, when needed diff --git a/src/wallet/types.h b/src/wallet/types.h index 6198f1ae330..2e1390e7be7 100644 --- a/src/wallet/types.h +++ b/src/wallet/types.h @@ -13,6 +13,8 @@ #ifndef BITCOIN_WALLET_TYPES_H #define BITCOIN_WALLET_TYPES_H +#include + #include namespace wallet { @@ -62,6 +64,18 @@ enum class AddressPurpose { SEND, REFUND, //!< Never set in current code may be present in older wallet databases }; + +struct CreatedTransactionResult +{ + CTransactionRef tx; + CAmount fee; + FeeCalculation fee_calc; + std::optional change_pos; + + CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional _change_pos, const FeeCalculation& _fee_calc) + : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {} +}; + } // namespace wallet #endif // BITCOIN_WALLET_TYPES_H From 0b92ee2bc7a75074651e4688912e8e3c7de06a5a Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 22 Mar 2024 09:34:04 -0300 Subject: [PATCH 4/4] refactor: interfaces, make 'createTransaction' less error-prone Bundle all function's outputs inside the util::Result returned object. Reasons for the refactoring: - The 'change_pos' ref argument has been a source of bugs in the past. - The 'fee' ref argument is currently only set when the transaction creation process succeeds. --- src/interfaces/wallet.h | 6 +++--- src/qt/walletmodel.cpp | 24 +++++++++++++----------- src/wallet/interfaces.cpp | 14 +++----------- 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index c41f35829d7..7752745202a 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -34,6 +34,7 @@ enum class TransactionError; struct PartiallySignedTransaction; struct bilingual_str; namespace wallet { +struct CreatedTransactionResult; class CCoinControl; class CWallet; enum class AddressPurpose; @@ -142,11 +143,10 @@ class Wallet virtual void listLockedCoins(std::vector& outputs) = 0; //! Create transaction. - virtual util::Result createTransaction(const std::vector& recipients, + virtual util::Result createTransaction(const std::vector& recipients, const wallet::CCoinControl& coin_control, bool sign, - int& change_pos, - CAmount& fee) = 0; + std::optional change_pos) = 0; //! Commit transaction. virtual void commitTransaction(CTransactionRef tx, diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index ff01b3163d2..f47aef81bf8 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include // for CRecipient #include @@ -153,6 +154,8 @@ bool WalletModel::validateAddress(const QString& address) const WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransaction &transaction, const CCoinControl& coinControl) { + transaction.getWtx() = nullptr; // reset tx output + CAmount total = 0; bool fSubtractFeeFromAmount = false; QList recipients = transaction.getRecipients(); @@ -204,22 +207,21 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact } try { - CAmount nFeeRequired = 0; - int nChangePosRet = -1; - auto& newTx = transaction.getWtx(); - const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled(), nChangePosRet, nFeeRequired); - newTx = res ? *res : nullptr; - transaction.setTransactionFee(nFeeRequired); - if (fSubtractFeeFromAmount && newTx) - transaction.reassignAmounts(nChangePosRet); - - if (!newTx) { + const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled(), /*change_pos=*/std::nullopt); + if (!res) { Q_EMIT message(tr("Send Coins"), QString::fromStdString(util::ErrorString(res).translated), - CClientUIInterface::MSG_ERROR); + CClientUIInterface::MSG_ERROR); return TransactionCreationFailed; } + newTx = res->tx; + CAmount nFeeRequired = res->fee; + transaction.setTransactionFee(nFeeRequired); + if (fSubtractFeeFromAmount && newTx) { + transaction.reassignAmounts(res->change_pos ? int(*res->change_pos) : -1); + } + // Reject absurdly high fee. (This can never happen because the // wallet never creates transactions with fee greater than // m_default_max_tx_fee. This merely a belt-and-suspenders check). diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 0c1cae7253c..03a704971d3 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -274,21 +274,13 @@ class WalletImpl : public Wallet LOCK(m_wallet->cs_wallet); return m_wallet->ListLockedCoins(outputs); } - util::Result createTransaction(const std::vector& recipients, + util::Result createTransaction(const std::vector& recipients, const CCoinControl& coin_control, bool sign, - int& change_pos, - CAmount& fee) override + std::optional change_pos) override { LOCK(m_wallet->cs_wallet); - auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos), - coin_control, sign); - if (!res) return util::Error{util::ErrorString(res)}; - const auto& txr = *res; - fee = txr.fee; - change_pos = txr.change_pos ? int(*txr.change_pos) : -1; - - return txr.tx; + return CreateTransaction(*m_wallet, recipients, change_pos, coin_control, sign); } void commitTransaction(CTransactionRef tx, WalletValueMap value_map,