util: Add type safe GetTime #16046

maflcko · 2019-05-19T11:47:43Z

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.

DrahtBot · 2019-05-19T13:36:54Z

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

Conflicts

No conflicts as of last run.

promag · 2019-05-19T20:51:16Z

Concept ACK.

practicalswift · 2019-05-20T07:32:36Z

Concept ACK

promag

utACK faaff50.

promag · 2019-05-20T13:48:21Z

src/test/util_tests.cpp

+{
+    SetMockTime(111);
+    // Check that mock time does not change after a sleep
+    for (const auto& num_sleep : {0, 1}) {


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

Yeah, those are unit tests. Probably enough to not have them at all 🤷‍♀️

promag · 2019-05-20T13:49:02Z

src/util/time.h

@@ -8,27 +8,34 @@

 #include <stdint.h>
 #include <string>
+#include <chrono>


nit, sorted.

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

Fixed in #18358, maybe?

laanwj · 2019-05-20T15:12:57Z

src/util/time.h


 /**
- * GetTimeMicros() and GetTimeMillis() both return the system time, but in


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

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

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.

Yeah, makes sense. Fixed up documentation a bit

promag · 2019-05-27T23:04:57Z

utACK fa01366.

laanwj · 2019-05-29T11:40:07Z

utACK fa01366

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

Summary: This is a backport of [[bitcoin/bitcoin#16046 | PR16046]] Test Plan: ninja check ninja bench-bitcoin Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D5445

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

Summary: This is a backport of [[bitcoin/bitcoin#16046 | PR16046]] Test Plan: ninja check ninja bench-bitcoin Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D5445

f3f3d6c util: fix compilation with mingw-w64 7.0.0 (Fuzzbawls) f24da7d util: Avoid potential uninitialized read in FormatISO8601DateTime (Fuzzbawls) 120a12a util: Add type safe GetTime (Fuzzbawls) be48766 Fix for utiltime to compile with msvc. (Aaron Clauson) 2833406 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) 3331c25 Format automatic wallet backup file names in ISO 8601 basic format (Fuzzbawls) c32a208 Format timestamps using ISO 8601 formatting (e.g. "2018-02-28T12:34:56Z") (practicalswift) f8bd173 [logging] log system time and mock time (John Newbery) Pull request description: This is a backport of a collection of upstream PRs to bring our time utility more up to date. The following upstream PRs are included: - bitcoin#10383 - bitcoin#12567 - bitcoin#12973 - bitcoin#13031 - bitcoin#16046 - bitcoin#18162 - bitcoin#18358 The two user-facing notable changes here have been documented in the release notes template. I've also connected the functionality that `-logtimemicros` was supposed to provide. ACKs for top commit: furszy: code review ACK f3f3d6c random-zebra: utACK f3f3d6c after rebase, and merging... Tree-SHA512: 64f9cc1f7fc65c192f3b3ab869c057a81e007cf0fae82ecd23993c6b51830e4bc80656c699ba40bb8e019115a24d1ea88a618be0c0d7112749d9ce125d62ce44

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

maflcko added the Utils/log/libs label May 19, 2019

maflcko force-pushed the 1905-typeTime branch from fa1a581 to faa69f9 Compare May 19, 2019 13:31

maflcko force-pushed the 1905-typeTime branch 5 times, most recently from 2d19686 to fac4143 Compare May 19, 2019 15:02

maflcko force-pushed the 1905-typeTime branch from fac4143 to faaff50 Compare May 20, 2019 12:20

promag reviewed May 20, 2019

View reviewed changes

laanwj reviewed May 20, 2019

View reviewed changes

maflcko force-pushed the 1905-typeTime branch from faaff50 to fad0ab4 Compare May 21, 2019 20:30

util: Add type safe GetTime

fa01366

maflcko force-pushed the 1905-typeTime branch from fad0ab4 to fa01366 Compare May 23, 2019 18:14

laanwj merged commit fa01366 into bitcoin:master May 29, 2019

maflcko deleted the 1905-typeTime branch May 29, 2019 12:10

codablock mentioned this pull request Apr 8, 2020

Backport bitcoin#14897 and bitcoin#15834 and modify it to work with Dash messages dashpay/dash#3397

Merged

Fuzzbawls mentioned this pull request Apr 8, 2021

[Util] Time utility updates PIVX-Project/PIVX#2300

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

util: Add type safe GetTime #16046

util: Add type safe GetTime #16046

Uh oh!

maflcko commented May 19, 2019

Uh oh!

DrahtBot commented May 19, 2019 •

edited

Loading

Uh oh!

promag commented May 19, 2019

Uh oh!

practicalswift commented May 20, 2019

Uh oh!

promag left a comment

Uh oh!

promag May 20, 2019

Uh oh!

maflcko May 20, 2019

Uh oh!

promag May 20, 2019

Uh oh!

maflcko Mar 17, 2020

Uh oh!

maflcko Mar 17, 2020

Uh oh!

laanwj May 20, 2019

Uh oh!

maflcko May 20, 2019

Uh oh!

laanwj May 23, 2019

Uh oh!

maflcko May 23, 2019

Uh oh!

promag commented May 27, 2019

Uh oh!

laanwj commented May 29, 2019

Uh oh!

Uh oh!


		/**
		* GetTimeMicros() and GetTimeMillis() both return the system time, but in

util: Add type safe GetTime #16046

util: Add type safe GetTime #16046

Uh oh!

Conversation

maflcko commented May 19, 2019

Uh oh!

DrahtBot commented May 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented May 19, 2019

Uh oh!

practicalswift commented May 20, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented May 27, 2019

Uh oh!

laanwj commented May 29, 2019

Uh oh!

Uh oh!

DrahtBot commented May 19, 2019 •

edited

Loading