random: make GetRand() support entire range (incl. max)

The existing code uses GetRand(nMax), with a default value for nMax, where nMax is the
range of values (not the maximum!) that the output is allowed to take. This will always
miss the last possible value (e.g. GetRand<uint32_t>() will never return 0xffffffff).

Fix this, by moving the functionality largely in RandomMixin, and also adding a
separate RandomMixin::rand function, which returns a value in the entire (non-negative)
range of an integer.

Author: sipa

src/random.cpp

@@ -679,11 +679,6 @@ void RandAddPeriodic() noexcept
 
 void RandAddEvent(const uint32_t event_info) noexcept { GetRNGState().AddEvent(event_info); }
 
-uint64_t GetRandInternal(uint64_t nMax) noexcept
-{
-    return FastRandomContext().randrange(nMax);
-}
-
 uint256 GetRandHash() noexcept
 {
     uint256 hash;

src/random.h

@@ -80,31 +80,6 @@
  * Thread-safe.
  */
 void GetRandBytes(Span<unsigned char> bytes) noexcept;
-/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */
-uint64_t GetRandInternal(uint64_t nMax) noexcept;
-/** Generate a uniform random integer of type T in the range [0..nMax)
- *  nMax defaults to std::numeric_limits<T>::max()
- *  Precondition: nMax > 0, T is an integral type, no larger than uint64_t
- */
-template<typename T>
-T GetRand(T nMax=std::numeric_limits<T>::max()) noexcept {
-    static_assert(std::is_integral<T>(), "T must be integral");
-    static_assert(std::numeric_limits<T>::max() <= std::numeric_limits<uint64_t>::max(), "GetRand only supports up to uint64_t");
-    return T(GetRandInternal(nMax));
-}
-/** Generate a uniform random duration in the range [0..max). Precondition: max.count() > 0 */
-template <typename D>
-D GetRandomDuration(typename std::common_type<D>::type max) noexcept
-// Having the compiler infer the template argument from the function argument
-// is dangerous, because the desired return value generally has a different
-// type than the function argument. So std::common_type is used to force the
-// call site to specify the type of the return value.
-{
-    assert(max.count() > 0);
-    return D{GetRand(max.count())};
-};
-constexpr auto GetRandMicros = GetRandomDuration<std::chrono::microseconds>;
-constexpr auto GetRandMillis = GetRandomDuration<std::chrono::milliseconds>;
 
 /**
  * Return a timestamp in the future sampled from an exponential distribution
@@ -251,17 +226,17 @@ class RandomMixin
         }
     }
 
-    /** Generate a random integer in the range [0..range).
-     * Precondition: range > 0.
-     */
-    uint64_t randrange(uint64_t range) noexcept
+    /** Generate a random integer in the range [0..range), with range > 0. */
+    template<std::integral I>
+    I randrange(I range) noexcept
     {
-        assert(range);
-        --range;
-        int bits = std::bit_width(range);
+        static_assert(std::numeric_limits<I>::max() <= std::numeric_limits<uint64_t>::max());
+        Assume(range > 0);
+        uint64_t maxval = range - 1U;
+        int bits = std::bit_width(maxval);
         while (true) {
             uint64_t ret = Impl().randbits(bits);
-            if (ret <= range) return ret;
+            if (ret <= maxval) return ret;
         }
     }
 
@@ -284,6 +259,16 @@ class RandomMixin
         }
     }
 
+    /** Generate a random integer in its entire (non-negative) range. */
+    template<std::integral I>
+    I rand() noexcept
+    {
+        static_assert(std::numeric_limits<I>::max() <= std::numeric_limits<uint64_t>::max());
+        static constexpr auto BITS = std::bit_width(uint64_t(std::numeric_limits<I>::max()));
+        static_assert(std::numeric_limits<I>::max() == std::numeric_limits<uint64_t>::max() >> (64 - BITS));
+        return I(Impl().template randbits<BITS>());
+    }
+
     /** Generate random bytes. */
     template <BasicByte B = unsigned char>
     std::vector<B> randbytes(size_t len) noexcept
@@ -441,6 +426,33 @@ void Shuffle(I first, I last, R&& rng)
     }
 }
 
+/** Generate a uniform random integer of type T in the range [0..nMax)
+ *  Precondition: nMax > 0, T is an integral type, no larger than uint64_t
+ */
+template<typename T>
+T GetRand(T nMax) noexcept {
+    return T(FastRandomContext().randrange(nMax));
+}
+
+/** Generate a uniform random integer of type T in its entire non-negative range. */
+template<typename T>
+T GetRand() noexcept {
+    return T(FastRandomContext().rand<T>());
+}
+
+/** Generate a uniform random duration in the range [0..max). Precondition: max.count() > 0 */
+template <typename D>
+D GetRandomDuration(typename std::common_type<D>::type max) noexcept
+// Having the compiler infer the template argument from the function argument
+// is dangerous, because the desired return value generally has a different
+// type than the function argument. So std::common_type is used to force the
+// call site to specify the type of the return value.
+{
+    return D{GetRand(max.count())};
+};
+constexpr auto GetRandMicros = GetRandomDuration<std::chrono::microseconds>;
+constexpr auto GetRandMillis = GetRandomDuration<std::chrono::milliseconds>;
+
 /* Number of random bytes returned by GetOSRand.
  * When changing this constant make sure to change all call sites, and make
  * sure that the underlying OS APIs for all platforms support the number.

src/test/util/net.cpp

@@ -126,7 +126,7 @@ std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candida
             /*fRelevantServices=*/random_context.randbool(),
             /*m_relay_txs=*/random_context.randbool(),
             /*fBloomFilter=*/random_context.randbool(),
-            /*nKeyedNetGroup=*/random_context.randrange(100),
+            /*nKeyedNetGroup=*/random_context.randrange(100u),
             /*prefer_evict=*/random_context.randbool(),
             /*m_is_local=*/random_context.randbool(),
             /*m_network=*/ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())],