mining: Break compatibility with existing IPC mining clients

[ryanofsky]

This PR increments the field number of the Init.makeMining method and makes the old makeMining method return an error, so IPC mining clients not using the latest schema file will get an error and not be able to access the Mining interface.

Normally, there shouldn't be a need to break compatibility this way, but the mining interface has evolved a lot since it was first introduced, with old clients using the original methods less stable and performant than newer clients. So now is a good time to introduce a cutoff, drop deprecated methods, and stop supporting old clients which can't function as well.

Bumping the field number is also an opportunity to make other improvements that would be awkward to implement compatibly:

• Making Cap'n Proto default parameter and field values match default values of corresponding C++ methods and structs.
• Adding missing Context parameters to Mining.createNewBlock and checkBlock methods so these methods will be executed on separate execution threads and not block the Cap'n Proto event loop thread.

More details about these changes are in the commit messages.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, enirox001, ismaelsadeeq, sedited

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34422 (Update libmultiprocess subtree to be more stable with rust IPC client by ryanofsky)
• #34284 (ipc, test: Add tests for unclean disconnect and thread busy behavior by ryanofsky)
• #34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
• #34038 (logging: replace -loglevel with -trace, various API improvements by ajtowns)
• #33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
• #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
• #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
• #29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21933214675/job/63341541522
LLM reason (✨ experimental): Linting failed: ruff found an unused import (CTransaction) in test_framework/ipc_util.py, causing the lint step to fail.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being
  incompatible with the current code in the target branch). If so, make sure to rebase on the latest
  commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the
  affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[Sjors]

Concept ACK

Thanks, maybe split out a separate commit that drops the deprecated methods (first).

If #34184 lands first, I would suggest adding a separate commit to switch its default to true, which will churn quite a few tests but should nonetheless be easy to review.

[ryanofsky]

Updated e555eb8 -> 7730911 (pr/mbreak.1 -> pr/mbreak.2, compare) fixing lint error (https://github.com/bitcoin/bitcoin/actions/runs/21933214675/job/63341541522), splitting commits, adding release notes, and adding checks to make sure c++/capnp constant values are consistent

[Sjors]

Can you try absorbing eaa1209 from #34184? It creates a non-trivial issue that distracts from the cooldown PR core: #34184 (comment)

[ryanofsky]

Thanks, maybe split out a separate commit that drops the deprecated methods (first).

Yeah commit was definitely too big and is spilt up now. I do feel like it is good to make backwards incompatible changes atomically, so you can check out any commit in bitcoin core history and build bitcoin-node binary and check out any other commit in bitcoin core history and use the schema files to build an external client, and IPC calls made by that client will succeed or fail but they will never send corrupt parameters & return values.

This would mean practically that you should never change a default value without bumping the mining interface number in the same commit, which should be a reasonable rule to follow.

If #34184 lands first, I would suggest adding a separate commit to switch its default to true, which will churn quite a few tests but should nonetheless be easy to review.

Can do this but I think I'm missing something. if you think true is an more appropriate default, why not just set the default value to true in #34184? Would this break something?

[Sjors]

why not just set the default value to true in #34184? Would this break something?

It would create a lot of churn which makes review in that PR more difficult. I'd rather churn in a refactor PR like this.

[ryanofsky]

Can you try absorbing eaa1209 from #34184? It creates a non-trivial issue that distracts from the cooldown PR core: #34184 (comment)

I think all the changes there are incorporated here. That commit was my starting point for this PR. But let me know if anything is missing

[ryanofsky]

why not just set the default value to true in #34184? Would this break something?

It would create a lot of churn which makes review in that PR more difficult. I'd rather churn in a refactor PR like this.

Is that true? If it's an entirely new parameter I'd think it would just require adding , false to a few existing calls.

Another thing I was wondering about is if this PR should get rid of the waitTipChanged method or if that method is still useful even with waitNext

[Sjors]

I prefer to keep waitTipChanged(), it's potentially useful before you create a template or even for applications that don't need a template.

[Sjors]

just require adding , false to a few existing calls

All of the tests (functional, unit, fuzz, bench) have to opt out of cooldown. Though I haven't tried to see how big the actual diff is.

---

Update: it's a big diff, though perhaps can be shortened because not all tests will break if use cooldown.

diff

diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h
index 14327c01c0..2c7aac7b0f 100644
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -151,11 +151,11 @@ public:
      *                     tip to catch up. Recommended, except for regtest and
      *                     signets with only one miner.
      * @retval BlockTemplate a block template.
      * @retval std::nullptr if the node is shut down or interrupt() is called.
      */
-    virtual std::unique_ptr<BlockTemplate> createNewBlock(const node::BlockCreateOptions& options = {}, bool cooldown = false) = 0;
+    virtual std::unique_ptr<BlockTemplate> createNewBlock(const node::BlockCreateOptions& options = {}, bool cooldown = true) = 0;

     /**
      * Interrupts createNewBlock and waitTipChanged.
      */
     virtual void interrupt() = 0;
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index ee80b90324..d8a9690481 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -163,11 +163,11 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock&& block, uint64_t&

 static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const CScript& coinbase_output_script, int nGenerate, uint64_t nMaxTries)
 {
     UniValue blockHashes(UniValue::VARR);
     while (nGenerate > 0 && !chainman.m_interrupt) {
-        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({ .coinbase_output_script = coinbase_output_script, .include_dummy_extranonce = true }));
+        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({ .coinbase_output_script = coinbase_output_script, .include_dummy_extranonce = true }, /*cooldown=*/false));
         CHECK_NONFATAL(block_template);

         std::shared_ptr<const CBlock> block_out;
         if (!GenerateBlock(chainman, block_template->getBlock(), nMaxTries, block_out, /*process_new_block=*/true)) {
             break;
@@ -374,11 +374,11 @@ static RPCHelpMan generateblock()

     ChainstateManager& chainman = EnsureChainman(node);
     {
         LOCK(chainman.GetMutex());
         {
-            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({.use_mempool = false, .coinbase_output_script = coinbase_output_script, .include_dummy_extranonce = true})};
+            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({.use_mempool = false, .coinbase_output_script = coinbase_output_script, .include_dummy_extranonce = true}, /*cooldown=*/false)};
             CHECK_NONFATAL(block_template);

             block = block_template->getBlock();
         }

