refactor: add coinbase constraints to BlockAssembler::Options

[Sjors]

trafficstars

When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs.

At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually produced an invalid block which exceeded the sigops limit.

The existince of such patches suggests it may be useful to make this value configurable. This PR would make such a change easier. However, the main motivation is that in the Stratum v2 spec requires the pool to communicate the maximum bytes they intend to add to the coinbase outputs.

Specifically the CoinbaseOutputDataSize message which is part of the Template Distribution Protocol has a field coinbase_output_max_additional_size.

A proposed change to the spec adds the max additional sigops as well: https://github.com/stratum-mining/sv2-spec/pull/86. Whether that change makes it into the spec is not important though, as adding both to BlockAssembler::Options makes sense.

The first commit is a test refactor followup for #30335, related to the code that's changed here, but not required.

The second commit adds coinbase_output_max_additional_size and coinbase_output_max_additional_sigops to BlockAssembler::Options, with their defaults set to the existing hardcoded values.

The third commit introduces util/mining.h as a place to store these defaults in a place where both the RPC and node interface can access them. I picked libbitcoin_util instead of libbitcoin_common to avoid a future dependency of the Stratum v2 Template Provider (#29432) on libbitcoin_common. The latter goal requires additional changes and is mostly just nice to have (the important bit is not depending on libbitcoin_node).

The last commit adds the two arguments to the mining interface. By default it uses the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	itornaza, ryanofsky, ismaelsadeeq
Concept NACK	luke-jr
Stale ACK	tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #30475 (Stratum v2 Template Provider common functionality by Sjors)
• #30443 (Introduce waitFeesChanged() mining interface by Sjors)
• #30440 (Have createNewBlock() return a BlockTemplate interface by Sjors)
• #30437 (multiprocess: add bitcoin-mine test program by ryanofsky)
• #30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals by Sjors)
• #30391 (BlockAssembler: return selected packages vsize and feerate by ismaelsadeeq)
• #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
• #29432 (Stratum v2 Template Provider (take 3) by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[TheCharlatan]

I picked libbitcoin_util instead of libbitcoin_common to avoid a future dependency of the Stratum v2 Template Provider (https://github.com/bitcoin/bitcoin/pull/29432) on libbitcoin_common. The latter goal requires additional changes and is mostly just nice to have (the important bit is not depending on libbitcoin_node).

So I guess it should ideally only have the kernel as a dependency? It is not quite clear to me yet how this is going to be achieved. The current mining interface requires knowledge of a CBlockTemplate, which I don't think should be part of the kernel in the future.

EDIT: To be clear, since the constants are given a new header, there is no new hard library dependency introduced no matter which directory you choose to put them into. It just feels weird to put something very specific to the inner workings of our mining code into a directory called util.

[Sjors]

@TheCharlatan happy to put the constants somewhere else... see https://github.com/Sjors/bitcoin/pull/47 for an attempt at introducing libbitcoin_net, which might reduce the dependencies of a future libbitcoin_sv2 to libbitcoin_net, libbitcoin_crypto and libbitcoin_util. I got stuck there on where to put XOnlyPubKey and a few other things.

A similar problem might exist for CBlockTemplate if I actually tried to make libbitcoin_sv2, which I haven't done yet.

[Sjors]

I renamed coinbase_output_max_additional_size to coinbase_max_additional_weight. With the current Stratum v2 spec the latter could be calculated as follows:

coinbase_max_additional_weight = (coinbase_output_max_additional_size + 100 + 0 + 2) * 4

• coinbase_output_max_additional_size * 4: Coinbase outputs are not part of the witness so their weight is simply the number of bytes specificied in the CoinbaseOutputDataSize message times 4.

• 100 * 4: the spec also requires taking into account a "maximally-sized (100 byte) coinbase field", which refers to the scriptSig of the coinbase. It currently doesn't allow for setting a custom coinbase witness.

• 0 * 4: a CompactSize encodes the size of the above "coinbase field" (the spec is ambiguous whether this is included). Given the maximum of 100 its always encoded with 1 byte, so no additional weight units are needed.

• 2 * 4: "Further, the Template Provider MUST consider the maximum additional bytes required in the output count variable-length integer in the coinbase transaction when complying with the size limits." - this is tricky because we don't know how many outputs the pool intends to add, and we might want to add an arbitrary number of outputs ourselves. Since there can't be more than 1 million outputs, 3 bytes is always enough to encode the number of outputs. So that's 2 additional bytes in the worst case.

The spec should probably be clarified a bit, cc @TheBlueMatt, @Fi3.

In any case, tracking coinbase reserved space in term of weight units here should be enough to support Stratum v2 later, and even allow for using the coinbase witness.

[Sjors]

Rebased after #30324.

[TheCharlatan]

Is there a reason why

BlockAssembler::Options options;
ApplyArgsManOptions(gArgs, options);

has to be inside the block assembler class? Why not construct them externally and pass a reference of the options to the BlockAssembler? This seems a bit more efficient, since the options don't have to be constructed and the arguments not read every time a template is retrieved.

[Sjors]

cc @glozow who added this as part of #26695.

[glozow]

Is there a reason why

BlockAssembler::Options options;
ApplyArgsManOptions(gArgs, options);

has to be inside the block assembler class?

It doesn't need to be inside BlockAssembler. Prior to #26695, the ctor was using the global gArgs to decide on parameters (yuck!). We didn't have the node/*args kernel/*opts conventions at the time and I was pretty happy adding an ApplyArgsMan that didn't do that.

Why not construct them externally and pass a reference of the options to the BlockAssembler?

That sounds fine.

[luke-jr]

Weak approach NACK. This is just limiting the max block weight, which is already a configurable parameter. Having two params for the same thing seems like a poor way to do it. I'm not sure it avoids the risk of bugs producing invalid blocks, either - it just moves the bug risk from one place to another equally probable.

Instead, maybe just perform the block weight cap on initialization rather than block assembly?

[Sjors]

This is just limiting the max block weight

This had me confused initially, and I tried to combine them, but there's really two different things:

1. The max block weight demanded by the user via -blockmaxweight
2. Weight units reserved for the coinbase

With stratum v1 the distinction isn't very important. We just subtract (2) from (1). We could indeed do that at initialization.

But with Stratum v2 (2) is a per-client setting communicated to us (the Template Provider) via the CoinbaseOutputDataSize message. We can set it at connection time, but not during node initialization.

I'm not sure it avoids the risk of bugs producing invalid blocks, either - it just moves the bug risk from one place to another equally probable.

The fact that Stratum v2 makes this configurable introduces that risk. But removing that ability from the protocol would probably just cause miners to roll their own extension and do it anyway. At 500 sats/byte and $50K per coin, squeezing in an extra 1000 vbyte gets you $250 per block. Pennies in front of steamrollers perhaps, but we've seen miners do this with the current code.

Why not construct them externally and pass a reference of the options to the BlockAssembler?

That sounds fine.

Happy to take patches.

[ryanofsky]

Code review ACK e67e4669a9e263d90f0e276c8e65e93c39170375, but I think the current implementation is too complicated and has too many layers of indirection.

Would suggest applying the following patch and squashing the PR down to a single commit which would be just +41/-16 with the suggested changes:

diff

--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -313,7 +313,6 @@ BITCOIN_CORE_H = \
   util/hasher.h \
   util/insert.h \
   util/macros.h \
-  util/mining.h \
   util/moneystr.h \
   util/overflow.h \
   util/overloaded.h \
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -5,10 +5,11 @@
 #ifndef BITCOIN_INTERFACES_MINING_H
 #define BITCOIN_INTERFACES_MINING_H
 
+#include <node/types.h>
+#include <uint256.h>
+
 #include <memory>
 #include <optional>
-#include <uint256.h>
-#include <util/mining.h>
 
 namespace node {
 struct CBlockTemplate;
@@ -42,14 +43,10 @@ public:
      * Construct a new block template
      *
      * @param[in] script_pub_key the coinbase output
-     * @param[in] use_mempool set false to omit mempool transactions
-     * @param[in] coinbase_max_additional_weight maximum additional weight which the pool will add to the coinbase transaction
-     * @param[in] coinbase_output_max_additional_sigops maximum additional sigops which the pool will add in coinbase transaction outputs
+     * @param[in] options options for creating the block
      * @returns a block template
      */
-    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true,
-                                                                 size_t coinbase_max_additional_weight = DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT,
-                                                                 size_t coinbase_output_max_additional_sigops = DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS) = 0;
+    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options={}) = 0;
 
     /**
      * Processes new block. A valid new block is automatically relayed to peers.
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -883,25 +883,11 @@ public:
         return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root);
     }
 
-    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool,
-                                                   size_t coinbase_max_additional_weight,
-                                                   size_t coinbase_output_max_additional_sigops) override
+    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, const BlockCreateOptions& options) override
     {
-        BlockAssembler::Options options;
-        ApplyArgsManOptions(gArgs, options);
-
-        Assume(coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
-        options.coinbase_max_additional_weight = coinbase_max_additional_weight;
-
-        Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
-        options.coinbase_output_max_additional_sigops = coinbase_output_max_additional_sigops;
-
-        // The BlockAssembler constructor calls ClampOptions which clamps
-        // nBlockMaxWeight between coinbase_output_max_additional_size and
-        // DEFAULT_BLOCK_MAX_WEIGHT.
-        // In other words, coinbase (reserved) outputs can safely exceed
-        // -blockmaxweight, but the rest of the block template will be empty.
-        return BlockAssembler{chainman().ActiveChainstate(), use_mempool ? context()->mempool.get() : nullptr, options}.CreateNewBlock(script_pub_key);
+        BlockAssembler::Options assemble_options{options};
+        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
+        return BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(script_pub_key);
     }
 
     NodeContext* context() override { return &m_node; }
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -59,14 +59,17 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 
 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 {
+    Assume(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
+    Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
     // Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
+    // Coinbase (reserved) outputs can safely exceed -blockmaxweight, but the rest of the block template will be empty.
     options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT);
     return options;
 }
 
 BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options)
     : chainparams{chainstate.m_chainman.GetParams()},
-      m_mempool{mempool},
+      m_mempool{options.use_mempool ? mempool : nullptr},
       m_chainstate{chainstate},
       m_options{ClampOptions(options)}
 {
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -6,6 +6,7 @@
 #ifndef BITCOIN_NODE_MINER_H
 #define BITCOIN_NODE_MINER_H
 
+#include <node/types.h>
 #include <policy/policy.h>
 #include <primitives/block.h>
 #include <txmempool.h>
@@ -20,8 +21,6 @@
 #include <boost/multi_index/tag.hpp>
 #include <boost/multi_index_container.hpp>
 
-#include <util/mining.h>
-
 class ArgsManager;
 class CBlockIndex;
 class CChainParams;
@@ -155,24 +154,13 @@ private:
     Chainstate& m_chainstate;
 
 public:
-    struct Options {
+    struct Options : BlockCreateOptions {
         // Configuration parameters for the block size
         size_t nBlockMaxWeight{DEFAULT_BLOCK_MAX_WEIGHT};
         CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
         // Whether to call TestBlockValidity() at the end of CreateNewBlock().
         bool test_block_validity{true};
         bool print_modified_fee{DEFAULT_PRINT_MODIFIED_FEE};
-        /**
-         * The maximum additional weight which the pool will add to the coinbase
-         * scriptSig, witness and outputs. This must include any additional
-         * weight needed for larger CompactSize encoded lengths.
-         */
-        size_t coinbase_max_additional_weight{DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT};
-        /**
-         * The maximum additional sigops which the pool will add in coinbase
-         * transaction outputs.
-         */
-        size_t coinbase_output_max_additional_sigops{DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS};
     };
 
     explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options);
