wallet: "avoid_reuse" wallet flag for improved privacy #13756
Conversation
|
Concept ACK Nice work! |
|
Perhaps fodder for a separate PR, but listtransactions and the GUI should get indicators that an incoming payment was dirty when received. By having that parties that reuse addresses will start getting a frowny-recycle-icon or whatever on their payments, which will increase awareness that their behaviour has downsides. Edit: I see I'm repeating myself from an earlier PR. :P |
9e31139 to
8cd0e8f
Compare
You can always spend them by using the I do think we should at least warn users about that kind of behaviour though, but it feels like a UI fix that can come later.
I will see about making that the case for the CLI side of things. I think a GUI side fix would be great, but unfortunately I don't seem to be talented at writing QT code. (Practice, I guess.)
Sorry about that. I felt like it was worthwhile to re-PR since the old PR has a lot of outdated talk that is mitigated by #12257. |
8cd0e8f to
389d97a
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
I think "dirty" is a confusing concept. Maybe add an exclamation mark (or a detective icon) next to the transaction and when the user clicks on that, say something like "This address has been used before, for privacy reasons it's better to create a new address each time you wish to receive coins, even from the same person". Detective icon is a nice hint that there's privacy related information, but an exclamation mark can also be used to explain other potential problems with an incoming transaction, e.g. if the fee is extremely low. A parallel measure, that doesn't involve UI changes, could be for coin selection to not spend from dirty addresses as long as possible. And then try to spend them if there's an exact match (no other outputs, no change), perhaps even only if there's no other exact match, depending on how strongly you want to avoid spending these things. When the user wants to "spend all" funds and the dirty amount is less than x%, a dialog could popup and suggest to "exclude a small amount for privacy reasons". This is the kind of behavior that if we want people to use it, should be on by default. That can wait for another PR, but it's useful to think about it a bit when designing the RPC. Telling people to manually go into coin selection is not a good idea. If a user receives a non-trivial amount, they expect to be able to just spend it with no additional hoops. That means in the current form it's probably never a user friendly default. |
@Sjors I like that, and then a tooltip/popup could explain the warning.
"Avoid" is not binary right? |
It's called "dirty" in the CLI as well, but there's also a different concept in the wallet code called 'dirty' which is completely unrelated. In short, it may be a good idea to rename this feature, but I can't think of a good name. "Compromised" is too long. "Seen" is too vague. @luke-jr any ideas?
I like "!" too. If it is used for multiple things, it could take away from the importance though (e.g. user ignores it thinking it's 'just cause of the 1 sat/b fee'). As a sidenote, we also should mark UTXOs in coin selection dialog somehow. Same approach? Maybe need to include a warning popup if they mix dirty and clean.
I don't know, I think we should always avoid dirty inputs unless the user explicitly marks them as 'clean' or picks them directly using manual coin control.
I didn't think about this case, and it's probably fairly common. Then again, I don't think I can do a lot from the CLI side, so this is probably GUI level stuff.
What about it is too aggressive?
Maybe it should be sensitive to the amount. A good middle ground may be to, for any input that is X times higher than current dust threshold, that goes to an already spent-from address, to show the user a dialog saying the input is dirty and ask them whether to force-mark it as clean or keep it as dirty. |
A user might receive their entire salary on a reused address. If the current implementation of This is where the threshold comes in, but I think it's non-trivial to figure out what the right threshold is, and it might be game-able. Perhaps the ! / detective icon in the transaction view could offer the user a choice to manually quarantine funds. Something like "If you did not expect this transaction someone may be spying on you, waiting for you to spend the coins. Would you like to quarantine them?" You could even quarantine funds by default based on some heuristic as long as the UI makes that very clear and it's easy for a user to unquarantine it (similar to firewall and anti-virus popups). But maybe try the opt-in quarantine approach first. So then there's Depending on what ends up happening, the documentation should perhaps point out that the GUI ignores this setting. Alternatively (for the current implementation) dirty coins could be hidden in the GUI, not shown in coin selection and not used when spending all, when this setting is enabled. Both options require a warning, and neither seems ideal, but updating the GUI in a more sophisticated way is pretty time consuming. |
|
@Sjors That makes sense to me. It seems like adding this default off (as is the current proposal) makes the most sense. GUI work seems like it will be a good deal of work to get right, but in the meantime, there are real (advanced) users who would benefit from having this feature now, even without the GUI/intuitive component. |
|
N.B. This doesn't actually fully solve #10065, since transactions received with a dirty address are still shown in the GUI / RPC. |
|
(as for a term... "reused" perhaps? I don't know that this flag should be exposed as-is, but more like a "will never confirm until spent" status) |
|
I think "reused" is a bit vague, and doesn't convey the fact it's considered a thing to be avoided if possible, in the way that "dirty" does. |
57fca7d to
e4df634
Compare
|
Note: I realized the test was invalid, as |
8452b59 to
9337fb3
Compare
|
It feels to me that this should be a persistent per-wallet setting, rather than a global config option (I think we should be eliminating the global wallet config options wherever possible. See #13044) |
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
Summary: this was cleaned up in a previous diff but is now a hurdle to backporting [[bitcoin/bitcoin#13756 | PR13756]]. this change makes the backport possible and brings us a tad closer to core for less merge conflicts down the road. Test Plan: ninja Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6367
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
…debit values c9e6e7e wallet: add cachable amounts for caching credit/debit values (Karl-Johan Alm) Pull request description: This is a refactoring that will make bitcoin#13756 a lot cleaner and straight-forward, since it adds another combination to the pile (watch-only * spendable * reused). It's also a nice change in general. Tree-SHA512: 6c876d58bbffd5cb85ef632dea4fd6afed163904bbde5efdb307fa119af178ed3cb5df047255da7e9a9136fed876922f1116fce61a3710f308c72275f9b7d18b
Add a new wallet flag called
avoid_reusewhich, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.
This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in #10386 (comment) are also addressed due to #12257.
Note: this builds on top of #15780.(merged)