@@ -869,11 +869,11 @@ static RPCHelpMan getblocktemplate()
         nTransactionsUpdatedLast = mempool.GetTransactionsUpdated();
         CBlockIndex* pindexPrevNew = chainman.m_blockman.LookupBlockIndex(tip);
         time_start = GetTime();

         // Create new block
-        block_template = miner.createNewBlock({.include_dummy_extranonce = true});
+        block_template = miner.createNewBlock({.include_dummy_extranonce = true}, /*cooldown=*/false);
         CHECK_NONFATAL(block_template);


         // Need to update only after we know createNewBlock succeeded
         pindexPrev = pindexPrevNew;
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index 3b4beebeff..7f05e4e9d8 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -120,55 +120,55 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const

     LOCK(tx_mempool.cs);
     BOOST_CHECK(tx_mempool.size() == 0);

     // Block template should only have a coinbase when there's nothing in the mempool
-    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
+    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
     CBlock block{block_template->getBlock()};
     BOOST_REQUIRE_EQUAL(block.vtx.size(), 1U);

     // waitNext() on an empty mempool 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);

     // Unless fee_threshold is 0
     block_template = block_template->waitNext({.timeout = MillisecondsDouble{0}, .fee_threshold = 0});
     BOOST_REQUIRE(block_template);

     // Test the ancestor feerate transaction selection.
     TestMemPoolEntryHelper entry;

     // Test that a medium fee transaction will be selected after a higher fee
     // rate package with a low fee rate parent.
     CMutableTransaction tx;
     tx.vin.resize(1);
     tx.vin[0].scriptSig = CScript() << OP_1;
     tx.vin[0].prevout.hash = txFirst[0]->GetHash();
     tx.vin[0].prevout.n = 0;
     tx.vout.resize(1);
     tx.vout[0].nValue = 5000000000LL - 1000;
     // This tx has a low fee: 1000 satoshis
     Txid hashParentTx = tx.GetHash(); // save this txid for later use
     const auto parent_tx{entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
     TryAddToMempool(tx_mempool, parent_tx);

     // This tx has a medium fee: 10000 satoshis
     tx.vin[0].prevout.hash = txFirst[1]->GetHash();
     tx.vout[0].nValue = 5000000000LL - 10000;
     Txid hashMediumFeeTx = tx.GetHash();
     const auto medium_fee_tx{entry.Fee(10000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
     TryAddToMempool(tx_mempool, medium_fee_tx);

     // This tx has a high fee, but depends on the first transaction
     tx.vin[0].prevout.hash = hashParentTx;
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee
     Txid hashHighFeeTx = tx.GetHash();
     const auto high_fee_tx{entry.Fee(50000).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx)};
     TryAddToMempool(tx_mempool, high_fee_tx);

-    block_template = mining->createNewBlock(options);
+    block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
     block = block_template->getBlock();
     BOOST_REQUIRE_EQUAL(block.vtx.size(), 4U);
     BOOST_CHECK(block.vtx[1]->GetHash() == hashParentTx);
     BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx);
@@ -251,26 +251,26 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     tx.vout.resize(1);
     feeToUse = blockMinFeeRate.GetFee(freeTxSize);
     tx.vout[0].nValue = 5000000000LL - 100000000 - feeToUse;
     Txid hashLowFeeTx2 = tx.GetHash();
     TryAddToMempool(tx_mempool, entry.Fee(feeToUse).SpendsCoinbase(false).FromTx(tx));
-    block_template = mining->createNewBlock(options);
+    block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
     block = block_template->getBlock();

     // Verify that this tx isn't selected.
     for (size_t i=0; i<block.vtx.size(); ++i) {
         BOOST_CHECK(block.vtx[i]->GetHash() != hashFreeTx2);
         BOOST_CHECK(block.vtx[i]->GetHash() != hashLowFeeTx2);
     }

     // This tx will be mineable, and should cause hashLowFeeTx2 to be selected
     // as well.
     tx.vin[0].prevout.n = 1;
     tx.vout[0].nValue = 100000000 - 10000; // 10k satoshi fee
     TryAddToMempool(tx_mempool, entry.Fee(10000).FromTx(tx));
-    block_template = mining->createNewBlock(options);
+    block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
     block = block_template->getBlock();
     BOOST_REQUIRE_EQUAL(block.vtx.size(), 9U);
     BOOST_CHECK(block.vtx[8]->GetHash() == hashLowFeeTx2);
 }
