refactor: add coinbase constraints to BlockAssembler::Options

Sjors commented at 8:56 am on June 28, 2024: member

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 introduces BlockCreateOptions, with just use_mempool.

The thirds commit adds coinbase_max_additional_weight and coinbase_output_max_additional_sigops to BlockCreateOptions. They use the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432.

DrahtBot commented at 8:56 am on June 28, 2024: contributor

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.

DrahtBot added the label Refactoring on Jun 28, 2024

in src/interfaces/mining.h:46 in 4c57f2707e outdated

42@@ -42,9 +43,13 @@ class Mining
43      *
44      * @param[in] script_pub_key the coinbase output
45      * @param[in] use_mempool set false to omit mempool transactions
46+     * @param[in] coinbase_output_max_additional_size maximum additional serialized bytes which the pool will add in coinbase transaction outputs

tdb3 commented at 8:04 pm on June 28, 2024:

nit: Is it comprehensive using the term “pool” here, or would it be better to keep things generic (and perhaps mention a pool as an example)? This is reserving space in the template for the coinbase and the entity requesting the template could be a pool or a solo miner (albeit less commonly), right? Maybe something like maximum additional serialized bytes the block requestor can add in coinbase transaction outputs (for example, to enable pool payouts).

nit: weight and bytes seem to be used interchangeably here and in the function definition. It might prevent confusion to stick to one nomenclature.

Sjors commented at 8:36 am on July 1, 2024:

I agree something like “block requestor” is more generic than “pool”, but also more confusing.

weight and bytes seem to be used interchangeably here and in the function definition

These bytes all go in the output which doesn’t have a witness discount, so each byte counts as 4 weight units. I think we should prefer the term “bytes”, but make sure it’s used correctly since we use weight units for internal accounting.

Sjors commented at 8:45 am on July 1, 2024:

Which has a few more hairy details:

The Template Provider MUST NOT provide NewMiningJob messages which would represent consensus-invalid blocks once this additional size — along with a maximally-sized (100 byte) coinbase field — is added. 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.

Sjors commented at 10:31 am on July 1, 2024:

I ended up renaming coinbase_output_max_additional_size to coinbase_max_additional_weight to account for these issues, see below.

tdb3 commented at 9:14 pm on June 28, 2024: contributor

Approach ACK This adds flexibility for SV2, and having less magic numbers is better. I support adding a configuration parameter for this (would prevent others from needing to fork).

Left a couple small nits for now, but plan to test/exercise in more detail.

TheCharlatan commented at 9:23 pm on June 30, 2024: contributor

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.

in src/node/interfaces.cpp:893 in 4c57f2707e outdated

889+                                                   size_t coinbase_output_max_additional_sigops) override
890     {
891         BlockAssembler::Options options;
892         ApplyArgsManOptions(gArgs, options);
893 
894+        Assume(coinbase_output_max_additional_size <= DEFAULT_BLOCK_MAX_WEIGHT);

Sjors commented at 8:37 am on July 1, 2024:

This probably needs to be DEFAULT_BLOCK_MAX_WEIGHT / 4, let me check the spec as well…

Sjors commented at 8:42 am on July 1, 2024: member

@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 force-pushed on Jul 1, 2024

Sjors commented at 10:30 am on July 1, 2024: member

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.

DrahtBot added the label Needs rebase on Jul 2, 2024

Sjors force-pushed on Jul 4, 2024

Sjors commented at 6:36 am on July 4, 2024: member

Rebased after #30324.

TheCharlatan commented at 6:57 am on July 4, 2024: contributor

Is there a reason why

0BlockAssembler::Options options;
1ApplyArgsManOptions(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.

DrahtBot removed the label Needs rebase on Jul 4, 2024

Sjors commented at 7:59 am on July 4, 2024: member

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

glozow commented at 8:17 am on July 4, 2024: member

Is there a reason why
0BlockAssembler::Options options;
1ApplyArgsManOptions(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 commented at 5:21 pm on July 6, 2024: member

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 commented at 1:27 pm on July 8, 2024: member

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:

The max block weight demanded by the user via -blockmaxweight
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.

lozanopo approved

in src/interfaces/mining.h:46 in d89637ecea outdated

42@@ -42,9 +43,13 @@ class Mining
43      *
44      * @param[in] script_pub_key the coinbase output
45      * @param[in] use_mempool set false to omit mempool transactions
46+     * @param[in] coinbase_max_addition_weight maximum additional weight which the pool will add to the coinbase transaction

itornaza commented at 6:25 pm on July 9, 2024:

I take it you wanted to rename it to coinbase_max_additional_weight instead

Sjors commented at 8:43 am on July 11, 2024:

Thanks.

in src/interfaces/mining.h:51 in d89637ecea outdated

47+     * @param[in] coinbase_output_max_additional_sigops maximum additional sigops which the pool will add in coinbase transaction outputs
48      * @returns a block template
49      */
50-    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
51+    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true,
52+                                                                 size_t coinbase_max_addition_weight = DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT,

itornaza commented at 6:26 pm on July 9, 2024:

Same here, did you meant to name it coinbase_max_additional_weight?

in src/node/interfaces.cpp:887 in d89637ecea outdated

882@@ -883,11 +883,24 @@ class MinerImpl : public Mining
883         return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root);
884     }
885 
886-    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool) override
887+    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool,
888+                                                   size_t coinbase_max_addition_weight,

