Skip to content

Commit 378fdfa

Browse files
committed
Merge #14624: Some simple improvements to the RNG code
e414486 Do not permit copying FastRandomContexts (Pieter Wuille) 022cf47 Simplify testing RNG code (Pieter Wuille) fd3e797 Make unit tests use the insecure_rand_ctx exclusively (Pieter Wuille) 8d98d42 Bugfix: randbytes should seed when needed (non reachable issue) (Pieter Wuille) 273d025 Use a FastRandomContext in LimitOrphanTxSize (Pieter Wuille) 3db746b Introduce a Shuffle for FastRandomContext and use it in wallet and coinselection (Pieter Wuille) 8098379 Use a local FastRandomContext in a few more places in net (Pieter Wuille) 9695f31 Make addrman use its local RNG exclusively (Pieter Wuille) Pull request description: This improves a few minor issues with the RNG code: * Avoid calling `GetRand*()` functions (which currently invoke OpenSSL, later may switch to using our own RNG pool) inside loops in addrman, networking code, `KnapsackSolver`, and `LimitOrphanSize` * Fix a currently unreachable bug in `FastRandomContext::randbytes`. * Make a number of simplifications to the unit tests' randomness code (some tests unnecessarily used their own RNG or the OpenSSL one, instead of using the unit test specific `insecure_rand_ctx`). * As a precaution, make it illegal to copy a `FastRandomContext`. Tree-SHA512: 084c70b533ea68ca7adc0186c39f0b3e0a5c0ae43a12c37286e5d42086e056a8cd026dde61b12c0a296dc80f87fdc87fe303b9e8e6161b460ac2086cf7615f9d
2 parents 24b3b78 + e414486 commit 378fdfa

16 files changed

+134
-82
lines changed

src/addrman.cpp

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ void CAddrMan::Good_(const CService& addr, bool test_before_evict, int64_t nTime
217217
return;
218218

219219
// find a bucket it is in now
220-
int nRnd = RandomInt(ADDRMAN_NEW_BUCKET_COUNT);
220+
int nRnd = insecure_rand.randrange(ADDRMAN_NEW_BUCKET_COUNT);
221221
int nUBucket = -1;
222222
for (unsigned int n = 0; n < ADDRMAN_NEW_BUCKET_COUNT; n++) {
223223
int nB = (n + nRnd) % ADDRMAN_NEW_BUCKET_COUNT;
@@ -291,7 +291,7 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP
291291
int nFactor = 1;
292292
for (int n = 0; n < pinfo->nRefCount; n++)
293293
nFactor *= 2;
294-
if (nFactor > 1 && (RandomInt(nFactor) != 0))
294+
if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0))
295295
return false;
296296
} else {
297297
pinfo = Create(addr, source, &nId);
@@ -356,37 +356,37 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
356356

357357
// Use a 50% chance for choosing between tried and new table entries.
358358
if (!newOnly &&
359-
(nTried > 0 && (nNew == 0 || RandomInt(2) == 0))) {
359+
(nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) {
360360
// use a tried node
361361
double fChanceFactor = 1.0;
362362
while (1) {
363-
int nKBucket = RandomInt(ADDRMAN_TRIED_BUCKET_COUNT);
364-
int nKBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE);
363+
int nKBucket = insecure_rand.randrange(ADDRMAN_TRIED_BUCKET_COUNT);
364+
int nKBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
365365
while (vvTried[nKBucket][nKBucketPos] == -1) {
366366
nKBucket = (nKBucket + insecure_rand.randbits(ADDRMAN_TRIED_BUCKET_COUNT_LOG2)) % ADDRMAN_TRIED_BUCKET_COUNT;
367367
nKBucketPos = (nKBucketPos + insecure_rand.randbits(ADDRMAN_BUCKET_SIZE_LOG2)) % ADDRMAN_BUCKET_SIZE;
368368
}
369369
int nId = vvTried[nKBucket][nKBucketPos];
370370
assert(mapInfo.count(nId) == 1);
371371
CAddrInfo& info = mapInfo[nId];
372-
if (RandomInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30))
372+
if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30))
373373
return info;
374374
fChanceFactor *= 1.2;
375375
}
376376
} else {
377377
// use a new node
378378
double fChanceFactor = 1.0;
379379
while (1) {
380-
int nUBucket = RandomInt(ADDRMAN_NEW_BUCKET_COUNT);
381-
int nUBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE);
380+
int nUBucket = insecure_rand.randrange(ADDRMAN_NEW_BUCKET_COUNT);
381+
int nUBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE);
382382
while (vvNew[nUBucket][nUBucketPos] == -1) {
383383
nUBucket = (nUBucket + insecure_rand.randbits(ADDRMAN_NEW_BUCKET_COUNT_LOG2)) % ADDRMAN_NEW_BUCKET_COUNT;
384384
nUBucketPos = (nUBucketPos + insecure_rand.randbits(ADDRMAN_BUCKET_SIZE_LOG2)) % ADDRMAN_BUCKET_SIZE;
385385
}
386386
int nId = vvNew[nUBucket][nUBucketPos];
387387
assert(mapInfo.count(nId) == 1);
388388
CAddrInfo& info = mapInfo[nId];
389-
if (RandomInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30))
389+
if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30))
390390
return info;
391391
fChanceFactor *= 1.2;
392392
}
@@ -482,7 +482,7 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr)
482482
if (vAddr.size() >= nNodes)
483483
break;
484484