@@ -340,195 +340,195 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // Just to make sure we can still make simple blocks
-        auto block_template{mining->createNewBlock(options)};
+        auto block_template{mining->createNewBlock(options, /*cooldown=*/false)};
         BOOST_REQUIRE(block_template);
         CBlock block{block_template->getBlock()};

         auto txs = CreateBigSigOpsCluster(txFirst[0]);

         int64_t legacy_sigops = 0;
         for (auto& t : txs) {
             // If we don't set the number of sigops in the CTxMemPoolEntry,
             // template creation fails during sanity checks.
             TryAddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(t));
             legacy_sigops += GetLegacySigOpCount(*t);
             BOOST_CHECK(tx_mempool.GetIter(t->GetHash()).has_value());
         }
         assert(tx_mempool.mapTx.size() == 51);
         assert(legacy_sigops == 20001);
-        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-blk-sigops"));
+        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options, /*cooldown=*/false), std::runtime_error, HasReason("bad-blk-sigops"));
     }

     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // Check that the mempool is empty.
         assert(tx_mempool.mapTx.empty());

         // Just to make sure we can still make simple blocks
-        auto block_template{mining->createNewBlock(options)};
+        auto block_template{mining->createNewBlock(options, /*cooldown=*/false)};
         BOOST_REQUIRE(block_template);
         CBlock block{block_template->getBlock()};

         auto txs = CreateBigSigOpsCluster(txFirst[0]);

         int64_t legacy_sigops = 0;
         for (auto& t : txs) {
             TryAddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).SigOpsCost(GetLegacySigOpCount(*t)*WITNESS_SCALE_FACTOR).FromTx(t));
             legacy_sigops += GetLegacySigOpCount(*t);
             BOOST_CHECK(tx_mempool.GetIter(t->GetHash()).has_value());
         }
         assert(tx_mempool.mapTx.size() == 51);
         assert(legacy_sigops == 20001);

