Merge bitcoin/bitcoin#32420: mining, ipc: omit dummy extraNonce from coinbase

d511adb664edcfb97be44bc0738f49b679240504 [miner] omit dummy extraNonce via IPC (Sjors Provoost)
bf3b5d6d069a0bbb39af0c487fd597257f862f31 test: clarify getCoinbaseRawTx() comparison (Sjors Provoost)
78df9003d63414e4a17b686af7647aeefd706ec5 [doc] Update comments on dummy extraNonces in tests (Anthony Towns)

Pull request description:

  This PR changes the Mining IPC interface to stop including a dummy `extraNonce` in the coinbase `scriptSig` by default, exposing only the consensus-required BIP34 height. This simplifies downstream mining software (including Stratum v2), avoids forcing clients to strip or ignore data we generate, and reduces the risk of incompatibilities if future soft forks add required commitments to the `scriptSig`.

  Existing behavior is preserved for RPCs, tests, regtest, and internal mining by explicitly opting in to the dummy `extraNonce` where needed (e.g. to satisfy `bad-cb-length` at low heights), so consensus rules and test coverage are unchanged. The remainder of the PR consists of small comment fixes, naming clarifications, and test cleanups to make the intent and behavior clearer.

ACKs for top commit:
  achow101:
    ACK d511adb664edcfb97be44bc0738f49b679240504
  ryanofsky:
    Code review ACK d511adb664edcfb97be44bc0738f49b679240504. Just rebased since last review and make suggested tweaks. I'd really like to see this PR merged for the cleanups and sanity it brings to this code. Needs another reviewer though.
  sedited:
    ACK d511adb664edcfb97be44bc0738f49b679240504

Tree-SHA512: d41fa813eb6b5626f4f475d8abc506b29090f4a2d218f2d6824db58b5ebe2ed7c584a903b44de18ccec142bb79c257b0aba6d6da073f56175aec88df96aaaaba

src/bench/block_assemble.cpp

@@ -30,6 +30,7 @@ static void AssembleBlock(benchmark::Bench& bench)
     witness.stack.push_back(WITNESS_STACK_ELEM_OP_TRUE);
     BlockAssembler::Options options;
     options.coinbase_output_script = P2WSH_OP_TRUE;
+    options.include_dummy_extranonce = true;
 
     // Collect some loose transactions that spend the coinbases of our mined blocks
     constexpr size_t NUM_BLOCKS{200};

src/node/miner.cpp

@@ -177,11 +177,17 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     coinbase_tx.block_reward_remaining = block_reward;
 
     // Start the coinbase scriptSig with the block height as required by BIP34.
-    // The trailing OP_0 (historically an extranonce) is optional padding and
-    // could be removed without a consensus change. Mining clients are expected
-    // to append extra data to this prefix, so increasing its length would reduce
-    // the space they can use and may break existing clients.
-    coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0;
+    // Mining clients are expected to append extra data to this prefix, so
+    // increasing its length would reduce the space they can use and may break
+    // existing clients.
+    coinbaseTx.vin[0].scriptSig = CScript() << nHeight;
+    if (m_options.include_dummy_extranonce) {
+        // For blocks at heights <= 16, the BIP34-encoded height alone is only
+        // one byte. Consensus requires coinbase scriptSigs to be at least two
+        // bytes long (bad-cb-length), so tests and regtest include a dummy
+        // extraNonce (OP_0)
+        coinbaseTx.vin[0].scriptSig << OP_0;
+    }
     coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig;
     Assert(nHeight > 0);
     coinbaseTx.nLockTime = static_cast<uint32_t>(nHeight - 1);
@@ -212,6 +218,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     pblock->nNonce         = 0;
 
     if (m_options.test_block_validity) {
+        // if nHeight <= 16, and include_dummy_extranonce=false this will fail due to bad-cb-length.
         if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
             throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
         }

src/node/types.h

