Merge bitcoin/bitcoin#30409: Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block

7942951 Remove unused g_best_block (Ryan Ofsky)
e3a560c rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a Remove unused CRPCSignals (Sjors Provoost)
dca9231 Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1 rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855 rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9f rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27c Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74 refactor: rename BlockKey to BlockRef (Sjors Provoost)

Pull request description:

  This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.

  `waitTipChanged()` uses a new kernel notification `notifications().m_tip_block_mutex`, which this PR also introduces (a previous version used `g_best_block`).

  In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.

  There's a commit to add (direct) tests for the methods that are about to be refactored:
  - `waitfornewblock` was already implicitly tested by `feature_shutdown.py`.
  - `waitforblockheight` by `feature_coinstatsindex.py` and `example_test.py`

  This PR renames `getTipHash()` to `getTip()` and returns a `BlockRef` (renamed from `BlockKey`) so that callers can use either the height or hash.

  The later commits make trivial improvements to the `waitfor*` RPC calls (not needed for this PR).

  The `waitTipChanged()` method could probably also be used for the longpoll functionality in `getblocktemplate`, but I'm a bit reluctant to touch that.

  `RPCServer::OnStarted` no longer does anything and `RPCServer::OnStopped` merely prints a log statement. They were added in #5711 as a refactor. This PR drops them entirely.

  Finally `g_best_block` is also dropped.

ACKs for top commit:
  achow101:
    ACK 7942951
  ryanofsky:
    Code review ACK 7942951. Just rebased since last review
  TheCharlatan:
    Re-ACK 7942951

Tree-SHA512: a5559446b4000c95e07aad33284b7ee2e57aafd87e1ae778b3825d59689566d047a8047e47a10f76e6e341e7dc72fd265a65afbc0a9c011d17c4cafd55031837

Author: achow101

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;
     }
 

src/index/base.h

@@ -7,6 +7,7 @@
 
 #include <dbwrapper.h>
 #include <interfaces/chain.h>
+#include <interfaces/types.h>
 #include <util/string.h>
 #include <util/threadinterrupt.h>
 #include <validationinterface.h>
@@ -107,7 +108,7 @@ class BaseIndex : public CValidationInterface
     void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override;
 
     /// Initialize internal state from the database and block index.
-    [[nodiscard]] virtual bool CustomInit(const std::optional<interfaces::BlockKey>& block) { return true; }
+    [[nodiscard]] virtual bool CustomInit(const std::optional<interfaces::BlockRef>& 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 @@ class BaseIndex : public CValidationInterface
 
     /// 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;
 

src/index/blockfilterindex.cpp

@@ -112,7 +112,7 @@ BlockFilterIndex::BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, Blo
     m_filter_fileseq = std::make_unique<FlatFileSeq>(std::move(path), "fltr", FLTR_FILE_CHUNK_SIZE);
 }
 
-bool BlockFilterIndex::CustomInit(const std::optional<interfaces::BlockKey>& block)
+bool BlockFilterIndex::CustomInit(const std::optional<interfaces::BlockRef>& 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<CDBIterator> db_it(m_db->NewIterator());

src/index/blockfilterindex.h

@@ -52,13 +52,13 @@ class BlockFilterIndex final : public BaseIndex
     std::optional<uint256> ReadFilterHeader(int height, const uint256& expected_block_hash);
 
 protected:
-    bool CustomInit(const std::optional<interfaces::BlockKey>& block) override;
+    bool CustomInit(const std::optional<interfaces::BlockRef>& 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; }
 

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<CDBIterator> 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<CCoinsStats> CoinStatsIndex::LookUpStats(const CBlockIndex& block_
     return stats;
 }
 
-bool CoinStatsIndex::CustomInit(const std::optional<interfaces::BlockKey>& block)
+bool CoinStatsIndex::CustomInit(const std::optional<interfaces::BlockRef>& block)
 {
     if (!m_db->Read(DB_MUHASH, m_muhash)) {
         // Check that the cause of the read failure is that the key does not

src/index/coinstatsindex.h

@@ -43,13 +43,13 @@ class CoinStatsIndex final : public BaseIndex
     bool AllowPrune() const override { return true; }
 
 protected:
-    bool CustomInit(const std::optional<interfaces::BlockKey>& block) override;
+    bool CustomInit(const std::optional<interfaces::BlockRef>& 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; }
 

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();
@@ -429,20 +429,6 @@ 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");
-}
-
 void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
 {
     SetupHelpOptions(argsman);
@@ -723,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;
     }
@@ -2017,11 +2001,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);

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.

src/interfaces/mining.h

@@ -6,11 +6,13 @@
 #define BITCOIN_INTERFACES_MINING_H
 
 #include <consensus/amount.h>       // for CAmount
+#include <interfaces/types.h>       // for BlockRef
 #include <node/types.h>             // for BlockCreateOptions
 #include <primitives/block.h>       // for CBlock, CBlockHeader
 #include <primitives/transaction.h> // for CTransactionRef
 #include <stdint.h>                 // for int64_t
 #include <uint256.h>                // for uint256
+#include <util/time.h>              // for MillisecondsDouble
 
 #include <memory>   // for unique_ptr, shared_ptr
 #include <optional> // for optional
@@ -55,10 +57,21 @@ class Mining
     //! Returns whether IBD is still in progress.
     virtual bool isInitialBlockDownload() = 0;
 
-    //! Returns the hash for the tip of this chain
-    virtual std::optional<uint256> getTipHash() = 0;
+    //! Returns the hash and height for the tip of this chain
+    virtual std::optional<BlockRef> 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

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 <uint256.h>
+
+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