sparrowwallet / sparrow

Desktop Bitcoin Wallet focused on security and privacy. Free and open source.
https://sparrowwallet.com/
Apache License 2.0
1.28k stars 185 forks source link

[feat]: Add support for wallets with mixed address types #466

Open chrisguida opened 2 years ago

chrisguida commented 2 years ago

Edit: I created this issue in order to explore the possibility of importing a Mycelium wallet to Sparrow.

I believe this would be a combo-type descriptor as seen here: https://github.com/bitcoin/bitcoin/blob/e04720ec3336e3df7fce522e3b1da972aa65ff62/doc/descriptors.md?plain=1#L47

But it's possible Mycelium doesn't follow this format exactly. You cannot export the wallet as a combo descriptor for instance, so implementing this feature may require adding the ability to import simply a concatentation of an xpub, a ypub, and a zpub as one wallet instead of 3 separate wallets.

Supporting exporting from old wallet implementations is a convenient way to get users to switch to Sparrow.

craigraw commented 2 years ago

It would actually need to be 3 separate wallets under the hood, each with their own addresses and separate transaction histories.

I'm not sure implementing this is wise however. It would reduce the transparency of the wallet (for example, you would now need to to look at three Addresses tabs instead of one). It is also not clear which of the three wallets should be the default, or whether Taproot wallets should be included too. The server load would increase as well, at least by two additional gap limits. In short, the additional complexity may be greater than the benefit provided.

chrisguida commented 2 years ago

It would actually need to be 3 separate wallets under the hood, each with their own addresses and separate transaction histories.

Maybe just implement combo descriptors then? I can probably generate a combo descriptor from the privkey, just need to study the docs a bit more.

It would reduce the transparency of the wallet (for example, you would now need to to look at three Addresses tabs instead of one).

Can you explain what you mean by "reduce transparency"?

It is also not clear which of the three wallets should be the default

This could be implemented by allowing the user to toggle between the different address types, as Mycelium already does (and many other wallets already do). This would also solve the above problem. Simply display the addresses for the address type the user has selected.

or whether Taproot wallets should be included too.

Seems like Taproot would behave the same as any other address type. That is, it doesn't seem to hurt to simply include it even if the user isn't using it.

craigraw commented 2 years ago

Maybe just implement combo descriptors then?

It's the same thing - there are still 3 types of addresses to be watched.

Can you explain what you mean by "reduce transparency"?

Some wallets don't show the list of addresses at all. On that spectrum, any changes that hide addresses behind additional UI selections reduce transparency - i.e. they make it less obvious where funds have come into the wallet, and from which addresses they are being sent.

This could be implemented by allowing the user to toggle between the different address types

Indeed. But many users may not understand this is necessary, and it complicates the UI by adding another layer of indirection. I am not sure this cost is worth it to support legacy script types.

it doesn't seem to hurt to simply include it even if the user isn't using it.

Apart from the concerns above, the additional burden on the server is a consideration as well, along with load times and wallet memory usage.

chrisguida commented 2 years ago

I guess the larger question here is: do you never foresee a situation where supporting multiple script types from the same private key would be useful? It seems to me that as the number of script types proliferates, this functionality will be needed more, not less. But perhaps I'm missing something.

It's the same thing - there are still 3 types of addresses to be watched.

Bitcoind implements support for combo descriptors:

$ bitcoin-cli deriveaddresses "combo(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)#lq9sf04s"
[
  "1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH",
  "1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH",
  "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4",
  "3JvL6Ymt8MVWiCNHC7oWU6nLeHNJKLZGLN"
]

Bitcoind treats such a combo wallet as a single "wallet", rather than as three separate wallets. My understanding is that Sparrow uses bitcoind's wallet interface. Perhaps bitcoind's support for combo wallets would need to be extended in order to fully support implementing combo wallets in Sparrow (eg being able to search a range of addresses, or being able to only search for addresses of a specific type). But in general, combo wallets strike me as a useful feature, not just to support legacy address types. Otherwise I'm wondering why they were implemented in bitcoind at all.

Some wallets don't show the list of addresses at all

Really? All the wallets I know of tell you whether you're using 1, 3, or bc1 addresses, at least on the next receive address. Mycelium shows you a QR and lets you toggle between the three types by tapping the QR. It also shows the derivation path for each type.

Blockstream Green shows your your account and your address types as "sub-accounts". I've actually never seen an example of a wallet that hides this from a user, but I believe you that they're out there.

image

I suppose we can quibble here, but I don't see how this "reduces transparency" at all.

