In commit “interfaces: refactor: move waitTipChanged implementation to miner” (c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6)
I think name ChainTipChanged could be a little misleading since this returns a bool and you might think it returns true when the tip has changed, false otherwise. But in reality it returns true whether or not this tip has changed (after the timeout), and the only case it returns false is when the node is shutting down.
To be less confusing I think it would be good to rename this and return the actual tip instead of a bool. Could do:
0```diff
1--- a/src/node/interfaces.cpp
2+++ b/src/node/interfaces.cpp
3@@ -974,7 +974,7 @@ public:
4
5 std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
6 {
7- if (ChainTipChanged(chainman(), notifications(), current_tip, timeout)) return getTip();
8+ if (WaitTipChanged(chainman(), notifications(), current_tip, timeout)) return getTip();
9 return {};
10 }
11
12--- a/src/node/miner.cpp
13+++ b/src/node/miner.cpp
14@@ -539,7 +539,7 @@ std::optional<std::unique_ptr<CBlockTemplate>> WaitNext(ChainstateManager& chain
15 return std::nullopt;
16 }
17
18-bool ChainTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout)
19+std::optional<uint256> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout)
20 {
21 Assume(timeout >= 0ms); // No internal callers should use a negative timeout
22 if (timeout < 0ms) timeout = 0ms;
23@@ -554,16 +554,14 @@ bool ChainTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_no
24 kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
25 return kernel_notifications.TipBlock() || chainman.m_interrupt;
26 });
27- if (chainman.m_interrupt) return false;
28+ if (chainman.m_interrupt) return {};
29 // At this point TipBlock is set, so continue to wait until it is
30 // different then `current_tip` provided by caller.
31 kernel_notifications.m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
32 return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt;
33 });
34+ if (chainman.m_interrupt) return {};
35+ return Assume(kernel_notifications.TipBlock());
36 }
37-
38- if (chainman.m_interrupt) return false;
39-
40- return true;
41 }
42 } // namespace node
43--- a/src/node/miner.h
44+++ b/src/node/miner.h
45@@ -239,8 +239,8 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
46 std::optional<std::unique_ptr<CBlockTemplate>> WaitNext(ChainstateManager& chainman, KernelNotifications& kernel_notifications, CTxMemPool* mempool, const uint256& prev_block_hash,
47 const std::vector<CAmount>& tx_fees, const BlockWaitOptions& options, const BlockAssembler::Options& assemble_options);
48
49-/* Waits for the connected tip to change until timeout has elapsed. During node initialization, this will wait until the tip is connected (regardless of `timeout`). */
50-bool ChainTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout);
51+/* Waits for the connected tip to change until timeout has elapsed. During node initialization, this will wait until the tip is connected (regardless of `timeout`). Returns the current tip hash, or nullopt if the node is shutting down. */
52+std::optional<uint256> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout);
53 } // namespace node
54
55 #endif // BITCOIN_NODE_MINER_H