itornaza commented at 6:28 pm on July 9, 2024:

Again, should it be coinbase_max_additional_weight??

in src/node/interfaces.cpp:894 in d89637ecea outdated

890     {
891         BlockAssembler::Options options;
892         ApplyArgsManOptions(gArgs, options);
893 
894+        Assume(coinbase_max_addition_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
895+        options.coinbase_max_additional_weight = coinbase_max_addition_weight;

itornaza commented at 6:28 pm on July 9, 2024:

coinbase_max_additional_weight?

itornaza approved

itornaza commented at 6:45 pm on July 9, 2024: none

Concept ACK d89637eceab0145a64c2d1af1139ace204a3ab3c

Giving the option to assign the values of coinbase_max_additional_weight and coinbase_output_max_additional_sigops needed by the Stratum V2 protocol is essential for its integration. I understand the risks involved by having two parameters for the same thing:

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

but it seems unavoidable at a first glance due to their different invocation domains. During the node initialisation for the former and connection time for the later.

Left some small nit that only applies if the intended name for the variable was coinbase_max_additional_weight instead of coinbase_max_addition_weight

Sjors force-pushed on Jul 11, 2024

DrahtBot added the label CI failed on Jul 12, 2024

DrahtBot removed the label CI failed on Jul 12, 2024

ryanofsky commented at 1:47 pm on July 15, 2024: contributor

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:

  0--- a/src/Makefile.am
  1+++ b/src/Makefile.am
  2@@ -313,7 +313,6 @@ BITCOIN_CORE_H = \
  3   util/hasher.h \
  4   util/insert.h \
  5   util/macros.h \
  6-  util/mining.h \
  7   util/moneystr.h \
  8   util/overflow.h \
  9   util/overloaded.h \
 10--- a/src/interfaces/mining.h
 11+++ b/src/interfaces/mining.h
 12@@ -5,10 +5,11 @@
 13 #ifndef BITCOIN_INTERFACES_MINING_H
 14 #define BITCOIN_INTERFACES_MINING_H
 15 
 16+#include <node/types.h>
 17+#include <uint256.h>
 18+
 19 #include <memory>
 20 #include <optional>
 21-#include <uint256.h>
 22-#include <util/mining.h>
 23 
 24 namespace node {
 25 struct CBlockTemplate;
 26@@ -42,14 +43,10 @@ public:
 27      * Construct a new block template
 28      *
 29      * [@param](/bitcoin-bitcoin/contributor/param/)[in] script_pub_key the coinbase output
 30-     * [@param](/bitcoin-bitcoin/contributor/param/)[in] use_mempool set false to omit mempool transactions
 31-     * [@param](/bitcoin-bitcoin/contributor/param/)[in] coinbase_max_additional_weight maximum additional weight which the pool will add to the coinbase transaction
 32-     * [@param](/bitcoin-bitcoin/contributor/param/)[in] coinbase_output_max_additional_sigops maximum additional sigops which the pool will add in coinbase transaction outputs
 33+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] options options for creating the block
 34      * [@returns](/bitcoin-bitcoin/contributor/returns/) a block template
 35      */
 36-    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true,
 37-                                                                 size_t coinbase_max_additional_weight = DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT,
 38-                                                                 size_t coinbase_output_max_additional_sigops = DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS) = 0;
 39+    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options={}) = 0;
 40 
 41     /**
 42      * Processes new block. A valid new block is automatically relayed to peers.
 43--- a/src/node/interfaces.cpp
 44+++ b/src/node/interfaces.cpp
 45@@ -883,25 +883,11 @@ public:
 46         return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root);
 47     }
 48 
 49-    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool,
 50-                                                   size_t coinbase_max_additional_weight,
 51-                                                   size_t coinbase_output_max_additional_sigops) override
 52+    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, const BlockCreateOptions& options) override
 53     {
 54-        BlockAssembler::Options options;
 55-        ApplyArgsManOptions(gArgs, options);
 56-
 57-        Assume(coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
 58-        options.coinbase_max_additional_weight = coinbase_max_additional_weight;
 59-
 60-        Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
 61-        options.coinbase_output_max_additional_sigops = coinbase_output_max_additional_sigops;
 62-
 63-        // The BlockAssembler constructor calls ClampOptions which clamps
 64-        // nBlockMaxWeight between coinbase_output_max_additional_size and
 65-        // DEFAULT_BLOCK_MAX_WEIGHT.
 66-        // In other words, coinbase (reserved) outputs can safely exceed
 67-        // -blockmaxweight, but the rest of the block template will be empty.
 68-        return BlockAssembler{chainman().ActiveChainstate(), use_mempool ? context()->mempool.get() : nullptr, options}.CreateNewBlock(script_pub_key);
 69+        BlockAssembler::Options assemble_options{options};
 70+        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
 71+        return BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(script_pub_key);
 72     }
 73 
 74     NodeContext* context() override { return &m_node; }
 75--- a/src/node/miner.cpp
 76+++ b/src/node/miner.cpp
 77@@ -59,14 +59,17 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 78 
 79 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 80 {
 81+    Assume(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
 82+    Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
 83     // Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
 84+    // Coinbase (reserved) outputs can safely exceed -blockmaxweight, but the rest of the block template will be empty.
 85     options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT);
 86     return options;
 87 }
 88 
 89 BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options)
 90     : chainparams{chainstate.m_chainman.GetParams()},
 91-      m_mempool{mempool},
 92+      m_mempool{options.use_mempool ? mempool : nullptr},
 93       m_chainstate{chainstate},
 94       m_options{ClampOptions(options)}
 95 {
 96--- a/src/node/miner.h
 97+++ b/src/node/miner.h
 98@@ -6,6 +6,7 @@
 99 #ifndef BITCOIN_NODE_MINER_H
100 #define BITCOIN_NODE_MINER_H
101 
102+#include <node/types.h>
103 #include <policy/policy.h>
104 #include <primitives/block.h>
105 #include <txmempool.h>
106@@ -20,8 +21,6 @@
107 #include <boost/multi_index/tag.hpp>
108 #include <boost/multi_index_container.hpp>
109 
110-#include <util/mining.h>
111-
112 class ArgsManager;
113 class CBlockIndex;
114 class CChainParams;
115@@ -155,24 +154,13 @@ private:
116     Chainstate& m_chainstate;
117 
118 public:
119-    struct Options {
120+    struct Options : BlockCreateOptions {
121         // Configuration parameters for the block size
122         size_t nBlockMaxWeight{DEFAULT_BLOCK_MAX_WEIGHT};
123         CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
124         // Whether to call TestBlockValidity() at the end of CreateNewBlock().
125         bool test_block_validity{true};
126         bool print_modified_fee{DEFAULT_PRINT_MODIFIED_FEE};
127-        /**
128-         * The maximum additional weight which the pool will add to the coinbase
129-         * scriptSig, witness and outputs. This must include any additional
130-         * weight needed for larger CompactSize encoded lengths.
131-         */
132-        size_t coinbase_max_additional_weight{DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT};
133-        /**
134-         * The maximum additional sigops which the pool will add in coinbase
135-         * transaction outputs.
136-         */
137-        size_t coinbase_output_max_additional_sigops{DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS};
138     };
139 
140     explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options);
141--- a/src/node/types.h
142+++ b/src/node/types.h
143@@ -13,6 +13,8 @@
144 #ifndef BITCOIN_NODE_TYPES_H
145 #define BITCOIN_NODE_TYPES_H
146 
147+#include <cstddef>
148+
149 namespace node {
150 enum class TransactionError {
151     OK, //!< No error
152@@ -24,6 +26,24 @@ enum class TransactionError {
153     MAX_BURN_EXCEEDED,
154     INVALID_PACKAGE,
155 };
156+
157+struct BlockCreateOptions {
158+    /**
159+     * Set false to omit mempool transactions
160+     */
161+    bool use_mempool{true};
162+    /**
163+     * The maximum additional weight which the pool will add to the coinbase
164+     * scriptSig, witness and outputs. This must include any additional
165+     * weight needed for larger CompactSize encoded lengths.
166+     */
167+    size_t coinbase_max_additional_weight{4000};
168+    /**
169+     * The maximum additional sigops which the pool will add in coinbase
170+     * transaction outputs.
171+     */
172+    size_t coinbase_output_max_additional_sigops{400};
173+};
174 } // namespace node
175 
176 #endif // BITCOIN_NODE_TYPES_H
177--- a/src/rpc/mining.cpp
178+++ b/src/rpc/mining.cpp
179@@ -371,7 +371,7 @@ static RPCHelpMan generateblock()
180 
181     ChainstateManager& chainman = EnsureChainman(node);
182     {
183-        std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, /*use_mempool=*/false)};
184+        std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, {.use_mempool = false})};
185         if (!blocktemplate) {
186             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
187         }
188deleted file mode 100644
189--- a/src/util/mining.h
190+++ /dev/null
191@@ -1,13 +0,0 @@
192-// Copyright (c) 2024 The Bitcoin Core developers
193-// Distributed under the MIT software license, see the accompanying
194-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
195-
196-#ifndef BITCOIN_UTIL_MINING_H
197-#define BITCOIN_UTIL_MINING_H
198-
199-/** Default for coinbase_max_additional_weight */
200-static constexpr unsigned int DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT{4000};
201-/** Default for coinbase_output_max_additional_sigops */
202-static constexpr unsigned int DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS{400};
203-
204-#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.

