[WIP] wallet: tx creation, don't select outputs from txes that are being replaced #26732

furszy · 2022-12-20T20:51:17Z

Depends on #27601.

Adding test coverage and the needed checks for two scenarios
where the wallet create invalid transactions.

First Scenario, Do Not Use Outputs From the Tx Being Replaced

When the wallet creates/fund a replacement transaction, it should
not fund the new transaction with outputs belonging to the tx
being replaced.

Reason:
Once replaced, those outputs are no longer going to exist, there
by the created/funded transaction will be invalid.

Second Scenario, Do Not Create BIP125 Rule 2 Invalid Txes

Prevent adding new unconfirmed outputs to the transaction being
created/funded If any preset input was already spent by a mempool
transaction.

Note:
Both scenarios can can be verified cherry-picking the tests commits
on master. They will fail.

DrahtBot · 2022-12-20T20:51:20Z

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
Concept ACK	glozow, 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:

#bitcoin-core/gui/807 (refactor: interfaces, make 'createTransaction' less error-prone by furszy)
#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)
#25269 (wallet: re-activate "AmountWithFeeExceedsBalance" error by furszy)
#21283 (Implement BIP 370 PSBTv2 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.

achow101 · 2022-12-20T22:44:14Z

To be clear, this is if you make a replacement manually, not with bumpfee. bumpfee avoids this problem by setting a minimum confirmations in the coin control object. IIRC BIP 125 disallows adding new unconfirmed inputs too.

furszy · 2022-12-21T14:23:22Z

To be clear, this is if you make a replacement manually, not with bumpfee. bumpfee avoids this problem by setting a minimum confirmations in the coin control object.

Yes, could happen if you manually pre-select an input from an unconfirmed transaction and provides it to any RPC command that creates or fund a transaction (e.g. send()). Not an issue for bumpfee as it skips unconfirmed UTXOs.

IIRC BIP 125 disallows adding new unconfirmed inputs too.

You mean, grouping inputs from different unconfirmed transactions into a single replacement tx?
(in other words, a single tx that replaces many unconfirmed txes).

So far, what the test proves is that the wallet, during the transaction creation process, can automatically select outputs from the unconfirmed transaction that is being replaced to use them as inputs. Which shouldn't happen as those coins will no longer exist once the tx gets replaced.

achow101 · 2022-12-21T16:06:48Z

You mean, grouping inputs from different unconfirmed transactions into a single replacement tx?
(in other words, a single tx that replaces many unconfirmed txes).

Unconfirmed inputs that were in the original transactions being replaced are fine. It's adding new unconfirmed inputs that is not allowed.

furszy · 2022-12-22T14:15:33Z

You mean, grouping inputs from different unconfirmed transactions into a single replacement tx?
(in other words, a single tx that replaces many unconfirmed txes).

Unconfirmed inputs that were in the original transactions being replaced are fine. It's adding new unconfirmed inputs that is not allowed.

Good to know. Thanks, it's BIP125 rule 2.

The wallet can create invalid transactions due not be contemplating that neither.
Same as with the other case, could happen on any of the "send*" and "fund*" RPC commands, when the user preselects an output that is being used as input for another unconfirmed transaction.

So, added test coverage and internal checks for it as well.

Summary:
Now this PR adds support and test coverage for two different scenarios where the wallet creates invalid transactions.

If is creating a replacement tx; in the available coins fetching process, the wallet need to skip the outputs that corresponds to the transaction being replaced (those coins are no longer going to exist once it's replaced, there by they are not "available").
Prevent adding new unconfirmed outputs to the transaction being created/funded If any preset input was already spent by a mempool transaction. (BIP125 rule 2).

glozow

So far, what the test proves is that the wallet, during the transaction creation process, can automatically select outputs from the unconfirmed transaction that is being replaced to use them as inputs. Which shouldn't happen as those coins will no longer exist once the tx gets replaced.

Indeed this sounds like a bug that should be fixed.
Would it be better to stop + warn the user that they have selected an input that is already spent by something in this mempool, and, if this is intentional, they should use bumpfee RPC instead? Otherwise you end up implementing RBF checks in 2 places?

glozow · 2022-12-23T13:36:38Z

src/wallet/spend.cpp

        }
    }

+    // If this tx is a replacement, disallow unconfirmed UTXO selection by rising the
+    // coin control min depth param. Ensuring that we don't add any new unconfirmed UTXOs
+    // to replacement transactions (satisfying BIP125 rule 2).


please do not add new mentions of BIP125 that aren't signaling-related, see #25775

furszy · 2022-12-26T21:01:26Z

