rpc: make `uptime` monotonic across NTP jumps #34328

Not sure if this is accurate. I think it would be better to change the gui to also use raw monotonic uptime, instead of made-up wall clock timestamps.

Isn't this what users would be interested in, what they would see on their clocks locally? @achow101, can you help us out here?

Yeah, I guess it is just a nit. Would seem a bit odd to have the gui startup time not refer to any startup time written to the debug log, but I guess all odds are lost when the user is using the wrong clock.

Yeah, it's the least awkward solution I found

nit: You are changing all call-sites anyway, so my preference would be to return the full SteadyClock resolution here, and only cast at call-sites, when needed.

I can add a TicksSeconds helper which would keep the result small and focused and only collapse the type as late as possible - taken, thanks

nit: Could inline this into the function body to reduce the scope? Also, globals usually start with g_...

That would increase the diff, but simplify the result - taken, added you as coauthor, thanks

        assert uptime_after - uptime_before <= wait_time, "uptime should not jump with wall clock"

Why not just keep the previous variable and logic? Possibly with wait_time=20_000

The previous logic didn't recreate the problem, but I can simplify to just state that the actual uptime reported is small (instead of comparing it to actual elapsed).

nit: Still missing g_ prefix for this global?

It's function-local static with reduced scope, so it's a file-scope global, I prefer keeping it as is.

Yeah, I think those are using the g_ prefix as well, see:

src/random.cpp:RNGState& GetRNGState() noexcept
src/random.cpp-{
src/random.cpp-    // This idiom relies on the guarantee that static variable are initialized
src/random.cpp-    // on first call, even when multiple parallel calls are permitted.
src/random.cpp-    static std::vector<RNGState, secure_allocator<RNGState>> g_rng(1);
src/random.cpp-    return g_rng[0];
src/random.cpp-}

ok, done

/// Monotonic uptime (not affected by system time changes).

nit: I think doxygen comments require three ///.

Sure, done

not sure about this alias. I think it is fine to just use the Ticks<...> helper directly. Otherwise, we'll add an alias for every chrono duration type?

We already have helpers for most of these, seconds seem common enough that I would like to cover it with a dedicated helper, the alternative is very noisy in my opinion

I think it is good for casts to be noisy, as they should only be used rarely (rpc interface, debug log, ...). Also, it seems good to see at the call site that a cast is done via the angle-brackets.

The count_* helpers below exist, because they are cast-free.

But just a nit, not a blocker.

nit: Seems fine, but not a fan of introducing more use of the deprecated GetTime. I think

    return QDateTime::currentDateTime().addSecs(Ticks<...>(GetUptime()).toString();

Sure, done

    assert uptime_after - uptime_before < wait_time  # uptime should not jump with wall clock

nit: Could use wait_time here, to avoid something hard-coded? (Here and in setmocktime)

I'm not sure what wait_time refers to, added self.rpc_timeout since that should be a hard limit.

I'm not sure what wait_time refers to, added self.rpc_timeout since that should be a hard limit.

It is just the name of the symbol that refers to a duration used in this test:

wait_time = 20_000
uptime_before = self.nodes[0].uptime()
self.nodes[0].setmocktime(int(time.time() + wait_time))
uptime_after = self.nodes[0].uptime()
assert uptime_after - uptime_before < wait_time  # uptime should not jump with wall clock
self.nodes[0].setmocktime(0)

I think the above test should fail on current master and pass with this pull request.

Do you have anything against the current solution?

I don't have strong preference, but changed it anyway.

I think the above test should fail on current master

yes:

  File "/Users/lorinc/IdeaProjects/bitcoin/build/test/functional/rpc_uptime.py", line 23, in run_test
    self._test_uptime()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/lorinc/IdeaProjects/bitcoin/build/test/functional/rpc_uptime.py", line 34, in _test_uptime
    assert uptime_after - uptime_before < wait_time, "uptime should not jump with wall clock"
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: uptime should not jump with wall clock

style-nit, to make the diff smaller:

self.nodes[0].setmocktime(int(time.time() + wait_time))

I did it deliberately to avoid + overload - seemed simpler

llm-nit: Possible places where comparison-specific test macros should replace generic comparisons:

• test/functional/rpc_uptime.py "assert uptime_after - uptime_before < wait_time, "uptime should not jump with wall clock"" -> recommendation: use the comparison helper by reversing operands, e.g. assert_greater_than(wait_time, uptime_after - uptime_before), to avoid a bare assert.

I find this unreadable, I will use them when someone renames it to assert_gt or something similar that's less noisy.

The utility exits so that intermittent failures are easier to debug. Arguably not an issue here, but I think if a utility was added for a purpose, it should be used for that purpose consistently.

If you don't like the name, it can be renamed. I don't care about the name, so I am happy to ack. I mostly care about the functionality.

Would be nice to be able to use the helper but also get the string context 🤷🏼‍♂️

I don't see any difference between assert_gt and assert_greater_than myself though...

Boost has BOOST_CHECK_GE while the python equivalent is assert_greater_than_or_equal - that's a whole line of noise replacing >= - I'll push a PR to unify them with the boost checks to improve the signal-to-noise ratio a bit...

Edit: pushed to https://github.com/bitcoin/bitcoin/pull/34395

nit: If you touch again could add an empty line here :D

It was deliberate, but I don't mind doing it if I have to retouch