achow101
commented at 6:40 PM on June 12, 2023:
member
In #27286, the wallet keeps track of all of its transaction outputs, even if they are already spent or are otherwise unspendable. This TXO set is iterated for balance checking and coin selection preparation, which can still be slow for wallets that have had a lot of activity. This PR aims to improve the performance of such wallets by moving UTXOs that are definitely no longer spendable to a different map in the wallet so that far fewer TXOs need to be iterated for the aforementioned functions.
Unspendable TXOs (not to be confused with Unspent TXOs) are those which have a spending transaction that has been confirmed, or are no longer valid due to reorgs. TXOs that are spent in unconfirmed transactions remain in the primary TXO set, and are filtered out of balance and coin selection as before.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
#35587 (Remove boost as a unit test runner by rustaceanrob)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#35501 (wallet: store all witness variants of a transaction by achow101)
#35302 (Silent Payments: Sending (take 2) by Eunovo)
#35294 (wallet: Update tx chain state during loading during AttachChain instead of before by achow101)
#35151 (wallet, follow-up: Refactor IsSpent to use HowSpent by musaHaruna)
#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#34872 (wallet: fix mixed-input transaction accounting in history RPCs by w0xlt)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#32763 (wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members by achow101)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
DrahtBot added the label Wallet on Jun 12, 2023
DrahtBot added the label CI failed on Jun 12, 2023
DrahtBot added the label Needs rebase on Jun 28, 2023
achow101 force-pushed on Jun 28, 2023
DrahtBot removed the label Needs rebase on Jun 28, 2023
achow101 force-pushed on Jun 28, 2023
achow101 force-pushed on Jun 28, 2023
DrahtBot added the label Needs rebase on Sep 6, 2023
achow101 force-pushed on Sep 7, 2023
DrahtBot removed the label Needs rebase on Sep 7, 2023
achow101
commented at 9:32 PM on September 7, 2023:
member
@t-bast You may be interested in this for very large wallets.
t-bast
commented at 12:26 PM on September 11, 2023:
contributor
Neat, thanks @achow101, @remyers will give that a try and will review the related PRs!
remyers
commented at 7:49 PM on September 12, 2023:
contributor
Concept ACK - on a large synthetic test wallet I'm seeing a 250x speed up.
I created a large wallet in regtest with many fanout style transactions - 100,000 blocks each creating 1000 utxos. There are only 117 utxos currently, but the wallet.dat file is 370M.
Using the same wallet.dat file and (transaction history) fundrawtransaction saw a substantial speedup:
~5 seconds using v25.99.0-a36134fcc7b4
~20 milliseconds using v25.99.0-d9c4dcfe68c5 (latest wallet-unspent-txos)
I will try to recreate my large test wallet.dat file tomorrow so I can confirm the steps to reproduce it. Is there anything in particular I can do to help? Would you like more testing? code review?
NB: I had to export my original wallet descriptors and re-import them to work around the issue fixed in #27920. Before the export/import the wallet.dat file was 1.1G, but the speed of fundrawtransaction was 5 sec both in the original file and the tested one created via importing descriptors.
achow101
commented at 8:55 PM on September 12, 2023:
member
Is there anything in particular I can do to help? Would you like more testing? code review?
The prerequisite PR (#27286) will need review and testing before this can be merged. That should also provide some speed up.
remyers
commented at 9:53 AM on September 13, 2023:
contributor
The prerequisite PR (#27286) will need review and testing before this can be merged. That should also provide some speed up.
Will do. I'm getting 400-500ms for the same test using just #27286, so indeed also ~100x speed-up.
DrahtBot added the label Needs rebase on Sep 14, 2023
achow101 force-pushed on Sep 14, 2023
DrahtBot removed the label Needs rebase on Sep 14, 2023
achow101 force-pushed on Sep 15, 2023
achow101 force-pushed on Sep 15, 2023
DrahtBot added the label Needs rebase on Sep 19, 2023
achow101 force-pushed on Sep 27, 2023
DrahtBot removed the label Needs rebase on Sep 27, 2023
DrahtBot added the label Needs rebase on Oct 16, 2023
achow101 force-pushed on Oct 16, 2023
DrahtBot removed the label Needs rebase on Oct 16, 2023
DrahtBot added the label Needs rebase on Oct 23, 2023
achow101 force-pushed on Oct 24, 2023
DrahtBot removed the label Needs rebase on Oct 24, 2023
achow101 force-pushed on Oct 24, 2023
DrahtBot added the label Needs rebase on Oct 29, 2023
achow101 force-pushed on Nov 13, 2023
DrahtBot removed the label Needs rebase on Nov 13, 2023
DrahtBot added the label Needs rebase on Nov 15, 2023
achow101 force-pushed on Nov 15, 2023
DrahtBot removed the label Needs rebase on Nov 15, 2023
DrahtBot added the label Needs rebase on Nov 24, 2023
achow101 force-pushed on Nov 28, 2023
DrahtBot removed the label Needs rebase on Nov 28, 2023
DrahtBot added the label Needs rebase on Dec 8, 2023
achow101 force-pushed on Dec 8, 2023
DrahtBot removed the label Needs rebase on Dec 8, 2023
DrahtBot added the label Needs rebase on Dec 11, 2023
achow101 force-pushed on Dec 11, 2023
DrahtBot removed the label Needs rebase on Dec 11, 2023
DrahtBot added the label Needs rebase on Dec 12, 2023
achow101 force-pushed on Dec 19, 2023
DrahtBot removed the label Needs rebase on Dec 19, 2023
murchandamus
commented at 7:47 PM on December 28, 2023:
member
Concept ACK
DrahtBot added the label Needs rebase on Feb 3, 2024
achow101 force-pushed on Feb 3, 2024
DrahtBot removed the label Needs rebase on Feb 3, 2024
DrahtBot added the label Needs rebase on Feb 20, 2024
achow101 force-pushed on Feb 20, 2024
DrahtBot removed the label Needs rebase on Feb 21, 2024
DrahtBot added the label Needs rebase on Mar 27, 2024
achow101 force-pushed on Apr 1, 2024
DrahtBot removed the label Needs rebase on Apr 1, 2024
DrahtBot added the label Needs rebase on Apr 8, 2024
achow101 force-pushed on Apr 25, 2024
DrahtBot removed the label Needs rebase on Apr 25, 2024
achow101 force-pushed on Jun 6, 2024
DrahtBot added the label Needs rebase on Aug 27, 2024
achow101 force-pushed on Aug 29, 2024
DrahtBot removed the label Needs rebase on Aug 29, 2024
achow101 force-pushed on Sep 10, 2024
DrahtBot added the label Needs rebase on Oct 24, 2024
achow101 force-pushed on Oct 24, 2024
DrahtBot removed the label Needs rebase on Oct 25, 2024
achow101 force-pushed on Oct 28, 2024
achow101 force-pushed on Oct 28, 2024
achow101 force-pushed on Oct 28, 2024
DrahtBot removed the label CI failed on Oct 28, 2024
jonatack
commented at 12:35 PM on November 26, 2024:
member
Concept ACK, pending #27286, if this improves performance for wallets with many transactions.
DrahtBot added the label Needs rebase on Mar 13, 2025
achow101 force-pushed on Apr 10, 2025
DrahtBot removed the label Needs rebase on Apr 10, 2025
DrahtBot added the label Needs rebase on Apr 25, 2025
achow101 force-pushed on Apr 25, 2025
DrahtBot removed the label Needs rebase on Apr 25, 2025
DrahtBot added the label Needs rebase on May 7, 2025
achow101 force-pushed on May 7, 2025
DrahtBot removed the label Needs rebase on May 7, 2025
achow101 force-pushed on May 7, 2025
DrahtBot added the label Needs rebase on May 14, 2025
achow101 force-pushed on May 15, 2025
DrahtBot removed the label Needs rebase on May 15, 2025
achow101 force-pushed on May 16, 2025
DrahtBot added the label Needs rebase on May 16, 2025
achow101 force-pushed on May 16, 2025
DrahtBot removed the label Needs rebase on May 16, 2025
achow101 force-pushed on May 16, 2025
DrahtBot added the label CI failed on May 19, 2025
achow101 force-pushed on May 19, 2025
DrahtBot removed the label CI failed on May 19, 2025
DrahtBot added the label Needs rebase on May 19, 2025
achow101 force-pushed on May 20, 2025
DrahtBot removed the label Needs rebase on May 20, 2025
DrahtBot added the label Needs rebase on May 21, 2025
achow101 force-pushed on May 21, 2025
DrahtBot removed the label Needs rebase on May 21, 2025
DrahtBot added the label Needs rebase on May 30, 2025
achow101 force-pushed on May 30, 2025
DrahtBot removed the label Needs rebase on May 30, 2025
in
src/wallet/test/util.cpp:26
in
9554a4d79aoutdated
DrahtBot added the label Needs rebase on Jun 25, 2025
achow101 force-pushed on Jun 27, 2025
DrahtBot added the label CI failed on Jun 27, 2025
DrahtBot
commented at 3:08 AM on June 27, 2025:
contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/44895352538</sub>
<sub>LLM reason (✨ experimental): The failure is caused by a Python lint error due to an unused variable in the wallet_backwards_compatibility.py test.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
DrahtBot removed the label Needs rebase on Jun 27, 2025
achow101 force-pushed on Jun 27, 2025
achow101 force-pushed on Jun 27, 2025
DrahtBot removed the label CI failed on Jun 28, 2025
achow101 force-pushed on Jul 1, 2025
achow101 marked this as ready for review on Jul 1, 2025
in
src/wallet/transaction.h:222
in
25a16d0a25outdated
214 | @@ -215,6 +215,11 @@ class CWalletTx
215 | * CWallet::ComputeTimeSmart().
216 | */
217 | unsigned int nTimeSmart;
218 | + /** 219 | + * For every isminetype, we want to track whether the transaction spends any 220 | + * inputs that match that isminetype. 221 | + */ 222 | + std::optional<bool> m_from_me;
Using a default value has the potential very problematic because its value is expected to be correct. By using an optional, we can detect when there is a programming error that did not set the value.
DrahtBot added the label Needs rebase on Jul 7, 2025
achow101 force-pushed on Jul 7, 2025
DrahtBot removed the label Needs rebase on Jul 7, 2025
Zeegaths
commented at 12:54 AM on August 1, 2025:
none
Performance Testing Results
Tested this PR against master using regtest with high-activity wallets.
Test Setup:
Ubuntu 22.04, CMake build
Fresh regtest environment for both branches
1,110 transactions (200 initial coinbase + 500 small sends + 100 coinbase + 300 large sends + 10 final coinbase)
Identical test patterns on both branches
Results:
Operation
Master
PR Branch
Performance Change
getwalletinfo
0.008s
0.011s
+38% slower
getbalance
0.009s
0.010s
+11% slower
listunspent
0.051s
0.035s
-31% faster
sendtoaddress
0.036s
0.037s
+3% slower
Observations:
The TXO separation optimization shows its intended benefit in listunspent (31% improvement), which makes sense given that this operation iterates through unspent outputs - exactly what the PR optimizes. Other operations show minimal performance differences within expected measurement variance.
Results suggest the optimization is working as designed for the primary use case.
DrahtBot added the label Needs rebase on Aug 16, 2025
achow101 force-pushed on Aug 16, 2025
DrahtBot removed the label Needs rebase on Aug 16, 2025
DrahtBot added the label Needs rebase on Aug 19, 2025
achow101 force-pushed on Aug 19, 2025
DrahtBot removed the label Needs rebase on Aug 19, 2025
achow101 force-pushed on Aug 19, 2025
achow101 force-pushed on Aug 20, 2025
DrahtBot added the label Needs rebase on Aug 21, 2025
achow101 force-pushed on Aug 21, 2025
DrahtBot removed the label Needs rebase on Aug 21, 2025
achow101 force-pushed on Sep 1, 2025
DrahtBot added the label CI failed on Sep 1, 2025
DrahtBot
commented at 10:58 PM on September 1, 2025:
contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/49358787700</sub>
<sub>LLM reason (✨ experimental): Lint failure: unused variable fee in wallet_listtransactions.py (F841) causing the CI to fail.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
achow101 force-pushed on Sep 2, 2025
DrahtBot removed the label CI failed on Sep 2, 2025
DrahtBot added the label Needs rebase on Sep 12, 2025
achow101 force-pushed on Sep 23, 2025
achow101 force-pushed on Sep 23, 2025
DrahtBot removed the label Needs rebase on Sep 24, 2025
rkrux
commented at 8:00 AM on October 8, 2025:
contributor
Concept ACK7b2718f
Seems like a natural step after #27286, will get to this PR soon.
DrahtBot added the label Needs rebase on Oct 31, 2025
achow101 force-pushed on Nov 10, 2025
DrahtBot removed the label Needs rebase on Nov 11, 2025
DrahtBot added the label Needs rebase on Dec 27, 2025
achow101 force-pushed on Jan 2, 2026
DrahtBot removed the label Needs rebase on Jan 2, 2026
This naming collision (m_txos in both CWallet::m_txos and CWalletTx::m_txos) can be confusing.
Since CWalletTx::m_txos stores pointers into the wallet-level maps (CWallet::m_txos and m_unusable_txos), keyed by output index (0, 1, 2, ...), a clearer naming might be:
in
src/wallet/wallet.cpp:4045
in
52b677d455outdated
4041 | // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for
4042 | - bool is_mine = IsMine(*wtx->tx) || IsFromMe(*wtx->tx);4043 | + bool mine = IsMine(*wtx->tx);4044 | + bool from_me = IsFromMe(*wtx->tx);4045 | + bool is_mine = mine || from_me;4046 | + if (is_mine) {
brunoerg
commented at 12:34 PM on January 21, 2026:
Unkilled mutant:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index c68390c82e..d0b9488e20 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4042,7 +4042,7 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
bool mine = IsMine(*wtx->tx);
bool from_me = IsFromMe(*wtx->tx);
bool is_mine = mine || from_me;
- if (is_mine) {
+ if (1==1) {
wtx->m_from_me = from_me;
// Rewrite all txs that are "mine" to ensure that any tx upgrades, including the from_me update, is written
local_wallet_batch.WriteTx(*wtx);
achow101
commented at 6:56 PM on January 21, 2026:
This isn't really testable as it is performing an upgrade that would happen on startup anyways. The if is there as an optimization to avoid rewriting transaction records that are going to be deleted later.
achow101
commented at 9:36 PM on January 29, 2026:
Spent a while looking at this and I've come to the conclusion that this entire loop is actually unnecessary, so I've removed it.
in
src/wallet/wallet.cpp:898
in
52b677d455outdated
893 | @@ -883,13 +894,14 @@ DBErrors CWallet::ReorderTransactions()
894 | continue;
895 |
896 | // Since we're changing the order, write it back
897 | - if (!batch.WriteTx(*pwtx)) 898 | - return DBErrors::LOAD_FAIL; 899 | + if (!batch.WriteTx(*pwtx)) { 900 | + return false;
brunoerg
commented at 12:36 PM on January 21, 2026:
Unkilled mutant:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index c68390c82e..3f660e2c87 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -895,7 +895,7 @@ bool CWallet::ReorderTransactions(WalletBatch& batch)
// Since we're changing the order, write it back
if (!batch.WriteTx(*pwtx)) {
- return false;
+ return true;
}
}
}
achow101
commented at 6:25 PM on January 21, 2026:
ReorderTransaction is basically impossible to test for as it requires a wallet created in 0.6.0 or earlier.
brunoerg
commented at 12:41 PM on January 21, 2026:
contributor
I ran an incremental mutation testing for src/wallet/wallet.cpp - it generated 58 mutants and the analysis showed a mutation score of 94.8% (excellent). Left some comments with the unkilled mutants, feel free to address them or ignore, in any case a feedback would be valuable.
w0xlt
commented at 11:24 PM on January 21, 2026:
contributor
I don't think that matters as m_tx_version is const.
in
src/wallet/wallet.cpp:1396
in
3fb18453cd
1392 | @@ -1394,6 +1393,7 @@ void CWallet::RecursiveUpdateTxState(WalletBatch* batch, const Txid& tx_hash, co
1393 | // If a transaction changes its tx state, that usually changes the balance
1394 | // available of the outputs it spends. So force those to be recomputed
1395 | MarkInputsDirty(wtx.tx);
1396 | + // Make the non-conflicted inputs usable again
RefreshTXOsFromTx(*wtx) creates or updates WalletTXO entries using the current wtx->m_from_me value.
During migration that value may still be the legacy-wallet value, and it is recomputed later with wtx->m_from_me = from_me, so the refresh needs to happen after that assignment.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 73bd712910..ab2902efea 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4121,9 +4121,7 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
}
}
for (const auto& [_pos, wtx] : wtxOrdered) {
- // First update the UTXOs
wtx->m_txos.clear();
- RefreshTXOsFromTx(*wtx);
// Check it is the watchonly wallet's
// solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for
bool mine = IsMine(*wtx->tx);
@@ -4131,6 +4129,7 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
bool is_mine = mine || from_me;
if (is_mine) {
wtx->m_from_me = from_me;
+ RefreshTXOsFromTx(*wtx);
// Rewrite all txs that are "mine" to ensure that any tx upgrades, including the from_me update, is written
local_wallet_batch.WriteTx(*wtx);
}
Fixed, but the issue is not reachable and the unit test is not meaningfully helpful. The in memory state after migration should never be used, I'm pretty sure there are a few other places where it is out of sync. We specifically reload the wallet in order to ensure consistency before the wallet goes back into use.
w0xlt
commented at 7:57 AM on May 23, 2026:
contributor
CWallet::RemoveTxs() deletes wallet transactions, but it was not keeping the new TXO caches (m_txos and m_unusable_txos) in sync.
Suggestion:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 73bd712910..a8faa6be39 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2522,6 +2522,7 @@ util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs
// Register callback to update the memory state only when the db txn is actually dumped to disk
batch.RegisterTxnListener({.on_commit=[&, erased_txs]() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
+ std::vector<COutPoint> spent_outpoints;
// Update the in-memory state and notify upper layers about the removals
for (const auto& it : erased_txs) {
const Txid hash{it->first};
@@ -2534,14 +2535,24 @@ util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs
break;
}
}
+ spent_outpoints.push_back(txin.prevout);
}
for (unsigned int i = 0; i < it->second.tx->vout.size(); ++i) {
- m_txos.erase(COutPoint(hash, i));
+ const COutPoint outpoint{hash, i};
+ m_txos.erase(outpoint);
+ m_unusable_txos.erase(outpoint);
}
mapWallet.erase(it);
NotifyTransactionChanged(hash, CT_DELETED);
}
+ for (const auto& outpoint : spent_outpoints) {
+ auto txo_it = m_unusable_txos.find(outpoint);
+ if (txo_it != m_unusable_txos.end() && !std::get_if<TxStateBlockConflicted>(&txo_it->second.GetState()) && !IsSpent(outpoint, /*min_depth=*/1)) {
+ MarkTXOUsable(outpoint);
+ }
+ }
+
MarkDirty();
}, .on_abort={}});
achow101
commented at 10:53 PM on May 26, 2026:
member
CWallet::RemoveTxs() deletes wallet transactions, but it was not keeping the new TXO caches (m_txos and m_unusable_txos) in sync.
I'm not convinced that MarkTXOUsable is correct or relevant. Please write a functional test that fails.
achow101 force-pushed on May 26, 2026
DrahtBot added the label CI failed on May 26, 2026
DrahtBot removed the label CI failed on May 28, 2026
sedited requested review from w0xlt on May 28, 2026
sedited removed review request from w0xlt on May 28, 2026
sedited requested review from polespinasa on May 28, 2026
sedited requested review from w0xlt on May 28, 2026
w0xlt
commented at 6:40 PM on June 2, 2026:
contributor
I'm not convinced that MarkTXOUsable is correct or relevant. Please write a functional test that fails.
Done.
The test below passes on master and on 61ba701b9e with the suggested patch, but fails on 61ba701b9e without it.
diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py
index 95e2a5b3a4..bd08819fdc 100755
--- a/test/functional/wallet_importprunedfunds.py
+++ b/test/functional/wallet_importprunedfunds.py
@@ -157,6 +157,29 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
available_utxos = [u["txid"] for u in node.listunspent(minconf=0)]
assert utxo["txid"] not in available_utxos, "UTXO should still be spent by conflicting tx"
+ self.log.info("Test removeprunedfunds restores inputs of removed confirmed spend")
+ parent_txid = node.sendtoaddress(node.getnewaddress(), 1)
+ self.generate(node, 1)
+ parent_utxo = next(
+ utxo for utxo in node.listunspent() if utxo["txid"] == parent_txid
+ )
+ parent_outpoint = (parent_txid, parent_utxo["vout"])
+
+ child_txid = node.sendall(
+ recipients=[w1.getnewaddress()], inputs=[parent_utxo]
+ )["txid"]
+ self.generate(node, 1)
+
+ def parent_is_unspent():
+ return any(
+ (utxo["txid"], utxo["vout"]) == parent_outpoint
+ for utxo in node.listunspent(minconf=0)
+ )
+
+ assert_equal(parent_is_unspent(), False)
+ node.removeprunedfunds(child_txid)
+ assert_equal(parent_is_unspent(), True)
+
if __name__ == '__main__':
ImportPrunedFundsTest(__file__).main()
achow101 force-pushed on Jun 6, 2026
DrahtBot added the label CI failed on Jun 6, 2026
DrahtBot
commented at 6:12 AM on June 6, 2026:
contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/27053283280/job/79852692010</sub>
<sub>LLM reason (✨ experimental): CI failed due to a Clang compilation error in src/test/util/__/wallet/test/util.cpp: CWallet::CreateNew is called with 6 arguments but 7 are required.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
The unconditional MarkTXOUsable(txin.prevout) works when the removed tx is the only confirmed spend, but if another conflicting wallet tx is already confirmed, the same outpoint should stay unusable.
Collecting the inputs in spent_outpoints lets us erase the removed txs from mapWallet first, then run the suggested !IsSpent(outpoint, /*min_depth=*/1) check against the remaining wallet transactions before moving anything back to m_txos.
Suggested test:
diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py
index bd08819fdc..0b2c05c0f7 100755
--- a/test/functional/wallet_importprunedfunds.py
+++ b/test/functional/wallet_importprunedfunds.py
@@ -157,6 +157,36 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
available_utxos = [u["txid"] for u in node.listunspent(minconf=0)]
assert utxo["txid"] not in available_utxos, "UTXO should still be spent by conflicting tx"
+ self.log.info("Test removeprunedfunds preserves inputs spent by another confirmed tx")
+ parent_txid = node.sendtoaddress(node.getnewaddress(), 1)
+ self.generate(node, 1)
+ parent_utxo = next(
+ utxo for utxo in node.listunspent() if utxo["txid"] == parent_txid
+ )
+ parent_outpoint = (parent_txid, parent_utxo["vout"])
+
+ child_txid = node.send(outputs=[{node.getnewaddress(): Decimal("0.5")}], inputs=[parent_utxo])["txid"]
+ child_fee = node.gettransaction(child_txid)["fee"]
+
+ # Replace child_txid with a conflicting tx and confirm it. Removing
+ # child_txid must not make parent_outpoint spendable again.
+ output_value = parent_utxo["amount"] + child_fee - Decimal("0.00001")
+ raw_conflict_tx = node.createrawtransaction(inputs=[parent_utxo], outputs=[{node.getnewaddress(): output_value}])
+ signed_conflict_tx = node.signrawtransactionwithwallet(raw_conflict_tx)
+ conflict_txid = node.sendrawtransaction(signed_conflict_tx["hex"])
+ assert_not_equal(conflict_txid, child_txid)
+ self.generate(node, 1)
+
+ def confirmed_parent_is_unspent():
+ return any(
+ (utxo["txid"], utxo["vout"]) == parent_outpoint
+ for utxo in node.listunspent(minconf=0)
+ )
+
+ assert_equal(confirmed_parent_is_unspent(), False)
+ node.removeprunedfunds(child_txid)
+ assert_equal(confirmed_parent_is_unspent(), False)
+
self.log.info("Test removeprunedfunds restores inputs of removed confirmed spend")
parent_txid = node.sendtoaddress(node.getnewaddress(), 1)
self.generate(node, 1)
test: Test removedprunedfunds makes input TXOs spendable580ff43bc5
wallet: Make SubmitTxMemoryPoolAndRelay non-conste96fff1415
wallet: Add m_from_me to cache "from me" status
m_from_me is used to track whether a transaction is "from me", i.e. has
any inputs which belong to the wallet.
ede999f412
wallet: Replace CachedTxIsFromMe with direct m_from_me lookup
Instead of looking at the cached amounts or searching every input of a
transaction each time we want to determine whether it is "from me", use
m_from_me which stores this value for us.
6525ef73df
wallet: Make CWalletTx::m_state private with {get,set}ters536e7b6c7d
achow101 force-pushed on Jul 1, 2026
achow101
commented at 10:53 PM on July 1, 2026:
member
I think the earlier RemoveTxs() suggestion (#27865 (comment)) may still be needed here.
Checking the states to revert them was starting to get a bit complicated, so I ended up doing the overkill thing of recomputing all of the TXOs after removing any transaction. That should resolve any issues here.
Suggested test:
Included the test.
Also ended up reworking how the tx states are set so that CWalletTx does not need to hold pointers to this that might disappear out from under it.
wallet: Store the TxState in each WalletTXO as well
The states will be updated whenever CWaleltTx::SetState is called too.
This is achieved by having CWalletTx::SetState take a function that
applies the new state to the specified TXO. This ensures that CWalletTx
states and WalletTXO states are kept in sync.
fbf1ec1d07
wallet: Get the depth in main chain for a TxState55bada3a16
wallet: Get the coinbase maturity state for a TXO and TxState15a02db2b6
wallet: Use WalletTXO stored state and coinbase rather than wtx3fc24ccfc5
walletdb: Move ReorderTransactions to immediately after loading txs
Perform the transaction reorder upgrade immediately after loading txs
instead of waiting for the end of loading.
ac9c453609
wallet: Store a copy of m_from_me in WalletTXOs and use for "from me"
Since we need to know whether the transaction that creates a WalletTXO
is "from me", we should store this state in the WalletTXO too, copied
from its parent CWalletTx.
fadc83697a
wallet: Have WalletTXOs also store parent tx time
WalletTXOs need to know their parent tx's timestamp for AvailableCoins
to work.
20c70c8a65
wallet: Include transaction version in WalletTXO41155ae2d7
A min_conf parameter is added to IsSpent so that it can set a
confirmation threshold for whether something is considered spent.
c2f4d77c28
wallet: Iterate block txs in reverse on blockDisconnected
When a block is disconnected, we need to process the transactions in
reverse order so that the wallet's TXO set is updated in the correct
order.
e897a586e6
wallet, tests: Have CreateSyncedWallet use CWallet::Create
CWallet::Create will properly connect the wallet to the chain, so we
should be doing that rather than ad-hoc chain connection.
86662bfc13
wallet: Move definintely unusable TXOs to a separate container
Definitely unusable TXOs are those that are spent by a confirmed
transaction or were produced by a now conflicted transaction. However,
we still need them for GetDebit, so we store them in a separate
m_unusable_txos container. MarkConflicted, AbandonTransaction, and
loading (via PruneSpentTXOs) will ensure that these unusable TXOs are
properly moved.
d588e22f3a
achow101 force-pushed on Jul 1, 2026
DrahtBot added the label CI failed on Jul 1, 2026
DrahtBot
commented at 11:12 PM on July 1, 2026:
contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/28552964176/job/84654149872</sub>
<sub>LLM reason (✨ experimental): CI failed because the build stopped on Clang -Wthread-safety-analysis errors in wallet.cpp (accessing m_txos/m_unusable_txos without holding cs_wallet, treated as -Werror).</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
w0xlt
commented at 4:01 AM on July 2, 2026:
contributor
AddToWallet()'s isInactive() branch marks the inputs of an inactive tx usable again without checking whether another confirmed tx still spends them, which lets a confirmed-spent TXO back into m_txos and trips the new Assert(!wallet.IsSpent(outpoint, 1)) in GetBalance(), aborting the node on every subsequent balance query.
Sequence:
UTXO O is spent by T1, which confirms → O moves to m_unusable_txos.
send with O as a preset input builds T2: FetchSelectedInputs resolves it via GetTXO() (which now also returns unusable TXOs) and has no IsSpent check on the preset path.
CommitTransaction calls AddToWallet(T2, TxStateInactive{})before broadcasting; the isInactive() branch runs MarkTXOUsable(O). The broadcast then fails with bad-txns-inputs-missingorspent, but the wallet state was already mutated.
} else if (wtx.isInactive()) {
- // When a transaction becomes inactive, we need to mark its inputs as usable again
+ // When a transaction becomes inactive, we need to mark its inputs as usable again,
+ // unless an input is still spent by a different confirmed transaction
for (const CTxIn& txin : wtx.tx->vin) {
- MarkTXOUsable(txin.prevout);
+ if (!IsSpent(txin.prevout, /*min_depth=*/1)) {
+ MarkTXOUsable(txin.prevout);
+ }
}
}
(Alternatively the guard could live inside MarkTXOUsable() itself, which would protect the other/future callers too.)
The test below crashes the node at getbalances on the current head and passes with the patch:
<details>
<summary>diff</summary>
#!/usr/bin/env python3
"""Test that committing a tx that spends an already confirmed-spent input
does not resurrect the spent TXO into the usable TXO set."""
from decimal import Decimal
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal
class WalletSpentTXOResurrectTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
def run_test(self):
node = self.nodes[0]
node.createwallet("resurrect")
wallet = node.get_wallet_rpc("resurrect")
default = node.get_wallet_rpc(self.default_wallet_name)
self.log.info("Fund the wallet with UTXO O")
txid = default.sendtoaddress(wallet.getnewaddress(), 1)
self.generate(node, 1)
utxo = next(u for u in wallet.listunspent() if u["txid"] == txid)
outpoint = {"txid": utxo["txid"], "vout": utxo["vout"]}
self.log.info("Spend O in T1 and confirm it -> O becomes definitely unspendable")
t1 = wallet.send(outputs=[{default.getnewaddress(): Decimal("0.9")}],
inputs=[outpoint], add_inputs=False)
assert t1["complete"]
self.generate(node, 1)
self.log.info("Try to double-spend O via a preset input")
# The broadcast fails, but the tx is committed to the wallet first.
try:
wallet.send(outputs=[{default.getnewaddress(): Decimal("0.8")}],
inputs=[outpoint], add_inputs=False)
except Exception as e:
self.log.info(f"send reported: {e}")
self.log.info("Balance queries must still work and O must remain spent")
wallet.getbalances() # aborts the node without the fix
assert_equal(utxo["txid"] in [u["txid"] for u in wallet.listunspent()], False)
if __name__ == "__main__":
WalletSpentTXOResurrectTest(__file__).main()
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-07-02 05:52 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
