search

trezor / trezor-crypto

:lock: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
MIT License
501 stars 201 forks source link

Fix decred bip32 name #167

Closed matheusd closed 6 years ago

matheusd commented 6 years ago

Standard decred software uses Bitcoin seed instead of Decred seed as the bip32 master key[1][2].

Ideally, trezor should use the same master key as standard decred software to allow better interoperability; in particular to allow users to import their trezor seed (after a small conversion) into standard decred software.

While this breaks any existing decred trezor wallets, no software has been officially released to allow using decred with trezor. Neither trezor web wallet nor any decred wallet has been released to the general public yet. We can also provide a recovery path for any outstanding trezor users that might have already used the recently released firmware 1.6.2 with mainnet decred coins.

We'll need to have this fix applied then wait for the next round of firmware updates to be released before releasing official decred wallets with trezor support.

[1] https://github.com/decred/dcrd/blob/master/hdkeychain/extendedkey.go#L97 [2] https://github.com/decred/dcrd/blob/master/hdkeychain/extendedkey.go#L464

prusnak commented 6 years ago

@saleemrashid @alexlyp thoughts?

jrick commented 6 years ago

I'm the maintainer for dcrwallet and I agree with this change. We intentionally did not change the master key from "Bitcoin seed" so that the key derivation would be the same for multicurrency wallets that did share seeds.

alexlyp commented 6 years ago

Yup agree.

prusnak commented 6 years ago

so that the key derivation would be the same

This would not work, because you use Blake hash in the BIP32 for Decred.

jrick commented 6 years ago

I said key derivation, not string representation :)

We are only using BLAKE256 for the checksum hash.

prusnak commented 6 years ago

No, you use Blake also for BIP32 as per .hasher_bip32 = HASHER_BLAKE

matheusd commented 6 years ago

@prusnak: please look here: https://github.com/decred/dcrd/blob/master/hdkeychain/extendedkey.go#L464

We do not use bip39, so the seed parameter there is our 256bit seed derived from the mnemonic (or, when importing from trezor, the 512 bit seed derived from the bip39 process).

jhoenicke commented 6 years ago

I guess @prusnak was thinking about child key derivation in bip32. However hasher_bip32 isn't used for that. It is only used to compute some part of the fingerprint (seems like a bug to me, shouldn't it be hasher_pubkey for fingerprint).

We use hardcoded SHA-512 for child key derivation in every curve. So indeed the private and public keys are the same as for other secp256k based coins if the seed is changed. Not a problem, since we use BIP44/49/84 to ensure that different coins use different keys.

Questions (Unrelated to this pull request):

  1. Shouldn't hasher_pubkey include the the ripemd160 step? It seems strange that this is extra and always hardcoded as ripemd160.
  2. Should we change hasherbip32 to SHA512 and use it in the ckd* routines? Or should we remove hasher_bip32 and keep it hard coded, at least until the first altcoin decides to use keccak or kerl for this?
prusnak commented 6 years ago

Jochen: I'll address this in pull request to trezor-crypto

prusnak commented 6 years ago

@jhoenicke I addressed your comments in 5d62454c6a7fef411517125b363d3e244414adbc, feel free to check, but the tests pass, so I think the change is fine