Merge bitcoin/bitcoin#30396: random: add benchmarks and drop unnecessary Shuffle function

6ecda04 random: drop ad-hoc Shuffle in favor of std::shuffle (Pieter Wuille)
da28a26 bench random: benchmark more functions, and add InsecureRandomContext (Pieter Wuille)
0a9bbc6 random bench refactor: move to new bench/random.cpp (Pieter Wuille)

Pull request description:

  This adds benchmarks for various operations on `FastRandomContext` and `InsecureRandomContext`, and then removes the ad-hoc `Shuffle` functions, now that it appears that standard library `std::shuffle` has comparable performance. The other reason for keeping `Shuffle`, namely the fact that libstdc++ used self-move (which debug mode panics on) has been fixed as well (see bitcoin/bitcoin#29625 (comment)).

ACKs for top commit:
  achow101:
    ACK 6ecda04
  hodlinator:
    ACK 6ecda04
  dergoegge:
    Code review ACK 6ecda04

Tree-SHA512: 2560b7312410581ff2b9bd0716e0f1558d910b5eadb9544785c972384985ac0f11f72d6b2797cfe2e7eb71fa57c30cffd98cc009cb4ee87a18b1524694211417

Author: achow101

src/Makefile.bench.include

@@ -49,6 +49,7 @@ bench_bench_bitcoin_SOURCES = \
   bench/poly1305.cpp \
   bench/pool.cpp \
   bench/prevector.cpp \
+  bench/random.cpp \
   bench/readblock.cpp \
   bench/rollingbloom.cpp \
   bench/rpc_blockchain.cpp \

src/bench/crypto_hash.cpp

@@ -196,22 +196,6 @@ static void SipHash_32b(benchmark::Bench& bench)
     });
 }
 
