wizardsardine / liana

The missing safety net for your coins
https://wizardsardine.com/liana
BSD 3-Clause "New" or "Revised" License
329 stars 58 forks source link

Stop generating addresses without user explicitly asking so #1461

Open edouardparis opened 1 week ago

edouardparis commented 1 week ago

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 commented 1 week ago

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

did i miss something?

edouardparis commented 1 week ago
  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.

benma commented 1 week ago

Thanks for this issue!

It would also be also useful to :

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:

pythcoiner commented 1 week ago

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 commented 1 week ago
  • mark which address was already used

related to #1050, #1030, #809

pythcoiner commented 1 week ago
  • 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 #809)

image

benma commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 :see_no_evil: 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 commented 1 week ago

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.