So far, what the test proves is that the wallet, during the transaction creation process, can automatically select outputs from the unconfirmed transaction that is being replaced to use them as inputs. Which shouldn't happen as those coins will no longer exist once the tx gets replaced.

Indeed this sounds like a bug that should be fixed. Would it be better to stop + warn the user that they have selected an input that is already spent by something in this mempool, and, if this is intentional, they should use bumpfee RPC instead? Otherwise you end up implementing RBF checks in 2 places?

That is a good point and topic.

Another point of view would be to understand bumpfee as the sendtoaddress of the replacement transaction creation commands: the simplest and most straightforward way to create a replacement transaction, where regular users provide few inputs and the command just work. Without almost any possible customization etc.

Then, under this rationale, instead of blocking replacement txes from happening in lower level commands like fundrawtransaction, walletcreatefundedpsbt etc, it might be good to unify code (not duplicate the checks, which would be ugly for sure) and enable the same RBF validations on other processes too.
Only performing RBF checks if the crafted/funded transaction has at least one already spent input somewhere else (which only could happens if the user manually preset the input/s).

Which, based on a brief walk-through the bumpfee process, might not be that bad as we are duplicating some code that is already inside the general fundtransaction function.

Then, for advanced users, this view might be more adequate as otherwise we would need to add more customization options inside bumpfee so it provides almost the same functionality as the other lower level commands (e.g. be able to bump a tx that have at least one input that does not belong to the wallet, be able to add new confirmed inputs manually, modify an output value, provide an input weight manually, among other "not standard for bump txes" flows that advanced users might want to do).

Still.. have to say that this comes from an out loud thinking.. atm, I don't have a strong preference over any approach:
The blocking approach is reasonable as it would make our life easier for the time being but then, if users start requesting tx customizations, we will end up adding lot of stuff into bumpfee that is already somewhere else. Which will force us to unify code anyway.

glozow

Concept ACK, apologies for the late re-review

Just to recap the whiteboarding session we had offline, I think what we want is to add a SanitizeCoinControl function that can be used early on across both bumpfee and fundraw RPCs:

If any of the selected inputs is already spent in mempool, is_replacement=true and keep track of which inputs are being spent again.
If the selected inputs includes UTXOs spent and created by the same transaction (i.e. selected output from a tx that is being replaced), return error "invalid input selection"
If is_replacement=true, automatically set min_depth=1
If is_replacement and any of the selected inputs is <min_depth but not part of the replaced tx, return error "invalid input selection" (i.e. selected an additional unconfirmed output in an RBF)

The transactions should fail regardless, but this gives the user a helpful "invalid input selection" to let them know that they have manually selected some inputs that don't make sense.

murchandamus

Concept ACK

furszy

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.

It is still relevant. I'm waiting for #27601 outcome to rebase it. This work depends on it.

maflcko · 2023-12-11T15:53:05Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.

It is still relevant. I'm waiting for #27601 outcome to rebase it. This work depends on it.

Is there an outcome? Seems like nothing has happened on the pull since, other than it needing rebase.

furszy · 2023-12-12T14:01:03Z

Is there an outcome? Seems like nothing has happened on the pull since, other than it needing rebase.

Not yet. Still need to rework #27601 first. Other wallet works have gotten more priority than this working path. This two PRs are still bugs within the wallet but they could wait a bit in favor of other PRs.
I don't have a fixed time to get back to this work, but will try to do it after new year. We should be able to merge few of the ongoing big PRs by then.

When the wallet creates/fund a replacement transaction (spending one or more inputs of one or many existent transactions), it should not fund the new transaction with outputs belonging to the txes being replaced. Reason: Once replaced, those outputs are no longer going to exist, there by the created/funded transaction will be invalid.

The wallet tx creation process should not select any output belonging to the transaction that is being replaced.

Ensuring that we don't add any new unconfirmed UTXOs to the replacement transaction (satisfying BIP125 rule 2). note: needed to slightly adapt the 'test_add_inputs_default_value' functional test case because it was creating a replacement tx and adding new unconfirmed inputs without noticing it (we are testing a totally different things there).

the wallet should skip unconfirmed outputs when it is creating/funding a replacement tx Per BIP125 rule 2, the replacement tx must only have unconfirmed inputs from the original transaction that is being replaced. Cannot contain new unconfirmed inputs.

DrahtBot · 2024-12-03T01:52:14Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

furszy · 2024-12-30T19:29:50Z

Closing for now. Might revive it in the future, post #27286.

murchandamus · 2025-07-07T20:54:43Z

