bisq-network/bisq

Review BIP 44 Implementation

ManfredKarrer opened this issue · 3 comments

ManfredKarrer commented

For having the BSQ and the BTC wallet keys on different BIP 44 paths we added support for BIP 44 which was only partly supported in BitcoinJ.
It would be good to get review and if needed improvements added. As we use that code base in production changes have to be backward compatible.

We use the same seed words for both BTC and BSQ wallet but we store the wallets in differnet files.
Having the same seed words increase usability (would be cumbersome fi users need to backup 2 tiems the seeds). Having 2 differnet files for storage add more resilience in case a file gets corrupted. The different path guarantees that a user cannot be accident use his BSQ in another wallet as BTC (e.g. using the seeds).

oscarguindzberg commented

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 bitcoinj/bitcoinj#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 bitcoinj/bitcoinj#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 below 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 is 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

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

stale commented

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ManfredKarrer commented

Is completed.