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	furszy, sedited
Concept ACK	rkrux
Stale 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)

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 -> # Put the malleated back into the mempool by invalidating the block [“mempol” is a misspelling of “mempool”]

2026-09-13 23:09:11

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>

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 outdated

 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)

furszy commented at 9:58 PM on September 13, 2026:

In 19d08a2e4b30ecd4f41b5b1673f925c465d1d92d:

Lambda copies CTxIn here, better to take refs:

            std::ranges::equal(vin, other.vin, [&opts](const CTxIn& self, const CTxIn& other) {

achow101 commented at 11:09 PM on September 13, 2026:

Done

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

 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.

jeanpablojp commented at 10:13 AM on August 20, 2026: contributor

I noticed that bumping the malleation instead of the original and then restarting the node loses the replaced_by_txid marker on both txs, and a second bumpfee is accepted afterwards.

MarkReplaced only persists the bumped tx, and on load the set re-syncs from the oldest one's stale record.

achow101 force-pushed on Aug 20, 2026

achow101 commented at 6:04 PM on August 20, 2026: member

I noticed that bumping the malleation instead of the original and then restarting the node loses the replaced_by_txid marker on both txs, and a second bumpfee is accepted afterwards.

Nice catch. Added to the test and a commit to fix.

jeanpablojp commented at 12:31 PM on August 21, 2026: contributor

re-ACK 740b3848e322f782b4382c641fe8be5aeb0f5580

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

Concept ACK 740b384

furszy commented at 8:37 PM on September 9, 2026: member

Nice work, the crash is annoying.

Spent some time thinking if there was a faster and friendlier way of doing this and think I found one. This is the overall branch https://github.com/furszy/bitcoin-core/commits/pr35975 . Feel free to just push the branch here if you like it.

The main change is on the second commit c2bc5a66bdf2204a7ecb259426097fc397192385, because I suffered reading it a bit and thought that could also run faster. The new one furszy/bitcoin-core@36d2f22c44b8c1eb5a8f307e2c6fb7413db80fd8 takes advantage of the fact that all variants spend wtx's first input, so a single mapTxSpends lookup finds all candidates and the per-input intersection is not needed anymore.

Other than that, with GetMalleatedVariants decoupled in the commit mentioned above, the MarkReplaced change 267ce83e697e59e7a427654c30d84f5f65bb7515 can be written simpler furszy/bitcoin-core@661f8f0a6384e8bef9ee17ce74539df3d1fcf259

Then, lastly, while was there, couldn't contain myself from simplifying 8852df11e99b7eec033db7f6b21c552a209de12c to furszy/bitcoin-core@5149588e220ad06d29a1e237d7da4b08ed0b7671 and furszy/bitcoin-core@45031e5b4fc52770b159b83a3b6784770f6732cc .

achow101 force-pushed on Sep 9, 2026

achow101 commented at 9:10 PM on September 9, 2026: member

Taken the suggestions

achow101 force-pushed on Sep 9, 2026

DrahtBot added the label CI failed on Sep 9, 2026

achow101 force-pushed on Sep 9, 2026

DrahtBot removed the label CI failed on Sep 9, 2026

furszy commented at 3:13 PM on September 10, 2026: member

Some extra coverage to squash in the last commit:

diff --git a/test/functional/wallet_txn_clone.py b/test/functional/wallet_txn_clone.py
--- a/test/functional/wallet_txn_clone.py	(revision 1b1e5f3b3621d8d4e7ec2d5cd4a382c4a5be165b)
+++ b/test/functional/wallet_txn_clone.py	(date 1789053048451)
@@ -171,21 +171,28 @@
         wallet = self.nodes[0].get_wallet_rpc("metadata_clone")
         def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
 
-        # Make non-segwit UTXOs that can be malleated
-        for _ in range(5):
-            def_wallet.sendtoaddress(wallet.getnewaddress(address_type="legacy"), 1)
+        # Make non-segwit UTXOs that can be malleated. Smaller than the spending amount
+        # to create multiple inputs.
+        for _ in range(6):
+            def_wallet.sendtoaddress(wallet.getnewaddress(address_type="legacy"), 0.5)
 
         self.generate(self.nodes[0], 1)
 
         # Bumping either should prevent the other from being bumped as well
         for bump_malleated in [False, True]:
             original_txid = wallet.sendtoaddress(def_wallet.getnewaddress(), 0.9, comment="testing", fee_rate=1)
             malleated_tx, malleated_txid = self.malleate_tx(wallet, original_txid)
 
             blockhash = self.generateblock(self.nodes[0], def_wallet.getnewaddress(), [malleated_tx])["hash"]
 
             assert_equal(wallet.gettransaction(malleated_txid)["comment"], "testing")
 
+            # Check synced comment was written to disk
+            wallet.unloadwallet()
+            self.nodes[0].loadwallet("metadata_clone")
+            assert_equal(wallet.gettransaction(malleated_txid)["comment"], "testing")
+
             # Put the malleated back into the mempol by invalidating the block
             self.nodes[0].invalidateblock(blockhash)
 
@@ -232,6 +239,13 @@
 
         self.generateblock(self.nodes[0], def_wallet.getnewaddress(), [malleated_tx])
 
+        txinfo = wallet.gettransaction(malleated_txid)
+        assert_equal(txinfo["comment"], "testing")
+        assert_equal(txinfo["replaces_txid"], orig_txid)
+
+        # Synced metadata must survive a reload
+        wallet.unloadwallet()
+        self.nodes[0].loadwallet("rbf_metadata_clone")
         txinfo = wallet.gettransaction(malleated_txid)
         assert_equal(txinfo["comment"], "testing")
         assert_equal(txinfo["replaces_txid"], orig_txid)

furszy commented at 3:14 PM on September 10, 2026: member

ACK 1b1e5f3b3621d8d4e7ec2d5cd4a382c4a5be165b

Can quickly re-ack if you squash the coverage shared above.

DrahtBot requested review from rkrux on Sep 10, 2026

DrahtBot requested review from jeanpablojp on Sep 10, 2026

achow101 force-pushed on Sep 10, 2026

achow101 commented at 6:17 PM on September 10, 2026: member

Took the suggestion

furszy commented at 6:45 PM on September 10, 2026: member

ACK c3298ba67c191424ee5c4e83558b3d9a14c49a51

sedited approved

sedited commented at 10:44 AM on September 12, 2026: contributor

utACK c3298ba67c191424ee5c4e83558b3d9a14c49a51

Could run clang-format, but I don't think that needs to hold up merging this.

sedited commented at 10:51 AM on September 12, 2026: contributor

Looks like there is a silent merge conflict after merging #35935:

[ 60%] Built target bitcoin-tx
/home/drgrid/bitcoin/src/wallet/wallet.cpp: In member function ‘void wallet::CWallet::SyncMalleatedTxMetadata(wallet::WalletBatch&, const wallet::CWalletTx&)’:
/home/drgrid/bitcoin/src/wallet/wallet.cpp:785:21: error: ‘class wallet::WalletBatch’ has no member named ‘WriteTx’; did you mean ‘WriteIC’?
  785 |         (void)batch.WriteTx(*copyTo);
      |                     ^~~~~~~
      |                     WriteIC
/home/drgrid/bitcoin/src/wallet/wallet.cpp: In member function ‘bool wallet::CWallet::MarkReplaced(const Txid&, const Txid&)’:
/home/drgrid/bitcoin/src/wallet/wallet.cpp:1035:20: error: ‘class wallet::WalletBatch’ has no member named ‘WriteTx’; did you mean ‘WriteIC’?
 1035 |         if (!batch.WriteTx(*variant)) {
      |                    ^~~~~~~
      |                    WriteIC
gmake[2]: *** [src/wallet/CMakeFiles/bitcoin_wallet.dir/build.make:457: src/wallet/CMakeFiles/bitcoin_wallet.dir/wallet.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:2610: src/wallet/CMakeFiles/bitcoin_wallet.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....
[ 60%] Built target bitcoin-cli
[ 60%] Linking CXX static library ../../../../lib/libtest_fuzz.a
[ 60%] Built target test_fuzz
gmake: *** [Makefile:146: all] Error 2
ERROR: Running '/home/drgrid/.pre-merge-build.sh' failed.

test: Test that metadata is synced to malleated transactions fc718ade4f

wallet: simplify and restrict SyncMetaData to malleated txs

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.

Code should now be friendlier to read/maintain and faster as well.

2efaa6763b

achow101 force-pushed on Sep 12, 2026

achow101 commented at 8:13 PM on September 12, 2026: member

Looks like there is a silent merge conflict after merging #35935:

Rebased and fixed

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>

b973a355c4

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.

34533d5d22

wallet: sync tx replacement metadata to malleated txs 6c16d76f79

wallet: simplify wtx metadata sync 31eedfc6fc

wallet: persist synced metadata and do not sync on load

Because we update variants during insertion, we no longer
need to sync them during load.

752fd437c7

test: Test rbf metadata sync of malleated tx a44f9ad350

test: Bumping a transaction prevents bumping malleations 2c6047df87

achow101 force-pushed on Sep 13, 2026

in src/rpc/rawtransaction.cpp:392 in b973a355c4

 388 | @@ -389,7 +389,7 @@ static RPCMethod getrawtransaction()
 389 |      }
 390 |  
 391 |      CTxUndo* undoTX {nullptr};
 392 | -    auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return *t == *tx; });

furszy commented at 11:48 PM on September 13, 2026:

nano nit: this could really be a hash comparison as getrawtransaction receives a tx id?

    auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return t->GetHash() == tx->GetHash(); });

