spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.27k stars 3.04k forks source link

Imported wallets send change back to "from address". address-reuse #5353

Closed SomberNight closed 5 years ago

SomberNight commented 5 years ago

Dean_Guss reported in #electrum on freenode IRC:

When I import 2 private keys to create a wallet, and 1 address has coins and the other is unused, then I create a transaction which spends the funds in the funded address, by default change goes back to the same address (address reuse), whereas if there is an unused address in the wallet, change should go to the unused address. Imported addresses which are empty don't seem to be among the available change addresses.

It would be lovely if electrum preferred sending change to wallet addresses in this order:

  • Unused, not frozen (Ideal)
  • Unused, frozen (prioritizes not re-using addresses)
  • Used, not frozen
  • Used, frozen (last resort!)
SomberNight commented 5 years ago

Imported wallets send change back to "from address"

ACK, this is indeed the current behaviour.

gits7r commented 5 years ago

I don't think this is a bug or a feature we'd want. I think the current behavior is correct. Wallets with imported private keys can't possible use the same logic as Electrum generated deterministic wallets, which has a purpose for a given address when it's generated, it uses different paths for receive and change and also nests them under separate buckets. This is not technically possible with imported private keys.

How can we know that if an user imported a private key, he wants Electrum to use it for change?

More important, if I freeze an address, this means maybe I have some plans with it, or I froze it with a reason. Would it be OK for Electrum to gracefully use it for change, just to force me not to re-use addresses? Won't the feature "Freeze Address" be confusing in this case? I don't think prioritizing not re-using addresses is more important than what an user explicitly does (freeze), because address re-use is not optimal and not recommended but if would be critical it would have to be addressed at protocol level. Only critical or at least severe practices should be overwritten by the application without caring of user option.

I mean, of course address re-use is bad and not recommended, but this is something to address at protocol level, not at wallet application level. It has been discussed over and over. While the enforcement at application level is totally OK and ideal for Electrum deterministic generated wallets, I don't think I can say the same for imported private keys.

Imported private keys involve something more than creating a simple wallet in a wizzard with next next next (they need to be exported from something else first, etc) so it is safe to assume that an user who does it at least has a clue about what he is doing. Maybe an user imports 2 private keys, one belonging to an address from site A and one belonging to an address from site B. By their policies, you are not allowed to have accounts on both site A and site B. Site A already paid this user, but site B did not yet. Site A and site B and doing blockchain analysis to detect potential policy violations and freeze funds. User spends the payment received from site A to buy something, and when the change goes to the address that is in the withdraw request from site B (which is unused in Electrum, maybe even frozen), they know they are dealing with the same user.

The bottom line is that I think this is invasive for privacy. At most I'd say we should put a privacy warning when dealing with imported addresses: " A wallet with imported addresses doesn't use different change addresses " I wouldn't make the decision for an user how and where to use his imported private key. That's why it is imported, so the user can do whatever.

We should limit as much as we can the amount of actions and decisions Electrum takes on behalf of users.

ecdsa commented 5 years ago

I agree with s7r. we should not implement that.

chris-belcher commented 3 years ago

The privacy reasoning by @gits7r is wrong, here's why:

Address reuse is very damaging for privacy, it is one of the easiest things a user can do to improve their privacy and it's been known about since the very start of bitcoin.

We agree that imported wallets are a feature for advanced users. Almost all regular users should create seed phrases instead.

