In commit “rpc: handle shutdown during long poll and wait methods” (fa99eddd926e0d49604d9ca8f21fac85d63652f4)
I think this new behavior of only using timeout as time limit for tip changing, not as a time limit for the tip to be set on startup makes a lot of sense and should be much more convenient for callers than previous behavior.
(Previous version in ad3af401c19b8d05ce69011a359db3090b7018e1 would return ambiguous null values on startup that could mean either the node was shutting down or the timeout was reached, and would force callers to immediately make another waitTipChanged call without even giving them any new information in the meantime.)
Only downside of this behavior is that mining API no longer includes a function that can wait with a limited timeout for the tip to be non-null on startup. But if anybody wants this we could add a timeout option to getTip() or a new separate waitForStartup() method accepting a timeout. Or we could not provide anything, since it is pretty easy for callers to implemented their own timeouts if they are using the capnproto client API.
However, the current implementation of this behavior in fa99eddd926e0d49604d9ca8f21fac85d63652f4 looks a bit janky to me, with double nested wait loop, unnecessary 100ms polling, unnecessary tip_hash variable, and an Assume() check out of place and delayed for no reason. Following is untested, but I think would a simpler implementation that cleans all these issues up:
0 std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
1 {
2 if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
3 auto deadline{std::chrono::steady_clock::now() + timeout};
4 {
5 WAIT_LOCK(notifications().m_tip_block_mutex, lock);
6 // For callers convenience, wait longer than the provided timeout
7 // during startup for the tip to be non-null. That way this function
8 // always returns valid tip information when possible and only
9 // returns null when shutting down, not when timing out.
10 notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
11 return notifications().TipBlock() || chainman().m_interrupt;
12 });
13 // At this point TipBlock is set, so continue to wait until it is
14 // different then `current_tip` provided by caller.
15 notifications().m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
16 return Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt;
17 });
18 }
19
20 if (chainman().m_interrupt) return {};
21
22 // Must release m_tip_block_mutex before getTip() locks cs_main, to
23 // avoid deadlocks.
24 return getTip();
25 }