-        BOOST_REQUIRE(mining->createNewBlock(options));
+        BOOST_REQUIRE(mining->createNewBlock(options, /*cooldown=*/false));
     }

     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // block size > limit
         tx.vin.resize(1);
         tx.vout.resize(1);
         tx.vout[0].nValue = BLOCKSUBSIDY;
         // 36 * (520char + DROP) + OP_1 = 18757 bytes
         std::vector<unsigned char> vchData(520);
         for (unsigned int i = 0; i < 18; ++i) {
             tx.vin[0].scriptSig << vchData << OP_DROP;
             tx.vout[0].scriptPubKey << vchData << OP_DROP;
         }
         tx.vin[0].scriptSig << OP_1;
         tx.vout[0].scriptPubKey << OP_1;
         tx.vin[0].prevout.hash = txFirst[0]->GetHash();
         tx.vin[0].prevout.n = 0;
         tx.vout[0].nValue = BLOCKSUBSIDY;
         for (unsigned int i = 0; i < 63; ++i) {
             tx.vout[0].nValue -= LOWFEE;
             hash = tx.GetHash();
             bool spendsCoinbase = i == 0; // only first tx spends coinbase
             TryAddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(spendsCoinbase).FromTx(tx));
             BOOST_CHECK(tx_mempool.GetIter(hash).has_value());
             tx.vin[0].prevout.hash = hash;
         }
-        BOOST_REQUIRE(mining->createNewBlock(options));
+        BOOST_REQUIRE(mining->createNewBlock(options, /*cooldown=*/false));
     }

     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // orphan in tx_mempool, template creation fails
         hash = tx.GetHash();
         TryAddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).FromTx(tx));
