fix leading zero and add test

[klim0v]

Correction of key length with initial zero
The length of the child private key must be at least 32

[klim0v]

@jcvernaleo @jakesyl

[jcvernaleo]

@klim0v thanks! Kind of swamped today but hope to get to this one by tomorrow.

[jcvernaleo]

@klim0v the decred code for this is identical to our original code (it was just copied from here of course).
https://github.com/decred/dcrd/blob/master/hdkeychain/extendedkey.go#L320
Do we think decred's code needs this fix too? And if not, why?

[klim0v]

@klim0v the decred code for this is identical to our original code (it was just copied from here of course).
https://github.com/decred/dcrd/blob/master/hdkeychain/extendedkey.go#L320
Do we think decred's code needs this fix too? And if not, why?

Yes, I can help you do this.

[klim0v]

@jcvernaleo
Is everything okay here?

[jcvernaleo]

@klim0v sorry about that (and thanks for the reminder). Totally flake out on this. Could you just squash it and it will be good to go.

Then I think you should make a matching PR on decred too.

[davecgh]

This is technically accurate, however, I will point out that fixing it means it will break anything that has derived keys using the existing code and therefore it is a breaking API change that should be accompanied with a major version bump. This change can and will suddenly cause people to be unable to derive the private keys they need to spend any coins which pay to public keys that were originally derived under the current code.

I'm really not sure what type of versioning the project is using nowadays, but this change could easily make coins unspendable for some consumers.

This is a decision for the maintainers of the project now, but I suspect this change should be rejected as is, despite it being technically correct, due to that. A likely reasonable solution would be to expose the existing derivation semantics via a legacy function and make sure to loudly warn about the difference.

Do we think decred's code needs this fix too? And if not, why?

This fix can't be deployed in Decred as is because of the aforementioned issue. The primary wallet software in widespread use derives keys based on the current code, so, any change in this regard would absolutely make people lose access to their coins which is obviously not acceptable.

[jcvernaleo]

Okay, definitely do not want to break things for existing usage.

@klim0v what was the initial motivation for this?

[davecgh]

The motivation was certainly because the existing code does not conform with the BIP32 specification
which results in different derivations in some circumstances than the spec calls for.

In particular the spec defines the function ser256(p): serializes the integer p as a 32-byte sequence, most significant byte first. While it's not really explicit about it, that implies it might need to be padded with leading zeroes to ensure it is a full 32-bytes. The problem is that big.Int in Go is arbitrary precision and consequently it strips the leading zero bytes in its serialization. The result is that values which end up with one or more leading zeroes have a different serialization than the spec calls for. In terms of the math operations this doesn't matter since e.g. 0x0123 = 0x123, however, that serialization is fed through HMAC-512 which is sensitive to the exact serialization.

So, the fix is indeed technically accurate, as I stated above, however, changing it also has the aforementioned problem of breaking people who have secured coins with the the non-conformant derivation code.

[klim0v]

The reason for this correction was the fact that our organization https://github.com/MinterTeam creates blockchain services, in which the creation of addresses is similar to the algorithm in ethereum.

When I wrote the SDK for golang using transaction signing, it turned out that in about 1% of cases, the addresses created from the seed phrases do not match the addresses received in the SDK written in other languages.

For this I used libraries such as:

• Invalid private key generation miguelmota/go-ethereum-hdwallet#10
• Invalid private key generation foxnut/go-hdwallet#2
• Invalid private key generation wenweih/ethereum-cold-wallet#2

which used your library to create the master key.

I agree with @davecgh that this fix, which is not part of the major version, could lead to the loss of wallets. But at the same time I would like to nevertheless restore the technical correctness of the algorithm, and at least indicate this inaccuracy in the description. Otherwise, it will lead to conflicts in other new projects.

[jcvernaleo]

Hmmm, okay, so I guess we have a few options here (and if I missed anything, someone please correct me):

1. Just document that things are not technically correct but then consider this something we are stuck with and move on. I very much think decred should do this as leaving it undocumented is not good at all.
2. Bump the major version, call this an incompatible upgrade, and go along that way.
3. Is there some other option like adding a legacy and a 'correct' method?

Thoughts? Opinions? @Roasbeef @jakesyl ?

[jcvernaleo]

Just requesting changes here so we don't accidentally merge while trying to decide the best way forward. Not sure if there is a more elegant way to do that.

