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

@@ -39,6 +39,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

@@ -64,6 +64,14 @@ struct BlockCreateOptions {
      * coinbase_max_additional_weight and coinbase_output_max_additional_sigops.
      */
     CScript coinbase_output_script{CScript() << OP_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

@@ -871,7 +871,7 @@ static RPCHelpMan getblocktemplate()
         time_start = GetTime();
 
         // Create new block
-        block_template = miner.createNewBlock();
+        block_template = miner.createNewBlock({.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

@@ -141,7 +141,8 @@ 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)
             txfees = await template.result.getTxFees(ctx)
             assert_equal(len(txfees.result), 0)
             txsigops = await template.result.getTxSigops(ctx)
@@ -150,6 +151,19 @@ async def async_routine():
             coinbase = CTransaction()
             coinbase.deserialize(coinbase_data)
             assert_equal(coinbase.vin[0].prevout.hash, 0)
+            # Check that coinbase for an empty block doesn't have a witness
+            assert(coinbase.wit.is_null())
+            # Inspect coinbase template returned by getCoinbase()
+            # coinbase_template = (await template.result.getCoinbase(ctx)).result
+            # assert(not coinbase_template._has('witness'))
+
+            # Unless requested otherwise
+            opts.alwaysAddCoinbaseCommitment = True
+            template = mining.result.createNewBlock(ctx, opts)
+            coinbase_data = BytesIO((await template.result.getCoinbaseTx(ctx)).result)
+            coinbase.deserialize(coinbase_data)
+            assert(not coinbase.wit.is_null())
+
             self.log.debug("Wait for a new template")
             waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
             waitoptions.timeout = timeout
@@ -180,6 +194,8 @@ async def async_routine():
             template6 = await waitnext
             block4 = await self.parse_and_deserialize_block(template6, ctx)
             assert_equal(len(block4.vtx), 3)
+            # Check that coinbase for non-empty block has a witness")
+            assert(not block4.vtx[0].wit.is_null())
             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(), {})