wallet: track mempool conflicts with wallet transactions by ishaanam · Pull Request #27307 · bitcoin/bitcoin

ishaanam · 2023-03-22T21:30:01Z

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

DrahtBot · 2023-03-22T21:30:09Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, ryanofsky, furszy
Approach ACK	glozow
Stale ACK	murchandamus

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:

#29680 (wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict by Eunovo)
#28616 (Show transactions as not fully confirmed during background validation by Sjors)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#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.

src/wallet/transaction.h

ryanofsky · 2024-02-09T17:21:32Z

re: #27307 (comment)

Thanks ishaanam for pointing out the lines of code that answer my questions. Since original links got broken in rebase, here are stable links that point to the two lines of code which prioritize the abandoned state over the conflicted state:

This is better than what I was afraid of, since the abandoned state consistently takes priority over the mempoolconflicted state. But this behavior is fragile and unnecessarily complicates our internal model and potential RPC and GUI interfaces, because there's no logical reason for the abandoned and mempool conflicted states to contradict each other, when both can be true at the same time.

If we take the current approach in 3471ebc8c6ee268ad501525121ea3f3a221fb5ad, we will have to be careful going forward, whenever we do any state change, to remember that the abandoned state is supposed to take priority over the mempool conflicted state (this is not even documented anywhere), and decide how we want to deal with potentially odd states like mempool_conflicts set being nonempty when the transaction is not in the MempoolConflicted state.

For background, I already think it's bad that abandoned and block-conflicted states exclude each other, and that the block conflicted state takes priority over the abandoned state and resets it. But the block conflicted state is pretty permanent, so it's basically just a UI quirk if a block conflict causes the abandoned state to be forgotten about. There are also backwards compatibility benefits to storing the block conflict and abandoned bit exclusively in the same field.

In the current case, though, there are no backwards compatibility concerns, so it seems wrong and unnecessary to go out our way to create an unenforced, undocumented, and confusing hierarchy of states: MempoolConflicted < Abandoned < BlockConflicted, when there is a simpler alternative of just straightforwardly representing the information we have about transactions directly.

It seems pretty clear to me that it would make this change simpler, safer and easier to review, make the code more straightforward and maintainable long term, and avoid potential bugs and confusion in the UI if it just represented abandoned and mempool conflicted states independently and didn't try to establish a hierarchy between them.

I think a good approach would avoid adding a TxStateMempoolConflicted top-level state, and just add a bool mempool_conflicts or std::set<Txid> mempool_conflicts member to the TxStateInactive struct. And it is good to rename TxStateConflicted to TxStateBlockConflicted too for clarity. If you're skeptical of this approach, I'd be curious to know why, but we can also experiment in a separate PR if you are concerned there are benefits in this PR which would be lost.

If it would be helpful, I could make another PR next week to compare the two approaches. But I'd be curious to know what you think. There is a lot of a complexity and a long history of bugs in wallet transaction state tracking (https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking) so I think think it is worth taking a little more time to get this right.

ishaanam · 2024-02-11T05:19:58Z

I think a good approach would avoid adding a TxStateMempoolConflicted top-level state, and just add a bool mempool_conflicts or std::set mempool_conflicts member to the TxStateInactive struct.

Thanks for your feedback, I have implemented this.

ryanofsky

Code review c891a65067f57ba87e443d0cdc4f7e6b434c436f.

Thanks for the updates! A lot of nice changes and test coverage here, and I think the PR looks close to ready to ACK, though I did leave a number of suggestions below. Most of these should be very simple or simplify code, so should not create too much work.

src/wallet/transaction.h

src/wallet/wallet.cpp

src/wallet/transaction.h

ryanofsky · 2024-02-15T18:35:31Z

src/wallet/wallet.cpp

In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)

I was surprised to see this change, since I thought intent of the preceding commit was to preserve current behavior and consider spending transactions active even they conflicted with mempool transactions. Both approaches seem ok, but if this behavior is intended, it seems like it would be simpler to write this condition as:

if (!wtx.IsAbandoned() && !wtx.isConflicted()) { return true; // spent }

The preceding was more of a refactor, while this commit aims to have IsSpent return false for txos spent by mempool-conflicted transactions.

In commit "wallet: track mempool conflicts" (a650caab9f11be7f5927f9aa556bb7d2a8ebed33)

The preceding was more of a refactor, while this commit aims to have IsSpent return false for txos spent by mempool-conflicted transactions.

Sounds good. I guess I'm still curious why you prefer over:

if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned() && !wtx.isMempoolConflicted())

over

if (!wtx.IsAbandoned() && !wtx.isConflicted())

since both checks should equivalent. Either approach seems fine to me, though.

src/wallet/transaction.h

src/wallet/wallet.cpp

test/functional/wallet_conflicts.py

DrahtBot · 2024-02-22T23:52:46Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21886408495}

ryanofsky

Code review ACK 923734ef6528e5d4e13748db7387f44d02237d4d. Seems like a very nice change that improves the wallet balance calculation and ability to spend, and also returns useful information about the mempool.

