Bip32fix

[matiu]

This is an alternative to #97, aim to keep backwards compatibility by default, so bitcore users that upgrade the library without knowing the details of the key derivation changes, wont be on risk of loosing access to private keys.

[braydonf]

Bumping the version to 1.0.0 would prevent unknowingly upgrading the changes. The only change required to maintain compatibility would be to use deriveNonCompliant instead of derive in #97

Any new people using the library would then be defaulting to correct implementation, without needing to know about the alternative.

[kleetus]

should we add v6 to the testing matrix? I don't where this project stands in terms of node versions >= 4

[braydonf]

I have a PR that does this at #79 there are several changes needed in the build to fix this, primarily because of how npm@3 installs dependencies (more flat).

[gabegattis]

Since we aren't going to make this PR as a breaking change, it would make sense to keep these versions here.

[matiu]

@braydonf I am still afraid that someone upgrades to 1.0.0 (by explicitly modifying package.json or similar) without fully understanding the derivation problem and then lost access to its private keys.

Given that the current implementation is not a critical bug, but a deviation from the BIP32 standard, I think is safer to keep the current .derive behaviour, and introduce a new function getChild that behaves correctly. We will also modify the documentation and examples, to only mention getChild as the main derivation function.

[braydonf]

I am still afraid that someone upgrades to 1.0.0 (by explicitly modifying package.json or similar) without fully understanding the derivation problem and then lost access to its private keys.

That's a fair point.

I'm still concerned about there being confusion between derive and getChild, it may be useful to remove derive entirely, and introduce new methods to avoid any unknowing accidental upgrades.

[matiu]

I'm still concerned about there being confusion between derive and getChild, it may be useful to
remove derive entirely, and introduce new methods to avoid any unknowing accidental upgrades.

Yes. Lets do that in a next release (maybe .derive to ._derive), and for now we will add a warning if .derive is used with the old signature.

[kleetus]

LGTM, ack on merging

[gabegattis]

NACK

I don't agree with the strategy in this PR. I don't believe that we should maintain backwards compatibility of the API. That is why we wanted to make a major version update to begin with. I think that retaining the buggy code as the default does more harm than good.

I will write up some more of my thoughts later today.

[matiu]

@gabegattis

I don't believe that we should maintain backwards compatibility of the API. That is why we wanted to make a major version update to begin with.

IMO, if we dont do it this way, it will be much worst than only "broken API compatibility".

Using exactly the same inputs, .derive will deliver different results, with no warning at all. That could be absolutely tragic: lost of funds, lost of authentication keys, etc, for any bitcore-lib user.

I think that retaining the buggy code as the default does more harm than good.
Note that It will be the default only for old users (since documentation is updated to the new API). So new users wont see the bug anymore, and we can eventually remove the old code.

[gabegattis]

I realize my previous comment wasn't particularly helpful and did not do a good job of explaining my thoughts. I also don't think I completely understood what @matiu had proposed.

@kleetus @matiu and I just talked about this on a call, and we have all come to a consensus about how to handle these api changes.

[gabegattis]

For now, we will add .getChild() and .getChildNonCompliant() as new methods. We will leave .derive() as it currently is. We will make this a minor version release on 0.x.x. The new methods will not be documented. This will allow us to start making the fixes in Copay without having to wait for a proper 1.0.0 release. (this will not be the only change in 1.0.0)

When we release 1.0.0, we will make a document in the repo explaining the various different changes (particularly breaking changes) we made, and how to upgrade from 0.x.x to 1.0.0. We will add api documentation for .getChild() and .getChildNonCompliant() and remove the api documentation for .derive(). We will update the .derive() method to always throw an error referencing this problem and pointing to the api upgrade doc.

This solves 2 problems with the existing solutions. With the previous solutions, if a developer upgrades to 1.0.0 without paying attention to the breaking changes they will have a problem:

With the original solution, the naive code would continue to call .derive(), but it would start using new derivation rules without the user/developer knowing about it. This is the worst scenario.

With the more recent solution, the naive code would continue to call .derive() and do buggy derivations. The user/developer might not ever see the warning console.log(), and so they would never know and would continue to do buggy derivations. Also, console.log() is not something that we have anywhere in bitcore-lib, and it is generally not desirable for dependency libraries to log to stdout. Errors are better.

This is a serious issue, and it is important that all users of bitcore-lib be aware of it. By throwing, if a developer upgrades to 1.0.0 without paying attention to the breaking changes, we will make it very obvious that something is wrong. Their tests will fail. If they don't have tests(shame on them), their manual testing will fail. If they don't do manual testing, their customers/users will complain and make them aware of the problem. They will be forced to read the docs and update to .getChild() or .getChildNonCompliant(), which will make it obvious what the code is supposed to do.

[gabegattis]

@braydonf

[braydonf]

If derive throws an error or is removed, I think having getChild (bip32) and getNonCompliantChild (non-compliant-bip32) for API method replacements makes sense.

[gabegattis]

This should be removed.

[gabegattis]

Since we are adding .getNonCompliantChild(), we should leave this method as it was.

[braydonf]

#97 has been updated to deprecate derive with deriveChild and deriveNonCompliantChild, and docs included that explain the bug.

[braydonf]

In the current form, I do not think that this PR should be merged. It's very important that the different derivation methods are not confused.

[matiu]

Closed in favor of #97