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

pull Eunovo wants to merge 4 commits into bitcoin:master from Eunovo:fix-unknown-parent-conflict changing 24 files +248 −95
  1. 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.

  2. 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, ismaelsadeeq

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

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Wallet on Mar 20, 2024
  4. DrahtBot added the label CI failed on Mar 20, 2024
  5. 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

  6. Eunovo force-pushed on Mar 22, 2024
  7. DrahtBot removed the label CI failed on Mar 22, 2024
  8. Eunovo force-pushed on Mar 25, 2024
  9. Eunovo force-pushed on Mar 26, 2024
  10. Eunovo commented at 3:15 pm on March 26, 2024: none
    Draft until I’ve gotten some more feedback on the approach.
  11. glozow commented at 12:12 pm on March 27, 2024: member
    cc @achow101?
  12. DrahtBot added the label Needs rebase on Mar 27, 2024
  13. 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?
  14. 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

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

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

  17. Eunovo force-pushed on Apr 9, 2024
  18. 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.

  19. DrahtBot removed the label Needs rebase on Apr 9, 2024
  20. Eunovo force-pushed on Apr 9, 2024
  21. DrahtBot added the label CI failed on Apr 9, 2024
  22. 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

  23. Eunovo force-pushed on Apr 10, 2024
  24. DrahtBot removed the label CI failed on Apr 10, 2024
  25. Eunovo marked this as ready for review on Apr 10, 2024
  26. Eunovo commented at 3:43 am on April 21, 2024: none
    Putting in draft while I fix falling test
  27. Eunovo marked this as a draft on Apr 21, 2024
  28. Eunovo force-pushed on Apr 22, 2024
  29. DrahtBot added the label CI failed on Apr 22, 2024
  30. Eunovo force-pushed on Apr 23, 2024
  31. DrahtBot removed the label CI failed on Apr 23, 2024
  32. Eunovo marked this as ready for review on Apr 23, 2024
  33. DrahtBot added the label CI failed on May 4, 2024
  34. Eunovo force-pushed on May 7, 2024
  35. Eunovo force-pushed on May 9, 2024
  36. DrahtBot removed the label CI failed on May 11, 2024
  37. 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

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

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

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

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

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

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

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

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

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

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

  48. DrahtBot added the label Needs rebase on May 15, 2024
  49. Eunovo force-pushed on May 20, 2024
  50. DrahtBot removed the label Needs rebase on May 20, 2024
  51. DrahtBot added the label CI failed on May 20, 2024
  52. 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

  53. Eunovo force-pushed on May 21, 2024
  54. DrahtBot removed the label CI failed on May 22, 2024
  55. 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.

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

  57. 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
  58. 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
  59. 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

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

  61. 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.
  62. in src/kernel/mempool_removal_reason.h:48 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
  63. 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.
  64. DrahtBot added the label Needs rebase on May 24, 2024
  65. Eunovo force-pushed on Jun 2, 2024
  66. DrahtBot removed the label Needs rebase on Jun 3, 2024
  67. 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
  68. 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.

  69. Eunovo force-pushed on Jun 3, 2024
  70. Eunovo commented at 11:24 pm on June 3, 2024: none
    Thanks for the reviews @josibake and @furszy I have implemented your suggested changes
  71. DrahtBot added the label CI failed on Jun 4, 2024
  72. 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

  73. Eunovo marked this as a draft on Jun 4, 2024
  74. Eunovo force-pushed on Jun 5, 2024
  75. DrahtBot removed the label CI failed on Jun 5, 2024
  76. Eunovo marked this as ready for review on Jun 6, 2024
  77. DrahtBot added the label Needs rebase on Jul 2, 2024
  78. Eunovo force-pushed on Jul 3, 2024
  79. DrahtBot removed the label Needs rebase on Jul 3, 2024
  80. Eunovo requested review from furszy on Jul 8, 2024
  81. achow101 requested review from achow101 on Oct 15, 2024
  82. DrahtBot added the label CI failed on Oct 20, 2024
  83. DrahtBot removed the label CI failed on Oct 24, 2024
  84. 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.

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

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

  87. Eunovo force-pushed on Nov 5, 2024
  88. DrahtBot added the label CI failed on Nov 20, 2024
  89. in test/functional/wallet_conflicts.py:479 in 19f39271fa outdated 
    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)
  90. DrahtBot added the label Needs rebase on Nov 20, 2024
  91. Eunovo force-pushed on Dec 1, 2024
  92. DrahtBot removed the label Needs rebase on Dec 1, 2024
  93. Eunovo commented at 9:49 am on December 1, 2024: none

    Rebased https://github.com/bitcoin/bitcoin/commit/19f39271fa9f5b26075ca8f66afe25751813bb75 to https://github.com/bitcoin/bitcoin/commit/ef7b1e8e82819e5104c1f1f49f46ea74b416ced0

    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.

  94. DrahtBot removed the label CI failed on Dec 1, 2024
  95. Eunovo force-pushed on Dec 2, 2024
  96. DrahtBot added the label CI failed on Dec 31, 2024
  97. DrahtBot removed the label CI failed on Jan 2, 2025
  98. DrahtBot commented at 1:43 pm on January 2, 2025: contributor

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

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

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

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

  100. Eunovo commented at 12:12 pm on January 5, 2025: none
    Putting in draft while I fix failing CI
  101. Eunovo marked this as a draft on Jan 5, 2025
  102. Eunovo force-pushed on Jan 15, 2025
  103. DrahtBot added the label CI failed on Jan 15, 2025
  104. 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.
    4c64dd5a3c
  105. 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
    71200b72ab
  106. test: Conflict with unconfirmed parent not in wallet c162984fbc
  107. Handle new blocks with unrelated parents conflicts 
    Watch for wallet transaction conflicts triggered by adding conflicting blocks
    6db2961f60
  108. Eunovo force-pushed on Jan 16, 2025
  109. in src/interfaces/chain.h:7 in 4c64dd5a3c outdated 
    6@@ -7,6 +7,7 @@
