ipc: breaking but non-urgent changes

doc/release-notes-33819.md

@@ -0,0 +1,6 @@
+Mining IPC
+----------
+
+- Adds `getCoinbase()` which clients should use instead of `getCoinbaseTx()`. It
+  contains all fields required to construct a coinbase transaction, and omits the
+  dummy output which Bitcoin Core uses internally. (#33819)

src/interfaces/mining.h

@@ -42,9 +42,18 @@ class BlockTemplate
     // Sigop cost per transaction, not including coinbase transaction.
     virtual std::vector<int64_t> getTxSigops() = 0;
 
-    virtual CTransactionRef getCoinbaseTx() = 0;
-    virtual std::vector<unsigned char> getCoinbaseCommitment() = 0;
-    virtual int getWitnessCommitmentIndex() = 0;
+    /** Return fields needed to construct a coinbase transaction */
+    virtual node::CoinbaseTemplate getCoinbase() = 0;
+
+    virtual void getCoinbaseTx() {
+        throw std::runtime_error("Old mining interface (@5) not supported. Please update your client!");
+    }
+    virtual void getCoinbaseCommitment() {
+        throw std::runtime_error("Old mining interface (@6) not supported. Please update your client!");
+    };
+    virtual void getWitnessCommitmentIndex() {
+        throw std::runtime_error("Old mining interface (@7) not supported. Please update your client!");
+    }
 
     /**
      * Compute merkle path to the coinbase transaction

src/ipc/capnp/mining.capnp

@@ -17,8 +17,8 @@ interface Mining $Proxy.wrap("interfaces::Mining") {
     isInitialBlockDownload @1 (context :Proxy.Context) -> (result: Bool);
     getTip @2 (context :Proxy.Context) -> (result: Common.BlockRef, hasResult: Bool);
     waitTipChanged @3 (context :Proxy.Context, currentTip: Data, timeout: Float64) -> (result: Common.BlockRef);
-    createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate);
-    checkBlock @5 (block: Data, options: BlockCheckOptions) -> (reason: Text, debug: Text, result: Bool);
+    createNewBlock @4 (context :Proxy.Context, options: BlockCreateOptions) -> (result: BlockTemplate);
+    checkBlock @5 (context :Proxy.Context, block: Data, options: BlockCheckOptions) -> (reason: Text, debug: Text, result: Bool);
 }
 
 interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
@@ -27,9 +27,13 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
     getBlock @2 (context: Proxy.Context) -> (result: Data);
     getTxFees @3 (context: Proxy.Context) -> (result: List(Int64));
     getTxSigops @4 (context: Proxy.Context) -> (result: List(Int64));
-    getCoinbaseTx @5 (context: Proxy.Context) -> (result: Data);
-    getCoinbaseCommitment @6 (context: Proxy.Context) -> (result: Data);
-    getWitnessCommitmentIndex @7 (context: Proxy.Context) -> (result: Int32);
+    getCoinbase @12 (context: Proxy.Context) -> (result: CoinbaseTemplate);
+
+    # DEPRECATED in favor of getCoinbase(), server returns an error:
+    getCoinbaseTx @5 () -> ();
+    getCoinbaseCommitment @6 () -> ();
+    getWitnessCommitmentIndex @7 () -> ();
+
     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);
@@ -40,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") {
@@ -51,3 +56,13 @@ struct BlockCheckOptions $Proxy.wrap("node::BlockCheckOptions") {
     checkMerkleRoot @0 :Bool $Proxy.name("check_merkle_root");
     checkPow @1 :Bool $Proxy.name("check_pow");
 }
+
+struct CoinbaseTemplate $Proxy.wrap("node::CoinbaseTemplate") {
+    version @0 :UInt32 $Proxy.name("version");
+    sequence @1 :UInt32 $Proxy.name("sequence");
+    scriptSigPrefix @2 :Data $Proxy.name("script_sig_prefix");
+    witness @3 :Data $Proxy.name("witness");
+    valueRemaining @4 :Int64 $Proxy.name("value_remaining");
+    requiredOutputs @5 :List(Data) $Proxy.name("required_outputs");
+    lockTime @6 :UInt32 $Proxy.name("lock_time");
+}

src/node/interfaces.cpp

@@ -83,6 +83,7 @@ using interfaces::Node;
 using interfaces::WalletLoader;
 using node::BlockAssembler;
 using node::BlockWaitOptions;
+using node::CoinbaseTemplate;
 using util::Join;
 
 namespace node {
@@ -863,10 +864,17 @@ class BlockTemplateImpl : public BlockTemplate
     explicit BlockTemplateImpl(BlockAssembler::Options assemble_options,
                                std::unique_ptr<CBlockTemplate> block_template,
                                NodeContext& node) : m_assemble_options(std::move(assemble_options)),
+                                                    m_coinbase_template(ExtractCoinbaseTemplate(*Assert(block_template))),
                                                     m_block_template(std::move(block_template)),
                                                     m_node(node)
     {
         assert(m_block_template);
+
+        if (m_assemble_options.clear_coinbase) {
+            // Clear dummy coinbase so it's not exposed to callers of getBlock()
+            CMutableTransaction empty_tx;
+            m_block_template->block.vtx[0] = MakeTransactionRef(std::move(empty_tx));
+        }
     }
 
     CBlockHeader getBlockHeader() override
@@ -889,19 +897,9 @@ class BlockTemplateImpl : public BlockTemplate
         return m_block_template->vTxSigOpsCost;
     }
 
-    CTransactionRef getCoinbaseTx() override
-    {
-        return m_block_template->block.vtx[0];
-    }
-
-    std::vector<unsigned char> getCoinbaseCommitment() override
+    CoinbaseTemplate getCoinbase() override
     {
-        return m_block_template->vchCoinbaseCommitment;
-    }
-
-    int getWitnessCommitmentIndex() override
-    {
-        return GetWitnessCommitmentIndex(m_block_template->block);
+        return m_coinbase_template;
     }
 
     std::vector<uint256> getCoinbaseMerklePath() override
@@ -929,6 +927,8 @@ class BlockTemplateImpl : public BlockTemplate
 
     const BlockAssembler::Options m_assemble_options;
 
+    const CoinbaseTemplate m_coinbase_template;
+
     const std::unique_ptr<CBlockTemplate> m_block_template;
 
     bool m_interrupt_wait{false};

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);
 
@@ -591,4 +596,51 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
     // avoid deadlocks.
     return GetTip(chainman);
 }
+
+node::CoinbaseTemplate ExtractCoinbaseTemplate(const node::CBlockTemplate& block_template)
+{
+    CTransactionRef coinbase_tx{block_template.block.vtx[0]};
+    node::CoinbaseTemplate coinbase{};
+
+    coinbase.version = coinbase_tx->version;
+    Assert(coinbase_tx->vin.size() == 1);
+    coinbase.script_sig_prefix = coinbase_tx->vin[0].scriptSig;
+    // The CoinbaseTemplate interface guarantees a size limit. Raising it (e.g.
+    // if a future softfork needs to commit more than BIP34) is a
+    // (potentially silent) breaking change for clients.
+    if (!Assume(coinbase.script_sig_prefix.size() <= 8)) {
+        LogWarning("Unexpected %d byte scriptSig prefix size.",
+                    coinbase.script_sig_prefix.size());
+    }
+
+    if (coinbase_tx->HasWitness()) {
+        const auto& witness_stack{coinbase_tx->vin[0].scriptWitness.stack};
+        // Consensus requires the coinbase witness stack to have exactly one
+        // element of 32 bytes.
+        Assert(witness_stack.size() == 1 && witness_stack[0].size() == 32);
+        coinbase.witness = uint256(witness_stack[0]);
+    }
+
+    coinbase.sequence = coinbase_tx->vin[0].nSequence;
+
+    // Extract only OP_RETURN coinbase outputs (witness commitment, merge
+    // mining, etc). BlockAssembler::CreateNewBlock adds a dummy output with
+    // the full reward that we must exclude.
+    for (const auto& output : coinbase_tx->vout) {
+        if (!output.scriptPubKey.empty() && output.scriptPubKey[0] == OP_RETURN) {
+            coinbase.required_outputs.push_back(output);
+        } else {
+            // The (single) dummy coinbase output produced by CreateBlock() has
+            // an nValue set to nFee + the Block Subsidy.
+            Assume(coinbase.value_remaining == 0);
+            coinbase.value_remaining = output.nValue;
+        }
+    }
+
+    coinbase.lock_time = coinbase_tx->nLockTime;
+
+    coinbase.default_witness_commitment = block_template.vchCoinbaseCommitment;
+
+    return coinbase;
+}
 } // namespace node

src/node/miner.h

@@ -259,6 +259,17 @@ std::optional<BlockRef> GetTip(ChainstateManager& chainman);
 /* Waits for the connected tip to change until timeout has elapsed. During node initialization, this will wait until the tip is connected (regardless of `timeout`).
  * Returns the current tip, or nullopt if the node is shutting down. */
 std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout);
