The hsd wallet has an edge case regarding watch only wallet usage
that results in the incorrect path being returned to the client.
This will result in invalid signature creation because the wrong
key will be derived and used to sign the transaction. Using an
extra API call, it is possible to work around this problem right
now, but it is not ideal and is fixable.
The hsd wallet server uses bip44 key derivation to make key backup
simple. In practice, users often have business constraints that do
not perfectly match 1:1 with the way that bip44 works. The hsd wallet
server uses the Wallet abstraction to represent a mnemonic, or the
root of a bip44 based wallet, and the Account abstraction to represent
the depth 3 index. This is often referred to as the "account index".
A single Wallet can have many Accounts, as creating a new account
is as simple as incrementing the integer at depth 3 in the bip44 path
and rederiving keys. All of the receive addresses and change addresses
derived from the extended public key that represents the account can
be deterministically regenerated and will automatically be indexed
by the hsd wallet database as new blocks come in.
Currently, the hsd wallet increments the account index for each new
account that is created. This makes sense if the mnemonic is on the
server, but that is a less safe practice for large sums of money.
The hsd wallet also supports "watch only wallets", or when the private
keys are not kept on the server at all and instead the depth 3 extended
public key is indexed. This is generally called an xpub and can derive
receive and change addresses, which allows the wallet to watch and index
any transactions but not spend them.
The problem arises when the user adds an xpub from a different index than the next expected index that the wallet is expecting. Since the wallet increments the account index each time and does not validate incoming xpubs, it is possible to index an xpub of any account index at the incorrect account index in the database. This will result in an incorrect path being returned by the database for account index, as it will return the index in the database instead of the actual index of the xpub. Luckily this bug can be worked around by parsing the actual account index out of the xpub, since it gets serialized into it. The current method to use watch only wallets reliably involves fetching the account information and the key information for the utxo being signed. The account information will include the xpub, from which the account index can be parsed from, and the key information includes the branch and index. All of this information together will result in the full path, assuming standard bip44:
m/44'/0'/accountIndex'/branch/index
The current wallet database has a uniqueness constraint on the account index. The proposed solution involves changing the account index to an "account id", which autoincrements and has a uniqueness constraint, but has no relationship to the bip44 account index. This would allow for many xpubs with the same account index to be indexed in the database without a problem (ie different mnemonics, different ancestor in the hd tree). It also means that two database lookups would be necessary instead of one.
Another index could be added to hold the bip44 account index, but that would be duplicating data as the bip44 account index is serialized into the xpub. Pending further inspection of the code, it could be possible to make this change without needing to add extra buckets to the database.
Some questions to take into account:
- Should watch only wallets be strict bip44 compliant?
- Enforce the xpub's index and parent chain matches
- Should there be a catch all type of wallet that allows more flexiblity
In the lib/wallet/account.js, some changes to Account might be necessary.
Account.idis the wallet name, a human readable stringAccount.accountIndexneeds to be altered to not be used at the primary key in the database
Line 24 needs to switch from account index to account id, and everywhere index is metioned, it needs to swtich to id.