wallet: add ability to set account depth on first import; remove hsd soft fork

[chikeichan]

Main goal of the PR is to remove kyokan/hsd fork. Since this issue in hsd is not resolved yet, I have also added a new UI feature to manually set account depth on account import. For users who were previously affected by an issue where wallets with high lookahead values could not rescan properly, this provides a way to bypass the issue.

This should allow Bob to go back to using official hsd and resolve a known issue re: rescan loop!

TODO:

• don't allow account depth to go below 200 (it's already at 200 since it's the default hsd lookahead value)

[sdtsui]

copy changes. merge when ready

[pinheadmz]

If you are going to switch back to handshake-org/hsd v2.3, the wallet lookahead is only one byte and the default value is 200, essentially already the maximum. Why would a user ever need to reduce this? They won't be able to increase it because of the 8-bit limit.

This will also require (I'm pretty sure) a walletDB migration for all users. Bob currently forces a lookahead of 1000 but that value isn't written to the database, meaning everyones' database still has lookahead 10. Only brand new wallets will get a new lookahead value, unless there is a migration (maybe we should include a migrate script in hsd?).

The increased lookahead in hsd from 10 to 200 should be sufficient for most users. The most active wallets may require a second or third rescan to fully recover.

I started working on a real fix for this -- re-re-scanning a block if lookahead is exceeded:

branch)

but after finally talking to JJ about it and reading this bcoin issue I realized my approach was wrong. So I abandoned that and went back to reviewing other PRs and addressing dns issues, etc. I'll try to get back on it starting next week.

As you've discovered it's a big mess. Not to mention that hsd must continue to support full node, spv node, and remote wallet node. The asynchronicity between wallet and node is a big issue. In fact, the current Bob construction with remote wallet node may start to cause bigger issues during rescans since the wallet is totally async from the node, and as blocks get added it can fill up memory: bcoin-org/bcoin#929 (comment)

[pinheadmz]

We've also already discussed user configurable lookahead: #203 (comment)

[chikeichan]

Thanks for the review @pinheadmz

If you are going to switch back to handshake-org/hsd v2.3, the wallet lookahead is only one byte and the default value is 200, essentially already the maximum. Why would a user ever need to reduce this? They won't be able to increase it because of the 8-bit limit.

We did not change data type to u32 so no migration is require.

We've also already discussed user configurable lookahead: #203 (comment)

We are not changing lookahead value, we are only pre-generating address keys before first rescan. Lookahead value stay consistent with hsd default.

In fact, the current Bob construction with remote wallet node may start to cause bigger issues during rescans since the wallet is totally async from the node, and as blocks get added it can fill up memory

happy to discuss on a separate issue as it's unrelated to this PR.

---

Really like to be able to merge back to hsd 0.2.3 while still handle issues for wallet with large lookahead. I know of at least one user's wallets with a lookahead value of 4k+ currently and it doesn't rescan properly with 0.2.3 without this fix.

[pinheadmz]

@chikeichan OHHHhhhh gotcha so sorry for misinterpreting. Yeah I see, you're setting the depth not the lookahead, that is clever. The wallet just derives a ton of keys in advance, concept ACK!

[chikeichan]

A couple new UX changes

[pinheadmz]

So I think this is missing something actually: if you look at account.syncDepth() it also sets the depth in the account metadata which then gets written to the DB. If I'm reading this correctly, what this means is that although the wallet may have indeed saved all the addresses to the database, it still thinks its depth is:

latest address index + lookahead

I'm not sure what the effect of this will be, it might just mean that the wallet will try to derive and save new addresses (that it already knows about, so no problem) up until the user-selected account depth actually matches the wallets internal account depth. In other words, the walletDB may have all the address, but the wallet/account object doesn't "know" that.

When users ask Bob for a new receive address, they will probably get the address at the wallet's internal depth index, which may be an address they have already used.

[pinheadmz]

Ok I see that actually in 646164f is where the accounts depths are being set. So, this is bad because then users are going to put in a high number like 10,000 and end up giving the 10,000th address away for payment, and then not realize they will always need to generate 10,000 addresses every time they recover the wallet. That's what I was getting at here: #203 (comment)

[pinheadmz]

hsd master branch should be considered unstable, I think it'd be more prudent to follow hsd releases either from github or npm (latest is v2.3.0) I think this is fine for now, we need a better hsd release process anyway with release candidates etc to ensure we don't release a bug. Whats interesting about you using this commit in particular is that now Bob will have the "presigned REVEAL" endpoint API: handshake-org/hsd#529

[pinheadmz]

Sorry I am so late to review this, I left a few things to think about and the warning about address reuse may be worth looking into / fixing before release.