-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Better randomness test for secp256k1_rand_bits #367
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
Conversation
33e150c
to
a601917
Compare
Still too twitchy: I am able to observe failures. FWIW, generally checks out fine, and I confirmed it catches some obscure bugs I tried adding. |
Can we go forward with this but disable the testing RNG tests by default and make running them part of our release process? I don't like the possibility of spurious failures for users, even if we make it much lower. 98% of the value of this is catching boneheaded mistakes like changes that make the RNG return all zero or all even values. :P |
For reference / easier testing:
This is at fa33017. |
Here's a more recent one on 67a429f, posting this here because the other seed may not work any longer
|
Can I suggest that this be changed so that it runs the randomness tests with a static seed? This way it won't cause spurious failures for users, but will still likely catch bugs introduced by changes to the rng? |
You mean run the current tests with a static seed? Or simply add some known-output tests then to detect any change in the rng (and maybe run them first in the test suite)? |
Current test with a static seed. just for the RNG tests. People tend to copy-paste new responses when a non-normative function changes, so known responses really are only just "this changed" tests-- useful to that extent but don't replace tests that check invariants. If the RNG is changed then then it might trigger a spurious failure with the static seed but then the author can check it out and increment the seed if its appropriate to just suppress . What I want to avoid is the current situation where it passes 99.99% of the time but then fails randomly for some user, but otherwise not change anything. |
FWIW if my math is right, the current test succeeds with 99.984% chance. https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/issues/208#note_457729449 Fixed seed sounds like a nice idea. An alternative idea: Increase the rounds so that each call to A possible improvement: build an actual counting histogram (up to 27 * 6 * 64 bins), rather than a binary "appeared at least once"-ogram. The counts will follow a binomial distribution. Check if counts exceed min/max limits, and make those limits generous enough for the desired failure rate.
|
Closing this. Our RNG has been replaced with a well-analyzed existing one (xoshiro256++), which doesn't really need this kind of testing. Going to open another PR that replaces these tests with static test vectors from xorshiro256++. |
No description provided.