Skip to content

Commit 5eaaa83

Browse files
laanwjlaanwj
committed
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`.
1 parent 8d46429 commit 5eaaa83

28 files changed

+92
-66
lines changed

‎src/addrman.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -358,8 +358,8 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
358358
int nKBucket = RandomInt(ADDRMAN_TRIED_BUCKET_COUNT);
359359
int nKBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE);
360360
while (vvTried[nKBucket][nKBucketPos] == -1) {
361-
nKBucket = (nKBucket + insecure_rand()) % ADDRMAN_TRIED_BUCKET_COUNT;
362-
nKBucketPos = (nKBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE;
361+
nKBucket = (nKBucket + insecure_rand.rand32()) % ADDRMAN_TRIED_BUCKET_COUNT;
362+
nKBucketPos = (nKBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE;
363363
}
364364
int nId = vvTried[nKBucket][nKBucketPos];
365365
assert(mapInfo.count(nId) == 1);
@@ -375,8 +375,8 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
375375
int nUBucket = RandomInt(ADDRMAN_NEW_BUCKET_COUNT);
376376
int nUBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE);
377377
while (vvNew[nUBucket][nUBucketPos] == -1) {
378-
nUBucket = (nUBucket + insecure_rand()) % ADDRMAN_NEW_BUCKET_COUNT;
379-
nUBucketPos = (nUBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE;
378+
nUBucket = (nUBucket + insecure_rand.rand32()) % ADDRMAN_NEW_BUCKET_COUNT;
379+
nUBucketPos = (nUBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE;
380380
}
381381
int nId = vvNew[nUBucket][nUBucketPos];
382382
assert(mapInfo.count(nId) == 1);

‎src/addrman.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,9 @@ class CAddrMan
211211
//! secret key to randomize bucket select with
212212
uint256 nKey;
213213

214+
//! Source of random numbers for randomization in inner loops
215+
FastRandomContext insecure_rand;
216+
214217
//! Find an entry.
215218
CAddrInfo* Find(const CNetAddr& addr, int *pnId = NULL);
216219

‎src/main.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4758,6 +4758,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma
47584758
uint64_t hashAddr = addr.GetHash();
47594759
std::multimap<uint64_t, CNode*> mapMix;
47604760
const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24*60*60));
4761+
FastRandomContext insecure_rand;
47614762

47624763
auto sortfunc = [&mapMix, &hasher](CNode* pnode) {
47634764
if (pnode->nVersion >= CADDR_TIME_VERSION) {
@@ -4766,9 +4767,9 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma
47664767
}
47674768
};
47684769

4769-
auto pushfunc = [&addr, &mapMix, &nRelayNodes] {
4770+
auto pushfunc = [&addr, &mapMix, &nRelayNodes, &insecure_rand] {
47704771
for (auto mi = mapMix.begin(); mi != mapMix.end() && nRelayNodes-- > 0; ++mi)
4771-
mi->second->PushAddress(addr);
4772+
mi->second->PushAddress(addr, insecure_rand);
47724773
};
47734774

47744775
connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));
@@ -5078,14 +5079,15 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
50785079
if (fListen && !IsInitialBlockDownload())
50795080
{
50805081
CAddress addr = GetLocalAddress(&pfrom->addr, pfrom->GetLocalServices());
5082+
FastRandomContext insecure_rand;
50815083
if (addr.IsRoutable())
50825084
{
50835085
LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString());
5084-
pfrom->PushAddress(addr);
5086+
pfrom->PushAddress(addr, insecure_rand);
50855087
} else if (IsPeerAddrLocalGood(pfrom)) {
50865088
addr.SetIP(pfrom->addrLocal);
50875089
LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString());
5088-
pfrom->PushAddress(addr);
5090+
pfrom->PushAddress(addr, insecure_rand);
50895091
}
50905092
}
50915093

@@ -6008,8 +6010,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
60086010

60096011
pfrom->vAddrToSend.clear();
60106012
vector<CAddress> vAddr = connman.GetAddresses();
6013+
FastRandomContext insecure_rand;
60116014
BOOST_FOREACH(const CAddress &addr, vAddr)
6012-
pfrom->PushAddress(addr);
6015+
pfrom->PushAddress(addr, insecure_rand);
60136016
}
60146017

60156018

@@ -6842,7 +6845,7 @@ bool SendMessages(CNode* pto, CConnman& connman)
68426845
// until scheduled broadcast, then move the broadcast to within MAX_FEEFILTER_CHANGE_DELAY.
68436846
else if (timeNow + MAX_FEEFILTER_CHANGE_DELAY * 1000000 < pto->nextSendTimeFeeFilter &&
68446847
(currentFilter < 3 * pto->lastSentFeeFilter / 4 || currentFilter > 4 * pto->lastSentFeeFilter / 3)) {
6845-
pto->nextSendTimeFeeFilter = timeNow + (insecure_rand() % MAX_FEEFILTER_CHANGE_DELAY) * 1000000;
6848+
pto->nextSendTimeFeeFilter = timeNow + GetRandInt(MAX_FEEFILTER_CHANGE_DELAY) * 1000000;
68466849
}
68476850
}
68486851
}

‎src/net.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,8 @@ void AdvertiseLocal(CNode *pnode)
187187
if (addrLocal.IsRoutable())
188188
{
189189
LogPrint("net", "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
190-
pnode->PushAddress(addrLocal);
190+
FastRandomContext insecure_rand;
191+
pnode->PushAddress(addrLocal, insecure_rand);
191192
}
192193
}
193194
}

‎src/net.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -735,14 +735,14 @@ class CNode
735735
addrKnown.insert(_addr.GetKey());
736736
}
737737

738-
void PushAddress(const CAddress& _addr)
738+
void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand)
739739
{
740740
// Known checking here is only to save space from duplicates.
741741
// SendMessages will filter it again for knowns that were added
742742
// after addresses were pushed.
743743
if (_addr.IsValid() && !addrKnown.contains(_addr.GetKey())) {
744744
if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) {
745-
vAddrToSend[insecure_rand() % vAddrToSend.size()] = _addr;
745+
vAddrToSend[insecure_rand.rand32() % vAddrToSend.size()] = _addr;
746746
} else {
747747
vAddrToSend.push_back(_addr);
748748
}

‎src/netbase.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -596,8 +596,8 @@ static bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDe
596596
// do socks negotiation
597597
if (proxy.randomize_credentials) {
598598
ProxyCredentials random_auth;
599-
random_auth.username = strprintf("%i", insecure_rand());
600-
random_auth.password = strprintf("%i", insecure_rand());
599+
static std::atomic_int counter;
600+
random_auth.username = random_auth.password = strprintf("%i", counter++);
601601
if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket))
602602
return false;
603603
} else {

‎src/policy/fees.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -594,7 +594,7 @@ FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee)
594594
CAmount FeeFilterRounder::round(CAmount currentMinFee)
595595
{
596596
std::set<double>::iterator it = feeset.lower_bound(currentMinFee);
597-
if ((it != feeset.begin() && insecure_rand() % 3 != 0) || it == feeset.end()) {
597+
if ((it != feeset.begin() && insecure_rand.rand32() % 3 != 0) || it == feeset.end()) {
598598
it--;
599599
}
600600
return *it;

‎src/policy/fees.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#include "amount.h"
99
#include "uint256.h"
10+
#include "random.h"
1011

1112
#include <map>
1213
#include <string>
@@ -298,5 +299,6 @@ class FeeFilterRounder
298299

299300
private:
300301
std::set<double> feeset;
302+
FastRandomContext insecure_rand;
301303
};
302304
#endif /*BITCOIN_POLICYESTIMATOR_H */

‎src/random.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -178,22 +178,21 @@ uint256 GetRandHash()
178178
return hash;
179179
}
180180

181-
uint32_t insecure_rand_Rz = 11;
182-
uint32_t insecure_rand_Rw = 11;
183-
void seed_insecure_rand(bool fDeterministic)
181+
FastRandomContext::FastRandomContext(bool fDeterministic)
184182
{
185183
// The seed values have some unlikely fixed points which we avoid.
186184
if (fDeterministic) {
187-
insecure_rand_Rz = insecure_rand_Rw = 11;
185+
Rz = Rw = 11;
188186
} else {
189187
uint32_t tmp;
190188
do {
191189
GetRandBytes((unsigned char*)&tmp, 4);
192190
} while (tmp == 0 || tmp == 0x9068ffffU);
193-
insecure_rand_Rz = tmp;
191+
Rz = tmp;
194192
do {
195193
GetRandBytes((unsigned char*)&tmp, 4);
196194
} while (tmp == 0 || tmp == 0x464fffffU);
197-
insecure_rand_Rw = tmp;
195+
Rw = tmp;
198196
}
199197
}
198+

‎src/random.h

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,25 +28,22 @@ uint256 GetRandHash();
2828
void GetStrongRandBytes(unsigned char* buf, int num);
2929

3030
/**
31-
* Seed insecure_rand using the random pool.
32-
* @param Deterministic Use a deterministic seed
31+
* Fast randomness source. This is seeded once with secure random data, but
32+
* is completely deterministic and insecure after that.
33+
* This class is not thread-safe.
3334
*/
34-
void seed_insecure_rand(bool fDeterministic = false);
35-
36-
/**
37-
* MWC RNG of George Marsaglia
38-
* This is intended to be fast. It has a period of 2^59.3, though the
39-
* least significant 16 bits only have a period of about 2^30.1.
40-
*
41-
* @return random value
42-
*/
43-
extern uint32_t insecure_rand_Rz;
44-
extern uint32_t insecure_rand_Rw;
45-
static inline uint32_t insecure_rand(void)
46-
{
47-
insecure_rand_Rz = 36969 * (insecure_rand_Rz & 65535) + (insecure_rand_Rz >> 16);
48-
insecure_rand_Rw = 18000 * (insecure_rand_Rw & 65535) + (insecure_rand_Rw >> 16);
49-
return (insecure_rand_Rw << 16) + insecure_rand_Rz;
50-
}
35+
class FastRandomContext {
36+
public:
37+
explicit FastRandomContext(bool fDeterministic=false);
38+
39+
uint32_t rand32() {
40+
Rz = 36969 * (Rz & 65535) + (Rz >> 16);
41+
Rw = 18000 * (Rw & 65535) + (Rw >> 16);
42+
return (Rw << 16) + Rz;
43+
}
44+
45+
uint32_t Rz;
46+
uint32_t Rw;
47+
};
5148

5249
#endif // BITCOIN_RANDOM_H

0 commit comments

Comments
 (0)