wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict #29680

Eunovo · 2024-03-20T09:23:35Z

This PR implements a fix for the issue described in #29435.

The problem is that the wallet is unable to abandon transactions that have unrelated parent conflicts. The solution implemented here, augments the mempool transaction REPLACED signal with the double-spending transaction which the wallet stores and watches for in Block notifications. A map is added to the wallet to track conflicting tx ids and their child transactions. The entry is erased when the double-spending tx is removed from MemPool.

DrahtBot · 2024-03-20T09:23:38Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	josibake, ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32562 (doc: remove // for ... comments by fanquake)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

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.

DrahtBot · 2024-03-20T10:20:28Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/22873809948}

Eunovo · 2024-03-26T15:15:38Z

Draft until I've gotten some more feedback on the approach.

glozow · 2024-03-27T12:12:48Z

cc @achow101?

achow101 · 2024-03-27T17:50:56Z

I'm actually wondering now if it would be sufficient to just have the MemPoolRejectReason include the replacement txid for replacements, and the conflicting block hash for conflicts. If our tx is in the mempool, we should get these notifications when it is removed which I think would be enough without having to be looking for the parents or looking to see if we have any descendants?

Eunovo · 2024-03-28T06:24:40Z

I'm actually wondering now if it would be sufficient to just have the MemPoolRejectReason include the replacement txid for replacements, and the conflicting block hash for conflicts. If our tx is in the mempool, we should get these notifications when it is removed which I think would be enough without having to be looking for the parents or looking to see if we have any descendants?

@achow101 I think this makes sense. Once we add conflicting block hash for conflicts then we can safely mark wallet tx as conflicted which should solve the issue. What would we still need the replacement txid for?

EDIT
On second thought, I have realized that the wallet might not get the Conflict MemPoolRemovalReason for its tx. The wallet tx may be kicked out of the Mempool early due to replacement so when CtxMemPool::removeForBlock is called, the wallet tx will no longer be in the Mempool. Therefore, the issue is not completely solved by adding conflicting block hash to Conflict MemPoolRemovalReason, But adding it is still useful for handling cases where a new block is received that conflicts with wallet tx.

When you say "replacement txid", are you referring to the txid of the tx causing the wallet tx to kicked out? if so, I believe the current fix implemented here does exactly what you're describing, See 635e2e4

achow101 · 2024-03-28T17:24:46Z

When you say "replacement txid", are you referring to the txid of the tx causing the wallet tx to kicked out?

Yes, we need the replacement txid for instances where the tx is removed by replacement rather than a block conflict.

if so, I believe the current fix implemented here does exactly what you're describing, See 635e2e4

It appears to also be watching for replacements of those replacements too, and I think that is unnecessary.

Eunovo · 2024-03-28T19:44:58Z

It appears to also be watching for replacements of those replacements too, and I think that is unnecessary.

Thanks @achow101. Yes, I added that when I realized that if the replacement is also replaced then the check in the BlockConnected callback will fail to mark the wallet tx as conflicted. Unless for some reason, the replacement cannot be replaced?

Eunovo · 2024-04-09T16:52:08Z

Rebased 532d25f to 8ee9629

It appears to also be watching for replacements of those replacements too, and I think that is unnecessary.

@achow101 I took this out because the new replacement is not guaranteed to conflict with the original wallet transaction

Added conflicting_block_hash and conflicting_block_height to ConflictReason in 8b5d3d7 and
used this information to mark wallet tx has conflicted in 8ee9629.

I had to use RecursiveUpdateTxState directly in 8ee9629 because CWallet::MarkConflicted checks that the conflicting block height is not more than the m_last_block_processed by the wallet but transactionRemovedFromMempool is triggered before blockConnected so the wallet hasn't had a chance to process the block causing the conflict notification. I had to force the wallet txs update. I wonder what the repercussions for doing this are.

Curious to see what others think.

EDIT

This PR now modifies AddToWalletIfInovlingMe to do this for all TxStateBlockConflicted transactions. See e868b2d

Now marking this PR as ready for review.

DrahtBot · 2024-04-09T18:43:27Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23623413738}

Eunovo · 2024-04-21T03:43:30Z

Putting in draft while I fix falling test

Eunovo · 2024-12-01T09:49:55Z

Rebased 19f3927 to ef7b1e8

This PR implements a fix for the issue described in #29435.

The problem is that the wallet is unable to abandon transactions that have unrelated parent conflicts. The solution implemented here augments the mempool transaction REPLACED signal with the double-spending transaction which the wallet stores and watches for in Block notifications. A map is added to the wallet to track conflicting tx ids and their child transactions. The entry is erased when the double-spending tx is removed from MemPool.

