fix(random)!: replace rejection sampling with fixed-count Lemire draw - #281
Open
coderdan wants to merge 4 commits into
Open
fix(random)!: replace rejection sampling with fixed-count Lemire draw#281coderdan wants to merge 4 commits into
coderdan wants to merge 4 commits into
Conversation
next_bounded_u32 drew bounded integers with a rejection loop whose iteration count depended on RNG output. With a secret-seeded SafeRand (PermutationKey key derivation), that made derivation time weakly dependent on secret material — the same timing channel removed from the ore.rs PRP. The power-of-two branch also broke the documented inclusive contract, silently biasing Fisher–Yates permutations, and the inclusive upper bound made password generation index one past the character table (reachable panic, p ≈ 1/95 per character). A single multiply-high (Lemire) reduction over one 64-bit draw fixes all three: exactly one draw per call (timing independent of values drawn), branch-free, and half-open [0, range) semantics so callers can no longer be off by one. The duplicate rejection implementation in bounded.rs is folded into the same helper. Call sites updated: Fisher–Yates passes i + 1; password generators now stay in-bounds by construction. Also fixes STANDARD_CHARS containing a duplicate 'e' and no 'd'. Tests: deterministic boundary draws, half-open contract across ranges, chi-squared uniformity for the draw itself, a Fisher–Yates position- matrix chi-squared test that catches the power-of-two bias, and a password-generation regression that panics with overwhelming probability under the old inclusive bound. BREAKING CHANGE: next_bounded_u32 and BoundedRng::next_bounded now return values in the half-open range [0, range) instead of the inclusive [0, max], and consume one u64 draw instead of one-or-more u32 draws. Deterministic outputs derived from a seeded SafeRand through bounded draws (e.g. PermutationKey::from_seed) differ from previous releases; the raw next_u32/next_u64/fill_bytes stream is unchanged. Closes #198 Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
Contributor
🧬 Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 19 | 0 | 4 | 0 |
✅ Every mutant in the changed lines was caught by a test.
Contributor
✅ No CRAP threshold violations395 function(s) analyzed · threshold 30 |
The mutants gate flagged the Protected<u32> impl as untested: replacing its body with a constant survived every test. Compare it draw-for-draw against the plain u32 impl from an identically-seeded RNG. Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
Code review of the Lemire change surfaced three loose ends: - The crate README (compiled into docs.rs via include_str!) still documented the old inclusive contract with `assert!(value <= 10)` — and because <= is weaker than <, the doctest kept passing. Document the half-open contract and tighten the assertions. - BoundedRng<usize> was left unimplemented!(): the most natural call for indexing (`rng.next_bounded(items.len())`) type-checked and then panicked at runtime. Implement it via a shared u64-width Lemire reduction (usize is at most 64 bits on all supported targets), with tests pinning it to the u32 impl and the half-open contract. - The migration note (inclusive -> half-open, new panic on zero range) lived only on SafeRand::next_bounded_u32; consumers migrating through the public BoundedRng trait never saw it. Document the contract, migration, and panic on the trait method itself. Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
The statistical-distance term (<= range / 2^64) was documented only on the pub(crate) helper. Protocol authors reading docs.rs need it to decide whether the reduction's deviation from uniform matters at their range, so state it on SafeRand::next_bounded_u32 and the BoundedRng trait method. Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #198.
What
Replaces the rejection-sampling bounded draw in
vitaminc-randomwith a single fixed-count Lemire multiply-high reduction, returning a half-open[0, range)value. One implementation now serves bothSafeRand::next_bounded_u32andBoundedRng::next_bounded(the duplicate inbounded.rsis folded in).Fixes all three problems from #198:
PermutationKeyderivation time was weakly dependent on secret seed material. Now exactly one 64-bit draw per call, branch-free.[0, max)while the contract (and rejection branch) was inclusive[0, max], so Fisher–Yates steps at power-of-two indices silently drew from the wrong range, biasing the permutation. Half-open semantics +i + 1at the call site fix this by construction.next_bounded_u32(94)return 94, indexing pastSTANDARD_CHARS(p ≈ 1/95 per character). Half-open draws stay in-bounds; the sub-alphabet skew for alphanumeric/alpha passwords is gone too.Also fixes
STANDARD_CHARScontaining a duplicate'e'and no'd'(spotted during review of the call sites).Breaking change
next_bounded_u32(range)/next_bounded(range)are now half-open and consume oneu64draw instead of one-or-moreu32draws.SafeRandthrough bounded draws (PermutationKey::from_seed,from_controlled_seed) produces different results than previous releases. The rawnext_u32/next_u64/fill_bytesstream is unchanged. This is unavoidable — preserving the old sequence means preserving the bias and the panic. Note Constant-time oblivious permutation generation: sort-by-random-key over a Batcher network #280 (stacked next) changes key derivation again; both should land before the next release so seed-derived keys break once.Tests
https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm