[util] Remove expand_seed routine. #24571
Conversation
| log.error("ERROR: Seed shorter than 256 bits.") | ||
| sys.exit(1) | ||
|
|
||
| if seed.bit_length() < 250: |
There was a problem hiding this comment.
If this is to fail %1 of the time whenever we change seeds, how do you feel about reporting an error instead? I think the Makefile output is too verbose, so there is a good chance this warning will be missed.
There was a problem hiding this comment.
There's two issues:
- Eliminating seeds below a certain lower bound would reduce the input distribution, so the actual seed would not be able to be fully randomly sampled. The loss in security bound is only marginal, but it's still not ideal; we want to sample from the full distribution of 0..2^256-1.
- The DV setup always passes a 32-bit seed. It uses the command-line override, though, so we could error only if the value is from HJSON.
The fix for this that I prefer long-term is to:
- make the values in the HJSON files byte-strings, so their length is easy to check and can include leading 0s, and error if the length is not exactly 32 bytes
- add a command-line argument for "allow small seeds" so the DV setup can override the error
However, when I was trying to make bigger fixes to the PRNG and surrounding logic the change quickly became pretty big, so here I wanted to start with as narrow a hotfix as possible to remove the biggest issue.
There was a problem hiding this comment.
Hi @jadephilipoom, this explanation makes sense and I agree with your approach in this PR. Thanks for the additional details.
jadephilipoom
force-pushed
the
expand-seed-fix
branch
from
34c7d3a
to
2ff4983
Compare
This routine wasn't currently affecting the logic, but would have weakened certain seeds that happened to have low values. Ideally this should all be refactored to use byte strings so it's clearer what range the value is sampled from, but for now just removing the harmful logic should suffice. Signed-off-by: Jade Philipoom <[email protected]>
jadephilipoom
force-pushed
the
expand-seed-fix
branch
from
2ff4983
to
fb43637
Compare
|
LGTM |
|
I confirmed that the single test that is failing is also failing on |
timothytrippel
added
the
CherryPick:earlgrey_1.0.0
|
Successfully created backport PR for |
This routine wasn't currently affecting the logic, but would have weakened certain seeds that happened to have low values. Ideally this should all be refactored to use byte strings so it's clearer what range the value is sampled from, but for now just removing the harmful logic should suffice.