wallet: Replace mapWallet and wtxOrdered with a boost::multi

achow101 commented at 12:33 AM on July 14, 2026: member

The wallet needs to access transactions both by txid, and by insertion order into the wallet. mapWallet was the main container for the wallet transactions, and it was keyed by txid. wtxOrdered was a secondary container which was keyed by order, and held pointers to the CWalletTx objects stored in mapWallet. These are replaced by a new boost multi index which indexes on txid and insertion order, allowing us to get rid of the 2 separate containers, and holding raw pointers to CWalletTx.

This depends on #35501 for the CWalletTx constructor changes that are necessary for this.

DrahtBot added the label Wallet on Jul 14, 2026

DrahtBot commented at 12:33 AM on July 14, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35716.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	theuni, pablomartin4btc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35975 (wallet: Fix CWalletTx malleated transaction metadata sync by achow101)
#35935 (wallet: Avoid unnecessary wtxvariant rewrites by achow101)
#35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#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)
#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#34371 (wallet: allow importprunedfunds for spending transactions by 8144225309)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

ListTransactions(*pwallet, *it, 0, true, ret, filter_label) in src/wallet/rpc/transactions.cpp
ListTransactions(wallet, tx, 0, true, transactions, filter_label, include_change) in src/wallet/rpc/transactions.cpp
ListTransactions(wallet, *wtx, -100000000, true, removed, filter_label, include_change) in src/wallet/rpc/transactions.cpp

2026-08-12 18:17:55

DrahtBot added the label CI failed on Jul 14, 2026

DrahtBot commented at 1:26 AM on July 14, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/29296252667/job/86970309393 LLM reason (✨ experimental): CI failed because the fuzz build (src/test/fuzz/spend.cpp) stopped with a Clang -Werror,-Wunused-variable error for an unused txid variable.

<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 Jul 14, 2026

theuni commented at 10:03 PM on July 14, 2026: member

Concept ACK. multi_index seems like the right thing to use here. Since this doesn't leak into consensus/node, I don't see the harm in spreading the dependency to wallet.

Getting rid of multi_index for the kernel will require something that essentially re-implements its functionality, and I don't see anything new here that would complicate that implementation.

Note that I'm biased: I'm currently working on such a re-implementation that I hope to propose for v33.

I do have a few requests to make my life easier though.

Firstly, please specify an index for each operation, as opposed to just using m_txs. What happens behind the scenes in that case is that the first index is used, which is not at all obvious to reviewers. In my rewrite I'm hoping to drop the implicit first index functionality and require an index for each call, as that greatly simplifies the implementation.

For ex, rather than m_txs.find(hash) use m_txs.get<index_by_txid>().find(hash). That also means adding the tag for the txid index, which is currently missing.

To simplify that, in wallet.h, you can do:

WalletTxs m_txs;
WalletTxs::index<index_by_txid>& m_txs_by_txid;
WalletTxs::index<index_by_pos>& m_txs_by_pos;

And in the constructor:

m_txs_by_txid(m_txs.get<index_by_txid>());
m_txs_by_pos(m_txs.get<m_txs_by_pos>());

Then everywhere you would use m_txs, use m_txs_by_txid or m_txs_by_pos instead. That makes the intent much more clear.

Here's an example of the mempool conversion to work that way (though I used indices rather than tags, which is uglier): https://github.com/theuni/bitcoin/commit/b58ab92f9d6b231967328f560701d42710e04b92 .

Secondly, please use extract/insert rather than modify. The former is compatible with the std container apis and has familiar (and much simpler) semantics wrt re-insertion failure. I was planning on removing our existing modify calls for the same reason, so it'd be nice to not be adding more in the meantime.

achow101 commented at 7:01 PM on July 17, 2026: member

Secondly, please use extract/insert rather than modify. The former is compatible with the std container apis and has familiar (and much simpler) semantics wrt re-insertion failure. I was planning on removing our existing modify calls for the same reason, so it'd be nice to not be adding more in the meantime.

Hmm, modify is nicer though since it doesn't require remembering to re-insert and I find it easier to read. I'm also running into trouble with extract and insert where it doesn't work well for looping over the entire multi index and modifying each item.

achow101 force-pushed on Jul 17, 2026

achow101 force-pushed on Jul 20, 2026

achow101 commented at 9:39 PM on July 20, 2026: member

I've changed all of the modify to extract then insert, with some changes to the functions that iterate over everything in the wallet. These mainly involved calling CWalletTx::MarkDirty. Since that function only changes non-key mutable members of CWalletTx, I made MarkDirty const so that it can be called without needing to do the extract then insert.

achow101 force-pushed on Jul 21, 2026

DrahtBot removed the label CI failed on Jul 21, 2026

DrahtBot added the label Needs rebase on Aug 4, 2026

wallet: Replace many direct mapWallet lookups with GetWalletTx b7812cd087

wallet, test: Use AddToWallet instead of direct mapWallet.emplace

When a transaction needs to be added to the wallet for testing, we can
use AddToWallet instead of direct insertion into mapWallet.

9d06fbacb8

wallet: Mark CWalletTx::MarkDirty as const

