bitcoincash-wallet / bitcoincash-wallet

Bitcoin Cash Wallet app for your Android device. Standalone Bitcoin Cash node, no centralized backend required.
25 stars 6 forks source link

Add BIP39 support for backup and restore #12

Open snapfractalpop opened 6 years ago

snapfractalpop commented 6 years ago

I noticed that the MnemonicCode is included, and even initialized with an included wordlist in the onCreate() method of the main activity. I don't see any obstacles to implementing this.

The current backup and restore UI options are in the menu under "Safety". These new methods of backing up and restoring could be either sibling menu items of the current ones, or children menu choices. Alternatively, backup and restore UI could be refactored as another category.

In any case, for restoring, a text input will be required (perhaps a special keyboard is warranted here that minimizes exposure to the sensitive data, and has some autocomplete based on the wordlist?). Likewise, for both backing up and restoring, perhaps screenshots should be prevented (this was done in the signal app, but appears to not be the case anymore).

Any thoughts?

HashEngineering commented 6 years ago

Yes, this app has a BIP32 wallet and it already has a seed.

In another wallet app, I have added this feature. https://github.com/HashEngineering/dash-wallet/commit/6bbafb21a62537e499396883ad813a360a754ea4 (this is only the first commit of many). This probably could use help with making it more secure.

I am not sure about the menu system. In general I prefer fewer changes compared to Bitcoin Wallet, where possible to make merging updates easier.

One thing about this feature is that it will require adding strings in the other languages. Thus far, I haven't had to do that for this app, since no new features have been added. We have no means of getting new translations, currently.

snapfractalpop commented 6 years ago

Good point about keeping merging maintenance easy. Since this is something that both wallets would benefit from, I'm going to see if a similar issue / feature request is assigned at Bitcoin Wallet. If not, I'll open an issue there and see if we can get this done for both wallets.

Re: other languages, I know there are some word lists floating around (there are probably pretty standard ones by now), and for that part of it, it's merely a matter of supplying those assets. If the secure autocomplete keyboard is implemented properly, it should be agnostic to the language and character set used for the words. The rest of the app's string assets are another matter.

I'll follow up on this when I find out about this issue/feature on Bitcoin Wallet.

HashEngineering commented 6 years ago

There is a pull request here, regarding this: https://github.com/bitcoin-wallet/bitcoin-wallet/pull/433

This request doesn’t support a Wallet with a PIN, but mine does.

snapfractalpop commented 6 years ago

Thanks for that, I was just looking through that thread as well as bitcoin-wallet#245. There are a lot of good ideas here that I need to distill down. What is your opinion on the wallet birthdate issue? How much of a performance impact does it have to use a fixed (old) date (on both clients and "servers")?

snapfractalpop commented 6 years ago

Reflecting more on the birthdate issue, the "birth" occurs not at generation time of the seed, but at the blockheight of the first incoming tx (please correct me if I'm wrong). Does it make sense to use blockheight as the birthdate, instead of YYYYMMDD that was mentioned in the other threads?

Also, if the burden to the network caused by replaying to catch up is a large enough concern, a tangential issue / feature may be to allow multiple "accounts" or key chains from different seeds to co-exist. As far as I understand it, currently, the whole wallet is based on a deterministic key chain from a single BIP32 seed. Restoring from different seed words forces a replay each time, whereas importing a seed side by side with another would not require as much replay (going back and forth between them). However, this raises all sorts of other questions / issues that are probably better suited for a different thread.

HashEngineering commented 6 years ago

The birthdate is when the seed is created, which is stored as a long. My thinking when reading the issues in bitcoin-wallet is that the birthdate would be represented by a single word in the mnemonic seed. I am not sure how much data is represented by a word, but that could be converted to a number of months since January 2009 or a block height (or checkpoint). Given that the app doesn't store the blockchain on the device, it may not be able to determine the block height of the birthdate, but it could get the nearest checkpoint block.

One possible scheme: 12 words for the seed, 1 word for the birthdate, 1 word for a checksum

if only 12 words were provided, then it would start scanning from when BIP32 was created.

For Bitcoin Wallet, they have to think about segwit (which would also require a version code to be included). If we adopted the same scheme as bitcoin, we would only support non-segwit versions while having maximum compatibility - that was the main point of this app originally, was to be able to get BCH from Bitcoin Wallet. Sometimes, users actually send BTC to this app and there is a way to retrieve them. In the future when we support the cash address format, that will be less of an issue.

Regarding burden to the network: I have some knowledge here because of another app I am working where it will support two key chains with the same seed (BIP32 and BIP44). When rescanning the blockchain, it only has to do it once, regardless of the number of chains. It will create a single bloom filter to submit to its peers to get relevant transactions. The bloom filter will have two chains of addresses in it.

What you say is right, that two chains can be scanned side by side. That is a topic for another thread as we may want to add other types of chains to be compatible with other apps at a later point in time.

Back to the having the birthdate in the seed, that certainly helps sync the blockchain faster and reduces the load on the servers and for mobile users there is less bandwidth and battery usage compared to syncing from 2014, when HD wallets were created.

snapfractalpop commented 6 years ago

The birthdate is when the seed is created, which is stored as a long.

This is true, but I was thinking, for practical purposes, does the date of generation actually matter, or is it more important to record the date of the first on chain "interaction" with the key chain? That's what had me thinking in terms of block height.

HashEngineering commented 6 years ago

It could be the first on chain interaction.

Currently, when bitcoinj creates a new wallet with a new seed it assigns the birthdate as the time of creation. The wallet protobuf file stores that particular date.

At a later time, which could be never, the wallet will receive a transaction.

We would want to probably keep the original birthdate vs the first block that has a transaction if the birthdate is stored in the seed. A user would see their seed change the last few words otherwise [comparing after installing vs after receiving a transaction]. This would be confusing, though it would still work the same.

This is a very interesting topic.

snapfractalpop commented 6 years ago

That's a good point about the seed changing. I agree that it could be confusing, and given the importance of the seed words it would probably be a bad user experience (I can imagine at least some users even becoming suspicious about such an observation).

One "workaround" would be to have the birthdate be "tentative" until it is shown to the user, at which point it becomes fixed. But I'm doubtful that the savings gained by adding a flag like this would warrant the added complexity, as it would only be saving the replay between seed generation and first use.