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.

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, rkrux
Stale ACK	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:

#35998 (wallet: Handle or explicitly ignore WalletBatch write failures by achow101)
#35820 (refactor: keep duration calculations typed by l0rinc)
#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)
#34872 (wallet: fix mixed-input transaction accounting in history RPCs by w0xlt)
#34400 (wallet: parallel fast rescan (approx 8x speed up with 8 threads) by Eunovo)
#34371 (wallet: allow importprunedfunds for spending transactions by 8144225309)
#33034 (wallet: Store transactions in a separate sqlite table 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 typos and grammar issues:

// Check it is the watchonly wallet's -> // Check that it is the watchonly wallet's ... [incomplete fragment; the intended meaning isn’t fully expressed]

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-09-14 23:44:32

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

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...

achow101 force-pushed on Aug 12, 2026

in src/wallet/wallet.h:675 in c883fb08c2 outdated

 673 |      using UpdateWalletTxFn = std::function<bool(CWalletTx& wtx, bool new_tx)>;
 674 |  
 675 |      /**
 676 |       * Add the transaction to the wallet, wrapping it up inside a CWalletTx
 677 | -     * @return the recently added wtx pointer or nullptr if there was a db write error.
 678 | +     * @return iterator to the recently added wtx pointer or nullopt if there was a db write error.

pablomartin4btc commented at 4:07 AM on August 26, 2026:

nit: the AddToWallet doc comment landed slightly garbled — "iterator to the recently added wtx pointer or nullopt..." mixes "iterator" and "pointer" and drops the "or updated" that was suggested, even though the function still handles both new and existing txs...

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

achow101 commented at 6:14 PM on August 26, 2026:

If I retouch

pablomartin4btc commented at 4:45 AM on August 26, 2026: member

ACK c883fb08c293dfc2dcaf35322dfda72e946acb98

While checking ReorderTransactions() specifically (it had no direct test coverage), I found this PR incidentally fixes a real bug on master: ReorderTransactions() mutates each CWalletTx::nOrderPos in place but never touches wtxOrdered, the std::multimap used for position-ordered iteration (used by listtransactions/ listsinceblock and ComputeTimeSmart). Any tx loaded with nOrderPos == -1 (a legacy/unordered record) is keyed at -1 in wtxOrdered at load time, and since -1 sorts before every real position, it stays stuck at the front of that ordering forever — regardless of the real position ReorderTransactions() later computes. So a legacy wallet needing reorder can return transactions in the wrong order from listtransactions today. (In practice this only triggers for a wallet.dat old enough to predate order-position tracking — nOrderPos is always assigned before a normal write in every current code path — so it's a narrow, but real, latent bug.)

This PR's multi_index refactor makes that bug structurally impossible, since nOrderPos is the sort key of the ordered index itself — any mutation has to go through extract()+insert(), which re-sorts automatically.

Created a test in b27d3b9b67ae3fe2d101d567c3dc004255021af5 that reproduces this: writes 3 tx records directly to a mock DB, one of them the chronologically newest but with nOrderPos == -1 to simulate the legacy state, then loads the wallet and checks it ends up last when iterated in position order. Confirmed red/green — fails on master (order.back() returns the wrong tx), passes on this branch.

Also, left a nit...

DrahtBot requested review from theuni on Aug 26, 2026

rkrux commented at 3:19 PM on August 26, 2026: contributor

Concept ACK c883fb0

This also scratches the itch of renaming/removing the variable name mapWallet - it was not an intuitive name and didn't represent what the object contained within it.

DrahtBot added the label Needs rebase on Sep 12, 2026

wallet: Replace many direct mapWallet lookups with GetWalletTx 01306fcab7

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.

5deee3bcde

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.

cd552e1ba4

achow101 force-pushed on Sep 14, 2026

DrahtBot removed the label Needs rebase on Sep 14, 2026

DrahtBot added the label CI failed on Sep 14, 2026

DrahtBot commented at 11:02 PM on September 14, 2026: contributor

🚧 At least one of the CI tasks failed. Task previous releases: https://github.com/bitcoin/bitcoin/actions/runs/34901174478/job/104167227362 LLM reason (✨ experimental): CI failed due to a C++ build error in src/wallet/wallet.cpp where return nullptr; cannot be converted to the expected std::optional<...> type (compiler error).

<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>

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.

f31dfaeedf

achow101 force-pushed on Sep 14, 2026

DrahtBot removed the label CI failed on Sep 15, 2026

bitcoin blocked a user on Sep 22, 2026

bitcoin deleted a comment on Sep 22, 2026

DrahtBot commented at 1:02 AM on September 26, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Sep 26, 2026

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)