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

Eunovo commented at 9:23 am on March 20, 2024: none

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 commented at 9:23 am on March 20, 2024: 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/29680.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	josibake

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:

#31122 (cluster mempool: Implement changeset interface for mempool by sdaftuar)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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 added the label Wallet on Mar 20, 2024

DrahtBot added the label CI failed on Mar 20, 2024

DrahtBot commented at 10:20 am on March 20, 2024: contributor

🚧 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 force-pushed on Mar 22, 2024

DrahtBot removed the label CI failed on Mar 22, 2024

Eunovo force-pushed on Mar 25, 2024

Eunovo force-pushed on Mar 26, 2024

Eunovo commented at 3:15 pm on March 26, 2024: none

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

glozow commented at 12:12 pm on March 27, 2024: member

cc @achow101?

DrahtBot added the label Needs rebase on Mar 27, 2024

achow101 commented at 5:50 pm on March 27, 2024: member

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 commented at 6:24 am on March 28, 2024: none

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 https://github.com/bitcoin/bitcoin/pull/29680/commits/635e2e47908cbc443604e388b5a823958e5b5e30

achow101 commented at 5:24 pm on March 28, 2024: member

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 https://github.com/bitcoin/bitcoin/commit/635e2e47908cbc443604e388b5a823958e5b5e30

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

Eunovo commented at 7:44 pm on March 28, 2024: none

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 force-pushed on Apr 9, 2024

Eunovo commented at 4:52 pm on April 9, 2024: none

Rebased https://github.com/bitcoin/bitcoin/commit/532d25fb536da953db2b7a6ce7405a4b105c1a56 to https://github.com/bitcoin/bitcoin/pull/29680/commits/8ee9629b51eed250f7b5a8a6644b40ca725f3990

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 https://github.com/bitcoin/bitcoin/pull/29680/commits/8b5d3d72fa54b9e35507739f53d158f5f46f05ac and used this information to mark wallet tx has conflicted in https://github.com/bitcoin/bitcoin/pull/29680/commits/8ee9629b51eed250f7b5a8a6644b40ca725f3990.

I had to use RecursiveUpdateTxState directly in https://github.com/bitcoin/bitcoin/pull/29680/commits/8ee9629b51eed250f7b5a8a6644b40ca725f3990 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 https://github.com/bitcoin/bitcoin/pull/29680/commits/e868b2d92dd040930ea18a3dcebb81468dc711b9

Now marking this PR as ready for review.

DrahtBot removed the label Needs rebase on Apr 9, 2024

Eunovo force-pushed on Apr 9, 2024

DrahtBot added the label CI failed on Apr 9, 2024

DrahtBot commented at 6:43 pm on April 9, 2024: contributor

🚧 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 force-pushed on Apr 10, 2024

DrahtBot removed the label CI failed on Apr 10, 2024

Eunovo marked this as ready for review on Apr 10, 2024

Eunovo commented at 3:43 am on April 21, 2024: none

Putting in draft while I fix falling test

Eunovo marked this as a draft on Apr 21, 2024

Eunovo force-pushed on Apr 22, 2024

DrahtBot added the label CI failed on Apr 22, 2024

Eunovo force-pushed on Apr 23, 2024

DrahtBot removed the label CI failed on Apr 23, 2024

Eunovo marked this as ready for review on Apr 23, 2024

DrahtBot added the label CI failed on May 4, 2024

Eunovo force-pushed on May 7, 2024

Eunovo force-pushed on May 9, 2024

DrahtBot removed the label CI failed on May 11, 2024

josibake commented at 1:05 pm on May 13, 2024: member

Concept ACK

Seems like a reasonable approach to the problem described in #29435 , will dig in more. Using a variant as a replacement for the Enum seems a bit odd at first glance and requires a lot of duplicate code for each struct. Perhaps another approach could be a class for RemovalReason, which contains the Enum as a field and then uses a std::variant to hold the data needed by the wallet, if any. Would be more extensible, and then the to string method could be defined once on the class. Special logic per reason can also be handled all in the same place by switching on the Enum value and taking appropriate action per reason.

EDIT: this approach might also make it easier to do the first commit as a scripted-diff

Eunovo commented at 2:25 pm on May 13, 2024: none

