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

6ecda04fefad980872c72fba89844393f5581120 random: drop ad-hoc Shuffle in favor of std::shuffle (Pieter Wuille)
da28a26aae3178fb7663efbe20bb650857ace775 bench random: benchmark more functions, and add InsecureRandomContext (Pieter Wuille)
0a9bbc64c157a314e5472ecd98300e30b12d3fdf 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 https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049).

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

Tree-SHA512: 2560b7312410581ff2b9bd0716e0f1558d910b5eadb9544785c972384985ac0f11f72d6b2797cfe2e7eb71fa57c30cffd98cc009cb4ee87a18b1524694211417

16 files changed, 123 insertions, 65 deletions

diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include
index 2ba72c9e76..cd2626b330 100644
--- a/src/Makefile.bench.include
+++ b/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 \
diff --git a/src/bench/crypto_hash.cpp b/src/bench/crypto_hash.cpp
index 1685a120b4..2551ff3593 100644
--- a/src/bench/crypto_hash.cpp
+++ b/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);
diff --git a/src/bench/random.cpp b/src/bench/random.cpp
new file mode 100644
index 0000000000..cff215d5a7
--- /dev/null
+++ b/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);
diff --git a/src/net.cpp b/src/net.cpp
index 4e4f6304b9..d265d78548 100644
--- a/src/net.cpp
+++ b/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) {
diff --git a/src/net.h b/src/net.h
index 8075975d62..0c3bf5cbc8 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1625,7 +1625,7 @@ private:
                 }
             }
             if (shuffle) {
-                Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext{});
+                std::shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext{});
             }
         }
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index df58abc3a9..aaf9273c7b 100644
--- a/src/net_processing.cpp
+++ b/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)
diff --git a/src/random.h b/src/random.h
index 8a6ef13d5e..536e697cca 100644
--- a/src/random.h
+++ b/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
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 75b538061d..ed9ef2c159 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/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();
diff --git a/src/test/fuzz/random.cpp b/src/test/fuzz/random.cpp
index 96668734fd..6b2d42738b 100644
--- a/src/test/fuzz/random.cpp
+++ b/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);
 }
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 7e39e9e4de..c99a4594ce 100644
--- a/src/test/miniscript_tests.cpp
+++ b/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;
diff --git a/src/test/net_peer_eviction_tests.cpp b/src/test/net_peer_eviction_tests.cpp
index 51d6c4384a..d9e1c2332e 100644
--- a/src/test/net_peer_eviction_tests.cpp
+++ b/src/test/net_peer_eviction_tests.cpp
@@ -31,7 +31,7 @@ bool IsProtected(int num_peers,
     for (NodeEvictionCandidate& candidate : candidates) {
         candidate_setup_fn(candidate);
     }
-    Shuffle(candidates.begin(), candidates.end(), random_context);
+    std::shuffle(candidates.begin(), candidates.end(), random_context);
 
     const size_t size{candidates.size()};
     const size_t expected{size - size / 2}; // Expect half the candidates will be protected.
@@ -572,7 +572,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
 // Returns true if any of the node ids in node_ids are selected for eviction.
 bool IsEvicted(std::vector<NodeEvictionCandidate> candidates, const std::unordered_set<NodeId>& node_ids, FastRandomContext& random_context)
 {
-    Shuffle(candidates.begin(), candidates.end(), random_context);
+    std::shuffle(candidates.begin(), candidates.end(), random_context);
     const std::optional<NodeId> evicted_node_id = SelectNodeToEvict(std::move(candidates));
     if (!evicted_node_id) {
         return false;
diff --git a/src/test/random_tests.cpp b/src/test/random_tests.cpp
index 9fa7135b77..3d8b543e64 100644
--- a/src/test/random_tests.cpp
+++ b/src/test/random_tests.cpp
@@ -206,10 +206,6 @@ BOOST_AUTO_TEST_CASE(stdrandom_test)
         for (int j = 1; j <= 10; ++j) {
             BOOST_CHECK(std::find(test.begin(), test.end(), j) != test.end());
         }
-        Shuffle(test.begin(), test.end(), ctx);
-        for (int j = 1; j <= 10; ++j) {
-            BOOST_CHECK(std::find(test.begin(), test.end(), j) != test.end());
-        }
     }
 }
 
@@ -220,7 +216,7 @@ BOOST_AUTO_TEST_CASE(shuffle_stat_test)
     uint32_t counts[5 * 5 * 5 * 5 * 5] = {0};
     for (int i = 0; i < 12000; ++i) {
         int data[5] = {0, 1, 2, 3, 4};
-        Shuffle(std::begin(data), std::end(data), ctx);
+        std::shuffle(std::begin(data), std::end(data), ctx);
         int pos = data[0] + data[1] * 5 + data[2] * 25 + data[3] * 125 + data[4] * 625;
         ++counts[pos];
     }
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
index 478121cc6f..8c873c85a3 100644
--- a/src/test/txpackage_tests.cpp
+++ b/src/test/txpackage_tests.cpp
@@ -303,7 +303,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup)
 
         // The parents can be in any order.
         FastRandomContext rng;
