Kill insecure_random and associated global state

There are only a few uses of `insecure_random` outside the tests.
This PR replaces uses of insecure_random (and its accompanying global
state) in the core code with an FastRandomContext that is automatically
seeded on creation.

This is meant to be used for inner loops. The FastRandomContext
can be in the outer scope, or the class itself, then rand32() is used
inside the loop. Useful e.g. for pushing addresses in CNode or the fee
rounding, or randomization for coin selection.

As a context is created per purpose, thus it gets rid of
cross-thread unprotected shared usage of a single set of globals, this
should also get rid of the potential race conditions.

- I'd say TxMempool::check is not called enough to warrant using a special
  fast random context, this is switched to GetRand() (open for
  discussion...)

- The use of `insecure_rand` in ConnectThroughProxy has been replaced by
  an atomic integer counter. The only goal here is to have a different
  credentials pair for each connection to go on a different Tor circuit,
  it does not need to be random nor unpredictable.

- To avoid having a FastRandomContext on every CNode, the context is
  passed into PushAddress as appropriate.

There remains an insecure_random for test usage in `test_random.h`.

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;
+                nKBucket = (nKBucket + insecure_rand.rand32()) % ADDRMAN_TRIED_BUCKET_COUNT;
-                nKBucketPos = (nKBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE;
+                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;
+                nUBucket = (nUBucket + insecure_rand.rand32()) % ADDRMAN_NEW_BUCKET_COUNT;
-                nUBucketPos = (nUBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE;
+                nUBucketPos = (nUBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE;
             }
             int nId = vvNew[nUBucket][nUBucketPos];
             assert(mapInfo.count(nId) == 1);

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);
 

src/main.cpp

@@ -4758,6 +4758,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) {
@@ -4766,9 +4767,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));
@@ -5078,14 +5079,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);
                 }
             }
 
@@ -6008,8 +6010,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);
     }
 
 
@@ -6842,7 +6845,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;
             }
         }
     }

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

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

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());
+        static std::atomic_int counter;
-        random_auth.password = strprintf("%i", insecure_rand());
+        random_auth.username = random_auth.password = strprintf("%i", counter++);
         if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket))
             return false;
     } else {

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;

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 */

src/random.cpp

@@ -178,22 +178,21 @@ uint256 GetRandHash()
     return hash;
 }
 
-uint32_t insecure_rand_Rz = 11;
+FastRandomContext::FastRandomContext(bool fDeterministic)
-uint32_t insecure_rand_Rw = 11;
-void seed_insecure_rand(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;
     }
 }
+

src/random.h

@@ -28,25 +28,22 @@ uint256 GetRandHash();
 void GetStrongRandBytes(unsigned char* buf, int num);
 
 /**
- * Seed insecure_rand using the random pool.
+ * Fast randomness source. This is seeded once with secure random data, but
- * @param Deterministic Use a deterministic seed
+ * is completely deterministic and insecure after that.
+ * This class is not thread-safe.
  */
-void seed_insecure_rand(bool fDeterministic = false);
+class FastRandomContext {
+public:
+    explicit FastRandomContext(bool fDeterministic=false);
 
-/**
+    uint32_t rand32() {
- * MWC RNG of George Marsaglia
+        Rz = 36969 * (Rz & 65535) + (Rz >> 16);
- * This is intended to be fast. It has a period of 2^59.3, though the
+        Rw = 18000 * (Rw & 65535) + (Rw >> 16);
- * least significant 16 bits only have a period of about 2^30.1.
+        return (Rw << 16) + Rz;
- *
+    }
- * @return random value
+
- */
+    uint32_t Rz;
-extern uint32_t insecure_rand_Rz;
+    uint32_t Rw;
-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;
-}
 
 #endif // BITCOIN_RANDOM_H

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)

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"

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"
 

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>
 

src/test/net_tests.cpp

@@ -23,7 +23,7 @@ public:
     void MakeDeterministic()
     {
         nKey.SetNull();
-        seed_insecure_rand(true);
+        insecure_rand = FastRandomContext(true);
     }
 };
 

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>

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;
+    FastRandomContext rand_cache;
-    uint32_t insecure_rand_Rw_cache;
 
 
     template <typename A, typename B>
@@ -172,14 +171,13 @@ public:
     }
     ~prevector_tester() {
         BOOST_CHECK_MESSAGE(passed, "insecure_rand_Rz: "
-                << insecure_rand_Rz_cache 
+                << 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;
+        rand_cache = insecure_rand_ctx;
-        insecure_rand_Rw_cache = insecure_rand_Rw;
     }
 };
 

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);

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"

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"
 

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();

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

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"

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"

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());

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"

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"

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;