wallet: track mempool conflicts with wallet transactions #27307
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
ef72243 to
f153a00
Compare
f153a00 to
ce757a5
Compare
1479572 to
d9356d2
Compare
d9356d2 to
03dc162
Compare
03dc162 to
f151405
Compare
f151405 to
d4cbfee
Compare
8c1773b to
0538ad7
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
There was a problem hiding this comment.
Code review AC 8000 K 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/wallet.cpp
Outdated
| @@ -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())) | |||
There was a problem hiding this comment.
In commit "wallet: track mempool conflicts" (a650caa)
The preceding was more of a refactor, while this commit aims to have
IsSpentreturn 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.
923734e to
b853688
Compare
There was a problem hiding this comment.
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
Outdated
| // 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()) |
There was a problem hiding this comment.
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).
src/wallet/wallet.cpp
Outdated
| // 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())) |
There was a problem hiding this comment.
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())| 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]] |
There was a problem hiding this comment.
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)
-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-
b853688 to
dbcc4a7
Compare
There was a problem hiding this comment.
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.
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
dbcc4a7 to
5952292
Compare
|
ACK 5952292 |
| """ | ||
|
|
||
| self.test_block_conflicts() | ||
| self.generatetoaddress(self.nodes[0], COINBASE_MATURITY + 7, self.nodes[2].getnewaddress()) |
There was a problem hiding this comment.
nit:
Would be good to mention why this generatetoaddress is needed. I removed it locally and the test still passes.
| @@ -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", | |||
There was a problem hiding this comment.
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.
|
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 |
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
The
mempool_conflictsvariable is added toCWalletTx, 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.IsSpentnow returns false for an output being spent by a mempool conflicted transaction where it previously returned true.A txid is added to
mempool_conflictsduringtransactionAddedToMempool. A txid is removed frommempool_conflictsduringtransactionRemovedFromMempool.This PR also adds a
mempoolconflictsfield to thegettransactionwallet RPC result.Builds on #27145
Second attempt at #18600