wallet: when a block is disconnected, update transactions that are no longer conflicted #27145

ishaanam · 2023-02-22T19:00:38Z

This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the Bitcoin DevWiki. A test which tested the previous behavior has also been updated.

Second attempt at #17543

DrahtBot · 2023-02-22T19:00:41Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rajarshimaitra, achow101, glozow, furszy
Concept ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27307 (wallet: track mempool conflicts with wallet transactions by ishaanam)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)

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.

ryanofsky · 2023-03-01T17:19:12Z

Concept ACK on refreshing state of conflicted transactions in the wallet when a block is disconnected. I think there is a potential problem with the implementation though because it isn't recursively marking conflicted transactions as unconfirmed, only marking the top level transactions without recursing.

I think a good approach could be to steal from the previous implementation of this idea in commits b8b4592 and 3c8bd68 from #17543, which did recurse. That approach was also nice because it reused the existing MarkConflicted function in order to avoid code duplication. Commit ec5a89a from that PR also added some more test coverage which could be used here.

ishaanam · 2023-03-05T06:17:10Z

@ryanofsky

I think there is a potential problem with the implementation though because it isn't recursively marking conflicted transactions as unconfirmed, only marking the top level transactions without recursing.

Yes, I had forgotten about this, thanks for pointing it out!

I think a good approach could be to steal from the previous implementation of this idea in commits b8b4592 and 3c8bd68 from #17543, which did recurse.

I have used ideas from that PR so that the transaction state updating will recurse.

Commit ec5a89a from that PR also added some more test coverage which could be used here.

I have cherry-picked that commit to this branch with a few adjustments.

glozow

Concept ACK

There is a missing dash in "Co-authored-by:" in the commit messages

test/functional/wallet_conflicts.py

src/wallet/wallet.cpp

ishaanam · 2023-04-08T04:59:49Z

Thanks for the review @glozow, I have addressed your comments and also made a modification to RecursiveUpdateTxState which will allow it to be more extensible.

src/wallet/wallet.cpp

furszy

Haven't finished checking the latest changes but, if you like it, furszy@8bb14c1 is all yours. During the review, the functional test wasn't clear enough for me so I spent a bit of time making it more friendly.

furszy · 2023-04-12T01:22:21Z

src/wallet/wallet.cpp

+
+                if (!wtx.isConflicted()) continue;
+
+                auto try_updating_state = [&](CWalletTx& wtx) {


I would try to not provide the context by reference to the function. The lambda function's wtx shadows the function's wtx. Could just provide disconnect_height.

The lambda function's wtx shadows the function's wtx.

I have renamed the lambda function's wtx.

Could just provide disconnect_height.

By this do you mean providing the disconnect_height of the block to RecursiveUpdateTxState or providing the wallet tx's conflicting_block_height to try_updating_state instead of the whole wallet tx by reference?

src/wallet/wallet.cpp

furszy · 2023-04-13T19:45:39Z

src/wallet/wallet.cpp

            assert(!wtx.InMempool());
            wtx.m_state = TxStateInactive{/*abandoned=*/true};
-            wtx.MarkDirty();
-            batch.WriteTx(wtx);
            NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);


In f998cdb:

Would be good to move this NotifyTransactionChanged to RecursiveUpdateTxState too. So MarkConflicted and the BlockDisconnected txes state changes are notified to the upper layers.

I think that this is a good idea, but would be out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.

In commit "wallet: introduce generic recursive tx state updating function" (f998cdb)

I think that this is a good idea, but would out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.

Agree we should avoid changing behavior, but I think the current commit might also be changing behavior by notifying before calling MarkDirty and WriteTx instead of after.

I think the best thing to do would be to change TryUpdatingStateFn to return an enum instead of a bool like:

enum class TxUpdate { UNCHANGED, CHANGED, NOTIFY_CHANGED }; using TryUpdatingStateFn = std::function<TxUpdate(CWalletTx& wtx)>;

