mining: bugfix: Fix duplicate coinbase tx weight reservation #31384

• This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
• Fixes #21950

We reserve 4000 weight units for coinbase transaction in DEFAULT_BLOCK_MAX_WEIGHT

https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3bbac641f05d490fd5c984156b33/src/policy/policy.h#L23

And also reserve additional 4000 weight units in the default BlockCreationOptions struct.

https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3bbac641f05d490fd5c984156b33/src/node/types.h#L36-L40

Motivation

• This issue was first noticed during a review here #11100 (review))
• It was later reported in issue #21950.
• I also came across the bug while writing a test for building the block template. I could not create a block template above 3,992,000 in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.

This PR fixes this by consolidating the reservation to be in a single location in the codebase.

This PR then adds a new startup option -blockreservedweight whose default is 8000 that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.

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:

• #31283 (Add waitNext() to BlockTemplate interface by Sjors)
• #28676 ([WIP] Cluster mempool implementation by sdaftuar)

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

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Something like this should do:

 0diff --git a/src/init.cpp b/src/init.cpp
 1index f79ebe881d..881a254efc 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -54,6 +54,7 @@
 5 #include <node/mempool_persist_args.h>
 6 #include <node/miner.h>
 7 #include <node/peerman_args.h>
 8+#include <node/types.h>           // for BlockCreateOptions
 9 #include <policy/feerate.h>