Thanks for the updates, too. I think comparing current a650caab9f11be7f5927f9aa556bb7d2a8ebed33 to previous a6ae5b23b1497ab6f4899db49348db623700a2d8, the resulting code is simpler and the charges are more direct and easier to review.

I left some more suggestions below, but most are not important and none are critical so feel free to ignore them.

src/wallet/transaction.h

src/wallet/receive.cpp

ryanofsky · 2024-02-23T15:45:16Z

src/wallet/wallet.cpp

In commit "wallet: track mempool conflicts" (a650caab9f11be7f5927f9aa556bb7d2a8ebed33)

The preceding was more of a refactor, while this commit aims to have IsSpent return false for txos spent by mempool-conflicted transactions.

Sounds good. I guess I'm still curious why you prefer over:

if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned() && !wtx.isMempoolConflicted())

over

if (!wtx.IsAbandoned() && !wtx.isConflicted())

since both checks should equivalent. Either approach seems fine to me, though.

src/wallet/wallet.cpp

test/functional/wallet_conflicts.py

ryanofsky

Code review ACK b853688c92d3745d5ec2ec5119584148084e7c1c. Just more simplifications since that last review. This PR keeps getting smaller and simpler, and seems like a very obvious improvement now.

I left another suggestion (really 2 related suggestions), but it is not very important. This looks good in its current form.

src/wallet/wallet.cpp

ryanofsky · 2024-02-29T19:14:50Z

src/wallet/wallet.cpp

In commit "wallet: track mempool conflicts" (9fa7f813281d8520a5eb5adbd29fe3a0d2ab5a4f)

Would suggest deleting the "An output is considered spent..." comment above now that the code is simpler and it no longer adds any new information. The part of the comment about checking for the "inverse condition" is also no longer true, because the code is now checking directly if wtx is conflicted or abandoned, instead of checking for the inverse condition (that it is confirmed, or in mempool, or inactive and not abandoned).

ryanofsky · 2024-02-29T19:18:20Z

src/wallet/wallet.cpp

In commit "wallet: use CWalletTx member functions to determine tx state" (92b3204b69e5ec25cbb29bd158f25d2b07e48cf6)

Would suggest simplifying this code and deleting the comment so this is just:

const auto& wtx = mit->second; if (!wtx.isAbandoned() && !wtx.isBlockConflicted())

Using the simpler condition would make this commit easier to understand, and make the change in the next commit more obvious where the line becomes:

if (!wtx.isAbandoned() && !wtx.isMempoolConflicted() && !wtx.isBlockConflicted())

furszy

left few small things. Will get deeper.

test/functional/wallet_conflicts.py

furszy · 2024-03-06T22:42:43Z

test/functional/wallet_conflicts.py

In d3f7764c802fe:

Tiny note for reviewers:
vout is always 0 because "alice only contain 3 utxo of 25 btc each. So, tx1 and tx2 are changeless transactions. (could be good to mention this in the code too)

src/wallet/wallet.cpp

test/functional/wallet_conflicts.py

src/wallet/wallet.h

src/wallet/wallet.cpp

-BEGIN VERIFY SCRIPT- sed -i 's/TxStateConflicted/TxStateBlockConflicted/g' src/wallet/wallet.cpp src/wallet/interfaces.cpp src/wallet/transaction.h src/wallet/transaction.cpp sed -i 's/isConflicted/isBlockConflicted/g' src/wallet/transaction.h src/wallet/wallet.cpp -END VERIFY SCRIPT-

ryanofsky

Code review ACK dbcc4a75e4a1108f4a04c22cfae363848994c24d. Changes since last review were a few test improvements and some more simplifications. This looks very good. Left suggestions below, all minor, none important. This PR seems like a clear improvement and should be merged if it gets more ACKs.

src/wallet/interfaces.cpp

src/wallet/wallet.cpp

Behavior changes are: - if a tx has a mempool conflict, the wallet will not attempt to rebroadcast it - if a txo is spent by a mempool-conflicted tx, that txo is no longer considered spent

achow101 · 2024-03-26T19:36:57Z

ACK 5952292

ryanofsky

Code review ACK 5952292. Just small suggested changes since last review

furszy

ACK 5952292

furszy · 2024-03-27T12:56:45Z

test/functional/wallet_conflicts.py