Returning CHANGED/UNCHANGED should make the code clearer anyway because it's less obvious what true and false mean.

I have implemented this suggestion, I agree that it is much clearer now.

ishaanam · 2023-04-14T06:25:57Z

Thanks for the review @achow101 and @furszy!

Haven't finished checking the latest changes but, if you like it, furszy@8bb14c1 is all yours. During the review, the functional test wasn't clear enough for me so I spent a bit of time making it more friendly.

Thanks, I'll use these improvements here once I review them.
Edit: I've added the suggested changes to the functional tests.

src/wallet/wallet.cpp

This commit also changed `MarkConflicted` and `AbandonTransaction` to use this new function Co-authored-by: ariard <antoine.riard@gmail.com>

ishaanam · 2023-04-22T15:08:34Z

Rebased to fix failing CI.

furszy

ACK 6f1d394

furszy · 2023-05-04T13:15:40Z

test/functional/wallet_abandonconflict.py

-        # There is currently a minor bug around this and so this test doesn't work.  See Issue #7315
-        # Invalidate the block with the double spend and B's 10 BTC output should no longer be available
-        # Don't think C's should either
+        # Invalidate the block with the double spend. B & C's 10 BTC outputs should no longer be available


Let me see if I'm following this:

Tx_AB1 spends B, and Tx_ABC2 (the child of Tx_AB1) spends C. So, when the double spend tx gets disconnected, Tx_AB1 state changes to inactive, marking output B and C as spent again.

Which ends up with the txes stuck now right?, and the user need to manually re-submit them.

Yes, the user would need to manually re-submit them or wait for the wallet to re-submit them automatically.

good. Not sure if there is any but would be nice to have coverage for the automatic re-submission. Would just need to mock the node time 24hs in the future and see if they get re-submitted.

There are some tests for that here: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_resendwallettransactions.py. Or did you mean that a test for that should be added for this PR specifically?

yeah, great if that is already covered.
(sorry for the delayed answer)

In `blockDisconnected`, for each transaction in the block, look for any wallet transactions spending the same inputs. If any of these transactions were marked conflicted, they are now marked as inactive. Co-authored-by: ariard <antoine.riard@gmail.com>

Test the case of a tx being conflicted by multiple txs with different depths. The conflicted tx is also spent by a child tx for which confirmation status is tied to the parent's. After a reorg of conflicting txs, the conflicted status should be undone properly. Co-authored-by: furszy <mfurszy@protonmail.com>

ishaanam · 2023-05-14T14:52:56Z

I've fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in mapTxSpends.

rajarshimaitra

tACK 89df798.

Changes look very clean. We reviewed it in out weekly club and went over the new code addition.

Ran the tests on the master branch and saw it failing assert_equal(conflicted["confirmations"], 0). In the master conflicting transaction's state doesn't update after reorg.

achow101 · 2023-05-25T17:02:50Z

ACK 89df798

furszy · 2023-05-26T02:15:37Z

I've fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in mapTxSpends.

Could you expand this?

Not sure if I follow the latest push, where only the inequality was changed.
If mapTxSpends.count(outpoint) == 1, the outpoint was spent only once, so the wallet shouldn't contain any conflicting tx.

glozow

ACK 89df798

ishaanam · 2023-05-27T02:08:29Z

If mapTxSpends.count(outpoint) == 1, the outpoint was spent only once, so the wallet shouldn't contain any conflicting tx.

This is not necessarily always true, because even though the conflicted wallet transaction must always have its spends added to mapTxSpends, this is not the case for all conflicting transactions.

For example, let's say that Alice has a utxo. She creates a transaction, tx1, spending this utxo to Bob. AddToSpends will get called on Bob's node for this transaction, because Bob is the recipient. Alice then creates a conflicting transaction to tx1, called tx2, which sends the funds back to herself. However, Bob's node does not call AddToSpends on tx2, because Bob is not the recipient. If tx2 gets mined, but then that block is disconnected from our node, Bob's mapTxSpends will only show a single spend for Alice's utxo in mapTxSpends, even though there are technically two.

