-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sim: Use a CSPRNG #10806
Sim: Use a CSPRNG #10806
Conversation
d82a9cc
to
5a070aa
Compare
b9a93e8
to
d5a34be
Compare
Doesn't this break RNG accuracy in old gens? |
I am currently unable to
Here's sample output on main:
In addition, although I can import an old inputlog, it does not give the same RNG results (on master, this Inferno will miss; on this branch it hits). So we should either have it use the old RNG when using an older seed, or prevent old inputlogs from being imported, because a good chunk of the time you will get a crash or the battle will otherwise be very screwed up from trying to perform actions only possible because of previous RNG. |
This is a pretty major refactor which is mostly unrelated to the feature, but it does make the code a lot simpler.
@@ -32,8 +32,7 @@ export const Conditions: import('../../../sim/dex-conditions').ModdedConditionDa | |||
// However, just in case, use 1 if it is undefined. | |||
const counter = this.effectState.counter || 1; | |||
if (counter >= 256) { | |||
// 2^32 - special-cased because Battle.random(n) can't handle n > 2^16 - 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holy shit, I traced this back to
9bbb0b8#diff-1876d2faa56ede4eb79712f3c52e03d209fbd5e4b99ad47f5d1b31d3197fbfafR8460
Back then we were using the Gen 3 RNG, which only generated 16-bit numbers. Very different times.
@@ -1248,7 +1248,7 @@ export const Moves: import('../../../sim/dex-moves').ModdedMoveDataTable = { | |||
target.clearBoosts(); | |||
this.add('-clearboost', target); | |||
target.addVolatile('protect'); | |||
const set = Math.floor(Math.random() * 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, this is a real bug and pretty bad.
data/mods/gen5/conditions.ts
Outdated
@@ -32,8 +32,7 @@ export const Conditions: import('../../../sim/dex-conditions').ModdedConditionDa | |||
// However, just in case, use 1 if it is undefined. | |||
const counter = this.effectState.counter || 1; | |||
if (counter >= 256) { | |||
// 2^32 - special-cased because Battle.random(n) can't handle n > 2^16 - 1 | |||
return (this.random() * 4294967296 < 1); | |||
return this.random(0x100000000) === 0; // 2^32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait why is this 2^32 when counter >= 256
? Before that point, the chance is 1 in counter
and counter
doubles on each success, so the chance of each successive success would be 1/1, 1/2, 1/4, 1/8, 1/16, 1/32, 1/64, 1/128, 1/4294967296…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously it was already like this before this PR I’m just noticing this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is how it actually works in gen 5? Need a mechanics expert, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://bulbapedia.bulbagarden.net/wiki/Protection#Generation_V
Yep, I was right.
(Also removes the Buffer dependency from PRNG, and slightly improves comments.)
Yes, it's a new dependency. No, I do not think we should roll our own crypto, nor should we maintain our own crypto.