DrahtBot requested review from itornaza on Jul 15, 2024

DrahtBot requested review from tdb3 on Jul 15, 2024

in src/node/miner.h:173 in af3f85fb0f outdated

168+        size_t coinbase_max_additional_weight{4000};
169+        /**
170+         * The maximum additional sigops which the pool will add in coinbase
171+         * transaction outputs.
172+         */
173+        size_t coinbase_output_max_additional_sigops{400};

ismaelsadeeq commented at 2:02 pm on July 15, 2024:

I think it should not be necessary to reference pool in here

 0        /**
 1         * The maximum additional weight that can be added  to the coinbase
 2         * scriptSig, witness and outputs. This must include any additional
 3         * weight needed for larger CompactSize encoded lengths.
 4         */
 5        size_t coinbase_max_additional_weight{4000};
 6        /**
 7         * The maximum additional sigops that can be added in coinbase
 8         * transaction outputs.
 9         */
10        size_t coinbase_output_max_additional_sigops{400};

Sjors commented at 8:18 am on July 16, 2024:

See above #30356 (review), omitting the word “pool” because in theory there could be something other than a pool that wants to add data to our coinbase, just seems confusing to me.

in src/node/miner.h:167 in af3f85fb0f outdated

159@@ -160,6 +160,17 @@ class BlockAssembler
160         // Whether to call TestBlockValidity() at the end of CreateNewBlock().
161         bool test_block_validity{true};
162         bool print_modified_fee{DEFAULT_PRINT_MODIFIED_FEE};
163+        /**
164+         * The maximum additional weight which the pool will add to the coinbase
165+         * scriptSig, witness and outputs. This must include any additional
166+         * weight needed for larger CompactSize encoded lengths.
167+         */