@@ -67,6 +67,10 @@ struct BlockCreateOptions {
      * coinbase_max_additional_weight and coinbase_output_max_additional_sigops.
      */
     CScript coinbase_output_script{CScript() << OP_TRUE};
+    /**
+     * Whether to include an OP_0 as a dummy extraNonce in the template's coinbase
+     */
+    bool include_dummy_extranonce{false};
 };
 
 struct BlockWaitOptions {

src/rpc/mining.cpp

@@ -165,7 +165,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const
 {
     UniValue blockHashes(UniValue::VARR);
     while (nGenerate > 0 && !chainman.m_interrupt) {
-        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({ .coinbase_output_script = coinbase_output_script }));
+        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({ .coinbase_output_script = coinbase_output_script, .include_dummy_extranonce = true }));
         CHECK_NONFATAL(block_template);
 
         std::shared_ptr<const CBlock> block_out;
@@ -376,7 +376,7 @@ static RPCHelpMan generateblock()
     {
         LOCK(chainman.GetMutex());
         {
-            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({.use_mempool = false, .coinbase_output_script = coinbase_output_script})};
+            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({.use_mempool = false, .coinbase_output_script = coinbase_output_script, .include_dummy_extranonce = true})};
             CHECK_NONFATAL(block_template);
 
             block = block_template->getBlock();
@@ -871,7 +871,7 @@ static RPCHelpMan getblocktemplate()
         time_start = GetTime();
 
         // Create new block
-        block_template = miner.createNewBlock();
+        block_template = miner.createNewBlock({.include_dummy_extranonce = true});
         CHECK_NONFATAL(block_template);
 
 

src/test/blockfilter_index_tests.cpp

@@ -69,6 +69,7 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev,
 {
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
+    options.include_dummy_extranonce = true;
     std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}.CreateNewBlock();
     CBlock& block = pblocktemplate->block;
     block.hashPrevBlock = prev->GetBlockHash();

src/test/fuzz/package_eval.cpp

@@ -46,6 +46,7 @@ void initialize_tx_pool()
 
     BlockAssembler::Options options;
     options.coinbase_output_script = P2WSH_EMPTY;
+    options.include_dummy_extranonce = true;
 
     for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) {
         COutPoint prevout{MineBlock(g_setup->m_node, options)};

src/test/fuzz/process_message.cpp

@@ -41,7 +41,9 @@ void ResetChainman(TestingSetup& setup)
     setup.m_make_chainman();
     setup.LoadVerifyActivateChainstate();
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
-        MineBlock(setup.m_node, {});
+        node::BlockAssembler::Options options;
+        options.include_dummy_extranonce = true;
+        MineBlock(setup.m_node, options);
     }
     setup.m_node.validation_signals->SyncWithValidationInterfaceQueue();
 }

src/test/fuzz/process_messages.cpp

@@ -35,8 +35,10 @@ void ResetChainman(TestingSetup& setup)
     setup.m_node.chainman.reset();
     setup.m_make_chainman();
     setup.LoadVerifyActivateChainstate();
+    node::BlockAssembler::Options options;
+    options.include_dummy_extranonce = true;
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
-        MineBlock(setup.m_node, {});
+        MineBlock(setup.m_node, options);
     }
     setup.m_node.validation_signals->SyncWithValidationInterfaceQueue();
 }

src/test/fuzz/tx_pool.cpp

@@ -48,6 +48,7 @@ void initialize_tx_pool()
 
     BlockAssembler::Options options;
     options.coinbase_output_script = P2WSH_OP_TRUE;
+    options.include_dummy_extranonce = true;
 
     for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) {
         COutPoint prevout{MineBlock(g_setup->m_node, options)};
@@ -97,6 +98,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
         BlockAssembler::Options options;
         options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT);
         options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)};
+        options.include_dummy_extranonce = true;
         auto assembler = BlockAssembler{chainstate, &tx_pool, options};
         auto block_template = assembler.CreateNewBlock();
         Assert(block_template->block.vtx.size() >= 1);

src/test/fuzz/utxo_total_supply.cpp

@@ -45,6 +45,7 @@ FUZZ_TARGET(utxo_total_supply)
     };
     BlockAssembler::Options options;
     options.coinbase_output_script = CScript() << OP_FALSE;
