Skip to content

refactor: interfaces, make 'createTransaction' less error-prone #807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hebasto merged 3 commits into from
Feb 10, 2026
Merged

refactor: interfaces, make 'createTransaction' less error-prone #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4537217
gui: remove AmountWithFeeExceedsBalance error special case
furszy Mar 22, 2024
e2c3ec9
refactor: move CreatedTransactionResult to types.h
furszy Mar 21, 2024
4c0d4f6
refactor: interfaces, make 'createTransaction' less error-prone
furszy Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/interfaces/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ namespace node {
enum class TransactionError;
} // namespace node
namespace wallet {
struct CreatedTransactionResult;
class CCoinControl;
class CWallet;
enum class AddressPurpose;
Expand Down Expand Up @@ -142,11 +143,10 @@ class Wallet
virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0;

//! Create transaction.
virtual util::Result<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
virtual util::Result<wallet::CreatedTransactionResult> createTransaction(const std::vector<wallet::CRecipient>& recipients,
const wallet::CCoinControl& coin_control,
bool sign,
int& change_pos,
CAmount& fee) = 0;
std::optional<unsigned int> change_pos) = 0;

//! Commit transaction.
virtual void commitTransaction(CTransactionRef tx,
Expand Down
3 changes: 0 additions & 3 deletions src/qt/sendcoinsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
29 changes: 13 additions & 16 deletions src/qt/walletmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <psbt.h>
#include <util/translation.h>
#include <wallet/coincontrol.h>
#include <wallet/types.h>
#include <wallet/wallet.h>

#include <cstdint>
Expand Down Expand Up @@ -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
Copy link
Contributor

@vasild vasild Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this is needed.

Copy link
Member Author

@furszy furszy Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this is needed.

It is just a safety belt that resets the state in case the caller invokes the function with an existing WalletModelTransaction. This shouldn’t happen nowadays since we create a new pointer per call, but if it ever does, for example with a duplicate recipient, the function would return StatusCode::OK with a tx even though nothing was sent.
Overall, this function needs lot more love but this reset is quite simple and effective to prevent other issues for now.


CAmount total = 0;
bool fSubtractFeeFromAmount = false;
QList<SendCoinsRecipient> recipients = transaction.getRecipients();
Expand Down Expand Up @@ -199,27 +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)
{
if(!fSubtractFeeFromAmount && (total + nFeeRequired) > nBalance)
{
return SendCoinsReturn(AmountWithFeeExceedsBalance);
Copy link
Contributor

@ryanofsky ryanofsky May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "gui: remove unreachable AmountWithFeeExceedsBalance error" (355199c)

This is a nice catch noticing that this code doesn't work. But I think the fact that it doesn't work is a pretty unfortunate problem, and the current PR will make fixing it harder.

It seems like bitcoin/bitcoin#20640 unintentionally introduced a bug where the more accurate error message "The total exceeds your balance when the %1 transaction fee is included" can never trigger, and instead only less informative "Insufficient funds" or "The amount exceeds your balance" errors will be shown.

It looks like there are few ways this could be fixed:

  • An easy way to fix it would be to restore the CAmount& fee output parameter removed by #20640 so it is always returned and this code can work the way it was originally intended.

  • A slightly more complicated but cleaner fix would be to use CreatedTransactionResult as a return type instead of util::Result<CreatedTransactionResult> and add a bilingual_str error field to CreatedTransactionResult so fee information and an error string can be returned together.

  • Another alternative fix could be to build on bitcoin/bitcoin#25665 and keep using util::Result, but return the fee in the new util::Result failure field.

  • And the best but probably most complicated fix would be to remove error message code from the GUI and instead generate detailed error messages in wallet code. This would require bigger changes in the GUI code and might require changing the interface to provide the wallet with more information about transactions.

Against that background, I think this PR (as of edc3f9a) makes some nice cleanups, but I don't like the current changes because I think they make all of the fixes above except maybe the last one harder to implement.

So I don't think I would ACK this PR currently, I'd happily ACK it if it either (1) fixed the problem, maybe using one of the easier fixes suggested above, or (2) did not fix the problem, but at least did not make it harder to fix in the future. This could be implemented by keeping most of the changes in the PR, but not deleting this GUI code in this commit and not deleting the CAmount& fee output parameter from the wallet interface after this commit.

}
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<int>(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).
Expand Down
1 change: 0 additions & 1 deletion src/qt/walletmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ class WalletModel : public QObject
InvalidAmount,
InvalidAddress,
AmountExceedsBalance,
AmountWithFeeExceedsBalance,
DuplicateAddress,
TransactionCreationFailed, // Error returned when wallet is still locked
AbsurdFee
Expand Down
14 changes: 3 additions & 11 deletions src/wallet/interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,21 +257,13 @@ class WalletImpl : public Wallet
LOCK(m_wallet->cs_wallet);
return m_wallet->ListLockedCoins(outputs);
}
util::Result<CTransactionRef> createTransaction(const std::vector<CRecipient>& recipients,
util::Result<wallet::CreatedTransactionResult> createTransaction(const std::vector<CRecipient>& recipients,
const CCoinControl& coin_control,
bool sign,
int& change_pos,
CAmount& fee) override
std::optional<unsigned int> 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,
Expand Down
12 changes: 1 addition & 11 deletions src/wallet/spend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <util/result.h>
#include <wallet/coinselection.h>
#include <wallet/transaction.h>
#include <wallet/types.h>
#include <wallet/wallet.h>

#include <map>
Expand Down Expand Up @@ -186,17 +187,6 @@ util::Result<SelectionResult> 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<unsigned int> change_pos;

CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional<unsigned int> _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
Expand Down
14 changes: 13 additions & 1 deletion src/wallet/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#ifndef BITCOIN_WALLET_TYPES_H
#define BITCOIN_WALLET_TYPES_H

#include <type_traits>
#include <policy/fees/block_policy_estimator.h>

namespace wallet {
/**
Expand All @@ -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<unsigned int> change_pos;

CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional<unsigned int> _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
Loading