random: drop ad-hoc Shuffle in favor of std::shuffle

Benchmarks show it is no longer faster with modern standard C++ libraries,
and the debug-mode failure due to self-move has been fixed as well.

Author: sipa

src/bench/random.cpp

@@ -54,16 +54,6 @@ void BenchRandom_randrange(benchmark::Bench& bench, RNG&& rng) noexcept
     });
 }
 
-template<int RANGE, typename RNG>
-void BenchRandom_Shuffle(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([&] {
-        Shuffle(std::begin(data), std::end(data), rng);
-    });
-}
-
 template<int RANGE, typename RNG>
 void BenchRandom_stdshuffle(benchmark::Bench& bench, RNG&& rng) noexcept
 {
@@ -81,7 +71,6 @@ void FastRandom_randbits(benchmark::Bench& bench) { BenchRandom_randbits(bench,
 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_Shuffle100(benchmark::Bench& bench) { BenchRandom_Shuffle<100>(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)); }
@@ -91,7 +80,6 @@ void InsecureRandom_randbits(benchmark::Bench& bench) { BenchRandom_randbits(ben
 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_Shuffle100(benchmark::Bench& bench) { BenchRandom_Shuffle<100>(bench, InsecureRandomContext(251438)); }
 void InsecureRandom_stdshuffle100(benchmark::Bench& bench) { BenchRandom_stdshuffle<100>(bench, InsecureRandomContext(251438)); }
 
 } // namespace
@@ -103,7 +91,6 @@ 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_Shuffle100, benchmark::PriorityLevel::HIGH);
 BENCHMARK(FastRandom_stdshuffle100, benchmark::PriorityLevel::HIGH);
 
 BENCHMARK(InsecureRandom_rand64, benchmark::PriorityLevel::HIGH);
@@ -113,5 +100,4 @@ 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_Shuffle100, 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) {
@@ -2208,7 +2208,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)) {
@@ -2435,7 +2435,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;

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;

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];
     }