Lets create a new major version of Scrypt

[barrysteyn]

Hi everyone

I would like to create a new major version of Scrypt, Scrypt version 7. So this could (and actually will) be breaking changes.

Can I get some discussion/feedback about what should be in here.

[BrandonZacharie]

The hash/hashSync functions patch is at the top of my wishlist.

[barrysteyn]

Excellent @BrandonZacharie

Everyone, please keep the suggestions coming.

[chrisveness]

Having a quick look at scrypt-async-js, I like @dchest’s idea of accepting either N or logN. You could do something like if (params.N > 256) params.N = Math.log(params.N)*Math.LOG2E;. I don’t think there would be any ambiguity, as I can’t imagine anyone would actually want to use logN <= 8!

[dchest]

@chrisveness I wouldn't do this, I think just accepting N is good enough and standardized between implementations, and my logN thing was a mistake, which I fixed in "modern API" (still allowing to specify logN only for backwards compatibility) . Also, N can theoretically be any number, not a power of two, it's just all implementations of scrypt accept only a power of two for simplicity. Those who want to use logN can just pass 1<<logN (up to logN=30) or Math.pow(2, logN). Your proposal is also error-prone: while not recommended, N <= 8 is a perfectly valid input, and it would be very strange if function interpreted different range of numbers differently.

[chrisveness]

Colin Percival says “The parameter N must be a power of 2 greater than 1” (www.tarsnap.com/scrypt.html), so allowing any value of N would not seem a good idea for standardisation.

I like to be able to work with logN as I can immediately recognise the difference in workload between 14 and 20, whereas I have to get my calculator out to recognise workloads of 16384 or 1048576 – though I agree, I could fairly easily use 2**14...

Edit: though accepting either N or logN for scrypt.kdf() paramsObject would make it a non-breaking change...

[dchest]

Colin Percival says “The parameter N must be a power of 2 greater than 1” (www.tarsnap.com/scrypt.html), so allowing any value of N would not seem a good idea for standardisation.

True, that's because none of the implementations support not-power-of-two N for simplicity, but the function itself is defined in the paper without such restrictions. Here's a footnote from page 6:

"We expect that for reasons of performance and simplicity, implementors will restrict N to being a power of 2, in which case the function Integerify can be replaced by reading the first (or last) machine-length word from a k-bit block."

I like to be able to work with logN as I can immediately recognise the difference in workload between 14 and 20, whereas I have to get my calculator out to recognise workloads of 16384 or 1048576 – though I agree, I could fairly easily use 2**14...

I'm also used to logN, but even nicer would be to write 16 << 10 for 16384, 1024 << 10 for 1048576, etc. This way with r=8 you can assume the first number is the number of mebibytes of memory to use. Very convenient :)

[chrisveness]

Is that implementors including himself? :)

[dchest]

Yes, the reference implementation allows only powers of two.

[barrysteyn]

Ensuring that N is a power of 2 should not force us to use a logarithm. If we want to stick to the way Colin has done things in his source code, we could make a check for a power 2 and fail. Or if that approach would be too jarring, we could round up (or down!! - probably up) to the nearest power of two and print a warning to the console. What do you guys think?

[dchest]

I think throwing an error is the right thing to do. That's how most of implementations behave.

In my opinion, rounding up or down isn't the correct way to do this, since in theory any number can be used, we just don't support it. Let's not introduce surprises.

[barrysteyn]

@dchest - okay, I have no problem with throwing an error.

[marksteward]

For consistency with other libraries, I think it would be better to use N for exposed interfaces (including params) instead of logN, and do a power of 2 check in the wrappers. Creating confusion over N and logN seems like a bad thing for a security-sensitive library.

Either way, it would be good to make the internal naming consistent. Currently you have¹:

   obj->Set(Nan::New("N").ToLocalChecked(), Nan::New<Integer>(logN));

the less obvious^{2, 3}:

  const unsigned int result = Hash(key_ptr, key_size, salt_ptr, salt_size, params.N, params.r, params.p, hash_ptr, hash_size);
  ...
  Hash(const uint8_t* key, size_t keylen, const uint8_t *salt, size_t saltlen, uint64_t logN, uint32_t r, uint32_t p, uint8_t *buf, size_t buflen) {

and the README examples haven't been updated.

Happy to help out if you need a hand with this.