Bip32 compliance-round3

[gabegattis]

#96 #97

[braydonf]

As mentioned in #115: It's very important that the different derivation methods are not confused, as such by including both derive (non-BIP32) and deriveChild (BIP32), that is not clearly defined.

Futhermore:

• Removing the hdkeycache can be it's own atomic change, and is unrelated to fixing bip32 derivation.
• Is this for 1.0.0? privatekey.toBuffer() has breaking changes included here, and docs indicating this is indented for 1.0.0 branch.

[gabegattis]

Thanks for the continued feedback.

It is true that the cache can be removed separately. My idea was that if we know we are going to remove it anyway, it seems reasonable to forgo adding additional complexity to the cache logic when we could just remove the whole thing now. If we want to make it an atomic change, which is probably a good idea, we can make that change first, and then make the bip32 changes on top of that. I will make a separate PR for that. #117

To clarify, this is not for v1.0.0. This is meant to be an immediate change so that we can start making future-compatible changes to copay/BWC. v1.0.0 will involve other changes, and we don't want to wait until everything else for v1.0.0 is ready before using this new bip32 code. The goal is to add new methods for now and not break the API yet.

When we were first talking about this, the idea was for these new methods to be undocumented. I have warnings in the jsdocs that HDPublicKey.deriveChild(), HDPrivateKey.deriveChild(), and HDPrivateKey.deriveNonCompliantChild() will not be officially supported until v1.0.0, as I wanted to leave open the possibility of making breaking changes to those methods in the meantime(not that I expect that to happen). To avoid confusion, it is probably best to leave the documentation changes until v1.0.0. A warning about derive() is still probably justified though.

Good catch about the privateKey.toBuffer() breaking change. The HDPrivateKey code is using bn.toBuffer() directly instead of privateKey.toBuffer(), so we can delay the change to privateKey.toBuffer() to avoid breaking the API. That change can wait until v1.0.0.

[isocolsky]

ACK

[matiu]

ACK

[braydonf]

Still don't think it's a good idea to have both derive (that has the bug) and deriveChild (that has the bug fixed) together. It's very import that these are not confused. So I do not think this should be merged.

NACK

[kleetus]

ACK

[dabura667]

Also I would like to point out the deprecation warning says 0ffffff... is the problem (which implies a 1 / 16 rate of occurrence.) but it's actually 00ffffffff... (the first byte of the Buffer object created by BigNumber.toByteArrayUnsigned() get's dropped because it doesn't zero pad)

The actual occurrence rate in my tests is 1/256 per hard derivation, so each BIP44 wallet there's a 3/256 chance the first account hits the problem, and each additional account has a 1/256 of being a bad account.

[Giszmo]

Testvectors are merged to the BIP

[NicSil]

Waiting for the fix.. Any date?

[matiu]

@NicSil it is already fixed.

[Giszmo]

@matiu sorry, but in which release is it fixed? Is that available via npm? I think @NicSil is not eager to use the git repo.

@NicSil you would also have to adapt your code by using the new deriveChild() method and not the deprecated derive().

[NicSil]

Here is the code used to get the address:

var Mnemonic = require('bitcore-mnemonic'); // https://bitcore.io/api/mnemonic/

exports.createWallet = function(seed) {
	var seed = new Mnemonic(seed);
	var hdPrivateKey = seed.toHDPrivateKey(); // https://bitcore.io/api/lib/hd-keys
	
	var derived = hdPrivateKey.derive("m/44'/0'/0'/0/0");
	var privateKey = derived.privateKey;
	var address = privateKey.toAddress();
	
	return {
		seed: seed,
		address: address
	};

How to you suggest to use deriveChild() to fix it?

[matiu]

1. Update bitcore-mnemonic to the latest version 2. use .deviceChild instead of .derive

…

On Sun, Mar 26, 2017 at 6:19 AM, NiS ***@***.***> wrote: Here is the code used to get the address: var Mnemonic = require('bitcore-mnemonic'); // https://bitcore.io/api/mnemonic/ exports.createWallet = function(seed) { var seed = new Mnemonic(seed); var hdPrivateKey = seed.toHDPrivateKey(); // https://bitcore.io/api/lib/hd-keys var derived = hdPrivateKey.derive("m/44'/0'/0'/0/0"); var privateKey = derived.privateKey; var address = privateKey.toAddress(); return { seed: seed, address: address }; How to you suggest to use deriveChild() to fix it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#116 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGCHJq_G4SObpHI-kdiHPhqN1ugqXDvks5rpi2JgaJpZM4LGCud> .

-- Matías Alejo Garcia @EMATIU Roads? Where we're going, we don't need roads!

[NicSil]

Thanks.
After updating as previously stated:

For "opera gadget typical file radio obvious inform concert beauty price also become"

I get:
address derive(): 1C93ChVKrK5Y6xLkw1DcFyEWe2o4VZqQBX
address deriveChild(): 1C93ChVKrK5Y6xLkw1DcFyEWe2o4VZqQBX

For "surface poem manual curve size banner truly just object soup inhale craft"
I get:
address derive(): 1CyEfZAvqfuw58hcwP9YAAjj1BcLLavBmi
address deriveChild(): 18qfi9NJpU1szrVRFLR2QmKqkd17G2UGd7

1CyEf...LavBmi was the one that did not work in Mycelium (but in Copay).