Skip to content

Wallet Transaction Conflict Tracking

Jump to bottom
Ishaana Misra edited this page Jun 10, 2023 · 9 revisions

The wallet uses heuristics to distinguish between unconfirmed, conflicted, and abandoned transactions. Transaction states are updated based on mempool and block connected notifications, abandontransaction calls, and the wallet's incomplete internal database of transactions, which tracks different transactions spending the same outputs (mapWallet, mapTxSpends).

This page exists to document ideas and requirements for improving transaction state heuristics.

Contents

  1. Transaction states
  2. Previous bugs
  3. Ideas for improvement
  4. Ideas for test coverage
  5. Ideas for refactoring
  6. History

Transaction states

Wallet transactions are represented by CWalletTx objects. A transaction can be confirmed in a block, or unconfirmed and in the mempool, or unconfirmed and not in the mempool because it conflicts with a block transaction, or unconfirmed and not in the mempool because it conflicts with a mempool transaction, or unconfirmed and not in the mempool for some other reason.

If a transaction isn't in the mempool, the wallet can't reliably determine if it conflicts with something, but it tries to guess if it conflicts with a confirmed block transaction, and store what the earliest conflicting block seems to be. The wallet currently treats MemPoolRemovalReason::CONFLICT transactions that conflict with the mempool as unconfirmed, not conflicted. This can cause inputs to these transactions to misleadingly appear as spent and wallet balances be lower, so the wallet provides an abandontransaction RPC to manually exclude these transactions and ignore them. abandontransaction doesn't need to be called very often because most of the time, a conflicting transaction will spend the same or more inputs as the original transaction, so there is no need to intervene and mark any unspent. abandontransaction doesn't affect the way transaction outputs are treated because outputs from transactions that are not in the mempool are already not trusted for spending or included in balances.

Transaction state is serialized and persisted CWalletTx::hashBlock and CWalletTx::nIndex fields with the following interpretation:

uint256 hashBlock int nIndex interpretation
0 0 unconfirmed
>1 >=0 confirmed
>1 -1 conflicted since #7105, unconfirmed prior
1 -1 abandoned since #7312, unconfirmed prior

Previous bugs

These bugs have now been fixed by #27145, but could still use additional test coverage and further internal consistency checking.

  • bug-stale-conflicted – After a reorg, transactions that are no longer conflicted will stay in the conflicted state until the next time the wallet is loaded, because blockDisconnected notification code was never written to handle this case. A possible fix there would be to mark all conflicted transactions as unconflicted if their conflicting block height is greater or equal to the disconnect block height (idea-disconnect-conflicted). This idea would just make state updates that will already happen on the next reload happen earlier. An alternative fix would be to rerun wallet conflict detection logic on transactions in disconnected blocks: idea-refresh-conflicted. It could make sense to implement one or both of these fixes. The first fix would be more general and future proof, but the second fix would also be logical could have better peformance.

  • bug-reorg-depth – As a consequence of the bug-stale-conflicted issue above, after a reorg, transactions that are no longer conflicted will return meaningless and usually incorrect GetDepthInMainChain values. This began with 0ff0387 from #15931, because the safeguard in getBlockDepth that returned 0 when the block is not part of the active chain was bypassed. The correct value to return is 0 (unconfirmed), but right now the implementation returns negative (conflicted) values if the current proof of work tip has a greater or equal height as the originally conflicting block, or positive (confirmed) values if current tip is shorter than the originally conflicting block height. It does return the correct 0 (unconfirmed) value by accident if the current tip height is exactly one less than the originally conflicting block height. Either of the idea-disconnect-conflicted or idea-refresh-conflicted fixes below for the bug-stale-conflicted issue above would fix this bug as well, but ideally this bug would also be directly addressed and tested as described idea-reorg-depth. After 40ede99 this bug is less severe and fixes itself when the wallet is reloaded. But until the wallet is reloaded, this bug can cause transaction inputs that should be considered spent to be spendable, and transactions that should be possible to abandon not possible to abandon.

  • bug-reorg-spent – As a consquence of the bug-stale-conflicted issue above, after a reorg, inputs of transactions that are no longer conflicted may be considered spent when they are not spent, due to CWalletTx balance caching and MarkInputsDirty never being called on the now-unconfirmed transactions. This problem was originally reported in #7315 but was made worse later by bug-reorg-depth. Either of the idea-disconnect-conflicted or idea-refresh-conflicted fixes below for the bug-stale-conflicted issue above would fix this bug as well, but it could also be addressed as described in idea-disconnect-inputs by marking all inputs of reorged transactions dirty. It would also be good to write an independent test for this bug that passes when this bug is fixed but bug-stale-conflicted is not fixed, so there is test coverage of balance caching logic being reliable even if conflict detection logic is broken. It could also be good to have checks running in debug builds that are skipped in release builds to verify CWalletTx cached balances are always correct.

