Various changes #1
Conversation
| return self.__get_k_from_node(node) | ||
|
|
||
| def __hmac_sha512(self, message_k): | ||
| return hmac.new(message_k, msg=b'bip-entropy-from-k', digestmod=hashlib.sha512).digest() |
There was a problem hiding this comment.
This is wrong. The format of the python function is hmac.new(key, msg, digest), where key is a shared key or salt, and message is the payload you are hashing like a password etc. See the python reference https://docs.python.org/3/library/hmac.html#hmac.new
And examples in the wild in pycoin, python-mnemonic, and Bitcoin Core here and here.
| return self.__hmac_sha512(self.__get_k_from_wif(wif)) | ||
| def bip32_xprv_to_hex(self, index, width, xprv_string): | ||
| # export entropy as hex | ||
| path = f"83696968p/128169p/{width}p/{index}p" |
There was a problem hiding this comment.
I don't agree we should fix the paths here. While you can use those paths, you may not want to, in the same was people may chose not to use BIP44 derivation paths for their wallets. This is the wrong place to enforce the path, as the library is simply performing the transformation of entropy to end result. If you want to standardize it it would need to be in a wrapper above (but I think this is not desirable).
| return BTC.keys.private(secret_exponent=int(entropy[:32].hex(), 16)).wif() | ||
| def bip32_xprv_to_xprv(self, index, xprv_string): | ||
| # app_no = 32 => XPRV to XPRV | ||
| path = f"83696968p/32p/{index}p" |
There was a problem hiding this comment.
Path should not be enforced here. see previous comment
| bits = (words - 1) * 11 // 8 + 1 | ||
| m = bip39(language) | ||
| return m.to_mnemonic(entropy[:bits]) | ||
| # if API to pycoin hadn't been shitcoined: |
There was a problem hiding this comment.
This wins the internet today.
|
|
||
| def test_xprv_application(): | ||
| e = bipentropy.BIPEntropy() | ||
| result = e.bip32_xprv_to_xprv(0, XPRV) |
There was a problem hiding this comment.
I would definitely prefer the library does not use fixed paths. The underlying is reason for these particular functions is the transformation, and if I want to use an arbitrary path, I shouldn't have to rewrite the underlying function to do so. You can always write a wrapper API layer around this.
There was a problem hiding this comment.
IMHO, the purpose of demo code like this is to fix the paths. It's the embodiment of the BIP.
|
|
||
| def test_mnemonic(): | ||
| e = bipentropy.BIPEntropy() | ||
| mnemonic = 'install scatter logic circle pencil average fall shoe quantum disease suspect usage' |
There was a problem hiding this comment.
Need to revert the test cases because hmac param order is flipped incorrectly elsewhere in this PR here
| assert e.entropy_to_bip39(entropy, 24) == words24 | ||
|
|
||
| def test_wif_from_entropy(): | ||
| # PDG: not sure about this? |
There was a problem hiding this comment.
I haven't tested this specific case against Coldcard, that's all.
|
I have incorporated my suggestions here 5dd0317 as I was unable to push to your pull request branch. The changeset introduces a clear API, without having to dig into internals, as well as maintaining the base layer. I didnt correct the param order in the hmac function in this commit to make it readable. |
|
Fixed parameter order and reverted tests in 4a7735c I can go ahead and merge your PR and pull the other two commits. |
We previously discussed this case - if it's necessary. I'm agnostic. But maybe in the context of this BIP, since the purpose of the BIP is to use a root key to derive entropy to then create something else, maybe your original proposal of using the 512bit HMAC entropy to create the XPRV is the more consistent and logical approach. |
|
Closing since key stuff already in master now. |
Now all in sync with latest on Coldcard PR#39
Hopefully a source of ideas & test vectors. I'll understand if this never gets merged.