Since all of the things that MarkDirty changes are mutable, we can mark
it as a const so it can be called on const CWalletTx objects.

1437534fa6

achow101 force-pushed on Aug 5, 2026

achow101 marked this as ready for review on Aug 5, 2026

DrahtBot removed the label Needs rebase on Aug 6, 2026

in src/wallet/wallet.cpp:743 in 347ad6e75c

 738 | @@ -745,22 +739,25 @@ void CWallet::SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator> ran
 739 |      for (TxSpends::iterator it = range.first; it != range.second; ++it)
 740 |      {
 741 |          const Txid& hash = it->second;
 742 | -        CWalletTx* copyTo = &mapWallet.at(hash);
 743 | -        if (copyFrom == copyTo) continue;
 744 | +        const auto& dst_it = m_txs_by_txid.find(hash);
 745 | +        if (copyFrom->GetWitnessHash() == dst_it->GetWitnessHash()) return;

pablomartin4btc commented at 11:30 PM on August 9, 2026:

shouldn't be continue instead of return?

achow101 commented at 6:16 PM on August 12, 2026:

Fixed, leftovers from the previous approach

in src/wallet/wallet.cpp:745 in 347ad6e75c

 751 | -        copyTo->m_comment_to = copyFrom->m_comment_to;
 752 | -        copyTo->m_replaces_txid = copyFrom->m_replaces_txid;
 753 | -        copyTo->m_replaced_by_txid = copyFrom->m_replaced_by_txid;
 754 | -        copyTo->m_messages = copyFrom->m_messages;
 755 | -        copyTo->m_payment_requests = copyFrom->m_payment_requests;
 756 | +        if (!copyFrom->IsEquivalentTo(*dst_it)) return;

pablomartin4btc commented at 11:31 PM on August 9, 2026:

same as above, shouldn't be continue?

achow101 commented at 6:16 PM on August 12, 2026:

Fixed, leftovers from the previous approach

in src/wallet/wallet.cpp:2653 in 347ad6e75c

2648 | @@ -2631,9 +2649,8 @@ util::Result<CTxDestination> CWallet::GetNewChangeDestination(const OutputType t
2649 |  }
2650 |  
2651 |  void CWallet::MarkDestinationsDirty(const std::set<CTxDestination>& destinations) {
2652 | -    for (auto& entry : mapWallet) {
2653 | -        CWalletTx& wtx = entry.second;
2654 | -        if (wtx.m_is_cache_empty) continue;
2655 | +    for (const CWalletTx& wtx : m_txs_by_txid) {
2656 | +        if (wtx.m_is_cache_empty) return;

pablomartin4btc commented at 11:32 PM on August 9, 2026:

same as in SyncMetaData, shouldn't be continue?

achow101 commented at 6:17 PM on August 12, 2026:

Fixed, leftovers from the previous approach

in src/wallet/wallet.h:675 in 347ad6e75c

 673 | @@ -629,7 +674,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 674 |       * Add the transaction to the wallet, wrapping it up inside a CWalletTx
 675 |       * @return the recently added wtx pointer or nullptr if there was a db write error.

pablomartin4btc commented at 11:41 PM on August 9, 2026:

nit: doc is stale...

     * [@return](/bitcoin-bitcoin/contributor/return/) iterator to the recently added or updated wtx, or nullopt if there was a db write error.

achow101 commented at 6:17 PM on August 12, 2026:

Done

in src/wallet/wallet.cpp:2854 in 347ad6e75c

2850 | @@ -2834,18 +2851,19 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old
2851 |                  int64_t latestNow = wtx.nTimeReceived;
2852 |                  int64_t latestEntry = 0;
2853 |  
2854 | +                LOCK(cs_wallet);

pablomartin4btc commented at 11:54 PM on August 9, 2026:

I think this is redundant? The caller already holds the lock to cs_wallet... shouldn't instead define ComputeTimeSmart as EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)?

achow101 commented at 6:17 PM on August 12, 2026:

Fixed, leftovers from the previous approach

pablomartin4btc commented at 3:01 AM on August 10, 2026: member

Concept ACK

Replace two containers — mapWallet (std::unordered_map<Txid, CWalletTx>) and wtxOrdered (std::multimap<int64_t, CWalletTx*> — a secondary index with raw pointers into mapWallet) — with a single boost::multi_index_container that owns the CWalletTx objects and provides both lookups natively. Most importantly, eliminates the raw pointer indirection and the synchronisation burden of keeping two containers in sync.

Specifying an index for each operation (rather than leaving it to the implicit first index usage that could be confusing) and the extract/insert pattern (instead of modify) were both implemented as @theuni suggested, for the latter, it required making MarkDirty const (all its changes are to mutable members).

PR description nit: This PR used to be dependent on already merged #35501 for the CWalletTx constructor changes that are necessary for this.

Left a few inline nits...

wallet: Replace mapWallet with multi index m_txs

Since we need to access transactions both by txid and insertion order,
instead of having 2 separate maps, the second with raw pointers into
first, we can use a multi index.

c883fb08c2

achow101 force-pushed on Aug 12, 2026

wallet: Replace mapWallet and wtxOrdered with a boost::multi_index #35716

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)