Merge #8914: Kill insecure_random and associated global state

5eaaa83 Kill insecure_random and associated global state (Wladimir J. van der Laan)

28 files changed, 92 insertions, 66 deletions

diff --git a/src/addrman.cpp b/src/addrman.cpp
index bfb8e94575..2016523212 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -358,8 +358,8 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
             int nKBucket = RandomInt(ADDRMAN_TRIED_BUCKET_COUNT);
             int nKBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE);
             while (vvTried[nKBucket][nKBucketPos] == -1) {
-                nKBucket = (nKBucket + insecure_rand()) % ADDRMAN_TRIED_BUCKET_COUNT;
-                nKBucketPos = (nKBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE;
+                nKBucket = (nKBucket + insecure_rand.rand32()) % ADDRMAN_TRIED_BUCKET_COUNT;
+                nKBucketPos = (nKBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE;
             }
             int nId = vvTried[nKBucket][nKBucketPos];
             assert(mapInfo.count(nId) == 1);
@@ -375,8 +375,8 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
             int nUBucket = RandomInt(ADDRMAN_NEW_BUCKET_COUNT);
             int nUBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE);
             while (vvNew[nUBucket][nUBucketPos] == -1) {
-                nUBucket = (nUBucket + insecure_rand()) % ADDRMAN_NEW_BUCKET_COUNT;
-                nUBucketPos = (nUBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE;
+                nUBucket = (nUBucket + insecure_rand.rand32()) % ADDRMAN_NEW_BUCKET_COUNT;
+                nUBucketPos = (nUBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE;
             }
             int nId = vvNew[nUBucket][nUBucketPos];
             assert(mapInfo.count(nId) == 1);
diff --git a/src/addrman.h b/src/addrman.h
index 9bab39049d..e9e137c978 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -211,6 +211,9 @@ protected:
     //! secret key to randomize bucket select with
     uint256 nKey;
 
+    //! Source of random numbers for randomization in inner loops
+    FastRandomContext insecure_rand;
+
     //! Find an entry.
     CAddrInfo* Find(const CNetAddr& addr, int *pnId = NULL);
 
diff --git a/src/main.cpp b/src/main.cpp
index ad4c3fce68..d14a1bed3a 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -4762,6 +4762,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma
     uint64_t hashAddr = addr.GetHash();
     std::multimap<uint64_t, CNode*> mapMix;
     const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24*60*60));
+    FastRandomContext insecure_rand;
 
     auto sortfunc = [&mapMix, &hasher](CNode* pnode) {
         if (pnode->nVersion >= CADDR_TIME_VERSION) {
@@ -4770,9 +4771,9 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma
         }
     };
 
-    auto pushfunc = [&addr, &mapMix, &nRelayNodes] {
+    auto pushfunc = [&addr, &mapMix, &nRelayNodes, &insecure_rand] {
         for (auto mi = mapMix.begin(); mi != mapMix.end() && nRelayNodes-- > 0; ++mi)
-            mi->second->PushAddress(addr);
+            mi->second->PushAddress(addr, insecure_rand);
     };
 
     connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));
@@ -5082,14 +5083,15 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
             if (fListen && !IsInitialBlockDownload())
             {
                 CAddress addr = GetLocalAddress(&pfrom->addr, pfrom->GetLocalServices());
+                FastRandomContext insecure_rand;
                 if (addr.IsRoutable())
                 {
                     LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString());
-                    pfrom->PushAddress(addr);
+                    pfrom->PushAddress(addr, insecure_rand);
                 } else if (IsPeerAddrLocalGood(pfrom)) {
                     addr.SetIP(pfrom->addrLocal);
                     LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString());
-                    pfrom->PushAddress(addr);
+                    pfrom->PushAddress(addr, insecure_rand);
                 }
             }
 
@@ -6012,8 +6014,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
 
         pfrom->vAddrToSend.clear();
         vector<CAddress> vAddr = connman.GetAddresses();
+        FastRandomContext insecure_rand;
         BOOST_FOREACH(const CAddress &addr, vAddr)