ismaelsadeeq commented at 2:39 pm on July 15, 2024:

Indicate the assumed upper limits of this options?

Sjors commented at 8:21 am on July 16, 2024:

#29432 implements this in more detail. I’m not even 100% sure if it’s correct there, so I’d rather wait with documenting the exact numbers - that might just encourage someone to recompile and burn themselves.

in src/node/interfaces.cpp:896 in e67e4669a9 outdated

892         ApplyArgsManOptions(gArgs, options);
893 
894+        Assume(coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
895+        options.coinbase_max_additional_weight = coinbase_max_additional_weight;
896+
897+        Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);

ismaelsadeeq commented at 2:44 pm on July 15, 2024:

Why are we allowing for this large values? shouldnt this be

0         Assume(coinbase_max_additional_weight <= MAX_STANDARD_TX_WEIGHT);
1    options.coinbase_max_additional_weight = coinbase_max_additional_weight;
2     Assume(options.coinbase_output_max_additional_sigops <= MAX_STANDARD_TX_SIGOPS_COST);

Sjors commented at 8:24 am on July 16, 2024:

MAX_STANDARD_TX_WEIGHT is a standardness rule. If we wanted to enforce it in the mining code, which would be unrelated to this PR, it shouldn’t be through an assert / assume.

Sjors commented at 3:46 pm on July 15, 2024: member

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

