Change address setting is ignored if sending from imported addresses

[DavidCWGA]

If you're sending from an imported address, the "use change address" setting is ignored, as per this line in wallet.py:

if not self.use_change or account == -1:

This situation can occur unexpectedly on a regular non-custom "Send" transaction if you have any imported addresses with a non-zero balance. This can result in the change going back to one of your regular addresses, instead of a change address.

The original intention may have been to prevent accidentally sweeping the entire balance of an imported paper wallet when you spend only some of it, but if a transaction has multiple source addresses, this can happen anyway, as per this thread on bitcointalk:

https://bitcointalk.org/index.php?topic=699082.0

It would be better to always use change addresses, even with imported addresses, if the user has turned the "Use change addresses" option on. This would also reduce the chance of lost coins, since change addresses are regenerated from the seed.

The proposed solution of changing the above line to:

if not self.use_change:

would seem to do the trick.

(Sorry I don't know how to create pull requests.)

[dabura667]

The line in question is https://github.com/spesmilo/electrum/blob/master/lib/wallet.py#L605 on current HEAD.

def add_tx_change( self, inputs, outputs, amount, fee, total, change_addr=None):
    "add change to a transaction"
    change_amount = total - ( amount + fee )
    if change_amount > DUST_THRESHOLD:
        if not change_addr:

            # send change to one of the accounts involved in the tx
            for i in range(len(inputs)): # For #769, if one input is not imported, then assume there will be no problem with paper wallet importing.
                address = inputs[i].get('address')
                account, _ = self.get_address_index(address)
                if account != IMPORTED_ACCOUNT: break

            if not self.use_change or account == IMPORTED_ACCOUNT:
                change_addr = inputs[-1]['address']
            else:
                change_addr = self.accounts[account].get_addresses(1)[-self.gap_limit_for_change]

        # Insert the change output at a random position in the outputs
        posn = random.randint(0, len(outputs))
        outputs[posn:posn] = [( 'address', change_addr,  change_amount)]
    return outputs

This would seem like a better solution:

Check all inputs, and if any one input is a non-imported address, THEN follow the use_change.

I guess whether you choose to change
if not self.use_change or account == IMPORTED_ACCOUNT: to if not self.use_change:
or to do the looping method I wrote above comes down to what Thomas decides is best for the project.

Or maybe he will say it is best as is... who knows.

I think looping and checking for a non-imported input would be the path of least danger.

[DavidCWGA]

Check all inputs, and if any one input is a non-imported address, THEN follow the use_change.

Possibly, but I still think that's potentially misleading. There's certainly no warning anywhere in the UI that turning on "Use change addresses" applies only to generated addresses. I would fully expect the change from an imported address to go into a change address.

[ecdsa]

This is wanted. we already had a situation where a user lost coins because they expected change to go back to their imported address.

[DavidCWGA]

Even if this is wanted, it's currently broken either way. I sent coins from an imported address (along with a couple of generated addresses) and the change want to a regular generated address, not a change address or the imported address. This behavior seems unquestionably wrong.

I sympathize with a user losing coins but the current behavior makes no sense - and would not prevent anyone from losing coins in a similar situation.

[dabura667]

The transaction is https://blockchain.info/tx/7ba987f202323c302b7cc736436a1858579255b131e4f7332caced3a96d945ed

The imported address is: 1DavidGAHiCiiSZgBrbteAkby5AUf6QR3o
Yet the change went to: 1K2TZGCiP8F2sZHtZ73LtyMQJUygFxwUUH

Wouldn't inputs[-1]['address'] be better as inputs[0]['address'] seeing as the input you are checking is input[0]?

[ecdsa]

sorry I did not understand.. reopening

[dabura667]

In the event of the transaction in question... I really do think that because there were inputs from the actual Electrum account that it is safe to assume the change can go to the change address of that account.

Reasoning that "I imported my paper wallet, spent from it, and expected the change to go back to my paper wallet." situation would assume that all inputs in the transaction came from the paper wallet. This is why I thought looping through the inputs and breaking when it found a non-imported address's input would be prudent.

Reasoning that "I assumed imported keys (from bitaddress.org or vanitygen etc) would send change to themselves!" could be fixed by adding a line to the import addresses warning box (such as "change will NOT go to any imported addresses and will only go to your Electrum wallet's change addresses. If you do not know what change is, please look it up before using imported addresses."). As currently, if the user imports address A and B, and sends from both, there is no rational way (except for adding some crazy functionality of letting the user "prioritize imported addresses use for change") to know which address A or B he would expect the change to go to...

However, regardless of this issue you had aimed to prevent, even if the desired functionality is "if input[0] is from imported, then send change to imported" then it is currently broken, and will send change to the LAST input address (input[x-1] where x is the number of inputs).

[dabura667]

Unless maybe you could say something like "if imported address A uses 2 BTC and imported address B uses 4 BTC to spend 5 BTC... the 1BTC of change should be split proportionally between A and B in the ratio of their inputs. so A:B :: 2:4 :: 1:2... so the 1 BTC change would go 0.33333333 to A and 0.66666667 to B....

but this solution would seem a little bit........ more complicated than necessary.