Unsafe reduce_output when new coins are added #28180

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

2. fanquake added this to the milestone 26.0 on Jul 28, 2023
3. MarcoFalke added the label Wallet on Jul 28, 2023
4. 
   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).

5. 
   MarcoFalke 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
6. MarcoFalke added the label RPC/REST/ZMQ on Sep 5, 2023
7. 
   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.

8. achow101 closed this on Sep 27, 2023

   ---

Contributors
Sjors furszy MarcoFalke

Labels
Wallet RPC/REST/ZMQ

Milestone
26.0