[davecgh]

It looks like BIP32 was updated to add a 3rd test vector specifically for this case. Depending on how this proceeds, I would suggest adding that new 3rd test vector to TestBIP0032Vectors accordingly.

[klim0v]

It looks like BIP32 was updated to add a 3rd test vector specifically for this case. Depending on how this proceeds, I would suggest adding that new 3rd test vector to TestBIP0032Vectors accordingly.

Thanks for the link. I added my test to TestBIP0032Vectors. Do not close PR, please. I will contact bitcoin/bips and suggest that they add my test to the documentation.

[klim0v]

A lot of packages use your bip32 implementation 😔

[jcvernaleo]

So does anyone have thoughts on the three sets of options I posted above?

[davecgh]

As I said before, I think the right approach is to fix it, offer a legacy version which retains the old behavior (internally you just abstract the core logic into a function with a flag to strip the leading zeros on the child key or not and then call it with true/false from the respective exported function), ensure the major version is bumped, and loudly warn consumers about the change and make it clear that if they have created seeds and derivations using the existing code they must use the legacy version. However, for all new seeds, they need to use the new version.

[junderw]

Question for @Roasbeef

This code is used in all versions of LND as of current, correct?

[klim0v]

I have added an optional parameter for this. We can make another wrapper for him. In the next major version, already do a wrapper with a check already for a false value

[klim0v]

I think it can be added to the new release without changing the major version

[Roasbeef]

This code is used in all versions of LND as of current, correct?

Yeah, btcwallet uses it internally, so the default lnd configuration does to derive all its keys. FWIW, with the way we made our seed format (aezeed), it's possible for us to add a new "external" version, which would then have btcwallet use this new (breaking) v2 API for those users moving forward. I agree with davec that we need to change the major version (to make this explicit), then point new users as this new more compatible API.

[Roasbeef]

This solution strikes me as being...inelegant. With this for the foreseeable future, the API is forced to carry a parameter that conditionally fixes an issue of spec compliance. This isn't backwards compatible as the comment indicates since this is actually itself a breaking API change, so it warrants a v2 major version bump IMO.

Another option would be to add a new method that always applies the leading zero fix, with the existing public one calling that with the fix toggled off.

[klim0v]

This solution strikes me as being...inelegant. With this for the foreseeable future, the API is forced to carry a parameter that conditionally fixes an issue of spec compliance. This isn't backwards compatible as the comment indicates since this is actually itself a breaking API change, so it warrants a v2 major version bump IMO.

Can you explain why this cannot be considered backward compatible? This parameter is optional and does not correct the default error if it is absent.

[junderw]

If someone created an interface in their app under the assumption that ExtendedKey fulfills it, and their interface has the call signature of Child(i uint32) (*TheirInterface, error) suddenly ExtendedKey no longer fulfills it.

Therefore it is breaking.

[Roasbeef]

I've come around to this approach, if you can update this PR before #182 does, I think it can land.

[Roasbeef]

Something like https://golang.org/pkg/testing/quick/#CheckEqual can likely be used to find any other divergences across the various BIP 32 implementations.

[Roasbeef]

Of course the upside of not doing a new major version (which requires users to manually switch over as there would be a new sub-directory), is that if the API is broken, they'll be forced to make a decision when they upgrade, vs somehow hearing about the new more compatible API.

[cfromknecht]

FWIW, with the way we made our seed format (aezeed), it's possible for us to add a new "external" version, which would then have btcwallet use this new (breaking) v2 API for those users moving forward.

Should be able to get away with only an internal version bump for aezeed since there's no modification to the serialization/interpretation of the internal payload.

[junderw]

Also @GuiltyMorishita might want to look at this too. This bug will affect Ginco wallet compatibility with other wallets.

[GuiltyMorishita]

@junderw Thank you for mentioning me. Ginco Wallet does not use the package, so there is no problem.
I tried this test just in case and it passed.

[LasTshaMAN]

@klim0v is there still anything useful in this PR, or can we close it ?

[jakesylvestre]

@Roasbeef/@klim0v what do we want to do here? :Looks like this pr needs to be updated if we're going to consider merging + @davecgh has pointed out some significant concerns

[onyb]

As far as I remember, this issue has been fixed in #182. We now have the Derive function, which is a replacement for the buggy Child function (now removed). The old behaviour is still available via the DeriveNonStandard function.