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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31384.

Reviews

See the guideline for information on the review process.

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:

• #31583 (rpc: add gettarget , target getmininginfo field and show next block info 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.

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/33621957087}

I'm in favor of fixing this bug, but differently, see #21950 (comment)

Something like this should do:

diff --git a/src/init.cpp b/src/init.cpp
index f79ebe881d..881a254efc 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -54,6 +54,7 @@
 #include <node/mempool_persist_args.h>
 #include <node/miner.h>
 #include <node/peerman_args.h>
+#include <node/types.h>           // for BlockCreateOptions
 #include <policy/feerate.h>
 #include <policy/fees.h>
 #include <policy/fees_args.h>
@@ -645,7 +646,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     argsman.AddArg("-whitelistrelay", strprintf("Add 'relay' permission to whitelisted peers with default permissions. This will accept relayed transactions even when not relaying transactions (default: %d)", DEFAULT_WHITELISTRELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
 
 
-    argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
+    argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d). Note that getblocktemplate reserves %d weight units for the pool to customize the coinbase.", DEFAULT_BLOCK_MAX_WEIGHT, node::BlockCreateOptions{}.coinbase_max_additional_weight), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
     argsman.AddArg("-blockmintxfee=<amt>", strprintf("Set lowest fee rate (in %s/kvB) for transactions to be included in block creation. (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_BLOCK_MIN_TX_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
     argsman.AddArg("-blockversion=<n>", "Override block version to test forking scenarios", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::BLOCK_CREATION);
 
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 4412f2db87..f0c9e822ec 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -20,7 +20,7 @@ class CFeeRate;
 class CScript;
 
 /** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/
-static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
+static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT};
 /** Default for -blockmintxfee, which sets the minimum feerate for a transaction in blocks created by mining code **/
 static constexpr unsigned int DEFAULT_BLOCK_MIN_TX_FEE{1000};
 /** The maximum weight for transactions we're willing to relay/mine */

While you're touching this, maybe init.cpp should also enforce that -blockmaxweight <= MAX_BLOCK_WEIGHT.

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/33738108686}

Thanks for your review, @Sjors.

I addressed all of your comments.

Something like this should do

Done. Instead of creating DEFAULT_BLOCK_MAX_WEIGHT that copies MAX_BLOCK_WEIGHT, I've used it directly.

Maybe init.cpp should also enforce that -blockmaxweight <= MAX_BLOCK_WEIGHT

We already clamp the block size in the block assembler, so it will never exceed MAX_BLOCK_WEIGHT, but done with a test.

I've added a functional test and release notes.

We already clamp the block size in the block assembler

True, but I think it's always better to clearly inform the user when they're trying to do something impossible. That said, not everyone might agree with that. If other reviewers don't like it you can always drop the check.

Will review shortly.

ACK f7a2f12 (see comment below)

I checked that the new test_block_max_weight test fails if I undo the changes in e1114bc.

Thanks for reviewing @pinheadmz @luke-jr @Sjors

I've recently foreced pushed from 2fb833e to 7ab1344 diff

Changes were:

1. Reverting 76a9552 but improving the help text.
2. Use policy DEFAULT_MAX_BLOCK_WEIGHT instead of consensus MAX_BLOCK_WEIGHT.
3. Rename -maxcoinbaseweight to -coinbasereservedweight and improve it's help text.
4. Update the release notes to be more informative.

I think https://github.com/bitcoin/bitcoin/pull/31384/files#r1908533084 should be addressed before merging (and so might as well address the other comments now instead of a followup).

I believe this is addressed now.

If you're going to touch the getblocktemplate help text, then consider cherry-picking 4facb94.

I am not touching getblocktemplate help text see #31384 (comment)

re-ACK dba211a

Concept ACK

@0xB10C pointed out the header always uses 80 bytes and there's few other unavoidable bytes in the block. So it sounds like there should be a minimum value for -coinbasereservedweight.

Additionally perhaps it should be renamed to just -reservedweight with a clarification in the document comment of how much of that goes to stuff other than the coinbase.

Alternatively we could make -coinbasereservedweight refer only to the coinbase, but internally account for these other bytes. That's probably less confusing. We'll end up with a default a bit under 8000 though for simplicity we could keep the round number.

Still a third option is to reduce DEFAULT_MAX_BLOCK_WEIGHT (and the max permitted -maxblockweight) to account for the overhead.

https://delvingbitcoin.org/t/analyzing-mining-pool-behavior-to-address-bitcoin-cores-double-coinbase-reservation-issue/1351/5?u=sjors

From #21950 (comment)

Note that the reserved weight is NOT only used for the coinbase transaction. The block header takes up 320 WU (80 bytes * 4) and the var_int for the number of transactions takes up either 4 WU (1 byte var_int for up to 252 txns in the block) or 12 WU (3 byte var_int for up to 65535 txns). For most mainnet blocks this is an additional 332 WU. See also https://delvingbitcoin.org/t/analyzing-mining-pool-behavior-to-address-bitcoin-cores-double-coinbase-reservation-issue/1351/4?u=0xb10c

Given that this weight is reserved for the coinbase and the header, I don't think we should call it "coinbase reserved weight" or introduce something called -coinbasereservedweight. Maybe do -blockreservedweight and document that it's for the header and coinbase?

edit: @Sjors was faster

Alternatively we could make -coinbasereservedweight refer only to the coinbase, but internally account for these other bytes. That's probably less confusing. We'll end up with a default a bit under 8000 though for simplicity we could keep the round number.

This change will alter the behavior, resulting in a transactions weight of 3992000 - 332 = 3991668 after calling GBT.

I think I prefer the first idea, which is similar to what @0xB10C suggested: renaming coinbasereservedweight to -blockreservedweight and setting a minimum value for it.

The minimum value would be:

• The block header's weight (332 WU)
• Plus the average coinbase transaction weight (1 input, 2 outputs, 562 WU)
• Totalling 894 WU (rounded up to 1000 WU).

In the help text, we can clearly indicate this information.

If we're going to set a minimum value for -blockreservedweight we should probably just pick the absolute minimum:

• a block with only a coinbase transaction
• header weight + 1 (for the number of transactions)
• the coinbase has 1 input (required by IsCoinBase())
  • just the BIP34 scriptSig
  • no witness commitment
• the coinbase has 1 output (required by CheckTransaction)
  • OP_TRUE output script (or empty?)

And maybe add warning, in the help or at startup, that any value under 1000 should not be attempted without a thorough understanding of block serialization.

(alternatively we could be pedantic and just not allow anything under 2000 - and then have a code comment above this constant to remind anyone about to recompile of the things they should consider)

Thanks for your review @Sjors and recent insight from @0xB10C
I've force pushed from dba211a to ad1bc03 see diff

Changes

1. Renamed -coinbasereservedweight to -blockreservedweight
2. Clarified that the reservation is also for block header data
3. Prevent startup when user provided a -blockreservedweight lower than 2000 WU
4. Update the release notes

The first commit a7d4364 changes coinbaseMaxAdditionalWeight in the Mining interface to blockReservedWeight. This decouples it from the Stratum v2 concept of CoinbaseOutputDataSize which is strictly limited to the outputs added:

The maximum additional serialized bytes which the pool will add in coinbase transaction outputs

https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---server

So I was initially a bit reluctant, but I think it's fine. The Template Provider (sidecar) in Sjors#48 will just have to convert CoinbaseOutputDataSize to blockReservedWeight.

I'll probably just have it add 2000 weight units, so it can never go below the minimum. It was already doing some math: Sjors@f481d65#diff-576b7d75ad2ccce85935852fbc155515ebc3afdfd8fe7b806f6b56c29053b786R124

Some background...

The Stratum v2 spec gives the following explanation of CoinbaseOutputDataSize:

Ultimately, the pool is responsible for adding coinbase transaction outputs for payouts and other uses, and thus the Template Provider will need to consider this additional block size when selecting transactions for inclusion in a block (to not create an invalid, oversized block). Thus, this message is used to indicate that some additional space in the block/coinbase transaction be reserved for the pool’s use (while always assuming the pool will use the entirety of available coinbase space).

The Job Declarator MUST discover the maximum serialized size of the additional outputs which will be added by the pool(s) it intends to use this work. It then MUST communicate the maximum such size to the Template Provider via this message. 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.

By "coinbase field" they refer to coinbase scriptSig, which has the extra nonce, pool name, etc.`

The "Job Declarator" is another piece of software that sits between the pool and the Template Provider.

The way I interpret the spec is that it puts the onus of accounting for nitty gritty encoding stuff on the Template Provider.

Prevent startup when user provided a -blockreservedweight lower than 2000 WU

Maybe @wangchun from F2Pool can confirm if this would work for F2Pool and how many weigh units they currently reserve. And if they do it with a custom patch or -maxblockweight.

Prevent startup when user provided a -blockreservedweight lower than 2000 WU

Maybe @wangchun from F2Pool can confirm if this would work for F2Pool and how many weigh units they currently reserve. And if they do it with a custom patch or -maxblockweight.

We (F2Pool) reserved 2000 WU for the coinbase transaction. This was achieved by patching the source code, ensuring that the check in src/init.cpp does not affect us.
In fact, we do not face the "duplicate reservation" issue because the same modification has already been applied in src/policy/policy.h.

@wincss thanks for the clarification. You should be able to use -blockreservedweight=2000 after this change (-maxblockweight=4000000 will be the default). Is there anything else you need a patch for? Fee free to open a Github issue.

Forced pushed from ad1bc03 to fd31031 see diff

Changes

1. Addressed @Sjors comments

ACK fd31031