hash/hashSync functions don't take the params object correctly

[jrnewell]

The N parameter of the params/paramsSync output object is a power of base 2. However the hash functions except a raw value for N unlike the kdf functions which do the power transformation internally. This PR changes the hash functions to execute the power transformation internally too, so it can the params object directly.

[barrysteyn]

This is by design, and is in fact how the raw "hash" function is implemented in the original Scrypt implementation. I think that Colin Percival's thinking is that one can put whatever input the hash function they like (even if it is incorrect).

Why don't you speak to him and ask him about this question.

[barrysteyn]

Sorry, I may have been too quick to examine this issue. I will look into this evening or tomorrow when I have some more time.

[jrnewell]

Either way is fine with me... it is just a little weird IMO that it very similar to the kdf functions but slightly different with the sole difference is now you can type in invalid parameters. I think I just got thrown off by the hash section in the README where it says:

paramsObject - [REQUIRED] - parameters to control scrypt hashing (see params above).

It kind of implies that you should use the output of the params function. Took me a while to figure out why it would sometimes work (I would get back 16 sometimes) and sometimes crap out. I probably should have paid more attention to the example sections later on 😄

That being said, I can see the argument against changing it since there are probably a few projects that depend on this behavior and it is consistent with the original library.

Anyway, thanks for responding!

[BrandonZacharie]

I also ran in to this recently. I vote for this PR but updating the documentation to make it clear that there are two versions of paramsObject works as well.

[barrysteyn]

Okay, let me look into this. give me a day or so please...

[barrysteyn]

I will update package.json soon

[barrysteyn]

Okay, I need to update the documentation before I update the package.json. This is a breaking change, so version 7, here we come!

Any objections?

[chrisveness]

LGTM (though the current documentation documents the fixed behaviour, so you just need to document it as a (potentially breaking) bug fix, I guess).

What needs to change in package.json? (other than version bump).

[jordanbtucker]

Any update on when this is going to hit NPM? I just encountered this issue too, and it took way too long to figure it out because the documentation still hasn't been updated to say that the params from scrypt.params are not compatible with scrypt.hash.

[nathanph]

Also just ran into this issue. What's the holdup on this?

You should at least update the readme if you're not going to publish this to GitHub.

[barrysteyn]

I am trying to find some time to update this module in a big way... I am thinking I will get that time near the end of March.

[chrisveness]

@barrysteyn do you have any objection to merging my PR #141? It will at least warn people if they are using N rather than logN.

Whether or not you wish to update the documentation at all, I think this check would be valuable.

As it only implements a RangeError check, I don't think it could be considered a breaking change.

Edit: you could do that in anticipation of any major changes, and get it out quickly!

[barrysteyn]

@chrisveness I do not. But look at #150 - I want to correct it over there.

I am not anywhere near my home PC since I am traveling for business. All my keys are there, so I will need to get home before I am able to merge and publish.