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

Wallet: support import of x/y/zpubkey (BIP49 and BIP84) #616

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

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Oct 6, 2018

Todo:

  • Write tests creating spendable x/y/z wallets, passing purpose as an option

Addresses #606

Supports BIP49 and BIP84

Motivation

I wanted to create a watch-only bcoin wallet that follows my Trezor wallet. For SegWit users, Trezor created ypub and zpub version types (see SLIP32):

Coin Public Key Private Key Address Encoding BIP 32 Path
Bitcoin 0x0488b21e - xpub 0x0488ade4 - xprv P2PKH or P2SH m/44'/0'
Bitcoin 0x049d7cb2 - ypub 0x049d7878 - yprv P2WPKH in P2SH m/49'/0'
Bitcoin 0x04b24746 - zpub 0x04b2430c - zprv P2WPKH m/84'/0'

Currently bcoin only supports xpub and in fact all wallets derive accounts and addresses from the path m/44' so these new paths would be unreachable.

Method

Networks

I started with the keyPrefix object in networks.js to add all the new codes. Note that bcoin uses xprv internally for testnet even though those keys are prefixed with tprv like so:

  "key": {
    "xprivkey": "tprv8ZgxMBicQKsPeXAktk2K3Q92rbVj1C9x6J69knw5e6tiVjRMN9KTTkVAhrvCFLVaYaC8vFhWuLL5nMkE4JTqbZMngnshvDx6LG6dC2y9HSG"
  },

I maintain that convention and use x/y/z throughout, even though the actual user-facing prefixes may be different.

HD private/public key

Key objects have a new property purpose that is either x (default), y, or z. purpose can be passed as an option, but mainly it is derived from an imported extended private or public key, based on the prefix bytes.

Wallet Account

Wallets and Accounts also have a purpose that is either passed as an option or derived from the accountKey option (for watch-only). This is mostly where the x/y/z/ logic gets switched. Each purpose has a specific BIP44 purpose in the derivation path AND specifically dictated address types. For y and z, witness is always true. For y, addresses are always witness-nested-in-P2SH. Normally these addresses could only be derived on branch = 2 from a regular wallet. This PR ALWAYS returns nested addresses for receive and change if the account type is y. Similarly, z wallets always return bech32-encoded native SegWit addresses.

Serialized Account for database

Ok here's where it gets a bit touchy. Accounts are serialized before saving to the database. Currently, all the relevant bits from the accountKey are included EXCEPT the header bytes:

toRaw() ... 
    bw.writeU8(flags);
    bw.writeU8(this.type);
    bw.writeU8(this.m);
    bw.writeU8(this.n);
    bw.writeU32(this.receiveDepth);
    bw.writeU32(this.changeDepth);
    bw.writeU32(this.nestedDepth);
    bw.writeU8(this.lookahead);
    writeKey(this.accountKey, bw);
    bw.writeU8(this.keys.length);

writeKey() ...
  bw.writeU8(key.depth);
  bw.writeU32BE(key.parentFingerPrint);
  bw.writeU32BE(key.childIndex);
  bw.writeBytes(key.chainCode);
  bw.writeBytes(key.publicKey);

So what I did was, steal the TWO leftmost bits of the flags byte and re-purposed them for purpose. This means we can only have 4 values for purpose and we still have 6 bits for actual flags (of which only two are currently used: initialized and witness).

flags |= (purposeBits << 6);

Alternatively, this can be adjusted if we want more future-proof-ness and use less bits for flags and more for purpose, etc.

This has the benefit of some backwards-compatibility:

  • upgrade old->new bcoin with old wallet: no issues, new features available
  • downgrade new->old bcoin with old wallet: no issues, new features not available
  • downgrade new->old bcoin with new wallet using y or z: no failures but y and z wallets will not derive new addresses correctly (they will derive everything as if it is purpose = 'x')

@codecov-io
Copy link

codecov-io commented Oct 6, 2018

Codecov Report

Merging #616 into master will increase coverage by 0.13%.
The diff coverage is 88.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
+ Coverage   61.94%   62.07%   +0.13%     
==========================================
  Files         156      156              
  Lines       26084    26162      +78     
==========================================
+ Hits        16157    16240      +83     
+ Misses       9927     9922       -5
Impacted Files Coverage Δ
lib/protocol/networks.js 100% <ø> (ø) ⬆️
lib/hd/common.js 83.33% <100%> (+1.11%) ⬆️
lib/wallet/http.js 47.91% <100%> (+0.08%) ⬆️
lib/wallet/common.js 34.42% <100%> (+3.39%) ⬆️
lib/wallet/walletkey.js 98.07% <100%> (+0.11%) ⬆️
lib/primitives/keyring.js 75.08% <100%> (ø) ⬆️
lib/wallet/account.js 77.97% <100%> (+0.64%) ⬆️
lib/wallet/wallet.js 68.9% <100%> (+0.31%) ⬆️
lib/protocol/network.js 85.93% <75%> (+2.34%) ⬆️
lib/hd/private.js 71.54% <78.57%> (+3.7%) ⬆️
... and 6 more

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 2148e6b...5c59d4a. Read the comment docs.

@pinheadmz pinheadmz changed the title Wallet: support import of x/y/zpubkey Wallet: support import of x/y/zpubkey (Support BIP49 and BIP84) Oct 7, 2018
@pinheadmz pinheadmz changed the title Wallet: support import of x/y/zpubkey (Support BIP49 and BIP84) Wallet: support import of x/y/zpubkey (BIP49 and BIP84) Oct 7, 2018
@pinheadmz pinheadmz force-pushed the hdtypes branch 2 times, most recently from dc464f6 to 7f5fc42 Compare October 7, 2018 17:18
lib/hd/private.js Outdated Show resolved Hide resolved
@pinheadmz pinheadmz added enhancement Improving a current feature tests Has tests for code change is just an addtional test labels Jan 23, 2019
@tynes
Copy link
Member

tynes commented Jan 25, 2019

@pinheadmz what is the status on the TODOs for this pull request?

@pinheadmz
Copy link
Member Author

Rebased on master. I think this has been a useful branch to maintain for some users even if it doesn't get merged. BIP49 nested-segwit wallets were the default on Trezor for a while and this patch can functionally watch those accounts.

@pinheadmz
Copy link
Member Author

pinheadmz commented Nov 20, 2019

Rebased on master again, to include client inside repo. Also added purpose to options available when creating a wallet from client, including bwallet-cli .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving a current feature tests Has tests for code change is just an addtional test wallet Wallet related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants