HD/BIP 32 implementation mistake? #8684

issue Ayms openend this issue on September 8, 2016
  1. Ayms commented at 12:51 pm on September 8, 2016: none

    Are you sure of your BIP 32 implementation? Or am I missing something particular?

    My test wallet.dat file shows:

    extended private masterkey: xprv9s21ZrQH143K4Wv2JLdfN8td2qVm8qTU7c7hD36gSfsnfNp7AjjgcaGiem5v7KvJnmpee8JeciN8dGvK5r2KZtEt8N4hgnQ3kRP6mQp2JVL

    KyruVb9pCVBtxHvof6VrxB7GNk7fmqeSh1wFNwjubyuo4BJLfywz 2016-09-05T11:11:48Z hdmaster=1 # addr=1Dk8v9pw3eAC1uXrQd2PhSwAq7pQr3VUs9 hdkeypath=m

    So master seed private key should be: 0bfe13930513cf7ad70bb34161f758417df200d6393e2301d49aa92a9cb6f777

    But KyruVb9pCVBtxHvof6VrxB7GNk7fmqeSh1wFNwjubyuo4BJLfywz corresponds to 4ecf2e71d567072fe2f9cda40873afcaae4224e3f249018621a90dd43e88f8de (+01)

    Even if hdmaster was finally m/0 (because I don’t really see the point of including the master private key in the file and twice) then it should be c9065bdee993a97d148f36ccba46644f67e4572a25a149b02d59bc73a3a1b543

  2. jonasschnelli commented at 1:51 pm on September 8, 2016: contributor

    @Ayms: The hdmaster=1 key is the entropy/seed for the BIP32 chain and not the private key at path m. Internally, the seed is stored as a (standard) key, this is why it is also included in the dump. IMO dumping the seed (hdmaster=1) can be useful.

    But the question is, if the term hdmaster=1 is confusing… maybe hdseed=1 would be better.

  3. jonasschnelli added the label Wallet on Sep 8, 2016
  4. dcousens commented at 2:31 pm on September 8, 2016: contributor
    @jonasschnelli IMHO, hdseed is more appropriate.
  5. Ayms commented at 4:21 pm on September 8, 2016: none

    @jonasschnelli @dcousens

    Ah, OK, not self explainatory indeed (and hdseed looks better), and why then do you include the xprv stuff?

    I would have expected the seed only, but another question is: should you include any of them? Why not let the user chose the seed (ie how can you trust a sw doing this for you, even open source))? Or let the user chose a non bip 32 wallet… or a bip 32 wallet where the tree cannot be parsed… etc…

    Probably a lot of past discussions about it, currently writing something to try to simplify all this for the users (ie create their own wallet the way they like, btw if there is a link somewhere of the final .dat format I would be glad to get it, did read a lot of your (good) docs but did not find it and this would spare some time compared to finding it in the code) and will comment the choices they have, will let you know if you find an interest.

    That’s not a counter productive statement but knowing the theory I find it a kind of amazing to dig into it for real…

    And now 4 days passed and the complete blockchain is still not there, these are part of the issues that Convergence intends to address (sorry not very detailed but the concepts are not vague at all)

  6. jonasschnelli commented at 6:42 am on September 9, 2016: contributor

    I would have expected the seed only, but another question is: should you include any of them?

    The xpriv (extended master private key) was included to provide a flexible and common way how to export your master key and – maybe once – import in into a different Core on non-core wallet. The hdmaster=1 key is there for legacy reasons.

    […] Or let the user chose a non bip 32 wallet…

    Users can disable the HD feature by pass in -usehd=0.

    Why not let the user chose the seed (ie how can you trust a sw doing this for you, even open source))?

    This would be another question. Providing user based entropy can be dangerous and should only be exposed to experts. Starting a HD wallet with a given xpriv should be possible (PR is on the way).

    You need to understand that there where serval tries (search after BIP32 in the closed pull requests) to add HDness to Cores wallet (much more flexible and complex ones).

    The current implementation mainly solves one fundamental issue: the need of repeating create backups.

    With the current BIP32, the “only features” is that users can backup right after creation and can be sure every key they will create in future will be reconstructable.

  7. Ayms commented at 4:42 pm on September 9, 2016: none

    The hdmaster=1 key is there for legacy reasons.

    For legacy reasons??

    Providing user based entropy can be dangerous and should only be exposed to experts.

    Yes, working on this, but like importing keys the possibility should exist

    Starting a HD wallet with a given xpriv should be possible (PR is on the way).

    When do you think it could be available? Why based on xpriv and not the seed? Is it foreseen to have an option to importwallet and override the existing one?

    The current implementation mainly solves one fundamental issue: the need of repeating create backups.

    Yes

    With the current BIP32, the “only features” is that users can backup right after creation and can be sure every key they will create in future will be reconstructable.

    Yes, is BIP32 trustable remains questionable, but that’s what we have, probably it would not be bad to be able to import BIP32 generated keys without any reference to it + the master key/seed + the tree (cf my importwallet suggestion above)

  8. Ayms commented at 12:52 pm on September 13, 2016: none
    Please see bitcoin-wallets
  9. Ayms closed this on Sep 13, 2016

  10. MarcoFalke commented at 7:56 pm on September 13, 2016: member
    So what about renaming to hdseed? Is anyone working on this?
  11. jonasschnelli commented at 12:33 pm on September 14, 2016: contributor

    @MarcoFalke we could change it, though, we would have to be careful.

    1. We are using a similar term in our RPC API (hdmasterkeyid).
    2. The internal function to set a extended keys seed is named setMaster(seed).
    3. Maybe there are implementations in the wild (until the rename to hdseed gets deployed) who rely on the hdmaster=1 string

    I think hdmaster is not entirely wrong. Its not an extended master key (xpriv...), but someone could argue that it’s still a master key (a.k.a. seed).

  12. jonasschnelli referenced this in commit 6738813bcb on May 21, 2018
  13. UdjinM6 referenced this in commit a39799d0e9 on Jun 18, 2021
  14. UdjinM6 referenced this in commit e1bebeb731 on Jun 24, 2021
  15. UdjinM6 referenced this in commit 984a37825d on Jun 26, 2021
  16. UdjinM6 referenced this in commit 7de8332d9e on Jun 26, 2021
  17. UdjinM6 referenced this in commit 21bb444ad3 on Jun 28, 2021
  18. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me