Allow r: 1, p: 8 in scrypt

[homura]

https://github.com/paulmillr/noble-hashes/blob/ba9d92fc3a7e60b481b66c664719db54abeea5ba/src/scrypt.ts#L110-L118

{
    "kdfparams": {
      "dklen": 32,
      "n": 262144,
      "p": 8,
      "r": 1,
      "salt": "ab0c7876052600dd703518d6fc3fe8984592145b591fc8fb5c6d43190334ba19"
    }
}

Test vector from the Ethereum developer community

the cost 262144 here is over 65535(2 ** (blockSize / 8)

[paulmillr]

It's compatible with scrypt spec.

Using p: 8, r: 1 is wrong. No one should be doing that. This is an error in web3 test vectors that was made a while ago.

Instead, p: 1, r: 8 should be done.

[homura]

Got it, you're right. Thanks for your quickly reply

https://datatracker.ietf.org/doc/html/rfc7914#section-2

The parameter r ("blockSize") specifies the block size. The CPU/Memory cost parameter N ("costParameter") must be larger than 1, a power of 2, and less than 2^(128 * r / 8).

[sreyemnayr]

There have been errata filed for RFC7914 that correct this "incorrect conversion between bits/bytes."

https://www.rfc-editor.org/errata_search.php?rfc=7914

The presented limit on N was incorrectly derived from the original scrypt publication. The correct theoretical upper limit on N is 2^(128 * r) for r < 5, and 2^512 for all other values of r. Thus, the least upper bound is 2^128, which far exceeds all possible values for N in the foreseeable future, making the limit irrelevant for current implementations.

See the conversation here for more context.

I got here via dependency rabbit hole of the Rabby wallet, which depends on @ethereumjs/wallet, which depends on ethereum-cryptography, which depends on @noble/hashes.

Ironically, the issue is intra-organizational Paul vs Paul... the default settings for ethereum/eth-account's keystore write by @pacrob are incompatible with the keystore read ethereum/ethereum-js-cryptography by @paulmillr

[sreyemnayr]

It actually looks like the defaults were fixed in eth-keyfile 0.7, but eth-account pins to >= 0.6, so they still generate files that noble-hashes won't read. That all said, the check is based on bad math, so should probably be a warning at most, rather than raising an error.

[paulmillr]

That’s unfortunate, but, is relaxing the check really the best outcome?

I’m not scrypt expert and will need to check if that doesn’t impact security etc.

sure, there are some folks who’ve generated keys using wrong algorithms.

Probably there are other ones, with bugs in algorithms, which make their implementations incompatible with everyone else. Is asking to be compatible with those a good thing? I think answer can be positive - but only when the library is popular. Is eth account popular? Has it ever been?

[sreyemnayr]

It’s used by pretty much every Python web3 library, so I’d say it’s pretty popular. Also, it doesn’t affect security afaik, the limit was essentially a typo in the spec.

On Wed, Aug 28, 2024 at 6:54 PM Paul Miller @.***> wrote:

That’s unfortunate, but, is relaxing the check really the best outcome?

I’m not scrypt expert and will need to check if that doesn’t impact security etc.

sure, there are some folks who’ve generated keys using wrong algorithms.

Probably there are other ones, with bugs in algorithms, which make their implementations incompatible with everyone else. Is asking to be compatible with those a good thing? I think answer can be positive - but only when the library is popular. Is eth account popular? Has it ever been?

— Reply to this email directly, view it on GitHub https://github.com/paulmillr/noble-hashes/issues/61#issuecomment-2316434555, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBJQTT2BQX6L6HSGZ2LWZTZTZPLTAVCNFSM6AAAAABNIP3B52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJWGQZTINJVGU . You are receiving this because you commented.Message ID: @.***>

[sreyemnayr]

For a little less "source: trust me, bro", the pypi stats for the eth-account package can be found here.

Over 1m downloads a week is pretty significant. They've just updated their latest release to require a newer version of eth-keyfile (which is actually the culprit of defaults being incompatible with your library) so it will eventually wash itself out over the long term, but the fact still remains that the check is based on incorrectly reported math initially in the RFC. I think updating to change it to a non-lethal warning is an only-positive move.

[paulmillr]

Confirmation by scrypt creator that it's ok: https://github.com/golang/go/issues/33703#issuecomment-568198927

RFC errata that it's ok: https://www.rfc-editor.org/errata_search.php?rfc=7914