+        """
+
+        self.test_block_conflicts()
+        self.generatetoaddress(self.nodes[0], COINBASE_MATURITY + 7, self.nodes[2].getnewaddress())


nit:
Would be good to mention why this generatetoaddress is needed. I removed it locally and the test still passes.

furszy · 2024-03-27T13:06:45Z

src/wallet/rpc/transactions.cpp

           }},
           {RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx was replaced."},
           {RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx replaces another."},
+           {RPCResult::Type::ARR, "mempoolconflicts", "Transactions that directly conflict with either this transaction or an ancestor transaction",


Not for this PR just so you don't have to re-touch it but it would be good to describe the difference between walletconflicts and mempoolconflicts inside the help text. walletconflicts description is quite vague.

ryanofsky · 2024-03-27T17:00:09Z

This is merged but it would be good to followup with documentation / test improvements from #27307 (review).

Might also be useful to add release notes saying the wallet has a new heuristic to detect when wallet transactions conflict with the mempool, that conflicting mempool transactions are shown in a new gettransaction "mempoolconflicts" field, that this allows inputs to the conflicted transactions to be respent without manually abandoning the transactions, and can cause wallet balances to appear higher.

DrahtBot added the Wallet label Mar 22, 2023

This was referenced Mar 23, 2023

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

Merged

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

Closed

DrahtBot mentioned this pull request Apr 8, 2023

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

Merged

fanquake mentioned this pull request Apr 11, 2023

[wallet] Track conflicted transactions removed from mempool and fix UI notifications #18600

Closed

ishaanam force-pushed the track_mempool_wallet_conflicts branch 2 times, most recently from ef72243 to f153a00 Compare April 13, 2023 06:59

ishaanam force-pushed the track_mempool_wallet_conflicts branch from f153a00 to ce757a5 Compare May 3, 2023 00:02

DrahtBot added the CI failed label May 3, 2023

ishaanam force-pushed the track_mempool_wallet_conflicts branch 3 times, most recently from 1479572 to d9356d2 Compare May 15, 2023 00:06

DrahtBot removed the CI failed label May 15, 2023

ishaanam force-pushed the track_mempool_wallet_conflicts branch from d9356d2 to 03dc162 Compare May 18, 2023 00:58

DrahtBot added the CI failed label May 18, 2023

DrahtBot added the Needs rebase label May 27, 2023

ishaanam force-pushed the track_mempool_wallet_conflicts branch from 03dc162 to f151405 Compare May 28, 2023 01:23

ishaanam marked this pull request as ready for review May 28, 2023 02:26

DrahtBot removed CI failed Needs rebase labels May 28, 2023

This was referenced Jun 5, 2023

Silent Payments: send and receive #27827

Closed

wallet: Track no-longer-spendable TXOs separately #27865

Open

ishaanam force-pushed the track_mempool_wallet_conflicts branch from f151405 to d4cbfee Compare June 15, 2023 20:37

DrahtBot added the CI failed label Jun 15, 2023

ishaanam force-pushed the track_mempool_wallet_conflicts branch 3 times, most recently from 8c1773b to 0538ad7 Compare June 18, 2023 17:16

DrahtBot removed the CI failed label Jun 18, 2023

evansmj reviewed Jun 28, 2023

View reviewed changes

src/wallet/transaction.h Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Feb 9, 2024

ishaanam force-pushed the track_mempool_wallet_conflicts branch from 3471ebc to 09bc269 Compare February 9, 2024 17:18

DrahtBot removed the CI failed label Feb 9, 2024

ishaanam force-pushed the track_mempool_wallet_conflicts branch from 09bc269 to c891a65 Compare February 11, 2024 05:15

ryanofsky reviewed Feb 15, 2024

View reviewed changes

ishaanam force-pushed the track_mempool_wallet_conflicts branch from c891a65 to 923734e Compare February 22, 2024 22:47

DrahtBot added the CI failed label Feb 22, 2024

DrahtBot mentioned this pull request Feb 23, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

ryanofsky approved these changes Feb 23, 2024

View reviewed changes

DrahtBot mentioned this pull request Feb 27, 2024

build: Enable -Wshadow #29485

Closed

ryanofsky approved these changes Feb 29, 2024

View reviewed changes

furszy reviewed Mar 7, 2024

View reviewed changes

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

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

ishaanam added 2 commits March 14, 2024 17:09

test: Add tests for wallet mempool conflicts

180973a

DrahtBot mentioned this pull request Mar 15, 2024

Show transactions as not fully confirmed during background validation #28616

Closed

ryanofsky approved these changes Mar 15, 2024

View reviewed changes

wallet refactor: use CWalletTx member functions to determine tx state

d64922b

DrahtBot mentioned this pull request Mar 20, 2024

wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict #29680

Closed

ishaanam added 2 commits March 20, 2024 15:05

wallet: track mempool conflicts

54e07ee

Behavior changes are: - if a tx has a mempool conflict, the wallet will not attempt to rebroadcast it - if a txo is spent by a mempool-conflicted tx, that txo is no longer considered spent

wallet, rpc: show mempool conflicts in gettransaction result

5952292

ryanofsky approved these changes Mar 26, 2024

View reviewed changes

furszy reviewed Mar 27, 2024

View reviewed changes

ishaanam mentioned this pull request Jun 30, 2024

#27307 follow-up: update mempool conflict tests + docs #30365

Merged

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

ryanofsky commented Feb 9, 2024

Uh oh!

ishaanam commented Feb 11, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Feb 22, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy Mar 6, 2024 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Mar 26, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Mar 22, 2023 •

edited

Loading

DrahtBot commented Mar 22, 2023 •

edited

Loading

furszy Mar 6, 2024 •

edited

Loading