Perhaps another approach could be a class for RemovalReason, which contains the Enum as a field and then uses a std::variant to hold the data needed by the wallet, if any. Would be more extensible, and then the to string method could be defined once on the class. Special logic per reason can also be handled all in the same place by switching on the Enum value and taking appropriate action per reason.

Thanks for the review @josibake. Will this approach still require the definition of structs to hold each data for each reason?

josibake commented at 2:39 pm on May 13, 2024: member

Will this approach still require the definition of structs to hold each data for each reason?

I don’t think so: only the Conflict and Replace reasons need extra data, and from what I understand it can only be one reason at any given time. Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, std::variant<CTransactionRef, BlockData>, where block data holds the block hash and block number.

Eunovo commented at 3:11 pm on May 13, 2024: none

I don’t think so: only the Conflict and Replace reasons need extra data, and from what I understand it can only be one reason at any given time. Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, std::variant<CTransactionRef, BlockData>, where block data holds the block hash and block number. @josibake Makes sense but I’m skeptical about how this affects the removal reason usage. For example, you can just define SizeLimitReason() right now, but with this approach, you’d have to do RemovalReason(RemovalReasons::SIZE_LIMIT) and this new approach still requires the use of std::variant, so, I’m worried that the current code in https://github.com/bitcoin/bitcoin/pull/29680/commits/59ee08e43255bf0e96d1fab8eb243a4b8b7864c2 will still be more clear. WDYT?

josibake commented at 3:54 pm on May 13, 2024: member

The benefits I see are we can keep the existing Enum and code for converting the Enum to a string. This would also allow updating the function signatures with a scripted-diff, which makes larger refactors like this easier to verify as a reviewer.

It also feels like a more natural fit to me and my gut feeling is that having a class to encapsulate the removal reasons and necessary data will be more maintainable and extensible going forward.

Eunovo commented at 6:47 pm on May 13, 2024: none

The benefits I see are we can keep the existing Enum and code for converting the Enum to a string. This would also allow updating the function signatures with a scripted-diff, which makes larger refactors like this easier to verify as a reviewer.

It also feels like a more natural fit to me and my gut feeling is that having a class to encapsulate the removal reasons and necessary data will be more maintainable and extensible going forward. @josibake Makes sense. I’ll create a RemovalReason class

ryanofsky commented at 8:10 pm on May 13, 2024: contributor

Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, std::variant<CTransactionRef, BlockData>, where block data holds the block hash and block number.

The drawback of a separate data field is that there is no longer a guarantee that data accompanying the reason is always present when the reason is set, and always absent when it is not set. So it is possible for code to only partially initialize the RemovalReason class or initialize it in an inconsistent state.

In general I like the idea of replacing enums with variants for more safety, and to be able to express rules about state in type definitions. But variants in c++ are more awkward than sum types in other languages, and I did not look into this specific case, so maybe the tradeoffs in this case are not worth it.

josibake commented at 8:21 am on May 14, 2024: member

The drawback of a separate data field is that there is no longer a guarantee that data accompanying the reason is always present when the reason is set, and always absent when it is not set. So it is possible for code to only partially initialize the RemovalReason class or initialize it in an inconsistent state.

