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 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.

refactor: use CHECK_NONFATAL to avoid single-use symbol 64fef353c9

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
Approach 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:

#30324 (optimization: Moved repeated -printpriority fetching out of AddToBlock by paplorinc)

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.

refactor: add coinbase constraints to BlockAssembler::Options

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 adds both to BlockAssembler::Options. The default
is the same as the previously hardcode values.

fc77f3b522

Add util/mining.h for default coinbase constraints

These defaults are needed by the Mining interface in the next commit.

a404aef4bc

Pass coinbase constraints to createNewBlock

Existing callers of createNewBlock use the defaults,
so this commit does not change behavior and the
Assume(s) won't be false.

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

3d459210aa

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 commented at 9:49 pm on July 2, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jul 2, 2024

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

Code Coverage

Reviews

Conflicts