wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof · Pull Request #13756 · bitcoin/bitcoin

kallewoof · 2018-07-25T07:34:14Z

Add a new wallet flag called avoid_reuse which, 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)

promag

Concept ACK.

Should we also disallow sending to a dirty address? If we don't then those coins can't be spend if the flag is set, or am I missing something?

test/functional/feature_avoidreuse.py

src/wallet/init.cpp

src/wallet/coincontrol.h

src/wallet/wallet.cpp

practicalswift · 2018-07-25T15:58:55Z

Concept ACK

Nice work!

gmaxwell · 2018-07-25T17:53:17Z

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

kallewoof · 2018-07-26T08:39:14Z

@promag

Should we also disallow sending to a dirty address? If we don't then those coins can't be spend if the flag is set, or am I missing something?

You can always spend them by using the allowdirty flag or by manually selecting them using coin control (via createrawtx or via the GUI).

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.

@gmaxwell

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.

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

Edit: I see I'm repeating myself from an earlier PR. :P

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.

DrahtBot · 2018-07-26T09:27:14Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16215 (gui: Refactor wallet controller activities by promag)
#15937 (WIP: Add loadwallet and createwallet load_on_startup options by ryanofsky)
#15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
#15906 ([wallet] Remove AvailableCoins nMinDepth argument by amitiuttarwar)
#15729 (rpc: Raise error in getbalance if minconf is not zero by promag)
#15450 ([GUI] Create wallet menu option by achow101)
#15064 ([PoC] GUI: Migrate BIP70 merchant info to mapValue["to"] by luke-jr)
#14533 ([WIP] wallet: ensure wallet files are not reused across chains by mrwhythat)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
#10615 (RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet by luke-jr)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

Sjors · 2018-07-26T19:47:40Z

@gmaxwell:

the GUI should get indicators that an incoming payment was dirty when received

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. -avoidreuse doesn't really capture the above. It's too aggressive / binary an option to be useful in a GUI IMO.

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.

promag · 2018-07-26T19:58:25Z

Maybe add an exclamation mark

@Sjors I like that, and then a tooltip/popup could explain the warning.

-avoidreuse doesn't really capture the above. It's too aggressive / binary an option to be useful in a GUI IMO.

"Avoid" is not binary right?

kallewoof · 2018-07-27T02:15:11Z

I think "dirty" is a confusing concept.

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?

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.

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.

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.

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.

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

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.

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. -avoidreuse doesn't really capture the above. It's too aggressive / binary an option to be useful in a GUI IMO.

What about it is too aggressive?

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.

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.

Sjors · 2018-07-27T09:45:10Z

What about it is too aggressive?

A user might receive their entire salary on a reused address. If the current implementation of -avoidreuse=1 were to become the default, then the UI would need to honor that setting. That means at minimum asking the user for confirmation when they're about to spend these funds. But that would be a very unintuitive question. The Spend screen is not the right place to handle this. Warning the user when they receive such funds is more appropriate.

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 dirty with a subset of quarantined. -avoidreuse=1 could e.g. not spend quarantined without an override, give other dirty coins special treatment, but still spend them if needed. That way the setting can be made a default in a future version, in way that the GUI can honor it.

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.

kallewoof · 2018-07-28T03:00:26Z

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

luke-jr · 2018-07-29T22:02:24Z

N.B. This doesn't actually fully solve #10065, since transactions received with a dirty address are still shown in the GUI / RPC.

luke-jr · 2018-07-29T22:05:40Z

(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)

kallewoof · 2018-07-30T04:20:19Z

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.

kallewoof · 2018-07-30T06:20:05Z

Note: I realized the test was invalid, as getbalance would return only clean amount, so I added support to getbalance and fixed the tests.

jnewbery · 2018-07-30T16:33:19Z

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)

createwallet, getbalance, getwalletinfo, listunspent, sendtoaddress rpc/wallet: listunspent include reused flag and show reused utxos by default

…enable for avoid_reuse wallets

kallewoof · 2019-05-29T09:50:32Z

