The issue is actually pretty hard to trigger because the Cap’n Proto’s internal reference counting keeps the BlockTemplate object alive by default until the waitNext call returns. So a client would have to explicitly call destroy on the BlockTemplate object in that case or the Mining object in this case for there to be a crash.
So I think the current implementation of this is ok and unlikely to lead to any accidental crashes.
I did go ahead and implement my suggestion before realizing the problem was not so bad, so will paste it here for reference, but your current solution seems cleaner and I’d recommend keeping it.
0--- a/src/node/interfaces.cpp
1+++ b/src/node/interfaces.cpp
2@@ -929,7 +929,7 @@ public:
3
4 void interruptWait() override
5 {
6- InterruptWait(notifications(), m_interrupt_wait);
7+ InterruptWait(notifications(), &m_interrupt_wait);
8 }
9
10 const BlockAssembler::Options m_assemble_options;
11@@ -964,7 +964,7 @@ public:
12
13 std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
14 {
15- return WaitTipChanged(chainman(), notifications(), current_tip, timeout, m_interrupt);
16+ return WaitTipChanged(chainman(), notifications(), current_tip, timeout);
17 }
18
19 std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
20@@ -976,7 +976,7 @@ public:
21 // Avoid generating a new template immediately after connecting a block while
22 // our best known header chain still has more work. This prevents a burst of
23 // block templates during the final catch-up moments after IBD.
24- if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt)) return {};
25+ if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip)) return {};
26
27 BlockAssembler::Options assemble_options{options};
28 ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
29@@ -985,7 +985,7 @@ public:
30
31 void interrupt() override
32 {
33- InterruptWait(notifications(), m_interrupt);
34+ InterruptWait(notifications());
35 }
36
37 bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
38@@ -1000,7 +1000,6 @@ public:
39 NodeContext* context() override { return &m_node; }
40 ChainstateManager& chainman() { return *Assert(m_node.chainman); }
41 KernelNotifications& notifications() { return *Assert(m_node.notifications); }
42- bool m_interrupt{false};
43 NodeContext& m_node;
44 };
45 } // namespace
46--- a/src/node/kernel_notifications.h
47+++ b/src/node/kernel_notifications.h
48@@ -61,6 +61,14 @@ public:
49 //! Might be unset during an early shutdown.
50 std::optional<uint256> TipBlock() EXCLUSIVE_LOCKS_REQUIRED(m_tip_block_mutex);
51
52+ //! Whether a createNewBlock() / waitNext() cancellation has been requested.
53+ //! Having this variable here hacky, but avoids the problem of
54+ //! createNewBlock not being interruptible during IBD by mining clients, and
55+ //! avoids memory safety issues that would occur if the variable was
56+ //! somewhere else. An idea for getting rid of this is described in
57+ //! [#34184 (review)](/bitcoin-bitcoin/34184/#discussion_r2742950202)
58+ bool m_blocktemplate_request_interrupted GUARDED_BY(m_tip_block_mutex);
59+
60 private:
61 const std::function<bool()>& m_shutdown_request;
62 std::atomic<int>& m_exit_status;
63--- a/src/node/miner.cpp
64+++ b/src/node/miner.cpp
65@@ -341,13 +341,27 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
66 block.fChecked = false;
67 }
68
69-void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait)
70+void InterruptWait(KernelNotifications& kernel_notifications, bool* interrupt_wait)
71 {
72 LOCK(kernel_notifications.m_tip_block_mutex);
73- interrupt_wait = true;
74+ kernel_notifications.m_blocktemplate_request_interrupted = true;
75+ if (interrupt_wait) *interrupt_wait = true;
76 kernel_notifications.m_tip_block_cv.notify_all();
77 }
78
79+static bool Interrupted(const KernelNotifications& kernel_notifications, const bool* interrupt_wait = nullptr) EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex)
80+{
81+ return kernel_notifications.m_blocktemplate_request_interrupted || (interrupt_wait && *interrupt_wait);
82+}
83+
84+static bool ResetInterrupt(KernelNotifications& kernel_notifications, bool* interrupt_wait = nullptr) EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex)
85+{
86+ if (!Interrupted(kernel_notifications, interrupt_wait)) return false;
87+ kernel_notifications.m_blocktemplate_request_interrupted = false;
88+ if (interrupt_wait) *interrupt_wait = false;
89+ return true;
90+}
91+
92 std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
93 KernelNotifications& kernel_notifications,
94 CTxMemPool* mempool,
95@@ -379,12 +393,9 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
96 // method on BlockTemplate and no template could have been
97 // generated before a tip exists.
98 tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock;
99- return tip_changed || chainman.m_interrupt || interrupt_wait;
100+ return tip_changed || chainman.m_interrupt || Interrupted(kernel_notifications, &interrupt_wait);
101 });
102- if (interrupt_wait) {
103- interrupt_wait = false;
104- return nullptr;
105- }
106+ if (ResetInterrupt(kernel_notifications, &interrupt_wait)) return nullptr;
107 }
108
109 if (chainman.m_interrupt) return nullptr;
110@@ -445,7 +456,7 @@ std::optional<BlockRef> GetTip(ChainstateManager& chainman)
111 return BlockRef{tip->GetBlockHash(), tip->nHeight};
112 }
113
114-bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip, bool& interrupt)
115+bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip)
116 {
117 constexpr auto COOLDOWN{1s};
118 // Use a steady clock so cooldown is independent of mocktime.
119@@ -455,12 +466,9 @@ bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& ke
120 WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
121 kernel_notifications.m_tip_block_cv.wait_until(lock, cooldown_deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
122 const auto tip_block = kernel_notifications.TipBlock();
123- return chainman.m_interrupt || interrupt || (tip_block && *tip_block != last_tip_hash);
124+ return chainman.m_interrupt || Interrupted(kernel_notifications) || (tip_block && *tip_block != last_tip_hash);
125 });
126- if (chainman.m_interrupt || interrupt) {
127- interrupt = false;
128- return false;
129- }
130+ if (chainman.m_interrupt || ResetInterrupt(kernel_notifications)) return false;
131
132 // If the tip changed during the wait, extend the deadline
133 const auto tip_block = kernel_notifications.TipBlock();
134@@ -477,7 +485,7 @@ bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& ke
135 return true;
136 }
137
138-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt)
139+std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout)
140 {
141 Assume(timeout >= 0ms); // No internal callers should use a negative timeout
142 if (timeout < 0ms) timeout = 0ms;
143@@ -490,21 +498,15 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
144 // always returns valid tip information when possible and only
145 // returns null when shutting down, not when timing out.
146 kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
147- return kernel_notifications.TipBlock() || chainman.m_interrupt || interrupt;
148+ return kernel_notifications.TipBlock() || chainman.m_interrupt || Interrupted(kernel_notifications);
149 });
150- if (chainman.m_interrupt || interrupt) {
151- interrupt = false;
152- return {};
153- }
154+ if (chainman.m_interrupt || ResetInterrupt(kernel_notifications)) return {};
155 // At this point TipBlock is set, so continue to wait until it is
156 // different then `current_tip` provided by caller.
157 kernel_notifications.m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
158- return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt || interrupt;
159+ return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt || Interrupted(kernel_notifications);
160 });
161- }
162- if (chainman.m_interrupt || interrupt) {
163- interrupt = false;
164- return {};
165+ if (chainman.m_interrupt || ResetInterrupt(kernel_notifications)) return {};
166 }
167
168 // Must release m_tip_block_mutex before getTip() locks cs_main, to
169--- a/src/node/miner.h
170+++ b/src/node/miner.h
171@@ -143,7 +143,7 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
172
173
174 /* Interrupt a blocking call. */
175-void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait);
176+void InterruptWait(KernelNotifications& kernel_notifications, bool* interrupt_wait = nullptr);
177 /**
178 * Return a new block template when fees rise to a certain threshold or after a
179 * new tip; return nullopt if timeout is reached.
180@@ -163,7 +163,7 @@ std::optional<BlockRef> GetTip(ChainstateManager& chainman);
181 * Returns the current tip, or nullopt if the node is shutting down or interrupt()
182 * is called.
183 */
184-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt);
185+std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout);
186
187 /**
188 * Wait while the best known header extends the current chain tip AND at least
189@@ -177,11 +177,10 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
190 * once per connected client. Subsequent templates are provided by waitNext().
191 *
192 * [@param](/bitcoin-bitcoin/contributor/param/) last_tip tip at the start of the cooldown window.
193- * [@param](/bitcoin-bitcoin/contributor/param/) interrupt set to true to interrupt the cooldown.
194 *
195 * [@returns](/bitcoin-bitcoin/contributor/returns/) false if interrupted.
196 */
197-bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip, bool& interrupt);
198+bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip);
199 } // namespace node
200
201 #endif // BITCOIN_NODE_MINER_H
