Merge bitcoin/bitcoin#31283: Add waitNext() to BlockTemplate interface

cadbd4137d miner: have waitNext return after 20 min on testnet (Sjors Provoost)
d4020f502a Add waitNext() to BlockTemplate interface (Sjors Provoost)

Pull request description:

  This PR introduces `waitNext()`. It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.

  On testnet3 and testnet4 the difficulty drops after 20 minutes, so the second ensures that a new template is returned in that case.

  Alternative approach to #31003, suggested in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2451942362

ACKs for top commit:
  ryanofsky:
    Code review ACK cadbd4137d. Main change since last review is adding back a missing `m_interrupt` check in the waitNext loop. Also made various code cleanups in both commits.
  ismaelsadeeq:
    Code review ACK cadbd4137d
  vasild:
    ACK cadbd4137d

Tree-SHA512: c5a40053723c1c1674449ba1e4675718229a2022c8b0a4853b12a2c9180beb87536a1f99fde969a0ef099bca9ac69ca14ea4f399d277d2db7f556465ce47de95

src/interfaces/mining.h

@@ -7,7 +7,7 @@
 
 #include <consensus/amount.h>       // for CAmount
 #include <interfaces/types.h>       // for BlockRef
-#include <node/types.h>             // for BlockCreateOptions
+#include <node/types.h>             // for BlockCreateOptions, BlockWaitOptions
 #include <primitives/block.h>       // for CBlock, CBlockHeader
 #include <primitives/transaction.h> // for CTransactionRef
 #include <stdint.h>                 // for int64_t
@@ -56,6 +56,19 @@ public:
      * @returns if the block was processed, independent of block validity
      */
     virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) = 0;
+
+    /**
+     * Waits for fees in the next block to rise, a new tip or the timeout.
+     *
+     * @param[in] options   Control the timeout (default forever) and by how much total fees
+     *                      for the next block should rise (default infinite).
+     *
+     * @returns a new BlockTemplate or nothing if the timeout occurs.
+     *
+     * On testnet this will additionally return a template with difficulty 1 if
+     * the tip is more than 20 minutes old.
+     */
+    virtual std::unique_ptr<BlockTemplate> waitNext(const node::BlockWaitOptions options = {}) = 0;
 };
 
 //! Interface giving clients (RPC, Stratum v2 Template Provider in the future)

src/ipc/capnp/mining.capnp

@@ -31,6 +31,7 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
     getWitnessCommitmentIndex @7 (context: Proxy.Context) -> (result: Int32);
     getCoinbaseMerklePath @8 (context: Proxy.Context) -> (result: List(Data));
     submitSolution @9 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
+    waitNext @10 (context: Proxy.Context, options: BlockWaitOptions) -> (result: BlockTemplate);
 }
 
 struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
@@ -39,6 +40,11 @@ struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
     coinbaseOutputMaxAdditionalSigops @2 :UInt64 $Proxy.name("coinbase_output_max_additional_sigops");
 }
 
+struct BlockWaitOptions $Proxy.wrap("node::BlockWaitOptions") {
+    timeout @0 : Float64 $Proxy.name("timeout");
+    feeThreshold @1 : Int64 $Proxy.name("fee_threshold");
+}
+
 # Note: serialization of the BlockValidationState C++ type is somewhat fragile
 # and using the struct can be awkward. It would be good if testBlockValidity
 # method were changed to return validity information in a simpler format.

src/node/interfaces.cpp

@@ -82,6 +82,7 @@ using interfaces::Mining;
 using interfaces::Node;
 using interfaces::WalletLoader;
 using node::BlockAssembler;