-            pfrom->PushAddress(addr);
+            pfrom->PushAddress(addr, insecure_rand);
     }
 
 
@@ -6846,7 +6849,7 @@ bool SendMessages(CNode* pto, CConnman& connman)
             // until scheduled broadcast, then move the broadcast to within MAX_FEEFILTER_CHANGE_DELAY.
             else if (timeNow + MAX_FEEFILTER_CHANGE_DELAY * 1000000 < pto->nextSendTimeFeeFilter &&
                      (currentFilter < 3 * pto->lastSentFeeFilter / 4 || currentFilter > 4 * pto->lastSentFeeFilter / 3)) {
-                pto->nextSendTimeFeeFilter = timeNow + (insecure_rand() % MAX_FEEFILTER_CHANGE_DELAY) * 1000000;
+                pto->nextSendTimeFeeFilter = timeNow + GetRandInt(MAX_FEEFILTER_CHANGE_DELAY) * 1000000;
             }
         }
     }
diff --git a/src/net.cpp b/src/net.cpp
index 8d00c616f6..34dc67eb43 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -187,7 +187,8 @@ void AdvertiseLocal(CNode *pnode)
         if (addrLocal.IsRoutable())
         {
             LogPrint("net", "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
-            pnode->PushAddress(addrLocal);
+            FastRandomContext insecure_rand;
+            pnode->PushAddress(addrLocal, insecure_rand);
         }
     }
 }
diff --git a/src/net.h b/src/net.h
index 90a4db2c63..58b492e592 100644
--- a/src/net.h
+++ b/src/net.h
@@ -735,14 +735,14 @@ public:
         addrKnown.insert(_addr.GetKey());
     }
 
-    void PushAddress(const CAddress& _addr)
+    void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand)
     {
         // Known checking here is only to save space from duplicates.
         // SendMessages will filter it again for knowns that were added
         // after addresses were pushed.
         if (_addr.IsValid() && !addrKnown.contains(_addr.GetKey())) {
             if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) {
-                vAddrToSend[insecure_rand() % vAddrToSend.size()] = _addr;
+                vAddrToSend[insecure_rand.rand32() % vAddrToSend.size()] = _addr;
             } else {
                 vAddrToSend.push_back(_addr);
             }
diff --git a/src/netbase.cpp b/src/netbase.cpp
index 7d7f1b6788..9fe34108f5 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -596,8 +596,8 @@ static bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDe
     // do socks negotiation
     if (proxy.randomize_credentials) {
         ProxyCredentials random_auth;
-        random_auth.username = strprintf("%i", insecure_rand());
-        random_auth.password = strprintf("%i", insecure_rand());
+        static std::atomic_int counter;
+        random_auth.username = random_auth.password = strprintf("%i", counter++);
         if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket))
             return false;
     } else {
diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
index 7b0e8b7d08..c07cd2eff8 100644
--- a/src/policy/fees.cpp
+++ b/src/policy/fees.cpp
@@ -594,7 +594,7 @@ FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee)
 CAmount FeeFilterRounder::round(CAmount currentMinFee)
 {
     std::set<double>::iterator it = feeset.lower_bound(currentMinFee);
-    if ((it != feeset.begin() && insecure_rand() % 3 != 0) || it == feeset.end()) {
+    if ((it != feeset.begin() && insecure_rand.rand32() % 3 != 0) || it == feeset.end()) {
         it--;
     }
     return *it;
diff --git a/src/policy/fees.h b/src/policy/fees.h
index 463f62f710..2c1ac3b934 100644
--- a/src/policy/fees.h
+++ b/src/policy/fees.h
@@ -7,6 +7,7 @@
 
 #include "amount.h"
 #include "uint256.h"
+#include "random.h"
 
 #include <map>
 #include <string>
@@ -298,5 +299,6 @@ public:
 
 private:
     std::set<double> feeset;
+    FastRandomContext insecure_rand;
 };
 #endif /*BITCOIN_POLICYESTIMATOR_H */
diff --git a/src/random.cpp b/src/random.cpp
index d9a8cc145e..aa027e49c4 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -178,22 +178,21 @@ uint256 GetRandHash()
     return hash;
 }
 