#30072 introduced Mempool Changesets for packages and single transactions. Previously, I captured the replacement_tx when a tx was being kicked out of the Mempool, but with the addition of packages, any of the tx in the package could be the reason for the conflict.
I've changed the replacement_tx to be the last child tx in the package so the wallet doesn't have to watch the entire replacement package.

This solution assumes that all transactions in a package will be added to a Block. Suppose the first transaction in a 1P1C package is the actual replacement tx and the package only partially gets added to a Block. In that case, the wallet will not abandon the conflicted wallet tx because it's watching the child of the package.

DrahtBot · 2025-01-02T13:43:13Z

The CI failed once (intermittently?): https://cirrus-ci.com/task/5838005635645440?logs=ci#L2655

 node0 2024-12-31T19:11:37.979779Z [httpworker.13] [rpc/request.cpp:241] [parse] [rpc] ThreadRPCServer method=walletcreatefundedpsbt user=__cookie__ 
 test  2024-12-31T19:11:37.980000Z TestFramework (ERROR): JSONRPC error 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/test/functional/test_framework/test_framework.py", line 135, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_conflicts.py", line 41, in run_test
                                       self.test_unknown_parent_conflict()
                                     File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_conflicts.py", line 495, in test_unknown_parent_conflict
                                       child_txid = test_wallet(wallet)
                                     File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_conflicts.py", line 479, in test_wallet
                                       conflict_psbt = def_wallet.walletcreatefundedpsbt(inputs=[gp_utxo], outputs=[{def_wallet.getnewaddress(): 2}], fee_rate="{0:.8}".format(Decimal(parent_feerate*3)))["psbt"]
                                     File "/ci_container_base/test/functional/test_framework/coverage.py", line 50, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "/ci_container_base/test/functional/test_framework/authproxy.py", line 146, in __call__
                                       raise JSONRPCException(response['error'], status)
                                   test_framework.authproxy.JSONRPCException: Invalid amount (-3)

DrahtBot · 2025-01-02T13:44:31Z

Thats https://github.com/bitcoin/bitcoin/pull/29680/files#r1849679192

Maybe turn into a draft for now, while CI is failing intermittently?

Eunovo · 2025-01-05T12:12:27Z

Putting in draft while I fix failing CI

This allows the mempool to send additional data with TransactionRemovedFromMempool event. Now, we can send conflicting_block_hash and conflicting_block_height for Conflicts and replacement_tx for Replacements.

Detect replacement of wallet txs and wait for confirmation of replacement tx before marking wallet tx as conflicted

Watch for wallet transaction conflicts triggered by adding conflicting blocks

ismaelsadeeq

Concept ACK

Left some initial comments

Previously, I captured the replacement_tx when a tx was being kicked out of the Mempool, but with the addition of packages, any of the tx in the package could be the reason for the conflict.
I've changed the replacement_tx to be the last child tx in the package so the wallet doesn't have to watch the entire replacement package.

Can you elaborate on how that work, and the wallet detects that, what if the conflict is not from the child, it's from a parent transaction?

This solution assumes that all transactions in a package will be added to a Block.

This is Incorrect I think, you can have a package and not all the transaction will be added in the block.

Suppose the first transaction in a 1P1C package is the actual replacement tx and the package only partially gets added to a Block.
In that case, the wallet will not abandon the conflicted wallet tx because it's watching the child of the package.

How is this not an issue?

ismaelsadeeq · 2025-01-17T18:18:19Z

src/interfaces/chain.h

@@ -7,6 +7,7 @@



In 4c64dd5 "Change MemPoolRemovalReason to variant type"
nit: Commit message should follow the guidelines in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

The explanation lines are long, you should break it.

ismaelsadeeq · 2025-01-17T18:19:54Z

src/kernel/mempool_removal_reason.h

+    uint256 conflicting_block_hash;
+    unsigned int conflicting_block_height;
+
+    explicit ConflictReason(const uint256& conflicting_block_hash, int conflicting_block_height) : conflicting_block_hash(conflicting_block_hash), conflicting_block_height(conflicting_block_height) {}


In 4c64dd5 "Change MemPoolRemovalReason to variant type"
nit: this line is too long, please break.

ismaelsadeeq · 2025-01-17T18:22:15Z

src/wallet/wallet.cpp

