Merge bitcoin/bitcoin#31318: Drop script_pub_key arg from createNewBlock

52fd151 test: drop scriptPubKeyIn arg from CreateNewBlock (Sjors Provoost)
ff41b9e Drop script_pub_key arg from createNewBlock (Sjors Provoost)
7ab733e rpc: rename coinbase_script to coinbase_output_script (Sjors Provoost)

Pull request description:

  Providing a script for the coinbase transaction is only done in test code and for (unoptimized) CPU solo mining.

  Production miners use the `getblocktemplate` RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.

  This commit removes the `script_pub_key argument` from `createNewBlock()` in the Mining interface.

  A coinbase script can still be passed via `BlockCreateOptions` instead. Tests are modified to do so.

ACKs for top commit:
  ryanofsky:
    Code review ACK 52fd151. No change since last review other than rebase
  TheCharlatan:
    Re-ACK 52fd151
  vasild:
    ACK 52fd151

Tree-SHA512: c4b3a53774d9a5dc90950e77f47a64dbb68f971baffbb9a0d8f59332ef8e52d0c039130c925bde73135b3d0e79e65d91d1df30dc4cff13f32d8a72e5c56669d8

src/bench/block_assemble.cpp

@@ -20,19 +20,23 @@
 #include <memory>
 #include <vector>
 
+using node::BlockAssembler;
+
 static void AssembleBlock(benchmark::Bench& bench)
 {
     const auto test_setup = MakeNoLogFileContext<const TestingSetup>();
 
     CScriptWitness witness;
     witness.stack.push_back(WITNESS_STACK_ELEM_OP_TRUE);
+    BlockAssembler::Options options;
+    options.coinbase_output_script = P2WSH_OP_TRUE;
 
     // Collect some loose transactions that spend the coinbases of our mined blocks
     constexpr size_t NUM_BLOCKS{200};
     std::array<CTransactionRef, NUM_BLOCKS - COINBASE_MATURITY + 1> txs;
     for (size_t b{0}; b < NUM_BLOCKS; ++b) {
         CMutableTransaction tx;
-        tx.vin.emplace_back(MineBlock(test_setup->m_node, P2WSH_OP_TRUE));
+        tx.vin.emplace_back(MineBlock(test_setup->m_node, options));
         tx.vin.back().scriptWitness = witness;
         tx.vout.emplace_back(1337, P2WSH_OP_TRUE);
         if (NUM_BLOCKS - b >= COINBASE_MATURITY)
@@ -48,19 +52,20 @@ static void AssembleBlock(benchmark::Bench& bench)
     }
 
     bench.run([&] {
-        PrepareBlock(test_setup->m_node, P2WSH_OP_TRUE);
+        PrepareBlock(test_setup->m_node, options);
     });
 }
 static void BlockAssemblerAddPackageTxns(benchmark::Bench& bench)
 {
     FastRandomContext det_rand{true};
     auto testing_setup{MakeNoLogFileContext<TestChain100Setup>()};
     testing_setup->PopulateMempool(det_rand, /*num_transactions=*/1000, /*submit=*/true);
-    node::BlockAssembler::Options assembler_options;
+    BlockAssembler::Options assembler_options;
     assembler_options.test_block_validity = false;
+    assembler_options.coinbase_output_script = P2WSH_OP_TRUE;
 
     bench.run([&] {
-        PrepareBlock(testing_setup->m_node, P2WSH_OP_TRUE, assembler_options);
+        PrepareBlock(testing_setup->m_node, assembler_options);
     });
 }
 

src/interfaces/mining.h

@@ -88,11 +88,10 @@ class Mining
    /**
      * Construct a new block template
      *
-     * @param[in] script_pub_key the coinbase output
      * @param[in] options options for creating the block
      * @returns a block template
      */
