From aa6749a65d5e0f90d8a56c2d0a71c4bab856716b Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 21 May 2024 16:01:13 -0700 Subject: [PATCH 1/3] Make gRandomEngine thread_local --- src/util/Math.cpp | 44 +++++++++++++++++++++++++++++++++++--------- src/util/Math.h | 5 +++-- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/util/Math.cpp b/src/util/Math.cpp index 91f29cd808..e4931f40b4 100644 --- a/src/util/Math.cpp +++ b/src/util/Math.cpp @@ -12,13 +12,15 @@ #include #include #include +#include #include #include namespace stellar { -stellar_default_random_engine gRandomEngine; +thread_local stellar_default_random_engine + gRandomEngine(getLastGlobalStateSeed()); std::uniform_real_distribution uniformFractionDistribution(0.0, 1.0); double @@ -169,14 +171,38 @@ k_means(std::vector const& points, uint32_t k) return centroids; } +static std::mutex gGlobalSeedMutex; static unsigned int lastGlobalSeed{0}; + +unsigned int +getLastGlobalStateSeed() +{ + std::lock_guard guard(gGlobalSeedMutex); + return lastGlobalSeed; +} + static void reinitializeAllGlobalStateWithSeedInternal(unsigned int seed) { - lastGlobalSeed = seed; + { + std::lock_guard guard(gGlobalSeedMutex); + lastGlobalSeed = seed; + } PubKeyUtils::clearVerifySigCache(); srand(seed); + + // gRandomEngine is a thread_local, and is initialized / initially seeded + // with the result of calling getLastGlobalStateSeed(). This means that the + // main thread's gRandomEngine will be initialized with the initial value of + // lastGlobalSeed: 0. So we _reseed_ it here. But other non-main threads + // will initialize their thread_local gRandomEngine instances as the threads + // are launched, which should happen _after_ we've set lastGlobalSeed to + // some nontrivial value (either test-provided, user-provded or from the + // system PRNG), so they will pick up that nontrivial value and we don't + // need to "reseed" them explicitly the same way. + assert(threadIsMain()); gRandomEngine.seed(seed); + randHash::initialize(); } @@ -184,8 +210,13 @@ void initializeAllGlobalState() { releaseAssert(lastGlobalSeed == 0); - auto const seed = static_cast( - std::chrono::system_clock::now().time_since_epoch().count()); + // libstdc++ and libc++ accept and use "/dev/urandom" as the token + // identifying the system _nonblocking_ random number generator: we're not + // after strong cryptographic randomness here, just nonblocking best-effort. + // + // MSVC ignores this parameter and calls into the RtlGenRandom / + // CryptGenRandom complex (depending on Windows version) to get the seed. + auto const seed = std::random_device("/dev/urandom")(); reinitializeAllGlobalStateWithSeedInternal(seed); // shortHash needs to be initialized with a strong random seed shortHash::initialize(); @@ -203,10 +234,5 @@ reinitializeAllGlobalStateWithSeed(unsigned int seed) autocheck::rng().seed(seed); } -unsigned int -getLastGlobalStateSeed() -{ - return lastGlobalSeed; -} #endif } diff --git a/src/util/Math.h b/src/util/Math.h index 0f6b3eabef..b884080df2 100644 --- a/src/util/Math.h +++ b/src/util/Math.h @@ -25,7 +25,7 @@ bool rand_flip(); typedef std::minstd_rand stellar_default_random_engine; -extern stellar_default_random_engine gRandomEngine; +extern thread_local stellar_default_random_engine gRandomEngine; template T @@ -76,7 +76,8 @@ void initializeAllGlobalState(); // shouldn't be resetting globals like this mid-run -- especially not things // like hash function keys. void reinitializeAllGlobalStateWithSeed(unsigned int seed); -unsigned int getLastGlobalStateSeed(); #endif +unsigned int getLastGlobalStateSeed(); + } From 18ad150e4c61a89e861ef5372f0271a6ca8d6b19 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 21 May 2024 16:01:40 -0700 Subject: [PATCH 2/3] Make gVerifySigCache thread_local --- src/crypto/SecretKey.cpp | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/crypto/SecretKey.cpp b/src/crypto/SecretKey.cpp index 45377eb627..3381469393 100644 --- a/src/crypto/SecretKey.cpp +++ b/src/crypto/SecretKey.cpp @@ -41,10 +41,9 @@ namespace stellar // makes all signature-verification in the program faster and // has no effect on correctness. -static std::mutex gVerifySigCacheMutex; -static RandomEvictionCache gVerifySigCache(0xffff); -static uint64_t gVerifyCacheHit = 0; -static uint64_t gVerifyCacheMiss = 0; +static thread_local RandomEvictionCache gVerifySigCache(0xffff); +static thread_local uint64_t gVerifyCacheHit = 0; +static thread_local uint64_t gVerifyCacheMiss = 0; static Hash verifySigCacheKey(PublicKey const& key, Signature const& signature, @@ -316,14 +315,12 @@ SecretKey::fromStrKeySeed(std::string const& strKeySeed) void PubKeyUtils::clearVerifySigCache() { - std::lock_guard guard(gVerifySigCacheMutex); gVerifySigCache.clear(); } void PubKeyUtils::flushVerifySigCacheCounts(uint64_t& hits, uint64_t& misses) { - std::lock_guard guard(gVerifySigCacheMutex); hits = gVerifyCacheHit; misses = gVerifyCacheMiss; gVerifyCacheHit = 0; @@ -438,7 +435,6 @@ PubKeyUtils::verifySig(PublicKey const& key, Signature const& signature, auto cacheKey = verifySigCacheKey(key, signature, bin); { - std::lock_guard guard(gVerifySigCacheMutex); if (gVerifySigCache.exists(cacheKey)) { ++gVerifyCacheHit; @@ -453,7 +449,6 @@ PubKeyUtils::verifySig(PublicKey const& key, Signature const& signature, bool ok = (crypto_sign_verify_detached(signature.data(), bin.data(), bin.size(), key.ed25519().data()) == 0); - std::lock_guard guard(gVerifySigCacheMutex); ++gVerifyCacheMiss; gVerifySigCache.put(cacheKey, ok); return ok; From 59e6e071473b37f76cf83625063a461f72025a63 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 21 May 2024 17:22:30 -0700 Subject: [PATCH 3/3] Remove separate PRNG from QIC since PRNG is now TLS --- src/herder/HerderImpl.cpp | 10 ++--- src/herder/QuorumIntersectionChecker.h | 27 ++++++------ src/herder/QuorumIntersectionCheckerImpl.cpp | 43 ++++++++------------ src/herder/QuorumIntersectionCheckerImpl.h | 7 +--- src/herder/test/QuorumIntersectionTests.cpp | 16 ++++---- 5 files changed, 43 insertions(+), 60 deletions(-) diff --git a/src/herder/HerderImpl.cpp b/src/herder/HerderImpl.cpp index c4a475cd8f..f75fbb13d2 100644 --- a/src/herder/HerderImpl.cpp +++ b/src/herder/HerderImpl.cpp @@ -1788,15 +1788,13 @@ HerderImpl::checkAndMaybeReanalyzeQuorumMap() mLastQuorumMapIntersectionState.mCheckingQuorumMapHash = curr; auto& cfg = mApp.getConfig(); releaseAssert(threadIsMain()); - auto seed = gRandomEngine(); auto qic = QuorumIntersectionChecker::create( - qmap, cfg, mLastQuorumMapIntersectionState.mInterruptFlag, seed); + qmap, cfg, mLastQuorumMapIntersectionState.mInterruptFlag); auto ledger = trackingConsensusLedgerIndex(); auto nNodes = qmap.size(); auto& hState = mLastQuorumMapIntersectionState; auto& app = mApp; - auto worker = [curr, ledger, nNodes, qic, qmap, cfg, seed, &app, - &hState] { + auto worker = [curr, ledger, nNodes, qic, qmap, cfg, &app, &hState] { try { ZoneScoped; @@ -1809,8 +1807,8 @@ HerderImpl::checkAndMaybeReanalyzeQuorumMap() // intersecting; if not intersecting we should finish ASAP // and raise an alarm. critical = QuorumIntersectionChecker:: - getIntersectionCriticalGroups( - qmap, cfg, hState.mInterruptFlag, seed); + getIntersectionCriticalGroups(qmap, cfg, + hState.mInterruptFlag); } app.postOnMainThread( [ok, curr, ledger, nNodes, split, critical, &hState] { diff --git a/src/herder/QuorumIntersectionChecker.h b/src/herder/QuorumIntersectionChecker.h index d2e486acdd..387baa1c84 100644 --- a/src/herder/QuorumIntersectionChecker.h +++ b/src/herder/QuorumIntersectionChecker.h @@ -23,24 +23,21 @@ class QuorumIntersectionChecker static std::shared_ptr create(QuorumTracker::QuorumMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed, bool quiet = false); + std::atomic& interruptFlag, bool quiet = false); static std::shared_ptr create(QuorumSetMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed, bool quiet = false); - - static std::set> getIntersectionCriticalGroups( - QuorumTracker::QuorumMap const& qmap, - std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed); - - static std::set> getIntersectionCriticalGroups( - QuorumSetMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed); + std::atomic& interruptFlag, bool quiet = false); + + static std::set> + getIntersectionCriticalGroups(QuorumTracker::QuorumMap const& qmap, + std::optional const& cfg, + std::atomic& interruptFlag); + + static std::set> + getIntersectionCriticalGroups(QuorumSetMap const& qmap, + std::optional const& cfg, + std::atomic& interruptFlag); virtual ~QuorumIntersectionChecker(){}; virtual bool networkEnjoysQuorumIntersection() const = 0; diff --git a/src/herder/QuorumIntersectionCheckerImpl.cpp b/src/herder/QuorumIntersectionCheckerImpl.cpp index 716d46cf8f..32b8ca108d 100644 --- a/src/herder/QuorumIntersectionCheckerImpl.cpp +++ b/src/herder/QuorumIntersectionCheckerImpl.cpp @@ -58,8 +58,7 @@ QBitSet::getSuccessors(BitSet const& nodes, QGraph const& inner) // Slightly tweaked variant of Lachowski's next-node function. size_t -MinQuorumEnumerator::pickSplitNode( - stellar::stellar_default_random_engine& randEngine) const +MinQuorumEnumerator::pickSplitNode() const { std::vector& inDegrees = mQic.mInDegrees; inDegrees.assign(mQic.mGraph.size(), 0); @@ -84,7 +83,7 @@ MinQuorumEnumerator::pickSplitNode( // currDegree same as existing max: replace it // only probabilistically. maxCount++; - if (rand_uniform(0, maxCount, randEngine) == 0) + if (rand_uniform(0, maxCount, gRandomEngine) == 0) { // Not switching max element with max degree. continue; @@ -237,7 +236,7 @@ MinQuorumEnumerator::anyMinQuorumHasDisjointQuorum() } // Phase two: recurse into subproblems. - size_t split = pickSplitNode(mQic.mRand); + size_t split = pickSplitNode(); if (mQic.mLogTrace) { CLOG_TRACE(SCP, "recursing into subproblems, split={}", split); @@ -269,14 +268,13 @@ MinQuorumEnumerator::anyMinQuorumHasDisjointQuorum() QuorumIntersectionCheckerImpl::QuorumIntersectionCheckerImpl( QuorumIntersectionChecker::QuorumSetMap const& qmap, std::optional const& cfg, std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed, bool quiet) + bool quiet) : mCfg(cfg) , mLogTrace(Logging::logTrace("SCP")) , mQuiet(quiet) , mTSC() , mInterruptFlag(interruptFlag) , mCachedQuorums(MAX_CACHED_QUORUMS_SIZE) - , mRand(seed) { buildGraph(qmap); // Awkwardly, the graph size is zero when we initialize mTSC. Update it @@ -817,40 +815,35 @@ toQuorumIntersectionMap(QuorumTracker::QuorumMap const& qmap) namespace stellar { std::shared_ptr -QuorumIntersectionChecker::create( - QuorumTracker::QuorumMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed, bool quiet) +QuorumIntersectionChecker::create(QuorumTracker::QuorumMap const& qmap, + std::optional const& cfg, + std::atomic& interruptFlag, bool quiet) { - return create(toQuorumIntersectionMap(qmap), cfg, interruptFlag, seed, - quiet); + return create(toQuorumIntersectionMap(qmap), cfg, interruptFlag, quiet); } std::shared_ptr -QuorumIntersectionChecker::create( - QuorumSetMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed, bool quiet) +QuorumIntersectionChecker::create(QuorumSetMap const& qmap, + std::optional const& cfg, + std::atomic& interruptFlag, bool quiet) { return std::make_shared( - qmap, cfg, interruptFlag, seed, quiet); + qmap, cfg, interruptFlag, quiet); } std::set> QuorumIntersectionChecker::getIntersectionCriticalGroups( QuorumTracker::QuorumMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed) + std::atomic& interruptFlag) { return getIntersectionCriticalGroups(toQuorumIntersectionMap(qmap), cfg, - interruptFlag, seed); + interruptFlag); } std::set> QuorumIntersectionChecker::getIntersectionCriticalGroups( QuorumSetMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar_default_random_engine::result_type seed) + std::atomic& interruptFlag) { // We're going to search for "intersection-critical" groups, by considering // each SCPQuorumSet S that (a) has no innerSets of its own and (b) occurs @@ -936,9 +929,9 @@ QuorumIntersectionChecker::getIntersectionCriticalGroups( } // Check to see if this modified config is vulnerable to splitting. - auto checker = QuorumIntersectionChecker::create(test_qmap, cfg, - interruptFlag, seed, - /*quiet=*/true); + auto checker = + QuorumIntersectionChecker::create(test_qmap, cfg, interruptFlag, + /*quiet=*/true); if (checker->networkEnjoysQuorumIntersection()) { CLOG_DEBUG(SCP, diff --git a/src/herder/QuorumIntersectionCheckerImpl.h b/src/herder/QuorumIntersectionCheckerImpl.h index 24341e17d1..637cdee61f 100644 --- a/src/herder/QuorumIntersectionCheckerImpl.h +++ b/src/herder/QuorumIntersectionCheckerImpl.h @@ -427,8 +427,7 @@ class MinQuorumEnumerator QuorumIntersectionCheckerImpl const& mQic; // Select the next node in mRemaining to split recursive cases between. - size_t - pickSplitNode(stellar::stellar_default_random_engine& randEngine) const; + size_t pickSplitNode() const; // Size limit for mCommitted beyond which we should stop scanning. size_t maxCommit() const; @@ -534,9 +533,7 @@ class QuorumIntersectionCheckerImpl : public stellar::QuorumIntersectionChecker QuorumIntersectionCheckerImpl( stellar::QuorumIntersectionChecker::QuorumSetMap const& qmap, std::optional const& cfg, - std::atomic& interruptFlag, - stellar::stellar_default_random_engine::result_type seed, - bool quiet = false); + std::atomic& interruptFlag, bool quiet = false); bool networkEnjoysQuorumIntersection() const override; std::pair, std::vector> diff --git a/src/herder/test/QuorumIntersectionTests.cpp b/src/herder/test/QuorumIntersectionTests.cpp index 284fb70ee1..483cb7ea36 100644 --- a/src/herder/test/QuorumIntersectionTests.cpp +++ b/src/herder/test/QuorumIntersectionTests.cpp @@ -882,8 +882,7 @@ TEST_CASE("quorum intersection interruption", "[herder][quorumintersection]") Config cfg(getTestConfig()); cfg = configureShortNames(cfg, orgs); std::atomic interruptFlag{false}; - auto qic = QuorumIntersectionChecker::create(qm, cfg, interruptFlag, - gRandomEngine()); + auto qic = QuorumIntersectionChecker::create(qm, cfg, interruptFlag); std::thread canceller([&interruptFlag]() { std::this_thread::sleep_for(std::chrono::milliseconds(100)); interruptFlag = true; @@ -897,9 +896,9 @@ TEST_CASE("quorum intersection interruption", "[herder][quorumintersection]") std::this_thread::sleep_for(std::chrono::milliseconds(100)); interruptFlag = true; }); - REQUIRE_THROWS_AS(qic->getIntersectionCriticalGroups(qm, cfg, interruptFlag, - gRandomEngine()), - QuorumIntersectionChecker::InterruptedException); + REQUIRE_THROWS_AS( + qic->getIntersectionCriticalGroups(qm, cfg, interruptFlag), + QuorumIntersectionChecker::InterruptedException); canceller2.join(); } @@ -982,12 +981,11 @@ TEST_CASE("quorum intersection criticality", cfg = configureShortNames(cfg, orgs); debugQmap(cfg, qm); std::atomic flag{false}; - auto qic = - QuorumIntersectionChecker::create(qm, cfg, flag, gRandomEngine()); + auto qic = QuorumIntersectionChecker::create(qm, cfg, flag); REQUIRE(qic->networkEnjoysQuorumIntersection()); - auto groups = QuorumIntersectionChecker::getIntersectionCriticalGroups( - qm, cfg, flag, gRandomEngine()); + auto groups = + QuorumIntersectionChecker::getIntersectionCriticalGroups(qm, cfg, flag); REQUIRE(groups.size() == 1); REQUIRE(groups == std::set>{{orgs[3][0]}}); }