Seems easily addressed with a constructor, no? Something like:

 0class RemovedReason {
 1public:
 2    MemPoolRemovalReason m_reason;
 3    std::variant<std::monostate, CTxReference, BlockData> m_extra_data;
 4
 5    // Constructor for reasons that don't require extra data
 6    RemovedReason(MemPoolRemovalReason r) : reason(r) {
 7        if (requiresExtraData(r)) {
 8            throw std::invalid_argument("reason X requires data Y etc");
 9        }
10    }
11
12    // Constructor needing CTxRef
13    RemovedReason(RemovalReason r, const CTxRef& data) : reason(r), extra_data(data) {
14        if (!IsA(r)) {
15            throw std::invalid_argument("CTxRef is required for reason A, got bla");
16        }
17    }
18
19    // Constructor needing BlockData
20    RemovedReason(RemovalReason r, const BlockData& data) : reason(r), extra_data(data) {
21        if (!IsB(r)) {
22            throw std::invalid_argument("BlockData is required for reason B, got bla");
23        }
24    }

Maybe this is starting to be more complicated than just having a struct per each reason? But I’d still argue this is a better approach in that it keeps all the logic for mempool removal reasons in one place and avoids duplicating code on each struct.

Eunovo commented at 2:26 pm on May 14, 2024: none

Maybe this is starting to be more complicated than just having a struct per each reason? But I’d still argue this is a better approach in that it keeps all the logic for mempool removal reasons in one place and avoids duplicating code on each struct.

Same thing I was thinking. Using the class right now looks like it will make things more complicated. Maybe we should leave the class for a future change where it becomes necessary? @josibake @ryanofsky

ryanofsky commented at 2:45 pm on May 14, 2024: contributor

Seems easily addressed with a constructor, no? Something like:

Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition. Depending on the constructors it may also only provide runtime checking rather than compile-time checking like in your example. And if the struct is mutable could allow invalid representations of state after construction.

I don’t know what is best in this particular case, I would just stand up for:

0struct MyState1 { int data; };
1struct MyState2 { bool flag; };
2struct MyState3 {};
3using MyState = std::variant<MyState1, MyState2, MyState3>;

as a good alternative to:

0enum class MyState {
1  STATE1,
2  STATE2,
3  STATE3,
4};
5
6class MyData {
7  MyState m_state,
8  // ... more data and methods...
9};

in many cases.

Maybe we should leave the class for a future change where it becomes necessary?

I’m not sure the answer to this, but it is probably worth experimenting and choosing the approach that seems simplest.

josibake commented at 2:57 pm on May 14, 2024: member

Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition

Fair point, in that bugs could be introduced by someone not writing these pre-checks correctly / efficiently. I’ll admit I’m not fully convinced that the struct per state approach isn’t going to be harder to maintain / extend in the future, but given that I haven’t convinced you guys on that point and you have convinced me there is an advantage per the struct per state approach, I’ll retract my suggestion we change it :smile:

DrahtBot added the label Needs rebase on May 15, 2024

Eunovo force-pushed on May 20, 2024

DrahtBot removed the label Needs rebase on May 20, 2024

DrahtBot added the label CI failed on May 20, 2024

in test/functional/wallet_conflicts.py:486 in 7da8f981eb outdated

481+
482+            assert_equal(def_wallet.gettransaction(parent_txid)["confirmations"], 0)
483+            assert_equal(current_wallet.gettransaction(child_txid)["confirmations"], 0)
484+
485+            # Make a conflict spending parent
486+            conflict_psbt = def_wallet.walletcreatefundedpsbt(inputs=[gp_utxo], outputs=[{def_wallet.getnewaddress(): 2}], fee_rate=parent_feerate*3)["psbt"]

DrahtBot commented at 3:57 pm on May 20, 2024:

 0 node0 2024-05-20T11:47:13.485435Z [httpworker.2] [rpc/request.cpp:222] [parse] [rpc] ThreadRPCServer method=walletcreatefundedpsbt user=__cookie__ 
 1 test  2024-05-20T11:47:13.486000Z TestFramework (ERROR): JSONRPC error 
 2                                   Traceback (most recent call last):
 3                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
 4                                       self.run_test()
 5                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_conflicts.py", line 41, in run_test
 6                                       self.test_unknown_parent_conflict()
 7                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_conflicts.py", line 486, in test_unknown_parent_conflict
 8                                       conflict_psbt = def_wallet.walletcreatefundedpsbt(inputs=[gp_utxo], outputs=[{def_wallet.getnewaddress(): 2}], fee_rate=parent_feerate*3)["psbt"]
 9                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
11                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
12                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
14                                       raise JSONRPCException(response['error'], status)
15                                   test_framework.authproxy.JSONRPCException: Invalid amount (-3)

https://cirrus-ci.com/task/5869965947961344?logs=ci#L3151

Eunovo force-pushed on May 21, 2024

DrahtBot removed the label CI failed on May 22, 2024

in src/kernel/mempool_removal_reason.h:14 in 7debac0b09 outdated

10+
11 #include <string>
12+#include <variant>
13 
14-/** Reason why a transaction was removed from the mempool,
15+/** Reasons why a transaction was removed from the mempool,

josibake commented at 8:46 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

nit: I think it’s more correct to leave this as “Reason.” “Reasons” implies a single transaction can have multiple reasons for being removed at the same time.

in src/validationinterface.cpp:22 in 7debac0b09 outdated

18@@ -19,7 +19,7 @@
19 #include <unordered_map>
20 #include <utility>
21 
22-std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;
23+// std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;

josibake commented at 8:54 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

Can remove this line.

in src/validationinterface.h:30 in 7debac0b09 outdated

25@@ -25,7 +26,8 @@ class BlockValidationState;
26 class CBlock;
27 class CBlockIndex;
28 struct CBlockLocator;
29-enum class MemPoolRemovalReason;
30+class CValidationInterface;
31+class CScheduler;

josibake commented at 8:56 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

AFAICT, these are unused (I was able to compile this commit fine without them). Maybe leftover from a different approach?

Eunovo commented at 2:54 pm on May 23, 2024:

Same here. I’ll take them out

in src/wallet/wallet.h:59 in 7debac0b09 outdated

55@@ -56,7 +56,6 @@ class CKeyID;
56 class CPubKey;
57 class Coin;
58 class SigningProvider;
59-enum class MemPoolRemovalReason;

josibake commented at 9:05 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

Don’t we also need to #include kernel/mempool_removal_reasons.h , for Include What You Use (IWYU)? I’ll admit, I’m totally sure what our conventions are on this. cc @TheCharlatan or @maflcko

Eunovo commented at 11:22 pm on June 3, 2024:

I followed IWYU here and added kernel/mempool_removal_reason.h to wallet.h

in src/validation.cpp:3119 in 7debac0b09 outdated

3056@@ -3057,7 +3057,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
3057              Ticks<MillisecondsDouble>(time_chainstate) / num_blocks_total);
3058     // Remove conflicting transactions from the mempool.;
3059     if (m_mempool) {
3060-        m_mempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight);
3061+        m_mempool->removeForBlock(blockConnecting.vtx, pthisBlock->GetHash(), pindexNew->nHeight);

josibake commented at 9:26 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

Was a bit surprised that pthisBlock->GetHash() isn’t returning a member variable and instead is calculating the hash each time its called. Worth mentioning we are adding an extra hash to ConnectTip. Probably negligible but wanted to mention it.

Eunovo commented at 2:42 pm on May 23, 2024:

@josibake I looked around a bit and there might be something here. GetHash() is called before this point

when we read the block from disk https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/node/blockstorage.cpp#L1092
when ActivateBestChain is called https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/validation.cpp#L3354

There might be some gain in caching GetHash() but I think that has to be addressed on its own. I’ll have to measure the runtimes and see its worth a PR.

I did also discover that pindexNew which is a CBlockIndex does store the blockhash, see https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/chain.h#L244 Looks like I can use that here to prevent recalculating the blockhash.

furszy commented at 0:13 am on June 3, 2024:

Can use pindexNew hash. We cache the block hash there.

in src/test/mempool_tests.cpp:19 in 7debac0b09 outdated

15@@ -15,7 +16,7 @@
16 
17 BOOST_FIXTURE_TEST_SUITE(mempool_tests, TestingSetup)
18 
19-static constexpr auto REMOVAL_REASON_DUMMY = MemPoolRemovalReason::REPLACED;
20+static const auto REMOVAL_REASON_DUMMY = ReplacedReason(nullptr);

josibake commented at 9:37 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

I think it would be better to pass a dummy CTxRef here instead of nullptr.

in src/kernel/mempool_removal_reason.h:57 in 7debac0b09 outdated

58+};
59+
60+struct ReplacedReason {
61+    CTransactionRef replacement_tx;
62+
63+    explicit ReplacedReason(const CTransactionRef replacement_tx) : replacement_tx(replacement_tx) {}

josibake commented at 9:46 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

Any reason why conflicting_block_hash is passed by reference but replacement_tx isn’t?

Would also be nice if we prevented these objects from be constructed with nullptrs. I’m not sure if we have a convention in our codebase around this or other examples to point to, but would be worth looking into.

Eunovo commented at 2:53 pm on May 23, 2024:

Any reason why conflicting_block_hash is passed by reference but replacement_tx isn’t?

CTransactionRef is already a pointer https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/primitives/transaction.h#L423-L424

Eunovo commented at 3:03 pm on May 23, 2024:

Would also be nice if we prevented these objects from be constructed with nullptrs. I’m not sure if we have a convention in our codebase around this or other examples to point to, but would be worth looking into.

It looks like I may be able to do this by deleting the constructor

0// Deleted constructor
1ReplacedReason(std::nullptr_t) = delete;

I’ll test it

Eunovo commented at 11:23 pm on June 3, 2024:

@josibake Deleting the nullptr constructor worked.

in src/kernel/mempool_removal_reason.h:44 in 7debac0b09 outdated

47+    }
48 };
49 
50+struct ConflictReason {
51+    uint256 conflicting_block_hash;
52+    unsigned int conflicting_block_height;

josibake commented at 10:01 am on May 22, 2024:

in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 (“Change MemPoolRemovalReason to variant type”):

I haven’t finished reviewing the later commits yet, but seems odd to pass both conflicting_block_hash and conflicting_block_height. Seems like we should be able to only use conflicting_block_hash?

Eunovo commented at 2:45 pm on May 23, 2024:

The TxStateBlockConflicted requires both that’s why I added the both of them

josibake commented at 10:02 am on May 22, 2024: member

Still reviewing the later commits, but had some initial feedback/questions for the first commit.

DrahtBot added the label Needs rebase on May 24, 2024

Eunovo force-pushed on Jun 2, 2024

DrahtBot removed the label Needs rebase on Jun 3, 2024

in src/wallet/wallet.cpp:1451 in b67c82a6f2 outdated

1448         RefreshMempoolStatus(it->second, chain());
1449     }
1450     // Handle transactions that were removed from the mempool because they
1451     // conflict with transactions in a newly connected block.
1452-    if (reason == MemPoolRemovalReason::CONFLICT) {
1453+    if (std::get_if<ConflictReason>(&reason)) {

furszy commented at 0:19 am on June 3, 2024:

Could replace all the std::get_if for a more readable IsReason<Conflict>(&reason) if you include this function:

0template <typename T>
1bool IsReason(const MemPoolRemovalReason& reason)
2{
3    return std::get_if<T>(reason) != nullptr;
4}

Eunovo commented at 5:22 pm on June 3, 2024:

Thanks @furszy

in src/validation.cpp:1239 in b67c82a6f2 outdated

1235@@ -1236,7 +1236,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
1236         );
1237         m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx());
1238     }
1239-    m_pool.RemoveStaged(m_subpackage.m_all_conflicts, false, MemPoolRemovalReason::REPLACED);
1240+    m_pool.RemoveStaged(m_subpackage.m_all_conflicting, false, ReplacedReason(entry->GetSharedTx()));

furszy commented at 0:26 am on June 3, 2024:

In b67c82a6f2ab6:

This commit does not compile, shouldn’t have changed m_all_conflicts for m_all_conflicting.

Eunovo force-pushed on Jun 3, 2024

Eunovo commented at 11:24 pm on June 3, 2024: none

Thanks for the reviews @josibake and @furszy I have implemented your suggested changes

DrahtBot added the label CI failed on Jun 4, 2024

DrahtBot commented at 0:41 am on June 4, 2024: contributor

🚧 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/25760658229

Eunovo marked this as a draft on Jun 4, 2024

Eunovo force-pushed on Jun 5, 2024

DrahtBot removed the label CI failed on Jun 5, 2024

Eunovo marked this as ready for review on Jun 6, 2024

DrahtBot added the label Needs rebase on Jul 2, 2024

Eunovo force-pushed on Jul 3, 2024

DrahtBot removed the label Needs rebase on Jul 3, 2024

Eunovo requested review from furszy on Jul 8, 2024

achow101 requested review from achow101 on Oct 15, 2024

DrahtBot added the label CI failed on Oct 20, 2024

DrahtBot removed the label CI failed on Oct 24, 2024

in src/wallet/wallet.cpp:1476 in 7eba56accb outdated

1463@@ -1452,6 +1464,24 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, const Mem
1464     if (it != mapWallet.end()) {
1465         RefreshMempoolStatus(it->second, chain());
1466     }
1467+
1468+    auto* replaced_reason = std::get_if<ReplacedReason>(&reason);
1469+
1470+    // Check if wallet transaction is being replaced by an unrelated parent transaction
1471+    if (replaced_reason != nullptr && IsFromMe(*tx) && !IsFromMe(*(replaced_reason->replacement_tx))) {

achow101 commented at 10:03 pm on November 4, 2024:

In 7eba56accbd1b4ce3c09656c8d69b5aa437a706f “Handle double-spending of unrelated parents to wallet txs”

The replacement could still be sending to us, and in that case, it isn’t unrelated.

Eunovo commented at 2:04 pm on November 5, 2024:

Updated comment in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5

in src/wallet/wallet.cpp:1475 in 7eba56accb outdated

1470+    // Check if wallet transaction is being replaced by an unrelated parent transaction
1471+    if (replaced_reason != nullptr && IsFromMe(*tx) && !IsFromMe(*(replaced_reason->replacement_tx))) {
1472+        m_unrelated_conflict_tx_watchlist.insert(std::make_pair(replaced_reason->replacement_tx->GetHash(), tx));
1473+    }
1474+
1475+    {

achow101 commented at 10:09 pm on November 4, 2024:

In 7eba56accbd1b4ce3c09656c8d69b5aa437a706f “Handle double-spending of unrelated parents to wallet txs”

The extra scoping is not necessary, just name it something else.

Eunovo commented at 2:03 pm on November 5, 2024:

Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5

in src/wallet/wallet.h:432 in 7eba56accb outdated

427@@ -428,6 +428,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
428     //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms
429     std::unordered_map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks;
430 
431+    /**
432+     * Tracks txids of unrelated double-spending txs  to wallet transactions

achow101 commented at 10:24 pm on November 4, 2024:

In 7eba56accbd1b4ce3c09656c8d69b5aa437a706f “Handle double-spending of unrelated parents to wallet txs”

nit: whitespace.

Also, I think it would be better to clarify that the key is the txid of the replacing transaction which may be unrelated to the wallet, and the value is the replaced transaction which is related to this wallet.

Eunovo commented at 2:03 pm on November 5, 2024:

Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5

Change MemPoolRemovalReason to variant type

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.

0e82ec3954

Handle double-spending of unrelated parents to wallet txs

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

3c893c5142

test: Conflict with unconfirmed parent not in wallet 78e6c4fce7

Handle new blocks with unrelated parents conflicts

Watch for wallet transaction conflicts triggered by adding conflicting blocks

19f39271fa

Eunovo force-pushed on Nov 5, 2024

DrahtBot added the label CI failed on Nov 20, 2024

in test/functional/wallet_conflicts.py:485 in 19f39271fa

480+
481+            assert_equal(def_wallet.gettransaction(parent_txid)["confirmations"], 0)
482+            assert_equal(current_wallet.gettransaction(child_txid)["confirmations"], 0)
483+
484+            # Make a conflict spending parent
485+            conflict_psbt = def_wallet.walletcreatefundedpsbt(inputs=[gp_utxo], outputs=[{def_wallet.getnewaddress(): 2}], fee_rate="{0:.8}".format(Decimal(parent_feerate*3)))["psbt"]

DrahtBot commented at 7:19 am on November 20, 2024:

https://cirrus-ci.com/task/5866756042915840?logs=ci#L2918

 0[02:23:17.596]  test  2024-11-20T02:23:16.130000Z TestFramework (ERROR): JSONRPC error 
 1[02:23:17.596]                                    Traceback (most recent call last):
 2[02:23:17.596]                                      File "/ci_container_base/test/functional/test_framework/test_framework.py", line 132, in main
 3[02:23:17.596]                                        self.run_test()
 4[02:23:17.596]                                      File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_conflicts.py", line 41, in run_test
 5[02:23:17.596]                                        self.test_unknown_parent_conflict()
 6[02:23:17.596]                                      File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_conflicts.py", line 485, in test_unknown_parent_conflict
 7[02:23:17.596]                                        conflict_psbt = def_wallet.walletcreatefundedpsbt(inputs=[gp_utxo], outputs=[{def_wallet.getnewaddress(): 2}], fee_rate="{0:.8}".format(Decimal(parent_feerate*3)))["psbt"]
 8[02:23:17.596]                                      File "/ci_container_base/test/functional/test_framework/coverage.py", line 50, in __call__
 9[02:23:17.596]                                        return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
10[02:23:17.596]                                      File "/ci_container_base/test/functional/test_framework/authproxy.py", line 146, in __call__
11[02:23:17.596]                                        raise JSONRPCException(response['error'], status)
12[02:23:17.596]                                    test_framework.authproxy.JSONRPCException: Invalid amount (-3)

DrahtBot added the label Needs rebase on Nov 20, 2024

DrahtBot commented at 2:50 pm on November 20, 2024: contributor

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

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

Code Coverage & Benchmarks

Reviews

Conflicts