Sanity checks for entropy size

[rubensayshi]

In #33 I tried coming up with a solution to deal witih odd sizes of entropy and the resulting checksum hell.

Instead the only sane solution seems to be to do better sanity checks in the entropy;

1. It should always be a multitude of 4 bytes, that way it will always result in a multitude of 11 bits (including checksum) and no weird padding things will happen with the checksum.
2. It should not allow entropy above 1024 bytes, because at that point we've reached the max length of the checksum (sha256) and if we continue (without padding the checksum) the above rule won't hold

The only alternative for the max 1024 bytes would be to pad the checksum to keep the first rule in tact, but since the BIP39 spec doesn't mention anything like that it doesn't seem like a good choice.

[rubensayshi]

@dcousens I think the other PR should be reverted, are you gonna commit revert it or rollback history?

I can rebase after

[afk11]

entropy.length % 4 === 0 probably introduces the biggest side effects. I had a situation where the data would vary in length, so I used padding to keep BIP39 happy. This should stop it from producing mnemonics it can't decode.

[dcousens]

I'm happy for you to revert it in this PR

[dcousens]

Maybe we should ask @voisine or @prusnak

[dcousens]

We shouldn't forget about this.
@afk11 lets try and close this soon too 👍

[afk11]

@dcousens definitely.

We should ping @voisine and @prusnak about the following constraints which apply to BIP39.

• entropyToMnemonic:
  • byteLength > 0 && byteLength <= 1024
  • byteLength % 4 == 0
• mnemonicToEntropy
  • derived byteLength <= 1024

I'll open a PR on bitcoin/bips to prompt discussion, since other implementations seem to have made different approaches.

NBitcoin is restrictive, but avoids it by rejecting entropy where the length isn't one of these: 128, 160, 192, 224, 256.
https://github.com/MetacoSA/NBitcoin/blob/733092083ce5b256337be3b6ea8791ddf500b8da/NBitcoin/BIP39/Mnemonic.cs#L91
https://github.com/MetacoSA/NBitcoin/blob/733092083ce5b256337be3b6ea8791ddf500b8da/NBitcoin/BIP39/Mnemonic.cs#L59

Bitcore-Mnemonic produces mnemonics it can't decode:
https://github.com/bitpay/bitcore-mnemonic/blob/master/lib/mnemonic.js#L247

BitcoinPHP (mine) had the same issue, and for variable length entropy, I resorted to padding to avoid problems. Ruben patched it already :)

[dcousens]

Was the conclusion:

please open issues/pull requests to implementations asking them to set limit between 128-256 bits

?
Do I close this and open a limiting PR?

Ref bitcoin/bips@5b1c59d