Low 45 Bitcoin Committed Jul 16, 2026 · 17:56 UTC

wallet: Deserialize directly in CWalletTx's ctor

19af439b View source ↗

Public commit record

What the developer wrote

Authored by Ava Chow

80/100 · Strong

wallet: Deserialize directly in CWalletTx's ctor

When loading a transaction, instead of constructing a CWalletTx with no
transaction, pass the DataStream into the constructor so that the
CWalletTx is RAII. This allows us to ensure that the transaction is
never a nullptr so that dereferences, especially once multiple txs are
stored, will not cause a segfault.

✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode

The short version

What changed, and why it matters

This commit changes how Bitcoin Core wallet transactions are loaded from disk or copied between wallets. Previously, a wallet transaction object could be created with no actual transaction inside it, and later filled in. The patch makes the transaction data mandatory from the start using a constructor that deserializes directly from a data stream. The stated goal is to prevent crashes (segfaults) caused by accidentally using a null transaction pointer, especially as more transaction types are stored in the wallet. It also improves error handling for corrupt wallet records.

Recommended action

This is a defensive hardening change. Users and downstream maintainers should treat it as a routine bug-fix/hardening patch. No immediate emergency action is required. Wallet operators should ensure they run a version containing this patch, especially if loading wallets with mixed or new transaction types. If a wallet previously triggered crashes on load, this patch may help; if loading now reports corruption, follow the suggested rescan/removal procedure.

Security signals we found

Null-pointer dereference prevention: CWalletTx transaction member is now guaranteed non-null at construction via Assert(tx) and direct deserialization.

RAII-style construction: transaction object is fully initialized in constructor rather than two-phase init-then-fill.

Improved corruption handling: deserialization exceptions are caught and reported as DBErrors::CORRUPT.

Refactoring of LoadToWallet to remove callback-based filling, reducing window where object is partially constructed.

Serialization round-trip used for copying wallet transactions between wallets during export and migration.

Technical analysis

Evidence from the diff

The patch refactors CWalletTx construction and CWallet::LoadToWallet. CWalletTx now has a constructor taking a deserialize_type tag and a Stream, which calls Unserialize() during construction and initializes m_state to TxStateInactive. The existing CWalletTx(CTransactionRef tx, const TxState& state) constructor now wraps tx with Assert(tx) to guarantee non-null. LoadToWallet’s signature changes from taking a Txid and a callback to taking a CWalletTx&&, making the caller responsible for constructing a valid object before insertion. walletdb.cpp now deserializes CWalletTx{deserialize, value} before calling LoadToWallet, and catches std::exception to report corrupt records. export.cpp and wallet.cpp’s migration code now serialize an existing CWalletTx and deserialize a copy rather than using a callback to fill fields. A default move constructor is enabled for CWalletTx.

Changed components

src/wallet/transaction.hsrc/wallet/wallet.hsrc/wallet/wallet.cppsrc/wallet/walletdb.cppsrc/wallet/export.cppCWalletTx classCWallet::LoadToWallet methodWallet database loading (LoadTxRecords)Watch-only wallet exportWallet migration

Inspect captured patch +36 / −38

diff --git a/src/wallet/export.cpp b/src/wallet/export.cpp
index 1df51b6b..d9ca2b34 100644
--- a/src/wallet/export.cpp
+++ b/src/wallet/export.cpp
@@ -159,12 +159,10 @@ util::Result<std::string> ExportWatchOnlyWallet(const CWallet& wallet, const fs:
 
             // Copy the transactions
             for (const auto& [txid, wtx] : wallet.mapWallet) {
-                if (!watchonly_wallet->LoadToWallet(txid, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(watchonly_wallet->cs_wallet) {
-                    if (!new_tx) return false;
-                    ins_wtx.SetTx(wtx.tx);
-                    ins_wtx.CopyFrom(wtx);
-                    return true;
-                })) {
+                DataStream wtx_ser;
+                wtx_ser << wtx;
+                CWalletTx copy_wtx(deserialize, wtx_ser);
+                if (!watchonly_wallet->LoadToWallet(std::move(copy_wtx))) {
                     return util::Error{strprintf(_("Error: Could not add tx %s to watchonly wallet"), txid.GetHex())};
                 }
                 watchonly_batch.WriteTx(watchonly_wallet->mapWallet.at(txid));
diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
index 6159bf12..a715aa81 100644
--- a/src/wallet/transaction.h
+++ b/src/wallet/transaction.h
@@ -234,11 +234,17 @@ public:
     mutable bool fChangeCached;
     mutable CAmount nChangeCached;
 
-    CWalletTx(CTransactionRef tx, const TxState& state) : tx(std::move(tx)), m_state(state)
+    CWalletTx(CTransactionRef tx, const TxState& state) : tx(std::move(Assert(tx))), m_state(state)
     {
         Init();
     }
 
+    template <typename Stream>
+    CWalletTx(deserialize_type, Stream& s) : m_state(TxStateInactive{})
+    {
+        Unserialize(s);
+    }
+
     void Init()
     {
         nTimeReceived = 0;
@@ -385,6 +391,9 @@ private:
 public:
     // Instead have an explicit copy function
     void CopyFrom(const CWalletTx&);
+
+    // Enable the default move constructor
+    CWalletTx(CWalletTx&&) = default;
 };
 
 struct WalletTxOrderComparator {
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index c36472b2..25510c5f 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1181,11 +1181,11 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
     return &wtx;
 }
 
-bool CWallet::LoadToWallet(const Txid& hash, const UpdateWalletTxFn& fill_wtx)
+bool CWallet::LoadToWallet(CWalletTx&& wtx_in)
 {
-    const auto& ins = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(nullptr, TxStateInactive{}));
+    const auto& ins = mapWallet.emplace(wtx_in.GetHash(), std::move(wtx_in));
     CWalletTx& wtx = ins.first->second;
-    if (!fill_wtx(wtx, ins.second)) {
+    if (!ins.second) {
         return false;
     }
     // If wallet doesn't have a chain (e.g when using bitcoin-wallet tool),
@@ -1193,9 +1193,7 @@ bool CWallet::LoadToWallet(const Txid& hash, const UpdateWalletTxFn& fill_wtx)
     if (HaveChain()) {
       wtx.updateState(chain());
     }
-    if (/* insertion took place */ ins.second) {
-        wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
-    }
+    wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
     AddToSpends(wtx);
     for (const CTxIn& txin : wtx.tx->vin) {
         auto it = mapWallet.find(txin.prevout.hash);
@@ -4049,13 +4047,10 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
             if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) {
                 // Add to watchonly wallet
                 const Txid& hash = wtx->GetHash();
-                const CWalletTx& to_copy_wtx = *wtx;
-                if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) {
-                    if (!new_tx) return false;
-                    ins_wtx.SetTx(to_copy_wtx.tx);
-                    ins_wtx.CopyFrom(to_copy_wtx);
-                    return true;
-                })) {
+                DataStream wtx_ser;
+                wtx_ser << *wtx;
+                CWalletTx copy_wtx(deserialize, wtx_ser);
+                if (!data.watchonly_wallet->LoadToWallet(std::move(copy_wtx))) {
                     return util::Error{strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex())};
                 }
                 watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash));
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 3e70a24b..9b1bb8b6 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -630,7 +630,7 @@ public:
      * @return the recently added wtx pointer or nullptr if there was a db write error.
      */
     CWalletTx* AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx=nullptr, bool rescanning_old_block = false);
-    bool LoadToWallet(const Txid& hash, const UpdateWalletTxFn& fill_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool LoadToWallet(CWalletTx&& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void transactionAddedToMempool(const CTransactionRef& tx) override;
     void blockConnected(const kernel::ChainstateRole& role, const interfaces::BlockInfo& block) override;
     void blockDisconnected(const interfaces::BlockInfo& block) override;
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 5ad29970..8f208223 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -992,27 +992,23 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, bool& any_
         DBErrors result = DBErrors::LOAD_OK;
         Txid hash;
         key >> hash;
-        // LoadToWallet call below creates a new CWalletTx that fill_wtx
-        // callback fills with transaction metadata.
-        auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) {
-            if(!new_tx) {
-                // There's some corruption here since the tx we just tried to load was already in the wallet.
-                err = "Error: Corrupt transaction found. This can be fixed by removing transactions from wallet and rescanning.";
-                result = DBErrors::CORRUPT;
-                return false;
+        try {
+            CWalletTx wtx{deserialize, value};
+            if (wtx.GetHash() != hash) {
+                result = std::max(result, DBErrors::NEED_RESCAN);
             }
-            value >> wtx;
-            if (wtx.GetHash() != hash)
-                return false;
 
-            if (wtx.nOrderPos == -1)
+            if (wtx.nOrderPos == -1) {
                 any_unordered = true;
+            }
 
-            return true;
-        };
-        if (!pwallet->LoadToWallet(hash, fill_wtx)) {
-            // Use std::max as fill_wtx may have already set result to CORRUPT
-            result = std::max(result, DBErrors::NEED_RESCAN);
+            if (!pwallet->LoadToWallet(std::move(wtx))) {
+                err = "Error: Corrupt transaction found. This can be fixed by removing transactions from wallet and rescanning.";
+                return DBErrors::CORRUPT;
+            }
+        } catch (const std::exception& e) {
+            err = strprintf("Error: Corrupt tx record found: %s" ,e.what());
+            return DBErrors::CORRUPT;
         }
         return result;
     });

Risk score

Why this scored 45/100

Our methodology →

Potential impact 12/30

Exploitability 5/25

Stealth signal 8/15

Affected reach 10/15

Confidence 7/10

Evidence quality 3/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 45/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`