Indeed. But many users may not understand this is necessary, and it complicates the UI by adding another layer of indirection. I am not sure this cost is worth it to support legacy script types.

Again, users are used to this exact feature in many other wallets. I don't think they will be confused at all. If you're arguing that it will be extra effort to add this toggle to the frontend, fair enough. But I don't think the effort will be wasted since, again, this seems like a useful feature because having different script types in the same "wallet" doesn't seem like a situation that will go away soon.

Apart from the concerns above, the additional burden on the server is a consideration as well, along with load times and wallet memory usage.

Why would there be additional server load with mixed address types? You don't need to fetch 3x as many addresses if you have 3x as many address types. The same number of addresses will do. But perhaps I'm misunderstanding how Sparrow fetches address data.

And if there is additional server load for additional address types, it seems this could be mitigated by allowing the user to deselect address types they aren't using. This would be additional frontend work, yes, but could be worth the effort, depending on how difficult it is to add a checkbox.

Transisto commented 2 years ago

There may be a need for a guide on how to import legacy address types but this should not be a burden on the wallets. These users should then move on, There is maybe 0.1% of services that don't allow withdrawal to segwit addresses. These services should usually be avoided for multiple other reasons.

https://en.bitcoin.it/wiki/Bech32_adoption

A good reason why it might be beneficial to support all address types is for the extra privacy of sending the change to the same address type. That's what Samourai does. Also if someone tries to import a Samourai wallet he might not see all his BTC.

craigraw commented 2 years ago

It seems to me that as the number of script types proliferates, this functionality will be needed more, not less.

I don't agree (at least not in the medium term). Users should already be moving on from P2PKH and P2SH-P2WPKH address types, if only because transacting in them is more expensive and their active anonymity sets are gradually decreasing. There is little reason to move on to single sig Taproot - there are no real fee reductions and the anonymity set is currently very small in any case. Multisig Taproot will eventually increase this anonymity set, but without fee pressure it's difficult to move users to new wallets.

at least on the next receive address

Showing the full list of addresses is different (admittedly difficult on mobile). My point was not about the list per se, but rather indicating the movement of funds to and from each address in one screen. This is the transparency I'm referring to.

Again, users are used to this exact feature in many other wallets.

My experience is that users are often confused by different script types.

Why would there be additional server load with mixed address types?

Consider that each wallet needs to have 20 watched addresses as a gap limit. So you are, in an empty wallet, watching 60 addresses instead of 20. This is how all BIP32 wallets work.

it seems this could be mitigated by allowing the user to deselect address types

Unfortunately, it's not that simple. There is no clear point when a server stops working - on an application level it is very difficult in practice to detect, and it's not practical to expect the user to manage this.

craigraw commented 2 years ago

A good reason why it might be beneficial to support all address types is for the extra privacy of sending the change to the same address type.

This is probably the most compelling reason, but it does become less compelling over time. Certainly when Samourai was initially implemented Segwit was much lower in adoption. There is something to be said for increasing the bech32 anonymity set by creating fewer change outputs in legacy script types.

chrisguida commented 2 years ago

Hey Craig, thanks for the response.

I don't agree (at least not in the medium term). Users should already be moving on from P2PKH and P2SH-P2WPKH address types, if only because transacting in them is more expensive and their active anonymity sets are gradually decreasing. There is little reason to move on to single sig Taproot - there are no real fee reductions and the anonymity set is currently very small in any case. Multisig Taproot will eventually increase this anonymity set, but without fee pressure it's difficult to move users to new wallets.

All of this is true, in the medium term (as you noted). However, in the long term, there will always be multiple "in-flight" script types, because we're never going to find the "perfect" script type. Seems like a good long-term investment to simply support mixed types sooner rather than later. It's just annoying to have to migrate to a different wallet every time some new and shiny script type gets released. It's a much better UX to simply add the new script type to the same wallet and allow the user to gradually (or, all at once) migrate their UTXOs.

Showing the full list of addresses is different (admittedly difficult on mobile). My point was not about the list per se, but rather indicating the movement of funds to and from each address in one screen. This is the transparency I'm referring to.

Which screen are you referring to here? Perhaps "Addresses"? Could this list not be sorted chronologically regardless of script type? Apologies if I'm misunderstanding.

My experience is that users are often confused by different script types.

I think this is something that can be solved in UX rather than simply disallowing the mixture of different script types. Users will always have to deal with multiple script types no matter what, so better to help them understand what's going on IMO.