-    virtual std::unique_ptr<BlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options = {}) = 0;
+    virtual std::unique_ptr<BlockTemplate> createNewBlock(const node::BlockCreateOptions& options = {}) = 0;
 
     /**
      * Processes new block. A valid new block is automatically relayed to peers.

src/ipc/capnp/mining.capnp

@@ -17,7 +17,7 @@ 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 (scriptPubKey: Data, options: BlockCreateOptions) -> (result: BlockTemplate);
+    createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate);
     processNewBlock @5 (context :Proxy.Context, block: Data) -> (newBlock: Bool, result: Bool);
     getTransactionsUpdated @6 (context :Proxy.Context) -> (result: UInt32);
     testBlockValidity @7 (context :Proxy.Context, block: Data, checkMerkleRoot: Bool) -> (state: BlockValidationState, result: Bool);

src/node/interfaces.cpp

@@ -1004,11 +1004,11 @@ class MinerImpl : public Mining
         return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root);
     }
 
-    std::unique_ptr<BlockTemplate> createNewBlock(const CScript& script_pub_key, const BlockCreateOptions& options) override
+    std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
     {
         BlockAssembler::Options assemble_options{options};
         ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
-        return std::make_unique<BlockTemplateImpl>(BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(script_pub_key), m_node);
+        return std::make_unique<BlockTemplateImpl>(BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
     }
 
     NodeContext* context() override { return &m_node; }

src/node/miner.cpp

@@ -106,7 +106,7 @@ void BlockAssembler::resetBlock()
     nFees = 0;
 }
 
-std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
+std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
 {
     const auto time_start{SteadyClock::now()};
 
@@ -151,7 +151,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     coinbaseTx.vin.resize(1);
     coinbaseTx.vin[0].prevout.SetNull();
     coinbaseTx.vout.resize(1);
-    coinbaseTx.vout[0].scriptPubKey = scriptPubKeyIn;
+    coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_output_script;
     coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus());
     coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0;
     pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx));

src/node/miner.h

@@ -169,8 +169,8 @@ class BlockAssembler
 
     explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options);
 
-    /** Construct a new block template with coinbase to scriptPubKeyIn */
-    std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
+    /** Construct a new block template */
+    std::unique_ptr<CBlockTemplate> CreateNewBlock();
 
     inline static std::optional<int64_t> m_last_block_num_txs{};
     inline static std::optional<int64_t> m_last_block_weight{};

src/node/types.h

@@ -3,8 +3,8 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 //! @file node/types.h is a home for public enum and struct type definitions
-//! that are used by internally by node code, but also used externally by wallet
-//! or GUI code.
+//! that are used by internally by node code, but also used externally by wallet,
+//! mining or GUI code.
 //!
 //! This file is intended to define only simple types that do not have external
 //! dependencies. More complicated types should be defined in dedicated header
@@ -14,6 +14,7 @@
 #define BITCOIN_NODE_TYPES_H
 
 #include <cstddef>
+#include <script/script.h>
 
 namespace node {
 enum class TransactionError {
@@ -43,6 +44,22 @@ struct BlockCreateOptions {
      * transaction outputs.
      */
     size_t coinbase_output_max_additional_sigops{400};
+    /**
+     * Script to put in the coinbase transaction. The default is an
+     * anyone-can-spend dummy.
+     *
+     * Should only be used for tests, when the default doesn't suffice.
+     *
+     * Note that higher level code like the getblocktemplate RPC may omit the
+     * coinbase transaction entirely. It's instead constructed by pool software
+     * using fields like coinbasevalue, coinbaseaux and default_witness_commitment.
+     * This software typically also controls the payout outputs, even for solo
+     * mining.
+     *
+     * The size and sigops are not checked against
+     * coinbase_max_additional_weight and coinbase_output_max_additional_sigops.
+     */
+    CScript coinbase_output_script{CScript() << OP_TRUE};
 };
 } // namespace node
 

src/rpc/mining.cpp

@@ -158,11 +158,11 @@ static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock&& b
     return true;
 }
 
-static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
+static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const CScript& coinbase_output_script, int nGenerate, uint64_t nMaxTries)
 {
     UniValue blockHashes(UniValue::VARR);
     while (nGenerate > 0 && !chainman.m_interrupt) {
-        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock(coinbase_script));
+        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({ .coinbase_output_script = coinbase_output_script }));
         CHECK_NONFATAL(block_template);
 
         std::shared_ptr<const CBlock> block_out;
