-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add information to "Confirm fee bump" window #186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 014498e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/ ?
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". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK 6be2bff
@prayank23
Do you mind squashing all commits into the single one?
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3d93f27, tested on Linux Mint 20.1 (Qt 5.12.8):
More sophisticated solutions (e.g., opening a coin control dialog) could be added in follow ups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(NACK until message is either CoinControl-only or understandable for non-technical folks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions below; thanks for updating.
utACK 84add0d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, minor language nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <jon@atack.com>
Tested ACK - 232d1f9 |
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
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)