Removed secondary parameter to SetWalletFlag and restored the UnsetWalletFlag method, and using that instead now.

jnewbery · 2019-06-06T07:34:40Z

ACK 5ebc6b0

(Reran the rebase from 66f3e97 myself and checked the diff)

laanwj · 2019-06-17T08:58:51Z

Concept and code-review ACK 5ebc6b0

Would be nice if this had final sign-off by @meshcollider as wallet maintainer before merge.

Thanks for working on privacy!

//! Forbids inclusion of dirty (previously used) addresses

Hehe. If this use of "dirty addresses" would catch on, maybe people would start avoiding re-using addresses out of a sense of hygiene. One can dream, right.
But agree with @Sjors that "dirty payments" is better to avoid—it's kind of loaded.

kallewoof · 2019-06-17T11:03:52Z

@laanwj f385578 is not the right commit, FYI!

jtimon · 2019-06-17T16:11:16Z

Since I don't know the wallet code all that well, it would take a lot of time to be able to properly review this, but concept ACK.

fanquake · 2019-06-18T00:22:13Z

@achow101 Might also want to take a final look?

laanwj · 2019-06-18T07:54:26Z

@laanwj f385578 is not the right commit, FYI!

Thanks for noticing, fixed, that was a copy/paste error I was copying a lot of commit hashes for #16200.

meshcollider · 2019-06-18T12:36:16Z

Code review ACK 5ebc6b0

Only lightly reviewed the test (8f2e208) but the actual code looks good, thank you @kallewoof for being so proactive in addressing review comments and rebasing! About time this finally gets merged. I'll wait a day for Andrew to review but otherwise I'll merge this tomorrow.

meshcollider · 2019-06-18T12:41:01Z

test/functional/wallet_avoidreuse.py

+        assert_raises_rpc_error(-8, "Wallet flag is already set to false", self.nodes[0].setwalletflag, 'avoid_reuse', False)
+        assert_raises_rpc_error(-8, "Wallet flag is already set to true", self.nodes[1].setwalletflag, 'avoid_reuse', True)
+
+    def test_immutable(self):


In addition to the above TODO, I also suggest this be moved to a different file later (its independent of the avoid-reuse tests)

Makes sense to me.

achow101 · 2019-06-18T14:14:37Z

src/script/ismine.h

    ISMINE_SPENDABLE  = 1 << 1,
+    ISMINE_USED       = 1 << 2,
    ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
+    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,


Is it really necessary to add new ismine types? AFAICT, these are only used as ismine filters which I think could just as easily be done with a boolean. IsMine() isn't returning these values, and ISMINE_ALL_USED isn't used anywhere.

You get most of the combinations with the new "used" flag, because a bunch of calls have a with and without "avoid reuse" state... so I think this is the cleanest approach.

ISMINE_ALL_USED is indeed not used; I added it to indicate that ISMINE_ALL is in fact not include the used ones. It's sort of a comment, with the added side effect that smart IDEs will show it when giving suggestions.

achow101 · 2019-06-18T14:20:33Z

src/wallet/rpcwallet.cpp