+    auto* replaced_reason = std::get_if<ReplacedReason>(&reason);
+
+    // Check if wallet transaction is being replaced by a parent transaction which is not from this wallet
+    if (IsFromMe(*tx) && replaced_reason != nullptr && replaced_reason->replacement_tx.has_value() && !IsFromMe(*replaced_reason->replacement_tx.value())) {
+        m_unrelated_conflict_tx_watchlist.insert(std::make_pair(replaced_reason->replacement_tx.value()->GetHash(), tx));
+    }
+
+    auto iter = m_unrelated_conflict_tx_watchlist.find(tx->GetHash());
+    if (iter != m_unrelated_conflict_tx_watchlist.end()) {
+        // The replacement tx was removed from the mempool, remove it from map
+        // This new replacement tx may not conflict with the original tx
+        // so leave wallet tx to remain as TxStateInactive
+        m_unrelated_conflict_tx_watchlist.erase(iter);
+    }


In 71200b7 "Handle double-spending of unrelated parents to wallet txs"

Suggested change

auto* replaced_reason = std::get_if<ReplacedReason>(&reason);

// Check if wallet transaction is being replaced by a parent transaction which is not from this wallet

if (IsFromMe(*tx) && replaced_reason != nullptr && replaced_reason->replacement_tx.has_value() && !IsFromMe(*replaced_reason->replacement_tx.value())) {

m_unrelated_conflict_tx_watchlist.insert(std::make_pair(replaced_reason->replacement_tx.value()->GetHash(), tx));

}

auto iter = m_unrelated_conflict_tx_watchlist.find(tx->GetHash());

if (iter != m_unrelated_conflict_tx_watchlist.end()) {

// The replacement tx was removed from the mempool, remove it from map

// This new replacement tx may not conflict with the original tx

// so leave wallet tx to remain as TxStateInactive

m_unrelated_conflict_tx_watchlist.erase(iter);

}

auto* replaced_reason = std::get_if<ReplacedReason>(&reason);

if (IsFromMe(*tx) && replaced_reason != nullptr) {

// Check if wallet transaction is being replaced by a parent transaction which is not from this wallet

if (replaced_reason->replacement_tx.has_value() && !IsFromMe(*replaced_reason->replacement_tx.value())) {

m_unrelated_conflict_tx_watchlist.insert(std::make_pair(replaced_reason->replacement_tx.value()->GetHash(), tx));

}

// Remove the replacement tx was removed from the mempool, remove it from map

// This new replacement tx may not conflict with the original tx

// so leave wallet tx to remain as TxStateInactive

m_unrelated_conflict_tx_watchlist.erase(tx->GetHash());

}

ismaelsadeeq · 2025-01-17T18:23:25Z

src/wallet/wallet.cpp

+        // This new replacement tx may not conflict with the original tx
+        // so leave wallet tx to remain as TxStateInactive


In 71200b7 "Handle double-spending of unrelated parents to wallet txs"

How do you know that without checking ?

Eunovo · 2025-01-18T15:41:12Z

@ismaelsadeeq I wanted to get other opinions on this. With the addition of changesets, the problem is more complicated.

Can you elaborate on how that work, and the wallet detects that, what if the conflict is not from the child, it's from a parent transaction?

It does work because it's guaranteed that the parent will be confirmed before the child. The wallet can mark the replaced wallet tx as conflicted when the child of the package gets confirmed. The problem though is that the child may never be confirmed even if the parent has been confirmed.

This is Incorrect I think, you can have a package and not all the transaction will be added in the block.
How is this not an issue?

It is.

DrahtBot · 2025-05-19T21:03:42Z

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

DrahtBot · 2025-08-16T00:30:48Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot added Wallet CI failed labels Mar 20, 2024

DrahtBot mentioned this pull request Mar 20, 2024

wallet: track mempool conflicts with wallet transactions #27307

Merged

Eunovo force-pushed the fix-unknown-parent-conflict branch from abaa8ca to 4853bb5 Compare March 22, 2024 21:13

DrahtBot removed the CI failed label Mar 22, 2024

Eunovo force-pushed the fix-unknown-parent-conflict branch 2 times, most recently from ba8c831 to 532d25f Compare March 26, 2024 10:55

DrahtBot mentioned this pull request Mar 27, 2024

Cluster size 2 package rbf #28984

Merged

DrahtBot added the Needs rebase label Mar 27, 2024

Eunovo force-pushed the fix-unknown-parent-conflict branch from 532d25f to 47750d3 Compare April 9, 2024 16:35

DrahtBot removed the Needs rebase label Apr 9, 2024

