clifordsymack / Electron-Cash

Electrum; Bitcoin thin client
MIT License
6 stars 3 forks source link

Add shuffle bucket derivation paths to wallet #73

Closed imaginaryusername closed 5 years ago

imaginaryusername commented 5 years ago

This is an alternative to #72 , only one of the approaches should be adopted. We can designate 0' as receiving (category 1) and 1' as category 3, and additionally 2' for category 2, 3' for category 4.

All the same logic outlined in #72 can be used, except for the address reuse part, where one cannot simply change a flag at address reuse incoming event. When an incoming tx is detected that reuses an existing address, the wallet should automatically combine both and send them to a fresh address in the appropriate path.

Additionally, loading a virgin wallet should result in a big warning sign; if the user approves, then the wallet should attempt to identify categories outlined in #72 ; if an output is in an inappropriate bucket, it can be moved in a 1-in-1-out transaction to its appropriate bucket. Note that this might result in leaking information via timing observations.

Alternatively, loading from virgin wallet can simply use the flagged method outlined in #72, and the derivation path approach only used for freshly created wallets. If we take this approach, additional logic is probably desired to identify whether a seed is a "new type" wallet or an old school one.

markblundeberg commented 5 years ago

I'd emphasize that this would be very painful to implement. If you look in lib/wallet.py you can see it's completely geared around only two classes of addresses: receiving and change.

Much easier would be to allow the user to send the fruits of shuffle to an arbitrary other xpub (could be another Electron Cash wallet file, another wallet software entirely, or even to a hardware wallet).

fyookball commented 5 years ago

I concur this is too much. We'd have to support a whole new seed type to make it backward compatible.

cculianu commented 5 years ago

We'd have to support a whole new seed type to make it backward compatible.

Indeed. However.. we could philosophically say "onward! upward!". After all .. 4.0 and all. But yeah -- it would be quite invasive and there's a risk of new bugs very late in the game here...

I'd emphasize that this would be very painful to implement. If you look in lib/wallet.py you can see it's completely geared around only two classes of addresses: receiving and change.

Yes. :/

This is the way derivation paths were intended to be used and in hindsight it would have been nice to have had more time to fix all the wallet code to do this.

It would indeed be ambitious.

Darn.. I sort of wish we had woken up to this possibility earlier. Better late than never. Thanks @imaginaryusername for creating this issue....

imaginaryusername commented 5 years ago

Indeed this will be painful to implement, hence I prefer #72, perhaps in combination with shuffling to another wallet.

Mengerian commented 5 years ago

As I think about this more, I think this is a good idea.

The reason being, once coins are shuffled, you really don't want them to ever be handled by a non "shuffle aware" wallet again. Once shuffled coins have been spent,

So for that reason, having the shuffled and shuffled change in a separate derivation path would be good.

Within the shuffled derivation path addresses, there is still value in having address tagging as proposed in issue #72 for things like shuffle level (for multi-stage shuffling).

@fyookball

I concur this is too much. We'd have to support a whole new seed type to make it backward compatible.

I don't think you want it to be backward compatible though. If you open the seed with an old wallet, you don't want the shuffled and shuffled change to be available. It's a good thing to not have it backwards compatible.

You would need a prominent warning when shuffling is enabled that it can no longer be accesses by old wallets.

imaginaryusername commented 5 years ago

If you open the seed with an old wallet, you don't want the shuffled and shuffled change to be available.

For most people I'd think fund availability is way more important than privacy, so if they have one wallet fail, transfer to another and see some of the funds simply disappear from sight, I'd argue that constitutes as a "loss of funds" offense in UX for them. It might create headaches for wallet providers - not to mention that as we implement additional fungibility layers, you'll either need more derivation paths (worsening the problem), or will need flags anyway as you mentioned, which still gets degraded upon transfer.

When transferring to an unaware wallet, between having some funds disappear and having privacy degraded, I'll lean the latter.

Derivation paths do have the advantage in making restoration between aware wallets easier - but I believe this can be partially mitigated considering how early we are, perhaps by standardizing some sort of exportable "flag file".

cculianu commented 5 years ago

Taking 1000 steps back it really is the perfect solution to use a dedicated derivation path. It frees up a lot of technical debt.

Also I agree with @Mengerian that dumber wallets that aren't "shuffle aware" not being able to see the funds could be argued is a feature rather than a bug. Another nicety of that is it may force the industry to take such a "shuffled address" path seriously -- it's viral that way. Other wallets might be compelled to support this derivation path and may even be tempted to be made shuffler-aware (or not?).

In addition, right now there's so much spaghetti in the code to deal with things like "reserving addresses" from the shared path and other race conditions -- and a lot of the GUI code had to be modified to take into account these reserved addresses or not touch shuffle addresses.

The derivation path solution is very clean and a lot of that debt vaporizes.

However I agree with @imaginaryusername that right now it may be a bit of a drastic change.

Aside from UX concerns (which may or may not be easily managed) -- another consideration is that we are already invested in this approach and switching Electron Cash right now to using multiple derivation paths beyond "receiving" and "change" runs the risk of introducing bugs initially (until we thoroughly test such modifications) -- so it may not really be ideal for this release.

So I totally agree with @Mengerian that derivation paths is super sexy -- I also am reticent to really do it now for the initial release.

Hindsight is 20/20. I wish we had thought to do this months ago. It could have been more elegant.

But yeah @imaginaryusername we can totally just flag UTXOs and addresses and see about UX tools for exporting such buckets, etc.

All this needs lots of thought.. which you guys are putting in -- so that's good!

imaginaryusername commented 5 years ago

Be careful about attempting to make other wallets aware of your implementations - the Electrum seed path comes to mind: it predates BIP39, was declared superior, and nobody gave a damn anyway - to this day all Electrum derivatives still use by default seeds irrecoverable anywhere else.

cculianu commented 5 years ago

Ha ha. Touché. Microsoftitis , I like to call it. (The insistance on your ‘superior’ proprietary standard when all evidence points to the fact that you really need to just join the industry).