Unsafe reduce_output when new coins are added

Sjors commented at 11:11 am on July 28, 2023: member

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

See inline discussion at #26467 (review) @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

fanquake added this to the milestone 26.0 on Jul 28, 2023

maflcko added the label Wallet on Jul 28, 2023

furszy commented at 9:15 pm on July 31, 2023: member

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 commented at 11:33 am on September 5, 2023: member

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

maflcko added the label RPC/REST/ZMQ on Sep 5, 2023

furszy commented at 1:06 pm on September 5, 2023: member

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 (https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1550813509).

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.

achow101 closed this on Sep 27, 2023

Frank-GER referenced this in commit 521291b6da on Oct 5, 2023

BlackcoinDev referenced this in commit 19a7e608f9 on Feb 5, 2024

bitcoin locked this on Sep 26, 2024

Unsafe reduce_output when new coins are added #28180