If a user wants to keep the old behaviour they can create a new imported wallet with just one single private key, or they could freeze an address if that feature is added. Electrum would then create transactions which use that single address as the change address. (I don't know why someone would ever want to do this, but they will always have the option to). Since imported wallets should only be used by advanced users, we know these advanced users will be smart enough to figure this out.

The idea of solving address reuse "at protocol level, not at wallet application level" doesn't make sense and is very impractical. If full nodes were required to stop address reuse at the protocol level then they would have to store a big database of every address that was ever used, and with that they could block transactions which reuse addresses. The much greater storage requirement would make full nodes much more costly to run and so would be harmful to bitcoin's decentralization. Address reuse can only ever be solved at the application level, indeed that's one of the main aims of seed phrases.

The point about privacy leaks happening when two addresses are co-spent together is true, but our advanced user will surely be aware of this and could mitigate it by just not doing that. (And in fact the evidence from co-spending isn't always so conclusive, because the transaction might have been a PayJoin). But none of this changes the fact that the user can still get the old behaviour back by creating another imported wallet with just that one private key, or possibly by freezing addresses.

This feature improves privacy without any downsides, and if the user believes there are downsides they can easily get the old behaviour back.

gmaxwell commented 3 years ago

If full nodes were required to stop address reuse at the protocol level then they would have to store a big database of every address that was ever used, and with that they could block transactions which reuse addresses.

That's not even the worst of it. If I saw a 10 BTC payment to 1apple I could rush out and quickly make a 0.0000001 payment to 1apple, and race the original transaction. If by dust payment ends up first, I would jam the payment of a third party.

You are never worse off privacy wise sending to a new never used address. If you send back to the original address an observer knows it is change. If you send to a new address, they might guess. Later when you spend the change, the way you spend it might link it-- but you're always equal or worse off from the reuse.

gits7r commented 3 years ago

@chris-belcher I mostly agree but I think I'm missing something. Of course address re-use is not encouraged and we should do all possible at wallet (application) level to discourage address re-use (Electrum already does this very good). The thing is, I was referring to a wallet that has a single imported private key inside, thus one address.

If change is to be sent to a separate address, then Electrum needs to generate more addresses. So, it needs a seed maybe -- so it breaks use cases where users have just one backed up private key. We could do some engineering to somehow generate change addresses from the imported private key deterministically so those will also be restored when the user tries to import the same private key, but this is over engineering and ultra-complicated for nothing because we assume in more than 90% of cases whichever user imports a private key and knows the difference between script types is advanced enough to know what address re-use is, and use a separate address for the change when spending a transaction from such a wallet.

The downsides are:

while the pros are 0, there is no privacy gain.

You want proper address handling, generate a normal wallet from seed and then Electrum knows what to do with address management, otherwise we are just over-engineering and over-complicating our code and annoying users with hard-coded behavior.

SomberNight commented 3 years ago

@gits7r Then you misunderstood. The scenario being discussed is what to do in case of an imported wallet that has multiple addresses, some of which are unused. Should change be sent to one of the unused addresses, or should it reuse the "from address"? Currently it reuses.

I agree that the current behaviour hurts privacy. Let me point out some of the concerns I see re choosing another change address automatically (among the unused ones in the imported wallet):

Like I said in the other thread (https://github.com/spesmilo/electrum/pull/7330#issuecomment-855972967), we could instead introduce an option; the safe approach would be to keep the default the current behaviour.

chris-belcher commented 3 years ago

Sending to different address types or script types doesn't have to be bad. The payment address might be of that same address type too, and the user might choose to set a different address type as change so that they can create a transaction where both the payment and change are of the same type. This is already done today in Samourai wallet, which automatically makes the change address type match the payment address type. Remember imported wallets are only for advanced users. Advanced users will have the skills and knowledge to do this.

Users they want the status quo behaviour they can easily get it back by creating another imported wallet with only used private keys. For the vast majority of users, especially newb users, none of this is relevant at all because they use seed phrases. I'm also aiming at this comment, a Tails OS user should definitely not be using imported wallets, instead they should be using seed phrases.

Regarding the losing-the-privkey case, since imported wallets are only for advanced users those users will have to take responsibility for their own storage and backups if they choose not to use seed phrases. Advanced users are also able to understand the transaction preview dialog which gives them detailed information of the transaction they're about to sign. Also worth noting that Electrum doesn't allow creating imported wallets with a mixture of privkeys and addresses, it's either all privkeys or all addresses, so it's not that easy to accidentality send your change to an address you dont control. And of course as a last resort, users can always create a transaction with the old behaviour of sending change to the input address by creating an imported wallet with just one privkey.

gits7r commented 3 years ago

If imported private key wallets are only for advanced users, as you say, which we all agree on, how is it not easier for those users to add the second "change" address when creating the transaction as an additional pay-to line, instead of having Electrum decide by force and guess private key X is good and OK to receive the change from private key Y. Even if private key X is unused, probably it is imported for a different reason, and could complicate things for users. How can Electrum know?

You are saying it's easier for the users who want the quo behavior to freeze address(es) or create wallets with only used private keys or single private keys. I argue it's easier for users that want other behavior than quo to add the second address with ! as amount when creating the transaction. It is less action required from the user and significantly less complications for Electrum.

SomberNight commented 3 years ago

We agree that imported wallets are a feature for advanced users. Almost all regular users should create seed phrases instead.

I agree in principle but I am not convinced that this reflects reality. For example, I am sure there are some noobs that have paper WIF keys (because e.g. they saw some post saying "use paper wallets to be secure") and hence use imported wallets.

how is it not easier for those users to add the second "change" address when creating the transaction as an additional pay-to line

That only works in the Qt GUI; not on Android.

You are saying it's easier for the users who want the quo behavior to freeze address(es)

I personally do not want to change the semantics of "freeze". The OP suggests we do, which is what the original reporter suggested, but I don't think it's a good idea.

having Electrum decide by force and guess private key X is good and OK to receive the change from private key Y. Even if private key X is unused, probably it is imported for a different reason, and could complicate things for users. How can Electrum know?

I think everyone agrees address reuse is bad for privacy and should be avoided when feasible. I also think that, like @gits7r suggests, addresses in an imported wallet cannot be treated as equal in general. In the HD wallet case, different branches of the derivation tree have long had a purpose by convention assigned to them (namely, separate branch for keys to be used for change). However, in a "just a bunch of keys" wallets, software in general cannot guess what the user wants to do with each key and hence which one it can safely use for e.g. change.

As I said in the other thread:

longer term I plan to rewrite wallet.py/keystore.py to allow importing output script descriptors (or similar) (https://github.com/spesmilo/electrum/issues/6016). I imagine the UI will have to expose some functionality to let the user mark descriptors as "to be used for change", which would solve all this.

I think that's the only way to resolve this properly. It should be the user who decides what certain keys/addresses/descriptors are used for.

This is the kind of decision where whatever we decide (re what the software should do by default), there will be complaints from the other side.

SomberNight commented 3 years ago

Made some changes in https://github.com/spesmilo/electrum/commit/fbd8c5f7b039fcabc30bcaa020246efb4e50b50c.

The default behaviour remains the same: imported wallets send change back to the "from address". However, if the use_change option is set, the change is sent to a random unused imported address.