+using node::BlockWaitOptions;
 using util::Join;
 
 namespace node {
@@ -877,7 +878,11 @@ public:
 class BlockTemplateImpl : public BlockTemplate
 {
 public:
-    explicit BlockTemplateImpl(std::unique_ptr<CBlockTemplate> block_template, NodeContext& node) : m_block_template(std::move(block_template)), m_node(node)
+    explicit BlockTemplateImpl(BlockAssembler::Options assemble_options,
+                               std::unique_ptr<CBlockTemplate> block_template,
+                               NodeContext& node) : m_assemble_options(std::move(assemble_options)),
+                                                    m_block_template(std::move(block_template)),
+                                                    m_node(node)
     {
         assert(m_block_template);
     }
@@ -942,9 +947,103 @@ public:
         return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
     }
 
+    std::unique_ptr<BlockTemplate> waitNext(BlockWaitOptions options) override
+    {
+        // Delay calculating the current template fees, just in case a new block
+        // comes in before the next tick.
+        CAmount current_fees = -1;
+
+        // Alternate waiting for a new tip and checking if fees have risen.
+        // The latter check is expensive so we only run it once per second.
+        auto now{NodeClock::now()};
+        const auto deadline = now + options.timeout;
+        const MillisecondsDouble tick{1000};
+        const bool allow_min_difficulty{chainman().GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};
+
+        do {
+            bool tip_changed{false};
+            {
+                WAIT_LOCK(notifications().m_tip_block_mutex, lock);
+                // Note that wait_until() checks the predicate before waiting
+                notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
+                    AssertLockHeld(notifications().m_tip_block_mutex);
+                    const auto tip_block{notifications().TipBlock()};
+                    // We assume tip_block is set, because this is an instance
+                    // method on BlockTemplate and no template could have been
+                    // generated before a tip exists.
+                    tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
+                    return tip_changed || chainman().m_interrupt;
+                });
+            }
+
+            if (chainman().m_interrupt) return nullptr;
+            // At this point the tip changed, a full tick went by or we reached
+            // the deadline.
+
+            // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
+            LOCK(::cs_main);
+
+            // On test networks return a minimum difficulty block after 20 minutes
+            if (!tip_changed && allow_min_difficulty) {
+                const NodeClock::time_point tip_time{std::chrono::seconds{chainman().ActiveChain().Tip()->GetBlockTime()}};
+                if (now > tip_time + 20min) {
+                    tip_changed = true;
+                }
+            }
+
+            /**
+             * We determine if fees increased compared to the previous template by generating
+             * a fresh template. There may be more efficient ways to determine how much
+             * (approximate) fees for the next block increased, perhaps more so after
+             * Cluster Mempool.
+             *
+             * We'll also create a new template if the tip changed during this iteration.
+             */
+            if (options.fee_threshold < MAX_MONEY || tip_changed) {
+                auto tmpl{std::make_unique<BlockTemplateImpl>(m_assemble_options,
+                                                              BlockAssembler{
+                                                                  chainman().ActiveChainstate(),
+                                                                  context()->mempool.get(),
+                                                                  m_assemble_options}
+                                                                  .CreateNewBlock(),
+                                                              m_node)};
+
+                // If the tip changed, return the new template regardless of its fees.
+                if (tip_changed) return tmpl;
+
+                // Calculate the original template total fees if we haven't already
+                if (current_fees == -1) {
+                    current_fees = 0;
+                    for (CAmount fee : m_block_template->vTxFees) {
+                        // Skip coinbase
+                        if (fee < 0) continue;
+                        current_fees += fee;
+                    }
+                }
+
+                CAmount new_fees = 0;
+                for (CAmount fee : tmpl->m_block_template->vTxFees) {
+                    // Skip coinbase
+                    if (fee < 0) continue;
+                    new_fees += fee;
+                    Assume(options.fee_threshold != MAX_MONEY);
+                    if (new_fees >= current_fees + options.fee_threshold) return tmpl;
+                }
+            }
+
+            now = NodeClock::now();
+        } while (now < deadline);
+
+        return nullptr;
+    }
+
+    const BlockAssembler::Options m_assemble_options;
+
     const std::unique_ptr<CBlockTemplate> m_block_template;
 
