The PR branch HEAD was 0538ad7 at the time of this review club meeting.
Notes
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
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
Are TxStateConflicted and TxStateMempoolConflicted transactions
treated the same in memory?
Are there any additional test cases you would like to see
implemented?
Why does wallet_abandonconflict.py need to be
modified in the second commit?
<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?
<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
<ishaanam[m]> LarryRuane: To clarify, that was not about what makes a transaction TxStateConflicted, it was about how a wallet treats a TxStateConflicted transaction
<abubakarsadiq> Solving an intermittent issue where mempool droped transaction are marked as TxStateConflicted, by creating a new state for mempool dropped transaction
<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?
<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?
<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
<BrandonOdiwuor> I think the PR tries to solve the issue of mempool conflicted transactions being treated as TxStateInactive by adding TxStateMempoolConflicted state
<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
<ishaanam[m]> That being said, what are the trade-offs with considering a mempool-conflicted transaction as TxStateMempoolConflicted instead of TxStateInactive?
<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)?
<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?
<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'?
<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
<ishaanam[m]> evansmj: it won't go through the mapTxSpends check that it does during `transactionAddedToMempool` and `transactionRemovedFromMempool`, if that's what you mean
<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
<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.)
<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.