util: Add type safe GetTime #16046

MarcoFalke commented at 11:47 AM on May 19, 2019: member

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 99464bc38e9575ff47f8e33223b252dcea2055e3.

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.

MarcoFalke added the label Utils/log/libs on May 19, 2019

MarcoFalke force-pushed on May 19, 2019

DrahtBot commented at 1:36 PM on May 19, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

MarcoFalke force-pushed on May 19, 2019

promag commented at 8:51 PM on May 19, 2019: member

Concept ACK.

practicalswift commented at 7:32 AM on May 20, 2019: contributor

Concept ACK

MarcoFalke force-pushed on May 20, 2019

in src/test/util_tests.cpp:139 in faaff50dd2 outdated

 131 | @@ -132,6 +132,27 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Date)
 132 |      BOOST_CHECK_EQUAL(FormatISO8601Date(1317425777), "2011-09-30");
 133 |  }
 134 |  
 135 | +BOOST_AUTO_TEST_CASE(util_time_GetTime)
 136 | +{
 137 | +    SetMockTime(111);
 138 | +    // Check that mock time does not change after a sleep
 139 | +    for (const auto& num_sleep : {0, 1}) {

promag commented at 1:48 PM on May 20, 2019:

Why the loop? Isn't enough one loop after MilliSleep(1)?

MarcoFalke commented at 2:01 PM on May 20, 2019:

Yeah, those are unit tests. Probably enough to not have them at all :woman_shrugging:

in src/util/time.h:11 in faaff50dd2 outdated

   7 | @@ -8,27 +8,34 @@
   8 |  
   9 |  #include <stdint.h>
  10 |  #include <string>
  11 | +#include <chrono>

promag commented at 1:49 PM on May 20, 2019:

nit, sorted.

MarcoFalke commented at 8:22 PM on March 17, 2020:

This is not possible and would lead to compile errors:

util/time.cpp: In function ‘std::__cxx11::string FormatISO8601DateTime(int64_t)’:

util/time.cpp:84:9: error: ‘gmtime_r’ was not declared in this scope

     if (gmtime_r(&time_val, &ts) == nullptr) {

         ^~~~~~~~

util/time.cpp:84:9: note: suggested alternative: ‘gmtime_s’

     if (gmtime_r(&time_val, &ts) == nullptr) {

         ^~~~~~~~

         gmtime_s

util/time.cpp: In function ‘std::__cxx11::string FormatISO8601Date(int64_t)’:

util/time.cpp:97:9: error: ‘gmtime_r’ was not declared in this scope

     if (gmtime_r(&time_val, &ts) == nullptr) {

         ^~~~~~~~

util/time.cpp:97:9: note: suggested alternative: ‘gmtime_s’

     if (gmtime_r(&time_val, &ts) == nullptr) {

         ^~~~~~~~

         gmtime_s

Makefile:11303: recipe for target 'util/libbitcoin_util_a-time.o' failed

MarcoFalke commented at 8:22 PM on March 17, 2020:

Fixed in #18358, maybe?

promag commented at 1:49 PM on May 20, 2019: member

utACK faaff50.

in src/util/time.h:13 in faaff50dd2 outdated

   7 | @@ -8,27 +8,34 @@
   8 |  
   9 |  #include <stdint.h>
  10 |  #include <string>
  11 | +#include <chrono>
  12 |  
  13 |  /**
  14 | - * GetTimeMicros() and GetTimeMillis() both return the system time, but in

laanwj commented at 3:12 PM on May 20, 2019:

Need to keep the information here that GetTimeMicros() and GetTimeMillis() aren't mocked

MarcoFalke commented at 3:20 PM on May 20, 2019:

Their docstring already says Returns the system time. Anything I should add?

laanwj commented at 6:05 PM on May 23, 2019:

I think it's better to explicitly mention that they ignore the mocked time. My implicit assumption while reading the documentation would be that mocktime would affect everyting.

MarcoFalke commented at 6:14 PM on May 23, 2019:

Yeah, makes sense. Fixed up documentation a bit

MarcoFalke force-pushed on May 21, 2019

util: Add type safe GetTime fa013664ae

MarcoFalke force-pushed on May 23, 2019

promag commented at 11:04 PM on May 27, 2019: member

utACK fa013664.

laanwj commented at 11:40 AM on May 29, 2019: member

utACK fa013664ae23d0682a195b9bded85bc19c99536e

laanwj merged this on May 29, 2019

laanwj closed this on May 29, 2019

laanwj referenced this in commit 62efead8a8 on May 29, 2019

MarcoFalke deleted the branch on May 29, 2019

deadalnix referenced this in commit ffaaea1a43 on Mar 8, 2020

codablock referenced this in commit a76bafb98c on Apr 8, 2020

ftrader referenced this in commit ed060277a7 on May 19, 2020

ckti referenced this in commit 4b1dfa3583 on Mar 28, 2021

random-zebra referenced this in commit 14060430d9 on May 5, 2021

gades referenced this in commit d0d213d4c5 on Jun 30, 2021

DrahtBot locked this on Feb 15, 2022