Merge #33819: mining: getCoinbase() returns struct instead of raw tx

48f57bb mining: add new getCoinbaseTx() returning a struct (Sjors Provoost)
d59b4cd mining: rename getCoinbaseTx() to ..RawTx() (Sjors Provoost)

Pull request description:

  The first commit renames `getCoinbaseTx()` to `getCoinbaseRawTx()` to reflect that it returns a serialised transaction. This does not impact IPC clients, because they do not use the function name.

  The second commit then introduces a replacement `getCoinbase()` that provides a struct with everything clients need to construct a coinbase. This avoids clients having to parse and manipulate our dummy transaction.

  Deprecate but don't remove `getCoinbaseRawTx()`, `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.

  After this change we can drop these deprecated methods, which in turn would allow us to clear the dummy transaction from the `getBlock()` result. But that is left for a followup to keep this PR focussed. See Sjors#106 for an approach.

  Expand the `interface_ipc.py` functional test to document its usage.

  Can be tested using:
  - stratum-mining/sv2-tp#59

ACKs for top commit:
  ryanofsky:
    Code review ACK 48f57bb. Just rebased and addressed comments and dropped coinbase tx "template" suffix, which is a nice change
  ismaelsadeeq:
    code review ACK 48f57bb
  vasild:
    ACK 48f57bb

Tree-SHA512: c4f1d752777fb3086a1a0b7b8b06e4205dbe2f3adb41f218855ad1dee952adccc263cf82acd3bf9300cc83c2c64cebd2b27f66a69beee32d325b9a85e3643b0d

Author: ryanofsky

doc/release-notes-33819.md

@@ -0,0 +1,8 @@
+Mining IPC
+----------
+
+- The `getCoinbaseTx()` method is renamed to `getCoinbaseRawTx()` and deprecated.
+  IPC clients do not use the function name, so they're not affected. (#33819)
+- Adds `getCoinbaseTx()` which clients should use instead of `getCoinbaseRawTx()`. 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,8 +42,29 @@ class BlockTemplate
     // Sigop cost per transaction, not including coinbase transaction.
     virtual std::vector<int64_t> getTxSigops() = 0;
 
-    virtual CTransactionRef getCoinbaseTx() = 0;
+    /**
+     * Return serialized dummy coinbase transaction.
+     *
+     * @note deprecated: use getCoinbaseTx()
+     */
+    virtual CTransactionRef getCoinbaseRawTx() = 0;
+
+    /** Return fields needed to construct a coinbase transaction */
+    virtual node::CoinbaseTx getCoinbaseTx() = 0;
+
+    /**
+     * Return scriptPubKey with SegWit OP_RETURN.
+     *
+     * @note deprecated: use getCoinbaseTx()
+     */
     virtual std::vector<unsigned char> getCoinbaseCommitment() = 0;
