diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 435151c5dda..735071b5902 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -89,20 +89,25 @@ 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 - * @returns Hash and height of the current chain tip after this call. + * @param[in] timeout how long to wait for a new tip (default is forever) + * + * @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 + * 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 50207c658d8..8aec2758f8b 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1070,24 +1070,41 @@ 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 { + // 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); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ac1ce6285f7..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,14 +287,17 @@ static RPCHelpMan waitfornewblock() NodeContext& node = EnsureAnyNodeContext(request.context); 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); - } + // 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; }, }; @@ -332,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 (IsRPCRunning() && 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; }, }; @@ -387,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 (IsRPCRunning() && 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..806771c5aeb 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; @@ -775,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; @@ -786,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)); } @@ -800,12 +816,20 @@ static RPCHelpMan getblocktemplate() LEAVE_CRITICAL_SECTION(cs_main); { MillisecondsDouble checktxtime{std::chrono::minutes(1)}; - while (tip == hashWatchedChain && IsRPCRunning()) { - tip = miner.waitTipChanged(hashWatchedChain, checktxtime).hash; - // Timeout: Check transactions for update - // without holding the mempool lock to avoid deadlocks - if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) + 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; + 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); } }