Skip to content

fix: MonotonicEntropy increment can never be 1, and is deterministic for inc==2 - #135

Open
dualfroz wants to merge 1 commit into
oklog:mainfrom
dualfroz:dualfroz/fix-monotonic-increment-min-one
Open

fix: MonotonicEntropy increment can never be 1, and is deterministic for inc==2#135
dualfroz wants to merge 1 commit into
oklog:mainfrom
dualfroz:dualfroz/fix-monotonic-increment-min-one

Conversation

@dualfroz

@dualfroz dualfroz commented Sep 5, 2026

Copy link
Copy Markdown

Problem

ulid.Monotonic(entropy, inc) documents that calls to MonotonicRead within
the same ULID timestamp return entropy "incremented by a random number
between 1 and inc inclusive" (ulid.go, doc comment on Monotonic).

That contract does not hold for the io.Reader-backed slow path used by any
entropy source that does not implement the internal Int63n(int64) int64
fast-path interface -- which includes crypto/rand.Reader, the entropy
source recommended by the package's own README for cases where
math/rand-based entropy is not appropriate.

Concretely:

  • An increment of exactly 1 is impossible.
  • When 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 same
millisecond, the entropy delta between consecutive ULIDs was recorded:

trials=200 delta histogram=map[2:200]

All 200 trials produced a delta of exactly 2. A delta of 1 never occurred,
contradicting the documented "1 and inc inclusive" range.

Root cause

ulid.go, (*MonotonicEntropy).random(), the rejection-sampling loop used
by the io.Reader slow path (around line 670 before the fix):

for inc == 0 || inc >= m.inc {
    ...
    inc = <value derived from freshly read random bytes, masked to [0, 2^bitLen)>
}
return 1 + inc, nil

The loop is intended to draw a uniform value in [0, m.inc) (by rejecting
draws >= m.inc, the same rejection-sampling technique used by
crypto/rand.Int, which this code is explicitly adapted from) and then
return 1 + inc so the final result lands in [1, m.inc].

The inc == 0 || clause incorrectly rejects a legitimately drawn value of 0
and forces a redraw. For m.inc == 2, the only accepted raw draw ends up
being 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 the
smallest 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 discrepant
re-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): a
for { ... } loop that always executes its body at least once and breaks
only once a value strictly less than m.inc has been drawn, rather than
gating the loop on a pre-loop condition. (A minimal one-line removal of the
inc == 0 || clause was tried first but is wrong: since inc is the
function's named return value, it is Go-zero-initialized to 0, so a bare
for inc >= m.inc loop condition would never enter its body -- 0 is never
>= m.inc for any m.inc >= 2 -- and random() would return 1
unconditionally without reading any entropy at all. The regression test
below caught this immediately.)

for {
    if _, err = io.ReadFull(m.Reader, m.rand[:byteLen]); err != nil {
        return 0, err
    }

    m.rand[0] &= uint8(int(1<<msbitLen) - 1)

    switch byteLen {
    case 1:
        inc = uint64(m.rand[0])
    case 2:
        inc = uint64(binary.LittleEndian.Uint16(m.rand[:2]))
    case 3, 4:
        inc = uint64(binary.LittleEndian.Uint32(m.rand[:4]))
    case 5, 6, 7, 8:
        inc = uint64(binary.LittleEndian.Uint64(m.rand[:8]))
    }

    if inc < m.inc {
        break
    }
}

return 1 + inc, nil

This draws a uniform value in [0, m.inc) (0 now included) and returns
1 + inc in [1, m.inc], matching both the documented contract and the
existing 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 always
returns a value >= 1 (never 0), so MonotonicEntropy.increment() still
strictly increases the entropy on every call within the same millisecond,
and overflow handling (uint80.Add / ErrMonotonicOverflow) is untouched.
The rejection-sampling technique itself (draw byteLen bytes, mask off
excess 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.

@dualfroz
dualfroz force-pushed the dualfroz/fix-monotonic-increment-min-one branch from a7f0dce to 55d0aee Compare September 5, 2026 22:42
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
dualfroz force-pushed the dualfroz/fix-monotonic-increment-min-one branch from 55d0aee to d52271c Compare September 5, 2026 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant