From 45372175c35b73bfd33a1387b2295fc61d9eaaa9 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 22 Mar 2024 09:12:11 -0300 Subject: [PATCH 1/3] gui: remove AmountWithFeeExceedsBalance error special case Since bitcoin#34299, the wallet handles the error internally and retrieves the proper message. --- src/qt/sendcoinsdialog.cpp | 3 --- src/qt/walletmodel.cpp | 7 +------ src/qt/walletmodel.h | 1 - 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index c475c6b73e9..90e46c79815 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -742,9 +742,6 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn case WalletModel::AmountExceedsBalance: msgParams.first = tr("The amount exceeds your balance."); break; - case WalletModel::AmountWithFeeExceedsBalance: - msgParams.first = tr("The total exceeds your balance when the %1 transaction fee is included.").arg(msgArg); - break; case WalletModel::DuplicateAddress: msgParams.first = tr("Duplicate address found: addresses should only be used once each."); break; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 578713c0abc..2880c6e7381 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -209,12 +209,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; diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index c4abde8ba39..ced057574b8 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -59,7 +59,6 @@ public: InvalidAmount, InvalidAddress, AmountExceedsBalance, - AmountWithFeeExceedsBalance, DuplicateAddress, TransactionCreationFailed, // Error returned when wallet is still locked AbsurdFee From e2c3ec9bf4126564070f4f1097bea45753e41ead Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 21 Mar 2024 17:57:19 -0300 Subject: [PATCH 2/3] 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, 14 insertions(+), 12 deletions(-) diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 75090b308c3..925111dce03 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -186,17 +187,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) {} -}; - /** * Set a height-based locktime for new transactions (uses the height of the * current chain tip unless we are not synced with the current chain diff --git a/src/wallet/types.h b/src/wallet/types.h index 97c18d4f3fd..09ad4a0ab4c 100644 --- a/src/wallet/types.h +++ b/src/wallet/types.h @@ -14,7 +14,7 @@ #ifndef BITCOIN_WALLET_TYPES_H #define BITCOIN_WALLET_TYPES_H -#include +#include namespace wallet { /** @@ -30,6 +30,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 4c0d4f6f93f371a8ad097735945d32510a7e83bb Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 22 Mar 2024 09:34:04 -0300 Subject: [PATCH 3/3] 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 40c612a4c51..bba9e058cc0 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -40,6 +40,7 @@ namespace node { enum class TransactionError; } // namespace node namespace wallet { +struct CreatedTransactionResult; class CCoinControl; class CWallet; enum class AddressPurpose; @@ -142,11 +143,10 @@ public: 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 2880c6e7381..7a35f8e4d35 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -149,6 +150,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(); @@ -199,22 +202,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(static_cast(res->change_pos.value_or(-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 8e0f240a6fe..3ffac043b95 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -257,21 +257,13 @@ public: 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,