@@ -236,17 +236,17 @@ static RPCHelpMan generatetodescriptor()
     const auto num_blocks{self.Arg<int>("num_blocks")};
     const auto max_tries{self.Arg<uint64_t>("maxtries")};
 
-    CScript coinbase_script;
+    CScript coinbase_output_script;
     std::string error;
-    if (!getScriptFromDescriptor(self.Arg<std::string>("descriptor"), coinbase_script, error)) {
+    if (!getScriptFromDescriptor(self.Arg<std::string>("descriptor"), coinbase_output_script, error)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
     }
 
     NodeContext& node = EnsureAnyNodeContext(request.context);
     Mining& miner = EnsureMining(node);
     ChainstateManager& chainman = EnsureChainman(node);
 
-    return generateBlocks(chainman, miner, coinbase_script, num_blocks, max_tries);
+    return generateBlocks(chainman, miner, coinbase_output_script, num_blocks, max_tries);
 },
     };
 }
@@ -292,9 +292,9 @@ static RPCHelpMan generatetoaddress()
     Mining& miner = EnsureMining(node);
     ChainstateManager& chainman = EnsureChainman(node);
 
-    CScript coinbase_script = GetScriptForDestination(destination);
+    CScript coinbase_output_script = GetScriptForDestination(destination);
 
-    return generateBlocks(chainman, miner, coinbase_script, num_blocks, max_tries);
+    return generateBlocks(chainman, miner, coinbase_output_script, num_blocks, max_tries);
 },
     };
 }
@@ -328,16 +328,16 @@ static RPCHelpMan generateblock()
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
     const auto address_or_descriptor = request.params[0].get_str();
-    CScript coinbase_script;
+    CScript coinbase_output_script;
     std::string error;
 
-    if (!getScriptFromDescriptor(address_or_descriptor, coinbase_script, error)) {
+    if (!getScriptFromDescriptor(address_or_descriptor, coinbase_output_script, error)) {
         const auto destination = DecodeDestination(address_or_descriptor);
         if (!IsValidDestination(destination)) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Error: Invalid address or descriptor");
         }
 
-        coinbase_script = GetScriptForDestination(destination);
+        coinbase_output_script = GetScriptForDestination(destination);
     }
 
     NodeContext& node = EnsureAnyNodeContext(request.context);
@@ -371,7 +371,7 @@ static RPCHelpMan generateblock()
 
     ChainstateManager& chainman = EnsureChainman(node);
     {
-        std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock(coinbase_script, {.use_mempool = false})};
+        std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({.use_mempool = false, .coinbase_output_script = coinbase_output_script})};
         CHECK_NONFATAL(block_template);
 
         block = block_template->getBlock();
@@ -814,8 +814,7 @@ static RPCHelpMan getblocktemplate()
         time_start = GetTime();
 
         // Create new block
-        CScript scriptDummy = CScript() << OP_TRUE;
-        block_template = miner.createNewBlock(scriptDummy);
+        block_template = miner.createNewBlock();
         CHECK_NONFATAL(block_template);
 
 

src/test/blockfilter_index_tests.cpp

@@ -67,8 +67,9 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev,
     const std::vector<CMutableTransaction>& txns,
     const CScript& scriptPubKey)
 {
-     BlockAssembler::Options options;
-    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}.CreateNewBlock(scriptPubKey);
+    BlockAssembler::Options options;
+    options.coinbase_output_script = scriptPubKey;
+    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}.CreateNewBlock();
     CBlock& block = pblocktemplate->block;
     block.hashPrevBlock = prev->GetBlockHash();
     block.nTime = prev->nTime + 1;

src/test/fuzz/mini_miner.cpp

@@ -174,15 +174,15 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
     miner_options.blockMinFeeRate = target_feerate;
     miner_options.nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT;
     miner_options.test_block_validity = false;
+    miner_options.coinbase_output_script = CScript() << OP_0;
 
     node::BlockAssembler miner{g_setup->m_node.chainman->ActiveChainstate(), &pool, miner_options};
     node::MiniMiner mini_miner{pool, outpoints};
     assert(mini_miner.IsReadyToCalculate());
 