furszy commented at 11:54 PM on September 13, 2026: member

utACK 2c6047df8746a1d5333d17ad999dd32c40aba90e

DrahtBot requested review from sedited on Sep 13, 2026

sedited approved

sedited commented at 7:57 AM on September 14, 2026: contributor

utACK 2c6047df8746a1d5333d17ad999dd32c40aba90e

sedited merged this on Sep 14, 2026

sedited closed this on Sep 14, 2026

Kino1994 referenced this in commit 1f530aa488 on Sep 20, 2026

josibake commented at 10:57 AM on September 21, 2026: member

Post merge NACK on https://github.com/bitcoin/bitcoin/commit/b973a355c4ebd94cfa8cef61b68ac6da92fad787

I find it concerning that https://github.com/bitcoin/bitcoin/commit/b973a355c4ebd94cfa8cef61b68ac6da92fad787 was merged, where the motivation seems to be a bug fix for the wallet. It also seems to undo the original intent of the PR it references (https://github.com/bitcoin/bitcoin/pull/32723), namely to enforce that equality actually means equality.

I haven't fully read the justification for why this PR went with "custom equality," but I can almost guarantee we could have come up with a better solution that did not involve changing CTransaction with something that, in my strong opinion, severely worsens the class with the a massive footgun in consensus critical code.