-uint32_t insecure_rand_Rz = 11;
-uint32_t insecure_rand_Rw = 11;
-void seed_insecure_rand(bool fDeterministic)
+FastRandomContext::FastRandomContext(bool fDeterministic)
 {
     // The seed values have some unlikely fixed points which we avoid.
     if (fDeterministic) {
-        insecure_rand_Rz = insecure_rand_Rw = 11;
+        Rz = Rw = 11;
     } else {
         uint32_t tmp;
         do {
             GetRandBytes((unsigned char*)&tmp, 4);
         } while (tmp == 0 || tmp == 0x9068ffffU);
-        insecure_rand_Rz = tmp;
+        Rz = tmp;
         do {
             GetRandBytes((unsigned char*)&tmp, 4);
         } while (tmp == 0 || tmp == 0x464fffffU);
-        insecure_rand_Rw = tmp;
+        Rw = tmp;
     }
 }
+
diff --git a/src/random.h b/src/random.h
index 31b80bd565..e97d2d1fb0 100644
--- a/src/random.h
+++ b/src/random.h
@@ -28,25 +28,22 @@ uint256 GetRandHash();
 void GetStrongRandBytes(unsigned char* buf, int num);
 
 /**
- * Seed insecure_rand using the random pool.
- * @param Deterministic Use a deterministic seed
+ * Fast randomness source. This is seeded once with secure random data, but
+ * is completely deterministic and insecure after that.
+ * This class is not thread-safe.
  */
-void seed_insecure_rand(bool fDeterministic = false);
-
-/**
- * MWC RNG of George Marsaglia
- * This is intended to be fast. It has a period of 2^59.3, though the
- * least significant 16 bits only have a period of about 2^30.1.
- *
- * @return random value
- */
-extern uint32_t insecure_rand_Rz;
-extern uint32_t insecure_rand_Rw;
-static inline uint32_t insecure_rand(void)
-{
-    insecure_rand_Rz = 36969 * (insecure_rand_Rz & 65535) + (insecure_rand_Rz >> 16);
-    insecure_rand_Rw = 18000 * (insecure_rand_Rw & 65535) + (insecure_rand_Rw >> 16);
-    return (insecure_rand_Rw << 16) + insecure_rand_Rz;
-}
+class FastRandomContext {
+public:
+    explicit FastRandomContext(bool fDeterministic=false);
+
+    uint32_t rand32() {
+        Rz = 36969 * (Rz & 65535) + (Rz >> 16);
+        Rw = 18000 * (Rw & 65535) + (Rw >> 16);
+        return (Rw << 16) + Rz;
+    }
+
+    uint32_t Rz;
+    uint32_t Rw;
+};
 
 #endif // BITCOIN_RANDOM_H
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 5ccaeb57bb..adff09f754 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -26,7 +26,7 @@ public:
     void MakeDeterministic()
     {
         nKey.SetNull();
-        seed_insecure_rand(true);
+        insecure_rand = FastRandomContext(true);
     }
 
     int RandomInt(int nMax)
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index e692326559..b487686136 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include "coins.h"
-#include "random.h"
+#include "test_random.h"
 #include "script/standard.h"
 #include "uint256.h"
 #include "utilstrencodings.h"
diff --git a/src/test/crypto_tests.cpp b/src/test/crypto_tests.cpp
index ff0adae1d2..c7b4fb240c 100644
--- a/src/test/crypto_tests.cpp
+++ b/src/test/crypto_tests.cpp
@@ -9,7 +9,7 @@
 #include "crypto/sha512.h"
 #include "crypto/hmac_sha256.h"
 #include "crypto/hmac_sha512.h"
-#include "random.h"
+#include "test_random.h"
 #include "utilstrencodings.h"
 #include "test/test_bitcoin.h"
 
diff --git a/src/test/merkle_tests.cpp b/src/test/merkle_tests.cpp
index b40ab848dc..66ca381ea7 100644
--- a/src/test/merkle_tests.cpp
+++ b/src/test/merkle_tests.cpp
@@ -4,7 +4,7 @@
 
 #include "consensus/merkle.h"
 #include "test/test_bitcoin.h"