+    options.include_dummy_extranonce = true;
     const auto PrepareNextBlock = [&]() {
         // Use OP_FALSE to avoid BIP30 check from hitting early
         auto block = PrepareBlock(node, options);

src/test/miner_tests.cpp

@@ -116,6 +116,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     auto mining{MakeMining()};
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
+    options.include_dummy_extranonce = true;
 
     LOCK(tx_mempool.cs);
     BOOST_CHECK(tx_mempool.size() == 0);
@@ -334,6 +335,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
 
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
+    options.include_dummy_extranonce = true;
 
     {
         CTxMemPool& tx_mempool{MakeMempool()};
@@ -660,6 +662,7 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const
 
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
+    options.include_dummy_extranonce = true;
 
     CTxMemPool& tx_mempool{MakeMempool()};
     LOCK(tx_mempool.cs);
@@ -749,6 +752,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     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);

src/test/peerman_tests.cpp

@@ -19,8 +19,10 @@ static constexpr int64_t NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS = 144;
 static void mineBlock(const node::NodeContext& node, std::chrono::seconds block_time)
 {
     auto curr_time = GetTime<std::chrono::seconds>();
+    node::BlockAssembler::Options options;
+    options.include_dummy_extranonce = true;
     SetMockTime(block_time); // update time so the block is created with it
-    CBlock block = node::BlockAssembler{node.chainman->ActiveChainstate(), nullptr, {}}.CreateNewBlock()->block;
+    CBlock block = node::BlockAssembler{node.chainman->ActiveChainstate(), nullptr, options}.CreateNewBlock()->block;
     while (!CheckProofOfWork(block.GetHash(), block.nBits, node.chainman->GetConsensus())) ++block.nNonce;
     block.fChecked = true; // little speedup
     SetMockTime(curr_time); // process block at current time

src/test/testnet4_miner_tests.cpp

@@ -35,6 +35,7 @@ BOOST_AUTO_TEST_CASE(MiningInterface)
     BOOST_REQUIRE(mining);
 
     BlockAssembler::Options options;
+    options.include_dummy_extranonce = true;
     std::unique_ptr<BlockTemplate> block_template;
 
     // Set node time a few minutes past the testnet4 genesis block

src/test/util/mining.cpp

@@ -29,6 +29,7 @@ COutPoint generatetoaddress(const NodeContext& node, const std::string& address)
     assert(IsValidDestination(dest));
     BlockAssembler::Options assembler_options;
     assembler_options.coinbase_output_script = GetScriptForDestination(dest);
+    assembler_options.include_dummy_extranonce = true;
 
     return MineBlock(node, assembler_options);
 }
@@ -49,6 +50,7 @@ std::vector<std::shared_ptr<CBlock>> CreateBlockChain(size_t total_height, const
         coinbase_tx.vout.resize(1);
         coinbase_tx.vout[0].scriptPubKey = P2WSH_OP_TRUE;
         coinbase_tx.vout[0].nValue = GetBlockSubsidy(height + 1, params.GetConsensus());
+        // Always include OP_0 as a dummy extraNonce.
         coinbase_tx.vin[0].scriptSig = CScript() << (height + 1) << OP_0;
         block.vtx = {MakeTransactionRef(std::move(coinbase_tx))};
 
@@ -138,6 +140,7 @@ std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coi
 {
     BlockAssembler::Options assembler_options;
     assembler_options.coinbase_output_script = coinbase_scriptPubKey;
+    assembler_options.include_dummy_extranonce = true;
     ApplyArgsManOptions(*node.args, assembler_options);
     return PrepareBlock(node, assembler_options);
 }

src/test/util/setup_common.cpp

@@ -413,6 +413,7 @@ CBlock TestChain100Setup::CreateBlock(
 {
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
+    options.include_dummy_extranonce = true;
     CBlock block = BlockAssembler{chainstate, nullptr, options}.CreateNewBlock()->block;
 
     Assert(block.vtx.size() == 1);

src/test/validation_block_tests.cpp

@@ -68,6 +68,7 @@ std::shared_ptr<CBlock> MinerTestingSetup::Block(const uint256& prev_hash)
 
     BlockAssembler::Options options;
     options.coinbase_output_script = CScript{} << i++ << OP_TRUE;
+    options.include_dummy_extranonce = true;
     auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}.CreateNewBlock();
     auto pblock = std::make_shared<CBlock>(ptemplate->block);
     pblock->hashPrevBlock = prev_hash;
@@ -82,7 +83,7 @@ std::shared_ptr<CBlock> MinerTestingSetup::Block(const uint256& prev_hash)
     txCoinbase.vout[1].nValue = txCoinbase.vout[0].nValue;
     txCoinbase.vout[0].nValue = 0;
     txCoinbase.vin[0].scriptWitness.SetNull();
-    // Always pad with OP_0 at the end to avoid bad-cb-length error
+    // Always pad with OP_0 as dummy extraNonce (also avoids bad-cb-length error for block <=16)
     const int prev_height{WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(prev_hash)->nHeight)};
     txCoinbase.vin[0].scriptSig = CScript{} << prev_height + 1 << OP_0;
     txCoinbase.nLockTime = static_cast<uint32_t>(prev_height);
@@ -336,6 +337,7 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index)
     pubKey << 1 << OP_TRUE;
     BlockAssembler::Options options;
     options.coinbase_output_script = pubKey;
+    options.include_dummy_extranonce = true;
     auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}.CreateNewBlock();
     CBlock pblock = ptemplate->block;
 

