Small unspent can get removed from OutputGroup for being uneconomical probably leading to later partial spending #20287

Placeholder, we are still testing this. cc: @achow101.

<!-- This issue tracker is only for technical issues related to Bitcoin Core. General bitcoin questions and/or support requests are best directed to the Bitcoin StackExchange at https://bitcoin.stackexchange.com. For reporting security issues, please read instructions at https://bitcoincore.org/en/contact/. If the node is "stuck" during sync or giving "block checksum mismatch" errors, please ensure your hardware is stable by running memtest and observe CPU temperature with a load-test tool such as linpack before creating an issue! -->

<!-- Describe the issue -->

Expected behavior

<!--- What behavior did you expect? -->

Actual behavior

<!--- What was the actual behavior (provide screenshots if the issue is GUI-related)? -->

To reproduce

<!--- How reliably can you reproduce the issue, what are the steps to do so? -->

System information

<!-- What version of Bitcoin Core are you using, where did you get it (website, self-compiled, etc)? -->

<!-- What type of machine are you observing the error on (OS/CPU and disk type)? -->

<!-- GUI-related issue? What is your operating system and its version? If Linux, what is your desktop environment and graphical shell? -->

<!-- Any extra information that might be useful in the debugging process. -->

<!--- This is normally the contents of a `debug.log` or `config.log` file. Raw text or a link to a pastebin type site are preferred. -->

It's a bit hard to test this, but I'm pretty sure it happens.

For BnB coin selection, we call OutputGroup::GetPositiveOnlyGroup. This will remove from the OutputGroup any dust outputs. But if after the removal, the OutputGroup's effective value is still positive, then it remains available for coin selection. So it is possible that BnB will select an OutputGroup but not the dust outputs associated with the same destination, so the avoid partial spends condition is violated.

This does have a privacy risk as the user may think that avoid partial spends is active, but then a partial spend occurs. If this risk is indeed a problem, then we could just remove any OutputGroup that contains dust outputs, regardless of the other outputs in that group. However this has a DoS attack risk. If an attacker sends dust outputs to existing addresses, then it is possible for that attacker to prevent someone who has enabled avoid partial spends from spending any of their coins because all OutputGroups would be removed for containing dust.

Even though OutputGroup::GetPositiveOnlyGroup has been refactored away, it looks to me that this is still an issue as GroupOutputs() is still filtering by EffectiveValue() >0:

https://github.com/bitcoin/bitcoin/blob/8d12127a9c19cb218d661a88ab9b6871c9d853b9/src/wallet/spend.cpp#L470-L473

If we remove the dust filter here then we become liable to a dust attack where an attacker could send us many dust outputs which would always be selected, causing our fee requirements to (significantly) increase. By retaining the dust filter we potentially open ourselves to future partial spending.

One thing I don't quite understand, will a dusty output not always be a dusty output, so long as the user doesn't modify -dustrelayfee? If so, then I don't think this is an issue -- that output will not be selected for an output group today, nor in the future. If however a user changes their -dustrelayfee then they can be notified of the interaction, perhaps via ParameterInteraction()?

For me it would be cleaner if the -avoidpartialspends flag worked as advertised and always prevented partial spends. In the event that the DoS attack mentioned occured, ideally the wallet would return an error message that there is enough output value to construct the transaction but the outputs are being excluded by the -avoidpartialspends flag, and the user must disable it so that they can be selected.

I agree, that we still have this issue with potentially making a partial spend, when we have an uneconomical UTXO that’s part of a group. I think there may be a confusion here between dust and uneconomical UTXOs, though: in Bitcoin Core, dust pertains to an output whose value is less than dustrelayfee × (input_weight + output_weight) (where dustrelayfee is a feerate) with the default dustrelayfee being 3 ṩ/vB. So, dust outputs will always be dust (unless the dustrelayfee is changed). We only use the dust criteria to prevent our own creation of such outputs and not relay of transactions that create dust outputs. An uneconomical UTXO is one whose input cost is higher than its own value. Whether a UTXO is uneconomical depends on the target feerate of the transaction we’re building. So, even something that we might classify as dust could still be economical to spend at the minRelayTxFee or slightly higher feerates.

I agree that it would be better if -avoidpartialspends always prevented partial spends, but only when we have a decent UX around being able to side-step the spending block. Otherwise, I would worry about it being easy to inconvenience Bitcoin users with forced-address-reuse attacks.

Additionally, I am pondering whether we should permit spending of uneconomical UTXOs when the user is creating a transaction with minRelayTxFee. That would be the cheapest time to clean up the UTXO set. Currently, Knapsack always permits it, which will cause the users to aggressively clean out uneconomical UTXOs even at higher feerates.

(It is by the way my opinion that Knapsack is detrimental and should be removed.)