Unsafe reduce_output when new coins are added

[Sjors]

Is there an existing issue for this?

• I have searched the existing issues

Current behaviour

See inline discussion at #26467 (comment)

@whitslack:

Consider the case where I am paying someone on chain under the stipulation that they will eat the mining fee. (Thus, I specify their address in subtractfeefrom when I'm constructing my transaction.) Later, they tell me that they can't wait for confirmation any longer and need an urgent fee bump. If I naïvely specify their output as the reduce_output, I may end up paying them much more than I intended to if I only have large UTxOs in my wallet.

Expected behaviour

Not sure.

Improving the docs is one option, but this behaviour seem quite counter intuitive.

My own suggestion:

Should we disable the adding of new inputs when reduce_output is set? If someone really intends to do that, they should probably use outputs.

Steps to reproduce

Haven't tried.

Relevant log output

No response

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

Master after #26467

Operating system and version

Any

Machine specifications

No response

[furszy]

Should we disable the adding of new inputs when reduce_output is set? If someone really intends to do that, they should probably use outputs.

I don't think so. This is more an implementation issue rather than a missing doc or a missing restriction.
This could also happen without someone intending to do it. For instance, could be a higher bump fee that make the tx require another input.

The issue arises because the code (as is now) mixes two different concepts; it treats the change output and the reduce output (output from where the fees will be taken) as they would be the same, when they are not. It can easily be seen here.
Instead of instructing the wallet to deduce the fee from certain output (by using SFFO), we discard the output from the recipients list and make the output script the custom change destination, so the wallet automatically adds the change output, at the end of the transaction creation process, going to the discarded output script, with an amount equal to the remainder: inputs - outputs - new_bump_fee.

To untangle the two concepts, we need to fix #27601 first. It is the reason why we drop the change output from the recipients list instead of simply mark it as SFFO (which would be the easiest implementation for a "reduce output" mechanism).

[maflcko]

What is the status here? This is the only issue tagged 26.0 that doesn't have a pull request associated

[furszy]

What is the status here? This is the only issue tagged 26.0 that doesn't have a pull request associated

Status is "waiting for others" in #27601 since July 27th. Happy to receive feedback on my last message there (#27601 (review)).

The reason why this happens is explained above #28180 (comment). if we manage to merge #27601 (in any form), we can fix this issue cleanly.

Basically, the only remaining point to discuss there is whether to have the transaction creation process automatically detecting change outputs when they match the custom change address or not.
I could also code a different version of the PR moving the automatic change output detection outside of the tx creation process too. Placing it on the upper layer (similar to what we do with the duplicate recipients check). But, in any case, concluding the discussion there prior to any changes would be preferable.