rot.js icon indicating copy to clipboard operation
rot.js copied to clipboard

ROT.RNG.setSeed unexpected behavior

Open nluqo opened this issue 5 years ago • 9 comments

I'm using setSeed for seeded runs in a game and was really surprised by the behavior. Two main issues:

  • The only valid input seems to be positive numbers. Passing a string would be really useful, but for both strings and negative numbers, the state is always the same. The first uniform is: 2.3283064365386963e-10
  • There is no avalanche effect. Similar seeds produce similar random numbers at least for the first one. 123 produces 0.0599007117561996 and 124 produces 0.060387709410861135. The numbers do diverge after that first uniform value, so it's probably easy enough to just throw away the first value?

Perhaps these are known behaviors, but from the developer perspective I found them really confusing... and it's quite easy to write some broken randomness by not knowing it. Even some notes on the docs would help out a lot.

nluqo avatar Jul 29 '20 17:07 nluqo

Hi @nluqo,

good finding! Thanks for the report.

* The only valid input seems to be positive numbers.

Right, the number type is also mentioned in the autogenerated documentation: https://ondras.github.io/rot.js/doc/classes/rng.rng.html#setseed

* Similar seeds produce similar random numbers at least for the first one.

This is a problem indeed. I am open to fixing it, but we are facing a backwards compatibility issue - folks might be expecting a particular behavior for their already-generated seeds.

* Passing a string would be really useful,

So, how about we add a seed-by-string feature that uses a better seeding mechanism and produces the desired avalanche effect? This would be a compatible change - and the documentation can mention that seeding with a string produces generally more robust results.

I am, however, not sure how to properly implement the seeding with a string. The current RNG implementation (Alea) is based on the work at https://github.com/nquinlan/better-random-numbers-for-javascript-mirror.

ondras avatar Jul 30 '20 06:07 ondras

I am, however, not sure how to properly implement the seeding with a string. The current RNG implementation (Alea) is based on the work at https://github.com/nquinlan/better-random-numbers-for-javascript-mirror.

I'd put (pre-salt + the string + post-salt) through SHA-256 and then pull the least significant bytes to turn into a seed value.

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest

The salt strings can be anything and don't need to be kept secret. Running dd if=/dev/urandom bs=1 count=24 | base64 gave me these:

PRE_SALT = "j2wAXcEUrFnZGpt9WsZE1RV4oNAOBcBO"
POST_SALT = "c5/0KcU6ad912gQ88c/53Ng+magf2x7P"

blinkdog avatar Jul 30 '20 07:07 blinkdog

I'd put (pre-salt + the string + post-salt) through SHA-256 and then pull the least significant bytes to turn into a seed value.

The RNG uses four state values, though I am not sure how exactly is the _c supposed to work - (current) seeding sets it always to 1. So you suggest using the last 8*3 bytes to set _s0, _s1 and _s2 ?

ondras avatar Jul 30 '20 07:07 ondras

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest

Also, I would prefer the seeding process to be synchronous :-)

ondras avatar Jul 30 '20 07:07 ondras

I'd put (pre-salt + the string + post-salt) through SHA-256 and then pull the least significant bytes to turn into a seed value.

The RNG uses four state values, though I am not sure how exactly is the _c supposed to work - (current) seeding sets it always to 1. So you suggest using the last 8*3 bytes to set _s0, _s1 and _s2 ?

Yeah, that'd be a good way to do it.

blinkdog avatar Jul 30 '20 07:07 blinkdog

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest Also, I would prefer the seeding process to be synchronous :-)

Hmmm, in Node.js I'd use: https://nodejs.org/api/crypto.html#crypto_crypto_createhash_algorithm_options

And the browser I'd borrow Browserify's browser implementation of Node's crypto: https://github.com/crypto-browserify/crypto-browserify https://github.com/crypto-browserify/createHash https://github.com/crypto-browserify/sha.js

These would give compatible synchronous strong hash functions.

blinkdog avatar Jul 30 '20 08:07 blinkdog

Sounds a bit like an overkill to me. We just need a way to convert a string to three 64bit values, without any particular crypto/security properties... how about the original Baagoe's Mash function? I would say this should have been used from the very beginning.

ondras avatar Jul 30 '20 08:07 ondras

Sounds a bit like an overkill to me. We just need a way to convert a string to three 64bit values, without any particular crypto/security properties... how about the original Baagoe's Mash function? I would say this should have been used from the very beginning.

Yeah, that's pretty cool. I'll bookmark this one.

blinkdog avatar Jul 30 '20 09:07 blinkdog

Right, the number type is also mentioned in the autogenerated documentation:

Ah yes, I forgot this even existed. Woops. Though I still think it should mention the sign of the number. I was using a library to generate SHA256 digests and getting an int out of it and it only returned signed integers... and thought I was good for a while.

It's easy to test a couple times (positive input, negative input, positive input) and think "these all look different enough" but not realize all your negative inputs will degenerate to the same state.

So, how about we add a seed-by-string feature

Sounds good to me. It will change from the current behavior if you were misusing the API and passing a string, but I assume this is unlikely.

nluqo avatar Jul 30 '20 12:07 nluqo