From f9cf8bd0ab77cdf125d78384197a5c466577fd8f Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Feb 2025 15:54:35 +0100 Subject: [PATCH 1/5] Handle negative timeout for waitTipChanged() --- src/interfaces/mining.h | 3 ++- src/node/interfaces.cpp | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 435151c5dda..249b9cefd4a 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -93,7 +93,8 @@ public: * * @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 + * @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. */ virtual BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 50207c658d8..59639414212 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1072,6 +1072,8 @@ public: BlockRef 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 { WAIT_LOCK(notifications().m_tip_block_mutex, lock); From a3bf43343f0d88ec9ff847a55fd48745aeebb429 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Feb 2025 12:44:03 +0100 Subject: [PATCH 2/5] rpc: drop unneeded IsRPCRunning() guards This was preventing the (hidden) waitfornewblock, waitforblock and waitforblockheight methods from being used in the GUI. The check was added in d6a5dc4a2eaa0d7348804254ca09e75fc3a858ab when these RPC methods were first introduced. They could have been dropped when dca923150e3ac10a57c23a7e29e76516d32ec10d refactored these methods to use waitTipChanged(), which already checks for shutdown. Making this change now simplifies the next commit. --- src/rpc/blockchain.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ac1ce6285f7..f7f082af39a 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -287,9 +287,7 @@ static RPCHelpMan waitfornewblock() Mining& miner = EnsureMining(node); auto block{CHECK_NONFATAL(miner.getTip()).value()}; - if (IsRPCRunning()) { - block = timeout ? miner.waitTipChanged(block.hash, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(block.hash); - } + block = timeout ? miner.waitTipChanged(block.hash, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(block.hash); UniValue ret(UniValue::VOBJ); ret.pushKV("hash", block.hash.GetHex()); @@ -334,7 +332,7 @@ static RPCHelpMan waitforblock() auto block{CHECK_NONFATAL(miner.getTip()).value()}; const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout}; - while (IsRPCRunning() && block.hash != hash) { + while (block.hash != hash) { if (timeout) { auto now{std::chrono::steady_clock::now()}; if (now >= deadline) break; @@ -390,7 +388,7 @@ static RPCHelpMan waitforblockheight() auto block{CHECK_NONFATAL(miner.getTip()).value()}; const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout}; - while (IsRPCRunning() && block.height < height) { + while (block.height < height) { if (timeout) { auto now{std::chrono::steady_clock::now()}; if (now >= deadline) break; From 64a2795fd4fe223a55564c31e9fa36264e79ac22 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Feb 2025 10:28:43 +0100 Subject: [PATCH 3/5] 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) From db14ca3556ca792546bf4343feb733271333690f Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 3 Feb 2025 18:11:00 +0100 Subject: [PATCH 4/5] Have createNewBlock() wait for a tip Additionally it returns null if the node started to shutdown before TipBlock() was set. --- src/interfaces/mining.h | 7 +++++-- src/node/interfaces.cpp | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 1e84969bc78..735071b5902 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -101,10 +101,13 @@ public: virtual std::optional waitTipChanged(uint256 current_tip, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0; /** - * Construct a new block template + * Construct a new block template. + * + * During node initialization, this will wait until the tip is connected. * * @param[in] options options for creating the block - * @returns a block template + * @retval BlockTemplate a block template. + * @retval std::nullptr if the node is shut down. */ virtual std::unique_ptr createNewBlock(const node::BlockCreateOptions& options = {}) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index e2507211327..8aec2758f8b 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1102,6 +1102,9 @@ public: std::unique_ptr createNewBlock(const BlockCreateOptions& options) override { + // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that. + if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {}; + BlockAssembler::Options assemble_options{options}; ApplyArgsManOptions(*Assert(m_node.args), assemble_options); return std::make_unique(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node); From cc1001f3bf17b31512c05fb359e09483a07fb2a3 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Feb 2025 10:51:55 +0100 Subject: [PATCH 5/5] rpc: clarify longpoll behavior Move the comparison to hashWatchedChain inside the while loop. Although this early return prevents the GetTransactionsUpdated() call in cases where the tip updates, it's only done to improve readability. The check itself is very cheap (although a more useful check might not be). Also add code comments. --- src/rpc/mining.cpp | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index ada1a153214..806771c5aeb 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -776,9 +776,22 @@ static RPCHelpMan getblocktemplate() static unsigned int nTransactionsUpdatedLast; const CTxMemPool& mempool = EnsureMemPool(node); - if (!lpval.isNull()) - { - // Wait to respond until either the best block changes, OR a minute has passed and there are more transactions + // Long Polling (BIP22) + if (!lpval.isNull()) { + /** + * Wait to respond until either the best block changes, OR there are more + * transactions. + * + * The check for new transactions first happens after 1 minute and + * subsequently every 10 seconds. BIP22 does not require this particular interval. + * On mainnet the mempool changes frequently enough that in practice this RPC + * returns after 60 seconds, or sooner if the best block changes. + * + * getblocktemplate is unlikely to be called by bitcoin-cli, so + * -rpcclienttimeout is not a concern. BIP22 recommends a long request timeout. + * + * The longpollid is assumed to be a tip hash if it has the right format. + */ uint256 hashWatchedChain; unsigned int nTransactionsUpdatedLastLP; @@ -787,6 +800,8 @@ static RPCHelpMan getblocktemplate() // Format: const std::string& lpstr = lpval.get_str(); + // Assume the longpollid is a block hash. If it's not then we return + // early below. hashWatchedChain = ParseHashV(lpstr.substr(0, 64), "longpollid"); nTransactionsUpdatedLastLP = LocaleIndependentAtoi(lpstr.substr(64)); } @@ -801,15 +816,20 @@ static RPCHelpMan getblocktemplate() LEAVE_CRITICAL_SECTION(cs_main); { MillisecondsDouble checktxtime{std::chrono::minutes(1)}; - while (tip == hashWatchedChain && IsRPCRunning()) { + while (IsRPCRunning()) { + // If hashWatchedChain is not a real block hash, this will + // return immediately. 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) + if (tip != hashWatchedChain) break; + + // Check transactions for update without holding the mempool + // lock to avoid deadlocks. + if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) { break; + } checktxtime = std::chrono::seconds(10); } }