test/functional/interface_ipc.py

@@ -20,10 +20,14 @@ from test_framework.messages import (
     ser_uint256,
     COIN,
 )
+from test_framework.script import (
+    CScript,
+    CScriptNum,
+)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    assert_not_equal
+    assert_not_equal,
 )
 from test_framework.wallet import MiniWallet
 from typing import Optional
@@ -144,7 +148,7 @@ class IPCInterfaceTest(BitcoinTestFramework):
         block.deserialize(block_data)
         return block
 
-    async def parse_and_deserialize_coinbase_tx(self, block_template, ctx):
+    async def get_coinbase_raw_tx(self, block_template, ctx):
         assert block_template is not None
         coinbase_data = BytesIO((await block_template.getCoinbaseRawTx(ctx)).result)
         tx = CTransaction()
@@ -194,6 +198,12 @@ class IPCInterfaceTest(BitcoinTestFramework):
         coinbase_tx.vin = [CTxIn()]
         coinbase_tx.vin[0].prevout = NULL_OUTPOINT
         coinbase_tx.vin[0].nSequence = coinbase_res.sequence
+
+        # Verify there's no dummy extraNonce in the coinbase scriptSig
+        current_block_height = self.nodes[0].getchaintips()[0]["height"]
+        expected_scriptsig = CScript([CScriptNum(current_block_height + 1)])
+        assert_equal(coinbase_res.scriptSigPrefix.hex(), expected_scriptsig.hex())
+
         # Typically a mining pool appends its name and an extraNonce
         coinbase_tx.vin[0].scriptSig = coinbase_res.scriptSigPrefix
 
@@ -224,8 +234,9 @@ class IPCInterfaceTest(BitcoinTestFramework):
 
         coinbase_tx.nLockTime = coinbase_res.lockTime
 
-        # Compare to dummy coinbase provided by the deprecated getCoinbaseTx()
-        coinbase_legacy = await self.parse_and_deserialize_coinbase_tx(template, ctx)
+        # Compare to dummy coinbase transaction provided by the deprecated
+        # getCoinbaseRawTx()
+        coinbase_legacy = await self.get_coinbase_raw_tx(template, ctx)
         assert_equal(coinbase_legacy.vout[0].nValue, coinbase_res.blockRewardRemaining)
         # Swap dummy output for our own
         coinbase_legacy.vout[0].scriptPubKey = coinbase_tx.vout[0].scriptPubKey
@@ -282,10 +293,6 @@ class IPCInterfaceTest(BitcoinTestFramework):
                 assert_equal(len(txfees.result), 0)
                 txsigops = await template.getTxSigops(ctx)
                 assert_equal(len(txsigops.result), 0)
-                coinbase_data = BytesIO((await template.getCoinbaseRawTx(ctx)).result)
-                coinbase = CTransaction()
-                coinbase.deserialize(coinbase_data)
-                assert_equal(coinbase.vin[0].prevout.hash, 0)
 
                 self.log.debug("Wait for a new template")
                 waitoptions = self.capnp_modules['mining'].BlockWaitOptions()

test/functional/test_framework/blocktools.py

@@ -164,7 +164,7 @@ def add_witness_commitment(block, nonce=0):
 def script_BIP34_coinbase_height(height):
     if height <= 16:
         res = CScriptOp.encode_op_n(height)
-        # Append dummy to increase scriptSig size to 2 (see bad-cb-length consensus rule)
+        # Append dummy extraNonce to increase scriptSig size to 2 (see bad-cb-length consensus rule)
         return CScript([res, OP_0])
     return CScript([CScriptNum(height)])