+
+    /**
+     * Return which output in the dummy coinbase contains the SegWit OP_RETURN.
+     *
+     * @note deprecated. Scan outputs from getCoinbaseTx() outputs field for the
+     *       SegWit marker.
+     */
     virtual int getWitnessCommitmentIndex() = 0;
 
     /**

src/ipc/capnp/mining.capnp

@@ -27,7 +27,8 @@ 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);
+    getCoinbaseRawTx @5 (context: Proxy.Context) -> (result: Data);
+    getCoinbaseTx @12 (context: Proxy.Context) -> (result: CoinbaseTx);
     getCoinbaseCommitment @6 (context: Proxy.Context) -> (result: Data);
     getWitnessCommitmentIndex @7 (context: Proxy.Context) -> (result: Int32);
     getCoinbaseMerklePath @8 (context: Proxy.Context) -> (result: List(Data));
@@ -51,3 +52,13 @@ struct BlockCheckOptions $Proxy.wrap("node::BlockCheckOptions") {
     checkMerkleRoot @0 :Bool $Proxy.name("check_merkle_root");
     checkPow @1 :Bool $Proxy.name("check_pow");
 }
+
+struct CoinbaseTx $Proxy.wrap("node::CoinbaseTx") {
+    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");
+    blockRewardRemaining @4 :Int64 $Proxy.name("block_reward_remaining");
+    requiredOutputs @5 :List(Data) $Proxy.name("required_outputs");
+    lockTime @6 :UInt32 $Proxy.name("lock_time");
+}

src/node/interfaces.cpp

@@ -84,6 +84,7 @@ using interfaces::WalletLoader;
 using kernel::ChainstateRole;
 using node::BlockAssembler;
 using node::BlockWaitOptions;
+using node::CoinbaseTx;
 using util::Join;
 
 namespace node {
@@ -888,11 +889,16 @@ class BlockTemplateImpl : public BlockTemplate
         return m_block_template->vTxSigOpsCost;
     }
 
-    CTransactionRef getCoinbaseTx() override
+    CTransactionRef getCoinbaseRawTx() override
     {
         return m_block_template->block.vtx[0];
     }
 
+    CoinbaseTx getCoinbaseTx() override
+    {
+        return m_block_template->m_coinbase_tx;
+    }
+
     std::vector<unsigned char> getCoinbaseCommitment() override
     {
         return m_block_template->vchCoinbaseCommitment;

src/node/miner.cpp

@@ -158,18 +158,51 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
 
     // Create coinbase transaction.
     CMutableTransaction coinbaseTx;
+
+    // Construct coinbase transaction struct in parallel
+    CoinbaseTx& coinbase_tx{pblocktemplate->m_coinbase_tx};
+    coinbase_tx.version = coinbaseTx.version;
+
     coinbaseTx.vin.resize(1);
     coinbaseTx.vin[0].prevout.SetNull();
     coinbaseTx.vin[0].nSequence = CTxIn::MAX_SEQUENCE_NONFINAL; // Make sure timelock is enforced.
+    coinbase_tx.sequence = coinbaseTx.vin[0].nSequence;
+
+    // Add an output that spends the full coinbase reward.
     coinbaseTx.vout.resize(1);
     coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_output_script;
-    coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus());
+    // Block subsidy + fees
+    const CAmount block_reward{nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus())};
+    coinbaseTx.vout[0].nValue = block_reward;
+    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;
+    coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig;
     Assert(nHeight > 0);
     coinbaseTx.nLockTime = static_cast<uint32_t>(nHeight - 1);
+    coinbase_tx.lock_time = coinbaseTx.nLockTime;
+
     pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx));
     pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev);
 
+    const CTransactionRef& final_coinbase{pblock->vtx[0]};
+    if (final_coinbase->HasWitness()) {
+        const auto& witness_stack{final_coinbase->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_tx.witness = uint256(witness_stack[0]);
+    }
+    if (const int witness_index = GetWitnessCommitmentIndex(*pblock); witness_index != NO_WITNESS_COMMITMENT) {
+        Assert(witness_index >= 0 && static_cast<size_t>(witness_index) < final_coinbase->vout.size());
+        coinbase_tx.required_outputs.push_back(final_coinbase->vout[witness_index]);
+    }
+
     LogInfo("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost);
 
     // Fill in header
@@ -440,4 +473,5 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
     // avoid deadlocks.
     return GetTip(chainman);
 }
+
 } // namespace node

src/node/miner.h

@@ -50,6 +50,11 @@ struct CBlockTemplate
     /* A vector of package fee rates, ordered by the sequence in which
      * packages are selected for inclusion in the block template.*/
     std::vector<FeePerVSize> m_package_feerates;
+    /*
+     * Template containing all coinbase transaction fields that are set by our
+     * miner code.
+     */
+    CoinbaseTx m_coinbase_tx;
 };
 
 /** Generate a new block, without valid proof-of-work */
@@ -157,6 +162,7 @@ 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);
+
 } // 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 {
@@ -99,6 +102,53 @@ struct BlockCheckOptions {
     bool check_pow{true};
 };
 