furszy · 2023-05-27T15:28:46Z

Ok nice, great. Thanks for the details.

furszy

Tested ACK 89df798

For example, let's say that Alice has a utxo. She creates a transaction, tx1, spending this utxo to Bob. AddToSpends will get called on Bob's node for this transaction, because Bob is the recipient. Alice then creates a conflicting transaction to tx1, called tx2, which sends the funds back to herself. However, Bob's node does not call AddToSpends on tx2, because Bob is not the recipient. If tx2 gets mined, but then that block is disconnected from our node, Bob's mapTxSpends will only show a single spend for Alice's utxo in mapTxSpends, even though there are technically two.

Made a quick unit test and all good. Good finding.
Would be good to have a real functional test in the future.

ishaanam · 2023-05-27T16:08:09Z

Would be good to have a real functional test in the future.

#27307 tests this behavior, which is how I found this bug.

…nsactions that are no longer conflicted 89df798 Add wallets_conflicts (Antoine Riard) dced203 wallet, tests: mark unconflicted txs as inactive (ishaanam) 096487c wallet: introduce generic recursive tx state updating function (ishaanam) Pull request description: This implements a fix for bitcoin#7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the [Bitcoin DevWiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#idea-refresh-conflicted). A test which tested the previous behavior has also been updated. Second attempt at bitcoin#17543 ACKs for top commit: achow101: ACK 89df798 rajarshimaitra: tACK 89df798. glozow: ACK 89df798 furszy: Tested ACK 89df798 Tree-SHA512: 3133b151477e8818302fac29e96de30cd64c09a8fe3a7612074a34ba1a332e69148162e6cb3f1074d9d9c9bab5ef9996967d325d8c4c99ba42b5fe3b66a60546

…nsactio…

5952292 wallet, rpc: show mempool conflicts in `gettransaction` result (ishaanam) 54e07ee wallet: track mempool conflicts (ishaanam) d64922b wallet refactor: use CWalletTx member functions to determine tx state (ishaanam) ffe5ff1 scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted (ishaanam) 180973a test: Add tests for wallet mempool conflicts (ishaanam) Pull request description: The `mempool_conflicts` variable is added to `CWalletTx`, it is a set of txids of txs in the mempool conflicting with the wallet tx or a wallet tx's parent. This PR only changes how mempool-conflicted txs are dealt with in memory. `IsSpent` now returns false for an output being spent by a mempool conflicted transaction where it previously returned true. A txid is added to `mempool_conflicts` during `transactionAddedToMempool`. A txid is removed from `mempool_conflicts` during `transactionRemovedFromMempool`. This PR also adds a `mempoolconflicts` field to the `gettransaction` wallet RPC result. Builds on #27145 Second attempt at #18600 ACKs for top commit: achow101: ACK 5952292 ryanofsky: Code review ACK 5952292. Just small suggested changes since last review furszy: ACK 5952292 Tree-SHA512: 615779606723dbb6c2e302681d8e58ae2052ffee52d721ee0389746ddbbcf4b4c4afacf01ddf42b6405bc6f883520524186a955bf6b628fe9b3ae54cffc56a29

DrahtBot added the Wallet label Feb 22, 2023

DrahtBot mentioned this pull request Feb 23, 2023

wallet: group independent db writes on single batched db transaction #25297

Closed

ishaanam force-pushed the mark_not_conflicted_inactive branch 2 times, most recently from 8f8990f to 77a2e52 Compare March 5, 2023 03:54

DrahtBot mentioned this pull request Mar 21, 2023

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

achow101 added this to the 25.0 milestone Mar 23, 2023

glozow reviewed Apr 6, 2023

View reviewed changes

