[qt] bumpfee: offer user to reduce output for transactions without change

Sjors commented at 12:52 pm on February 28, 2018: member

Builds on top of #12096. I’ll rebase when that’s merged.

If a user sends funds to a single destination, they might be trying to move all their funds to another wallet or an exchange. However it’s also possible that they are making a payment and coin selection found a matching input.

With the former it’s fine to reduce the output in order to increase the fee, but with the latter it’s not, e.g. it could lead to underpaying an invoice. In that case we show the following warning:

At the risk of too much UI clutter, the message could be made stronger by showing the original amount(s) vs. the new amount(s).

For comparison, this message is shown when a change output can be reduced:

If in the future we’re able to add inputs, we should probably offer the user a choice if they prefer to reduce an output instead. It’s worth keeping that in mind when choosing the right wording here.

[rpc] [wallet] Allow single-output transactions in bumpfee

Currently, bumpfee cannot be used to bump a single output transaction. This should be possible, as single output transactions are special in that the exact amount doesn't usually matter (the user is emptying a wallet or using coin control to move UTXOs).

69456ad302

[test] Test single-output transaction bumpfee cf02bcb3ed

f'throw when explicit -1 reduce_output is given in options 624f27c4d3

[qt] bumpfee: offer user to reduce output for transactions without change 81bcb253e1

fanquake added the label GUI on Feb 28, 2018

in src/qt/walletmodel.cpp:677 in 81bcb253e1

670@@ -671,14 +671,30 @@ bool WalletModel::bumpFee(uint256 hash)
671     CAmount old_fee;
672     CAmount new_fee;
673     CMutableTransaction mtx;
674-    if (feebumper::CreateTransaction(wallet, hash, coin_control, 0 /* totalFee */, errors, old_fee, new_fee, mtx) != feebumper::Result::OK) {
675+
676+    // If the transaction has a single output, ask user if it's OK to reduce:
677+    int32_t reduce_output = -1;
678+    auto it = wallet->mapWallet.find(hash);

promag commented at 12:58 pm on February 28, 2018:

Missing lock?

Sjors commented at 2:12 pm on February 28, 2018:

Is there good documentation about when locks are appropriate? Other than: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes

promag commented at 2:15 pm on February 28, 2018:

mapWallet is protected by cs_wallet. If this is moved to feebumper::CreateTransaction then the lock is already held there.

in src/qt/walletmodel.cpp:692 in 81bcb253e1

689          return false;
690     }
691 
692     // allow a user based fee verification
693-    QString questionString = tr("Do you want to increase the fee?");
694+    QString questionString;

promag commented at 1:04 pm on February 28, 2018:

Nit, and the diff is smaller:

0QString questionString = tr(...);
1if (reduce_output != -1) {
2     questionString = tr(...);
3}

promag commented at 1:06 pm on February 28, 2018: member

This logic in WalletModel::bumpFee sounds misplaced. I would add a parameter to feebumper::CreateTransaction to return whether a non-change output was reduced and move the logic there.

Sjors commented at 2:10 pm on February 28, 2018: member

add a parameter to feebumper::CreateTransaction to return whether a non-change output was reduced @kallewoof can that be part of #12096, e.g. if reduce_output==-2 it picks the biggest output?

luke-jr commented at 4:24 pm on February 28, 2018: member

I don’t think this is the right route. While I think users should be able to reduce outputs if desired, we should by default offer to add inputs, not reduce outputs.

promag commented at 4:52 pm on February 28, 2018: member

@luke-jr I think that discussion is for #12096. Also see #12096 (comment).

Edit: ok you were there already.

☝️IIUC feebumper::CreateTransaction doesn’t add inputs. I’ve suggested that in #12096 (comment)

luke-jr commented at 5:07 pm on February 28, 2018: member

Indeed, to rephrase: users should be able to reduce outputs; it just should require explicitly asking to do so, and shouldn’t be the default/recommended behaviour.

bedri commented at 8:07 pm on February 28, 2018: none

If a customer paying for a good for the vendor and vendor agrees to pay the transaction fee, then the customer can make a transaction with nearly zero fee which will wait a long time for confirmations and then increase the fee to the agreed amount and accepting the reduction in the output. So that vendor will be able to pay the transaction fee. If the vendor does not willing to pay the transaction fee (no agreement made between the vendor and the customer about this) then the customer can make a regular transaction to pay for the good. If customer decides that he/she put a low fee on the transaction so that the transaction will take a long time to occur then he/she should bumpfee without reducing the output.

So bumpfee reduction from the output should be optional and should be asked. For the above showcase, it may also be a good idea that if a bumpfee made by reducing the output, the receiving side of the transaction (vendor in the above case) should also be asked if he/she agrees with the reduction. If he/she does not agree then the bumpfee should not occur.

On Wed, Feb 28, 2018 at 8:18 PM, Luke Dashjr notifications@github.com wrote:

Indeed, to rephrase: users should be able to reduce outputs; it just should require explicitly asking to do so, and shouldn’t be the default/recommended behaviour.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/12565#issuecomment-369309361, or mute the thread https://github.com/notifications/unsubscribe-auth/AAuuLwNRNM-bPtKZ1uaMYcWwIvEps_4Pks5tZYfNgaJpZM4SWmpP .

Sjors commented at 8:52 am on March 1, 2018: member

@luke-jr Cancel is the default option, but without an alternative I can see how users would just click OK even if it’s not a good idea. Perhaps it’s better to hold back this PR until feebumper::CreateTransaction supports adding inputs. By default the OK button would just add an input. We could show each of the destination addresses with a check box “reduce amount”. That would seem like a safer UX.

Unless that change takes forever. Maybe we can tag this with 0.17, and revisit the discussion if this better alternative isn’t ready? @bedri there’s no way (that I know) for the wallet to communicate with the merchant. The only thing we can do is inform the user that they should do this.

Sjors commented at 11:20 am on May 15, 2018: member

Closing until feebumper::CreateTransaction supports adding inputs. The UI should only reduce outputs if the user explicitly opts in to that (e.g. when they’re sweeping all their funds to some other place).

Sjors closed this on May 15, 2018

MarcoFalke locked this on Sep 8, 2021

[qt] bumpfee: offer user to reduce output for transactions without change #12565