wallet: add "accountPath" attribute to handle edges cases with watch-only accountKeys

[BluSyn]

Motivation:
In case of watch-only wallets, accountIndex is "arbitrary".
It simply maps to internal walletdb account instead of
BIP44 account path as defined in original accountKey.

It is important to return the actual accountIndex for use-cases
such as hardware-wallets, so clients can easily reproduce the
original path for this account.

To maintain backwards compatbility with existing wallets,
it is easiest to add this as a separate accountPath field
instead of refactoring all the existing uses of accountIndex.

Note: This was discussed out-of-band and after many iterations we decided adding a new field is probably the simplest solution. At the very least it results in the fewest code changes.

[pinheadmz]

I think the account key's child index is already hardened, and therefore already > 0x80000000
so I think this xor is redundant. And actually if the imported xpub for whatever reason is not hardened, we probably need to leave it that way

[BluSyn]

This returns the expected value for hardened keys, which should always be the case with xpubs. (eg, returns accountPath '1' instead of '2147483649`). Perhaps there are edge cases of non-hardened xpubs, but not sure how to handle that one...

[pinheadmz]

Oh I see its an xor not or so the constant gets cancelled out 👍

[tynes]

@braydonf @tuxcanfly @nodar-chkuaselidze @boymanjor
Could we review this PR because its a pretty bad bug for watch only wallets and we need to merge it into both bcoin and hsd. Kyokan is waiting on the patch for hsd

[braydonf]

So I think that this is likely a validation issue for watch-only wallet and account creation:

The accountKey (the xpub) for wallet creation is not validated to be the BIP44 m'/44'/0' derived xpub, that the account public keys would be derived. From testing I was able to create a wallet with both m'/44'/0' and m'/44'/0'/0' without issues (using the example at #688).

Because the account key is hardened (m'/44'/0'/0'), the wallet key (m'/44'/0') isn't able to create the account keys with only the wallet xpub, and thus they need to be included when accounts are created.

When creating accounts for that wallet, I was also able to create an account for the wallet derived at m'/44'/0'/5', however because it was the second account created, the account was assumed to have been derived at m'/44'/0'/1' and the account would equal 1 — reproducing the issue for the pull request.

The fingerprint in the account xpub can be used to verify that this is a derived xpub of the wallet, and further that the account of the xpub matches what is expected.

Being able to import arbitrary account numbers into the wallet could potentially lead to issues during recovery of the wallet from the seed, as there could be around 2 ^ 32 possible accounts to scan for instead of scanning sequentially from 0 to n.

[nodech]

When importing xpub, you are expected to import m/44'/0'/{index}' -> xpub, account is not managed by bcoin for that, basically you end up having a mapping from local account index to xpub account index (that does not actually matter for wallet) -- and this PR just exposes xpub account path, just a cosmetic change. (No migrations)

You must not import private key for that.. if you really want to import private key, just import master and it will derive xprivs and xpubs for you.

To limit wallet to one Master, you could use parentFingerprint(fingerPrint of m/44'/0'/) to validate origin of xpubs is the same for any xpubs they you imported (Currently you can import any xpub with totally different masters and it wont complain, I used that as a feature couple of times)

As for bcoin.index <-> xpub.index mapping, it will need bigger refactor. wallet will need to allow gaps defined by users.

As for recovering from the seed, I believe user is in charge of naming/creating those accounts, without naming them accounts are not much of use. Unfortunately, current API wont allow to use arbitrary number as a accountIndex, even if you want to have indexes - 0, 100, 1000 only. (Only way to use it this way would be watchOnly)

[braydonf]

Regarding gaps between accounts:

Software should prevent a creation of an account if a previous account does not have a transaction history (meaning none of its addresses have been used before).
https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#account

[nodech]

Then, simplest solution for this problem would be to enforce importing xpubs from 0.

1. Store xpub parentFingerprint(or get it from existing account to avoid migration)
2. Enforce accountIndex === xpub.index
3. Enforce first.parentFingerprint === xpub.parentFingerprint.

--
Gaps can happen for watchOnly, when you have some accounts but you only want to watch several on specific machine, e.g. watch personal account on one machine and watch work account on another.

edit:
Because of watching different accounts from different nodes can happen, we can't enforce above rules.
Also parentFingerPrint can't be verified, but can serve as basic validation for this purpose.

[braydonf]

Okay here are the test cases that demonstrate the issues: #696

[braydonf]

I don't completely follow the case with gaps between accounts for the purposes of running on different machines.

Consider this case:

• Alice creates a new wallet externally (w/ private mnemonic phrase) without access to blockchain state (aside from the watch-only wallet), and makes a backup of the phrase.
• She then creates a watch-only wallet and adds the first account xpub.
• The first account receives many transactions.
• Alice creates a second account externally and adds it to the watch only wallet.
• The second account does not receive any transactions.
• Much later, she then creates new account account externally, and adds it to the watch-only wallet.
• The third account receives funds.

That creates the case that deviates from what BIP44 specifies, and in an attempt to rescan based on the original mnemonic phrase could miss potential coins.

[nodech]

When you are importing xpub you can't miss anything during rescan, you are only interested in this particular xpub(a.k.a. account public key), you are deriving branch/index from there and only those are being taken care of by bcoin. It does not actually care what's the accountIndex/Depth in the account, you only care about addresses you have to watch based on xpub.

Account gap that you mean is when you have Master key available and if we supported automatic account creation on master key import (that's not the case and I don't think it should be, you can't recover names, better create those accounts one by one (meaning derive next account, accountDepth in our case)). In that case you CANT skip account, so bip44 rule is still preserved, you have to create one by one.

If I only want import watchOnly xpub with account index 10 - let's say I use it for something specific, I don't care anything else (on this machine or maybe right now), I should not have to import xpubs as watchOnly I don't care about.

[braydonf]

The wallet has the perspective of many accounts as a whole organized set, for example a BIP44 wallet. In the case the wallet has a master private seed, it can derive all accounts. In the case of watch-only, that derivation has to be done externally. The external derivation is unaware of which accounts do or do not have transactions (no access to chain or block data), and is unable to apply the BIP44 logic that "Software should prevent a creation of an account if a previous account does not have a transaction history". Thus the watch-only wallet needs to take care of that.

[braydonf]

I've created an issue specifically for the gaps between accounts: #698

[tuxcanfly]

Agree with @braydonf on this, even though it sacrifices certain use cases. Ideally maybe there could be multiple wallet types where each wallet is specialized for a particular use case. But until then I believe #696 is the 'correct' way to handle this.