The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28264 (test: refactor: support sending funds with outpoint result by theStack)
• #28212 (test: verify spend from 999-of-999 taproot multisig wallet by eriknylund)
• #28136 (refactor: move GetServicesNames from rpc/util.{h,cpp} to rpc/net.cpp by jonatack)
• #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
• #27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)
• #26573 (Wallet: don't underestimate the fees when spending a Taproot output by darosior)

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.

I frequently send single coins, or a small group, using the subtract fee from output feature in the GUI. I can imagine people using this with the RPC too, e.g. to move a single coin to or from cold storage, or when sending coins to an exchange where you don't care about the precise amount, but you do care about privacy.

Limiting this feature to subtracting from a single output, rather than an array of outputs, is fine with me though, if that helps simplifying it. Maybe also constrain it to fully manual coin selection.

I frequently send single coins, or a small group, using the subtract fee from output feature in the GUI. I can imagine people using this with the RPC too, e.g. to move a single coin to or from cold storage, or when sending coins to an exchange where you don't care about the precise amount, but you do care about privacy.

The sweep function proposed in #24118 can be extended to support this use case. You are essentially sweeping a list of specific UTXOs rather than a list of all UTXOs in the wallet. So sweep can be extended to take a list of specific UTXOs and just spend those. That way we don't need to maintain any SFFO logic. It really is not necessary to keep SFFO in coin selection logic if you aren't going to even use the coin selection logic by restricting to a specific set of UTXOs.

The sweep function proposed in #24118 can be extended to support this use case.

I guess I probably need to look at the code to understand, but I'm confused how having two different sending functions that both support coin selection could be less of a maintenance burden than an option that mainly consists of an extra for loop doing txout.nValue -= to_reduce / outputs_to_subtract_fee_from. If the new code is better than the old code, that wouldn't be surprising because the old code quality was not great. But it seems like any cleanup could be a refactor without a UI change, not a refactor which forces a UI change.

