nit in 7eccdaf: Any reason to use the throwing function? The internal error isn't caught anywhere, IIUC. So, the program will terminate either way. By using *Assert(AnyPtr(context)) here instead, at least a usable error message is printed.

I probably just copied similar code from another place :-)

Can make a followup, though *Assert(AnyPtr(context)) looks pretty scary.

You probably copied it from an RPC method implementation. There, it would be the right choice, because the http threads catch exceptions and then continue with the next RPC call.

However, here no exceptions are caught (and doing so wouldn't make sense, probably). The program will terminate either way, for example:

2024-09-24T10:38:06Z opencon thread exit
terminate called after throwing an instance of 'UniValue'
Aborted (core dumped)

However, by using Assert/assert, at least a source location is printed. If you don't like the assert, you can also use an if guard against a nullptr, but I am not sure if that is cleaner.

In any case, this is just a style nit.

b94b27c: Can you explain the meaning of "overflow" in the context of double?

To clarify: Signed interger overflow is UB, so it can't happen. And overflow for double can't happen either, so this is dead code either way, unless I am missing something.

It was in response to: #30409 (comment) where @ryanofsky wrote:

I'm also concerned about the deadline = now() + timeout assignment above because it seems like that will be an overflowed value when timeout is MillisecondsDouble::max() so the while loop condition might be invalid.

Nice catch. I believe this can be simplified to:

--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -941,9 +941,7 @@ public:
         // Interrupt check interval
         const MillisecondsDouble tick{1000};
         auto now{std::chrono::steady_clock::now()};
-        auto deadline = now + timeout;
-        // std::chrono does not check against overflow
-        if (deadline < now) deadline = std::chrono::steady_clock::time_point::max();
+        const auto deadline{now + timeout};
         {
             WAIT_LOCK(notifications().m_tip_block_mutex, lock);
             while ((notifications().m_tip_block == uint256() || notifications().m_tip_block == current_tip) && !chainman().m_interrupt) {

This previous code written to handle the case where now + timeout calculation would overflow, basically assuming both values were unsigned integer values and overflow behavior would be well defined. But since timeout is a floating point number where "overflow" would just result in inf being set, trying to deal with it this way doesn't make sense.

This needs a GUARDED_BY(m_tip_block_mutex).

Adding it causes new warnings in validation_chainstate_tests.cpp. For ex:

src/test/validation_chainstate_tests.cpp:73:46: warning: reading variable 'm_tip_block' requires holding mutex 'm_node.notifications->m_tip_block_mutex' [-Wthread-safety-analysis]

I assume those particular accesses are harmless (maybe actually racy in the right conditions?), but future users may not be.

Related: It's a shame that the condvar/mutex leak out of here. Is there a reason not to add getter/waiter/notifier functions?