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

[Eunovo]

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]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

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

No conflicts as of last run.

[DrahtBot]

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

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

[glozow]

cc @achow101?

[achow101]

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]

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]

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]

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]

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]

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

Putting in draft while I fix falling test

[josibake]

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:

class RemovedReason {
public:
    MemPoolRemovalReason m_reason;
    std::variant<std::monostate, CTxReference, BlockData> m_extra_data;

    // Constructor for reasons that don't require extra data
    RemovedReason(MemPoolRemovalReason r) : reason(r) {
        if (requiresExtraData(r)) {
            throw std::invalid_argument("reason X requires data Y etc");
        }
    }

    // Constructor needing CTxRef
    RemovedReason(RemovalReason r, const CTxRef& data) : reason(r), extra_data(data) {
        if (!IsA(r)) {
            throw std::invalid_argument("CTxRef is required for reason A, got bla");
        }
    }

    // Constructor needing BlockData
    RemovedReason(RemovalReason r, const BlockData& data) : reason(r), extra_data(data) {
        if (!IsB(r)) {
            throw std::invalid_argument("BlockData is required for reason B, got bla");
        }
    }

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]

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]

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:

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

as a good alternative to:

enum class MyState {
  STATE1,
  STATE2,
  STATE3,
};

class MyData {
  MyState m_state,
  // ... more data and methods...
};

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]

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 😄

[josibake]

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

[Eunovo]

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

[DrahtBot]

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