wallet: undo conflicts properly in case of blocks disconnection #17543
Conversation
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.
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.
Extend SyncTransaction and AddToWalletIfInvolvingMe to check that disconnected conflicting tx was the deepest one and conflicted tx status can be safely change to unconfirmed.
ariard
changed the title
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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.
ariard
force-pushed
the
2019-11-fix-wallet-unconflicts
branch
from
f6f00dd to
ec5a89a
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Closing for lack of interest. May reopen a rebased, cleaner version in the future. Feel free for grabs. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
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
| for (const CTxIn& txin : wtx.tx->vin) { | ||
| auto it = mapWallet.find(txin.prevout.hash); | ||
| if (it != mapWallet.end()) { | ||
| CWalletTx& prevtx = it->second; | ||
| if (prevtx.isConflicted()) { | ||
| MarkConflicted(prevtx.m_confirm.hashBlock, prevtx.m_confirm.block_height, wtx.GetHash()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In commit "Remove MarkConflicted in LoadToWallet" (cd15809)
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.
|
Dropped up for grabs here, see #27145. |
4 participants
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
CONFLICTEDinstead of being marked again asUNCONFIRMED. 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
MarkConflictedwhich is renamedUpdatedConflictsto cover both instances of unconfirmed to conflicted and conflicted to unconfirmed cases.Lambda is used, but I'm open to other approaches.