-#include "random.h"
+#include "test_random.h"
 
 #include <boost/test/unit_test.hpp>
 
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 4a7d6e778f..f4b5768693 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -23,7 +23,7 @@ public:
     void MakeDeterministic()
     {
         nKey.SetNull();
-        seed_insecure_rand(true);
+        insecure_rand = FastRandomContext(true);
     }
 };
 
diff --git a/src/test/pmt_tests.cpp b/src/test/pmt_tests.cpp
index e9c8691745..b7f83d38f0 100644
--- a/src/test/pmt_tests.cpp
+++ b/src/test/pmt_tests.cpp
@@ -9,7 +9,7 @@
 #include "uint256.h"
 #include "arith_uint256.h"
 #include "version.h"
-#include "random.h"
+#include "test_random.h"
 #include "test/test_bitcoin.h"
 
 #include <vector>
diff --git a/src/test/prevector_tests.cpp b/src/test/prevector_tests.cpp
index b8c45ca564..6cad02e738 100644
--- a/src/test/prevector_tests.cpp
+++ b/src/test/prevector_tests.cpp
@@ -4,7 +4,7 @@
 
 #include <vector>
 #include "prevector.h"
-#include "random.h"
+#include "test_random.h"
 
 #include "serialize.h"
 #include "streams.h"
@@ -27,8 +27,7 @@ class prevector_tester {
 
     typedef typename pretype::size_type Size;
     bool passed = true;
-    uint32_t insecure_rand_Rz_cache;
-    uint32_t insecure_rand_Rw_cache;
+    FastRandomContext rand_cache;
 
 
     template <typename A, typename B>
@@ -171,15 +170,14 @@ public:
         test();
     }
     ~prevector_tester() {
-        BOOST_CHECK_MESSAGE(passed, "insecure_rand_Rz: " 
-                << insecure_rand_Rz_cache 
+        BOOST_CHECK_MESSAGE(passed, "insecure_rand_Rz: "
+                << rand_cache.Rz
                 << ", insecure_rand_Rw: "
-                << insecure_rand_Rw_cache);
+                << rand_cache.Rw);
     }
     prevector_tester() {
         seed_insecure_rand();
-        insecure_rand_Rz_cache = insecure_rand_Rz;
-        insecure_rand_Rw_cache = insecure_rand_Rw;
+        rand_cache = insecure_rand_ctx;
     }
 };
 