-        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent"));
+        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options, /*cooldown=*/false), std::runtime_error, HasReason("bad-txns-inputs-missingorspent"));
     }

     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // child with higher feerate than parent
         tx.vin[0].scriptSig = CScript() << OP_1;
         tx.vin[0].prevout.hash = txFirst[1]->GetHash();
         tx.vout[0].nValue = BLOCKSUBSIDY - HIGHFEE;
         hash = tx.GetHash();
         TryAddToMempool(tx_mempool, entry.Fee(HIGHFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
         tx.vin[0].prevout.hash = hash;
         tx.vin.resize(2);
         tx.vin[1].scriptSig = CScript() << OP_1;
         tx.vin[1].prevout.hash = txFirst[0]->GetHash();
         tx.vin[1].prevout.n = 0;
         tx.vout[0].nValue = tx.vout[0].nValue + BLOCKSUBSIDY - HIGHERFEE; // First txn output + fresh coinbase - new txn fee
         hash = tx.GetHash();
         TryAddToMempool(tx_mempool, entry.Fee(HIGHERFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
-        BOOST_REQUIRE(mining->createNewBlock(options));
+        BOOST_REQUIRE(mining->createNewBlock(options, /*cooldown=*/false));
     }

     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // coinbase in tx_mempool, template creation fails
         tx.vin.resize(1);
         tx.vin[0].prevout.SetNull();
         tx.vin[0].scriptSig = CScript() << OP_0 << OP_1;
         tx.vout[0].nValue = 0;
         hash = tx.GetHash();
         // give it a fee so it'll get mined
         TryAddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx));
         // Should throw bad-cb-multiple
-        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-cb-multiple"));
+        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options, /*cooldown=*/false), std::runtime_error, HasReason("bad-cb-multiple"));
     }

     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // double spend txn pair in tx_mempool, template creation fails
         tx.vin[0].prevout.hash = txFirst[0]->GetHash();
         tx.vin[0].scriptSig = CScript() << OP_1;
         tx.vout[0].nValue = BLOCKSUBSIDY - HIGHFEE;
         tx.vout[0].scriptPubKey = CScript() << OP_1;
         hash = tx.GetHash();
         TryAddToMempool(tx_mempool, entry.Fee(HIGHFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
         tx.vout[0].scriptPubKey = CScript() << OP_2;
         hash = tx.GetHash();
         TryAddToMempool(tx_mempool, entry.Fee(HIGHFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
-        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent"));
+        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options, /*cooldown=*/false), std::runtime_error, HasReason("bad-txns-inputs-missingorspent"));
     }

     {
         CTxMemPool& tx_mempool{MakeMempool()};
         LOCK(tx_mempool.cs);

         // subsidy changing
         int nHeight = m_node.chainman->ActiveChain().Height();
         // Create an actual 209999-long block chain (without valid blocks).
         while (m_node.chainman->ActiveChain().Tip()->nHeight < 209999) {
             CBlockIndex* prev = m_node.chainman->ActiveChain().Tip();
             CBlockIndex* next = new CBlockIndex();
             next->phashBlock = new uint256(m_rng.rand256());
             m_node.chainman->ActiveChainstate().CoinsTip().SetBestBlock(next->GetBlockHash());
             next->pprev = prev;
             next->nHeight = prev->nHeight + 1;
             next->BuildSkip();
             m_node.chainman->ActiveChain().SetTip(*next);
         }
-        BOOST_REQUIRE(mining->createNewBlock(options));
+        BOOST_REQUIRE(mining->createNewBlock(options, /*cooldown=*/false));
         // Extend to a 210000-long block chain.
         while (m_node.chainman->ActiveChain().Tip()->nHeight < 210000) {
             CBlockIndex* prev = m_node.chainman->ActiveChain().Tip();
             CBlockIndex* next = new CBlockIndex();
             next->phashBlock = new uint256(m_rng.rand256());
             m_node.chainman->ActiveChainstate().CoinsTip().SetBestBlock(next->GetBlockHash());
             next->pprev = prev;
             next->nHeight = prev->nHeight + 1;
             next->BuildSkip();
             m_node.chainman->ActiveChain().SetTip(*next);
         }
-        BOOST_REQUIRE(mining->createNewBlock(options));
+        BOOST_REQUIRE(mining->createNewBlock(options, /*cooldown=*/false));

         // invalid p2sh txn in tx_mempool, template creation fails
         tx.vin[0].prevout.hash = txFirst[0]->GetHash();
         tx.vin[0].prevout.n = 0;
         tx.vin[0].scriptSig = CScript() << OP_1;
         tx.vout[0].nValue = BLOCKSUBSIDY - LOWFEE;
         CScript script = CScript() << OP_0;
         tx.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash(script));
         hash = tx.GetHash();
         TryAddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
         tx.vin[0].prevout.hash = hash;
         tx.vin[0].scriptSig = CScript() << std::vector<unsigned char>(script.begin(), script.end());
         tx.vout[0].nValue -= LOWFEE;
         hash = tx.GetHash();
         TryAddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx));
-        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("block-script-verify-flag-failed"));
+        BOOST_CHECK_EXCEPTION(mining->createNewBlock(options, /*cooldown=*/false), std::runtime_error, HasReason("block-script-verify-flag-failed"));

         // Delete the dummy blocks again.
         while (m_node.chainman->ActiveChain().Tip()->nHeight > nHeight) {
             CBlockIndex* del = m_node.chainman->ActiveChain().Tip();
             m_node.chainman->ActiveChain().SetTip(*Assert(del->pprev));
@@ -630,28 +630,28 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
     tx.vin[0].nSequence = CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
     BOOST_CHECK(TestSequenceLocks(CTransaction{tx}, tx_mempool)); // Sequence locks pass
     tx.vin[0].nSequence = CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | 1;
     BOOST_CHECK(!TestSequenceLocks(CTransaction{tx}, tx_mempool)); // Sequence locks fail

-    auto block_template = mining->createNewBlock(options);
+    auto block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);

     // None of the of the absolute height/time locked tx should have made
     // it into the template because we still check IsFinalTx in CreateNewBlock,
     // but relative locked txs will if inconsistently added to mempool.
     // For now these will still generate a valid template until BIP68 soft fork
     CBlock block{block_template->getBlock()};
     BOOST_CHECK_EQUAL(block.vtx.size(), 3U);
     // However if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of them should be mined
     for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
         CBlockIndex* ancestor{Assert(m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i))};
         ancestor->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
     }
     m_node.chainman->ActiveChain().Tip()->nHeight++;
     SetMockTime(m_node.chainman->ActiveChain().Tip()->GetMedianTimePast() + 1);

