Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate hd public keys when creating accounts #696

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

braydonf
Copy link
Contributor

@braydonf braydonf commented Feb 8, 2019

Validates extended public keys (e.g. xpub) during account creation for watch-only wallets:

  1. That the accountKey (extended public key) is at the correct depth, the account depth.
  2. That the accountKey is at the expected BIP44 account index, to avoid issues as described in wallet: add "accountPath" attribute to handle edges cases with watch-only accountKeys #689 and Wallet: support import of x/y/zpubkey (BIP49 and BIP84) #616 with the account index being unknown and mismatching.
  3. That the accountKey shares the same master key (skipped for now), as to avoid collisions of the BIP44 account index and further mismatches of the signing keys.
  4. That the accountKey matches the wallet derivation scheme (skipped for now), for example BIP44, BIP45, BIP49, or BIP84.

@codecov-io
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #696 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
- Coverage    62.1%   62.02%   -0.08%     
==========================================
  Files         147      147              
  Lines       25379    25381       +2     
==========================================
- Hits        15762    15743      -19     
- Misses       9617     9638      +21
Impacted Files Coverage Δ
lib/wallet/wallet.js 67.57% <100%> (+0.08%) ⬆️
lib/mempool/fees.js 54.59% <0%> (-6.57%) ⬇️
lib/hd/private.js 68.28% <0%> (+0.44%) ⬆️
lib/hd/common.js 88.88% <0%> (+6.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0551096...6b6565f. Read the comment docs.

}

assert(err);
assert.strictEqual(err.message, 'Master key mismatch.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're testing collisions maybe the error should say "key already exists at index..."

Copy link
Contributor Author

@braydonf braydonf Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's testing if the key is derived from the same parent. The collision of the accountDepth is one of the potential issues of mismatching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other issue is that if account 0, 1, 2 are from key-a, and account 3 is from key-b, it's a mismatch issue again, and an attempt to sign account 3 with key-a would fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parentFingerPrint can be used to validate that the accounts all derived from the same key to avoid those issues.

@braydonf braydonf added wallet Wallet related watch-only Watch only wallet labels Feb 8, 2019
@tynes
Copy link
Member

tynes commented Feb 8, 2019

I'm almost certain this will break existing infrastructure built on bcoin

@braydonf
Copy link
Contributor Author

braydonf commented Feb 8, 2019

Any existing watch-only usage that:

  • Uses 1 wallet with more than 1 hd tree can be migrated to use 1 wallet to 1 hd tree. This will avoid confusion about which master key to derive keys from, account index collision issues, and limit gaps between accounts.
  • Have accounts with gaps > 1 can be archived with all the possible private keys and addresses generated. A new hd tree generated and be used.
  • Have a subset of the entire account space (gaps), and no actual gaps between accounts, can include the entire account space as to verify and avoid any future issues with gaps.

This gives the benefit that the mnemonic seed phrase is the only data needed to backup for recovery of coins. Otherwise the account space, which is often updated, becomes necessary to backup similarly.

@braydonf
Copy link
Contributor Author

braydonf commented Feb 9, 2019

I've separated out the issue about gaps between accounts: #698

@braydonf braydonf changed the title wallet: validate hd public keys when creating accounts Validate hd public keys when creating accounts Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wallet Wallet related watch-only Watch only wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants