fix issue with 11 bit padding messing up checksum check.

[rubensayshi]

when experimenting with using BIP39 mnemonics for encoding non-standard entropy sizes we ran into some odd bug where lpad(index, '0', 11) was making the checksum check fail.
this doesn't occur with the recommended BIP39 entropy sizes, but unless the library is made to limit it's scope completely to the recommended sizes (at which this bug doesn't occur) I think it should be fixed.

in entropyToMnemonic, the chunks for the words we (entropy + checksum).chunk(11), this means the checksum can be split between chunks and the final chunk can be between 1 and 11 bits.

in mnemonicToEntropy, the bits from the words are padded to 11 bytes each and then we slice off the checksum which can contain a padding byte.
by redoing the same step as in entropyToMnemonic and concatenating entropy with newChecksum we always get a comparable result.

[rubensayshi]

@dcousens the tests failed on 0.10 .. ?!

[dcousens]

I literally just saw that I as I pushed merge... I'm investigating it now

[afk11]

thought there weren't any more v0.10 tests? or was that pbkdf2?

[dcousens]

@afk11 that was PBKDF2 ... which is probably why.

[rubensayshi]

yea PBKDF2 latest no longer works on 0.10, but I think you added the engines to the package.json too late maybe?

[dcousens]

No, it's failing on https://github.com/bitcoinjs/bip39/blob/master/test/index.js#L173-L175

[dcousens]

Completely unrelated to the (binary) issues in PBKDF2, or 0.10... or more importantly, this PR.

[dcousens]

Oh wait, right.
No it is related.
Its using SHA1... sigh, we should just hard throw rather than silently "work".

[dcousens]

browserify/pbkdf2#36 (comment)

I guess I'm ~OK with that, but, it just sounds like it could hurt someone.

Of course, our repos.
I suppose we drop 0.10 support too?

[rubensayshi]

@dcousens yes drop 0.10, it's happening all across the board in bitcoinjs related libs atm so might as well do it here too

[dcousens]

@rubensayshi done

[rubensayshi]

also, this PR actually might have been too hasty, we just realized that the vector I added is 1028 bytes, while > 1024 bytes of entropy the CS = ENT / 32 actually is larger than the max size of the checksum (256 bits)... and all bets are off because nothing is specced in BIP39 about that!

imo there are 2 options:

1. throw an error when entropy > 1024 bytes
2. Math.min(CS, 256)

I think 2 is sensible, but ...

[rubensayshi]

for now reversing the PR might be a good idea

[dcousens]

@rubensayshi I wasn't planning to release it for small while yet. I'm working through some examples locally.

[dcousens]

I think master being unstable has always been the policy, and certainly we should progress forward in resolving this. Do we really need to revert?

[rubensayshi]

I'll try to figure out what the right solution is and let you know

[jprichardson]

I suppose we drop 0.10 support too?

Yes 👍 It's unsupported now as of Oct 1st anyways.