ishaanam force-pushed the mark_not_conflicted_inactive branch from 77a2e52 to 9895be8 Compare April 8, 2023 04:53

DrahtBot mentioned this pull request Apr 8, 2023

wallet: track mempool conflicts with wallet transactions #27307

Merged

fanquake mentioned this pull request Apr 11, 2023

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

Closed

achow101 reviewed Apr 11, 2023

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

ishaanam force-pushed the mark_not_conflicted_inactive branch from 9895be8 to 1a5d998 Compare April 11, 2023 18:36

furszy reviewed Apr 11, 2023

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

ishaanam force-pushed the mark_not_conflicted_inactive branch from 1a5d998 to a90f3b6 Compare April 11, 2023 20:42

furszy reviewed Apr 12, 2023

View reviewed changes

achow101 removed this from the 25.0 milestone Apr 13, 2023

furszy reviewed Apr 13, 2023

View reviewed changes

ishaanam force-pushed the mark_not_conflicted_inactive branch from a90f3b6 to 5a31a45 Compare April 14, 2023 06:11

ishaanam force-pushed the mark_not_conflicted_inactive branch from 5a31a45 to fc48023 Compare April 14, 2023 21:08

ryanofsky reviewed Apr 17, 2023

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

fanquake added this to the 26.0 milestone Apr 18, 2023

ishaanam force-pushed the mark_not_conflicted_inactive branch 3 times, most recently from e6b0136 to e320f49 Compare April 18, 2023 22:14

furszy reviewed Apr 22, 2023

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

wallet: introduce generic recursive tx state updating function

096487c

This commit also changed `MarkConflicted` and `AbandonTransaction` to use this new function Co-authored-by: ariard <antoine.riard@gmail.com>

ishaanam force-pushed the mark_not_conflicted_inactive branch from e320f49 to 6f1d394 Compare April 22, 2023 15:06

furszy approved these changes May 4, 2023

View reviewed changes

ishaanam and others added 2 commits May 14, 2023 10:45

ishaanam force-pushed the mark_not_conflicted_inactive branch from 6f1d394 to 89df798 Compare May 14, 2023 14:45

rajarshimaitra mentioned this pull request May 14, 2023

wallet: when a block is disconnected, update transactions that are no longer conflicted #27145 Bitshala/BitcoinCore-PR-Review-Club#25

Closed

rajarshimaitra reviewed May 22, 2023

View reviewed changes

DrahtBot requested a review from furszy May 22, 2023 11:27

achow101 requested a review from ryanofsky May 22, 2023 20:40

glozow reviewed May 26, 2023

View reviewed changes

furszy approved these changes May 27, 2023

View reviewed changes

achow101 merged commit 7d33ae7 into bitcoin:master May 27, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 29, 2023

Merge bitcoin#27145: wallet: when a block is disconnected, update tra…

5cada6e

…nsactio…

ishaanam deleted the mark_not_conflicted_inactive branch November 30, 2023 03:16

bitcoin locked and limited conversation to collaborators Nov 29, 2024


		if (!wtx.isConflicted()) continue;

		auto try_updating_state = [&](CWalletTx& wtx) {

wallet: when a block is disconnected, update transactions that are no longer conflicted #27145

wallet: when a block is disconnected, update transactions that are no longer conflicted #27145

Uh oh!

Conversation

ishaanam commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ryanofsky commented Mar 1, 2023

Uh oh!

ishaanam commented Mar 5, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ishaanam commented Apr 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishaanam Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ishaanam commented Apr 22, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishaanam commented May 14, 2023

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 25, 2023

Uh oh!

ishaanam commented Feb 22, 2023 •

edited

Loading

DrahtBot commented Feb 22, 2023 •

edited

Loading

ishaanam Apr 14, 2023 •

edited

Loading

ishaanam commented Apr 14, 2023 •

edited

Loading

furszy commented May 27, 2023 •

edited

Loading