485-
int nRndPos = RandomInt(vRandom.size() - n) + n;
485+
int nRndPos = insecure_rand.randrange(vRandom.size() - n) + n;
486486
SwapRandom(n, nRndPos);
487487
assert(mapInfo.count(vRandom[n]) == 1);
488488

@@ -530,10 +530,6 @@ void CAddrMan::SetServices_(const CService& addr, ServiceFlags nServices)
530530
info.nServices = nServices;
531531
}
532532

533-
int CAddrMan::RandomInt(int nMax){
534-
return GetRandInt(nMax);
535-
}
536-
537533
void CAddrMan::ResolveCollisions_()
538534
{
539535
for (std::set<int>::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) {
@@ -593,7 +589,7 @@ CAddrInfo CAddrMan::SelectTriedCollision_()
593589
std::set<int>::iterator it = m_tried_collisions.begin();
594590

595591
// Selects a random element from m_tried_collisions
596-
std::advance(it, GetRandInt(m_tried_collisions.size()));
592+
std::advance(it, insecure_rand.randrange(m_tried_collisions.size()));
597593
int id_new = *it;
598594

599595
// If id_new not found in mapInfo remove it from m_tried_collisions

src/addrman.h

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -266,9 +266,6 @@ class CAddrMan
266266
//! Return a random to-be-evicted tried table address.
267267
CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
268268

269-
//! Wraps GetRandInt to allow tests to override RandomInt and make it determinismistic.
270-
virtual int RandomInt(int nMax);
271-
272269
#ifdef DEBUG_ADDRMAN
273270
//! Perform consistency check. Returns an error code or zero.
274271
int Check_() EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -473,7 +470,7 @@ class CAddrMan
473470
{
474471
LOCK(cs);
475472
std::vector<int>().swap(vRandom);
476-
nKey = GetRandHash();
473+
nKey = insecure_rand.rand256();
477474
for (size_t bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; bucket++) {
478475
for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; entry++) {
479476
vvNew[bucket][entry] = -1;

src/net.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,11 +134,12 @@ static std::vector<CAddress> convertSeed6(const std::vector<SeedSpec6> &vSeedsIn
134134
const int64_t nOneWeek = 7*24*60*60;
135135
std::vector<CAddress> vSeedsOut;
136136
vSeedsOut.reserve(vSeedsIn.size());
137+
FastRandomContext rng;
137138
for (const auto& seed_in : vSeedsIn) {
138139
struct in6_addr ip;
139140
memcpy(&ip, seed_in.addr, sizeof(ip));
140141
CAddress addr(CService(ip, seed_in.port), GetDesirableServiceFlags(NODE_NONE));
141-
addr.nTime = GetTime() - GetRand(nOneWeek) - nOneWeek;
142+
addr.nTime = GetTime() - rng.randrange(nOneWeek) - nOneWeek;
142143
vSeedsOut.push_back(addr);
143144
}
144145
return vSeedsOut;
@@ -189,16 +190,16 @@ void AdvertiseLocal(CNode *pnode)
189190
// If discovery is enabled, sometimes give our peer the address it
190191
// tells us that it sees us as in case it has a better idea of our
191192
// address than we do.
193+
FastRandomContext rng;
192194
if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() ||
193-
GetRand((GetnScore(addrLocal) > LOCAL_MANUAL) ? 8:2) == 0))
195+
rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0))
194196
{
195197
addrLocal.SetIP(pnode->GetAddrLocal());
196198
}
197199
if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
198200
{
199201
LogPrint(BCLog::NET, "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
200-
FastRandomContext insecure_rand;
201-
pnode->PushAddress(addrLocal, insecure_rand);
202+
pnode->PushAddress(addrLocal, rng);
202203
}
203204
}
204205
}