refactor: use CHECK_NONFATAL to avoid single-use symbol 323cfed595

refactor: pass BlockCreateOptions to createNewBlock

Rather than pass options individually to createNewBlock and then
combining them into BlockAssembler::Options, this commit introduces
BlockCreateOptions and passes that instead.

Currently there's only one option (use_mempool) but the next
commit adds more.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

6b4c817d4b

Sjors force-pushed on Jul 16, 2024

Sjors commented at 8:29 am on July 16, 2024: member

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

in src/node/miner.cpp:63 in 95c2edefe3 outdated

58@@ -59,8 +59,11 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
59 
60 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
61 {
62-    // Limit weight to between 4K and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
63-    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, 4000, DEFAULT_BLOCK_MAX_WEIGHT);
64+    Assume(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
65+    Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);

ryanofsky commented at 12:11 pm on July 16, 2024:

In commit “Pass coinbase constraints to createNewBlock” (e67e4669a9e263d90f0e276c8e65e93c39170375)

Current code seems ok, but depending on how important it is for this not to generate an invalid block, it may make sense to use assert instead of Assume, since Assume does not do anything in release builds. Or this could also abort in debug builds but cap the values in release builds:

0if (!Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST)) {
1    options.coinbase_output_max_additional_sigops = MAX_BLOCK_SIGOPS_COST
2}

Sjors commented at 6:19 pm on July 16, 2024:

Currently neither of these values can be configured. With Stratum v2 I intend to add runtime checks. If those checks are wrong then I assume the fuzzer would eventually find this Assume?

ryanofsky commented at 4:02 pm on July 17, 2024:

re: #30356 (review)

So you are saying if the values are too big at this point, it means there is a bug elsewhere in the code. If that is the case, I think it would be more appropriate to use assert or Assert rather than Assume so the bug does not go undetected until it is too late and eventually results in an invalid block being produced. I could be wrong, but I don’t see a rationale for preferring assume over assert here.

Sjors commented at 4:33 pm on July 17, 2024:

So you are saying if the values are too big at this point, it means there is a bug elsewhere in the code

Yes

Changed to Assert.

itornaza commented at 4:35 pm on July 17, 2024:

This is great! Despite that these values are hard coded, I also think it is safer to prefer an assert over Assume in this case. In that way we still have a safeguard in place during the whole integration process of Stratum V2 and even if the future run time checks somehow fail to deliver their intended purpose at least we have some feedback in debug mode. On the other hand the assert wont hurt anyone exactly because the values are hard coded.