@furszy: #27286 was merged, in case this is still relevant.

furszy · 2025-07-07T21:23:27Z

@furszy: #27286 was merged, in case this is still relevant.

Thx. It is still relevant but I don't have enough time to tackle it. Up for grabs.

DrahtBot added the Wallet label Dec 20, 2022

DrahtBot mentioned this pull request Dec 21, 2022

rpc: add minconf/maxconf options to sendall and fund transaction calls #25375

Merged

furszy force-pushed the 2022_wallet_do_not_select_utxo_from_the_tx_being_replaced branch from ca9bacd to c288c04 Compare December 21, 2022 22:42

DrahtBot mentioned this pull request Dec 22, 2022

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

furszy force-pushed the 2022_wallet_do_not_select_utxo_from_the_tx_being_replaced branch from c288c04 to 431387b Compare December 22, 2022 13:58

furszy force-pushed the 2022_wallet_do_not_select_utxo_from_the_tx_being_replaced branch 2 times, most recently from f82e7ba to d7e41b6 Compare December 22, 2022 19:54

DrahtBot mentioned this pull request Dec 23, 2022

Implement BIP 370 PSBTv2 #21283

Open

glozow reviewed Dec 23, 2022

View reviewed changes

This was referenced Dec 28, 2022

wallet: Replace GetBalance() logic with AvailableCoins() #26756

Closed

wallet, refactor: FundTransaction(): return out-params as util::Result structure #26129

Closed

DrahtBot mentioned this pull request Jan 6, 2023

RPC: Accept options as named-only parameters #26485

Merged

DrahtBot added the Needs rebase label Jan 16, 2023

achow101 requested review from glozow and josibake April 25, 2023 15:45

achow101 marked this pull request as draft April 25, 2023 15:45

glozow reviewed May 1, 2023

View reviewed changes

furszy mentioned this pull request May 8, 2023

wallet: don't duplicate change output if already exist #27601

Closed

murchandamus reviewed Jun 13, 2023

View reviewed changes

furszy commented Sep 12, 2023

View reviewed changes

furszy added 2 commits March 30, 2024 20:40

test: wallet, add coverage for replacement tx coin selection

b054222

The wallet tx creation process should not select any output belonging to the transaction that is being replaced.

furszy added 3 commits March 30, 2024 20:40

interfaces: add isSpentInMempool method

b244c09

furszy force-pushed the 2022_wallet_do_not_select_utxo_from_the_tx_being_replaced branch from d7e41b6 to 34df3c7 Compare March 30, 2024 23:41

furszy changed the title ~~wallet: tx creation, don't select outputs from txes that are being replaced~~ [WIP] wallet: tx creation, don't select outputs from txes that are being replaced Mar 30, 2024

DrahtBot added CI failed and removed Needs rebase labels Mar 31, 2024

This was referenced Mar 31, 2024

wallet: clarify FundTransaction signature #29564

Closed

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

Merged

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

Open

This was referenced May 23, 2024

refactor: interfaces, make 'createTransaction' less error-prone bitcoin-core/gui#807

Draft

wallet: re-activate "AmountWithFeeExceedsBalance" error #25269

Open

furszy closed this Dec 30, 2024

bitcoin deleted a comment from Hameedoooooo Jul 2, 2025

[WIP] wallet: tx creation, don't select outputs from txes that are being replaced #26732

[WIP] wallet: tx creation, don't select outputs from txes that are being replaced #26732

Uh oh!

Conversation

furszy commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First Scenario, Do Not Use Outputs From the Tx Being Replaced

Second Scenario, Do Not Create BIP125 Rule 2 Invalid Txes

Uh oh!

DrahtBot commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

achow101 commented Dec 20, 2022

Uh oh!

furszy commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Dec 21, 2022

Uh oh!

furszy commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Dec 23, 2022

Choose a reason for hiding this comment

Uh oh!

furszy commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus 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!

maflcko commented Dec 11, 2023

Uh oh!

furszy commented Dec 12, 2023

Uh oh!

DrahtBot commented Dec 3, 2024

Uh oh!

furszy commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murchandamus commented Jul 7, 2025

Uh oh!

furszy commented Jul 7, 2025

Uh oh!

Uh oh!

furszy commented Dec 20, 2022 •

edited

Loading

DrahtBot commented Dec 20, 2022 •

edited

Loading

furszy commented Dec 21, 2022 •

edited

Loading

furszy commented Dec 22, 2022 •

edited

Loading

furszy commented Dec 26, 2022 •

edited

Loading

furszy commented Dec 30, 2024 •

edited

Loading