Attempt to use empty imported addresses as change

[chris-belcher]

Previously when spending coins on imported-key wallets, the change address
would always be an address from one of the inputs. This address reuse is
pretty bad for privacy because it leaks which output is change.

This commit attempts to find unused imported addresses, and if there are any
then use them as change addresses.

The practical use-case I considered was the situation when using the Electrum
android app to do a cash-in-person trade. Some people might want to bring only
one of their UTXOs to the meetup for safety. This commit allows them to import
that one UTXO plus an unused change address, allowing them to create a
transaction without address reuse.

The diff is a single line which shouldn't cause any serious maintenance burden.

I haven't actually tested it on the Android app but by my reading it uses the classes in electrum/wallet.py all the same.

Here are screenshots showing before and after the edit.

Before:

After:

As you can see, the transaction created now uses the unused imported address as change.

[SomberNight]

See previous discussion at #5353

I agree that your use case makes sense.
However, I think there are scenarios, e.g. if a user imported addresses with different policies (e.g. (1) multisig and singlesig -- though this example is only possible currently for a watch-only wallet; or (2) p2pkh and p2wpkh: should change go to a legacy address to avoid reuse?), where it is difficult to tell where change should go.
Also note that there are users who do not want to use change addresses (see e.g. #6463).

Maybe there could be an option (e.g. in config) to toggle this (send change back to same address, or select arbitrary other address).
Note that there is already an option (not a config option, but rather a per-wallet setting) that toggles this kind of behaviour, except it is currently a no-op for imported wallets:

electrum/electrum/wallet.py

Line 291 in a425ab0

self.use_change = db.get('use_change', True)

electrum/electrum/wallet.py

Line 1268 in a425ab0

elif self.use_change:

In fact, it is a no-op precisely because get_change_addresses returns an empty list for imported wallets.
So that option would allow toggling this behaviour.

There is also the question of what the default value for the option should be.
use_change defaults to True but currently for an imported wallet it's as if it was set to False; so with your change we are effectively changing the default, affecting both existing and new users.

---

Note that longer term I plan to rewrite wallet.py/keystore.py to allow importing output script descriptors (or similar) (#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.

[chris-belcher]

Thanks for the reply.

I disagree with @gits7r's reasoning from the other thread that this would be harmful for privacy, and I've written a comment in that thread explaining why.

If users dont want to use change addresses then an easier way of doing that is to create another imported wallet with just their one single private key. Imported wallets are only really for advanced users, regular users should definitely just create seed phrase wallets. So these advances users shouldn't have a problem creating more imported wallet with just one private key if that's what they want, that brings the old behaviour back if that's what they really want.

I understand that this would change the default behaviour, though I'd argue address reuse is so bad for privacy that it's worth it to slightly break backward compatible behaviour.

On the desktop app this PR isn't even needed, because users can use "!" as an amount in the pay-to-many feature, and that address can act as change. This PR is only needed for mobile wallets.

I'm not sure where to go from here. I was thinking this PR might be something simple to easily merge but it looks like there's one or two unresolved issues.

[gits7r]

I do understand your use-case, and agree mostly with the arguments but the fix is not acceptable.

Basically, Electrum will force the user to use another imported address for change. What if that user imported that address for a particular reason, and the change sent to it will link it to the other one and create problems for the user? This is a decision the user has to take, when creating the transaction. not Electrum. That is why it's an imported wallet.

Either Electrum uses a NEW/FRESH address for the change from an imported private key, either re-use the address. Deciding "imported private key X is good to receive the change from imported private key Y" is a terrible idea that can have bad complications for the users, and it makes Electrum take a decision on user's behalf that it wouldn't normally want to take.

[gits7r]

And don't forget about the wallet back-up problem. Imagine I have just 1 private key saved. I want to restore it every day using an amnesic OS like Tails. I don't have no seed or anything. How do I recover my change? Playing with this behavior is not something we'd like and while of course address re-use is terrible for privacy, imported private keys is not the place where we can make guesses on what the user wants and how they want or don't want to link each imported private key in a wallet between each other.

[SomberNight]

Let's keep the high-level discussion in the other thread (#5353).

[chris-belcher]

My concern with use_change is that it seems pretty hard to edit on the desktop app, and basically impossible to edit on the android app. And the android app was my main use-case in mind for this PR.

It's well worth changing the default behaviour slightly towards the much-more-private solution.

[SomberNight]

My concern with use_change is that it seems pretty hard to edit on the desktop app, and basically impossible to edit on the android app. And the android app was my main use-case in mind for this PR.

Qt: menubar>Tools>Preferences>Transactions>Use change addresses
kivy: top-right-corner>Settings>Use change addresses

[chris-belcher]

Ah! Glad to have misunderstood.

I checked and use_change already has the effect you describe, where it can disable this feature. I simply added a line changing to default option to False for newly created wallets. I'd be personally happy with this for my use-case of importing into android wallets because the option is easy to change, and I think @gits7r would as well because the option is by-default off. It's regrettable that the bad privacy behaviour is the default, but it doesn't matter much since the vast majority of users dont use imported wallets but rather seed phrases.

[gits7r]

I am not against this feature as long as the default option is "off" but I'd just like to state again with apologies for the redundant comment that: given we think imported private key(s) wallets are for advanced users -- there's no argument about this we all agree on this, how is it not easier for these advanced users to add one more address in a new pay-to line with ! as amount or whatever, if they'd like the change to go there? Instead of creating a new wallet with only used private keys or freeze addresses, etc. I think it's easier for the user with the current behavior and more rational for Electrum. Electrum can't know if private key X (regardless it is fresh/unused) is OK to receive the change from private key Y. Maybe it was imported for a different reason, maybe it's waiting for an incoming payment not to be linked with the other imported keys, etc. - lot of scenarios.

My opinion is still that current status quo wrt this topic is OK, while I also heavily agree on the fact that addresses should not be reused, this is particular scenario (imported private key(s) wallets) is simply non-enforceable, except with a lot of tradeoffs and complications for Electrum.

[chris-belcher]

how is it not easier for these advanced users to add one more address in a new pay-to line with ! as amount or whatever, if they'd like the change to go there?

This is not possible on the android app (I think, I wasn't able to do it when I tried a few weeks ago, but I could be wrong, since I was wrong about Tools>Preferences>Transactions>Use change addresses just now)

You are right though, if this actually is possible on the android app then my PR isn't really necessary.

[SomberNight]

@chris-belcher
I did not like that get_change_addresses depended on the history of the wallet.
Though I appreciate you were most likely trying to minimise the diff :)
I've implemented the same functionality a bit differently in fbd8c5f

Thanks for the discussion and your work.

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.