wallet: track mempool conflicts with wallet transactions #27307

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

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.

7d55796 wallet: update mempool conflicts tests + docs (ishaanam) Pull request description: #27307 follow-up: - updates description of `mempoolconflicts` and `walletconflicts` in `gettransaction` - adds release notes for 27307 - removes unnecessary line from `wallet_conflicts.py` ACKs for top commit: fjahr: ACK 7d55796 achow101: ACK 7d55796 furszy: utACK 7d55796 tdb3: ACK 7d55796 Tree-SHA512: b3c368c7072cacdaf5fd18ecb0a88ab76ce02f65d56fce55a3316afa0989b9417c31e563aa8d9dd8f6294add154b4fdeb4ada5081c6b8a5fe9953f0e8a4812f4

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 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 requested a review from achow101 February 23, 2024 16:38

DrahtBot mentioned this pull request Feb 27, 2024

build: Enable -Wshadow #29485

Closed

ishaanam force-pushed the track_mempool_wallet_conflicts branch from 923734e to b853688 Compare February 28, 2024 06:04

DrahtBot removed the CI failed label Feb 28, 2024

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

ishaanam force-pushed the track_mempool_wallet_conflicts branch from b853688 to dbcc4a7 Compare March 14, 2024 21:50

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

ishaanam force-pushed the track_mempool_wallet_conflicts branch from dbcc4a7 to 5952292 Compare March 20, 2024 21:22

DrahtBot requested a review from ryanofsky March 26, 2024 19:37

ryanofsky approved these changes Mar 26, 2024

View reviewed changes

furszy reviewed Mar 27, 2024

View reviewed changes

ryanofsky merged commit c8e3978 into bitcoin:master Mar 27, 2024

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

bitcoin locked and limited conversation to collaborators Mar 27, 2025

wallet: track mempool conflicts with wallet transactions #27307

wallet: track mempool conflicts with wallet transactions #27307

Uh oh!

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!

DrahtBot commented Feb 22, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky Feb 23, 2024

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!

ryanofsky Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 29, 2024

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!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

furszy Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ishaanam commented Mar 22, 2023 •

edited

Loading

DrahtBot commented Mar 22, 2023 •

edited

Loading

furszy Mar 6, 2024 •

edited

Loading