+/**
+ * Template containing all coinbase transaction fields that are set by our
+ * miner code. Clients are expected to add their own outputs and typically
+ * also expand the scriptSig.
+ */
+struct CoinbaseTx {
+    /* nVersion */
+    uint32_t version;
+    /* nSequence for the only coinbase transaction input */
+    uint32_t sequence;
+    /**
+     * Prefix which needs to be placed at the beginning of the scriptSig.
+     * Clients may append extra data to this as long as the overall scriptSig
+     * size is 100 bytes or less, to avoid the block being rejected with
+     * "bad-cb-length" error.
+     *
+     * Currently with BIP 34, the prefix is guaranteed to be less than 8 bytes,
+     * but future soft forks could require longer prefixes.
+     */
+    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, and can be chosen
+     * arbitrarily by the node, but future soft forks may constrain it.
+     */
+    std::optional<uint256> witness;
+    /**
+     * Block subsidy plus fees, minus any non-zero required_outputs.
+     *
+     * Currently there are no non-zero required_outputs, so block_reward_remaining
+     * is the entire block reward. See also required_outputs.
+     */
+    CAmount block_reward_remaining;
+    /*
+     * To be included as the last outputs in the coinbase transaction.
+     * Currently this is only the witness commitment OP_RETURN, but future
+     * softforks or a custom mining patch could add more.
+     *
+     * The dummy output that spends the full reward is excluded.
+     */
+    std::vector<CTxOut> required_outputs;
+    uint32_t lock_time;
+};
+
 /**
  * How to broadcast a local transaction.
  * Used to influence `BroadcastTransaction()` and its callers.

test/functional/interface_ipc.py

@@ -6,16 +6,38 @@
 import asyncio
 import inspect
 from contextlib import asynccontextmanager, AsyncExitStack
+from dataclasses import dataclass
 from io import BytesIO
 from pathlib import Path
 import shutil
-from test_framework.messages import (CBlock, CTransaction, ser_uint256, COIN)
+from test_framework.blocktools import NULL_OUTPOINT
+from test_framework.messages import (
+    CBlock,
+    CTransaction,
+    CTxIn,
+    CTxOut,
+    CTxInWitness,
+    ser_uint256,
+    COIN,
+)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
     assert_not_equal
 )
 from test_framework.wallet import MiniWallet
+from typing import Optional
+
+# Stores the result of getCoinbaseTx()
+@dataclass
+class CoinbaseTxData:
+    version: int
+    sequence: int
+    scriptSigPrefix: bytes
+    witness: Optional[bytes]
+    blockRewardRemaining: int
+    requiredOutputs: list[bytes]
+    lockTime: int
 
 # Test may be skipped and not have capnp installed
 try:
@@ -123,11 +145,32 @@ async def parse_and_deserialize_block(self, block_template, ctx):
         return block
 
     async def parse_and_deserialize_coinbase_tx(self, block_template, ctx):
-        coinbase_data = BytesIO((await block_template.getCoinbaseTx(ctx)).result)
+        assert block_template is not None
+        coinbase_data = BytesIO((await block_template.getCoinbaseRawTx(ctx)).result)
         tx = CTransaction()
         tx.deserialize(coinbase_data)
         return tx
 
+    async def parse_and_deserialize_coinbase(self, block_template, ctx) -> CoinbaseTxData:
+        assert block_template is not None
+        # Note: the template_capnp struct will be garbage-collected when this
+        # method returns, so it is important to copy any Data fields from it
+        # which need to be accessed later using the bytes() cast. Starting with
+        # pycapnp v2.2.0, Data fields have type `memoryview` and are ephemeral.
+        template_capnp = (await block_template.getCoinbaseTx(ctx)).result
+        witness: Optional[bytes] = None
+        if template_capnp._has("witness"):
+            witness = bytes(template_capnp.witness)
+        return CoinbaseTxData(
+            version=int(template_capnp.version),
+            sequence=int(template_capnp.sequence),
+            scriptSigPrefix=bytes(template_capnp.scriptSigPrefix),
+            witness=witness,
+            blockRewardRemaining=int(template_capnp.blockRewardRemaining),
+            requiredOutputs=[bytes(output) for output in template_capnp.requiredOutputs],
+            lockTime=int(template_capnp.lockTime),
+        )
+
     def run_echo_test(self):
         self.log.info("Running echo test")
         async def async_routine():
@@ -142,6 +185,54 @@ 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 getCoinbaseTx()")
+        assert template is not None
+        coinbase_res = await self.parse_and_deserialize_coinbase(template, ctx)
+        coinbase_tx = CTransaction()
+        coinbase_tx.version = coinbase_res.version
+        coinbase_tx.vin = [CTxIn()]
+        coinbase_tx.vin[0].prevout = NULL_OUTPOINT
+        coinbase_tx.vin[0].nSequence = coinbase_res.sequence
+        # Typically a mining pool appends its name and an extraNonce
+        coinbase_tx.vin[0].scriptSig = coinbase_res.scriptSigPrefix
+
+        # We currently always provide a coinbase witness, even for empty
+        # blocks, but this may change, so always check:
+        has_witness = coinbase_res.witness is not None
+        if has_witness:
+            coinbase_tx.wit.vtxinwit = [CTxInWitness()]
+            coinbase_tx.wit.vtxinwit[0].scriptWitness.stack = [coinbase_res.witness]
+
+        # First output is our payout
+        coinbase_tx.vout = [CTxOut()]
+        coinbase_tx.vout[0].scriptPubKey = miniwallet.get_output_script()
+        coinbase_tx.vout[0].nValue = coinbase_res.blockRewardRemaining
+        # Add SegWit OP_RETURN. This is currently always present even for
+        # empty blocks, but this may change.
+        found_witness_op_return = False
+        # Compare SegWit OP_RETURN to getCoinbaseCommitment()
+        coinbase_commitment = (await template.getCoinbaseCommitment(ctx)).result
+        for output_data in coinbase_res.requiredOutputs:
+            output = CTxOut()
+            output.deserialize(BytesIO(output_data))
+            coinbase_tx.vout.append(output)
+            if output.scriptPubKey == coinbase_commitment:
+                found_witness_op_return = True
+
+        assert_equal(has_witness, found_witness_op_return)
+
+        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)
+        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
+        assert_equal(coinbase_tx.serialize().hex(), coinbase_legacy.serialize().hex())
+
+        return coinbase_tx
+
     def run_mining_test(self):
         self.log.info("Running mining test")
         block_hash_size = 32
@@ -191,7 +282,7 @@ async def async_routine():
                 assert_equal(len(txfees.result), 0)
                 txsigops = await template.getTxSigops(ctx)
                 assert_equal(len(txsigops.result), 0)
-                coinbase_data = BytesIO((await template.getCoinbaseTx(ctx)).result)
+                coinbase_data = BytesIO((await template.getCoinbaseRawTx(ctx)).result)
                 coinbase = CTransaction()
                 coinbase.deserialize(coinbase_data)
                 assert_equal(coinbase.vin[0].prevout.hash, 0)
@@ -248,9 +339,9 @@ async def wait_for_block():
             check_opts = self.capnp_modules['mining'].BlockCheckOptions()
             async with destroying((await mining.createNewBlock(opts)).result, ctx) as template:
                 block = await self.parse_and_deserialize_block(template, ctx)
-                coinbase = await self.parse_and_deserialize_coinbase_tx(template, ctx)
                 balance = miniwallet.get_balance()
-                coinbase.vout[0].scriptPubKey = miniwallet.get_output_script()
+                coinbase = await self.build_coinbase_test(template, ctx, miniwallet)
+                # Reduce payout for balance comparison simplicity
                 coinbase.vout[0].nValue = COIN
                 block.vtx[0] = coinbase
                 block.hashMerkleRoot = block.calc_merkle_root()