10 #include <policy/fees.h>
11 #include <policy/fees_args.h>
12@@ -645,7 +646,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
13     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);
14 
15 
16-    argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
17+    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);
18     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);
19     argsman.AddArg("-blockversion=<n>", "Override block version to test forking scenarios", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::BLOCK_CREATION);
20 
21diff --git a/src/policy/policy.h b/src/policy/policy.h
22index 4412f2db87..f0c9e822ec 100644
23--- a/src/policy/policy.h
24+++ b/src/policy/policy.h
25@@ -20,7 +20,7 @@ class CFeeRate;
26 class CScript;
27 
28 /** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/
29-static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
30+static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT};
31 /** Default for -blockmintxfee, which sets the minimum feerate for a transaction in blocks created by mining code **/
32 static constexpr unsigned int DEFAULT_BLOCK_MIN_TX_FEE{1000};
33 /** 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

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

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 f7a2f125c7b23c4c3c2e5253a6d3e2dcd8d3fe97 (see comment below)

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

Could you talk to why it is not a concern for existing deployed pool software which relies on the 8k value? It seems that reducing the default to 4k could at best cause issues to upgrading to a new Bitcoin Core version if noticed or at worst lead to creating an invalid block.

From a quick skim through a block explorer there is a bunch of blocks with a >4k coinbase transaction right now:

• https://blockstream.info/tx/121436b7346a49e12495a9286638636bf4baed0a218ce47867cad510c78030b1
• https://blockstream.info/tx/9017f8200301770d2bac3fc224302ab207ccec2ef42e00eae78a30928fdb40a7
• https://blockstream.info/tx/134a47f4853382ec2b066c580a2b98867e90fe264889b71e562c94fb83991350
• https://blockstream.info/tx/747a8b230bd07e738ad4d5392190121e91da3a8125d1a33ccac4ec21f37cddc0
• https://blockstream.info/tx/9d2203b1c72eddc072b566c4a16ed8757fcba95a3be6f270e17a128e41554b33
• https://blockstream.info/tx/0987ebb31c4d8abd8158d5c7d63af8aea76df8581289ba0d676d7d38923ecd4f

It seems those were all created by a single pool, so it might make sense to look into their software if it’s open source, or reach out to them. Also probably a good idea to scan the past year of blocks to see if any other miner was creating coinbase between 4k and 8k.

To be clear i think it makes absolute sense to not double count or at least to clarify the actual max block weight Bitcoin Core will use by default. Just want to make sure that lowering the default doesn’t lead our users to make an expensive mistake.

ACK f7a2f125c7b23c4c3c2e5253a6d3e2dcd8d3fe97

Built and tested on arm/macos. Confirmed test fails without patch. I noticed last year that all but one pools’ blocks were maxing out at 3996000 but I really appreciate @darosior point about pools that typically build large coinbases.

I wonder if it makes sense for a pool to set their own reserved coinbase weight, or pass in the outputs they want into getblocktemplate as an argument?

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK f7a2f125c7b23c4c3c2e5253a6d3e2dcd8d3fe97
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmdXM54ACgkQ5+KYS2KJ
 7yTpiAxAAwSCb+I2c2yJmDmlmcZcq8tyNAiHuNxr7r6RWCcIx35TDiELPJaYNBkhj
 8S4OuTeRV+2CR0VbzCudmkt2MAHPaAlxCv3kgsMe4giI9bKSaHpBlCL5c0Zw3G2f4
 9Y0GTuCHYXMuK/G6pfMr/ox82AXXjorgVMpxAqAnlAigaebDb0CGwdSWw0by2kYNu
101DH8ivkU2c590cRO5OYZr/9t8HdGLp7ySvnQ4Db/c4lJNC5EmopO0sq31wAcSKpP
11ZYlv+eE6tKG8XYABNf78PNXnE4k2gpf3T6Gid+3lTXGAHSdopLJODmlK3UnVbMeH
129d9UaYZ0La5eeGOdRe66iKOyByNI+gOHXE7N0bdmM3xM1OR7LG3CaGxVOx27WSwg
13QR/mGG43AbGdOGLbUY9MY6qjQWLeekYYTEQEWiBb22oltvn0vmoWBTZ07rKnkiBL
14rbmZzAFHVkyjEZD6AkOxJ7EzeSQb0cAqMFfCC9z8tRJnVLwyvnLU8UUS2xq+NvN/
15npKdyC6OLK8F0Rg77oS6oWsJRzcvg5+lzDw54TXyokRYSLEsQCWBsAR2Fjl91C6a
16Mf/4SN/d4oHdf1o1MxG+HqNiH+B8kgF5zSRxqmq2H1vPtQzf9Gpam+ol5uWFnXYl
17x26/0aESedAhHqoZrpdwqdCRNn5CZoSOnL0f6aiqQVWFzNnwdeE=
18=/h/d
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

I ran this branch in Warnet too, filled up a mempool ("size": 4563, "bytes": 1377134) and called GBT from two connected nodes, v27.0 and this branch with expected results:

0$ # branch
1$ warnet bitcoin rpc tank-0000 getblocktemplate '{\"rules\":[\"segwit\"]}' | jq '[.transactions[].weight] | add '
23995896
3$ # v27.0
4$ warnet bitcoin rpc tank-0001 getblocktemplate '{\"rules\":[\"segwit\"]}' | jq '[.transactions[].weight] | add '
53991958

Both block templates had the exact same transactions, with the exception of the last 2

I’m interested in replaying this test with 6k, 8k, 10k weight coinbase txs too.

Could you talk to why it is not a concern for existing deployed pool software which relies on the 8k value?

If a pool does not manually set a value for -maxblockweight then, before this change, 4000 weight units were reserved for the coinbase and 4000 were unallocated. That effectively makes it a 8000 weight unit reservation.

So if any existing pool relies on that space to be available they could produce an invalid block after upgrading.

From a quick skim through a block explorer there is a bunch of blocks with a >4k coinbase transaction right now:

To be more precise, we’re interested in coinbase transactions of 4000 < size <= 8000, only counting scriptSig, witness and outputs. But the above is enough evidence to be careful here, so I undid my ACK.

It seems those were all created by a single pool, so it might make sense to look into their software if it’s open source

It being open source is highly unlikely unfortunately.

I see two options:

1. Make it clear in the README that miners should check if their pool software produces large coinbase transactions. This relies a bit too much on people actually reading things.

2. Add a new configuration option -maxcoinbaseweight (leaving out “additional” for readability) and make it 8000 by default. Combined with the changes in this PR, it preserves the current behavior.

If we go for option (2) we should probably go ahead and also add -maxcoinbasesigops since we know at least one pool patched Bitcoin Core (incorrectly) to have this functionality. See https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426

Given the observations above I don’t know if we can safely recommend 4000. I’m a bit worried about miners yolo-adjusting this value too low.

Thanks for all your reviews.

Thanks @pinheadmz for testing this with warnet 🚀

It seems that reducing the default to 4k could at best cause issues with upgrading to a new Bitcoin Core version if noticed or at worst lead to creating an invalid block.

This is only the case if the miner starts their node with default configurations. Assuming this PR gets into the next release, miners who want more space for the coinbase can simply start their node with a custom -blockmaxweight option, reducing the weight to their preferred value. @darosior I have a test covering this case in b52aad748efd84dceff84f9d46604375a6e23d1f

This pool should start with -blockmaxweight=3996000, which would provide an additional 4,000 weight units, and they are fine. However, as @Sjors mentioned, “This relies a bit too much on people actually reading things,” and I completely agree.

I also agree it would be very useful to analyze the percentage of coinbase weight usage over the last few years so that we can make an informed decision based on historical data. I will work on this.

As it stands, most miners are likely to have coinbases with weights not exceeding 4,000 (note: this claim should be backed with data; I only did a quick review of a few blocks) and current master provides no way for miners to build a block template that fully utilizes the remaining 4,000 weight units of weight. And we see some pools using a custom block builder to try to max out the remaining space according to @pinheadmz observations.

We should consider providing a mechanism that allows users to adjust this parameter while preserving the current block behavior.

• One option is to introduce a -maxcoinbaseweight parameter as @Sjors suggested.
• Alternatively, if we want to avoid adding a new option, we could reserve 8,000 weight units by default. When users specify a -blockmaxweight lower than 4,000,000, the delta would determine the default coinbase transaction weight.
  • Users who want 4,000 weight units for the coinbase can set -blockmaxweight=3996000.
  • Users who prefer the default 8,000 weight units will just start their node with the default configuration.

I’m a bit worried about miners yolo-adjusting this value too low.

(we can prevent too low delta in all approaches, so I think the minimum should be 4000 weight units. This PR already does that)

I am open to implementing any of these suggestions in this PR.

Alternatively, if we want to avoid adding a new option, we could reserve 8,000 weight units by default.

Maybe that’s the best way to keep this PR simple. For Stratum v2 I don’t care because we override the value anyway.

When users specify a -blockmaxweight lower than 4,000,000, the delta would determine the default coinbase transaction weight.

I think we should strictly honor -blockmaxweight though.

And then add -maxcoinbaseweight later to allow actually lowering this.

Though we should probably do that in the same release as this PR, because otherwise those who currently use -blockmaxweight=4000000 would be annoyed.

@darosior @Sjors @pinheadmz

I’ve recently analyzed the blockchain for the last two years from 4th December 2022 to 23rd December 2024 to move this forward.

I will cross-write my findings here, but more seen here details can be seen here https://delvingbitcoin.org/t/analyzing-mining-pool-behavior-to-address-bitcoin-cores-double-coinbase-reservation-issue/1351

• Most mining pools adhere to Bitcoin Core’s default setup, keeping coinbase transaction weights well below 4000 WU. However, two pools stand out:
  • Ocean.xyz consistently uses larger coinbase transactions, with an average weight of 6,994 WU. Their smallest coinbase weight was 5,308 WU (in block 865820), and their largest reached 9,272 WU (block 863471). This suggests Ocean.xyz configures its node with a reduced -blockmaxweight setting, as their average block size is 3,986,489 WU, well below the default template size.
  • An Unknown Pool/miner also exceeded the default coinbase weight, averaging 7,432 WU, with a maximum of 7,864 WU but a very low block weight average which is unusual behavior.
• F2Pool blocks have an average weight of 3,997,857 WU, with some blocks exceeding 3,995,063 WU. For instance, block 779729 reached 3,998,554 WU with a coinbase weight of just 1,588 WU, showcasing their optimal use of the available block weight.
• The majority of pools—such as Foundry USA, Binance Pool, and AntPool—produce blocks averaging around 3,993,000 WU, consistent with Bitcoin Core’s current behavior. However, this depicts underutilization which is a direct result of the double-reservation bug.
• The small increase in the weights of the blocks among these pools suggests minor additions of transactions by miners after building a block template.
• The small increase in the weights of the blocks after selecting the transactions is accounted for by the block headers, as noted by @0xB10C here

If the double-reservation issue is fixed, block templates will increase to 3,996,000 WU, aligning theoretical and actual limits. This change benefits pools by allowing full utilization of the block weight while preserving 4000 WU for coinbase transactions and other small additions.

Miners who need more space, such as Ocean.xyz, can still configure their nodes with a reduced -blockmaxweight. For example, setting it to 3,999,000 WU frees up 1000 WU for larger coinbase transactions, ensuring compliance with consensus rules.

@ismaelsadeeq thanks for the research!

Ocean in the DATUM documentation indeed requires a reduced -blockmaxweight=3985000, so presumably they do something similar in their regular stratum pool: https://github.com/OCEAN-xyz/datum_gateway?tab=readme-ov-file#node-configuration

I still think we should use -blockmaxweight (default 4000000) to refer to the entire block, including coinbase, and then have a separate -maxcoinbaseweight (default 8000). This keeps the same behavior for miners with default settings.

Miners who manually configured -blockmaxweight before will, after this update, need to manually set -maxcoinbaseweight=4000 to preserve the same behaviour. But if they don’t do this, it’s not dangerous.

Later on we could even deprecate -blockmaxweight. Its original purpose was to constrain the block size far below the consensus limit, which nobody is doing anymore.

@Sjors

I still think we should use -blockmaxweight (default 4000000)

Agreed this reflects the current behaviour of this PR

and then have a separate -maxcoinbaseweight (default 8000). This keeps the same behavior for miners with default settings.

Miners who manually configured -blockmaxweight before will, after this update, need to manually set -maxcoinbaseweight=4000 to preserve the same behaviour. But if they don’t do this, it’s not dangerous.

Why do we want to preserve the same behavior? Yes, they now have the ability to lower it, but then our default is high above the average of what miners use (almost 4x), and users tend to stick to the default. As a result, they will mine at a disadvantage compared to their counter parties have a custom settings.

Could you clarify the effect of setting it to 4000 in light of the recent findings? I see no reason why we should not set it to 4000 because it’s more than enough for most users, and does not incentivize any custom set-up.

Could you clarify the effect of setting it to 4000 in light of the recent findings?

It’s still not worth the risk imo. Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don’t read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees) because they were doing something custom that you didn’t detect in the analysis.

In the example of Ocean it seems that their custom -blockmaxweight is low enough that this change won’t bite them, but without inspecting their software we can’t know if there’s some edge case where it does matter.

Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don’t read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees)

Fair enough.

I’ve pushed an update diff:

1. Set the default to 8000, based on this suggestion.
2. e504d15cfb4c0b6ed67c46f5aecc47980f233bcb Fixed the accounting of total block weight and total block sigop count to exclude the reserved values for coinbase transaction. These are estimates and can be incorrect when the actual values, which are usually lower, don’t match the reserved ones. It’s now explicit that the totals do not include the coinbase transaction.

I didn’t push the update for maxcoinbaseweight because I wanted to suggest making it a runtime option. The requirements can change for each block template, so it might be better to pass it as an RPC parameter in getblocktemplate instead of an init parameter. WDYT, @Sjors?

Also, you can skip reviewing the release notes they’re not final yet.

Changing anything about getblocktemplate can be tricky in my experience #27433 (comment), because it’s defined in BIP22: https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki

So I would leave “making it a runtime option” to a followup. That said, I’m skeptical how useful that would be. Legacy pool software probably won’t use it, Ocean has a fixed value that they tell users to configure and modern mining pool software should just use the new Mining IPC interface imo.

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35214287972

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Thanks for your feedback @Sjors and your review @TheCharlatan

I’ve forced pushed to see diff:

1. Add the -maxcoinbaseweight option with default as 8000.
2. Address review comments from @TheCharlatan

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35274044590

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

ACK 2fb833e69627a8f9b1c4b14b60d1df9903adf210

Reviewed each commit, built and ran unit and functional tests on arm/macos. I left a few questions and nits below.

Test coverage is great and I think this is the right approach to add a coinbase reserved weight option. Also it’s a nice way to get those extra 4000 weight for default miners without putting the heavy-coinbase mining pools at risk

I also have one last lingering question which is: when does the miner actually construct their coinbase transaction and is that ever compared to the reserved size? Or do we just ensure it fits in the current block template against MAX_BLOCK_WEIGHT? In either case, how is the miner warned that they may need to reduce their coinbase weight OR restart with a different -maxcoinbaseweight?

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 2fb833e69627a8f9b1c4b14b60d1df9903adf210
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmd+3sMACgkQ5+KYS2KJ
 7yTroqhAAg1FUDuanndz7w+oHkrzpJ1JvmAGoZnaoBU6Rua7FgN97+RgHCv44zPH5
 8w5knXBMPl77AiM6BSCvfqt/os15Opm+GvU1wkz3oW4w/JvzYuiQK3X3PqQv4eeW2
 9E46p5dAoClwkzpkqdTKo4AezqiM3tHo+pGuvPLcaUgOFggElt/lYA2yKSvbEocst
10kWyw3fW6iXa71u+pONRukWHzRYthBiozrBQu8vPsj7o1xqnUqN+o650kzqT5qHXU
11pD9LPJlVUoeAKXg2+uUAlXRa0NI23H+QiRS5z3AKC8Y1frLA9oTFo3RvxcbyP2ux
12qIr57H30yEX+DhwG3u4B7TF2O3xcP5+KuKPER7CJ/EYXEYhxzaXW6jOkIbAwEH8b
133mVvN9fPxVHozMeeFHTJl+jQN6Jm6m0SMF85HSkxnvfS1xTRl7/BkQFmy8D2ofVQ
14IBeyIaibQCM53uc0MTI5Fr/FdiL4r63GzJopG4fPYmU2pnhZPdX89fgodMtVVmIE
15DbgaLOLn+CnnlJcT2kWoP7D3sZdzAvgyLB0yxByjMkRia4tahMrRh/iAjoTuEPr1
16LJCw2vDPET/BZogPkJoXVdN9y9mxGu5jn4rHQBYh88oup+Wz1qdJ28H9RztP6XFs
17+uDEgRAYVwg0c5fSrRFtoKI+Mk3gtJIU+cQ0YBzaR3E3U8YYANs=
18=Fh++
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

I also have one last lingering question which is: when does the miner actually construct their coinbase transaction and is that ever compared to the reserved size?

This is entirely up to the pool software. They could call getblocktemplate in proposal mode (or checkblock if #31564 lands).

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

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

Thanks for reviewing @pinheadmz @luke-jr @Sjors

I’ve recently foreced pushed from 2fb833e69627a8f9b1c4b14b60d1df9903adf210 to 7ab1344eb13228998bcead7328d5cc0a905971d4 diff

Changes were:

1. Reverting https://github.com/bitcoin/bitcoin/commit/76a95522b322c1a8ab1594ca8bf5ac99ab379c46 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 https://github.com/bitcoin/bitcoin/commit/4facb94a481ca121a9117e0992799371bb429da1.

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

ACK 7ab1344eb13228998bcead7328d5cc0a905971d4

Just some feedback on the release notes.

@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 dba211a9ceafc57a953efc757adfe09c0f3fdb14 to ad1bc03245181b00a25ea0182373eddae1c151e1 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 a7d43645ea353021b2143407ab06336105c82434 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 https://github.com/Sjors/bitcoin/pull/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: https://github.com/Sjors/bitcoin/pull/49/commits/f481d653a8549160d42aa2041a1c3b17bf0e466b#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.

Forced pushed from ad1bc03245181b00a25ea0182373eddae1c151e1 to fd3103142de8bcb92b5db6a6501c4c66abc12abd see diff

Changes

1. Addressed @Sjors comments

utACK fd4af64d58901334d907dd0bd38efd303a21374a

Solves a trivial rebase conflict in 9bbdcd84c8582f6a88ae01f207849c41bcb28380 where I added new fields to the getmininginfo (documentation).

ACK fd4af64d58901334d907dd0bd38efd303a21374a

This patch consolidates two 4000 WU reservations into one constant, and adjusts configuration parameters to maintain existing behavior. It allows more fine tuned control of block template creation and in particular will allow some mining pools to squeeze an extra 4000 WU of transaction data into new blocks.

Reviewed each commit (again). Built and ran unit and functional tests on macos/arm. I left a few if-you-touch nits below to improve clarity.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK fd4af64d58901334d907dd0bd38efd303a21374a
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmedK0sACgkQ5+KYS2KJ
 7yToizxAAn4RmV88bUEtsgrCkjaQCXee9wsWaerz1cO8XYnbI3wqgc9xQ2lZh1C5X
 8nds8sazTFHLWWYScFH50eFE45CrPsW5wU9RLkKQofH60w0vOlyrITmVzYSHaGydV
 9O9xVSD2e2PLW5qHzBj52uGy33yWrXVYFyYvxonVJzC72aVKFqnsJr6Y3pAb0Asva
10PzSyM5T1E1e8FULLoTBb2e9AMfVbXZtuosLW4fzRCi4WgctyJBsP/5pz/IuDBV88
11FOp9/6d5bFpcsw9hQuJ5mBVerJ4NA7o2qRKOgvGbRFHH1s2ICYqlWbotJnpKDqkY
12kvwTWUYIguD2r8gMh6r8fG+O/rS7HUUd5raEfg6iMHyJxzx5dqPmQ/vEEQconVRJ
13pfeJ5vxbc609MXu/+JfBBApIvwRfOrIFYjCJ+yi5oJs0BR2Ka5gvf0bOfpAvKkMt
14gRDzN1EsmhCNRKEqtYrUWZVD6E1iQbmMGf1ZuE5OnSJWWzb5otBaZgqR6OU2OGBE
15z1gvtMzZxWN2q5LjtR6zL3cDlRSpnYzZ1xQ2xvuTr5nr84zmT8jITfEU3MJaQqB8
163l56ifbChppMdBb8IPI0cQzeaJqxkh/EZlAT/D/YhlXLKebigv2MxFgqe0s+aGVp
17aSq3TuxH9uv4O6HYYAkHeqTbpXC4g6O4Qj107ck4C/6QLJvOQRM=
18=GjoX
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

This patch consolidates two 4000 WU reservations into one constant, and adjusts configuration parameters to maintain existing behavior. It allows more fine tuned control of block template creation and in particular will allow some mining pools to squeeze an extra 4000 WU of transaction data into new blocks.

Only an extra 2000 WU, right? Before the change setting setting blockmaxheight to max led to the actual minimum reserved 4000 WU. After the change there is the explicit minimum of 2000 WU reserved. So the delta is 2000 WU. Or did I get something wrong?

Only an extra 2000 WU, right?

My understanding goes like this: Imagine a miner that never needs more than 4000 for their coinbase, and has always used default settings. Their blocks have left behind 4000 empty space. After this patch, that same miner continuing to use default settings will continue to leave 4000 empty space per block. But now with the patch that miner has the option of filling that space with transaction data using -blockreservedweight=4000

Thanks for your review, @pinheadmz and @fjahr. I have answered the review questions and will address any nits when a retouch is needed. @fjahr, think of it this way:

In the block assembler, we need to leave some space for the coinbase transaction, block header, and transaction count.

Previously, we reserved this space in multiple places 4000 WU twice.
The blockmaxweight was described as 3996000 WU because we intended to reserve space only once. However, due to duplicate reservations, the actual reserved weight was 8000WU resulting in our block template weight to be always below 3992000 WU.

After this PR:

• I updated the help text to indicate that the default blockmaxweight is 4000000 WU.
• I introduced a new option, blockreservedweight, allowing miners to customize the reserved space.
• By default, blockreservedweight is set to 8000 WU to maintain backward compatibility with previous behavior.

However, miners can reduce this value if they don’t need that much reserved space.

• For example, by setting blockreservedweight=4000, miners will now get blocks with a weight of 3996000 WU, effectively claiming an extra 0.1 fee. which means they have 4000WU more I think this is what @pinheadmz meant

The minimum value for blockreservedweight is 2000 WU because we determined that any value below that is too risky see comments before #31384 (comment).

Thus, the maximum block template weight that will be created after this change is 3998000 WU, which is an increase of 6000 WU compared to the previous behavior.

@ismaelsadeeq @pinheadmz Ok, I see what you are trying to say. But if we are assuming the miner was smart and using the config options available to optimize, then they could have already reached 3996000 WU by setting blockmaxweight to 4000000 WU before this change, right? This is even mentioned in the release notes… Maybe the docs make it more realistic that miners actually know about this but the comment was about behavior change, not the docs. So I think the actual delta made possible by the behavior change is just 2000 WU and I think that is the fairest characterization.

It comes down to what behavior you expect of the miners: Miners before the change using defaults had 8000 WU and with configs they could get down to 4000 WU. After the change with defaults its 4000 WU and with configs it’s 2000 WU. So the delta comparing default config miners is 4000 WU and comparing ‘smart’ config optimizing miners is 2000 WU. I think 6000 WU is comparing apples to oranges a bit but you can argue that we can hope that his happens because miners read the new docs and thus are converting from default to ‘smart’.

But if we assume the miner was smart and used the available config options to optimize, they could have already reached 3,996,000 WU by setting blockmaxweight to 4,000,000 WU before this change, right?

No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.

See this test https://github.com/ismaelsadeeq/bitcoin/commit/a8db3890b240a649024fcf2526eaaeca9498826b. It fails on master.

So, I don’t think your intuition is right

No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.

Ok, I am very confused now. In the release notes you write this:

0- Users who manually set `-blockmaxweight` to its maximum value `4,000,000` can lower
1`-blockreservedweight` to `4,000` to maintain the previous behaviour.

So you are saying setting -blockreservedweight to 4,000 will give you a block with 8000 WU reserved even after the change?

No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.

Maybe I am still not getting it but re-reading the release notes once more, I don’t think they reflect this correctly. They seem to suggest that setting -blockmaxweight to 4,000,000 does change something. I think they need to explain what is actually happening so that miners can make the right decision. Worst case (though maybe unlikely): A miner set -blockmaxweight=4000000 and added a coinbase of 5,000 WU, maybe not even measuring size, it just works (TM). But this only works because of the actual 8,000 WU reserved. So the section I quoted above suggest that this miner could now switch to -blockreservedweight set to 4,000. But from my understanding of the code change this creates a block with just 4,000 WU reserved and when the miner adds their 5,000 WU coinbase the block is invalid.

@pinheadmz wrote:

some mining pools to squeeze an extra 4000 WU of transaction data into new blocks @fjahr wrote:

Only an extra 2000 WU, right?

Just 2000 extra WU. They could already push the effective reservation to 4000 by setting blockmaxweight=4000000 and there’s a minimum value of -blockreservedweight=2000. @ismaelsadeeq wrote:

No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.

Now I’m confused as well.

You’re right though I think, the original code clamps like this:

0static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
1{
2    Assert(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
3    Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
4    // Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
5    // Coinbase (reserved) outputs can safely exceed -blockmaxweight, but the rest of the block template will be empty.
6    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT);
7    return options;
8}

With DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};

So setting -blockmaxweight=4000000 was a placebo and we’re indeed making it possible to squeeze another 6000 WU.

This requires some changes to the release note.

Forced pushed to address recent comments by @fjahr @pinheadmz and @Sjors see diff

Changes

1. Updated release notes to be more accurate
2. Introduce DEFAULT_BLOCK_RESERVED_WEIGHT
3. Fixed nits

Here’s a patch that limits the use of DEFAULT_BLOCK_MAX_WEIGHT to defaults:

 0diff --git a/src/init.cpp b/src/init.cpp
 1index 41db01b129..f9eb466408 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -19,6 +19,7 @@
 5 #include <common/args.h>
 6 #include <common/system.h>
 7 #include <consensus/amount.h>
 8+#include <consensus/consensus.h>
 9 #include <deploymentstatus.h>
10 #include <hash.h>
11 #include <httprpc.h>
12@@ -1018,15 +1019,15 @@ bool AppInitParameterInteraction(const ArgsManager& args)
13 
14     if (args.IsArgSet("-blockmaxweight")) {
15         const auto max_block_weight = args.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT);
16-        if (max_block_weight > DEFAULT_BLOCK_MAX_WEIGHT) {
17-            return InitError(strprintf(_("Specified -blockmaxweight (%d) exceeds policy default maximum block weight (%d)"), max_block_weight, DEFAULT_BLOCK_MAX_WEIGHT));
18+        if (max_block_weight > MAX_BLOCK_WEIGHT) {
19+            return InitError(strprintf(_("Specified -blockmaxweight (%d) exceeds consensus limit (%d)"), max_block_weight, MAX_BLOCK_WEIGHT));
20         }
21     }
22 
23     if (args.IsArgSet("-blockreservedweight")) {
24         const auto block_reserved_weight = args.GetIntArg("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT);
25-        if (block_reserved_weight > DEFAULT_BLOCK_MAX_WEIGHT) {
26-            return InitError(strprintf(_("Specified -blockreservedweight (%d) exceeds policy default maximum block weight (%d)"), block_reserved_weight, DEFAULT_BLOCK_MAX_WEIGHT));
27+        if (block_reserved_weight > MAX_BLOCK_WEIGHT) {
28+            return InitError(strprintf(_("Specified -blockreservedweight (%d) exceeds consensus maximum block weight (%d)"), block_reserved_weight, MAX_BLOCK_WEIGHT));
29         }
30         if (block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
31             return InitError(strprintf(_("Specified -blockreservedweight (%d) is lower than minimum safety value of (%d)"), block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT));
32diff --git a/src/node/miner.cpp b/src/node/miner.cpp
33index 091cb436dc..ca4d65ee08 100644
34--- a/src/node/miner.cpp
35+++ b/src/node/miner.cpp
36@@ -67,12 +67,12 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
37 
38 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
39 {
40-    Assert(options.block_reserved_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
41+    Assert(options.block_reserved_weight <= MAX_BLOCK_WEIGHT);
42     Assert(options.block_reserved_weight >= MINIMUM_BLOCK_RESERVED_WEIGHT);
43     Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
44-    // Limit weight to between block_reserved_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
45+    // Limit weight to between block_reserved_weight and MAX_BLOCK_WEIGHT for sanity:
46     // block_reserved_weight can safely exceed -blockmaxweight, but the rest of the block template will be empty.
47-    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.block_reserved_weight, DEFAULT_BLOCK_MAX_WEIGHT);
48+    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.block_reserved_weight, MAX_BLOCK_WEIGHT);
49     return options;
50 }

With matching test change:

 0diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py
 1index afc7585e07..68334e0417 100755
 2--- a/test/functional/mining_basic.py
 3+++ b/test/functional/mining_basic.py
 4@@ -289,7 +289,7 @@ class MiningTest(BitcoinTestFramework):
 5         self.stop_node(0)
 6         self.nodes[0].assert_start_raises_init_error(
 7             extra_args=[f"-blockreservedweight={MAX_BLOCK_WEIGHT + 1}"],
 8-            expected_msg=f"Error: Specified -blockreservedweight ({MAX_BLOCK_WEIGHT + 1}) exceeds policy default maximum block weight ({MAX_BLOCK_WEIGHT})",
 9+            expected_msg=f"Error: Specified -blockreservedweight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEIGHT})",
10         )
11 
12         self.log.info(f"Test that node will fail to start when user provide -blockreservedweight below {MINIMUM_BLOCK_RESERVED_WEIGHT}")
13@@ -303,7 +303,7 @@ class MiningTest(BitcoinTestFramework):
14         self.stop_node(0)
15         self.nodes[0].assert_start_raises_init_error(
16             extra_args=[f"-blockmaxweight={MAX_BLOCK_WEIGHT + 1}"],
17-            expected_msg=f"Error: Specified -blockmaxweight ({MAX_BLOCK_WEIGHT + 1}) exceeds policy default maximum block weight ({MAX_BLOCK_WEIGHT})",
18+            expected_msg=f"Error: Specified -blockmaxweight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus limit ({MAX_BLOCK_WEIGHT})",
19         )
20 

Dropping -maxblockweight entirely, along with DEFAULT_BLOCK_MAX_WEIGHT, would simplify things, but better left to a followup.

Force-pushed from 6709e185b93d97b9191597acfecb25c33ab43a0a to 386eecff5f14d508688e6e7374b67cb54aaa7249.

See 6709e185b9….386eecff5f14

• The changes in this push ensure that the consensus MAX_BLOCK_WEIGHT is used in block assembler instead of a policy constant DEFAULT_BLOCK_MAX_WEIGHT.

  • The policy value is only used when the user has not provided a custom value.
  • Currently, this value matches the consensus limit.
  • Using the consensus limit instead of the policy value prevents bugs, such as the one on the master branch, where clamping to the policy default would prevent user specified higher values from taking effect described in detail in https://github.com/bitcoin/bitcoin/pull/31384/commits/386eecff5f14d508688e6e7374b67cb54aaa7249

• Docs changes

C.I failure seems to be unrelated.

C.I failure seems to be unrelated.

It does indeed but so far 5 jobs have failed so it might be better to kick the CI since it could be that the unrelated (early) fail masks an actual issue in one of them. I’ve just asked for this on IRC.

Looking good, just curious about the fuzzer change.

I left comments there but I don’t want to bikeshed the release notes too much, so feel free to ignore them unless you have to retouch or don’t want to address them.