Add mockable clock type #25101

Doesn't Now<>() want a timepoint type, rather than a clock? Should be Now<NodeSeconds>() or Now<SteadyMilliseconds>(), no?

Good catch, reworked doxygen string

return Now<std::chrono::time_point<NodeClock, T>>().time_since_epoch() ?

Yes, makes sense to use the casting feature of Now.

Should this be called seconds_since_epoch (or the camelcase equivalent) perhaps? Looking back through #24697 it doesn't seem like it's just a temporary that will go away, so might be worth clearly distinguishing something that's working with a timepoint vs a duration.

At least as far as #24697 is concerned, this is effectively implementing the behaviour of NodeClock::to_time_t so it's a bit funny that you explicitly deleted it above then reimplemented it here :)

Is time_t using more than 32-bits on all platforms? Also rounding/truncation is implementation defined. So it seems best to avoid.

Also, I expect count_milliseconds to be added in the future, which to_time_t can't handle either.

seconds_since_epoch sounds good, will change.

Actually I think it is fine to silently do a cast inside the function. Similar to how it is fine to do a silent cast in Now.

So I'll be pushing a version of this that takes the duration as a template parameter.

Add a static_assert(std::is_integral_v<typename Duration::rep>) or have the return type be typename Duration::rep ?

Good catch. Used auto return type

s/sytem/system/

Between system_clock, steady_clock, and NodeClock, which would you see used for the CNodeStats?

I think NodeClock for everything except for really short waits, like ThreadMessageHandler (which is already steady_clock). system_clock only for implementing NodeClock and logging?

thx, fixed typo

Newline before { ? Isn't it better to put in public explicitly?

Public in structs might be unneeded, as access is public by default in structs / private by default in classes, and because https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c8-use-class-rather-than-struct-if-any-member-is-non-public

Yeah, it's definitely unneeded, just seems clearer for people to have it there explicitly? Seems like we usually have an explicit public when inheriting a struct from some other struct.

Yes, good point re inheriting.

Thx, added public inheritance

Note: SteadyMicroseconds is currently unused and untested.

Not sure how to test it or how to make it used. It was added in case someone wants to use it, but I am happy to remove it and let someone add it later if it gets used.

side note: It can be tested in the // Check that steady time and system time changes after a sleep test case.

Should the rule of five apply? https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all

I am not touching de-/con-structors, so I guess no?

Right, the intention here is to delete the to_time_t and from_time_t class members inherited from std::chrono::system_clock, so IIUC, we don't care here about copy constructor deletion implicitly suppressing move constructor and move assignment operators.

Verified these are deleted in the same way they are declared per https://en.cppreference.com/w/cpp/chrono/system_clock/to_time_t and https://en.cppreference.com/w/cpp/chrono/system_clock/from_time_t.

You don't normally instantiate instances of clock types, all the methods on them are static / class methods, so constructors/destructors/copying don't matter?

right. They don't matter and as I mentioned before, I am not touching them at all, so I fail to understand what the issue is.

See also:

(not meaningful, but compiles fine)

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index eeecbe5ea9..c6fd1ec548 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1483,6 +1483,17 @@ BOOST_AUTO_TEST_CASE(gettime)
 
 BOOST_AUTO_TEST_CASE(util_time_GetTime)
 {
+{
+auto c1{NodeClock{}};
+auto c3{c1};
+auto c2{std::move(c1)};
+}
+{
+auto c1{std::chrono::system_clock{}};
+auto c3{c1};
+auto c2{std::move(c1)};
+}
+
     SetMockTime(111);
     // Check that mock time does not change after a sleep
     for (const auto& num_sleep : {0ms, 1ms}) {

I think the last three comments agree, but I'm not sure :) I did want to ask to make sure and check my assumptions. Thanks for the replies.

Continuing with that test code snippet, verified these don't compile as expected.

test/util_tests.cpp:1487:25: error: attempt to use a deleted function
    auto c1{NodeClock{}.to_time_t(std::chrono::system_clock::now())};
                        ^
./util/time.h:22:24: note: 'to_time_t' has been explicitly marked deleted here
    static std::time_t to_time_t(const time_point&) = delete; // unused

test/util_tests.cpp:1487:25: error: attempt to use a deleted function
    auto c1{NodeClock{}.from_time_t(std::chrono::system_clock{})};
                        ^
./util/time.h:23:23: note: 'from_time_t' has been explicitly marked deleted here
    static time_point from_time_t(std::time_t) = delete;      // unused