Ideas for improvement

  • idea-disconnect-conflicted – When a block is disconnected, it would be good to loop over all conflicted transactions with conflicted block height greater or equal to the disconnected block height and mark them as unconfirmed instead of conflicted. This would fix bug-stale-conflicted, bug-reorg-depth, and bug-reorg-spent bugs above. If this is fixed, good test coverage should check not only that transaction is no longer marked conflicted, but also check that MarkInputsDirty is called on the transaction (test should fail if it is not called), so its inputs are considered unspent even if they were cached as spent previously.

  • idea-disconnect-inputs – Suggestion from #7315 to call MarkInputsDirty on all block transactions in the blockDisconnected handler regardless of whether AddToWalletIfInvolvingMe returns true or false, instead of only when it returns true. This would fix the bug-reorg-spent issue above, but would probably be overkill because other fixes are possible and because it is only technically necessary to mark inputs of transactions dirty when the transactions switch between conflicted and unconflicted states, not when they switch between confirmed and unconfirmed states.

  • idea-refresh-conflicted – [Implemented in #27145] When a transaction from a disconnected block is removed, it would be good to refresh other transactions sharing same inputs and mark them as unconfirmed if they are conflicted. Commit 3c8bd68 from #17543 implemented this but the change was not merged. This change would fix the same bugs as the idea-disconnect-conflicted change and should be accompanied by the same suggested new test coverage if it is picked up. This change could be implemented as a faster alternative to that change, or both changes could be implemented together in a way that lets the wallet do more internal consistency checking.

  • idea-reorg-depth – To fix bug-reorg-depth, GetDepthInMainChain could return 0 instead of -1 if a conflicting block is not in the active chain. Or after bug-stale-conflicted is fixed, it could just assert that the block hash must be in the active chain and that the block height value is valid and less than or equal to the last block processed height. Checking if the block is in the active chain is relatively expensive so might make sense to skip in release builds and only do in debug builds. Ideally a fix for this issue would include test coverage for all three cases where formerly conflicted transactions currently return negative, positive, and 0 depth values.

  • idea-abandon-bumped – It might make sense to mark bumpfee replaced transactions as abandoned, so their inputs will no longer be considered spent. There is not a need right now because replacement transactions are covered by conflict tracking and mempool checks, and spend all the same inputs as the original transactions anyway.

  • idea-sticky-abandoned – Conflicted and abandoned states shouldn't be exclusive. Because they are exclusive currently, the abandoned state takes precedence over the conflicted state. This is so if there is a reorg and an abandoned and previously-conflicted transaction becomes no longer conflicted, it will remain abandoned, and won't be rebroadcast, or spend its inputs, or affect the wallet balance. But more ideally it would be possible to see if a transaction is conflicted even if it is also abandoned. Having a stickier abandoned state might be useful in other cases. For example a blockDisconnected comment suggests continuing to treat a transaction abandoned if it is confirmed and then reorged out without being added back to the mempool. This is a narrower corner case, though.

  • idea-mempool-conflicted – Instead of only considering transactions that conflict with block transactions as conflicted, the wallet could also treat transactions that conflict with mempool transactions as conflicted, rather than unconfirmed. Reasons this is not done currently are noted in a transactionRemovedFromMempool code comment. An additional reason might be related to #7315: as long as the conflicted transaction is kept in an unconfirmed state instead of the conflicted state, its other nonconflicting inputs are not considered spendable, which could be safer for coin selection. On the other hand, treating these transactions as conflicted could remove some cases where it's neccessary to use the abandontransaction RPC, and might even make it possible to eliminate the RPC entirely. bdf40f3 from #18600 was an attempt to implement mempool conflict detection by treating mempool conflicts the same as block conflicts. But it did not add code to detect transactions becoming no longer conflicted. And probably it would make more sense to treat block and mempool conflicts differently, perhaps just tracking mempool conflicts in-memory, instead of trying to serialize mempool conflict state to disk. #18600 also went further and exposed conflict state in RPCs and sent new -walletnotify notifications for mempool conflicted transactions.

  • idea-more-conflicts – validation code's block connection logic is able to identify conflicts between transactions that the wallet conflict tracking code cannot always detect due to not having a complete view of all transaction inputs. Before cdb8934 from #17477, lists of conflicting transactions were passed to the wallet blockConnected handler, and in commit a31be09 from #16624 these were marked as conflicted. But this change was temporary (immediately reverted in the following commit) and the change also had a bug because it did not record the conflicting block hash. If desired though, it would be possible to restore some of this code to mark more transactions conflicted. This could help avoid the need to use abandontransaction in the case of indirect conflicts. However, it is probably not worth complexity to node and wallet code to implement this. It would also not be safe to implement this without first implementing the idea-disconnect-conflicted fix to mark these transactions unconflicted if the conflicting block is disconnected. #8692 has some older notes on this. The history section below covers the history of this code.

Ideas for test coverage

A top priority for improvement, and a requirement for any changes should be adding more test coverage. Existing code has poor coverage and will pass tests in many cases even if it's deliberately broken or has pieces removed. It would be possible to add lower-level C++ tests or higher-level python tests. Best would be high level python tests exhaustively covering different mempool and chain states and block and transaction notification sequences, verifying that transactions get updated to expected states, and that states result in expected spending and balance behavior. But even just adding tests for existing bugs would be a big improvement to prevent recurring problems and regressions such as #18878. Testing ideas:

  • It would be good to add test coverage for cases described in previous bugs and ideas for improvement sections above.

  • It would be good to add test coverage for the previously fixed offline reorg bug 40ede99 from #16624, which doesn't have any coverage currently. Particularly all tests seem to pass currently if the LoadToWallet wtx.isConflicted() condition is removed.

  • It would be good to add test coverage for LoadToWallet conflict detection code which was almost accidentally removed in cd15809a from #17543

  • It would be good to enumerate ways transactions can transition between states and check they are handled correctly:

    • Check that when a wallet transaction is added to a block, it is marked confirmed.
    • Check that when directly conflicting transactions are added to a block, wallet transactions are marked conflicted.
    • Check that when indirectly conflicting transactions are added to a block, wallet transactions are marked as not in the mempool and remain unconfirmed or abandoned.
    • Check that when a confirmed wallet transaction is in a disconnected block and doesn't conflict, it is marked unconfirmed.
    • Check that when a confirmed wallet transaction is in a disconnected block and there is a direct conflict, it is marked conflicted and not in the mempool.
    • Check that when a confirmed wallet transaction is in a disconnected block and there is an indirect conflict, it is marked unconfirmed and not in the mempool.
    • Check that when an unconfirmed transaction is added or removed from the mempool, it stays unconfirmed, but balances update because its outputs become trusted or untrusted.
    • Check that when an abandoned transaction is added to mempool, it becomes unconfirmed instead of abandoned.
    • Check that when a conflicted transaction is added to the mempool, it becomes unconfirmed instead of conflicted. (Or if idea-disconnect-conflicted or idea-refresh-conflicted ideas are implemented, add an assert to verify this case is impossible.)
    • Check that when an abandoned transaction is detected to be conflicted it remains abandoned.

    Existing wallet_txn_doublespend.py, feature_notifications.py, and wallet_conflicts.py tests cover some of these cases and show test setup that can be used to reproduce them.

Ideas for refactoring

  • It would be good to improve CWalletTx code quality to check at runtime that inconsistent states are impossible to set, or change the data structure so invalid states have a different representation than valid states and there is no way to set invalid states accidentally. A great first step would be to eliminiate the stored CWalletTx::Confirmation::status field and replace it with a method to return transaction status on the fly. Better would be to eliminate the confirmation struct entirely and replace it with unambiguous sum (variant) type that clearly separates the different possible states and state attributes. #18600 (comment) breaks down all the possible valid and invalid states and would be a good starting point for a variant definition.

History

The conflicted state was first added in #7105 and the abandoned state was added in #7312. Various changes and improvements have been made since then:

Clone this wiki locally