mining: make non-mandatory coinbase commitment opt-in

Previously the dummy coinbase returned by the Mining IPC interface
always had a witness and SegWit OP_RETURN output. This is unnecessary
for empty blocks as well as for blocks without any SegWit spends.

This commit makes this behavior configurable via
always_add_coinbase_commitment on BlockCreateOptions. It defaults to false,
making this a potentially breaking change to clients. The presence of this
field in requests makes it incompatible with Bitcoin Core v30, so it's a
breaking change in any case.

Preserve historical getblocktemplate RPC behavior. Although it doesn't return a
coinbase transaction, it does always provide a default_witness_commitment. This
is set in BlockAssembler::CreateNewBlock() via a call GenerateCoinbaseCommitment(),
which has the side-effect of adding a coinbase witness.

Author: Sjors

src/ipc/capnp/mining.capnp

@@ -44,6 +44,7 @@ struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
     useMempool @0 :Bool $Proxy.name("use_mempool");
     blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
     coinbaseOutputMaxAdditionalSigops @2 :UInt64 $Proxy.name("coinbase_output_max_additional_sigops");
+    alwaysAddCoinbaseCommitment @3 :Bool $Proxy.name("always_add_coinbase_commitment");
 }
 
 struct BlockWaitOptions $Proxy.wrap("node::BlockWaitOptions") {

src/node/miner.cpp

@@ -169,7 +169,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     Assert(nHeight > 0);
     coinbaseTx.nLockTime = static_cast<uint32_t>(nHeight - 1);
     pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx));
-    pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev);
+    if (m_options.always_add_coinbase_commitment || std::any_of(
+        pblock->vtx.begin(), pblock->vtx.end(),
+        [&](const CTransactionRef tx) { return tx->HasWitness(); }
+    )) {
+        pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev);
+    }
 
     LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost);
 

src/node/types.h

@@ -67,6 +67,7 @@ struct BlockCreateOptions {
      * coinbase_max_additional_weight and coinbase_output_max_additional_sigops.
      */
     CScript coinbase_output_script{CScript() << OP_TRUE};
+
     /**
      * Whether to clear the dummy coinbase from the block template.
      *
@@ -75,6 +76,14 @@ struct BlockCreateOptions {
      * can opt-in to keeping it.
      */
     bool clear_coinbase{true};
+
+    /*
+     * Whether blocks without SegWit transactions (e.g. empty blocks) should
+     * have a SegWit commitment, i.e. the coinbase witness and OP_RETURN.
+     *
+     * @note IPC clients should omit this field for compatibility with v30.0
+     */
+    bool always_add_coinbase_commitment{false};
 };
 
 struct BlockWaitOptions {

src/rpc/mining.cpp

@@ -167,7 +167,8 @@ static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const
     while (nGenerate > 0 && !chainman.m_interrupt) {
         std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({
             .coinbase_output_script = coinbase_output_script,
-            .clear_coinbase = false
+            .clear_coinbase = false,
+            .always_add_coinbase_commitment = true
         }));
         CHECK_NONFATAL(block_template);
 
@@ -382,7 +383,8 @@ static RPCHelpMan generateblock()
             std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({
                 .use_mempool = false,
                 .coinbase_output_script = coinbase_output_script,
-                .clear_coinbase = false
+                .clear_coinbase = false,
+                .always_add_coinbase_commitment = true
             })};
             CHECK_NONFATAL(block_template);
 
@@ -878,7 +880,10 @@ static RPCHelpMan getblocktemplate()
         time_start = GetTime();
 
         // Create new block
-        block_template = miner.createNewBlock({.clear_coinbase = false});
+        block_template = miner.createNewBlock({
+            .clear_coinbase = false,
+            .always_add_coinbase_commitment = true
+        });
         CHECK_NONFATAL(block_template);
 
 

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.always_add_coinbase_commitment = true;
     const auto PrepareNextBlock = [&]() {
         // Use OP_FALSE to avoid BIP30 check from hitting early
         auto block = PrepareBlock(node, options);

src/test/util/setup_common.cpp

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

test/functional/interface_ipc.py

@@ -146,8 +146,10 @@ async def async_routine():
             echo.destroy(ctx)
         asyncio.run(capnp.run(async_routine()))
 
-    async def build_coinbase_test(self, template, ctx, miniwallet):
-        self.log.debug("Build coinbase transaction using getCoinbase()")
+    async def build_coinbase_test(self, template, ctx, miniwallet, *, expect_witness: bool):
+        self.log.debug("Build coinbase transaction using getCoinbase() " +
+                       ("with" if expect_witness else "without") + " a witness")
+
         coinbase_template = await self.parse_and_deserialize_coinbase(template, ctx)
         coinbase = CTransaction()
         coinbase.version = coinbase_template.version
@@ -168,8 +170,7 @@ async def build_coinbase_test(self, template, ctx, miniwallet):
         coinbase.vout = [CTxOut()]
         coinbase.vout[0].scriptPubKey = miniwallet.get_output_script()
         coinbase.vout[0].nValue = coinbase_template.valueRemaining
-        # Add SegWit OP_RETURN. This is currently always present even for
-        # empty blocks, but this may change.
+        # Add SegWit OP_RETURN if provided
         found_witness_op_return = False
         for output_data in coinbase_template.requiredOutputs:
             output = CTxOut()
@@ -179,6 +180,7 @@ async def build_coinbase_test(self, template, ctx, miniwallet):
                 found_witness_op_return = True
 
         assert_equal(has_witness, found_witness_op_return)
+        assert_equal(has_witness, expect_witness)
 
         coinbase.nLockTime = coinbase_template.lockTime
 
@@ -227,7 +229,17 @@ async def async_routine():
             assert_equal(len(header.result), block_header_size)
             block = await self.parse_and_deserialize_block(template, ctx)
             assert_equal(ser_uint256(block.hashPrevBlock), newblockref.result.hash)
-            assert len(block.vtx) >= 1
+
+            # Template block.vtx only contains a (dummy) coinbase
+            assert_equal(len(block.vtx), 1)
+            # Check that coinbase for an empty block doesn't have a witness")
+            await self.build_coinbase_test(template, ctx, miniwallet, expect_witness=False)
+            # Unless requested otherwise
+            opts.alwaysAddCoinbaseCommitment = True
+            template = mining.result.createNewBlock(ctx, opts)
+            await self.build_coinbase_test(template, ctx, miniwallet, expect_witness=True)
+            opts.alwaysAddCoinbaseCommitment = False
+
             txfees = await template.result.getTxFees(ctx)
             assert_equal(len(txfees.result), 0)
             txsigops = await template.result.getTxSigops(ctx)
@@ -262,6 +274,7 @@ async def async_routine():
             template6 = await waitnext
             block4 = await self.parse_and_deserialize_block(template6, ctx)
             assert_equal(len(block4.vtx), 3)
+
             self.log.debug("Wait for another, but time out, since the fee threshold is set now")
             template7 = await template6.result.waitNext(ctx, waitoptions)
             assert_equal(template7.to_dict(), {})
@@ -293,7 +306,7 @@ async def interrupt_wait():
             template = await mining.result.createNewBlock(ctx, opts)
             block = await self.parse_and_deserialize_block(template, ctx)
 
-            coinbase = await self.build_coinbase_test(template, ctx, miniwallet)
+            coinbase = await self.build_coinbase_test(template, ctx, miniwallet, expect_witness=True)
 
             # Reduce payout for balance comparison simplicity
             coinbase.vout[0].nValue = COIN