Deprecate SubtractFeeFromOutputs

achow101 commented at 10:25 pm on January 24, 2022: member

With #24118 introducing sweep functionality, there is no reason to maintain the SubtractFeeFromOutputs (SFFO) behavior as its use case was to do sweeping. So it is now deprecated and usage of it via the RPC will require -deprecatedrpc=sffo. The functionality has also been removed from the GUI. The tests have been updated to either use sweep, not use SFFO, or have -deprecatedrpc=sffo as necessary.

There will be a followup which removes SFFO entirely.

There is also a change to IsDeprecatedRPCEnabled as there were linker errors for bitcoin-wallet with the normal way that it is used.

Required #24118.

DrahtBot added the label GUI on Jan 24, 2022

DrahtBot added the label RPC/REST/ZMQ on Jan 24, 2022

DrahtBot added the label Wallet on Jan 24, 2022

DrahtBot commented at 11:44 pm on January 24, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr

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.

in src/rpc/server.cpp:352 in 0ba369f5e2 outdated

350@@ -351,9 +351,7 @@ bool RPCIsInWarmup(std::string *outStatus)
351 
352 bool IsDeprecatedRPCEnabled(const std::string& method)

maflcko commented at 8:20 am on January 25, 2022:

nit: Wouldn’t it be better to remove this and replace it with IsDeprecatedRPCEnabled(gArgs, method) at the (currently) two call sites?

achow101 commented at 7:21 pm on January 25, 2022:

Done

achow101 force-pushed on Jan 25, 2022

Sjors commented at 11:56 am on January 28, 2022: member

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.

achow101 commented at 4:55 pm on January 28, 2022: member

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.

ryanofsky commented at 6:45 pm on January 28, 2022: contributor

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?

achow101 commented at 7:46 pm on January 28, 2022: member

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.

ryanofsky commented at 8:46 pm on January 28, 2022: contributor

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 #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

ryanofsky commented at 9:11 pm on January 28, 2022: contributor

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.

ryanofsky commented at 9:21 pm on January 28, 2022: contributor

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.

luke-jr commented at 9:05 pm on January 29, 2022: member

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)

achow101 commented at 10:17 pm on February 3, 2022: member

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.

achow101 closed this on Feb 3, 2022

hugohn commented at 3:18 am on February 8, 2022: contributor

thank you @achow101 !

bitcoin locked this on Feb 8, 2023

bitcoin unlocked this on Sep 18, 2023

achow101 reopened this on Sep 18, 2023

achow101 commented at 4:16 pm on September 18, 2023: member

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.

DrahtBot added the label Needs rebase on Sep 18, 2023

in src/wallet/rpc/spend.cpp:229 in b6c760474b outdated

139@@ -134,7 +140,7 @@ RPCHelpMan sendtoaddress()
140                     {"comment_to", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "A comment to store the name of the person or organization\n"
141                                          "to which you're sending the transaction. This is not part of the \n"
142                                          "transaction, just kept in your wallet."},
143-                    {"subtractfeefromamount", RPCArg::Type::BOOL, RPCArg::Default{false}, "The fee will be deducted from the amount being sent.\n"
144+                    {"subtractfeefromamount", RPCArg::Type::BOOL, RPCArg::Default{false}, "(DEPRECATED) The fee will be deducted from the amount being sent.\n"

jonatack commented at 6:12 pm on September 19, 2023:

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.

achow101 commented at 10:46 am on September 20, 2023: member

Another bug caused by SFFO: #26466

rpc, util: Add IsDeprecatedRPCEnabled that takes ArgsManager

IsDeprecatedRPCEnabled currently does not compile when used from
src/wallet/rpc/*, however a variant which takes the ArgsManager does
work. The original becomes a wrapper around this new function and it
just provides gArgs.

71e1bfc805

rpc: Deprecate subtractFeeFromOutputs

Hide the usage of the subtractFeeFromOutputs feature behind
-deprecatedrpc=sffo

6c8571052e

gui: Remove subtract fee from outputs

It's being deprecated, so get rid of it.

928e3c195b

tests: Updates for deprecating SFFO

Several tests were using SFFO in order to sweep the wallet. This usage
has been replaced with the sweep function.

The tests that are testing the SFFO behavior specifically will now have
the necessary nodes restarted with -deprecatedrpc=sffo.

A few tests were using SFFO but did not need to; these have been changed
to not use SFFO.

4c69c3b7c8

achow101 force-pushed on Sep 27, 2023

DrahtBot removed the label Needs rebase on Sep 27, 2023

DrahtBot added the label CI failed on Sep 28, 2023

Sjors commented at 8:31 am on September 28, 2023: member

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

DrahtBot commented at 3:54 pm on October 25, 2023: contributor

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

DrahtBot added the label Needs rebase on Oct 25, 2023

achow101 closed this on Oct 26, 2023

bitcoin locked this on Oct 25, 2024

Deprecate SubtractFeeFromOutputs #24142

Reviews

Conflicts