Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Receiving address should not offer addresses with unconfirmed transactions #5374

Closed
ziggamon opened this issue May 22, 2019 · 14 comments
Closed
Labels
bug 🐞 topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py user-interface 🔲

Comments

@ziggamon
Copy link

Have had several accidental cases of address reuse because the receive address seems to change only after incoming transactions have confirmed, not after a transaction has been detected, which I think should be the default behavior.

@ecdsa
Copy link
Member

ecdsa commented May 23, 2019

this is a protection against the creation of gaps larger than your gap limit.
the ability to recover coins from seed is more important than rare cases of address reuse.

@ziggamon
Copy link
Author

Can you elaborate? Why would you not include addresses with unconfirmed transactions in the gap limit? Is it because theoretically one of these transactions might not confirm and thus leave a gap?

Besides, even if that is the policy and a gap limit of 20 the wallet could still give out one of the other 19 (--N) addresses before this becomes an issue, which would solve the 99% of the problem without affecting gap limit stuff. When there's 20 unconfirmed transactions an alert could warn about this issue.

The address reuse with unconfirmed transactions is particularly nasty because you end up with sitations where one transaction confirms and not the other, so you might by accident spend just one of them, which then links together a large chunk of the wallet. I wouldn't have brought it up if I hadn't seen it happen many times.

@ecdsa
Copy link
Member

ecdsa commented May 23, 2019

the wallet could still give out one of the other 19 (--N) addresses before this becomes an issue

that is the intended behaviour. if it does not, then there's a bug

@ziggamon
Copy link
Author

that is the intended behaviour. if it does not, then there's a bug

Confirming that it does not and happy we agree that there is in deed a bug :)

@SomberNight
Copy link
Member

is this the same as #3812 ?
#3812 (comment)

@ziggamon
Copy link
Author

Yes, I think so. Noticing I was already commenting in there a year ago :)
I think this one explains the issue more clearly.

@SomberNight SomberNight added bug 🐞 topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py user-interface 🔲 labels May 27, 2019
@SomberNight
Copy link
Member

Ok so with 41802d8,
the text edit will now have a red background styling if the address is already used.

The user is supposed to use the New button in this case.
As the address is not updated automatically, this should avoid user confusion, and also avoid weird interactions with the request_list at the bottom of the tab (only visible if you actually have requests).

@ziggamon
Copy link
Author

ziggamon commented Jul 4, 2019

Updated to 3.3.7 which I presume has this fix and tried it now.

This is what it looks like after receiving an unconfirmed on the same address.

Screenshot 2019-07-04 at 22 55 34

Showing that electrum detected that transaction and knows it has a coin on it.
Screenshot 2019-07-04 at 23 00 50

I think the little red outline behind the copy button is far from enough. It looks like a UI glitch, which do happen in Electrum from time to time and usually dont cause any harm.

Is there any reason why you don't just display the next unused address (in the gap limit)?

@ziggamon
Copy link
Author

ziggamon commented Jul 4, 2019

Tooltip comes up above the text field (white bg), different tooltip above the copy button which has the red background.

In any case I think it would be better to display another address. For example the top address among the "Unused" in "Addresses", that is what I use.

@SomberNight
Copy link
Member

I think the little red outline behind the copy button is far from enough. It looks like a UI glitch, which do happen in Electrum from time to time and usually dont cause any harm.

Yes, I've just noticed this as well. This only happens on MacOS; not sure yet why. On Linux and Windows, the whole text field has that red background (not just the copy icon, lol).

Is there any reason why you don't just display the next unused address (in the gap limit)?

Yes, one reason is that it might be confusing to a casual user that the address suddenly changes; and another is that this text field is overloaded. E.g. if you create requests (which will be visible at the bottom of the tab) and select one of them, then that address will be displayed in the field.

@ziggamon
Copy link
Author

ziggamon commented Jul 4, 2019

Why would it be confusing if it changes when there is an incoming payment? The incoming payment is more reliable and expected than the incoming confirmation which happens at a random time.

If it can't be fixed I'd go as far and suggest to remove the "Receive" screen entirely and have the user rely on the "Addresses" which is more clear and always does the right thing, + educates users that there are multiple addresses and that reusing them is bad.

Anyways, I've said my piece. Just trying to change a behavior that leads to address reuse by default.

@SomberNight
Copy link
Member

Note: the Mac thing is this bug https://bugreports.qt.io/browse/QTBUG-73183
We would need to upgrade to qt 5.12.2 at least, to be able to customise QLineEdits.
However that would bump the min required MacOS version 10.12

@SomberNight SomberNight reopened this Jul 11, 2019
@gits7r
Copy link

gits7r commented Jul 12, 2019

Coloring it with red background is indeed a step forward, but it's not sufficient. Average users will be confused, as they might have no idea what the colored background means. Might not have an idea that address re-use is not recommended.

Since we have the code to actually detect when this race condition happens (and color the background of the address) why not simulate a push on the "New" button without the user actually doing anything, until the address there is not used and does not have to bare a red background? At least this way the effect is that all the time under "Receive" tab we have a fresh unused address.

We might want to freeze entirely the "Receive" tab until Electrum is fully synchronized. There's nothing that we can do about this when Electrum is running offline, except print a discrete warning / description on the Receive tab that We cannot know if the address there has been used or not and address re-use is discouraged, etc.

@SomberNight
Copy link
Member

The receive tab works different on master now. This issue is thus no longer relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py user-interface 🔲
Projects
None yet
Development

No branches or pull requests

4 participants