From 64a2795fd4fe223a55564c31e9fa36264e79ac22 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Feb 2025 10:28:43 +0100 Subject: [PATCH] rpc: handle shutdown during long poll and wait methods The waitTipChanged() now returns nullopt if the node is shutting down. Previously it would return the last known tip during shutdown, but this creates an ambiguous circumstance in the scenario where the node is started and quickly shutdown, before notifications().TipBlock() is set. The getblocktemplate, waitfornewblock and waitforblockheight RPC are updated to handle this. Existing behavior is preserved. Co-authored-by: Ryan Ofsky --- src/interfaces/mining.h | 7 +++--- src/node/interfaces.cpp | 28 ++++++++++++++++------- src/rpc/blockchain.cpp | 50 ++++++++++++++++++++++++++++------------- src/rpc/mining.cpp | 6 ++++- 4 files changed, 63 insertions(+), 28 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 249b9cefd4a..1e84969bc78 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -89,15 +89,16 @@ public: /** * Waits for the connected tip to change. During node initialization, this will - * wait until the tip is connected. + * wait until the tip is connected (regardless of `timeout`). * * @param[in] current_tip block hash of the current chain tip. Function waits * for the chain tip to differ from this. * @param[in] timeout how long to wait for a new tip (default is forever) * - * @returns Hash and height of the current chain tip after this call. + * @retval BlockRef hash and height of the current chain tip after this call. + * @retval std::nullopt if the node is shut down. */ - virtual BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0; + virtual std::optional waitTipChanged(uint256 current_tip, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0; /** * Construct a new block template diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 59639414212..e2507211327 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1070,22 +1070,34 @@ public: return BlockRef{tip->GetBlockHash(), tip->nHeight}; } - BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override + std::optional waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override { Assume(timeout >= 0ms); // No internal callers should use a negative timeout if (timeout < 0ms) timeout = 0ms; if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono + auto deadline{std::chrono::steady_clock::now() + timeout}; { WAIT_LOCK(notifications().m_tip_block_mutex, lock); - notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { - // We need to wait for m_tip_block to be set AND for the value - // to differ from the current_tip value. - return (notifications().TipBlock() && notifications().TipBlock() != current_tip) || chainman().m_interrupt; + // For callers convenience, wait longer than the provided timeout + // during startup for the tip to be non-null. That way this function + // always returns valid tip information when possible and only + // returns null when shutting down, not when timing out. + notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { + return notifications().TipBlock() || chainman().m_interrupt; + }); + if (chainman().m_interrupt) return {}; + // At this point TipBlock is set, so continue to wait until it is + // different then `current_tip` provided by caller. + notifications().m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { + return Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt; }); } - // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks. - LOCK(::cs_main); - return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight}; + + if (chainman().m_interrupt) return {}; + + // Must release m_tip_block_mutex before getTip() locks cs_main, to + // avoid deadlocks. + return getTip(); } std::unique_ptr createNewBlock(const BlockCreateOptions& options) override diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index f7f082af39a..6e5c656f3d8 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -64,6 +64,7 @@ using kernel::CCoinsStats; using kernel::CoinStatsHashType; +using interfaces::BlockRef; using interfaces::Mining; using node::BlockManager; using node::NodeContext; @@ -286,12 +287,17 @@ static RPCHelpMan waitfornewblock() NodeContext& node = EnsureAnyNodeContext(request.context); Mining& miner = EnsureMining(node); - auto block{CHECK_NONFATAL(miner.getTip()).value()}; - block = timeout ? miner.waitTipChanged(block.hash, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(block.hash); + // Abort if RPC came out of warmup too early + BlockRef current_block{CHECK_NONFATAL(miner.getTip()).value()}; + std::optional block = timeout ? miner.waitTipChanged(current_block.hash, std::chrono::milliseconds(timeout)) : + miner.waitTipChanged(current_block.hash); + + // Return current block upon shutdown + if (block) current_block = *block; UniValue ret(UniValue::VOBJ); - ret.pushKV("hash", block.hash.GetHex()); - ret.pushKV("height", block.height); + ret.pushKV("hash", current_block.hash.GetHex()); + ret.pushKV("height", current_block.height); return ret; }, }; @@ -330,22 +336,28 @@ static RPCHelpMan waitforblock() NodeContext& node = EnsureAnyNodeContext(request.context); Mining& miner = EnsureMining(node); - auto block{CHECK_NONFATAL(miner.getTip()).value()}; + // Abort if RPC came out of warmup too early + BlockRef current_block{CHECK_NONFATAL(miner.getTip()).value()}; + const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout}; - while (block.hash != hash) { + while (current_block.hash != hash) { + std::optional block; if (timeout) { auto now{std::chrono::steady_clock::now()}; if (now >= deadline) break; const MillisecondsDouble remaining{deadline - now}; - block = miner.waitTipChanged(block.hash, remaining); + block = miner.waitTipChanged(current_block.hash, remaining); } else { - block = miner.waitTipChanged(block.hash); + block = miner.waitTipChanged(current_block.hash); } + // Return current block upon shutdown + if (!block) break; + current_block = *block; } UniValue ret(UniValue::VOBJ); - ret.pushKV("hash", block.hash.GetHex()); - ret.pushKV("height", block.height); + ret.pushKV("hash", current_block.hash.GetHex()); + ret.pushKV("height", current_block.height); return ret; }, }; @@ -385,23 +397,29 @@ static RPCHelpMan waitforblockheight() NodeContext& node = EnsureAnyNodeContext(request.context); Mining& miner = EnsureMining(node); - auto block{CHECK_NONFATAL(miner.getTip()).value()}; + // Abort if RPC came out of warmup too early + BlockRef current_block{CHECK_NONFATAL(miner.getTip()).value()}; + const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout}; - while (block.height < height) { + while (current_block.height < height) { + std::optional block; if (timeout) { auto now{std::chrono::steady_clock::now()}; if (now >= deadline) break; const MillisecondsDouble remaining{deadline - now}; - block = miner.waitTipChanged(block.hash, remaining); + block = miner.waitTipChanged(current_block.hash, remaining); } else { - block = miner.waitTipChanged(block.hash); + block = miner.waitTipChanged(current_block.hash); } + // Return current block on shutdown + if (!block) break; + current_block = *block; } UniValue ret(UniValue::VOBJ); - ret.pushKV("hash", block.hash.GetHex()); - ret.pushKV("height", block.height); + ret.pushKV("hash", current_block.hash.GetHex()); + ret.pushKV("height", current_block.height); return ret; }, }; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index b527e686a46..ada1a153214 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -46,6 +46,7 @@ #include #include +using interfaces::BlockRef; using interfaces::BlockTemplate; using interfaces::Mining; using node::BlockAssembler; @@ -801,7 +802,10 @@ static RPCHelpMan getblocktemplate() { MillisecondsDouble checktxtime{std::chrono::minutes(1)}; while (tip == hashWatchedChain && IsRPCRunning()) { - tip = miner.waitTipChanged(hashWatchedChain, checktxtime).hash; + std::optional maybe_tip{miner.waitTipChanged(hashWatchedChain, checktxtime)}; + // Node is shutting down + if (!maybe_tip) break; + tip = maybe_tip->hash; // Timeout: Check transactions for update // without holding the mempool lock to avoid deadlocks if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP)