From b363cc93af8deb7b5d881d6c83c27cc8575c6fed Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Mon, 30 Oct 2023 16:40:16 +0000 Subject: [PATCH] Fix wrong random using (#955) Fixes #855 --- CMakeLists.txt | 1 + src/etl/LoadBalancer.cpp | 13 ++++--- src/util/Random.cpp | 27 ++++++++++++++ src/util/Random.h | 44 +++++++++++++++++++++++ unittests/data/cassandra/BackendTests.cpp | 20 +++++------ 5 files changed, 89 insertions(+), 16 deletions(-) create mode 100644 src/util/Random.cpp create mode 100644 src/util/Random.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 7a035ad2..a624108d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -137,6 +137,7 @@ target_sources (clio PRIVATE ## Util src/util/config/Config.cpp src/util/log/Logger.cpp + src/util/Random.cpp src/util/Taggable.cpp) # Clio server diff --git a/src/etl/LoadBalancer.cpp b/src/etl/LoadBalancer.cpp index 17ffdaac..4229ece6 100644 --- a/src/etl/LoadBalancer.cpp +++ b/src/etl/LoadBalancer.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -184,8 +185,10 @@ LoadBalancer::forwardToRippled( boost::asio::yield_context yield ) const { - srand(static_cast(time(0))); - auto sourceIdx = rand() % sources_.size(); + std::size_t sourceIdx = 0; + if (!sources_.empty()) + sourceIdx = util::Random::uniform(0ul, sources_.size()); + auto numAttempts = 0u; while (numAttempts < sources_.size()) { @@ -228,8 +231,10 @@ template bool LoadBalancer::execute(Func f, uint32_t ledgerSequence) { - srand(static_cast(time(0))); - auto sourceIdx = rand() % sources_.size(); + std::size_t sourceIdx = 0; + if (!sources_.empty()) + sourceIdx = util::Random::uniform(0ul, sources_.size()); + auto numAttempts = 0; while (true) { diff --git a/src/util/Random.cpp b/src/util/Random.cpp new file mode 100644 index 00000000..22079353 --- /dev/null +++ b/src/util/Random.cpp @@ -0,0 +1,27 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2023, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== +#include + +#include + +namespace util { + +std::mt19937_64 Random::generator_{std::chrono::system_clock::now().time_since_epoch().count()}; + +} // namespace util diff --git a/src/util/Random.h b/src/util/Random.h new file mode 100644 index 00000000..ea8fe308 --- /dev/null +++ b/src/util/Random.h @@ -0,0 +1,44 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2023, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== +#pragma once + +#include + +namespace util { + +class Random { +public: + template + static T + uniform(T min, T max) + { + assert(min <= max); + if constexpr (std::is_floating_point_v) { + std::uniform_real_distribution distribution(min, max); + return distribution(generator_); + } + std::uniform_int_distribution distribution(min, max); + return distribution(generator_); + } + +private: + static std::mt19937_64 generator_; +}; + +} // namespace util diff --git a/unittests/data/cassandra/BackendTests.cpp b/unittests/data/cassandra/BackendTests.cpp index 8e2c680b..5b952f62 100644 --- a/unittests/data/cassandra/BackendTests.cpp +++ b/unittests/data/cassandra/BackendTests.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -73,6 +74,8 @@ protected: EXPECT_TRUE(handle.connect()); handle.execute("DROP KEYSPACE " + std::string{keyspace}); } + + std::default_random_engine randomEngine{0}; }; TEST_F(BackendCassandraTest, Basic) @@ -437,8 +440,6 @@ TEST_F(BackendCassandraTest, Basic) EXPECT_FALSE(obj); } - // obtain a time-based seed: - auto const seed = std::chrono::system_clock::now().time_since_epoch().count(); std::string accountBlobOld = accountBlob; { lgrInfoNext.seq = lgrInfoNext.seq + 1; @@ -448,7 +449,7 @@ TEST_F(BackendCassandraTest, Basic) lgrInfoNext.accountHash = ~(lgrInfoNext.accountHash ^ lgrInfoNext.txHash); backend->writeLedger(lgrInfoNext, ledgerInfoToBinaryString(lgrInfoNext)); - std::shuffle(accountBlob.begin(), accountBlob.end(), std::default_random_engine(seed)); + std::shuffle(accountBlob.begin(), accountBlob.end(), randomEngine); backend->writeLedgerObject(std::string{accountIndexBlob}, lgrInfoNext.seq, std::string{accountBlob}); ASSERT_TRUE(backend->finishWrites(lgrInfoNext.seq)); @@ -560,7 +561,6 @@ TEST_F(BackendCassandraTest, Basic) auto generateAccountTx = [&](uint32_t ledgerSequence, auto txns) { std::vector ret; auto accounts = generateAccounts(ledgerSequence, 10); - std::srand(std::time(nullptr)); uint32_t idx = 0; for (auto& [hash, txn, meta] : txns) { AccountTransactionsData data; @@ -568,17 +568,16 @@ TEST_F(BackendCassandraTest, Basic) data.transactionIndex = idx; data.txHash = hash; for (size_t i = 0; i < 3; ++i) { - data.accounts.insert(accounts[std::rand() % accounts.size()]); + data.accounts.insert(accounts[util::Random::uniform(0ul, accounts.size() - 1)]); } ++idx; ret.push_back(data); } return ret; }; - auto generateNextLedger = [seed](auto lgrInfo) { + auto generateNextLedger = [this](auto lgrInfo) { ++lgrInfo.seq; lgrInfo.parentHash = lgrInfo.hash; - static auto randomEngine = std::default_random_engine(seed); std::shuffle(lgrInfo.txHash.begin(), lgrInfo.txHash.end(), randomEngine); std::shuffle(lgrInfo.accountHash.begin(), lgrInfo.accountHash.end(), randomEngine); std::shuffle(lgrInfo.hash.begin(), lgrInfo.hash.end(), randomEngine); @@ -967,8 +966,6 @@ TEST_F(BackendCassandraTest, CacheIntegration) obj = backend->fetchLedgerObject(key256, lgrInfoOld.seq - 1, yield); EXPECT_FALSE(obj); } - // obtain a time-based seed: - unsigned const seed = std::chrono::system_clock::now().time_since_epoch().count(); std::string accountBlobOld = accountBlob; { backend->startWrites(); @@ -979,7 +976,7 @@ TEST_F(BackendCassandraTest, CacheIntegration) lgrInfoNext.accountHash = ~(lgrInfoNext.accountHash ^ lgrInfoNext.txHash); backend->writeLedger(lgrInfoNext, ledgerInfoToBinaryString(lgrInfoNext)); - std::shuffle(accountBlob.begin(), accountBlob.end(), std::default_random_engine(seed)); + std::shuffle(accountBlob.begin(), accountBlob.end(), randomEngine); auto key = ripple::uint256::fromVoidChecked(accountIndexBlob); backend->cache().update({{*key, {accountBlob.begin(), accountBlob.end()}}}, lgrInfoNext.seq); backend->writeLedgerObject(std::string{accountIndexBlob}, lgrInfoNext.seq, std::string{accountBlob}); @@ -1065,10 +1062,9 @@ TEST_F(BackendCassandraTest, CacheIntegration) return objs; }; - auto generateNextLedger = [seed](auto lgrInfo) { + auto generateNextLedger = [this](auto lgrInfo) { ++lgrInfo.seq; lgrInfo.parentHash = lgrInfo.hash; - static auto randomEngine = std::default_random_engine(seed); std::shuffle(lgrInfo.txHash.begin(), lgrInfo.txHash.end(), randomEngine); std::shuffle(lgrInfo.accountHash.begin(), lgrInfo.accountHash.end(), randomEngine); std::shuffle(lgrInfo.hash.begin(), lgrInfo.hash.end(), randomEngine);