-    CScript spk_placeholder = CScript() << OP_0;
     // Use BlockAssembler as oracle. BlockAssembler and MiniMiner should select the same
     // transactions, stopping once packages do not meet target_feerate.
-    const auto blocktemplate{miner.CreateNewBlock(spk_placeholder)};
+    const auto blocktemplate{miner.CreateNewBlock()};
     mini_miner.BuildMockTemplate(target_feerate);
     assert(!mini_miner.IsReadyToCalculate());
     auto mock_template_txids = mini_miner.GetMockTemplateTxids();

src/test/fuzz/package_eval.cpp

@@ -21,6 +21,7 @@
 #include <validation.h>
 #include <validationinterface.h>
 
+using node::BlockAssembler;
 using node::NodeContext;
 
 namespace {
@@ -42,8 +43,11 @@ void initialize_tx_pool()
     static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
     g_setup = testing_setup.get();
 
+    BlockAssembler::Options options;
+    options.coinbase_output_script = P2WSH_EMPTY;
+
     for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) {
-        COutPoint prevout{MineBlock(g_setup->m_node, P2WSH_EMPTY)};
+        COutPoint prevout{MineBlock(g_setup->m_node, options)};
         if (i < COINBASE_MATURITY) {
             // Remember the txids to avoid expensive disk access later on
             g_outpoints_coinbase_init_mature.push_back(prevout);

src/test/fuzz/process_message.cpp

@@ -45,7 +45,7 @@ void initialize_process_message()
             {.extra_args = {"-txreconciliation"}});
     g_setup = testing_setup.get();
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
-        MineBlock(g_setup->m_node, CScript() << OP_TRUE);
+        MineBlock(g_setup->m_node, {});
     }
     g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
 }

src/test/fuzz/process_messages.cpp

@@ -35,7 +35,7 @@ void initialize_process_messages()
             {.extra_args = {"-txreconciliation"}});
     g_setup = testing_setup.get();
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
-        MineBlock(g_setup->m_node, CScript() << OP_TRUE);
+        MineBlock(g_setup->m_node, {});
     }
     g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
 }

src/test/fuzz/tx_pool.cpp

@@ -45,8 +45,11 @@ void initialize_tx_pool()
     static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
     g_setup = testing_setup.get();
 
+    BlockAssembler::Options options;
+    options.coinbase_output_script = P2WSH_OP_TRUE;
+
     for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) {
-        COutPoint prevout{MineBlock(g_setup->m_node, P2WSH_OP_TRUE)};
+        COutPoint prevout{MineBlock(g_setup->m_node, options)};
         // Remember the txids to avoid expensive disk access later on
         auto& outpoints = i < COINBASE_MATURITY ?
                               g_outpoints_coinbase_init_mature :
@@ -98,7 +101,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
         options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT);
         options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)};
         auto assembler = BlockAssembler{chainstate, &tx_pool, options};
-        auto block_template = assembler.CreateNewBlock(CScript{} << OP_TRUE);
+        auto block_template = assembler.CreateNewBlock();
         Assert(block_template->block.vtx.size() >= 1);
     }
     const auto info_all = tx_pool.infoAll();

src/test/fuzz/utxo_total_supply.cpp

@@ -17,6 +17,8 @@
 #include <util/chaintype.h>
 #include <validation.h>
 
+using node::BlockAssembler;
+
 FUZZ_TARGET(utxo_total_supply)
 {
     /** The testing setup that creates a chainman only (no chainstate) */
@@ -36,9 +38,11 @@ FUZZ_TARGET(utxo_total_supply)
         LOCK(chainman.GetMutex());
         return chainman.ActiveHeight();
     };
+    BlockAssembler::Options options;
+    options.coinbase_output_script = CScript() << OP_FALSE;
     const auto PrepareNextBlock = [&]() {
         // Use OP_FALSE to avoid BIP30 check from hitting early
-        auto block = PrepareBlock(node, CScript{} << OP_FALSE);
+        auto block = PrepareBlock(node, options);
         // Replace OP_FALSE with OP_TRUE
         {
             CMutableTransaction tx{*block->vtx.back()};