-        Shuffle(package.begin(), package.end(), rng);
+        std::shuffle(package.begin(), package.end(), rng);
         package.push_back(MakeTransactionRef(child));
 
         PackageValidationState state;
diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp
index dc257a0d51..0ca70d2c7a 100644
--- a/src/test/txrequest_tests.cpp
+++ b/src/test/txrequest_tests.cpp
@@ -392,7 +392,7 @@ void BuildBigPriorityTest(Scenario& scenario, int peers)
 
     // Determine the announcement order randomly.
     std::vector<NodeId> announce_order = request_order;
-    Shuffle(announce_order.begin(), announce_order.end(), g_insecure_rand_ctx);
+    std::shuffle(announce_order.begin(), announce_order.end(), g_insecure_rand_ctx);
 
     // Find a gtxid whose txhash prioritization is consistent with the required ordering within pref_peers and
     // within npref_peers.
@@ -697,7 +697,7 @@ void TestInterleavedScenarios()
         builders.emplace_back([](Scenario& scenario){ BuildWeirdRequestsTest(scenario); });
     }
     // Randomly shuffle all those functions.
-    Shuffle(builders.begin(), builders.end(), g_insecure_rand_ctx);
+    std::shuffle(builders.begin(), builders.end(), g_insecure_rand_ctx);
 
     Runner runner;
     auto starttime = RandomTime1y();
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index f1706b6800..edde912ce0 100644
--- a/src/wallet/coinselection.cpp
+++ b/src/wallet/coinselection.cpp
@@ -549,7 +549,7 @@ util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utx
     std::vector<size_t> indexes;
     indexes.resize(utxo_pool.size());
     std::iota(indexes.begin(), indexes.end(), 0);
-    Shuffle(indexes.begin(), indexes.end(), rng);
+    std::shuffle(indexes.begin(), indexes.end(), rng);
 
     CAmount selected_eff_value = 0;
     int weight = 0;
@@ -659,7 +659,7 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
     std::vector<OutputGroup> applicable_groups;
     CAmount nTotalLower = 0;
 
-    Shuffle(groups.begin(), groups.end(), rng);
+    std::shuffle(groups.begin(), groups.end(), rng);
 
     for (const OutputGroup& group : groups) {
         if (group.GetSelectionAmount() == nTargetValue) {
@@ -927,7 +927,7 @@ const std::set<std::shared_ptr<COutput>>& SelectionResult::GetInputSet() const
 std::vector<std::shared_ptr<COutput>> SelectionResult::GetShuffledInputVector() const
 {
     std::vector<std::shared_ptr<COutput>> coins(m_selected_inputs.begin(), m_selected_inputs.end());
-    Shuffle(coins.begin(), coins.end(), FastRandomContext());
+    std::shuffle(coins.begin(), coins.end(), FastRandomContext());
     return coins;
 }
 
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 0a59353052..11a42eb469 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -226,7 +226,7 @@ void CoinsResult::Erase(const std::unordered_set<COutPoint, SaltedOutpointHasher
 void CoinsResult::Shuffle(FastRandomContext& rng_fast)
 {
     for (auto& it : coins) {
-        ::Shuffle(it.second.begin(), it.second.end(), rng_fast);
+        std::shuffle(it.second.begin(), it.second.end(), rng_fast);
     }
 }