Consider that each wallet needs to have 20 watched addresses as a gap limit. So you are, in an empty wallet, watching 60 addresses instead of 20. This is how all BIP32 wallets work.

Sure. But why does it need to be this way? Surely the gap limit could be divided by the number of supported script types (since a user isn't going to be generating 3x as many receiving addresses just because their wallet supports 3x more script types). Or else the gap limit could be maximized on the selected script type while minimized on the others. 20 is just an arbitrary number.

Unfortunately, it's not that simple. There is no clear point when a server stops working - on an application level it is very difficult in practice to detect, and it's not practical to expect the user to manage this.

I'm not sure I understand your point here. Are you saying the user might think the server isn't working if they deselect a certain address type and it doesn't load?

This is probably the most compelling reason, but it does become less compelling over time.

Sure, privacy is definitely improved by only using one script type in your transactions, but it doesn't seem like enabling this would require any additional work. Sparrow already has the ability to select UTXOs, and to create wallets that are unmixed. If users want to keep their UTXOs of different script types in different wallets, they can already do so.

There is something to be said for increasing the bech32 anonymity set by creating fewer change outputs in legacy script types.

Agreed, this is actually a great argument in favor of supporting mixed wallets. You can generate change addresses to the "default" wallet, increasing adoption of that script type.

craigraw commented 2 years ago

To be clear, Sparrow can already handle mixed script types in a wallet - it needs to be able to do so in order to support receiving to segwit-enabled BIP47 wallets. What I'm not convinced on yet is the pros here outweigh the cons, as discussed above.

A few notes:

Could this list not be sorted chronologically regardless of script type?

It is already sorted by index, which is important. It would be much more cumbersome in the UX to sort by script type and then index, particularly since some script types would have more addresses than others.

Sure. But why does it need to be this way?

This is an entirely different debate, but reducing the gap limit from 20 is not recommended for general purpose receive chains in BIP32 wallets.

Are you saying the user might think the server isn't working if they deselect a certain address type and it doesn't load?

No, I'm saying server issues are always not easy to detect (they can be intermittent or partial), and increasing load causes increasing issues.

chrisguida commented 2 years ago

To be clear, Sparrow can already handle mixed script types in a wallet - it needs to be able to do so in order to support receiving to segwit-enabled BIP47 wallets.

Oh, neat! Is this implemented yet? Not seeing it in Sparrow 1.6.2.

It is already sorted by index, which is important. It would be much more cumbersome in the UX to sort by script type and then index, particularly since some script types would have more addresses than others.

Ok, that makes sense.

This is an entirely different debate, but reducing the gap limit from 20 is not recommended for general purpose receive chains in BIP32 wallets.

Right, presumably because that's the maximum number of unused addresses that are likely to be waiting to receive payments at once. Again, there's no reason to expect a user to generate 3x more addresses just because their wallet supports more script types. But it would definitely take some experimentation to determine the best way to monitor addresses in this situation. Perhaps this functionality should be implemented in the electrum server rather than the wallet. Just hand the combo descriptor to the electrum server and have it figure out what to do.

No, I'm saying server issues are always not easy to detect (they can be intermittent or partial), and increasing load causes increasing issues.

Ahh, gotcha.

craigraw commented 2 years ago

Oh, neat! Is this implemented yet? Not seeing it in Sparrow 1.6.2.

Sorry, I should have been clearer - the underlying infrastructure is (mostly) present to support multiple script types in a single wallet interface, the UI to create such a wallet in general sense is not. I'm still thinking about this.

Again, there's no reason to expect a user to generate 3x more addresses just because their wallet supports more script types.

I don't see any way around this. A gap limit of 20 is extremely standard, and sometimes needs to be increased. Every BIP32 receive chain requires its own gap limit. That said it's not a deal breaker, just another consideration.

chrisguida commented 2 years ago

the underlying infrastructure is (mostly) present to support multiple script types in a single wallet interface, the UI to create such a wallet in general sense is not. I'm still thinking about this.

Ok cool. That does sound very relevant to this feature request.

I don't see any way around this. A gap limit of 20 is extremely standard, and sometimes needs to be increased. Every BIP32 receive chain requires its own gap limit. That said it's not a deal breaker, just another consideration.

Gotcha. Perhaps the gap limit could also be user-configurable. Just a thought.

Anyway, I appreciate your consideration on this, looking forward to seeing where it goes! Let me know if you need anything from me to make it happen.

AndySchroder commented 4 months ago

Related, but when going to File->Import Wallet, if you type in a seed phrase, you get a Discover button. What does sparrow do when trying to import a seed that has funds in multiple script types?