-static void FastRandom_32bit(benchmark::Bench& bench)
-{
-    FastRandomContext rng(true);
-    bench.run([&] {
-        rng.rand32();
-    });
-}
-
-static void FastRandom_1bit(benchmark::Bench& bench)
-{
-    FastRandomContext rng(true);
-    bench.run([&] {
-        rng.randbool();
-    });
-}
-
 static void MuHash(benchmark::Bench& bench)
 {
     MuHash3072 acc;
@@ -274,8 +258,6 @@ BENCHMARK(SHA256D64_1024_STANDARD, benchmark::PriorityLevel::HIGH);
 BENCHMARK(SHA256D64_1024_SSE4, benchmark::PriorityLevel::HIGH);
 BENCHMARK(SHA256D64_1024_AVX2, benchmark::PriorityLevel::HIGH);
 BENCHMARK(SHA256D64_1024_SHANI, benchmark::PriorityLevel::HIGH);
-BENCHMARK(FastRandom_32bit, benchmark::PriorityLevel::HIGH);
-BENCHMARK(FastRandom_1bit, benchmark::PriorityLevel::HIGH);
 
 BENCHMARK(MuHash, benchmark::PriorityLevel::HIGH);
 BENCHMARK(MuHashMul, benchmark::PriorityLevel::HIGH);

src/bench/random.cpp

@@ -0,0 +1,103 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <bench/bench.h>
+#include <random.h>
+
+#include <cstdint>
+#include <numeric>
+
+namespace {
+
+template<typename RNG>
+void BenchRandom_rand64(benchmark::Bench& bench, RNG&& rng) noexcept
+{
+    bench.batch(1).unit("number").run([&] {
+        rng.rand64();
+    });
+}
+
+template<typename RNG>
+void BenchRandom_rand32(benchmark::Bench& bench, RNG&& rng) noexcept
+{
+    bench.batch(1).unit("number").run([&] {
+        rng.rand32();
+    });
+}
+
+template<typename RNG>
+void BenchRandom_randbool(benchmark::Bench& bench, RNG&& rng) noexcept
+{
+    bench.batch(1).unit("number").run([&] {
+        rng.randbool();
+    });
+}
+
+template<typename RNG>
+void BenchRandom_randbits(benchmark::Bench& bench, RNG&& rng) noexcept
+{
+    bench.batch(64).unit("number").run([&] {
+        for (int i = 1; i <= 64; ++i) {
+            rng.randbits(i);
+        }
+    });
+}
+
+template<int RANGE, typename RNG>
+void BenchRandom_randrange(benchmark::Bench& bench, RNG&& rng) noexcept
+{
+    bench.batch(RANGE).unit("number").run([&] {
+        for (int i = 1; i <= RANGE; ++i) {
+            rng.randrange(i);
+        }
+    });
+}
+
+template<int RANGE, typename RNG>
+void BenchRandom_stdshuffle(benchmark::Bench& bench, RNG&& rng) noexcept
+{
+    uint64_t data[RANGE];
+    std::iota(std::begin(data), std::end(data), uint64_t(0));
+    bench.batch(RANGE).unit("number").run([&] {
+        std::shuffle(std::begin(data), std::end(data), rng);
+    });
+}
+
+void FastRandom_rand64(benchmark::Bench& bench) { BenchRandom_rand64(bench, FastRandomContext(true)); }
+void FastRandom_rand32(benchmark::Bench& bench) { BenchRandom_rand32(bench, FastRandomContext(true)); }
+void FastRandom_randbool(benchmark::Bench& bench) { BenchRandom_randbool(bench, FastRandomContext(true)); }
+void FastRandom_randbits(benchmark::Bench& bench) { BenchRandom_randbits(bench, FastRandomContext(true)); }
+void FastRandom_randrange100(benchmark::Bench& bench) { BenchRandom_randrange<100>(bench, FastRandomContext(true)); }
+void FastRandom_randrange1000(benchmark::Bench& bench) { BenchRandom_randrange<1000>(bench, FastRandomContext(true)); }
+void FastRandom_randrange1000000(benchmark::Bench& bench) { BenchRandom_randrange<1000000>(bench, FastRandomContext(true)); }
+void FastRandom_stdshuffle100(benchmark::Bench& bench) { BenchRandom_stdshuffle<100>(bench, FastRandomContext(true)); }
+
+void InsecureRandom_rand64(benchmark::Bench& bench) { BenchRandom_rand64(bench, InsecureRandomContext(251438)); }
+void InsecureRandom_rand32(benchmark::Bench& bench) { BenchRandom_rand32(bench, InsecureRandomContext(251438)); }
+void InsecureRandom_randbool(benchmark::Bench& bench) { BenchRandom_randbool(bench, InsecureRandomContext(251438)); }
+void InsecureRandom_randbits(benchmark::Bench& bench) { BenchRandom_randbits(bench, InsecureRandomContext(251438)); }
+void InsecureRandom_randrange100(benchmark::Bench& bench) { BenchRandom_randrange<100>(bench, InsecureRandomContext(251438)); }
+void InsecureRandom_randrange1000(benchmark::Bench& bench) { BenchRandom_randrange<1000>(bench, InsecureRandomContext(251438)); }
+void InsecureRandom_randrange1000000(benchmark::Bench& bench) { BenchRandom_randrange<1000000>(bench, InsecureRandomContext(251438)); }
+void InsecureRandom_stdshuffle100(benchmark::Bench& bench) { BenchRandom_stdshuffle<100>(bench, InsecureRandomContext(251438)); }
+
+} // namespace
+
+BENCHMARK(FastRandom_rand64, benchmark::PriorityLevel::HIGH);
+BENCHMARK(FastRandom_rand32, benchmark::PriorityLevel::HIGH);
+BENCHMARK(FastRandom_randbool, benchmark::PriorityLevel::HIGH);
+BENCHMARK(FastRandom_randbits, benchmark::PriorityLevel::HIGH);
+BENCHMARK(FastRandom_randrange100, benchmark::PriorityLevel::HIGH);
+BENCHMARK(FastRandom_randrange1000, benchmark::PriorityLevel::HIGH);
+BENCHMARK(FastRandom_randrange1000000, benchmark::PriorityLevel::HIGH);
+BENCHMARK(FastRandom_stdshuffle100, benchmark::PriorityLevel::HIGH);
+
+BENCHMARK(InsecureRandom_rand64, benchmark::PriorityLevel::HIGH);
+BENCHMARK(InsecureRandom_rand32, benchmark::PriorityLevel::HIGH);
+BENCHMARK(InsecureRandom_randbool, benchmark::PriorityLevel::HIGH);
+BENCHMARK(InsecureRandom_randbits, benchmark::PriorityLevel::HIGH);
+BENCHMARK(InsecureRandom_randrange100, benchmark::PriorityLevel::HIGH);
+BENCHMARK(InsecureRandom_randrange1000, benchmark::PriorityLevel::HIGH);
+BENCHMARK(InsecureRandom_randrange1000000, benchmark::PriorityLevel::HIGH);
+BENCHMARK(InsecureRandom_stdshuffle100, benchmark::PriorityLevel::HIGH);

src/net.cpp

@@ -415,7 +415,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
     if (pszDest) {
         std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
         if (!resolved.empty()) {
-            Shuffle(resolved.begin(), resolved.end(), FastRandomContext());
+            std::shuffle(resolved.begin(), resolved.end(), FastRandomContext());
             // If the connection is made by name, it can be the case that the name resolves to more than one address.
             // We don't want to connect any more of them if we are already connected to one
             for (const auto& r : resolved) {
@@ -2212,7 +2212,7 @@ void CConnman::ThreadDNSAddressSeed()
 
     FastRandomContext rng;
     std::vector<std::string> seeds = m_params.DNSSeeds();
-    Shuffle(seeds.begin(), seeds.end(), rng);
+    std::shuffle(seeds.begin(), seeds.end(), rng);
     int seeds_right_now = 0; // Number of seeds left before testing if we have enough connections
 
     if (gArgs.GetBoolArg("-forcednsseed", DEFAULT_FORCEDNSSEED)) {
@@ -2439,7 +2439,7 @@ bool CConnman::MultipleManualOrFullOutboundConns(Network net) const
 bool CConnman::MaybePickPreferredNetwork(std::optional<Network>& network)
 {
     std::array<Network, 5> nets{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS};
-    Shuffle(nets.begin(), nets.end(), FastRandomContext());
+    std::shuffle(nets.begin(), nets.end(), FastRandomContext());
 
     LOCK(m_nodes_mutex);
     for (const auto net : nets) {

src/net.h

@@ -1625,7 +1625,7 @@ class CConnman
                 }
             }
             if (shuffle) {
-                Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext{});
+                std::shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext{});
             }
         }
 

src/net_processing.cpp

@@ -3339,7 +3339,7 @@ std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::Find1P1CPacka
     // Create a random permutation of the indices.
     std::vector<size_t> tx_indices(cpfp_candidates_different_peer.size());
     std::iota(tx_indices.begin(), tx_indices.end(), 0);
-    Shuffle(tx_indices.begin(), tx_indices.end(), m_rng);
+    std::shuffle(tx_indices.begin(), tx_indices.end(), m_rng);
 
     for (const auto index : tx_indices) {
         // If we already tried a package and failed for any reason, the combined hash was
@@ -4106,7 +4106,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         const bool rate_limited = !pfrom.HasPermission(NetPermissionFlags::Addr);
         uint64_t num_proc = 0;
         uint64_t num_rate_limit = 0;
-        Shuffle(vAddr.begin(), vAddr.end(), m_rng);
+        std::shuffle(vAddr.begin(), vAddr.end(), m_rng);
         for (CAddress& addr : vAddr)
         {
             if (interruptMsgProc)

src/random.h

@@ -458,29 +458,6 @@ inline uint256 GetRandHash() noexcept
     return hash;
 }
 
-/** More efficient than using std::shuffle on a FastRandomContext.
- *
- * This is more efficient as std::shuffle will consume entropy in groups of
- * 64 bits at the time and throw away most.
- *
- * This also works around a bug in libstdc++ std::shuffle that may cause
- * type::operator=(type&&) to be invoked on itself, which the library's
- * debug mode detects and panics on. This is a known issue, see
- * https://stackoverflow.com/questions/22915325/avoiding-self-assignment-in-stdshuffle
- */
-template <typename I, RandomNumberGenerator R>
-void Shuffle(I first, I last, R&& rng)
-{
-    while (first != last) {
-        size_t j = rng.randrange(last - first);
-        if (j) {
-            using std::swap;
-            swap(*first, *(first + j));
-        }
-        ++first;
-    }
-}
-
 /* ============================= MISCELLANEOUS TEST-ONLY FUNCTIONS ============================= */
 
 /** Check that OS randomness is available and returning the requested number

src/rpc/rawtransaction.cpp

@@ -1790,8 +1790,8 @@ static RPCHelpMan joinpsbts()
     std::iota(output_indices.begin(), output_indices.end(), 0);
 
     // Shuffle input and output indices lists
-    Shuffle(input_indices.begin(), input_indices.end(), FastRandomContext());
-    Shuffle(output_indices.begin(), output_indices.end(), FastRandomContext());
+    std::shuffle(input_indices.begin(), input_indices.end(), FastRandomContext());
+    std::shuffle(output_indices.begin(), output_indices.end(), FastRandomContext());
 
     PartiallySignedTransaction shuffled_psbt;
     shuffled_psbt.tx = CMutableTransaction();

src/test/fuzz/random.cpp

@@ -26,6 +26,5 @@ FUZZ_TARGET(random)
     (void)fast_random_context();
 
     std::vector<int64_t> integrals = ConsumeRandomLengthIntegralVector<int64_t>(fuzzed_data_provider);
-    Shuffle(integrals.begin(), integrals.end(), fast_random_context);
     std::shuffle(integrals.begin(), integrals.end(), fast_random_context);
 }

src/test/miniscript_tests.cpp

@@ -346,7 +346,7 @@ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, con
     auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
     std::vector<Challenge> challist(challenges.begin(), challenges.end());
     for (int iter = 0; iter < 3; ++iter) {
-        Shuffle(challist.begin(), challist.end(), g_insecure_rand_ctx);
+        std::shuffle(challist.begin(), challist.end(), g_insecure_rand_ctx);
         Satisfier satisfier(converter.MsContext());
         TestSignatureChecker checker(satisfier);
         bool prev_mal_success = false, prev_nonmal_success = false;