+        return NullUniValue;
+    }
+
+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) {


nit: Use RPCHelpMan help{...}. See https://github.com/bitcoin/bitcoin/blob/master/src/rpc/rawtransaction.cpp#L74 for an example.

Will address if rebase becomes necessary, otherwise will try to preserve ACKs and do a quick follow-up post-merge.

I can do this but it would mean some unnecessary calculations when users are not asking for help:

bitcoin/src/wallet/rpcwallet.cpp

Lines 2671 to 2674 in 44d8172

std::string flags = "";

for (auto& it : WALLET_FLAG_MAP)

if (it.second & MUTABLE_WALLET_FLAGS)

flags += (flags == "" ? "" : ", ") + it.first;

Still worth it?

achow101 · 2019-06-18T14:56:00Z

src/wallet/rpcwallet.cpp

            "    \"witnessScript\" : \"script\" (string) witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH\n"
            "    \"spendable\" : xxx,        (bool) Whether we have the private keys to spend this output\n"
            "    \"solvable\" : xxx,         (bool) Whether we know how to spend this output, ignoring the lack of keys\n"
+            + (avoid_reuse ?


I don't think we should have help output that switches on the active wallet for that command. This breaks help listunspent.

Why does it break?

You can do help listunspent without specifying -rpcwallet in order to just get the help for listunspent (and other commands). By making the help dependent on -rpcwallet, this makes the output of help listunspent less helpful because then the help sometimes will include reused and other times not. The help text should be consistent regardless of the other options specified. In general, all options and output should be displayed in the help even if some of those results will be omitted in actual command output. For example, getaddressinfo has a ton of extra fields that may or may not be part of the actual result, but all of them are listed in the help output.

Ahh, I see what you're saying. Will rework into static output post-merge and/or if rebase becomes necessary.

achow101 · 2019-06-18T15:09:45Z

ACK 5ebc6b0 modulo above nits

Reviewed the code and did a couple of manual tests.

Still not a fan of adding new isminetypes but I won't block this on that.

meshcollider · 2019-06-18T23:30:22Z

The few remaining nits are not worth blocking merge on this, it has waited long enough. A follow-up PR to address them would be good.

Sjors · 2019-06-18T23:52:09Z

I took a very brief range-diff look at the changes since my last review 69ec084ace48378c2eecd1b6568639ecfb50469b. Not too many changes. Congrats on the merge!

maflcko

Looked at the documentation and left some comments

maflcko · 2019-06-19T14:41:13Z

doc/release-notes-13756.md

+- getbalance
+- sendtoaddress
+
+In addition, `sendtoaddress` has been changed to enable `-avoidpartialspends` when


Can you explain what that means? sendtoaddress is an RPC call, how would it enable a command line flag (-avoidpartialspends) of a running program?

Changing wording to

In addition, `sendtoaddress` has been changed to always use `-avoidpartialspends` when `avoid_reuse` is enabled, as it would otherwise risk using up the "wrong" UTXO for an address reuse case.

Does that look better?

maflcko · 2019-06-19T14:42:39Z

doc/release-notes-13756.md

+These include:
+
+- createwallet
+- getbalance


What about getbalances?

maflcko · 2019-06-19T14:43:40Z

doc/release-notes-13756.md

+a wallet will distinguish between used and unused addresses, and default to not
+use the former in coin selection.
+
+(Note: rescanning the blockchain is required, to correctly mark previously


Could remove (Note: , since all sentences in this doc are release notes

Removed. I wanted to use it as in "Do note that [word of caution]", but it's no big deal.

maflcko · 2019-06-19T14:47:46Z

src/wallet/rpcwallet.cpp

            "       \"ECONOMICAL\"\n"
            "       \"CONSERVATIVE\""},
+                    {"avoid_reuse", RPCArg::Type::BOOL, /* default */ pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) ? "true" : "unavailable", "Avoid spending from dirty addresses; addresses are considered\n"
+            "                             dirty if they have previously been used in a transaction."},


This makes the help text change at run time. I'd prefer to make it static and explain it with something like "true if wallet falg is set, otherwise unavailable"

Yep, fixed in #16239.

maflcko · 2019-06-19T14:48:09Z

src/wallet/rpcwallet.cpp

                    {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."},
                    {"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."},
                    {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Also include balance in watch-only addresses (see 'importaddress')"},
+                    {"avoid_reuse", RPCArg::Type::BOOL, /* default */ pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) ? "true" : "unavailable", "Do not include balance in dirty outputs; addresses are considered dirty if they have previously been used in a transaction."},


Good catch. Added to #16239.

fanquake added the Wallet label Jul 25, 2018

promag reviewed Jul 25, 2018

View reviewed changes

DrahtBot added the Needs rebase label Jul 25, 2018

kallewoof force-pushed the feature-avoidreuse branch 3 times, most recently from 9e31139 to 8cd0e8f Compare July 26, 2018 08:37

kallewoof force-pushed the feature-avoidreuse branch from 8cd0e8f to 389d97a Compare July 26, 2018 08:41

DrahtBot removed the Needs rebase label Jul 26, 2018

laanwj added the Feature label Jul 26, 2018

jonasschnelli assigned Sjors Jul 26, 2018

promag mentioned this pull request Jul 27, 2018

Should the GUI have a "you're sending again to the same place you sent before"? #3266

Closed

kallewoof force-pushed the feature-avoidreuse branch 2 times, most recently from 57fca7d to e4df634 Compare July 30, 2018 06:17

kallewoof force-pushed the feature-avoidreuse branch 2 times, most recently from 8452b59 to 9337fb3 Compare July 30, 2018 06:26

kallewoof mentioned this pull request Jul 30, 2018

wallet: -avoidreuse with destination filters #13801

Closed

kallewoof added 6 commits May 29, 2019 18:40

wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS

f904723

wallet/rpc: add 'avoid_reuse' option to RPC commands

0bdfbd3

createwallet, getbalance, getwalletinfo, listunspent, sendtoaddress rpc/wallet: listunspent include reused flag and show reused utxos by default

test: add test for avoidreuse feature

8f2e208

wallet: enable avoid_partial_spends by default if avoid_reuse is set

2766955

doc: release notes for avoid_reuse

ada258f

bitcoind: update -avoidpartialspends description to account for auto-…

5ebc6b0

…enable for avoid_reuse wallets

meshcollider approved these changes Jun 18, 2019

View reviewed changes

achow101 reviewed Jun 18, 2019

View reviewed changes

This was referenced Jun 19, 2019

Support not reusing addresses #10065

Closed

wallet/rpc: follow-up clean-up/fixes to avoid_reuse #16239

Merged

maflcko reviewed Jun 19, 2019

View reviewed changes

hebasto mentioned this pull request Jun 25, 2019

refactoring: wallet: Fix GCC 7.4.0 warning #16286

Merged

This was referenced Feb 22, 2020

Bugfix: Wallet: Safely deal with change in the address book #18192

Closed

Bugfix: GUI: Remove broken ability to edit the address field in the sending address book #18194

Closed

This was referenced Apr 7, 2020

Store destdata for change in separate key for backward compatibility #18550

Closed

refactor: Remove CAddressBookData::destdata #18608

Closed

harding mentioned this pull request May 29, 2021

Wallet fingerprinting and avoidpartialspends #22018

Closed

SomberNight mentioned this pull request Sep 13, 2021

add option to avoid forced address reuse (e.g. dust attack) spesmilo/electrum#7497

Closed

	std::string flags = "";
	for (auto& it : WALLET_FLAG_MAP)
	if (it.second & MUTABLE_WALLET_FLAGS)
	flags += (flags == "" ? "" : ", ") + it.first;

Conversation

kallewoof commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jul 25, 2018

Uh oh!

gmaxwell commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jul 26, 2018

Uh oh!

DrahtBot commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Jul 26, 2018

Uh oh!

promag commented Jul 26, 2018

Uh oh!

kallewoof commented Jul 27, 2018

Uh oh!

Sjors commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jul 28, 2018

Uh oh!

luke-jr commented Jul 29, 2018

Uh oh!

luke-jr commented Jul 29, 2018

Uh oh!

kallewoof commented Jul 30, 2018

Uh oh!

kallewoof commented Jul 30, 2018

Uh oh!

jnewbery commented Jul 30, 2018

Uh oh!

kallewoof commented May 29, 2019

Uh oh!

jnewbery commented Jun 6, 2019

Uh oh!

laanwj commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jun 17, 2019

Uh oh!

jtimon commented Jun 17, 2019

Uh oh!

fanquake commented Jun 18, 2019

Uh oh!

laanwj commented Jun 18, 2019

Uh oh!

meshcollider commented Jun 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jul 25, 2018 •

edited

Loading

gmaxwell commented Jul 25, 2018 •

edited

Loading

DrahtBot commented Jul 26, 2018 •

edited

Loading

Sjors commented Jul 27, 2018 •

edited

Loading

laanwj commented Jun 17, 2019 •

edited

Loading