ryanofsky commented at 5:02 pm on July 17, 2024:

re: #30356 (review)

Just to clarify for other reviewers since the names of these macros are confusing: In bitcoin core, assert and Assert macros behave very differently from the assert macro in standard c++. In standard c++ assert statements are not executed at all in release builds, while in bitcoin core they are always executed and always cause the execution to abort if they fail. Other c++ libraries use the term check instead of assert for this behavior.

The Assume macro is more unusual, because it doesn’t abort execution in release builds, but does also doesn’t get skipped in release builds, so will always evaluate arguments passed to it. It can make sense to use Assume instead of assert when an unexpected condition indicates some kind of bug, but the bug is easy to recover from, so it is worth crashing debug builds but not release builds. Normally it make sense to check the Assume return value and do something like log a warning or fix invalid values, not just ignore the unexpected condition completely.

ryanofsky approved

ryanofsky commented at 12:13 pm on July 16, 2024: contributor

Code review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339

ryanofsky referenced this in commit 4e1a4342f3 on Jul 16, 2024

tdb3 approved

tdb3 commented at 1:18 am on July 17, 2024: contributor

Code Review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339 Good addition and the updates tidy things up nicely.

I would also support a bitcoind config parameter or possibly an RPC for this.

ryanofsky referenced this in commit e1ae38f4bb on Jul 17, 2024

ryanofsky referenced this in commit e1fa475a5b on Jul 17, 2024

ryanofsky approved

ryanofsky commented at 4:05 pm on July 17, 2024: contributor

Code review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339

ryanofsky referenced this in commit 5126ac430f on Jul 17, 2024

ryanofsky referenced this in commit bdd68d5bca on Jul 17, 2024

refactor: add coinbase constraints to BlockCreateOptions

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.
https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426

The existince of such patches suggests it may be useful to
make this value configurable. This commit would make such a
change easier.

The main motivation however is that the Stratum v2 spec
requires the pool to communicate the maximum bytes they intend
to add to the coinbase outputs. A proposed change to the spec
would also require them to communicate the maximum number of sigops.

This commit also documents what happens when
-blockmaxweight is lower than the coinbase
reserved value.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

c504b6997b

Sjors force-pushed on Jul 17, 2024

itornaza approved

itornaza commented at 4:50 pm on July 17, 2024: none

tested ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c

Reviewed all changes since my last #30356#pullrequestreview-2167070268. Built the commit and run all unit, functional and extended tests and all of them pass.

DrahtBot requested review from ryanofsky on Jul 17, 2024

DrahtBot requested review from tdb3 on Jul 17, 2024

Sjors referenced this in commit 173580b123 on Jul 17, 2024

ryanofsky approved

ryanofsky commented at 5:03 pm on July 17, 2024: contributor

Code review ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c

in src/node/types.h:35 in 6b4c817d4b outdated

30+struct BlockCreateOptions {
31+    /**
32+     * Set false to omit mempool transactions
33+     */
34+    bool use_mempool{true};
35+};

ismaelsadeeq commented at 11:56 am on July 18, 2024:

Is their a reason we are not adding coinbase_script into the BlockCreateOptions struct?

Sjors commented at 1:28 pm on July 18, 2024:

No strong reason, but the coinbase script is different for every block, unlike the other stuff we pass to BlockCreateOptions. It seems worthy of a positional argument.

ismaelsadeeq approved

ismaelsadeeq commented at 12:05 pm on July 18, 2024: member

Code review ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c

The PR description needs to be updated to match the PR.

Left a single question

Sjors referenced this in commit 7967d78d73 on Jul 18, 2024

Sjors referenced this in commit 79336c95fa on Jul 18, 2024

Sjors commented at 1:21 pm on July 18, 2024: member

@ismaelsadeeq thanks, updated the description.

ryanofsky merged this on Jul 18, 2024

ryanofsky closed this on Jul 18, 2024

glozow commented at 4:16 pm on July 18, 2024: member

post merge ACK

Sjors deleted the branch on Jul 18, 2024

refactor: add coinbase constraints to BlockAssembler::Options #30356

Code Coverage

Reviews

Conflicts