wallet: undo conflicts properly in case of blocks disconnection #17543

1. 
   ariard commented at 9:30 pm on November 20, 2019: member

   At the moment, in case of block disconnection, if any conflicting transaction was inside, we don’t undo conflicts on wallet transactions, i.e they stay in state CONFLICTED instead of being marked again as UNCONFIRMED. This shortcoming causes inaccurate balance computation and blocks the user from re-submitting the tx back to the mempool.

   Currently, if conflicted transaction was later confirmed, the status is updated accordingly and nothing changes. This PR undoes conflict(s) and also cleans up some of MarkConflicted which is renamed UpdatedConflicts to cover both instances of unconfirmed to conflicted and conflicted to unconfirmed cases.

   Lambda is used, but I’m open to other approaches.

2. Remove MarkConflicted in LoadToWallet

   While wallet is shutdown, no block connection can happen and
   the confict detection logic can't be triggered to change
   status of any wallet transactions. Therefore, updating
   childrens status based on father one shouldn't produce
   any change.

   cd15809a32
3. Refactor MarkConflicted to UpdateConflicts

   In next commit, we'll be able to reuse UpdateConflicts to also
   cover the conflicted to unconfirmed status update case.
   
   Remove check on conflicting height being ahead of the last block
   processed as both rescan logic and connection/disconnection guarantee
   than the block viewed is in best chain at time of processing.
   
   In case of block disconnection, next commit will undo conflicted status
   back to unconfirmed.

   b8b4592d98
4. Undo conflicts if deepest conflicting tx is removed from chain

   Extend SyncTransaction and AddToWalletIfInvolvingMe to check
   that disconnected conflicting tx was the deepest one and conflicted
   tx status can be safely change to unconfirmed.

   3c8bd6814c
5. ariard renamed this:
   Wallet: undo conflicts properly in case of blocks disconnection
   wallet: undo conflicts properly in case of blocks disconnection
   on Nov 20, 2019
6. fanquake added the label Wallet on Nov 20, 2019
7. 
   DrahtBot commented at 1:44 am on November 21, 2019: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #18278 (interfaces: Describe and follow some code conventions by ryanofsky)
   • #17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
   • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)
   • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

8. Add wallets_conflicts

   Test cover the case of a tx being conflicted by multiple
   txn with different depth. Conflicted tx is also spent by
   a child tx for which confirmation status is tied to parent
   one. After reorg of conflicting txn, conflicted status should be
   undo properly.

   ec5a89a26f
9. ariard force-pushed on Nov 21, 2019
10. DrahtBot added the label Needs rebase on Mar 23, 2020
11. 
    DrahtBot commented at 10:02 pm on March 23, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

12. 
    ariard commented at 6:57 am on April 28, 2020: member

    Closing for lack of interest. May reopen a rebased, cleaner version in the future. Feel free for grabs.
13. ariard closed this on Apr 28, 2020

    ---

14. in src/wallet/wallet.cpp:862 in cd15809a32 outdated

    857-            CWalletTx& prevtx = it->second;
    858-            if (prevtx.isConflicted()) {
    859-                MarkConflicted(prevtx.m_confirm.hashBlock, prevtx.m_confirm.block_height, wtx.GetHash());
    860-            }
    861-        }
    862-    }
    

    ---

    ---

    

    ryanofsky commented at 1:38 pm on January 11, 2021:

    In commit “Remove MarkConflicted in LoadToWallet” (cd15809a3213225849902dfd9d49b80b93da1d51)

    I think this code probably shouldn’t be removed. It has been around since #7105 and presumably it is needed to load old wallets from before #7105. Regardless whether this is kept or removed, there should test coverage to verify expected behavior. Loading a pre-#7105 wallet should either mark conflicted transactions, or it fail with a clear error message, but any behavior change should have a test.

15. ryanofsky added the label Up for grabs on Jan 11, 2021
16. 
    ryanofsky commented at 1:43 pm on January 11, 2021: member

    Marking up for grabs. This implements idea-refresh-conflicted from https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking and fixes some bugs described there
17. DrahtBot locked this on Aug 16, 2022
18. fanquake removed the label Up for grabs on Apr 11, 2023
19. fanquake removed the label Needs rebase on Apr 11, 2023
20. 
    fanquake commented at 9:55 am on April 11, 2023: member

    Dropped up for grabs here, see #27145.

Contributors
ariard DrahtBot ryanofsky fanquake

Labels
Wallet