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

furszy commented at 8:51 pm on December 20, 2022: member

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 commented at 8:51 pm on December 20, 2022: contributor

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.

DrahtBot added the label Wallet on Dec 20, 2022

achow101 commented at 10:44 pm on December 20, 2022: member

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 commented at 2:23 pm on December 21, 2022: member

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 commented at 4:06 pm on December 21, 2022: member

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 force-pushed on Dec 21, 2022

furszy force-pushed on Dec 22, 2022

furszy commented at 2:15 pm on December 22, 2022: member

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

furszy force-pushed on Dec 22, 2022

in src/wallet/spend.cpp:1447 in d7e41b63cd outdated

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

glozow commented at 1:36 pm on December 23, 2022:

please do not add new mentions of BIP125 that aren’t signaling-related, see https://github.com/bitcoin/bitcoin/pull/25775

glozow commented at 1:46 pm on December 23, 2022: member

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?

furszy commented at 9:01 pm on December 26, 2022: member

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.

DrahtBot added the label Needs rebase on Jan 16, 2023

achow101 requested review from glozow on Apr 25, 2023

achow101 requested review from josibake on Apr 25, 2023

achow101 marked this as a draft on Apr 25, 2023

glozow commented at 7:56 pm on May 1, 2023: member

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 commented at 5:03 pm on June 13, 2023: contributor

Concept ACK

furszy commented at 1:12 pm on September 12, 2023: member

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 commented at 3:53 pm on December 11, 2023: member

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 commented at 2:01 pm on December 12, 2023: member

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.

wallet: don't select outputs from txes that are being replaced

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.

a97b5801a1

test: wallet, add coverage for replacement tx coin selection

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

b054222293

interfaces: add isSpentInMempool method b244c09119

wallet: create/fund replacement tx, disallow unconfirmed UTXOs selection

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

8a94171e99

test: verify wallet not creating, BIP125 rule 2 wise, invalid txes

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.

34df3c790f

furszy force-pushed on Mar 30, 2024

furszy renamed this:
~~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
on Mar 30, 2024

DrahtBot removed the label Needs rebase on Mar 31, 2024

DrahtBot added the label CI failed on Mar 31, 2024

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

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

Second Scenario, Do Not Create BIP125 Rule 2 Invalid Txes

Code Coverage

Reviews

Conflicts