Add information to "Confirm fee bump" window#186
Add information to "Confirm fee bump" window#186jonasschnelli merged 1 commit intomasterfrom unknown repository
Conversation
percy-g2
left a comment
percy-g2
left a comment
There was a problem hiding this comment.
Concept ACK
Compiled successfully on Ubuntu 20.04.1 LTS and tested with below details:
Initial Tx: 64e2c03527b50a934f9cdedf87813ab534023afd161c907f4380dd24506f9a71
Selected one of the Inputs, used one address from wallet as change address and broadcasted RBF enabled Tx.
Replacement Tx: 02a48410d61974125ed145c465004e7e7c681b2398108ed4c4a5acea261b160c
New inputs and Outputs were added automatically.
User was informed about the changes in the "Confirm fee bump" dialog:
Can add one more <br /> in Line 517
Done. Added in 014498e |
There was a problem hiding this comment.
IIUC, the purpose of this change is to warn users about privacy issues while bumping a fee, right?
Why not to add this statement explicitly (however, not sure about exact wording) with an appropriate subtitle? Smth like "WARNING: fee bumping could leak privacy if new inputs and change output will be added to the transaction".
@prayank23
Do you mind changing this PR title s/Add information in RBF pop-up/Add information to "Confirm fee bump" window/ ?
ghost
changed the title
Inform user about changes in replacement transaction apart from fees which is already mentioned. Adding inputs and outputs can be considered good or bad for privacy or fees depending on the user. As luke-jr mentioned reducing change output is also a privacy issue
I was considering adding the word "warning" but didn't. It can be added. Mentioning about privacy looks scary and as I explained above it may not be a privacy issue for some users. Let me explain with one example: Alice uses coin control features to select one input, custom change address and broadcasts RBF enabled tx. It is not confirmed for hours and she needs this amount urgently on one exchange for trading. She increases the fee with a replacement transaction not aware of other inputs and outputs being added, broadcasts it. There were few UTXOs in the wallet which were associated with darknet transactions and some of them were used in replacement transaction. In this case it's a privacy issue. Bob has a business and uses coin control features to select one input, custom change address and broadcasts RBF enabled tx. Not confirmed for hours, few users related to this business are waiting. Bob increases the fee with replacement transaction which adds few extra inputs and outputs but Bob doesn't care as this wallet has all transactions related to business. Fees could have been less if less inputs/outputs but priority in this case is to complete the users requests asap. |
If a txout, that was not correlated to a user/wallet in any way, is added to a transaction via a new input, it is always bad for privacy. However, it is true that some users do not care about it. |
|
Makes sense. Users may not care but it affects privacy. Will rephrase the sentence and add "warning" and "privacy". |
|
@achow101 @meshcollider @luke-jr @Sjors @sipa Do you mind looking into this PR? |
@hebasto Will do once there are enough ACKs and we are sure about the statement that can be used in this warning. |
|
@prayank23
Doing in such a way all ACKs will be invalidated. It will require additional re-reviewing that increases the review burden. |
Done. Squashed commits. |
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
(NACK until message is either CoinControl-only or understandable for non-technical folks)
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
A few suggestions below; thanks for updating.
|
utACK 84add0d |
There was a problem hiding this comment.
ACK 84add0d
Happy to re-ACK after addressing the recent @luke-jr's suggestion.
Check if "Coin Control features" are enabled to display warning before broadcasting replacement transaction Workaround to fix issue: bitcoin/bitcoin#20795 Co-authored-by: Jon Atack <[email protected]>
|
Tested ACK - 232d1f9 |
|
(nit: not sure if it needs to be bold) |
232d1f9 Add information to "Confirm fee bump" window (Prayank) Pull request description: + Add information in bump fee confirmation box according to the documentation: https://bitcoincore.org/en/doc/0.20.0/rpc/wallet/bumpfee/ + Workaround to fix issue: bitcoin#20795 in which user isn't aware of new inputs, outputs added to replacement transaction before broadcasting. Initial transaction had used coin control features and custom change address. Until the issue is fixed by change in coin selection algorithm we can add this warning. + Waiting for comments from devs who are working on coin selection algorithm PRs or involved in related research. However got two comments from Luke Dashjr and Pieter Wuille: _luke-jr: Reducing the change output also could be a privacy problem, since it identifies which output was change._ _sipa: Wallet doesn't know the original transaction was using coin control. So I think its expected that if you use automatic fee bumping, you'll get whatever the coin selection algorithm decides. As for why its not decreasing the change and instead adding another input, that may be a bug._ (IRC: #bitcoin-core-dev) ACKs for top commit: jonasschnelli: Tested ACK - 232d1f9 Tree-SHA512: 2ff65db1ddb1d4a45f82670b6ca303a0bf48acf3d09defffc21f44ec81cb6182268959706f592f3442aae5db48f43b8ea86973d74ec2721be93d209ce0414953
ghost
mentioned this pull request
6 participants
Add information in bump fee confirmation box according to the documentation: https://bitcoincore.org/en/doc/0.20.0/rpc/wallet/bumpfee/
Workaround to fix issue: Privacy Issue - Increase Fee with Custom Change Address grabs new UTXO bitcoin/bitcoin#20795 in which user isn't aware of new inputs, outputs added to replacement transaction before broadcasting. Initial transaction had used coin control features and custom change address. Until the issue is fixed by change in coin selection algorithm we can add this warning.
Waiting for comments from devs who are working on coin selection algorithm PRs or involved in related research. However got two comments from Luke Dashjr and Pieter Wuille:
luke-jr: Reducing the change output also could be a privacy problem, since it identifies which output was change.
sipa: Wallet doesn't know the original transaction was using coin control. So I think its expected that if you use automatic fee bumping, you'll get whatever the coin selection algorithm decides. As for why its not decreasing the change and instead adding another input, that may be a bug. (IRC: #bitcoin-core-dev)