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

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 c891a65.

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

@@ -755,7 +755,7 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
            // An output is considered spent if a spending transaction
            // is either confirmed, or in the mempool, or inactive (but not abandoned)
            const auto& wtx = mit->second;
-            if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))
+            if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned() && !wtx.isMempoolConflicted()))


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

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" (a650caa)

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 923734e. 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 a650caa to previous a6ae5b2, 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

@@ -755,7 +755,7 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
            // An output is considered spent if a spending transaction
            // is either confirmed, or in the mempool, or inactive (but not abandoned)
            const auto& wtx = mit->second;
-            if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))
+            if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned() && !wtx.isMempoolConflicted()))


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

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 b853688. 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

            // check for the inverse condition and only consider spending
            // transactions in known, potentially active states.
            const auto& wtx = mit->second;
-            if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))
+            if (!wtx.isAbandoned() && !wtx.isMempoolConflicted() && !wtx.isBlockConflicted())


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

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

+            // An output is considered spent by a spending transaction
+            // unless the spending transaction is conflicted or
+            // abandoned. The check below is written conservatively to
+            // check for the inverse condition and only consider spending
+            // transactions in known, potentially active states.
+            const auto& wtx = mit->second;
+            if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))


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

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

+        raw_tx = alice.createrawtransaction(inputs=[unspents[1]], outputs=[{alice.getnewaddress() : 24.9999}])
+        tx2_conflict = alice.signrawtransactionwithwallet(raw_tx)['hex']
+
+        bob_unspents = [{"txid" : element, "vout" : 0} for element in [tx1_txid, tx2_txid]]


In d3f7764:

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 dbcc4a7. 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

@@ -417,6 +421,10 @@ static std::vector<RPCResult> TransactionDescriptionString()
           }},
           {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

Open

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

Draft

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

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

Draft

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

Open

ryanofsky approved these changes Mar 15, 2024

View reviewed changes

src/wallet/interfaces.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

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

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

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

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

Open

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
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: track mempool conflicts with wallet transactions #27307

wallet: track mempool conflicts with wallet transactions #27307

ishaanam commented Mar 22, 2023 •

edited

DrahtBot commented Mar 22, 2023 •

edited

ishaanam commented Feb 11, 2024

ryanofsky left a comment

ryanofsky Feb 15, 2024

ishaanam Feb 22, 2024

ryanofsky Feb 23, 2024

DrahtBot commented Feb 22, 2024

ryanofsky left a comment

ryanofsky Feb 23, 2024

ryanofsky left a comment

ryanofsky Feb 29, 2024

ryanofsky Feb 29, 2024

furszy left a comment

furszy Mar 6, 2024 •

edited

ryanofsky left a comment

achow101 commented Mar 26, 2024

ryanofsky left a comment

furszy left a comment

furszy Mar 27, 2024

furszy Mar 27, 2024

ryanofsky commented Mar 27, 2024

wallet: track mempool conflicts with wallet transactions #27307

wallet: track mempool conflicts with wallet transactions #27307

Conversation

ishaanam commented Mar 22, 2023 • edited

DrahtBot commented Mar 22, 2023 • edited

Code Coverage

Reviews

Conflicts

ishaanam commented Feb 11, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Feb 22, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

furszy Mar 6, 2024 • edited

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

achow101 commented Mar 26, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky commented Mar 27, 2024

ishaanam commented Mar 22, 2023 •

edited

DrahtBot commented Mar 22, 2023 •

edited

furszy Mar 6, 2024 •

edited