+
+/*
+ * Extract relevant fields from the dummy coinbase transaction in the template.
+ *
+ * Extracts only OP_RETURN coinbase outputs, i.e. the witness commitment. If
+ * BlockAssembler::CreateNewBlock() is patched to add more OP_RETURN outputs
+ * e.g. for merge mining, those will be included. The dummy output that spends
+ * the full reward is excluded.
+ */
+node::CoinbaseTemplate ExtractCoinbaseTemplate(const node::CBlockTemplate& block_template);
+
 } // namespace node
 
 #endif // BITCOIN_NODE_MINER_H

src/node/types.h

@@ -16,10 +16,13 @@
 #include <consensus/amount.h>
 #include <cstddef>
 #include <cstdint>
+#include <optional>
 #include <policy/policy.h>
+#include <primitives/transaction.h>
 #include <script/script.h>
 #include <uint256.h>
 #include <util/time.h>
+#include <vector>
 
 namespace node {
 enum class TransactionError {
@@ -64,6 +67,23 @@ 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.
+     *
+     * IPC clients are expected to call getCoinbase() rather than use the dummy
+     * transaction directly. It's cleared by default, but RPC and test code
+     * 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 {
@@ -99,6 +119,52 @@ struct BlockCheckOptions {
     bool check_pow{true};
 };
 
+struct CoinbaseTemplate {
+    /* nVersion */
+    uint32_t version;
+    /* nSequence for the only coinbase transaction input */
+    uint32_t sequence;
+    /**
+     * Bytes which are to be placed at the beginning of scriptSig. Guaranteed
+     * to be less than 8 bytes (not including the length byte). This allows
+     * clients to add up to 92 bytes.
+     */
+    CScript script_sig_prefix;
+    /**
+     * The first (and only) witness stack element of the coinbase input.
+     *
+     * Omitted for block templates without witness data.
+     *
+     * This is currently the BIP 141 witness reserved value. A future soft fork
+     * may move the witness reserved value elsewhere, but there will still be a
+     * coinbase witness.
+     */
+    std::optional<uint256> witness;
+    /**
+     * Block subsidy plus fees, minus any non-zero required_outputs.
+     *
+     * Currently there are no non-zero required_outputs, see below.
+     */
+    CAmount value_remaining;
+    /*
+     * To be included as the last outputs in the coinbase transaction.
+     * Currently this is only the witness commitment OP_RETURN, but future
+     * softforks could add more.
+     * If a patch to BlockAssembler::CreateNewBlock() adds outputs e.g. for
+     * merge mining, those will be included. The dummy output that spends
+     * the full reward is excluded.
+     */
+    std::vector<CTxOut> required_outputs;
+    uint32_t lock_time;
+
+    /**
+     * Default witness commitment
+     *
+     * For getblocktemplate RPC, not exposed via IPC.
+     */
+    std::vector<unsigned char> default_witness_commitment;
+};
+
 /**
  * How to broadcast a local transaction.
  * Used to influence `BroadcastTransaction()` and its callers.

src/rpc/mining.cpp

@@ -165,7 +165,11 @@ 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,
+            .clear_coinbase = false,
+            .always_add_coinbase_commitment = true
+        }));
         CHECK_NONFATAL(block_template);
 
         std::shared_ptr<const CBlock> block_out;
@@ -376,7 +380,12 @@ 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,
+                .clear_coinbase = false,
+                .always_add_coinbase_commitment = true
+            })};
             CHECK_NONFATAL(block_template);
 
             block = block_template->getBlock();
@@ -871,7 +880,10 @@ static RPCHelpMan getblocktemplate()
         time_start = GetTime();
 
         // Create new block
-        block_template = miner.createNewBlock();
+        block_template = miner.createNewBlock({
+            .clear_coinbase = false,
+            .always_add_coinbase_commitment = true
+        });
         CHECK_NONFATAL(block_template);
 
 
@@ -1015,8 +1027,9 @@ static RPCHelpMan getblocktemplate()
         result.pushKV("signet_challenge", HexStr(consensusParams.signet_challenge));
     }
 
-    if (!block_template->getCoinbaseCommitment().empty()) {
-        result.pushKV("default_witness_commitment", HexStr(block_template->getCoinbaseCommitment()));
+    auto default_witness_commitment{block_template->getCoinbase().default_witness_commitment};
+    if (!default_witness_commitment.empty()) {
+        result.pushKV("default_witness_commitment", HexStr(default_witness_commitment));
     }
 
     return result;

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/miner_tests.cpp

@@ -692,6 +692,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;
     BlockAssembler::Options options;
     options.coinbase_output_script = scriptPubKey;
+    options.clear_coinbase = false;
 
     // Create and check a simple template
     std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(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);