diff --git a/src/test/scheduler_tests.cpp b/src/test/scheduler_tests.cpp
index aa12dfbd54..891ecf5015 100644
--- a/src/test/scheduler_tests.cpp
+++ b/src/test/scheduler_tests.cpp
@@ -42,8 +42,6 @@ static void MicroSleep(uint64_t n)
 
 BOOST_AUTO_TEST_CASE(manythreads)
 {
-    seed_insecure_rand(false);
-
     // Stress test: hundreds of microsecond-scheduled tasks,
     // serviced by 10 threads.
     //
@@ -58,7 +56,7 @@ BOOST_AUTO_TEST_CASE(manythreads)
 
     boost::mutex counterMutex[10];
     int counter[10] = { 0 };
-    boost::random::mt19937 rng(insecure_rand());
+    boost::random::mt19937 rng(42);
     boost::random::uniform_int_distribution<> zeroToNine(0, 9);
     boost::random::uniform_int_distribution<> randomMsec(-11, 1000);
     boost::random::uniform_int_distribution<> randomDelta(-1000, 1000);
diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp
index 4a48347b70..0b1050d020 100644
--- a/src/test/sighash_tests.cpp
+++ b/src/test/sighash_tests.cpp
@@ -6,7 +6,7 @@
 #include "data/sighash.json.h"
 #include "hash.h"
 #include "main.h" // For CheckTransaction
-#include "random.h"
+#include "test_random.h"
 #include "script/interpreter.h"
 #include "script/script.h"
 #include "serialize.h"
diff --git a/src/test/skiplist_tests.cpp b/src/test/skiplist_tests.cpp
index f14b902fe1..b19f8fbffb 100644
--- a/src/test/skiplist_tests.cpp
+++ b/src/test/skiplist_tests.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include "chain.h"
-#include "random.h"
+#include "test_random.h"
 #include "util.h"
 #include "test/test_bitcoin.h"
 
diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp
index 02843d8525..27ea837fbf 100644
--- a/src/test/test_bitcoin.cpp
+++ b/src/test/test_bitcoin.cpp
@@ -27,6 +27,7 @@
 #include <boost/thread.hpp>
 
 std::unique_ptr<CConnman> g_connman;
+FastRandomContext insecure_rand_ctx(true);
 
 extern bool fPrintToConsole;
 extern void noui_connect();
diff --git a/src/test/test_random.h b/src/test/test_random.h
new file mode 100644
index 0000000000..e61b92b7bc
--- /dev/null
+++ b/src/test/test_random.h
@@ -0,0 +1,23 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-2014 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_TEST_RANDOM_H
+#define BITCOIN_TEST_RANDOM_H
+
+#include "random.h"
+
+extern FastRandomContext insecure_rand_ctx;
+
+static inline void seed_insecure_rand(bool fDeterministic = false)
+{
+    insecure_rand_ctx = FastRandomContext(fDeterministic);
+}
+
+static inline uint32_t insecure_rand(void)
+{
+    return insecure_rand_ctx.rand32();
+}
+
+#endif
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index efd4498747..0f1c7ab222 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -6,7 +6,7 @@
 
 #include "clientversion.h"
 #include "primitives/transaction.h"
-#include "random.h"
+#include "test_random.h"
 #include "sync.h"
 #include "utilstrencodings.h"
 #include "utilmoneystr.h"
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index 1f86a06a3f..ffc0ff6f8e 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include "chain.h"
-#include "random.h"
+#include "test_random.h"
 #include "versionbits.h"
 #include "test/test_bitcoin.h"
 #include "chainparams.h"
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 15fa6fbca3..0f1c166abc 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -647,7 +647,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
     if (nCheckFrequency == 0)
         return;
 
-    if (insecure_rand() >= nCheckFrequency)
+    if (GetRand(std::numeric_limits<uint32_t>::max()) >= nCheckFrequency)
         return;
 
     LogPrint("mempool", "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size());
diff --git a/src/txmempool.h b/src/txmempool.h
index 941644b2b2..1763930ba0 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -15,6 +15,7 @@
 #include "indirectmap.h"
 #include "primitives/transaction.h"
 #include "sync.h"
+#include "random.h"
 
 #undef foreach
 #include "boost/multi_index_container.hpp"
diff --git a/src/wallet/test/crypto_tests.cpp b/src/wallet/test/crypto_tests.cpp
index 05387f5f2b..c5f55ef5f0 100644
--- a/src/wallet/test/crypto_tests.cpp
+++ b/src/wallet/test/crypto_tests.cpp
@@ -2,7 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include "random.h"
+#include "test/test_random.h"
 #include "utilstrencodings.h"
 #include "test/test_bitcoin.h"
 #include "wallet/crypter.h"
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index a1f69dd94d..687d84e77a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1907,7 +1907,7 @@ static void ApproximateBestSubset(vector<pair<CAmount, pair<const CWalletTx*,uns
     vfBest.assign(vValue.size(), true);
     nBest = nTotalLower;
 
-    seed_insecure_rand();
+    FastRandomContext insecure_rand;
 
     for (int nRep = 0; nRep < iterations && nBest != nTargetValue; nRep++)
     {
@@ -1924,7 +1924,7 @@ static void ApproximateBestSubset(vector<pair<CAmount, pair<const CWalletTx*,uns
                 //that the rng is fast. We do not use a constant random sequence,
                 //because there may be some privacy improvement by making
                 //the selection random.
-                if (nPass == 0 ? insecure_rand()&1 : !vfIncluded[i])
+                if (nPass == 0 ? insecure_rand.rand32()&1 : !vfIncluded[i])
                 {
                     nTotal += vValue[i].first;
                     vfIncluded[i] = true;