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

Review BIP 44 Implementation #1853

Closed
ManfredKarrer opened this issue Nov 1, 2018 · 3 comments

Comments

@ManfredKarrer
Copy link
Member

@ManfredKarrer ManfredKarrer commented Nov 1, 2018

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

This comment has been minimized.

Copy link
Contributor

@oscarguindzberg oscarguindzberg commented Dec 15, 2018

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

This comment has been minimized.

Copy link

@stale stale bot commented Mar 15, 2019

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.

@stale stale bot added the was:dropped label Mar 15, 2019
@ManfredKarrer

This comment has been minimized.

Copy link
Member Author

@ManfredKarrer ManfredKarrer commented Mar 15, 2019

Is completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.