Merge bitcoin/bitcoin#24974: refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono)

fa74e726c4 refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) (MacroFake)
fa3b3cb9b5 Expose underlying clock in CThreadInterrupt (MacroFake)

Pull request description:

  This gets rid of the `value*1000` manual conversion.

ACKs for top commit:
  naumenkogs:
    utACK fa74e726c4
  dergoegge:
    Code review ACK fa74e726c4

Tree-SHA512: 90409c05c25f0dd2f1c4dead78f707ebfd78b7d84ea4db9fcefd9c4958a1a3338ac657cd9e99eb8b47d52d4485fa3c947dce4ee1559fb56ae65878685e1ed9a3

ci/test/06_script_b.sh

@@ -51,6 +51,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
           " src/rpc/fees.cpp"\
           " src/rpc/signmessage.cpp"\
           " src/test/fuzz/txorphan.cpp"\
+          " src/threadinterrupt.cpp"\
           " src/util/bip32.cpp"\
           " src/util/bytevectorhash.cpp"\
           " src/util/error.cpp"\

src/net.cpp

@@ -85,8 +85,8 @@ static constexpr int DNSSEEDS_DELAY_PEER_THRESHOLD = 1000; // "many" vs "few" pe
 /** The default timeframe for -maxuploadtarget. 1 day. */
 static constexpr std::chrono::seconds MAX_UPLOAD_TIMEFRAME{60 * 60 * 24};
 
-// We add a random period time (0 to 1 seconds) to feeler connections to prevent synchronization.
-#define FEELER_SLEEP_WINDOW 1
+// A random time period (0 to 1 seconds) is added to feeler connections to prevent synchronization.
+static constexpr auto FEELER_SLEEP_WINDOW{1s};
 
 /** Used to pass flags to the Bind() function */
 enum BindFlags {
@@ -1568,6 +1568,7 @@ int CConnman::GetExtraBlockRelayCount() const
 void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
 {
     SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET_OPEN_CONNECTION);
+    FastRandomContext rng;
     // Connect to specific addresses
     if (!connect.empty())
     {
@@ -1821,12 +1822,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
         }
 
         if (addrConnect.IsValid()) {
-
             if (fFeeler) {
                 // Add small amount of random noise before connection to avoid synchronization.
-                int randsleep = GetRand<int>(FEELER_SLEEP_WINDOW * 1000);
-                if (!interruptNet.sleep_for(std::chrono::milliseconds(randsleep)))
+                if (!interruptNet.sleep_for(rng.rand_uniform_duration<CThreadInterrupt::Clock>(FEELER_SLEEP_WINDOW))) {
                     return;
+                }
                 LogPrint(BCLog::NET, "Making feeler connection to %s\n", addrConnect.ToString());
             }
 

src/random.h

@@ -11,6 +11,7 @@
 #include <span.h>
 #include <uint256.h>
 
+#include <cassert>
 #include <chrono>
 #include <cstdint>
 #include <limits>
@@ -236,13 +237,19 @@ public:
     template <typename Tp>
     Tp rand_uniform_delay(const Tp& time, typename Tp::duration range)
     {
-        using Dur = typename Tp::duration;
-        Dur dur{range.count() > 0 ? /* interval [0..range) */ Dur{randrange(range.count())} :
-                range.count() < 0 ? /* interval (range..0] */ -Dur{randrange(-range.count())} :
-                                    /* interval [0..0] */ Dur{0}};
-        return time + dur;
+        return time + rand_uniform_duration<Tp>(range);
     }
 
+    /** Generate a uniform random duration in the range from 0 (inclusive) to range (exclusive). */
+    template <typename Chrono>
+    typename Chrono::duration rand_uniform_duration(typename Chrono::duration range) noexcept
+    {
+        using Dur = typename Chrono::duration;
+        return range.count() > 0 ? /* interval [0..range) */ Dur{randrange(range.count())} :
+               range.count() < 0 ? /* interval (range..0] */ -Dur{randrange(-range.count())} :
+                                   /* interval [0..0] */ Dur{0};
+    };
+
     // Compatibility with the C++11 UniformRandomBitGenerator concept
     typedef uint64_t result_type;
     static constexpr uint64_t min() { return 0; }

src/test/random_tests.cpp

@@ -53,6 +53,16 @@ BOOST_AUTO_TEST_CASE(fastrandom_tests)
     BOOST_CHECK_EQUAL(ctx1.randbits(3), ctx2.randbits(3));
     BOOST_CHECK(ctx1.rand256() == ctx2.rand256());
     BOOST_CHECK(ctx1.randbytes(50) == ctx2.randbytes(50));
+    {
+        struct MicroClock {
+            using duration = std::chrono::microseconds;
+        };
+        FastRandomContext ctx{true};
+        // Check with clock type
+        BOOST_CHECK_EQUAL(47222, ctx.rand_uniform_duration<MicroClock>(1s).count());
+        // Check with time-point type
+        BOOST_CHECK_EQUAL(2782, ctx.rand_uniform_duration<SteadySeconds>(9h).count());
+    }
 
     // Check that a nondeterministic ones are not
     g_mock_deterministic_tests = false;

src/threadinterrupt.cpp

@@ -28,18 +28,8 @@ void CThreadInterrupt::operator()()
     cond.notify_all();
 }
 
-bool CThreadInterrupt::sleep_for(std::chrono::milliseconds rel_time)
+bool CThreadInterrupt::sleep_for(Clock::duration rel_time)
 {
     WAIT_LOCK(mut, lock);
     return !cond.wait_for(lock, rel_time, [this]() { return flag.load(std::memory_order_acquire); });
 }
-
-bool CThreadInterrupt::sleep_for(std::chrono::seconds rel_time)
-{
-    return sleep_for(std::chrono::duration_cast<std::chrono::milliseconds>(rel_time));
-}
-
-bool CThreadInterrupt::sleep_for(std::chrono::minutes rel_time)
-{
-    return sleep_for(std::chrono::duration_cast<std::chrono::milliseconds>(rel_time));
-}

src/threadinterrupt.h

@@ -19,13 +19,12 @@
 class CThreadInterrupt
 {
 public:
+    using Clock = std::chrono::steady_clock;
     CThreadInterrupt();
     explicit operator bool() const;
     void operator()() EXCLUSIVE_LOCKS_REQUIRED(!mut);
     void reset();
-    bool sleep_for(std::chrono::milliseconds rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut);
-    bool sleep_for(std::chrono::seconds rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut);
-    bool sleep_for(std::chrono::minutes rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut);
+    bool sleep_for(Clock::duration rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut);
 
 private:
     std::condition_variable cond;