In any case, it doesn't seem great if the main justification for the UI change is a vague complaint about code maintenance. If having separate spend and sweep functions actually provides a better UI, that could make sense. But it doesn't seem like UI benefits have been explained anywhere (release notes in #24118 would be a good place to brag about them). What was the previous "unintuitive behavior"? Why is the new behavior more intuitive? Are there existing workflows broken by the new UI? Are there new workflows enabled by the new UI or simplified by it?

I'm confused how having two different sending functions that both support coin selection could be less of a maintenance burden than an option that mainly consists of an extra for loop doing txout.nValue -= to_reduce / outputs_to_subtract_fee_from.

It makes the analysis of coin selection logic much easier. Coin selection is already a multivariate analysis - we have to consider fees, whether a change may be created, the waste metric, etc. There are edge cases when values are near the balance or near dust. All of these things make analyzing coin selection fairly difficult. SFFO is another variable in that analysis, and it can and has caused issues with reasoning about what coin selection is going to do in certain scenarios (while working on #17331, I remember pulling my hair out trying to work out the logic to get SFFO to behave correctly and not interfere with the rest of coin selection). By removing it, we simplify this analysis and thus make it easier to reason about any changes to coin selection logic in the future. Having SFFO has already bitten us in the past, keeping it will surely bite us again in the future. Dealing with SFFO is far more than the extra for loop to reduce the output amounts. It has effects on the actual coin selection logic as it changes what value we use for each input (we use the real value instead of effective value with SFFO on).

The second sending function for sweep is very simple and so it is easy to maintain. It does not execute any coin selection logic at all - all UTXOs will be selected, no need to execute any algorithms to figure that out. While adding a second function is certainly a maintenance burden, removing SFFO is a much more significant reduction in maintenance that I am okay with the tradeoff.

In any case, it doesn't seem great if the main justification for the UI change is a vague complaint about code maintenance.

If having separate spend and sweep functions actually provides a better UI, that could make sense. But it doesn't seem like UI benefits have been explained anywhere (release notes in #24118 would be a good place to brag about them).

The sweep function certainly provides a better UI for sweeping a wallet than SFFO does. Instead of having to lookup the balance, specify the balance in the amount, and then specify output indices for SFFO, sweep is a simple function that does exactly what the name suggests. Furthermore, because sweep does not rely on actual coin selection logic, changes to coin selection logic will not unexpectedly break sweep.

What was the previous "unintuitive behavior"?

There have been a couple of bug reports with using SFFO where it fails unexpectedly or does something weird with the fees (#23026, #10034). There have also been some issues opened where users who specify a value near the balance results in the entire balance being unexpectedly spent (#16100).

Why is the new behavior more intuitive? Are there existing workflows broken by the new UI? Are there new workflows enabled by the new UI or simplified by it?

Sweep is unambiguous as to what it is going to do - it is going to spend all of the UTXOs in the wallet and send the amount (minus fees) to the specified address(es). It won't be broken by coin selection logic changes because it always chooses every UTXO.

Great feedback! Thank you for clarifying that. I didn't realize:

• The reason that sweep() implementation can be simpler than send(subtract_from_outputs=true) is that the sweep() implementation only has to support manual coin selection, while the send implementation also supports automatic coin selection.

• The reason that sweep() implementation is preferable to users is that it doesn't force them make additional calls to find out their balance().

I'm still skeptical about some things:

• It's hard to buy the claim that this is a code simplification and good for maintenance when PR adds and duplicates hundreds of lines of code without removing much. I understand your frustration with Use effective values throughout coin selection #17331, but the previous code was god-awful, and the new code is much better (thanks to your efforts!). And the edge cases have been discussed, debugged, documented, and have good test coverage. So deprecating subtract from outputs at this point almost seems like a revenge-killing. If you want to add a sweep RPC that calls just wraps the send code setting subtract=true, that seems like it would be more straightforward to review, and better for long term maintenance, without a duplicate implementation and different set of tests.

• I still don't know actual use-cases where subtract from outputs has done something bad or harmful or unexpected. To me seems like a straightforward and useful option and like a simplifying concept. In a typical bitcoin transaction, amount received is less than than amount sent due to fees, coin selection, etc. So when you create a transaction you have to decide whether you will fix the receive amount, and let the send amount vary. Or fix the send amount and let the receive amount vary. And "subtract from outputs" is toggle for doing that. Any time you don't care about receiver receiving an exact quantity (sending between wallets, depositing to an exchange or other online service) your probably may want to use this option. It seems unfortunate to me if it goes away. I assume people are using it as part of their existing workflows too, but I guess we will have have to find out about that.

EDIT: Changed "probably" -> "may." In typical case you need to create a change output anyway so using option provides no benefit

• The reason that sweep() implementation is preferable to users is that it doesn't force them make additional calls to find out their balance().

Thinking about this more, if sweep() implementation gains ability to manually select coins to send, this could be both a drawback and an advantage for usability. Advantage because you don't have to sum up coin amounts, but drawback because being forced to specify the amount you are sending when you send is kind of a safety feature. If you have explicitly specify the amount in BTC you are trying to send, it ensures you didn't off-by-one some transaction id and are now accidentally spending some ridiculous amount.

I'm more satisfied now that the sweep method with manual coin selection probably is sufficient to handle all previous use-cases for subtract-from-outputs. I still think it's a shame that it increases code size and adds all these separate functions and tests, and that it gets rid of a concept that clicked and seemed a little more symmetric to me. But no objections if this is the road we want to go down.

Concept NACK.

SFFO is a clean (at least conceptually) feature with many use cases (not just the one proposed to be special-cased by #24118)

During the IRC meeting today, we discussed SFFO and sweep. The conclusion was that SFFO does seem to have a use case outside of sweep, and many contributors do not want to see it go. However many people do use SFFO to sweep, and that would still be better served with a dedicated sweep function.

So the current idea is to still introduce sweep, and to direct people to use that instead of SFFO if the intention appears to be sweep. This makes it easier for us to reason about edge cases with values near the balance as SFFO will no longer need to be considered.

thank you @achow101 !

SFFO is a clean (at least conceptually) feature with many use cases (not just the one proposed to be special-cased by #24118)

What use cases? In terms of actual usage, it seems to be that sendall covers what everyone has used it for.

Another bug caused by SFFO: #26466

@jonatack wrote:

Concept NACK on removing subtractfeefromamount.

When sending, I set it to true by default for the frequent case when the recipient chooses the feerate based on how quickly they need the tx in a block. Aligning incentives, for instance, whenever an individual would like to receive peer-to-peer bitcoin and doesn’t have lightning set up or prefers onchain.

What if we instead add a feature (e.g. a new RPC method) to explicitly reduce an output? The easiest form would take a fixed amount (e.g. 500 sat), a more advanced version would take a relative fee rate (e.g. reduce output such that fee rate increases by 5 sats/byte). For either case it would not add new inputs, so there's no coin selection involved.

Note that only fixes the use case of fee bumping, not the first transaction.

🐙 This pull request conflicts with the target branch and needs rebase.