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

Make HD wallet compatible with wallets generated on https://bip32.org and bitcoin-cli #228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ok2
Copy link

@ok2 ok2 commented Apr 4, 2021

This pull request fixes issue #227 and makes the generation of private (in current BIP32 document named as "hardened") path components handling conform to BIP32. It allows also specifying start/end index with a ' at the end to mark, that the index should be also private. Now it is possible to generate such paths:

  • m/k'/0/i - like default pah on BIP32
  • m/0'/0'/i' - these paths are usually generated in bitcoin core (visible when you do bitcoin-cli dumpwallet)

@junderw
Copy link
Contributor

junderw commented Apr 5, 2021

Please remove the seed WIF addition and the iterations addition.

Pull Requests should be atomic. If subsequent pull requests are based on a previous pull request you should submit them as branches rebased on your first PR, and make a note that they are based on a previous PR.


So now that we've isolated the changes to only the derivation bug fix:

This is a breaking change, so there should be a way for users of the old broken site to recover keys if needed.

Whether this be a hidden button, a JavaScript console command, or just a checkbox ("Use broken hardened derivation" [ ] with a tooltip popup explaining that the old coinbin derived hardened paths incorrectly, so if they generated keys with the old coinbin they should check the box to recover them.) it doesn't really matter... I do think the checkbox with tooltip is easier for users to understand (and lowers risk of github issues increase).

@ok2
Copy link
Author

ok2 commented Apr 5, 2021

@junderw: I have removed now all additional stuff and added the configuration of old behavior to the #settings page. So in this pull request, we have only the BIP32 compliant hardened paths generation and the setting to use the old way.

@ok2 ok2 changed the title Make HD wallet compatible with wallets generated on https://bip32.org and bitcoin-cli. Make HD wallet compatible with wallets generated on https://bip32.org and bitcoin-cli Apr 5, 2021
@junderw
Copy link
Contributor

junderw commented Apr 5, 2021

  1. Please squash your commits
  2. A quick glance looks ok

Please keep in mind that I don't have any merge permissions, nor do I have control over coinbin's site so we still need to wait for approval.

utACK

This should work as intended. Haven't tested it though.

@OutCast3k
Copy link
Owner

Hi,

Sorry for the delay in responding and reviewing this request. I like what I see, but I will need to review it properly this week and possibly merge after.

Thanks for helping this project.

@ghost
Copy link

ghost commented May 16, 2022

@OutCast3k Hello, could you review this feature now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants