Track mempool conflicts with wallet transactions (wallet)

Host: ishaanam  -  PR author: ishaanam

The PR branch HEAD was 0538ad7 at the time of this review club meeting.


  • In Bitcoin Core, every wallet transaction has a transaction state (detailed here). These transaction states are part of how the wallet decides which transactions to allow the user to spend, and which transactions to count towards a user’s balance.

  • Wallet transaction states and conflicts were previously discussed in review club #27145

  • When a transaction is TxStateInactive:
    • its inputs are considered spent, and
    • its outputs are not available to spend and don’t count towards a user’s balance
  • When a transaction is TxStateConflicted:
    • its inputs are not considered spent, and
    • its outputs are not available to spend and don’t count towards a user’s balance
  • On master, wallet txs are only considered conflicted when the conflicting tx gets mined into a block. This means that if a transaction is only conflicted by a mempool tx, it is considered TxStateInactive instead. This can lead to confusion amongst users, because the funds seem to briefly “disappear”.

  • This PR treats transactions with conflicts in the mempool as conflicted as well, by adding another transaction state for mempool-conflicted transactions and keeping track of the conflicting transactions in MempoolConflicts, a map of wallet tx hashes to a set of their mempool conflicts’ tx hashes.

  • This idea and a previous attempt to implement it is described here


  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

  2. Is this PR fixing a bug or adding a feature? What is that bug/feature?

  3. What are the trade-offs with considering a mempool-conflicted transaction as conflicted instead of inactive?

  4. Is the first commit necessary for this PR? Does it change any existing behavior?

  5. What is the point of adding a MempoolConflicts map? Why can’t the wallet just check for conflicts in mapTxSpends?

  6. What is the benefit of adding another transaction state (TxStateMempoolConflicted) instead of just using TxStateConflicted?

  7. Should a user be able to abandon a transaction with a mempool conflict? With this PR is a user able to do so?

  8. After a wallet is reloaded, what will be the transaction state of a previously mempool-conflicted transaction?

  9. Do the tests added to fail on master for you?

  10. This PR doesn’t modify any of the balance calculation code directly, so how do the changes made in this PR affect the balance calculation of the wallet?xf

  11. Are TxStateConflicted and TxStateMempoolConflicted transactions treated the same in memory?

  12. Are there any additional test cases you would like to see implemented?

  13. Why does need to be modified in the second commit?