I could be wrong and missing something really obvious here, but at the very least its clear to me that this did not receive sufficient review for merging a change to the equality operator of CTransaction. Is it possible to revert commit and explore other solutions for what the wallet needs here?

sedited commented at 11:43 AM on September 21, 2026: contributor

I don't think this needs to be reverted, but the introduced member function could be made a free function and placed in wallet/transaction to keep the primitive small and focused on consensus code. The PR implements maflcko's suggestion from #32723 (https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-3028112892), so not sure about it undoing intent, or introducing a footgun.

josibake commented at 12:13 PM on September 21, 2026: member

The PR implements maflcko's suggestion

I looked at that PR, and I don't see any discussion on the suggestion for or against it. As I mentioned above, I do think this introduces a material footgun and severely degrades local reasoning in an area where it matters most. I think the author of the referenced PR was right not to take the suggestion. I could be missing something but I do think this needs to be its own PR with its own discussion, with a better justification than "maflcko suggested it."

Perhaps its better to flip this on its head: does this change make CTransaction better, in all use cases? If this is actually needed, it seems worth discussing in its own PR.

furszy commented at 3:30 PM on September 21, 2026: member

Perhaps its better to flip this on its head: does this change make CTransaction better, in all use cases? If this is actually needed, it seems worth discussing in its own PR.

I understand @josibake concern, and agree with @sedited. Instead of making this a discussion about the CTransaction primitive, this function could easily be moved to a utility file or the wallet module (with a different name if needed). As b973a355c4ebd94cfa8cef61b68ac6da92fad787 shows, the function use cases are pretty narrow. And comparing the fields directly has a clear advantage over the previous approach for the wallet.

I could be missing something

The previous code was misleading. We were comparing the cached wtxid, which is set to the txid during construction when there is no witness data. This isn't obvious when looking at the equality operator, which also could be labeled as a footgun. If we move this function into util/wallet, I would be in favor of dropping it rather than re-adding the old CTransaction equality operator.

josibake commented at 3:55 PM on September 21, 2026: member

We were comparing the cached wtxid, which is set to the txid during construction when there is no witness data

This feels off to me, not the equality operator. wtxid is a derived field, it shouldn't be possible to set it when the fields it hashes are missing. Regardless, this sounds like there are two specific things going on: a wallet specific use case that wants a specific comparison utility, and an equality operator on CTransaction that needs revisiting/removing. I think these should be separate discussions and/or PRs.

EDIT: I worded that poorly, a wtxidis set to a txid to indicate that a transaction is not a witness transaction. What I meant is it feels strange to set the wtxid to txid on a transaction that is meant to be a witness transaction. Either its a legacy transaction or its a witness transaction, and it can't really be a witness transaction until the necessary inputs are there and wtxid can be definitionally derived.

furszy commented at 7:52 PM on September 21, 2026: member

it feels we agree on both subjects?, just need someone doing the changes. Feel free to go ahead with them and tag me there. Happy to review them.