--- a/src/node/types.h
+++ b/src/node/types.h
@@ -13,6 +13,8 @@
 #ifndef BITCOIN_NODE_TYPES_H
 #define BITCOIN_NODE_TYPES_H
 
+#include <cstddef>
+
 namespace node {
 enum class TransactionError {
     OK, //!< No error
@@ -24,6 +26,24 @@ enum class TransactionError {
     MAX_BURN_EXCEEDED,
     INVALID_PACKAGE,
 };
+
+struct BlockCreateOptions {
+    /**
+     * Set false to omit mempool transactions
+     */
+    bool use_mempool{true};
+    /**
+     * The maximum additional weight which the pool will add to the coinbase
+     * scriptSig, witness and outputs. This must include any additional
+     * weight needed for larger CompactSize encoded lengths.
+     */
+    size_t coinbase_max_additional_weight{4000};
+    /**
+     * The maximum additional sigops which the pool will add in coinbase
+     * transaction outputs.
+     */
+    size_t coinbase_output_max_additional_sigops{400};
+};
 } // namespace node
 
 #endif // BITCOIN_NODE_TYPES_H
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -371,7 +371,7 @@ static RPCHelpMan generateblock()
 
     ChainstateManager& chainman = EnsureChainman(node);
     {
-        std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, /*use_mempool=*/false)};
+        std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, {.use_mempool = false})};
         if (!blocktemplate) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
         }
