random: Improve RandomMixin::randbits

The previous randbits code would, when requesting more randomness than available
in its random bits buffer, discard the remaining entropy and generate new.

Benchmarks show that it's usually better to first consume the existing randomness
and only then generate new ones. This adds some complexity to randbits, but it
doesn't weigh up against the reduced need to generate more randomness.

Author: sipa

src/random.h

@@ -10,6 +10,7 @@
 #include <crypto/common.h>
 #include <span.h>
 #include <uint256.h>
+#include <util/check.h>
 
 #include <bit>
 #include <cassert>
@@ -165,7 +166,7 @@ template<typename T>
 class RandomMixin
 {
 private:
-    uint64_t bitbuf;
+    uint64_t bitbuf{0};
     int bitbuf_size{0};
 
     /** Access the underlying generator.
@@ -175,12 +176,6 @@ class RandomMixin
      */
     RandomNumberGenerator auto& Impl() noexcept { return static_cast<T&>(*this); }
 
-    void FillBitBuffer() noexcept
-    {
-        bitbuf = Impl().rand64();
-        bitbuf_size = 64;
-    }
-
 public:
     RandomMixin() noexcept = default;
 
@@ -190,31 +185,42 @@ class RandomMixin
 
     RandomMixin(RandomMixin&& other) noexcept : bitbuf(other.bitbuf), bitbuf_size(other.bitbuf_size)
     {
+        other.bitbuf = 0;
         other.bitbuf_size = 0;
     }
 
     RandomMixin& operator=(RandomMixin&& other) noexcept
     {
         bitbuf = other.bitbuf;
         bitbuf_size = other.bitbuf_size;
+        other.bitbuf = 0;
         other.bitbuf_size = 0;
         return *this;
     }
 
     /** Generate a random (bits)-bit integer. */
     uint64_t randbits(int bits) noexcept
     {
-        if (bits == 0) {
-            return 0;
-        } else if (bits > 32) {
-            return Impl().rand64() >> (64 - bits);
-        } else {
-            if (bitbuf_size < bits) FillBitBuffer();
-            uint64_t ret = bitbuf & (~uint64_t{0} >> (64 - bits));
+        Assume(bits <= 64);
+        // Requests for the full 64 bits are passed through.
+        if (bits == 64) return Impl().rand64();
+        uint64_t ret;
+        if (bits <= bitbuf_size) {
+            // If there is enough entropy left in bitbuf, return its bottom bits bits.
+            ret = bitbuf;
             bitbuf >>= bits;
             bitbuf_size -= bits;
-            return ret;
+        } else {
+            // If not, return all of bitbuf, supplemented with the (bits - bitbuf_size) bottom
+            // bits of a newly generated 64-bit number on top. The remainder of that generated
+            // number becomes the new bitbuf.
+            uint64_t gen = Impl().rand64();
+            ret = (gen << bitbuf_size) | bitbuf;
+            bitbuf = gen >> (bits - bitbuf_size);
+            bitbuf_size = 64 + bitbuf_size - bits;
         }
+        // Return the bottom bits bits of ret.
+        return ret & ((uint64_t{1} << bits) - 1);
     }
 
     /** Generate a random integer in the range [0..range).

src/test/random_tests.cpp

@@ -39,9 +39,9 @@ BOOST_AUTO_TEST_CASE(fastrandom_tests)
         BOOST_CHECK_EQUAL(7, ctx.rand_uniform_delay(time_point, 9s).time_since_epoch().count());
         BOOST_CHECK_EQUAL(-6, ctx.rand_uniform_delay(time_point, -9s).time_since_epoch().count());
         BOOST_CHECK_EQUAL(1, ctx.rand_uniform_delay(time_point, 0s).time_since_epoch().count());
-        BOOST_CHECK_EQUAL(1467825113502396065, ctx.rand_uniform_delay(time_point, 9223372036854775807s).time_since_epoch().count());
-        BOOST_CHECK_EQUAL(-970181367944767837, ctx.rand_uniform_delay(time_point, -9223372036854775807s).time_since_epoch().count());
-        BOOST_CHECK_EQUAL(24761, ctx.rand_uniform_delay(time_point, 9h).time_since_epoch().count());
+        BOOST_CHECK_EQUAL(4652286523065884857, ctx.rand_uniform_delay(time_point, 9223372036854775807s).time_since_epoch().count());
+        BOOST_CHECK_EQUAL(-8813961240025683129, ctx.rand_uniform_delay(time_point, -9223372036854775807s).time_since_epoch().count());
+        BOOST_CHECK_EQUAL(26443, ctx.rand_uniform_delay(time_point, 9h).time_since_epoch().count());
     }
     BOOST_CHECK_EQUAL(ctx1.rand32(), ctx2.rand32());
     BOOST_CHECK_EQUAL(ctx1.rand32(), ctx2.rand32());
@@ -103,6 +103,52 @@ BOOST_AUTO_TEST_CASE(fastrandom_randbits)
     }
 }
 
+/** Verify that RandomMixin::randbits returns 0 and 1 for every requested bit. */
+BOOST_AUTO_TEST_CASE(randbits_test)
+{
+    FastRandomContext ctx_lens; //!< RNG for producing the lengths requested from ctx_test.
+    FastRandomContext ctx_test; //!< The RNG being tested.
+    int ctx_test_bitsleft{0}; //!< (Assumed value of) ctx_test::bitbuf_len
+
+    // Run the entire test 5 times.
+    for (int i = 0; i < 5; ++i) {
+        // count (first) how often it has occurred, and (second) how often it was true:
+        // - for every bit position, in every requested bits count (0 + 1 + 2 + ... + 64 = 2080)
+        // - for every value of ctx_test_bitsleft (0..63 = 64)
+        std::vector<std::pair<uint64_t, uint64_t>> seen(2080 * 64);
+        while (true) {
+            // Loop 1000 times, just to not continuously check std::all_of.
+            for (int j = 0; j < 1000; ++j) {
+                // Decide on a number of bits to request (0 through 64, inclusive; don't use randbits/randrange).
+                int bits = ctx_lens.rand64() % 65;
+                // Generate that many bits.
+                uint64_t gen = ctx_test.randbits(bits);
+                // Make sure the result is in range.
+                if (bits < 64) BOOST_CHECK_EQUAL(gen >> bits, 0);
+                // Mark all the seen bits in the output.
+                for (int bit = 0; bit < bits; ++bit) {
+                    int idx = bit + (bits * (bits - 1)) / 2 + 2080 * ctx_test_bitsleft;
+                    seen[idx].first += 1;
+                    seen[idx].second += (gen >> bit) & 1;
+                }
+                // Update ctx_test_bitself.
+                if (bits > ctx_test_bitsleft) {
+                    ctx_test_bitsleft = ctx_test_bitsleft + 64 - bits;
+                } else {
+                    ctx_test_bitsleft -= bits;
+                }
+            }
+            // Loop until every bit position/combination is seen 242 times.
+            if (std::all_of(seen.begin(), seen.end(), [](const auto& x) { return x.first >= 242; })) break;
+        }
+        // Check that each bit appears within 7.78 standard deviations of 50%
+        // (each will fail with P < 1/(2080 * 64 * 10^9)).
+        for (const auto& val : seen) {
+             assert(fabs(val.first * 0.5 - val.second) < sqrt(val.first * 0.25) * 7.78);
+        }
+    }
+}
+
 /** Does-it-compile test for compatibility with standard library RNG interface. */
 BOOST_AUTO_TEST_CASE(stdrandom_test)
 {

test/sanitizer_suppressions/ubsan

@@ -76,3 +76,4 @@ shift-base:crypto/
 shift-base:streams.h
 shift-base:FormatHDKeypath
 shift-base:xoroshiro128plusplus.h
+shift-base:RandomMixin<*>::randbits