search

bitpay / bitcore-mnemonic

BIP39 Mnemonics implemented for Bitcore
http://bitcore.io
MIT License
155 stars 212 forks source link

Fixed bug in checksum when entropy is more than 256 bits #3

Closed rubensayshi closed 9 years ago

rubensayshi commented 9 years ago

the hash is sliced and zero padded in such a way that it will only work for entropy up to 256 bits (a checksum of max 8 bits).

PR in 2 parts; first a failing test and then a fix

coveralls commented 9 years ago

Coverage Status

Coverage remained the same when pulling 8c10f132d1899e2673c46aea898f2a59e9476306 on blocktrail:entropy-checksum-pr into 6c2eb30496ae1d3801edb2cc997ea42f1d746b04 on bitpay:master.

maraoz commented 9 years ago

Thanks for your contribution!! :) can you rebase?

rubensayshi commented 9 years ago

sure, is there any way of requiring this module this module in my package.json like "bitcore-mnemonic": "git://github.com/blocktrail/bitcore-mnemonic.git#8c10f132d1899e2673c46aea898f2a59e9476306" ?

seems you do some building of some stuff before you push it to npm ?

I also don't know why the tests fail, but your own commits seem to have the same effect :/

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.04%) when pulling 8f9f07eec478f4650cf57fb3638f65a6a2734cf2 on blocktrail:entropy-checksum-pr into 7d713ad170f4fef4444a35ac987d5c2d3f41affb on bitpay:master.

yemel commented 9 years ago

Awesome, thanks for the contribution! Tests have been fixed on master, could you please rebase one more time :)

yemel commented 9 years ago

Also, the constructor has a validation for entropy not being more than 256:

if (ent % 32 !== 0 || ent < 128 || ent > 256) {
    throw new errors.InvalidArgument('ENT', 'Values must be 128 < ENT < 256 and ENT % 32 == 0');
  }

I think removing the upper bound should be fine. Could you please update that too?

rubensayshi commented 9 years ago

yea, I ran into the bug trying to use an mnemonic generated by another library

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.04%) when pulling 508697d3ab06d90ccfc243f75163eed94020f8b4 on blocktrail:entropy-checksum-pr into 421b2343b9169396f19e5bf164399f2fc2636c83 on bitpay:master.

rubensayshi commented 9 years ago

still fails or something that seems unrelated

yemel commented 9 years ago

The test was failing because a test exceeded mocha default timeout. I extended it and merge it on this other pull request: https://github.com/bitpay/bitcore-mnemonic/pull/4

Thanks again for the contribution. If you are using bitcore for a project let us know! We may include it on http://bitcore.io

rubensayshi commented 9 years ago

great!

We've been using bitcore for a while now for www.blocktrail.com for the part of our system that is connected to the P2P network and stores block and TX data in our database (inspired by insight).

Unfortunately it is (and will remain) closed source, but if you're listing closed source projects too then it would be cool to be listed :)

maraoz commented 9 years ago

That's great to know! I think we should definitely list the project on bitcore.io.