deleted file mode 100644
--- a/src/util/mining.h
+++ /dev/null
@@ -1,13 +0,0 @@
-// Copyright (c) 2024 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#ifndef BITCOIN_UTIL_MINING_H
-#define BITCOIN_UTIL_MINING_H
-
-/** Default for coinbase_max_additional_weight */
-static constexpr unsigned int DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT{4000};
-/** Default for coinbase_output_max_additional_sigops */
-static constexpr unsigned int DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS{400};
-
-#endif // BITCOIN_UTIL_MINING_H

Suggested changes:

• Make createNewBlock use an options struct so it easier and safer to use (less likely to have bugs from mixing up the order of integer parameters).
• Consolidates documentation for the options in one header instead of three headers.
• Avoids adding util/mining.h header since the options more logically belong to the node library than the util library.
• Moves implementation of mining options from node/interfaces.cpp to node/miner.cpp. The interfaces.cpp file is just meant to contain glue code exposing private implementations as public interfaces. Ideally, the interface methods should be 1-3 lines long and not implement real functionality, which belongs in other places like validation.cpp and miner.cpp so it can be found and reused more easily and better tested.

[Sjors]

@ryanofsky thanks! I'll look into using your patch. After that I'll check where @ismaelsadeeq's feedback still applies.

[Sjors]

I took @ryanofsky's patch and split it into two commits.

[Sjors]

@ismaelsadeeq thanks, updated the description.

[glozow]

post merge ACK