Eunovo force-pushed the fix-unknown-parent-conflict branch from 47750d3 to 8ee9629 Compare April 9, 2024 18:43

DrahtBot added the CI failed label Apr 9, 2024

DrahtBot mentioned this pull request Apr 9, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Open

Eunovo force-pushed the fix-unknown-parent-conflict branch from 8ee9629 to e868b2d Compare April 10, 2024 14:27

DrahtBot removed the CI failed label Apr 10, 2024

Eunovo marked this pull request as ready for review April 10, 2024 17:37

Eunovo marked this pull request as draft April 21, 2024 03:43

Eunovo force-pushed the fix-unknown-parent-conflict branch from e868b2d to e004ecf Compare April 22, 2024 10:20

Eunovo force-pushed the fix-unknown-parent-conflict branch from 19f3927 to ef7b1e8 Compare December 1, 2024 08:59

DrahtBot removed the Needs rebase label Dec 1, 2024

DrahtBot removed the CI failed label Dec 1, 2024

Eunovo force-pushed the fix-unknown-parent-conflict branch from ef7b1e8 to 20a4038 Compare December 2, 2024 13:56

DrahtBot added CI failed and removed CI failed labels Dec 31, 2024

Eunovo marked this pull request as draft January 5, 2025 12:12

DrahtBot mentioned this pull request Jan 13, 2025

refactor: Avoid copies by using const references or by move-construction #31650

Open

Eunovo force-pushed the fix-unknown-parent-conflict branch from 20a4038 to bba3235 Compare January 15, 2025 16:00

DrahtBot added the CI failed label Jan 15, 2025

Eunovo and others added 4 commits January 15, 2025 18:22

Change MemPoolRemovalReason to variant type

4c64dd5

This allows the mempool to send additional data with TransactionRemovedFromMempool event. Now, we can send conflicting_block_hash and conflicting_block_height for Conflicts and replacement_tx for Replacements.

Handle double-spending of unrelated parents to wallet txs

71200b7

Detect replacement of wallet txs and wait for confirmation of replacement tx before marking wallet tx as conflicted

test: Conflict with unconfirmed parent not in wallet

c162984

Handle new blocks with unrelated parents conflicts

6db2961

Watch for wallet transaction conflicts triggered by adding conflicting blocks

Eunovo force-pushed the fix-unknown-parent-conflict branch from bba3235 to 6db2961 Compare January 16, 2025 09:03

ismaelsadeeq reviewed Jan 17, 2025

View reviewed changes

DrahtBot mentioned this pull request Mar 6, 2025

wallet: Ensure best block matches wallet scan state #30221

Merged

DrahtBot mentioned this pull request Apr 2, 2025

refactor: Txid type safety (parent PR) #32189

Closed

DrahtBot mentioned this pull request Apr 13, 2025

Fee Estimation via Fee rate Forecasters #30157

Draft

DrahtBot mentioned this pull request May 7, 2025

test: Add and use ElapseTime helper #32430

Open

DrahtBot mentioned this pull request May 19, 2025

doc: remove // for ... comments #32562

Merged

DrahtBot added the Needs rebase label May 19, 2025

Eunovo closed this Aug 20, 2025

		// This new replacement tx may not conflict with the original tx
		// so leave wallet tx to remain as TxStateInactive

wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict #29680

wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict #29680

Uh oh!

Conversation

Eunovo commented Mar 20, 2024

Uh oh!

DrahtBot commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Mar 20, 2024

Uh oh!

Eunovo commented Mar 26, 2024

Uh oh!

glozow commented Mar 27, 2024

Uh oh!

achow101 commented Mar 27, 2024

Uh oh!

Eunovo commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Mar 28, 2024

Uh oh!

Eunovo commented Mar 28, 2024

Uh oh!

Eunovo commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 9, 2024

Uh oh!

Eunovo commented Apr 21, 2024

Uh oh!

Eunovo commented Dec 1, 2024

Uh oh!

DrahtBot commented Jan 2, 2025

Uh oh!

DrahtBot commented Jan 2, 2025

Uh oh!

Eunovo commented Jan 5, 2025

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

Eunovo commented Jan 18, 2025

Uh oh!

DrahtBot commented May 19, 2025

Uh oh!

DrahtBot commented Aug 16, 2025

Uh oh!

Uh oh!

DrahtBot commented Mar 20, 2024 •

edited

Loading

Eunovo commented Mar 28, 2024 •

edited

Loading

Eunovo commented Apr 9, 2024 •

edited

Loading

ismaelsadeeq Jan 17, 2025 •

edited

Loading