Marking chains of txs conflicted properly

morcos commented at 3:11 pm on September 9, 2016: member

(These obscure notes are for eventually cleaning up wallet confliction and are low priority)

In the block connection logic we call

0BOOST_FOREACH(const CTransaction &tx, txConflicted)
1        {
2            SyncWithWallets(tx, pindexNewTip);
3        }

txConflicted is returned by mempool.removeForBlock and can therefore contain transactions that should be marked conflicted that wouldn’t be caught by the wallets own dependency tracking code (chains of mempool txs not all of which are in the wallet).

However, calling SyncWithWallets without setting a posInBlock has virtually no effect other than marking the cached credit/debit values as dirty. Since #7105 however wallet txs, that aren’t explicitly marked conflicted via the wallets code, aren’t actually known to be conflicted and are treated as just unconfirmed. I think the result of this is this SyncWithWallets loop is of dubious value.

All txs that are caught by the wallets conflict detection would already be marked dirty, and the new txs this loop catches, can’t correctly reflect the right balance state anyway.
It actually serves a detrimental effect on the transactions that were marked as abandoned that will now erroneously have their abandoned state cleared and be treated as unconfirmed. Their true state of conflicted matches much more closely to abandoned. (This will be corrected in the immediately following loop for txs that the wallet code catches)

It may be possible to just correctly MarkConflicted all these transactions, and then I think the only issue will be the txs that are only conflicted because they are dependents won’t properly have their cached values dirtied if the conflict goes away. This issue already exists #7315.

See IRC discussion https://botbot.me/freenode/bitcoin-core-dev/2016-09-09/?msg=72745343&page=3

morcos commented at 10:16 pm on September 9, 2016: member

@sipa I just tested removing that loop, and having that loop actually call MarkConflicted. Both seem to pass the tests (not sure that counts for much). I’m not sure which I like better, the latter duplicates much of the MarkConflicted thats about to happen anyway and makes the wallet unreachable txs hard to unconflict…

fanquake added the label Wallet on Sep 10, 2016

fanquake added the label Priority Low on Sep 10, 2016

laanwj removed the label Priority Low on Dec 6, 2017

meshcollider referenced this in commit 5e202382a9 on Sep 5, 2019

sidhujag referenced this in commit 4c9cc729fc on Sep 10, 2019

ryanofsky commented at 1:36 pm on January 11, 2021: member

Closing this issue. Notes are still somewhat relevant but a lot has changed since this was written and https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking which references this is probably a better starting point for understanding wallet conflict tracking and how it can be improved currently. Particularly the idea-more-conflicts section covers the information in these notes and discusses how the wallet can handle block-connected conflicts better.

Below is the original issue description annotated with details of what’s changed since this was opened.

re: #8692#issue-176033489

(These obscure notes are for eventually cleaning up wallet confliction and are low priority)

In the block connection logic we call
0BOOST_FOREACH(const CTransaction &tx, txConflicted)
1        {
2            SyncWithWallets(tx, pindexNewTip);
3        }

At the time, this code was part of ActivateBestChain. This code was first added in #3694, and removed in #9240, because it was mistakenly thought to be unnecessary after #7105. It was then added back in #9371 and then accidentally removed again in https://github.com/bitcoin/bitcoin/pull/16624/commits/7e89994133725125dddbfa8d45484e3b9ed51c6e from #16624. Finally it was added back again in #18982.

txConflicted is returned by mempool.removeForBlock and can therefore contain transactions that should be marked conflicted that wouldn’t be caught by the wallets own dependency tracking code (chains of mempool txs not all of which are in the wallet).

It’s still true that block connection code can detect conflicts that wallet code will miss.

However, calling SyncWithWallets without setting a posInBlock has virtually no effect other than marking the cached credit/debit values as dirty. Since #7105 however wallet txs, that aren’t explicitly marked conflicted via the wallets code, aren’t actually known to be conflicted and are treated as just unconfirmed. I think the result of this is this SyncWithWallets loop is of dubious value.

It’s still true that the these transactions are marked as unconfirmed instead of conflicted, but this could easily be changed as suggested in the wiki. It would just require some new code to update these from conflicted to unconflicted during reorgs.

It isn’t not true the notification loop doesn’t have value. It’s just that the current value comes from triggering external notifications, not from updating internal state.

All txs that are caught by the wallets conflict detection would already be marked dirty, and the new txs this loop catches, can’t correctly reflect the right balance state anyway.

This is still true, but doesn’t have to be. It’s only true because the transactions are currently marked unconfirmed instead of conflicted.

It actually serves a detrimental effect on the transactions that were marked as abandoned that will now erroneously have their abandoned state cleared and be treated as unconfirmed. Their true state of conflicted matches much more closely to abandoned. (This will be corrected in the immediately following loop for txs that the wallet code catches)

I didn’t go back and check if this was true at the time when this was written, but it’s not true now. These transactions could not already be marked as abandoned because they are being removed from the mempool and transactions that are in the mempool can’t be abandoned.

It may be possible to just correctly MarkConflicted all these transactions, and then I think the only issue will be the txs that are only conflicted because they are dependents won’t properly have their cached values dirtied if the conflict goes away. This issue already exists #7315.

This is still possible. #7315 has changed since this was reported, but #7315 is just about reorg handling, and marking these conflicted and handling reorgs is easily possible

See IRC discussion botbot.me/freenode/bitcoin-core-dev/2016-09-09/?msg=72745343&page=3

Updated link: http://www.erisian.com.au/bitcoin-core-dev/log-2016-09-09.html#l-322

ryanofsky closed this on Jan 11, 2021

DrahtBot locked this on Aug 18, 2022

Marking chains of txs conflicted properly #8692