wallet: Fix `CWalletTx` malleated transaction metadata sync #35975

achow101 commented at 8:51 PM on August 14, 2026: member

SyncMetaData is intended to handle the case of malleated transactions by copying the metadata from a presumed original transaction to all of the malleations of that transaction. However, it did not do this correctly, leading to both a crash that can be reached during bumpfee, and failing to actually copy the metadata to some malleated transactions.

The crash was reachable by having both the original transaction and a malleation of it in the wallet, then calling bumpfee on the original, and then calling bumpfee on the malleation. Calling bumpfee on the malleation would result in an assertion failure in MarkReplaced, hitting Assert(!wtx.m_replaced_by_txid);. This is reached since adding the RBF to the wallet causes a metadata sync between the original and the malleation, which copies m_replaced_by_txid. MarkReplaced is called soon afterwards, resulting in the crash. This is fixed by syncing the metadata after MarkReplaced sets replaced_by_txid during the RBF of the original transaction so that bumpfee refuses to bump the malleation in the first place as it will check m_replaced_by_txid before bumping.

The other issue is that if a RBF transaction is malleated, SyncMetaData was not copying the metadata from the original RBF transaction to the malleation.

These are fixed by changing SyncMetaData to find all of the malleations of a transaction rather than all of the conflicts and simplifying how it is called. Additionally, CWalletTx::IsEquivalentTo is used by SyncMetaData to determine whether a transaction is a malleation, and this PR pulls in #32723 (comment) to make it explicitly clear which fields it is actually checking to determine the equivalence. Lastly, SyncMetaData is renamed to SyncMalleatedTxMetadata and IsEquivalntTo renamed to IsMalleation to clarify that these functions are for handling malleated txs.

The last 2 commits of this PR adds tests for these cases, and the first commit a test for basic SyncMalleatedTxMetadata functionality that should not change here.

DrahtBot added the label Wallet on Aug 14, 2026

DrahtBot commented at 8:51 PM on August 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/35975.

Reviews

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

Type	Reviewers
ACK	jeanpablojp

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:

#35916 (fuzz: improve ipc fuzz coverage by enirox001)
#35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35569 (Encapsulation for CTransaction by purpleKarrot)

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:

# Put the malleated back into the mempol by invalidating the block -> mempool [misspelled; the intended term is the transaction pool]

2026-08-14 22:20:33

achow101 force-pushed on Aug 14, 2026

DrahtBot added the label CI failed on Aug 14, 2026

DrahtBot commented at 8:57 PM on August 14, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/31839840376/job/94894105621 LLM reason (✨ experimental): IWYU detected missing/incorrect #includes (auto-fix for src/primitives/transaction.h) and failed the CI.

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

test: Test that metadata is synced to malleated transactions dd513cc802

wallet: Clarify and restrict SyncMetaData to malleated transactions

Clarifies that SyncMetaData is supposed to copy metadata for malleated
transactions only. Furthermore, ensure that copied metadata does
actually make it to malleated transactions, rather than copying from a
conflict that is not a malleation.

c2bc5a66bd

Replace CTransaction::operator== with Equals that has options

CTransaction::operator== is only used in a few places. In a few
instances of checking transaction equality, we want to control which
fields are actually being compared, so use a custom Equals() function
which takes a EqualsOptions struct to control the checks.

As suggested in https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-3028112892

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

42b8f3b34d

wallet: Clarify IsEquivalentTo is actually checking malleation

IsEquivalentTo is used to determine whether another CWalletTx is
actually a malleation of the current tx. Rename to clarify this.

4400dcd261

achow101 force-pushed on Aug 14, 2026

wallet: Sync tx replacement metadata to malleated txs 267ce83e69

test: Test rbf metadata sync of malleated tx 452d3cbf38

test: Bumping a transaction prevents bumping malleations fb59e60865

achow101 force-pushed on Aug 14, 2026

DrahtBot removed the label CI failed on Aug 14, 2026

jeanpablojp commented at 12:23 AM on August 17, 2026: contributor

tACK fb59e6086531e5c54185984bc56a8bbed7cc1df0

Reproduced the crash on the merge base, it does abort. On the head the second bumpfee is refused.

in src/primitives/transaction.h:357 in fb59e60865

 354 |      {
 355 | -        return a.GetWitnessHash() == b.GetWitnessHash();
 356 | +        return nLockTime == other.nLockTime &&
 357 | +            version == other.version &&
 358 | +            vout == other.vout &&
 359 | +            std::ranges::equal(vin, other.vin, [&opts](const CTxIn self, const CTxIn other) {

jeanpablojp commented at 12:23 AM on August 17, 2026:

could take these by reference here (but feel free to ignore)

in src/wallet/wallet.cpp:793 in fb59e60865

 816 | +    if (sync_from) {
 817 | +        copy_from = &wtx;
 818 | +    } else {
 819 | +        // When not sync_from, we want all the malleated wallet transactions to have the same metadata as
 820 | +        // the oldest. txs is sorted by nOrderPos already, so copy from the first one.
 821 | +        copy_from = *it;

vicjuma commented at 4:22 PM on August 17, 2026:

What are the chances of actually dereferencing an empty set here? Probably checking txs.empty() might help before resorting to this

achow101 commented at 7:55 PM on August 17, 2026:

It should never happen. The provided wtx should always be in the set, and that is enforced by an Assert above.