fix: MonotonicEntropy increment can never be 1, and is deterministic for inc==2 - #135
Open
dualfroz wants to merge 1 commit into
Open
fix: MonotonicEntropy increment can never be 1, and is deterministic for inc==2#135dualfroz wants to merge 1 commit into
dualfroz wants to merge 1 commit into
Conversation
dualfroz
force-pushed
the
dualfroz/fix-monotonic-increment-min-one
branch
from
September 5, 2026 22:42
a7f0dce to
55d0aee
Compare
MonotonicEntropy.random rejected a validly drawn value of 0 in its rejection-sampling loop, making an increment of exactly 1 unreachable for entropy sources without a fast Int63n path (e.g. crypto/rand.Reader). For inc == 2 this made the increment fully deterministic (always 2), contradicting the documented "random number between 1 and inc inclusive" contract. Restructure the loop to match the crypto/rand.Int pattern it is adapted from, so 0 is a valid draw and the result is correctly uniform over [1, inc].
dualfroz
force-pushed
the
dualfroz/fix-monotonic-increment-min-one
branch
from
September 5, 2026 23:25
55d0aee to
d52271c
Compare
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
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.
Problem
ulid.Monotonic(entropy, inc)documents that calls toMonotonicReadwithinthe same ULID timestamp return entropy "incremented by a random number
between 1 and
incinclusive" (ulid.go, doc comment onMonotonic).That contract does not hold for the
io.Reader-backed slow path used by anyentropy source that does not implement the internal
Int63n(int64) int64fast-path interface -- which includes
crypto/rand.Reader, the entropysource recommended by the package's own README for cases where
math/rand-based entropy is not appropriate.Concretely:
inc == 2, the increment is fully deterministic: it is always 2,never 1. This defeats the purpose of randomizing the increment for that
configuration.
Empirical reproduction (before the fix)
Using
ulid.Monotonic(crand.Reader, 2)and generating 200 ULIDs in the samemillisecond, the entropy delta between consecutive ULIDs was recorded:
All 200 trials produced a delta of exactly 2. A delta of 1 never occurred,
contradicting the documented "1 and
incinclusive" range.Root cause
ulid.go,(*MonotonicEntropy).random(), the rejection-sampling loop usedby the
io.Readerslow path (around line 670 before the fix):The loop is intended to draw a uniform value in
[0, m.inc)(by rejectingdraws
>= m.inc, the same rejection-sampling technique used bycrypto/rand.Int, which this code is explicitly adapted from) and thenreturn
1 + incso the final result lands in[1, m.inc].The
inc == 0 ||clause incorrectly rejects a legitimately drawn value of 0and forces a redraw. For
m.inc == 2, the only accepted raw draw ends upbeing 1 (0 is rejected, 2 and 3 are rejected as out of range), so the
function always returns
1 + 1 == 2. More generally, this excludes thesmallest value in the target range from ever being drawn, making a final
increment of 1 unreachable for any
m.inc.By contrast, the fast path for entropy sources that implement
Int63n(e.g.
math/rand.Rand) does not have this bug:1 + m.rng.Int63n(int64(m.inc))correctly draws from[0, m.inc)(0 inclusive)and shifts to
[1, m.inc]. The io.Reader path was a discrepantre-implementation of the same idea.
Fix
ulid.go,(*MonotonicEntropy).random():Restructured the rejection-sampling loop to match the actual pattern used by
crypto/rand.Int(the function this code says it's adapted from): afor { ... }loop that always executes its body at least once and breaksonly once a value strictly less than
m.inchas been drawn, rather thangating the loop on a pre-loop condition. (A minimal one-line removal of the
inc == 0 ||clause was tried first but is wrong: sinceincis thefunction's named return value, it is Go-zero-initialized to 0, so a bare
for inc >= m.incloop condition would never enter its body -- 0 is never>= m.incfor anym.inc >= 2-- andrandom()would return 1unconditionally without reading any entropy at all. The regression test
below caught this immediately.)
This draws a uniform value in
[0, m.inc)(0 now included) and returns1 + incin[1, m.inc], matching both the documented contract and theexisting fast-path behavior. Two stale doc comments ("Range: [1, m.inc)")
were also corrected to "[1, m.inc]" to match the actual (and now correct)
inclusive range.
This does not weaken the monotonicity guarantee:
random()still alwaysreturns a value
>= 1(never 0), soMonotonicEntropy.increment()stillstrictly increases the entropy on every call within the same millisecond,
and overflow handling (
uint80.Add/ErrMonotonicOverflow) is untouched.The rejection-sampling technique itself (draw
byteLenbytes, mask offexcess bits, reject and redraw if the value is out of range) is unchanged
and remains unbiased and loop-terminating; the fix only removes an
over-restrictive extra rejection, so the expected number of iterations per
call is now less than or equal to what it was before.