Informational 19 Bitcoin Committed Feb 6, 2026 · 23:15 UTC

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

4c0d4f6f View source ↗

Public commit record

What the developer wrote

Authored by furszy

85/100 · Strong

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.

✓ Specific, descriptive subject✓ Names a concrete action or component✓ Uses a recognizable type or scope✓ Provides detailed explanatory context

The short version

What changed, and why it matters

This is a code cleanup change in Bitcoin Core. It bundles the outputs of the 'createTransaction' function into a single result object instead of using separate reference arguments. The commit message says this makes the function less error-prone because the old way of passing back 'change position' and 'fee' through reference arguments had caused bugs before. There is no direct evidence in the diff of a currently exploitable security vulnerability being fixed.

Recommended action

No immediate action required. Treat as routine defensive refactoring. Reviewers may want to verify that all consumers of createTransaction correctly handle the new util::Result<CreatedTransactionResult> return type and that no callers still expect the old out-parameter behavior.

Security signals we found

Defensive refactoring to reduce bug-prone out-parameter usage

Commit message references past bugs related to change_pos out-parameter

No direct fix of an identified vulnerability in the diff

Change touches transaction creation code path (high-value area)

Technical analysis

Evidence from the diff

The patch refactors the wallet interface’s createTransaction method. Previously it returned util::Result and wrote change_pos and fee back via out-parameters. Now it returns util::Result containing tx, fee, and change_pos together. The implementation in wallet/interfaces.cpp becomes a thin wrapper around CreateTransaction. The Qt wallet model is updated to read results from the returned struct and to reset the transaction output pointer at the start. The commit explicitly frames this as defensive refactoring to prevent future bugs, not as a fix for an active security issue.

Changed components

src/interfaces/wallet.hsrc/qt/walletmodel.cppsrc/wallet/interfaces.cpp

Inspect captured patch +19 / −25

diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 40c612a4..bba9e058 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<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,
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 2880c6e7..7a35f8e4 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -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>
@@ -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<SendCoinsRecipient> 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<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).
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index 8e0f240a..3ffac043 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<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,

Risk score

Why this scored 19/100

Our methodology →

Potential impact 2/30

Exploitability 1/25

Stealth signal 1/15

Affected reach 3/15

Confidence 8/10

Evidence quality 4/5

Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.

What the developer wrote

What changed, and why it matters

Security signals we found

Evidence from the diff

Changed components

Why this scored 19/100

Community notes

Related watches

test: test the result order of a multiple import request is correct

test: test invalid or missing timestamp throws importdescriptors

ci, iwyu: Enforce warning-free `src/common`