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

Stop generating addresses without user explicitly asking so #1461

Open
edouardparis opened this issue Nov 13, 2024 · 14 comments
Open

Stop generating addresses without user explicitly asking so #1461

edouardparis opened this issue Nov 13, 2024 · 14 comments
Labels
Bug Something isn't working as expected Robustness Something could be made les brittle

Comments

@edouardparis
Copy link
Member

edouardparis commented Nov 13, 2024

Otherwise, the gap will be too big and some rescan with default gap limit on other wallets will not detect funds.

In the receive panel, the load should not fetch a new address at all.

@pythcoiner
Copy link
Collaborator

Otherwise, the gap will be too big and some rescan with default gap limit on other wallets will not detect funds.

In the receive panel, the load should not fetch a new address at all.
I think if we follow this way the address index should be incremented only if:

  • user copy address / show the qr code in receive panel / click on generate new address:
    image

  • user click on verify on hardware device from receive panel and copy the adress:
    image

did i miss something?

@edouardparis
Copy link
Member Author

  1. the panel should be empty at start up
  2. if the user clicks on generate a new address, it adds a address to the list, and the list is kept in the current state of the panel.

That's it, nothing to infer from user copying or not the address.

@nondiremanuel nondiremanuel added Bug Something isn't working as expected Robustness Something could be made les brittle labels Nov 13, 2024
@benma
Copy link

benma commented Nov 14, 2024

Thanks for this issue!

It would also be also useful to :

  • mark which address was already used
  • show the derivation index

Slightly related: how do change address derivation indices work? There you would also want to avoid the change index to increment every time you make (and abandon) a PSBT to avoid high gap limits. Probably best to pick the first unused change address, where unused means:

  • no confirmed/unconfirmed tx that uses it
  • no local PSBT created that uses it

@pythcoiner
Copy link
Collaborator

pythcoiner commented Nov 14, 2024

Slightly related: how do change address derivation indices work? There you would also want to avoid the change index to increment every time you make (and abandon) a PSBT to avoid high gap limits. Probably best to pick the first unused change address, where unused means:
no local PSBT created that uses it

i do not agree on this, this will lead to address reuse: once the user is able to export the psbt we should increment the index
(as of now the PSBT is not save in the db until it have at least one signature or if user click on save, but the user can export it)

@pythcoiner
Copy link
Collaborator

  • mark which address was already used

related to #1050, #1030, #809

@pythcoiner
Copy link
Collaborator

  • show the derivation index
    you can see it if you click on verify on hardware device, but i agree maybe we should display is the adress list (or be part of GUI: display a list of addresses. #809)

image

@benma
Copy link

benma commented Nov 14, 2024

Slightly related: how do change address derivation indices work? There you would also want to avoid the change index to increment every time you make (and abandon) a PSBT to avoid high gap limits. Probably best to pick the first unused change address, where unused means:
no local PSBT created that uses it

i do not agree on this, this will lead to address reuse: once the user is able to export the psbt we should increment the index (as of now the PSBT is not save in the db until it have at least one signature or if user click on save, but the user can export it)

You could save it then even before it has a signature, avoiding nearly all of the cases where address re-use could accidentally happen.

Imho avoiding address reuse is less important than being able to discover the funds. If you draft 6+ txs but only broadcast the 7th, the tx might not be discovered by other wallets, for example.

Doesn't recovering in Liana involve sending all UTXOs anyway, and usually also when simply refreshing UTXOs? In this case, avoiding address re-use is even less important, as one links them in these txs.

@pythcoiner
Copy link
Collaborator

Imho avoiding address reuse is less important than being able to discover the funds. If you draft 6+ txs but only broadcast the 7th, the tx might not be discovered by other wallets, for example.

hum in which case? actually, the only other wallet that can be used to recover a wallet crafted by liana is bitcoin-core and iirc it have a lookahead of 1000, right?
i think other wallets have a look ahead of at least 20/30?

@benma
Copy link

benma commented Nov 14, 2024

Imho avoiding address reuse is less important than being able to discover the funds. If you draft 6+ txs but only broadcast the 7th, the tx might not be discovered by other wallets, for example.

hum in which case? actually, the only other wallet that can be used to recover a wallet crafted by liana is bitcoin-core and iirc it have a lookahead of 1000, right? i think other wallets have a look ahead of at least 20/30?

Hopefully there will be many more wallets in the future that can import descriptors.

For change addresses, the BitBoxApp has a default gap limit of 6, Electrum has 10, Sparrow has 20. Not sure what BlueWallet has, I assume 20 or less. The higher it is, the worse the UX is as the account load/sync time is longer.

With change addresses in Liana, it's not as critical as with the receive addresses, as long as you only increment it if the user actually drafts a tx, and users hopefully will not draft many in parallel.

Still, with my proposal above address re-use would be almost completely mitigated, I don't see much of a downside to it. No reason to introduce unnecessary gaps.

@pythcoiner
Copy link
Collaborator

For change addresses, the BitBoxApp has a default gap limit of 6, Electrum has 10, Sparrow has 20. Not sure what BlueWallet has, I assume 20 or less. The higher it is, the worse the UX is as the account load/sync time is longer.

i dont see any valid reason to have a so low look ahead, sorry.(maybe i overlook something)
i think all electrum server implem accept batches of at least 100 requests, so you can subscribe to 100 adresses in a single TCP request and you should receive notifications in batches.

@pythcoiner
Copy link
Collaborator

Doesn't recovering in Liana involve sending all UTXOs anyway, and usually also when simply refreshing UTXOs? In this case, avoiding address re-use is even less important, as one links them in these txs.

i really think we should fix this

@benma
Copy link

benma commented Nov 14, 2024

i dont see any valid reason to have a so low look ahead, sorry.(maybe i overlook something)

It's more about being compatible with the rest, even if the rest is not doing it right 🙈 You won't be able to change every wallet's gap limit that easily. When it comes to potentially undiscovered funds, it's better to err on the safe side and have Liana try to keep the gap as low as possible that it generates. The user should not be able to create an arbitrarily high gap by clicking around in the UI.

@pythcoiner
Copy link
Collaborator

to be clear: my point is we have to fix our index being incrementing w/o strong reasons, but when it comes to choosing between risk of address reuse vs incrementing the index we should go for the latest: you can always find your coins later if you have messed w/ the index by clicking some buttons, but you cannot get back on a privacy loss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working as expected Robustness Something could be made les brittle
Projects
Status: Todo
Development

No branches or pull requests

5 participants
@benma @edouardparis @nondiremanuel @pythcoiner and others