+    NodeContext* context() { return &m_node; }
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
+    KernelNotifications& notifications() { return *Assert(m_node.notifications); }
     NodeContext& m_node;
 };
 
@@ -991,7 +1090,7 @@ public:
     {
         BlockAssembler::Options assemble_options{options};
         ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
-        return std::make_unique<BlockTemplateImpl>(BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
+        return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
     }
 
     NodeContext* context() override { return &m_node; }

src/node/types.h

@@ -13,9 +13,11 @@
 #ifndef BITCOIN_NODE_TYPES_H
 #define BITCOIN_NODE_TYPES_H
 
+#include <consensus/amount.h>
 #include <cstddef>
 #include <policy/policy.h>
 #include <script/script.h>
+#include <util/time.h>
 
 namespace node {
 enum class TransactionError {
@@ -61,6 +63,28 @@ struct BlockCreateOptions {
      */
     CScript coinbase_output_script{CScript() << OP_TRUE};
 };
+
+struct BlockWaitOptions {
+    /**
+     * How long to wait before returning nullptr instead of a new template.
+     * Default is to wait forever.
+     */
+    MillisecondsDouble timeout{MillisecondsDouble::max()};
+
+    /**
+     * The wait method will not return a new template unless it has fees at
+     * least fee_threshold sats higher than the current template, or unless
+     * the chain tip changes and the previous template is no longer valid.
+     *
+     * A caller may not be interested in templates with higher fees, and
+     * determining whether fee_threshold is reached is also expensive. So as
+     * an optimization, when fee_threshold is set to MAX_MONEY (default), the
+     * implementation is able to be much more efficient, skipping expensive
+     * checks and only returning new templates when the chain tip changes.
+     */
+    CAmount fee_threshold{MAX_MONEY};
+};
+
 } // namespace node
 
 #endif // BITCOIN_NODE_TYPES_H

src/test/CMakeLists.txt

@@ -99,6 +99,7 @@ add_executable(test_bitcoin
   streams_tests.cpp
   sync_tests.cpp
   system_tests.cpp
+  testnet4_miner_tests.cpp
   timeoffsets_tests.cpp
   torcontrol_tests.cpp
   transaction_tests.cpp

src/test/miner_tests.cpp

@@ -180,8 +180,11 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse;
     Txid hashLowFeeTx = tx.GetHash();
     AddToMempool(tx_mempool, entry.Fee(feeToUse).FromTx(tx));
-    block_template = mining->createNewBlock(options);
+
-    BOOST_REQUIRE(block_template);
+    // waitNext() should return nullptr because there is no better template
+    auto should_be_nullptr = block_template->waitNext({.timeout = MillisecondsDouble{0}, .fee_threshold = 1});
+    BOOST_REQUIRE(should_be_nullptr == nullptr);
+
     block = block_template->getBlock();
     // Verify that the free tx and the low fee tx didn't get selected
     for (size_t i=0; i<block.vtx.size(); ++i) {
@@ -196,7 +199,9 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     tx.vout[0].nValue -= 2; // Now we should be just over the min relay fee
     hashLowFeeTx = tx.GetHash();
     AddToMempool(tx_mempool, entry.Fee(feeToUse + 2).FromTx(tx));
-    block_template = mining->createNewBlock(options);
+
+    // waitNext() should return if fees for the new template are at least 1 sat up
+    block_template = block_template->waitNext({.fee_threshold = 1});
     BOOST_REQUIRE(block_template);
     block = block_template->getBlock();
     BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U);
@@ -671,9 +676,15 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     for (const auto& bi : BLOCKINFO) {
         const int current_height{mining->getTip()->height};
 
-        // Simple block creation, nothing special yet:
+        /**
-        block_template = mining->createNewBlock(options);
+         * Simple block creation, nothing special yet.
-        BOOST_REQUIRE(block_template);
+         * If current_height is odd, block_template will have already been
+         * set at the end of the previous loop.
+         */
+        if (current_height % 2 == 0) {
+            block_template = mining->createNewBlock(options);
+            BOOST_REQUIRE(block_template);
+        }
 
         CBlock block{block_template->getBlock()};
         CMutableTransaction txCoinbase(*block.vtx[0]);
@@ -709,8 +720,13 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
             auto maybe_new_tip{Assert(m_node.chainman)->ActiveChain().Tip()};
             BOOST_REQUIRE_EQUAL(maybe_new_tip->GetBlockHash(), block.GetHash());
         }
-        // This just adds coverage
+        if (current_height % 2 == 0) {
-        mining->waitTipChanged(block.hashPrevBlock);
+            block_template = block_template->waitNext();
+            BOOST_REQUIRE(block_template);
+        } else {
+            // This just adds coverage
+            mining->waitTipChanged(block.hashPrevBlock);
+        }
     }
 
     LOCK(cs_main);

src/test/testnet4_miner_tests.cpp

@@ -0,0 +1,75 @@
+// Copyright (c) 2025 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <common/system.h>
+#include <interfaces/mining.h>
+#include <node/miner.h>
+#include <util/time.h>
+#include <validation.h>
+
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+using interfaces::BlockTemplate;
+using interfaces::Mining;
+using node::BlockAssembler;
+using node::BlockWaitOptions;
+
+namespace testnet4_miner_tests {
+
+struct Testnet4MinerTestingSetup : public Testnet4Setup {
+    std::unique_ptr<Mining> MakeMining()
+    {
+        return interfaces::MakeMining(m_node);
+    }
+};
+} // namespace testnet4_miner_tests
+
+BOOST_FIXTURE_TEST_SUITE(testnet4_miner_tests, Testnet4MinerTestingSetup)
+
+BOOST_AUTO_TEST_CASE(MiningInterface)
+{
+    auto mining{MakeMining()};
+    BOOST_REQUIRE(mining);
+
+    BlockAssembler::Options options;
+    std::unique_ptr<BlockTemplate> block_template;
+
+    // Set node time a few minutes past the testnet4 genesis block
+    const int64_t genesis_time{WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()->GetBlockTime())};
+    SetMockTime(genesis_time + 3 * 60);
+
+    block_template = mining->createNewBlock(options);
+    BOOST_REQUIRE(block_template);
+
+    // The template should use the mocked system time
+    BOOST_REQUIRE_EQUAL(block_template->getBlockHeader().nTime, genesis_time + 3 * 60);
+
+    const BlockWaitOptions wait_options{.timeout = MillisecondsDouble{0}, .fee_threshold = 1};
+
+    // waitNext() should return nullptr because there is no better template
+    auto should_be_nullptr = block_template->waitNext(wait_options);
+    BOOST_REQUIRE(should_be_nullptr == nullptr);
+
+    // This remains the case when exactly 20 minutes have gone by
+    {
+        LOCK(cs_main);
+        SetMockTime(m_node.chainman->ActiveChain().Tip()->GetBlockTime() + 20 * 60);
+    }
+    should_be_nullptr = block_template->waitNext(wait_options);
+    BOOST_REQUIRE(should_be_nullptr == nullptr);
+
+    // One second later the difficulty drops and it returns a new template
+    // Note that we can't test the actual difficulty change, because the
+    // difficulty is already at 1.
+    {
+        LOCK(cs_main);
+        SetMockTime(m_node.chainman->ActiveChain().Tip()->GetBlockTime() + 20 * 60 + 1);
+    }
+    block_template = block_template->waitNext(wait_options);
+    BOOST_REQUIRE(block_template);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/util/setup_common.h

@@ -130,6 +130,12 @@ struct RegTestingSetup : public TestingSetup {
         : TestingSetup{ChainType::REGTEST} {}
 };
 
+/** Identical to TestingSetup, but chain set to testnet4 */
+struct Testnet4Setup : public TestingSetup {
+    Testnet4Setup()
+        : TestingSetup{ChainType::TESTNET4} {}
+};
+
 class CBlock;
 struct CMutableTransaction;
 class CScript;