Merge #16046: util: Add type safe GetTime

fa01366 util: Add type safe GetTime (MarcoFalke)

Pull request description:

  There are basically two ways to get the time in Bitcoin Core:
  * get the system time (via `GetSystemTimeInSeconds` or `GetTime{Millis,Micros}`)
  * get the mockable time (via `GetTime`)

  Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc.

  Fix that by deprecating `GetTime` and adding a `GetTime<>` that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible.

ACKs for commit fa0136:
  promag:
    utACK fa01366.

Tree-SHA512: efab9c463f079fd8fd3030c479637c7b1e8be567a881234bd0f555c8f87e518e3b43ef2466128103db8fc40295aaf24e87ad76d91f338c631246fc703477e95c

Author: laanwj

src/Makefile.bench.include

@@ -28,6 +28,7 @@ bench_bench_bitcoin_SOURCES = \
   bench/merkle_root.cpp \
   bench/mempool_eviction.cpp \
   bench/rpc_mempool.cpp \
+  bench/util_time.cpp \
   bench/verify_script.cpp \
   bench/base58.cpp \
   bench/bech32.cpp \

src/bench/util_time.cpp

@@ -0,0 +1,42 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <bench/bench.h>
+
+#include <util/time.h>
+
+static void BenchTimeDeprecated(benchmark::State& state)
+{
+    while (state.KeepRunning()) {
+        (void)GetTime();
+    }
+}
+
+static void BenchTimeMock(benchmark::State& state)
+{
+    SetMockTime(111);
+    while (state.KeepRunning()) {
+        (void)GetTime<std::chrono::seconds>();
+    }
+    SetMockTime(0);
+}
+
+static void BenchTimeMillis(benchmark::State& state)
+{
+    while (state.KeepRunning()) {
+        (void)GetTime<std::chrono::milliseconds>();
+    }
+}
+
+static void BenchTimeMillisSys(benchmark::State& state)
+{
+    while (state.KeepRunning()) {
+        (void)GetTimeMillis();
+    }
+}
+
+BENCHMARK(BenchTimeDeprecated, 100000000);
+BENCHMARK(BenchTimeMillis, 6000000);
+BENCHMARK(BenchTimeMillisSys, 6000000);
+BENCHMARK(BenchTimeMock, 300000000);

src/test/util_tests.cpp

@@ -1068,6 +1068,27 @@ BOOST_AUTO_TEST_CASE(gettime)
     BOOST_CHECK((GetTime() & ~0xFFFFFFFFLL) == 0);
 }
 
+BOOST_AUTO_TEST_CASE(util_time_GetTime)
+{
+    SetMockTime(111);
+    // Check that mock time does not change after a sleep
+    for (const auto& num_sleep : {0, 1}) {
+        MilliSleep(num_sleep);
+        BOOST_CHECK_EQUAL(111, GetTime()); // Deprecated time getter
+        BOOST_CHECK_EQUAL(111, GetTime<std::chrono::seconds>().count());
+        BOOST_CHECK_EQUAL(111000, GetTime<std::chrono::milliseconds>().count());
+        BOOST_CHECK_EQUAL(111000000, GetTime<std::chrono::microseconds>().count());
+    }
+
+    SetMockTime(0);
+    // Check that system time changes after a sleep
+    const auto ms_0 = GetTime<std::chrono::milliseconds>();
+    const auto us_0 = GetTime<std::chrono::microseconds>();
+    MilliSleep(1);
+    BOOST_CHECK(ms_0 < GetTime<std::chrono::milliseconds>());
+    BOOST_CHECK(us_0 < GetTime<std::chrono::microseconds>());
+}
+
 BOOST_AUTO_TEST_CASE(test_IsDigit)
 {
     BOOST_CHECK_EQUAL(IsDigit('0'), true);

src/util/time.cpp

@@ -1,5 +1,5 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2018 The Bitcoin Core developers
+// Copyright (c) 2009-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -27,6 +27,20 @@ int64_t GetTime()
     return now;
 }
 
+template <typename T>
+T GetTime()
+{
+    const std::chrono::seconds mocktime{nMockTime.load(std::memory_order_relaxed)};
+
+    return std::chrono::duration_cast<T>(
+        mocktime.count() ?
+            mocktime :
+            std::chrono::microseconds{GetTimeMicros()});
+}
+template std::chrono::seconds GetTime();
+template std::chrono::milliseconds GetTime();
+template std::chrono::microseconds GetTime();
+
 void SetMockTime(int64_t nMockTimeIn)
 {
     nMockTime.store(nMockTimeIn, std::memory_order_relaxed);

src/util/time.h

@@ -1,5 +1,5 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2018 The Bitcoin Core developers
+// Copyright (c) 2009-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -8,27 +8,34 @@
 
 #include <stdint.h>
 #include <string>
+#include <chrono>
 
 /**
- * GetTimeMicros() and GetTimeMillis() both return the system time, but in
- * different units. GetTime() returns the system time in seconds, but also
- * supports mocktime, where the time can be specified by the user, eg for
- * testing (eg with the setmocktime rpc, or -mocktime argument).
- *
- * TODO: Rework these functions to be type-safe (so that we don't inadvertently
- * compare numbers with different units, or compare a mocktime to system time).
+ * DEPRECATED
+ * Use either GetSystemTimeInSeconds (not mockable) or GetTime<T> (mockable)
  */
-
 int64_t GetTime();
+
+/** Returns the system time (not mockable) */
 int64_t GetTimeMillis();
+/** Returns the system time (not mockable) */
 int64_t GetTimeMicros();
+/** Returns the system time (not mockable) */
 int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable
+
+/** For testing. Set e.g. with the setmocktime rpc, or -mocktime argument */
 void SetMockTime(int64_t nMockTimeIn);
+/** For testing */
 int64_t GetMockTime();
+
 void MilliSleep(int64_t n);
 
+/** Return system time (or mocked time, if set) */
+template <typename T>
+T GetTime();
+
 /**
- * ISO 8601 formatting is preferred. Use the FormatISO8601{DateTime,Date,Time}
+ * ISO 8601 formatting is preferred. Use the FormatISO8601{DateTime,Date}
  * helper functions if possible.
  */
 std::string FormatISO8601DateTime(int64_t nTime);