Meeting Log

  117:00 <ishaanam[m]> #startmeeting
  217:00 <ishaanam[m]> Hey everyone, welcome to PR review club!
  317:00 <LarryRuane> hi
  417:00 <brunoerg> hi
  517:00 <ishaanam[m]> Today we will be reviewing #27307: "Track mempool conflicts with wallet transactions"
  617:00 <ishaanam[m]> The notes and questions can be found here:
  717:01 <Pins> hi
  817:01 <kevkevin> hi
  917:01 <ishaanam[m]> Feel free to ask any questions!
 1017:01 <ishaanam[m]> Has anyone gotten a chance to review the PR or look at the notes?
 1117:01 <evansmj> hey do scripted diffs normally come in an independent pr?  i think yes right?
 1217:02 <abubakarsadiq> hello
 1317:02 <LarryRuane> One really basic question, is there a `CWalletTx` instance in memory for every tx that is either paying *to* this wallet or paying *from* this wallet? And both mined and unmined transactions?
 1417:03 <sipa> Yes, and yes.
 1517:03 <Pins> ishaanam[m] I did
 1617:03 <LarryRuane> evansmj: no, scripted diff is generally *one* commit within a larger PR
 1717:04 <ishaanam[m]> Great, can someone give a short summary of what this PR is doing?
 1817:05 <LarryRuane> ishaanam[m]: I was confused by the description of `TxStateConflicted` in the notes... its inputs are not spent (by another tx? and its outputs are not available to spend, for example immature coinbase? To me, that doesn't sounds "conflicted" ... so I must be missing something
 1917:05 <Pins> creating a new tx state to the mempool conflicted tx
 2017:07 <ishaanam[m]> LarryRuane: To clarify, that was not about what makes a transaction TxStateConflicted, it was about how a wallet treats a TxStateConflicted transaction
 2117:08 <LarryRuane> oh i see, thanks, that helps a lot
 2217:08 <abubakarsadiq> Solving an intermittent issue where mempool droped transaction are marked as TxStateConflicted, by creating a new state for mempool dropped transaction
 2317:08 <LarryRuane> i thought it was defining those states
 2417:10 <ishaanam[m]> LarryRuane: yes, I think there was some confusion about that earlier as well, I think I'll go back and clarify the notes
 2517:10 <LarryRuane> I still didn't understand (sorry, you can go on) exactly which event(s) causes funds (by which I think you mean balance) to briefly disappear? And which event(s) cause them to come back?
 2617:11 <LarryRuane> But as I said, you can go on if everyone else gets it :)
 2717:11 <ishaanam[m]> abubakarasdiq: while a new transaction state is created for mempool conflicted transactions, I don't think that mempool dropped transactions were ever marked as TxStateConflicted?
 2817:12 <ishaanam[m]> Also, I want to clarify that this new transaction state is only applied to transactions which are conflicted by another transaction in the mempool, not transactions which have been dropped without conflicts
 2917:13 <abubakarsadiq> thanks
 3017:14 <BrandonOdiwuor> I think the PR tries to solve the issue of mempool conflicted transactions being treated as TxStateInactive by adding TxStateMempoolConflicted state
 3117:15 <ishaanam[m]> LarryRuane: Because the funds are marked as TxStateInactive when they are conflicted out of the mempool, the wallet will not show the outputs (the funds which disappear), but it will still consider the inputs as spent
 3217:15 <ishaanam[m]> BrandonOdiwuor: yes!
 3317:15 <LarryRuane> ishaanam[m]: +1 thanks
 3417:16 <ishaanam[m]> That being said, what are the trade-offs with considering a mempool-conflicted transaction as TxStateMempoolConflicted instead of TxStateInactive?
 3517:18 <ishaanam[m]> Meaning, is it more or less "safe", from a user's perspective
 3617:20 <abubakarsadiq> mempool-conflicted transaction inputs will no longer be considered as spent
 3717:20 <ishaanam[m]> Is it better to err on the side of caution and consider something as "spent" even if it technically is not, or consider something as "unspent" even though it's funds could potentially be spent (though not likely)?
 3817:21 <LarryRuane> considering something as spent that may not be seems like the more conservative view
 3917:21 <Pins> +1
 4017:22 <LarryRuane> and in bitcoin we like to be conservative 😄
 4117:22 <Pins> ;D
 4217:22 <ishaanam[m]> yes, that's true
 4317:23 <ishaanam[m]> What is the benefit of adding another transaction state (TxStateMempoolConflicted) instead of just using TxStateConflicted?
 4417:25 <evansmj> other wallets can differentiate between either mempool or block conflicts more clearly with the new state in transaction.h, and display it different or inform the user whats going on?
 4517:25 <BrandonOdiwuor> To distinguish between block conflicted and mempool conflicted transactions
 4617:25 <Pins> Diferentiate the kind of conflict
 4717:26 <ishaanam[m]> evansmj: BrandonOdiwuor: Pins: Yes, to make the distinction more clear.
 4817:27 <ishaanam[m]> There was a previous attempt at this PR where TxStateConflicted was used
 4917:27 <evansmj> does marking a tx as a mempool-conflict consider it "spent" or "unspent"?
 5017:27 <evansmj> vs inactive
 5117:27 <Pins> evansmj spent
 5217:29 <Pins> I'm not very sure
 5317:29 <ishaanam[m]> Also, because previously mempool-conflicted states were serialized differently, a new tx state was introduced to maintain this behavior
 5417:29 <ishaanam[m]> evansmj: Pins: Marking a tx as mempool-conflicted means that it's inputs are "unspent"
 5517:30 <Pins> ishaanam[m] Yes, sure ... thanks
 5617:30 <ishaanam[m]> The code for this is here:
 5717:31 <LarryRuane> One thing that I found was pretty interesting, and I wasn't aware of, is `std::get_if` ... that's some pretty advanced c++
 5817:31 <abubakarsadiq> whats the difference between mempool conficted and mempool dropped transactions?
 5917:32 <ishaanam[m]> After a wallet is reloaded, what will be the transaction state of a previously mempool-conflicted transaction?
 6017:32 <Pins> ishaanam[m] TxStateInactive
 6117:34 <ishaanam[m]> abubakarsadiq: the way I see it is that mempool conflicted transactions are a subset of mempool dropped transactions.
 6217:34 <abubakarsadiq> in master TxStateInactive, with this pr TxStateMempoolConflicted
 6317:34 <ishaanam[m]> mempool dropped transactions could have been dropped for some reason other than being conflicted
 6417:35 <evansmj> after a wallet reload will it go through WalletBatch::LoadWallet() which will start everything over again, or will it do the mapWallet check when a wallet is 'reloaded'?
 6517:35 <ishaanam[m]> For example, they could have been dropped from the mempool because of size-limiting
 6617:36 <ishaanam[m]> Pins: yes
 6717:37 <ishaanam[m]> abubakarsadiq: Because this PR doesn't change how mempool-conflicted transactions are serialized, and previously they would be considered TxStateInactive, it is still TxStateInactive
 6817:39 <ishaanam[m]> evansmj: it won't go through the mapTxSpends check that it does during `transactionAddedToMempool` and `transactionRemovedFromMempool`, if that's what you mean
 6917:40 <ishaanam[m]> Come to think of it, there should be a test for reloading a wallet with a mempool-conflicted transaction
 7017:40 <ishaanam[m]> Speaking of tests, has anyone tried running the added tests on master?
 7117:41 <evansmj> so just copy paste the new tests to master and make sure they fail?
 7217:42 <ishaanam[m]> evansmj: You could do that, or you could run git cherry-pick 0538ad7d4cfddb5a377f879cbf221b2b028c264a(the test commit hash) so that only the commit with the tests gets added to your master branch
 7317:43 <abubakarsadiq> evansmj: or can just run the test without compiling the pr
 7417:43 <abubakarsadiq> thanks ishaanam
 7517:45 <ishaanam[m]> Are there any additional test cases that you would like to see implemented?
 7617:49 <ishaanam[m]> What about the first commit of this PR, does it change any existing behavior?
 7717:50 <ishaanam[m]> This is the commit that I'm referring to:
 7817:54 <evansmj> makes CachedTxIsTrusted() use the new tx state instead of checking tx depth
 7917:55 <evansmj> same for IsTxInMainChain()
 8017:55 <LarryRuane> I don't think that commit changed behavior. You could run the existing tests on just that commit and see if any fail. If none fail, this *may* be a behavior non-change. (I guess you still can't be sure.)
 8117:57 <ishaanam[m]> evansmj: yes, there were multiple places that used the depth instead of just checking the state
 8217:58 <ishaanam[m]> LarryRuane: Yes, this commit is interesting because it is technically just a refactor, but this PR won't work without it. This is because it changed the balance calculation by modifying IsSpent.
 8317:58 <ishaanam[m]> If depth was still used, then mempool-conflicted transactions would have a depth of 0, so their inputs would be considered as spent.
 8418:59 <LarryRuane> +1 makes sense
 8518:00 <ishaanam[m]> It looks like an hour is up
 8618:00 <ishaanam[m]> thanks everyone for coming!
 8718:00 <ishaanam[m]> #endmeeting