7
    ismaelsadeeq commented at 6:18 pm on January 17, 2025:

    In 4c64dd5a3cc4a48e5470fff26c99dde20f81c7e0 “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.

  110. in src/kernel/mempool_removal_reason.h:50 in 4c64dd5a3c outdated 
    52+
53+struct ConflictReason {
54+    uint256 conflicting_block_hash;
55+    unsigned int conflicting_block_height;
56+
57+    explicit ConflictReason(const uint256& conflicting_block_hash, int conflicting_block_height) : conflicting_block_hash(conflicting_block_hash), conflicting_block_height(conflicting_block_height) {}
    ismaelsadeeq commented at 6:19 pm on January 17, 2025:
    In 4c64dd5a3cc4a48e5470fff26c99dde20f81c7e0 “Change MemPoolRemovalReason to variant type” nit: this line is too long, please break.
  111. in src/wallet/wallet.cpp:1493 in 71200b72ab outdated 
    1488+    if (iter != m_unrelated_conflict_tx_watchlist.end()) {
1489+        // The replacement tx was removed from the mempool, remove it from map
1490+        // This new replacement tx may not conflict with the original tx
1491+        // so leave wallet tx to remain as TxStateInactive
1492+        m_unrelated_conflict_tx_watchlist.erase(iter);
1493+    }
    ismaelsadeeq commented at 6:22 pm on January 17, 2025:

    In 71200b72ab848c3066e5123dc0badc24c9648f47 “Handle double-spending of unrelated parents to wallet txs”

     0    auto* replaced_reason = std::get_if<ReplacedReason>(&reason);
 1    if (IsFromMe(*tx) && replaced_reason != nullptr) {
 2
 3        // Check if wallet transaction is being replaced by a parent transaction which is not from this wallet
 4        if (replaced_reason->replacement_tx.has_value() && !IsFromMe(*replaced_reason->replacement_tx.value())) {
 5            m_unrelated_conflict_tx_watchlist.insert(std::make_pair(replaced_reason->replacement_tx.value()->GetHash(), tx));
 6        }
 7        // Remove the replacement tx was removed from the mempool, remove it from map
 8        // This new replacement tx may not conflict with the original tx
 9        // so leave wallet tx to remain as TxStateInactive
10        m_unrelated_conflict_tx_watchlist.erase(tx->GetHash());
11    }
  112. in src/wallet/wallet.cpp:1491 in 71200b72ab outdated 
    1486+
1487+    auto iter = m_unrelated_conflict_tx_watchlist.find(tx->GetHash());
1488+    if (iter != m_unrelated_conflict_tx_watchlist.end()) {
1489+        // The replacement tx was removed from the mempool, remove it from map
1490+        // This new replacement tx may not conflict with the original tx
1491+        // so leave wallet tx to remain as TxStateInactive
    ismaelsadeeq commented at 6:23 pm on January 17, 2025:

    In 71200b72ab848c3066e5123dc0badc24c9648f47 “Handle double-spending of unrelated parents to wallet txs”

    How do you know that without checking ?

  113. ismaelsadeeq commented at 6:33 pm on January 17, 2025: member

    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?

  114. Eunovo commented at 3:41 pm on January 18, 2025: none

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


Eunovo DrahtBot glozow achow101 josibake ryanofsky furszy ismaelsadeeq


furszy

Labels
Wallet CI failed

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me