re: #34184 (review)
I mostly just cloned the pattern we used for waitNext(). Does it have a similar issue?
You are right, I misremembered the way interruptWait was implemented and it does follow a similar pattern, and the issue is not as bad as I thought.
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.
<details><summary>diff</summary>
<p>
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -929,7 +929,7 @@ public:
void interruptWait() override
{
- InterruptWait(notifications(), m_interrupt_wait);
+ InterruptWait(notifications(), &m_interrupt_wait);
}
const BlockAssembler::Options m_assemble_options;
@@ -964,7 +964,7 @@ public:
std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
{
- return WaitTipChanged(chainman(), notifications(), current_tip, timeout, m_interrupt);
+ return WaitTipChanged(chainman(), notifications(), current_tip, timeout);
}
std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
@@ -976,7 +976,7 @@ public:
// Avoid generating a new template immediately after connecting a block while
// our best known header chain still has more work. This prevents a burst of
// block templates during the final catch-up moments after IBD.
- if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt)) return {};
+ if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip)) return {};
BlockAssembler::Options assemble_options{options};
ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
@@ -985,7 +985,7 @@ public:
void interrupt() override
{
- InterruptWait(notifications(), m_interrupt);
+ InterruptWait(notifications());
}
bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
@@ -1000,7 +1000,6 @@ public:
NodeContext* context() override { return &m_node; }
ChainstateManager& chainman() { return *Assert(m_node.chainman); }
KernelNotifications& notifications() { return *Assert(m_node.notifications); }
- bool m_interrupt{false};
NodeContext& m_node;
};
} // namespace
--- a/src/node/kernel_notifications.h
+++ b/src/node/kernel_notifications.h
@@ -61,6 +61,14 @@ public:
//! Might be unset during an early shutdown.
std::optional<uint256> TipBlock() EXCLUSIVE_LOCKS_REQUIRED(m_tip_block_mutex);
+ //! Whether a createNewBlock() / waitNext() cancellation has been requested.
+ //! Having this variable here hacky, but avoids the problem of
+ //! createNewBlock not being interruptible during IBD by mining clients, and
+ //! avoids memory safety issues that would occur if the variable was
+ //! somewhere else. An idea for getting rid of this is described in
+ //! [#34184 (review)](/bitcoin-bitcoin/34184/#discussion_r2742950202)
+ bool m_blocktemplate_request_interrupted GUARDED_BY(m_tip_block_mutex);
+
private:
const std::function<bool()>& m_shutdown_request;
std::atomic<int>& m_exit_status;
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -341,13 +341,27 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
block.fChecked = false;
}
-void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait)
+void InterruptWait(KernelNotifications& kernel_notifications, bool* interrupt_wait)
{
LOCK(kernel_notifications.m_tip_block_mutex);
- interrupt_wait = true;
+ kernel_notifications.m_blocktemplate_request_interrupted = true;
+ if (interrupt_wait) *interrupt_wait = true;
kernel_notifications.m_tip_block_cv.notify_all();
}
+static bool Interrupted(const KernelNotifications& kernel_notifications, const bool* interrupt_wait = nullptr) EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex)
+{
+ return kernel_notifications.m_blocktemplate_request_interrupted || (interrupt_wait && *interrupt_wait);
+}
+
+static bool ResetInterrupt(KernelNotifications& kernel_notifications, bool* interrupt_wait = nullptr) EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex)
+{
+ if (!Interrupted(kernel_notifications, interrupt_wait)) return false;
+ kernel_notifications.m_blocktemplate_request_interrupted = false;
+ if (interrupt_wait) *interrupt_wait = false;
+ return true;
+}
+
std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
KernelNotifications& kernel_notifications,
CTxMemPool* mempool,
@@ -379,12 +393,9 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
// method on BlockTemplate and no template could have been
// generated before a tip exists.
tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock;
- return tip_changed || chainman.m_interrupt || interrupt_wait;
+ return tip_changed || chainman.m_interrupt || Interrupted(kernel_notifications, &interrupt_wait);
});
- if (interrupt_wait) {
- interrupt_wait = false;
- return nullptr;
- }
+ if (ResetInterrupt(kernel_notifications, &interrupt_wait)) return nullptr;
}
if (chainman.m_interrupt) return nullptr;
@@ -445,7 +456,7 @@ std::optional<BlockRef> GetTip(ChainstateManager& chainman)
return BlockRef{tip->GetBlockHash(), tip->nHeight};
}
-bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip, bool& interrupt)
+bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip)
{
constexpr auto COOLDOWN{1s};
// Use a steady clock so cooldown is independent of mocktime.
@@ -455,12 +466,9 @@ bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& ke
WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
kernel_notifications.m_tip_block_cv.wait_until(lock, cooldown_deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
const auto tip_block = kernel_notifications.TipBlock();
- return chainman.m_interrupt || interrupt || (tip_block && *tip_block != last_tip_hash);
+ return chainman.m_interrupt || Interrupted(kernel_notifications) || (tip_block && *tip_block != last_tip_hash);
});
- if (chainman.m_interrupt || interrupt) {
- interrupt = false;
- return false;
- }
+ if (chainman.m_interrupt || ResetInterrupt(kernel_notifications)) return false;
// If the tip changed during the wait, extend the deadline
const auto tip_block = kernel_notifications.TipBlock();
@@ -477,7 +485,7 @@ bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& ke
return true;
}
-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt)
+std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout)
{
Assume(timeout >= 0ms); // No internal callers should use a negative timeout
if (timeout < 0ms) timeout = 0ms;
@@ -490,21 +498,15 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
// always returns valid tip information when possible and only
// returns null when shutting down, not when timing out.
kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
- return kernel_notifications.TipBlock() || chainman.m_interrupt || interrupt;
+ return kernel_notifications.TipBlock() || chainman.m_interrupt || Interrupted(kernel_notifications);
});
- if (chainman.m_interrupt || interrupt) {
- interrupt = false;
- return {};
- }
+ if (chainman.m_interrupt || ResetInterrupt(kernel_notifications)) return {};
// At this point TipBlock is set, so continue to wait until it is
// different then `current_tip` provided by caller.
kernel_notifications.m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
- return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt || interrupt;
+ return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt || Interrupted(kernel_notifications);
});
- }
- if (chainman.m_interrupt || interrupt) {
- interrupt = false;
- return {};
+ if (chainman.m_interrupt || ResetInterrupt(kernel_notifications)) return {};
}
// Must release m_tip_block_mutex before getTip() locks cs_main, to
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -143,7 +143,7 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
/* Interrupt a blocking call. */
-void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait);
+void InterruptWait(KernelNotifications& kernel_notifications, bool* interrupt_wait = nullptr);
/**
* Return a new block template when fees rise to a certain threshold or after a
* new tip; return nullopt if timeout is reached.
@@ -163,7 +163,7 @@ std::optional<BlockRef> GetTip(ChainstateManager& chainman);
* Returns the current tip, or nullopt if the node is shutting down or interrupt()
* is called.
*/
-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt);
+std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout);
/**
* Wait while the best known header extends the current chain tip AND at least
@@ -177,11 +177,10 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
* once per connected client. Subsequent templates are provided by waitNext().
*
* [@param](/bitcoin-bitcoin/contributor/param/) last_tip tip at the start of the cooldown window.
- * [@param](/bitcoin-bitcoin/contributor/param/) interrupt set to true to interrupt the cooldown.
*
* [@returns](/bitcoin-bitcoin/contributor/returns/) false if interrupted.
*/
-bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip, bool& interrupt);
+bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const BlockRef& last_tip);
} // namespace node
#endif // BITCOIN_NODE_MINER_H
</p>
</details>
