From 89a8f74bbbb900abfb3d8e946eea18ad7b1513ad Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 12 Aug 2024 11:08:59 +0200 Subject: [PATCH 01/12] refactor: rename BlockKey to BlockRef --- src/index/base.cpp | 2 +- src/index/base.h | 5 +++-- src/index/blockfilterindex.cpp | 4 ++-- src/index/blockfilterindex.h | 4 ++-- src/index/coinstatsindex.cpp | 6 +++--- src/index/coinstatsindex.h | 4 ++-- src/interfaces/chain.h | 6 ------ src/interfaces/types.h | 20 ++++++++++++++++++++ 8 files changed, 33 insertions(+), 18 deletions(-) create mode 100644 src/interfaces/types.h diff --git a/src/index/base.cpp b/src/index/base.cpp index 6f9860415f3..810358394a1 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -113,7 +113,7 @@ bool BaseIndex::Init() // Child init const CBlockIndex* start_block = m_best_block_index.load(); - if (!CustomInit(start_block ? std::make_optional(interfaces::BlockKey{start_block->GetBlockHash(), start_block->nHeight}) : std::nullopt)) { + if (!CustomInit(start_block ? std::make_optional(interfaces::BlockRef{start_block->GetBlockHash(), start_block->nHeight}) : std::nullopt)) { return false; } diff --git a/src/index/base.h b/src/index/base.h index beb1575ab21..fbd9069a515 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -107,7 +108,7 @@ protected: void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override; /// Initialize internal state from the database and block index. - [[nodiscard]] virtual bool CustomInit(const std::optional& block) { return true; } + [[nodiscard]] virtual bool CustomInit(const std::optional& block) { return true; } /// Write update index entries for a newly connected block. [[nodiscard]] virtual bool CustomAppend(const interfaces::BlockInfo& block) { return true; } @@ -118,7 +119,7 @@ protected: /// Rewind index to an earlier chain tip during a chain reorg. The tip must /// be an ancestor of the current best block. - [[nodiscard]] virtual bool CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) { return true; } + [[nodiscard]] virtual bool CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip) { return true; } virtual DB& GetDB() const = 0; diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 26de7eee32f..a808cc90850 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -112,7 +112,7 @@ BlockFilterIndex::BlockFilterIndex(std::unique_ptr chain, Blo m_filter_fileseq = std::make_unique(std::move(path), "fltr", FLTR_FILE_CHUNK_SIZE); } -bool BlockFilterIndex::CustomInit(const std::optional& block) +bool BlockFilterIndex::CustomInit(const std::optional& block) { if (!m_db->Read(DB_FILTER_POS, m_next_filter_pos)) { // Check that the cause of the read failure is that the key does not exist. Any other errors @@ -316,7 +316,7 @@ bool BlockFilterIndex::Write(const BlockFilter& filter, uint32_t block_height, c return true; } -bool BlockFilterIndex::CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) +bool BlockFilterIndex::CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip) { CDBBatch batch(*m_db); std::unique_ptr db_it(m_db->NewIterator()); diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h index cdb9563fb8e..ccb4845ef5e 100644 --- a/src/index/blockfilterindex.h +++ b/src/index/blockfilterindex.h @@ -52,13 +52,13 @@ private: std::optional ReadFilterHeader(int height, const uint256& expected_block_hash); protected: - bool CustomInit(const std::optional& block) override; + bool CustomInit(const std::optional& block) override; bool CustomCommit(CDBBatch& batch) override; bool CustomAppend(const interfaces::BlockInfo& block) override; - bool CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) override; + bool CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip) override; BaseIndex::DB& GetDB() const LIFETIMEBOUND override { return *m_db; } diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index dff8e50a4e5..c950a18f3f8 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -265,7 +265,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) return true; } -bool CoinStatsIndex::CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) +bool CoinStatsIndex::CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip) { CDBBatch batch(*m_db); std::unique_ptr db_it(m_db->NewIterator()); @@ -304,7 +304,7 @@ bool CoinStatsIndex::CustomRewind(const interfaces::BlockKey& current_tip, const return true; } -static bool LookUpOne(const CDBWrapper& db, const interfaces::BlockKey& block, DBVal& result) +static bool LookUpOne(const CDBWrapper& db, const interfaces::BlockRef& block, DBVal& result) { // First check if the result is stored under the height index and the value // there matches the block hash. This should be the case if the block is on @@ -350,7 +350,7 @@ std::optional CoinStatsIndex::LookUpStats(const CBlockIndex& block_ return stats; } -bool CoinStatsIndex::CustomInit(const std::optional& block) +bool CoinStatsIndex::CustomInit(const std::optional& block) { if (!m_db->Read(DB_MUHASH, m_muhash)) { // Check that the cause of the read failure is that the key does not diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index d6322bfa7cf..885b9e0a860 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -43,13 +43,13 @@ private: bool AllowPrune() const override { return true; } protected: - bool CustomInit(const std::optional& block) override; + bool CustomInit(const std::optional& block) override; bool CustomCommit(CDBBatch& batch) override; bool CustomAppend(const interfaces::BlockInfo& block) override; - bool CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) override; + bool CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip) override; BaseIndex::DB& GetDB() const override { return *m_db; } diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index be596b17657..cf0d1d78a61 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -41,12 +41,6 @@ namespace interfaces { class Handler; class Wallet; -//! Hash/height pair to help track and identify blocks. -struct BlockKey { - uint256 hash; - int height = -1; -}; - //! Helper for findBlock to selectively return pieces of block data. If block is //! found, data will be returned by setting specified output variables. If block //! is not found, output variables will keep their previous values. diff --git a/src/interfaces/types.h b/src/interfaces/types.h new file mode 100644 index 00000000000..e5edd301a71 --- /dev/null +++ b/src/interfaces/types.h @@ -0,0 +1,20 @@ +// Copyright (c) 2024 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_INTERFACES_TYPES_H +#define BITCOIN_INTERFACES_TYPES_H + +#include + +namespace interfaces { + +//! Hash/height pair to help track and identify blocks. +struct BlockRef { + uint256 hash; + int height = -1; +}; + +} // namespace interfaces + +#endif // BITCOIN_INTERFACES_TYPES_H From ebb8215f23644f901c46fd4977b7d4b08fae5104 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 8 Jul 2024 19:08:11 +0200 Subject: [PATCH 02/12] Rename getTipHash() to getTip() and return BlockRef --- src/interfaces/mining.h | 5 +++-- src/node/interfaces.cpp | 6 ++++-- src/rpc/mining.cpp | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index c73465bd2aa..ebda49bcc95 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -6,6 +6,7 @@ #define BITCOIN_INTERFACES_MINING_H #include // for CAmount +#include // for BlockRef #include // for BlockCreateOptions #include // for CBlock, CBlockHeader #include // for CTransactionRef @@ -55,8 +56,8 @@ public: //! Returns whether IBD is still in progress. virtual bool isInitialBlockDownload() = 0; - //! Returns the hash for the tip of this chain - virtual std::optional getTipHash() = 0; + //! Returns the hash and height for the tip of this chain + virtual std::optional getTip() = 0; /** * Construct a new block template diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 510541dfda9..60ff6d406a2 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -67,6 +68,7 @@ #include +using interfaces::BlockRef; using interfaces::BlockTemplate; using interfaces::BlockTip; using interfaces::Chain; @@ -925,12 +927,12 @@ public: return chainman().IsInitialBlockDownload(); } - std::optional getTipHash() override + std::optional getTip() override { LOCK(::cs_main); CBlockIndex* tip{chainman().ActiveChain().Tip()}; if (!tip) return {}; - return tip->GetBlockHash(); + return BlockRef{tip->GetBlockHash(), tip->nHeight}; } bool processNewBlock(const std::shared_ptr& block, bool* new_block) override diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index b986c26e7a3..8f6828319ee 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -661,7 +661,7 @@ static RPCHelpMan getblocktemplate() ChainstateManager& chainman = EnsureChainman(node); Mining& miner = EnsureMining(node); LOCK(cs_main); - uint256 tip{CHECK_NONFATAL(miner.getTipHash()).value()}; + uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash}; std::string strMode = "template"; UniValue lpval = NullUniValue; @@ -776,7 +776,7 @@ static RPCHelpMan getblocktemplate() } ENTER_CRITICAL_SECTION(cs_main); - tip = CHECK_NONFATAL(miner.getTipHash()).value(); + tip = CHECK_NONFATAL(miner.getTip()).value().hash; if (!IsRPCRunning()) throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, "Shutting down"); From 7eccdaf16081d6f624c4dc21df75b0474e049d2b Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 26 Aug 2024 18:16:08 +0200 Subject: [PATCH 03/12] node: Track last block that received a blockTip notification Also signal m_tip_block_cv when StopRPC is called, for consistency with g_best_block_cv. This is handled in StopRPC instead of OnRPCStopped() because the latter is deleted in a later commit. Co-authored-by: TheCharlatan Co-authored-by: Ryan Ofsky --- src/init.cpp | 2 +- src/node/interfaces.cpp | 2 +- src/node/kernel_notifications.cpp | 6 ++++++ src/node/kernel_notifications.h | 12 +++++++++++- src/rpc/server.cpp | 8 ++++++-- src/rpc/server.h | 3 ++- 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index d6c80d8f84d..65575f26fcc 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -284,7 +284,7 @@ void Shutdown(NodeContext& node) StopHTTPRPC(); StopREST(); - StopRPC(); + StopRPC(&node); StopHTTPServer(); for (const auto& client : node.chain_clients) { client->flush(); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 60ff6d406a2..0b7685aa873 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -139,7 +139,7 @@ public: // Stop RPC for clean shutdown if any of waitfor* commands is executed. if (args().GetBoolArg("-server", false)) { InterruptRPC(); - StopRPC(); + StopRPC(m_context); } } bool shutdownRequested() override { return ShutdownRequested(*Assert(m_context)); }; diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 9894052a3a0..40d45c8c2b8 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -50,6 +50,12 @@ namespace node { kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index) { + { + LOCK(m_tip_block_mutex); + m_tip_block = index.GetBlockHash(); + m_tip_block_cv.notify_all(); + } + uiInterface.NotifyBlockTip(state, &index); if (m_stop_at_height && index.nHeight >= m_stop_at_height) { if (!m_shutdown()) { diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index e37f4d4e1e4..9ff980ea56d 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -7,6 +7,10 @@ #include +#include +#include +#include + #include #include @@ -34,7 +38,7 @@ public: KernelNotifications(util::SignalInterrupt& shutdown, std::atomic& exit_status, node::Warnings& warnings) : m_shutdown(shutdown), m_exit_status{exit_status}, m_warnings{warnings} {} - [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override; + [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override EXCLUSIVE_LOCKS_REQUIRED(!m_tip_block_mutex); void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) override; @@ -52,6 +56,12 @@ public: int m_stop_at_height{DEFAULT_STOPATHEIGHT}; //! Useful for tests, can be set to false to avoid shutdown on fatal error. bool m_shutdown_on_fatal_error{true}; + + Mutex m_tip_block_mutex; + std::condition_variable m_tip_block_cv; + //! The block for which the last blockTip notification was received for. + uint256 m_tip_block; + private: util::SignalInterrupt& m_shutdown; std::atomic& m_exit_status; diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 2c07a2ff087..fb50d453fa9 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -311,16 +312,19 @@ void InterruptRPC() }); } -void StopRPC() +void StopRPC(const std::any& context) { static std::once_flag g_rpc_stop_flag; // This function could be called twice if the GUI has been started with -server=1. assert(!g_rpc_running); - std::call_once(g_rpc_stop_flag, []() { + std::call_once(g_rpc_stop_flag, [&]() { LogDebug(BCLog::RPC, "Stopping RPC\n"); WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear()); DeleteAuthCookie(); g_rpcSignals.Stopped(); + node::NodeContext& node = EnsureAnyNodeContext(context); + // The notifications interface doesn't exist between initialization step 4a and 7. + if (node.notifications) node.notifications->m_tip_block_cv.notify_all(); }); } diff --git a/src/rpc/server.h b/src/rpc/server.h index 56e8a63088b..a13ae3b1e58 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -178,7 +179,7 @@ extern CRPCTable tableRPC; void StartRPC(); void InterruptRPC(); -void StopRPC(); +void StopRPC(const std::any& context); UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors); #endif // BITCOIN_RPC_SERVER_H From b94b27cf05c709674117e308e441a8d1efddcd0a Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 26 Aug 2024 11:30:24 +0200 Subject: [PATCH 04/12] Add waitTipChanged to Mining interface Co-authored-by: TheCharlatan --- src/interfaces/mining.h | 12 ++++++++++++ src/node/interfaces.cpp | 23 +++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index ebda49bcc95..421c5ba762f 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -12,6 +12,7 @@ #include // for CTransactionRef #include // for int64_t #include // for uint256 +#include // for MillisecondsDouble #include // for unique_ptr, shared_ptr #include // for optional @@ -60,6 +61,17 @@ public: virtual std::optional getTip() = 0; /** + * Waits for the tip to change + * + * @param[in] current_tip block hash of the current chain tip. Function waits + * for the chain tip to change if this matches, otherwise + * it returns right away. + * @param[in] timeout how long to wait for a new tip + * @returns Hash and height of the current chain tip after this call. + */ + virtual BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0; + + /** * Construct a new block template * * @param[in] script_pub_key the coinbase output diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 0b7685aa873..8d32ff5c2c2 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -935,6 +936,27 @@ public: return BlockRef{tip->GetBlockHash(), tip->nHeight}; } + BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override + { + // Interrupt check interval + const MillisecondsDouble tick{1000}; + auto now{std::chrono::steady_clock::now()}; + auto deadline = now + timeout; + // std::chrono does not check against overflow + if (deadline < now) deadline = std::chrono::steady_clock::time_point::max(); + { + WAIT_LOCK(notifications().m_tip_block_mutex, lock); + while ((notifications().m_tip_block == uint256() || notifications().m_tip_block == current_tip) && !chainman().m_interrupt) { + now = std::chrono::steady_clock::now(); + if (now >= deadline) break; + notifications().m_tip_block_cv.wait_until(lock, std::min(deadline, now + tick)); + } + } + // 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}; + } + bool processNewBlock(const std::shared_ptr& block, bool* new_block) override { return chainman().ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/new_block); @@ -967,6 +989,7 @@ public: NodeContext* context() override { return &m_node; } ChainstateManager& chainman() { return *Assert(m_node.chainman); } + KernelNotifications& notifications() { return *Assert(m_node.notifications); } NodeContext& m_node; }; } // namespace From 285fe9fb51c808a9edd91b05bd3134fc18de0fb6 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 8 Jul 2024 18:32:01 +0200 Subject: [PATCH 05/12] rpc: add test for waitforblock and waitfornewblock --- test/functional/rpc_blockchain.py | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 91e9975ad54..f02e6914ef5 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -90,6 +90,7 @@ class BlockchainTest(BitcoinTestFramework): self._test_getdifficulty() self._test_getnetworkhashps() self._test_stopatheight() + self._test_waitforblock() # also tests waitfornewblock self._test_waitforblockheight() self._test_getblock() self._test_getdeploymentinfo() @@ -507,6 +508,38 @@ class BlockchainTest(BitcoinTestFramework): self.start_node(0) assert_equal(self.nodes[0].getblockcount(), HEIGHT + 7) + def _test_waitforblock(self): + self.log.info("Test waitforblock and waitfornewblock") + node = self.nodes[0] + + current_height = node.getblock(node.getbestblockhash())['height'] + current_hash = node.getblock(node.getbestblockhash())['hash'] + + self.log.debug("Roll the chain back a few blocks and then reconsider it") + rollback_height = current_height - 100 + rollback_hash = node.getblockhash(rollback_height) + rollback_header = node.getblockheader(rollback_hash) + + node.invalidateblock(rollback_hash) + assert_equal(node.getblockcount(), rollback_height - 1) + + self.log.debug("waitforblock should return the same block after its timeout") + assert_equal(node.waitforblock(blockhash=current_hash, timeout=1)['hash'], rollback_header['previousblockhash']) + + node.reconsiderblock(rollback_hash) + # The chain has probably already been restored by the time reconsiderblock returns, + # but poll anyway. + self.wait_until(lambda: node.waitforblock(blockhash=current_hash, timeout=100)['hash'] == current_hash) + + # roll back again + node.invalidateblock(rollback_hash) + assert_equal(node.getblockcount(), rollback_height - 1) + + node.reconsiderblock(rollback_hash) + # The chain has probably already been restored by the time reconsiderblock returns, + # but poll anyway. + self.wait_until(lambda: node.waitfornewblock(timeout=100)['hash'] == current_hash) + def _test_waitforblockheight(self): self.log.info("Test waitforblockheight") node = self.nodes[0] From 77ec072925a6d558b3c6b075becbed44727c5989 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 8 Jul 2024 18:37:39 +0200 Subject: [PATCH 06/12] rpc: fix waitfornewblock description The waitforblock RPC method takes a hash argument and waits for that specific block. The waitfornewblock waits for any new block. This commit fixes the documentation. --- src/rpc/blockchain.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index a9cea44779d..6445fb29267 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -275,7 +275,7 @@ void RPCNotifyBlockChange(const CBlockIndex* pindex) static RPCHelpMan waitfornewblock() { return RPCHelpMan{"waitfornewblock", - "\nWaits for a specific new block and returns useful info about it.\n" + "\nWaits for any new block and returns useful info about it.\n" "\nReturns the current block on timeout or exit.\n", { {"timeout", RPCArg::Type::NUM, RPCArg::Default{0}, "Time in milliseconds to wait for a response. 0 indicates no timeout."}, From de7c855b3af99fe6b62279c5c2d08888a5437c4a Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 13 Aug 2024 11:51:26 +0200 Subject: [PATCH 07/12] rpc: recommend -rpcclienttimeout=0 for waitfor* --- src/rpc/blockchain.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 6445fb29267..7c6ab578ea4 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -276,7 +276,8 @@ static RPCHelpMan waitfornewblock() { return RPCHelpMan{"waitfornewblock", "\nWaits for any new block and returns useful info about it.\n" - "\nReturns the current block on timeout or exit.\n", + "\nReturns the current block on timeout or exit.\n" + "\nMake sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)", { {"timeout", RPCArg::Type::NUM, RPCArg::Default{0}, "Time in milliseconds to wait for a response. 0 indicates no timeout."}, }, @@ -318,7 +319,8 @@ static RPCHelpMan waitforblock() { return RPCHelpMan{"waitforblock", "\nWaits for a specific new block and returns useful info about it.\n" - "\nReturns the current block on timeout or exit.\n", + "\nReturns the current block on timeout or exit.\n" + "\nMake sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)", { {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Block hash to wait for."}, {"timeout", RPCArg::Type::NUM, RPCArg::Default{0}, "Time in milliseconds to wait for a response. 0 indicates no timeout."}, @@ -365,7 +367,8 @@ static RPCHelpMan waitforblockheight() return RPCHelpMan{"waitforblockheight", "\nWaits for (at least) block height and returns the height and hash\n" "of the current tip.\n" - "\nReturns the current block on timeout or exit.\n", + "\nReturns the current block on timeout or exit.\n" + "\nMake sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)", { {"height", RPCArg::Type::NUM, RPCArg::Optional::NO, "Block height to wait for."}, {"timeout", RPCArg::Type::NUM, RPCArg::Default{0}, "Time in milliseconds to wait for a response. 0 indicates no timeout."}, From 2a40ee1121903847cdd3f6c5b4796e4d5471b2df Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 8 Jul 2024 18:39:31 +0200 Subject: [PATCH 08/12] rpc: check for negative timeout arg in waitfor* --- src/rpc/blockchain.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 7c6ab578ea4..44334b7a390 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -296,6 +296,7 @@ static RPCHelpMan waitfornewblock() int timeout = 0; if (!request.params[0].isNull()) timeout = request.params[0].getInt(); + if (timeout < 0) throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout"); CUpdatedBlock block; { @@ -343,6 +344,7 @@ static RPCHelpMan waitforblock() if (!request.params[1].isNull()) timeout = request.params[1].getInt(); + if (timeout < 0) throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout"); CUpdatedBlock block; { @@ -391,6 +393,7 @@ static RPCHelpMan waitforblockheight() if (!request.params[1].isNull()) timeout = request.params[1].getInt(); + if (timeout < 0) throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout"); CUpdatedBlock block; { From dca923150e3ac10a57c23a7e29e76516d32ec10d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 12 Aug 2024 12:36:49 +0200 Subject: [PATCH 09/12] Replace RPCNotifyBlockChange with waitTipChanged() This refactoring commit uses the newly introduced waitTipChanged mining interface method to replace the RPCNotifyBlockChange mechanism. --- src/init.cpp | 9 ----- src/rpc/blockchain.cpp | 84 ++++++++++++++++++++---------------------- src/rpc/blockchain.h | 3 -- 3 files changed, 39 insertions(+), 57 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 65575f26fcc..0a013d8ccb6 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -429,16 +429,12 @@ static void registerSignalHandler(int signal, void(*handler)(int)) } #endif -static boost::signals2::connection rpc_notify_block_change_connection; static void OnRPCStarted() { - rpc_notify_block_change_connection = uiInterface.NotifyBlockTip_connect(std::bind(RPCNotifyBlockChange, std::placeholders::_2)); } static void OnRPCStopped() { - rpc_notify_block_change_connection.disconnect(); - RPCNotifyBlockChange(nullptr); g_best_block_cv.notify_all(); LogDebug(BCLog::RPC, "RPC stopped.\n"); } @@ -2011,11 +2007,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // cannot yet be called. Before we make it callable, we need to make sure // that the RPC's view of the best block is valid and consistent with // ChainstateManager's active tip. - // - // If we do not do this, RPC's view of the best block will be height=0 and - // hash=0x0. This will lead to erroroneous responses for things like - // waitforblockheight. - RPCNotifyBlockChange(WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip())); SetRPCWarmupFinished(); uiInterface.InitMessage(_("Done loading").translated); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 44334b7a390..683a9dae882 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -61,21 +62,12 @@ using kernel::CCoinsStats; using kernel::CoinStatsHashType; +using interfaces::Mining; using node::BlockManager; using node::NodeContext; using node::SnapshotMetadata; using util::MakeUnorderedList; -struct CUpdatedBlock -{ - uint256 hash; - int height; -}; - -static GlobalMutex cs_blockchange; -static std::condition_variable cond_blockchange; -static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); - std::tuple, CCoinsStats, const CBlockIndex*> PrepareUTXOSnapshot( Chainstate& chainstate, @@ -262,16 +254,6 @@ static RPCHelpMan getbestblockhash() }; } -void RPCNotifyBlockChange(const CBlockIndex* pindex) -{ - if(pindex) { - LOCK(cs_blockchange); - latestblock.hash = pindex->GetBlockHash(); - latestblock.height = pindex->nHeight; - } - cond_blockchange.notify_all(); -} - static RPCHelpMan waitfornewblock() { return RPCHelpMan{"waitfornewblock", @@ -298,16 +280,14 @@ static RPCHelpMan waitfornewblock() timeout = request.params[0].getInt(); if (timeout < 0) throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout"); - CUpdatedBlock block; - { - WAIT_LOCK(cs_blockchange, lock); - block = latestblock; - if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&block]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); - else - cond_blockchange.wait(lock, [&block]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); - block = latestblock; + 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); } + UniValue ret(UniValue::VOBJ); ret.pushKV("hash", block.hash.GetHex()); ret.pushKV("height", block.height); @@ -346,14 +326,20 @@ static RPCHelpMan waitforblock() timeout = request.params[1].getInt(); if (timeout < 0) throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout"); - CUpdatedBlock block; - { - WAIT_LOCK(cs_blockchange, lock); - if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.hash == hash || !IsRPCRunning();}); - else - cond_blockchange.wait(lock, [&hash]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.hash == hash || !IsRPCRunning(); }); - block = latestblock; + NodeContext& node = EnsureAnyNodeContext(request.context); + Mining& miner = EnsureMining(node); + + auto block{CHECK_NONFATAL(miner.getTip()).value()}; + const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout}; + while (IsRPCRunning() && block.hash != hash) { + if (timeout) { + auto now{std::chrono::steady_clock::now()}; + if (now >= deadline) break; + const MillisecondsDouble remaining{deadline - now}; + block = miner.waitTipChanged(block.hash, remaining); + } else { + block = miner.waitTipChanged(block.hash); + } } UniValue ret(UniValue::VOBJ); @@ -395,15 +381,23 @@ static RPCHelpMan waitforblockheight() timeout = request.params[1].getInt(); if (timeout < 0) throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout"); - CUpdatedBlock block; - { - WAIT_LOCK(cs_blockchange, lock); - if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height >= height || !IsRPCRunning();}); - else - cond_blockchange.wait(lock, [&height]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height >= height || !IsRPCRunning(); }); - block = latestblock; + NodeContext& node = EnsureAnyNodeContext(request.context); + Mining& miner = EnsureMining(node); + + auto block{CHECK_NONFATAL(miner.getTip()).value()}; + const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout}; + + while (IsRPCRunning() && block.height < height) { + if (timeout) { + auto now{std::chrono::steady_clock::now()}; + if (now >= deadline) break; + const MillisecondsDouble remaining{deadline - now}; + block = miner.waitTipChanged(block.hash, remaining); + } else { + block = miner.waitTipChanged(block.hash); + } } + UniValue ret(UniValue::VOBJ); ret.pushKV("hash", block.hash.GetHex()); ret.pushKV("height", block.height); diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h index a23f5cf024e..89b9921d559 100644 --- a/src/rpc/blockchain.h +++ b/src/rpc/blockchain.h @@ -35,9 +35,6 @@ static constexpr int NUM_GETBLOCKSTATS_PERCENTILES = 5; */ double GetDifficulty(const CBlockIndex& blockindex); -/** Callback for when block tip changed. */ -void RPCNotifyBlockChange(const CBlockIndex*); - /** Block description to JSON */ UniValue blockToJSON(node::BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity) LOCKS_EXCLUDED(cs_main); From 460687a09c2af336fce853d9ffb790d01429eec6 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 26 Aug 2024 18:25:55 +0200 Subject: [PATCH 10/12] Remove unused CRPCSignals They are no longer used for anything since RPCNotifyBlockChange was replaced with waitTipChanged() from the mining interface. --- src/init.cpp | 12 ------------ src/rpc/server.cpp | 23 +++-------------------- src/rpc/server.h | 6 ------ 3 files changed, 3 insertions(+), 38 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0a013d8ccb6..c9e2c281442 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -429,16 +429,6 @@ static void registerSignalHandler(int signal, void(*handler)(int)) } #endif -static void OnRPCStarted() -{ -} - -static void OnRPCStopped() -{ - g_best_block_cv.notify_all(); - LogDebug(BCLog::RPC, "RPC stopped.\n"); -} - void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) { SetupHelpOptions(argsman); @@ -719,8 +709,6 @@ static void StartupNotify(const ArgsManager& args) static bool AppInitServers(NodeContext& node) { const ArgsManager& args = *Assert(node.args); - RPCServer::OnStarted(&OnRPCStarted); - RPCServer::OnStopped(&OnRPCStopped); if (!InitHTTPServer(*Assert(node.shutdown))) { return false; } diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index fb50d453fa9..618199c42f2 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -19,8 +19,7 @@ #include #include #include - -#include +#include #include #include @@ -70,22 +69,6 @@ struct RPCCommandExecution } }; -static struct CRPCSignals -{ - boost::signals2::signal Started; - boost::signals2::signal Stopped; -} g_rpcSignals; - -void RPCServer::OnStarted(std::function slot) -{ - g_rpcSignals.Started.connect(slot); -} - -void RPCServer::OnStopped(std::function slot) -{ - g_rpcSignals.Stopped.connect(slot); -} - std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest& helpreq) const { std::string strRet; @@ -298,7 +281,6 @@ void StartRPC() { LogDebug(BCLog::RPC, "Starting RPC\n"); g_rpc_running = true; - g_rpcSignals.Started(); } void InterruptRPC() @@ -321,10 +303,11 @@ void StopRPC(const std::any& context) LogDebug(BCLog::RPC, "Stopping RPC\n"); WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear()); DeleteAuthCookie(); - g_rpcSignals.Stopped(); + g_best_block_cv.notify_all(); node::NodeContext& node = EnsureAnyNodeContext(context); // The notifications interface doesn't exist between initialization step 4a and 7. if (node.notifications) node.notifications->m_tip_block_cv.notify_all(); + LogDebug(BCLog::RPC, "RPC stopped.\n"); }); } diff --git a/src/rpc/server.h b/src/rpc/server.h index a13ae3b1e58..a8896d4247a 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -19,12 +19,6 @@ class CRPCCommand; -namespace RPCServer -{ - void OnStarted(std::function slot); - void OnStopped(std::function slot); -} - /** Query whether RPC is running */ bool IsRPCRunning(); From e3a560ca68d79b056a105a65ed0c174a9631aba9 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 26 Aug 2024 18:35:16 +0200 Subject: [PATCH 11/12] rpc: use waitTipChanged for longpoll This removes the last remaining use of g_best_block by the RPC. --- src/rpc/mining.cpp | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 8f6828319ee..6270496b1bc 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -738,7 +738,6 @@ static RPCHelpMan getblocktemplate() { // Wait to respond until either the best block changes, OR a minute has passed and there are more transactions uint256 hashWatchedChain; - std::chrono::steady_clock::time_point checktxtime; unsigned int nTransactionsUpdatedLastLP; if (lpval.isStr()) @@ -759,19 +758,14 @@ static RPCHelpMan getblocktemplate() // Release lock while waiting LEAVE_CRITICAL_SECTION(cs_main); { - checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1); - - WAIT_LOCK(g_best_block_mutex, lock); - while (g_best_block == hashWatchedChain && IsRPCRunning()) - { - if (g_best_block_cv.wait_until(lock, checktxtime) == std::cv_status::timeout) - { - // Timeout: Check transactions for update - // without holding the mempool lock to avoid deadlocks - if (miner.getTransactionsUpdated() != nTransactionsUpdatedLastLP) - break; - checktxtime += std::chrono::seconds(10); - } + 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 (miner.getTransactionsUpdated() != nTransactionsUpdatedLastLP) + break; + checktxtime = std::chrono::seconds(10); } } ENTER_CRITICAL_SECTION(cs_main); From 7942951e3fcc58f7db0059546d03be9ea243f1db Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 26 Aug 2024 18:28:07 +0200 Subject: [PATCH 12/12] Remove unused g_best_block --- src/rpc/server.cpp | 1 - src/test/validation_chainstate_tests.cpp | 13 +++++++------ src/validation.cpp | 10 ---------- src/validation.h | 5 ----- 4 files changed, 7 insertions(+), 22 deletions(-) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 618199c42f2..086f609ac3e 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -303,7 +303,6 @@ void StopRPC(const std::any& context) LogDebug(BCLog::RPC, "Stopping RPC\n"); WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear()); DeleteAuthCookie(); - g_best_block_cv.notify_all(); node::NodeContext& node = EnsureAnyNodeContext(context); // The notifications interface doesn't exist between initialization step 4a and 7. if (node.notifications) node.notifications->m_tip_block_cv.notify_all(); diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 30c5982b17d..702af6578dc 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -4,6 +4,7 @@ // #include #include +#include #include #include #include @@ -69,14 +70,14 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches) BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) { ChainstateManager& chainman = *Assert(m_node.chainman); - uint256 curr_tip = ::g_best_block; + uint256 curr_tip = m_node.notifications->m_tip_block; // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can // be found. mineBlocks(10); // After adding some blocks to the tip, best block should have changed. - BOOST_CHECK(::g_best_block != curr_tip); + BOOST_CHECK(m_node.notifications->m_tip_block != curr_tip); // Grab block 1 from disk; we'll add it to the background chain later. std::shared_ptr pblockone = std::make_shared(); @@ -91,15 +92,15 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) // Ensure our active chain is the snapshot chainstate. BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.IsSnapshotActive())); - curr_tip = ::g_best_block; + curr_tip = m_node.notifications->m_tip_block; // Mine a new block on top of the activated snapshot chainstate. mineBlocks(1); // Defined in TestChain100Setup. // After adding some blocks to the snapshot tip, best block should have changed. - BOOST_CHECK(::g_best_block != curr_tip); + BOOST_CHECK(m_node.notifications->m_tip_block != curr_tip); - curr_tip = ::g_best_block; + curr_tip = m_node.notifications->m_tip_block; BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2); @@ -138,7 +139,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) // g_best_block should be unchanged after adding a block to the background // validation chain. BOOST_CHECK(block_added); - BOOST_CHECK_EQUAL(curr_tip, ::g_best_block); + BOOST_CHECK_EQUAL(curr_tip, m_node.notifications->m_tip_block); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 8e4ea8eda2f..ef9d3e7cd16 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -108,10 +108,6 @@ const std::vector CHECKLEVEL_DOC { * */ static constexpr int PRUNE_LOCK_BUFFER{10}; -GlobalMutex g_best_block_mutex; -std::condition_variable g_best_block_cv; -uint256 g_best_block; - const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locator) const { AssertLockHeld(cs_main); @@ -2988,12 +2984,6 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew) m_mempool->AddTransactionsUpdated(1); } - { - LOCK(g_best_block_mutex); - g_best_block = pindexNew->GetBlockHash(); - g_best_block_cv.notify_all(); - } - std::vector warning_messages; if (!m_chainman.IsInitialBlockDownload()) { const CBlockIndex* pindex = pindexNew; diff --git a/src/validation.h b/src/validation.h index 6bc8a14de95..f6aeea3faaf 100644 --- a/src/validation.h +++ b/src/validation.h @@ -85,11 +85,6 @@ enum class SynchronizationState { POST_INIT }; -extern GlobalMutex g_best_block_mutex; -extern std::condition_variable g_best_block_cv; -/** Used to notify getblocktemplate RPC of new tips. */ -extern uint256 g_best_block; - /** Documentation for argument 'checklevel'. */ extern const std::vector CHECKLEVEL_DOC;