src/net_processing.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -779,10 +779,11 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
779779
nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL;
780780
if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nErased);
781781
}
782+
FastRandomContext rng;
782783
while (mapOrphanTransactions.size() > nMaxOrphans)
783784
{
784785
// Evict a random orphan:
785-
uint256 randomhash = GetRandHash();
786+
uint256 randomhash = rng.rand256();
786787
std::map<uint256, COrphanTx>::iterator it = mapOrphanTransactions.lower_bound(randomhash);
787788
if (it == mapOrphanTransactions.end())
788789
it = mapOrphanTransactions.begin();

src/random.cpp

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,7 @@ uint256 FastRandomContext::rand256()
398398

399399
std::vector<unsigned char> FastRandomContext::randbytes(size_t len)
400400
{
401+
if (requires_seed) RandomSeed();
401402
std::vector<unsigned char> ret(len);
402403
if (len > 0) {
403404
rng.Output(&ret[0], len);
@@ -463,6 +464,20 @@ FastRandomContext::FastRandomContext(bool fDeterministic) : requires_seed(!fDete
463464
rng.SetKey(seed.begin(), 32);
464465
}
465466

467+
FastRandomContext& FastRandomContext::operator=(FastRandomContext&& from) noexcept
468+
{
469+
requires_seed = from.requires_seed;
470+
rng = from.rng;
471+
std::copy(std::begin(from.bytebuf), std::end(from.bytebuf), std::begin(bytebuf));
472+
bytebuf_size = from.bytebuf_size;
473+
bitbuf = from.bitbuf;
474+
bitbuf_size = from.bitbuf_size;
475+
from.requires_seed = true;
476+
from.bytebuf_size = 0;
477+
from.bitbuf_size = 0;
478+
return *this;
479+
}
480+
466481
void RandomInit()
467482
{
468483
RDRandInit();

src/random.h

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,14 @@ class FastRandomContext {
7676
/** Initialize with explicit seed (only for testing) */
7777
explicit FastRandomContext(const uint256& seed);
7878

79+
// Do not permit copying a FastRandomContext (move it, or create a new one to get reseeded).
80+
FastRandomContext(const FastRandomContext&) = delete;
81+
FastRandomContext(FastRandomContext&&) = delete;
82+
FastRandomContext& operator=(const FastRandomContext&) = delete;
83+
84+
/** Move a FastRandomContext. If the original one is used again, it will be reseeded. */
85+
FastRandomContext& operator=(FastRandomContext&& from) noexcept;
86+
7987
/** Generate a random 64-bit integer. */
8088
uint64_t rand64()
8189
{
@@ -130,6 +138,29 @@ class FastRandomContext {
130138
inline uint64_t operator()() { return rand64(); }
131139
};
132140

141+
/** More efficient than using std::shuffle on a FastRandomContext.
142+
*
143+
* This is more efficient as std::shuffle will consume entropy in groups of
144+
* 64 bits at the time and throw away most.
145+
*
146+
* This also works around a bug in libstdc++ std::shuffle that may cause
147+
* type::operator=(type&&) to be invoked on itself, which the library's
148+
* debug mode detects and panics on. This is a known issue, see
149+
* https://stackoverflow.com/questions/22915325/avoiding-self-assignment-in-stdshuffle
150+
*/
151+
template<typename I, typename R>
152+
void Shuffle(I first, I last, R&& rng)
153+
{
154+
while (first != last) {
155+
size_t j = rng.randrange(last - first);
156+
if (j) {
157+
using std::swap;
158+
swap(*first, *(first + j));
159+
}
160+
++first;
161+
}
162+
}
163+
133164
/* Number of random bytes returned by GetOSRand.
134165
* When changing this constant make sure to change all call sites, and make
135166
* sure that the underlying OS APIs for all platforms support the number.

src/test/addrman_tests.cpp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,6 @@ class CAddrManTest : public CAddrMan
3232
insecure_rand = FastRandomContext(true);
3333
}
3434

35-
int RandomInt(int nMax) override
36-
{
37-
state = (CHashWriter(SER_GETHASH, 0) << state).GetCheapHash();
38-
return (unsigned int)(state % nMax);
39-
}
40-
4135
CAddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr)
4236
{
4337
LOCK(cs);

src/test/cuckoocache_tests.cpp

Lines changed: 11 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -21,40 +21,23 @@
2121
* using BOOST_CHECK_CLOSE to fail.
2222
*
2323
*/
24-
FastRandomContext local_rand_ctx(true);
25-
2624
BOOST_AUTO_TEST_SUITE(cuckoocache_tests);
2725

28-
29-
/** insecure_GetRandHash fills in a uint256 from local_rand_ctx
30-
*/
31-
static void insecure_GetRandHash(uint256& t)
32-
{
33-
uint32_t* ptr = (uint32_t*)t.begin();
34-
for (uint8_t j = 0; j < 8; ++j)
35-
*(ptr++) = local_rand_ctx.rand32();
36-
}
37-
38-
39-
4026
/* Test that no values not inserted into the cache are read out of it.
4127
*
4228
* There are no repeats in the first 200000 insecure_GetRandHash calls
4329
*/
4430
BOOST_AUTO_TEST_CASE(test_cuckoocache_no_fakes)
4531
{
46-
local_rand_ctx = FastRandomContext(true);
32+
SeedInsecureRand(true);
4733
CuckooCache::cache<uint256, SignatureCacheHasher> cc{};
4834
size_t megabytes = 4;
4935
cc.setup_bytes(megabytes << 20);
50-
uint256 v;
5136
for (int x = 0; x < 100000; ++x) {
52-
insecure_GetRandHash(v);
53-
cc.insert(v);
37+
cc.insert(InsecureRand256());
5438
}
5539
for (int x = 0; x < 100000; ++x) {
56-
insecure_GetRandHash(v);
57-
BOOST_CHECK(!cc.contains(v, false));
40+
BOOST_CHECK(!cc.contains(InsecureRand256(), false));
5841
}
5942
};
6043

@@ -64,7 +47,7 @@ BOOST_AUTO_TEST_CASE(test_cuckoocache_no_fakes)
6447
template <typename Cache>
6548
static double test_cache(size_t megabytes, double load)
6649
{
67-
local_rand_ctx = FastRandomContext(true);
50+
SeedInsecureRand(true);
6851
std::vector<uint256> hashes;
6952
Cache set{};
7053
size_t bytes = megabytes * (1 << 20);
@@ -74,7 +57,7 @@ static double test_cache(size_t megabytes, double load)
7457
for (uint32_t i = 0; i < n_insert; ++i) {
7558
uint32_t* ptr = (uint32_t*)hashes[i].begin();
7659
for (uint8_t j = 0; j < 8; ++j)
77-
*(ptr++) = local_rand_ctx.rand32();
60+
*(ptr++) = InsecureRand32();
7861
}
7962
/** We make a copy of the hashes because future optimizations of the
8063
* cuckoocache may overwrite the inserted element, so the test is
@@ -135,7 +118,7 @@ template <typename Cache>
135118
static void test_cache_erase(size_t megabytes)
136119
{
137120
double load = 1;
138-
local_rand_ctx = FastRandomContext(true);
121+
SeedInsecureRand(true);
139122
std::vector<uint256> hashes;
140123
Cache set{};
141124
size_t bytes = megabytes * (1 << 20);
@@ -145,7 +128,7 @@ static void test_cache_erase(size_t megabytes)
145128
for (uint32_t i = 0; i < n_insert; ++i) {
146129
uint32_t* ptr = (uint32_t*)hashes[i].begin();
147130
for (uint8_t j = 0; j < 8; ++j)
148-
*(ptr++) = local_rand_ctx.rand32();
131+
*(ptr++) = InsecureRand32();
149132
}
150133
/** We make a copy of the hashes because future optimizations of the
151134
* cuckoocache may overwrite the inserted element, so the test is
@@ -198,7 +181,7 @@ template <typename Cache>
198181
static void test_cache_erase_parallel(size_t megabytes)
199182
{
200183
double load = 1;
201-
local_rand_ctx = FastRandomContext(true);
184+
SeedInsecureRand(true);
202185
std::vector<uint256> hashes;
203186
Cache set{};
204187
size_t bytes = megabytes * (1 << 20);
@@ -208,7 +191,7 @@ static void test_cache_erase_parallel(size_t megabytes)
208191
for (uint32_t i = 0; i < n_insert; ++i) {
209192
uint32_t* ptr = (uint32_t*)hashes[i].begin();
210193
for (uint8_t j = 0; j < 8; ++j)
211-
*(ptr++) = local_rand_ctx.rand32();
194+
*(ptr++) = InsecureRand32();
212195
}
213196
/** We make a copy of the hashes because future optimizations of the
214197
* cuckoocache may overwrite the inserted element, so the test is
@@ -300,7 +283,7 @@ static void test_cache_generations()
300283
// iterations with non-deterministic values, so it isn't "overfit" to the
301284
// specific entropy in FastRandomContext(true) and implementation of the
302285
// cache.
303-
local_rand_ctx = FastRandomContext(true);
286+
SeedInsecureRand(true);
304287

305288
// block_activity models a chunk of network activity. n_insert elements are
306289
// added to the cache. The first and last n/4 are stored for removal later
@@ -317,7 +300,7 @@ static void test_cache_generations()
317300
for (uint32_t i = 0; i < n_insert; ++i) {
318301
uint32_t* ptr = (uint32_t*)inserts[i].begin();
319302
for (uint8_t j = 0; j < 8; ++j)
320-
*(ptr++) = local_rand_ctx.rand32();
303+
*(ptr++) = InsecureRand32();
321304
}
322305
for (uint32_t i = 0; i < n_insert / 4; ++i)
323306
reads.push_back(inserts[i]);

src/test/denialofservice_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
111111

112112
static void AddRandomOutboundPeer(std::vector<CNode *> &vNodes, PeerLogicValidation &peerLogic)
113113
{
114-
CAddress addr(ip(GetRandInt(0xffffffff)), NODE_NONE);
114+
CAddress addr(ip(insecure_rand_ctx.randbits(32)), NODE_NONE);
115115
vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK|NODE_WITNESS), 0, INVALID_SOCKET, addr, 0, 0, CAddress(), "", /*fInboundIn=*/ false));
116116
CNode &node = *vNodes.back();
117117
node.SetSendVersion(PROTOCOL_VERSION);

src/test/prevector_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,8 @@ class prevector_tester {
189189

190190
prevector_tester() {
191191
SeedInsecureRand();
192-
rand_seed = insecure_rand_seed;
193-
rand_cache = insecure_rand_ctx;
192+
rand_seed = InsecureRand256();
193+
rand_cache = FastRandomContext(rand_seed);
194194
}
195195
};
196196

0 commit comments

Comments
 (0)