-    block_template = mining->createNewBlock(options);
+    block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
     block = block_template->getBlock();
     BOOST_CHECK_EQUAL(block.vtx.size(), 5U);
 }

@@ -723,92 +723,92 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const
     tx.vin[0].prevout.hash = hashFreeChild;
     tx.vout[0].nValue = 5000000000LL; // 0 fee
     Txid hashFreeGrandchild = tx.GetHash();
     TryAddToMempool(tx_mempool, entry.Fee(0).SpendsCoinbase(false).FromTx(tx));

-    auto block_template = mining->createNewBlock(options);
+    auto block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
     CBlock block{block_template->getBlock()};
     BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U);
     BOOST_CHECK(block.vtx[1]->GetHash() == hashFreeParent);
     BOOST_CHECK(block.vtx[2]->GetHash() == hashFreePrioritisedTx);
     BOOST_CHECK(block.vtx[3]->GetHash() == hashParentTx);
     BOOST_CHECK(block.vtx[4]->GetHash() == hashPrioritsedChild);
     BOOST_CHECK(block.vtx[5]->GetHash() == hashFreeChild);
     for (size_t i=0; i<block.vtx.size(); ++i) {
         // The FreeParent and FreeChild's prioritisations should not impact the child.
         BOOST_CHECK(block.vtx[i]->GetHash() != hashFreeGrandchild);
         // De-prioritised transaction should not be included.
         BOOST_CHECK(block.vtx[i]->GetHash() != hashMediumFeeTx);
     }
 }

 // NOTE: These tests rely on CreateNewBlock doing its own self-validation!
 BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 {
     auto mining{MakeMining()};
     BOOST_REQUIRE(mining);

     // Note that by default, these tests run with size accounting enabled.
     CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
     options.include_dummy_extranonce = true;

     // Create and check a simple template
-    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
+    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
     {
         CBlock block{block_template->getBlock()};
         {
             std::string reason;
             std::string debug;
             BOOST_REQUIRE(!mining->checkBlock(block, {.check_pow = false}, reason, debug));
             BOOST_REQUIRE_EQUAL(reason, "bad-txnmrklroot");
             BOOST_REQUIRE_EQUAL(debug, "hashMerkleRoot mismatch");
         }

         block.hashMerkleRoot = BlockMerkleRoot(block);

         {
             std::string reason;
             std::string debug;
             BOOST_REQUIRE(mining->checkBlock(block, {.check_pow = false}, reason, debug));
             BOOST_REQUIRE_EQUAL(reason, "");
             BOOST_REQUIRE_EQUAL(debug, "");
         }

         {
             // A block template does not have proof-of-work, but it might pass
             // verification by coincidence. Grind the nonce if needed:
             while (CheckProofOfWork(block.GetHash(), block.nBits, Assert(m_node.chainman)->GetParams().GetConsensus())) {
                 block.nNonce++;
             }

             std::string reason;
             std::string debug;
             BOOST_REQUIRE(!mining->checkBlock(block, {.check_pow = true}, reason, debug));
             BOOST_REQUIRE_EQUAL(reason, "high-hash");
             BOOST_REQUIRE_EQUAL(debug, "proof of work failed");
         }
     }

     // We can't make transactions until we have inputs
     // Therefore, load 110 blocks :)
     static_assert(std::size(BLOCKINFO) == 110, "Should have 110 blocks to import");
     int baseheight = 0;
     std::vector<CTransactionRef> txFirst;
     for (const auto& bi : BLOCKINFO) {
         const int current_height{mining->getTip()->height};

         /**
          * Simple block creation, nothing special yet.
          * 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);
+            block_template = mining->createNewBlock(options, /*cooldown=*/false);
             BOOST_REQUIRE(block_template);
         }

         CBlock block{block_template->getBlock()};
         CMutableTransaction txCoinbase(*block.vtx[0]);
