[bisq-network/bisq] Review BIP 44 Implementation (#1853)

[Oscar Guindzberg]

I reviewed BIP44 implementation on bisq.

**Bisq's BIP44 support**

- A Bisq node has both a BTC wallet (m/44’/0’/0’ base derivation path) and a BSQ wallet (m/44’/142’/0’ base derivation path). Using multiple Wallet instances for multiple BIP44 accounts is the recommended approach, so that is good news.
- Custom key derivation path is implemented by subclassing/overriding DeterministicKeyChain.getAccountPath().
- That is done on BisqDeterministicKeyChain and BtcDeterministicKeyChain.
- BisqWalletFactory, BisqKeyChainFactory, BisqKeyChainGroup and BsqWallet enable the use of those DeterministicKeyChain subclasses.


**Minor issues found**
- BtcDeterministicKeyChain uses coin type 0 (bitcoin mainnet). Testnet should use coin type 1. Same thing for BisqDeterministicKeyChain.
- Recovering from seed: If a bisq user recovers from seed using a seed created by another wallet that “fully” supports BIP44 and uses multiple accounts, accounts other than 0 won’t be "discovered" by bisq and the user will be informed to have less BTC than the ones she really has.

**BIP44 in bitcoinj**
- BIP44 is not fully supported by bitcoinj. Arbitrary path support was added over time so bitcoinj users can use any base derivation path they want. BIP44 specifies that wallets should support multiple accounts and also specifies how a wallet would discover the accounts given just the seed. That discovery process would be very inefficient for a light wallet like bitcoinj.
- A long time ago devrandom added partial support of BIP44 (ie just arbitrary paths) to bitcoinj by subclassing/overriding DeterministicKeyChain.getAccountPath() (See https://github.com/bitcoinj/bitcoinj/pull/339) - that code is included on bisq's bitcoinj fork. 
- During 2017 new code was added to bitcoinj master to allow arbitrary paths without subclassing/overriding DeterministicKeyChain.getAccountPath() (see https://github.com/bitcoinj/bitcoinj/pull/1341). That code was not merged into bisq bitcoinj fork. There is no current bug that requires merging that code. Merging that code would allow bisq to get rid of BisqDeterministicKeyChain, BtcDeterministicKeyChain, BisqKeyChainFactory, BisqKeyChainGroup, BsqWallet. Merging that code is not an easy task. IMHO that should be done only if bisq's bitcoinj fork is going to be updated with all the changes on bitcoinj master.
- There are other commits with bugfixes related to BIP44 (see bellow for a comprehensive list) 

**bitcoinj release status**
The latest major release is v0.14 (May 6, 2016)
Bisq bitcoinj fork is based on v0.14.4 (Feb 7, 2017)
The latest release patch to v0.14.7 is (Mar 30, 2018)
bitcoinj keeps receiving contributions but there was no major release for 2.5 years. Segwit has partial support on bitcoinj master.


**Reviewed bisq code**
- bisq's bitcoinj fork
- Classes on bisq.core.btc.setup package

**Reviewed bitcoinj commits/PR/issues**
- Flexible DeterministicKeyChain 
 - march 2015
 - contributed by devrandom 
 - https://github.com/bitcoinj/bitcoinj/pull/339
- Allow to create wallets with arbitrary path (e.g m/44'/0'/0/0) and sync/receive/send coins correctly
 - https://github.com/bitcoinj/bitcoinj/pull/1341
 - Feb to nov 2017 (Not included in the bisq bitcoinj version)
- Unnecessary accountPath included in Watch Key Methods
 - Jan 2018
 - https://github.com/bitcoinj/bitcoinj/issues/1501
 - https://github.com/bitcoinj/bitcoinj/pull/1509
 - https://github.com/bitcoinj/bitcoinj/commit/be382c68000590b9205c078e3cd8247c0ab2b905
 - https://github.com/bitcoinj/bitcoinj/commit/d3dca96a3cfdccdce65712eed0207d1c1c0e7e49
- Add support for importing HD keys that can sign transactions / Add ability to import an account key that allows for a wallet to spend
 - Jan 2018 / Feb 2018
 - https://github.com/bitcoinj/bitcoinj/issues/1502
 - https://github.com/bitcoinj/bitcoinj/pull/1514 
- DeterministicKeyChain: Allow encrypting / decrypting DeterministicKeyChain with arbitrary path
 - March 2018
 - https://github.com/bitcoinj/bitcoinj/pull/1537
 - https://github.com/bitcoinj/bitcoinj/pull/1534
- https://github.com/bitcoinj/bitcoinj/issues/1532 

**Keywords**
To search the bitcoinj repo I used these keywords
- BIP44 (and its variants "Bip 44", "Bip-44")
- Arbitrary path

**BIP44 Reference**
https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki
https://github.com/satoshilabs/slips/blob/master/slip-0044.md

**Related stuff**
- Multibit attempt to add BIP44 support to bitcoinj long time ago https://github.com/Multibit-Legacy/multibit-hd/issues/441 . It looks that https://github.com/bitcoinj/bitcoinj/pull/339 was merged instead.
- Work in Progress on segwit addresses derivation path https://github.com/bitcoinj/bitcoinj/pull/1563


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/issues/1853#issuecomment-447598673
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20181215/3d530987/attachment-0001.html>