diff --git a/src/test/testnet4_miner_tests.cpp b/src/test/testnet4_miner_tests.cpp
index 33e028a5c9..614d2fd63a 100644
--- a/src/test/testnet4_miner_tests.cpp
+++ b/src/test/testnet4_miner_tests.cpp
@@ -40,11 +40,11 @@ BOOST_AUTO_TEST_CASE(MiningInterface)

     // 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);
+    block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);

     // The template should use the mocked system time
     BOOST_REQUIRE_EQUAL(block_template->getBlockHeader().nTime, genesis_time + 3 * 60);

[ryanofsky]

Updated 7730911 -> dfea324 (pr/mbreak.2 -> pr/mbreak.3, compare) cleaning up some more constant usages and splitting out a test workaround into its own commit

[Sjors]

Looks like one of the intermediate commits broke interface_ipc.py.

@maflcko it would be handy if the each-commit job repeated the current commit at the end of its log.

[Sjors]

Code review dfea324

Would be nice to further split fc60bd4:

• bump version, with deprecation test
• drop deprecated methods
• add missing context :Proxy.Context
• set defaults

I do feel like it is good to make backwards incompatible changes atomically, so you can check out any commit in bitcoin core history and build bitcoin-node binary and check out any other commit in bitcoin core history and use the schema files to build an external client, and IPC calls made by that client will succeed or fail but they will never send corrupt parameters & return values.

I think it's sufficient that each release behaves this way, not individual commits.

[Sjors]

In 766edec ipc: Declare constants in mining.capnp for default field values: nice, makes it easy to keep these constants in sync

[Sjors]

In 766edec ipc: Declare constants in mining.capnp for default field values: maybe make it a nice round 3 trillion milliseconds (~95 years, const century)? Or 32 trillion for a millennium.

[ryanofsky]

re: #34568 (comment)

In 766edec ipc: Declare constants in mining.capnp for default field values: maybe make it a nice round 3 trillion milliseconds (~95 years, const century)? Or 32 trillion for a millennium.

Yes could switch to something rounder, this is just keeping the c++ and capnproto values the same.

[Sjors]

ACK 8437219

[Sjors]

In abd9a2f ipc mining: provide default option values (incompatible schema change): review note: changing this from 0 to the new default 400 is harmless. Ditto for blockReservedWeight.

Using defaults also improves test readability. When custom value shows up, it's now clear that it's relevant to the test.

[ryanofsky]

Why do we have to add deprecated methods at all, if we are breaking compatility, in an experimental interface?

We have a few options here with different tradeoffs:

1. We can freely delete methods from both c++ source code and capnp files breaking capnproto schema update rules. This will cause client and server binaries built from the schemas to appear to be compatible with each other and connect and communicate but send corrupt data back and forth, likely leading to confusion and wasted time during development since there is no clear way to tell which clients and servers are compatible and when they are sending corrupt data.

2. We can delete the method implementations from c++ source code, but follow schema update rules in capnp files, keeping method numbers the same, but giving them prefixes or suffixes indicating the methods have been removed, deleting the arguments and making calls to those methods throw exceptions. (Right implementing this requires a line of c++ code for each method virtual void removedFoo() { throw std::exception("foo not implemented"); } But this could be simplified by having libmultiprocess support a $Proxy.exception("foo not implmented") annotation in the capnp files so no c++ code is needed. Unlike the first approach this gives clear errors when clients and servers are incompatible with each other, but deleting method implementations will break older clients, and when the methods are simple accessors like the deprecated methods removed here, we might decide not to break clients without a clear reason to do so.

3. We can keep deprecated methods around for a period of time (like one release) so clients targeted at the most recent release continue to work on master, but might not work with the next release. Then at the end of the period we can delete them by bumping the Init.makeMining number.

This PR is taking approach (3) but we can also delete methods more proactively with (2). I'd like to avoid approach (1) since I don't think I don't think benefits outweigh the costs.

Also should note that the change here removing deprecated methods is only a small part of this PR (just commit e2770e2). The bigger part of this PR is making minor schema improvements (context arguments, default option values) we've been putting off but have more of a reason to implement now because of #34184.

[fanquake]

@ryanofsky Thanks for clarifying here.

[ryanofsky]

Updated 8437219 -> f700609 (pr/mbreak.5 -> pr/mbreak.6, compare) dropping GenerateCoinbaseCommitment return value and improving commit messages

[Sjors]

ACK f700609

[enirox001]

ACK f700609

Built the PR and verified the breaking change by connecting an existing Rust SV2 client (using the old schema) to the updated node. The client was rejected gracefully with the expected error (Old mining interface (@2) not supported) and the node remained stable, confirming the version bump logic works safely.

nano nit:
The comment in src/node/types.h for BlockCreateOptions states clients "cannot leave this field unset." With the new defaults in mining.capnp, this is no longer true and should be updated.

[Sjors]

cc @ismaelsadeeq

[ismaelsadeeq]

Concept ACK

[ismaelsadeeq]

ACK f700609

[ismaelsadeeq]

In "rpc refactor: stop using deprecated getCoinbaseCommitment method" df53a3e

This comment doesn't add any value here, not a fan :).
The CHECK_NON_FATAL made that obvious; a better comment should explain why we expect 1 output or why it's not fatal to continue.

[ryanofsky]

re: #34568 (comment)

This comment doesn't add any value here, not a fan :).

Agree comment could be better, I just added it because I wanted to be clear this is a "not currently expected to happen" check, not a "this should never happen" check. The designs of the mining API and segwit explicitly allow and anticipate this happening, it's just that the RPC code may need to change if it does.

Thinking about this more though, it would probably be best to generalize the code a little and drop this check. Instead of assuming there is only one output this code could use the GetWitnessCommitmentIndex logic to find the right output and return it. It'd be a slightly bigger change but would be a nice followup.

[ismaelsadeeq]

Nice to have these gone.

[ryanofsky]

Thanks for the reviews! Will leave this PR as-is for now but some nice followups have been suggested:

• Updating node/types.h comment (mining: Break compatibility with existing IPC mining clients #34568 (review))
• Clarifying RPC check or dropping it (mining: Break compatibility with existing IPC mining clients #34568 (comment))

[sedited]

ACK f700609

[sedited]

Just a question: Why are all these constants in a policy-related file as opposed to e.g. miner.h or its own header?

[ismaelsadeeq]

Hmm, the constant they are indeed Bitcoin Core mining defaults, which are whats recommended and seperate from consensus, hence they can be termed as mining policy?
see related discussion #31384 (comment)

Right now, policy/policy.h encapsulates both mempool policy, mempool, validation, relay and mining policy.

Are you suggesting they be split? It seems beyond the scope of this PR IMO?

[ryanofsky]

re: #34568 (comment)

Just a question: Why are all these constants in a policy-related file as opposed to e.g. miner.h or its own header?

I also thought this was strange, but you could think of them as a mining policy analogous to normal mempool policy as Abubakar suggests. Since this only adds one constant, this seemed the best place for it to go, but if there are preferences or suggestions for where else these should go, it could be nice to move them. Could do this in a PR with other suggested followups #34568 (comment)

[Sjors]

Could also be a followup for #33966. I considered moving mining "policy" things away from policy as well there, but didn't.

[ryanofsky]

In commit "ipc test: add workaround to block_reserved_weight exception test" (ff995b5)

I did some more work on bitcoin-core/libmultiprocess#218 and this comment is false. I thought that PR added code to catch uncaught exceptions from IPC server methods running on asynchronous threads, but it actually never did that. The code which does do that was added in bitcoin-core/libmultiprocess#240 commit bitcoin-core/libmultiprocess@c4762c7. That commit was only supposed to be a refactoring, but because it moved a kj::runCatchingExceptions call one level up the stack it fixed this problem inadvertently.