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

pull ismaelsadeeq wants to merge 6 commits into bitcoin:master from ismaelsadeeq:11-2024-fix-duplicate-coinbase-reservation-bug changing 11 files +188 −18
  1. ismaelsadeeq commented at 7:57 pm on November 27, 2024: member
    • 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.

  2. DrahtBot commented at 7:57 pm on November 27, 2024: contributor

    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.

    Type Reviewers
    ACK Sjors
    Concept ACK 0xB10C
    Approach ACK TheCharlatan
    Stale ACK pinheadmz

    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.

  3. DrahtBot added the label Mining on Nov 27, 2024
  4. ismaelsadeeq renamed this:
    mining: bugfix: Fix duplicate tx coinbase weight reservation
    mining: bugfix: Fix duplicate coinbase tx weight reservation
    on Nov 27, 2024
  5. ismaelsadeeq force-pushed on Nov 27, 2024
  6. DrahtBot added the label CI failed on Nov 27, 2024
  7. DrahtBot commented at 8:01 pm on November 27, 2024: contributor

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

  8. ismaelsadeeq force-pushed on Nov 27, 2024
  9. DrahtBot removed the label CI failed on Nov 27, 2024
  10. in src/node/types.h:40 in 74a5d7bf4b outdated 
    36@@ -37,7 +37,7 @@ struct BlockCreateOptions {
37      * scriptSig, witness and outputs. This must include any additional
38      * weight needed for larger CompactSize encoded lengths.
39      */
40-    size_t coinbase_max_additional_weight{4000};
41+    size_t coinbase_max_additional_weight{0};
    Sjors commented at 1:20 pm on November 29, 2024:
    I don’t think setting coinbase_max_additional_weight to 0 is the right approach. Stratum v2 relies on being able to tweak this value.
    ismaelsadeeq commented at 10:35 pm on November 30, 2024:
    Switched to the approach you suggested.
  11. in test/functional/mining_basic.py:79 in 74a5d7bf4b outdated 
    67@@ -68,7 +68,7 @@ def mine_chain(self):
68         mining_info = self.nodes[0].getmininginfo()
69         assert_equal(mining_info['blocks'], 200)
70         assert_equal(mining_info['currentblocktx'], 0)
71-        assert_equal(mining_info['currentblockweight'], 4000)
72+        assert_equal(mining_info['currentblockweight'], 0)
    Sjors commented at 1:23 pm on November 29, 2024:
    I don’t have an opinion on whether currentblockweight should include the reservation. But the documentation should be clarified either way.
    ismaelsadeeq commented at 10:36 pm on November 30, 2024:
    This is gone now, thanks.
    Sjors commented at 10:53 am on January 9, 2025:
    Sorry, I think @luke-jr is right and we shouldn’t change this: https://github.com/bitcoin/bitcoin/pull/31384/files#r1908533084
  12. Sjors commented at 1:55 pm on November 29, 2024: member
    I’m in favor of fixing this bug, but differently, see #21950 (comment)
  13. Sjors commented at 2:16 pm on November 29, 2024: member

    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.

  14. ismaelsadeeq force-pushed on Nov 30, 2024
  15. ismaelsadeeq force-pushed on Nov 30, 2024
  16. DrahtBot added the label CI failed on Nov 30, 2024
  17. DrahtBot commented at 10:42 pm on November 30, 2024: contributor

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

  18. ismaelsadeeq commented at 10:53 pm on November 30, 2024: member

    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.

  19. DrahtBot removed the label CI failed on Nov 30, 2024
  20. Sjors commented at 10:19 am on December 2, 2024: member

    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.

  21. DrahtBot added the label Needs rebase on Dec 4, 2024
  22. ismaelsadeeq force-pushed on Dec 4, 2024
  23. ismaelsadeeq force-pushed on Dec 4, 2024
  24. DrahtBot removed the label Needs rebase on Dec 5, 2024
  25. Sjors commented at 8:32 am on December 6, 2024: member

    ACK f7a2f125c7b23c4c3c2e5253a6d3e2dcd8d3fe97 (see comment below)

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

  26. in doc/release-notes-31384.md:5 in f7a2f125c7 outdated 
    0@@ -0,0 +1,7 @@
1+- Node and Mining
2+
3+---
4+
5+- PR #31384 fixed an issue where duplicate coinbase transaction weight reservation occurred. The `getblocktemplate` RPC will now correctly creates blocks with a weight of `3,996,000` weight units, reserving `4,000` weight units for the coinbase transaction.
    pinheadmz commented at 3:18 pm on December 9, 2024:

    nit:

    The getblocktemplate RPC will now create blocks up to a maximum weight of 3,996,000 weight units …

    ismaelsadeeq commented at 8:08 am on December 10, 2024:
    Thanks I will change when retouching.
    ismaelsadeeq commented at 6:49 pm on January 6, 2025:
    fixed. thanks @pinheadmz
  27. darosior commented at 3:57 pm on December 9, 2024: member

    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:

    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.

  28. in src/node/miner.cpp:74 in f7a2f125c7 outdated 
    72     Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
73-    // Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
74+    // Limit weight to between coinbase_max_additional_weight and MAX_BLOCK_WEIGHT for sanity:
75     // Coinbase (reserved) outputs can safely exceed -blockmaxweight, but the rest of the block template will be empty.
76-    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT);
77+    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, MAX_BLOCK_WEIGHT);
    pinheadmz commented at 5:55 pm on December 9, 2024:
    I don’t think you need to change anything but it’s kind of a funny comment (the existing line 73) since the coinbase size can only exceed -blockmaxweight if it does not also exceed MAX_BLOCK_WEIGHT
  29. pinheadmz approved
  30. pinheadmz commented at 6:17 pm on December 9, 2024: member

    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

  31. pinheadmz commented at 8:50 pm on December 9, 2024: member

    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.

  32. Sjors commented at 5:54 am on December 10, 2024: member

    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.

  33. ismaelsadeeq commented at 8:07 am on December 10, 2024: member

    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.

  34. Sjors commented at 9:19 am on December 10, 2024: member

    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.

  35. ismaelsadeeq commented at 6:57 pm on January 1, 2025: member

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

  36. Sjors commented at 9:18 am on January 2, 2025: member

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

  37. ismaelsadeeq commented at 3:18 pm on January 2, 2025: member

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

  38. Sjors commented at 11:06 am on January 3, 2025: member

    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.

  39. ismaelsadeeq force-pushed on Jan 4, 2025
  40. ismaelsadeeq commented at 1:41 am on January 4, 2025: member

    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.

  41. Sjors commented at 1:59 pm on January 4, 2025: member

    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.

  42. in src/init.cpp:652 in cd48e2ccaf outdated 
    648@@ -647,7 +649,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
649     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);
650 
651 
652-    argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
653+    argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d). Note: default policy is to reserve %d weight units for coinbase transaction.", MAX_BLOCK_WEIGHT, node::BlockCreateOptions{}.coinbase_max_additional_weight), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
    TheCharlatan commented at 11:55 am on January 5, 2025:
    Typo nit: s/for coinbase/for the coinbase/
    ismaelsadeeq commented at 6:31 pm on January 6, 2025:
    This is gone now.
  43. in src/init.cpp:58 in a350a67960 outdated 
    54@@ -54,6 +55,7 @@
55 #include <node/mempool_persist_args.h>
56 #include <node/miner.h>
57 #include <node/peerman_args.h>
58+#include <node/types.h> // for BlockCreateOptions
    TheCharlatan commented at 12:07 pm on January 5, 2025:
    Nit: I don’t think adding these comments after an include is useful. They can become stale easily and it adds to the general inconsistencies of the codebase. Can you remove it again?
    ismaelsadeeq commented at 6:31 pm on January 6, 2025:
    fixed
  44. in test/functional/mining_basic.py:237 in abefbd201d outdated 
    230+        utxos = [self.wallet.get_utxo(confirmed_only=True) for _ in range(LARGE_TXS_COUNT + 4)] # Add 4 more utxos that will be used in the test later
231+        self.send_transactions(utxos[:LARGE_TXS_COUNT], Decimal("0.0003"), LARGE_VSIZE)
232+
233+        # Send 2 normal transactions with a lower fee rate
234+        NORMAL_VSIZE = int(2000 / WITNESS_SCALE_FACTOR)
235+        NORMAL_FEERATE = Decimal("0.0001")
    TheCharlatan commented at 12:55 pm on January 5, 2025:
    Nit: Since you are creating a constant for the normal feerate, maybe create one for the fee rate of the large transactions too?
    ismaelsadeeq commented at 5:39 pm on January 6, 2025:
    done
  45. in test/functional/mining_basic.py:256 in abefbd201d outdated 
    251+        # Reducing the weight by 2000 units will prevent 1 large transaction from fitting into the block.
252+        self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}", f"-blockmaxweight={custom_block_weight}"])
253+
254+        self.log.info("Testing the block template with custom -blockmaxweight to include 9 large and 2 normal transactions.")
255+        self.verify_block_template(
256+            expected_tx_count=LARGE_TXS_COUNT + 1,
    TheCharlatan commented at 1:10 pm on January 5, 2025:
    Nit: This is a bit confusing, I think something like (LARGE_TXS_COUNT - 1) + NORMAL_TXS_COUNT would be more accurate, or just give the number like you do in the next test block.
    ismaelsadeeq commented at 5:39 pm on January 6, 2025:
    done.
  46. in test/functional/mining_basic.py:260 in abefbd201d outdated 
    255+        self.verify_block_template(
256+            expected_tx_count=LARGE_TXS_COUNT + 1,
257+            expected_weight=MAX_BLOCK_WEIGHT - DEFAULT_COINBASE_TX_WEIGHT - 2000,
258+        )
259+
260+        # Test that DEFAULT_COINBASE_TX_WEIGHT is always reserved
    TheCharlatan commented at 1:26 pm on January 5, 2025:
    Is there a material difference between this test block and the previous one that actually exercises the default coinbase weight? Could more exact weight accounting where the total weight is checked with assert_equal instead of assert_greater_than_or_equal in verify_block_template be useful?
    ismaelsadeeq commented at 4:57 pm on January 6, 2025:

    Is there a material difference between this test block and the previous one that actually exercises the default coinbase weight?

    There is no any material difference, removed.

    Could more exact weight accounting where the total weight is checked with assert_equal instead of assert_greater_than_or_equal in verify_block_template be useful?

    It would be useful, but the reason why I am not is because there is a little disparity between the target weight and the actual weight.

  47. in src/node/miner.cpp:103 in e504d15cfb outdated 
    101-    nBlockWeight = m_options.coinbase_max_additional_weight;
102-    nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops;
103-
104     // These counters do not include coinbase tx
105+    nBlockWeight = 0;
106+    nBlockSigOpsCost = 0;
    TheCharlatan commented at 1:55 pm on January 5, 2025:
    Does this have an effect on the log line in miner.cpp::158? Maybe clarify there that the numbers are given without the coinbase?
    ismaelsadeeq commented at 6:28 pm on January 6, 2025:

    I think it has.

    I’ve even modified it to also use the correct weight and clarify the log.

  48. in doc/release-notes-31384.md:8 in cd48e2ccaf outdated 
    0@@ -0,0 +1,11 @@
1+- Node and Mining
2+
3+---
4+
5+- PR #31384 fixed an issue where the coinbase transaction weight was being reserved in two different places.
6+ The fix ensures the reservation happens in one place, but does not change the previous behaviour by setting the default
7+ to the previous value of `8,000`.
8+ However, this PR offers the advantage of overriding the default with a user-desired value and allow creating blocks
    TheCharlatan commented at 2:06 pm on January 5, 2025:
    I’m a bit confused by this second sentence. How does the behaviour introduced here allow for overriding the default?
    ismaelsadeeq commented at 6:29 pm on January 6, 2025:
    This is fixed now.
  49. TheCharlatan commented at 2:06 pm on January 5, 2025: contributor
    Approach ACK
  50. ismaelsadeeq force-pushed on Jan 6, 2025
  51. ismaelsadeeq force-pushed on Jan 6, 2025
  52. DrahtBot commented at 6:42 pm on January 6, 2025: contributor

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

  53. DrahtBot added the label CI failed on Jan 6, 2025
  54. ismaelsadeeq commented at 6:45 pm on January 6, 2025: member

    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
  55. DrahtBot removed the label CI failed on Jan 7, 2025
  56. in doc/release-notes-31384.md:9 in ededdd13f3 outdated 
    0@@ -0,0 +1,12 @@
1+- Node and Mining
2+
3+---
4+
5+- PR #31384 fixed an issue where the coinbase transaction weight was being reserved in two different places.
6+  The fix ensures that the reservation happens in a single place.
7+
8+- This PR also introduces a new startup option, `-maxcoinbaseweight` (default: `8,000` weight units).
9+  All created block template will reserves the default or specified value for the miner coinbase transaction.
    Sjors commented at 9:07 am on January 7, 2025:

    ededdd13f3263df08f7878aa2514d3796436cbfb: maybe replace this line with:

    0This specifies how many weight units to reserve for
1the coinbase transaction.
2
3Upgrade note: the default of `-maxcoinbaseweight`
4ensures backward compatibility for users who did
5not override `-blockmaxweight`. If you previously
6configured `-blockmaxweight=4000000` then you can
7safely set `-maxcoinbaseweight=4000` to maintain
8existing behavior.
    ismaelsadeeq commented at 7:32 pm on January 7, 2025:
    Done
  57. in src/test/fuzz/mini_miner.cpp:153 in 354f41f5c7 outdated 
    147@@ -148,9 +148,9 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
148             }
149         }
150 
151-        // Stop if pool reaches DEFAULT_BLOCK_MAX_WEIGHT because BlockAssembler will stop when the
152+        // Stop if pool reaches MAX_BLOCK_WEIGHT because BlockAssembler will stop when the
153         // block template reaches that, but the MiniMiner will keep going.
154-        if (pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx) >= DEFAULT_BLOCK_MAX_WEIGHT) break;
155+        if (pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx) >= MAX_BLOCK_WEIGHT) break;
    Sjors commented at 12:09 pm on January 7, 2025:

    354f41f5c76f5f43cbfdddb63660cc77c4a76989: I think this should be MAX_BLOCK_WEIGHT - coinbase_max_additional_weight, because later on this fuzzer adds a small coinbase transaction.

    (can probably just hardcode something like - 4000)

    ismaelsadeeq commented at 7:34 pm on January 7, 2025:
    Nice catch, fixed but without hardcoding. The comment above also needs to be updated, I did that.
  58. in src/init.cpp:1021 in 6cc658075e outdated 
    1016@@ -1017,6 +1017,13 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1017         }
1018     }
1019 
1020+    if (args.IsArgSet("-blockmaxweight")) {
1021+        const auto customMaxBlockWeight = args.GetIntArg("-blockmaxweight", MAX_BLOCK_WEIGHT);
    Sjors commented at 12:14 pm on January 7, 2025:
    6cc658075e1be59ef49a060b36921538acf0edb1 nit: current naming convention uses snake case. Maybe just use max_block_weight.
    ismaelsadeeq commented at 7:34 pm on January 7, 2025:
    Done!
  59. in src/node/miner.cpp:158 in 76a95522b3 outdated 
    154@@ -158,7 +155,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
155     pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev);
156     pblocktemplate->vTxFees[0] = -nFees;
157 
158-    LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost);
159+    LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d. (excluding coinbase transaction)\n", nBlockWeight, nBlockTx, nFees, nBlockSigOpsCost);
    Sjors commented at 12:17 pm on January 7, 2025:
    76a95522b322c1a8ab1594ca8bf5ac99ab379c46: to keep log lines short, maybe say (ex. coinbase). Maybe also move it left: CreateNewBlock(): block weight (ex. coinbase): ...
    ismaelsadeeq commented at 7:35 pm on January 7, 2025:

    I think ex. is ambiguous so I shortened it by writing tx instead of transaction.

    Also this is not logged frequently, only by miners I think.

    Sjors commented at 10:30 am on January 8, 2025:
    With #31283 this will be logged every second (by miners).
  60. in src/rpc/mining.cpp:422 in 76a95522b3 outdated 
    417@@ -418,8 +418,8 @@ static RPCHelpMan getmininginfo()
418                     RPCResult::Type::OBJ, "", "",
419                     {
420                         {RPCResult::Type::NUM, "blocks", "The current block"},
421-                        {RPCResult::Type::NUM, "currentblockweight", /*optional=*/true, "The block weight of the last assembled block (only present if a block was ever assembled)"},
422-                        {RPCResult::Type::NUM, "currentblocktx", /*optional=*/true, "The number of block transactions of the last assembled block (only present if a block was ever assembled)"},
423+                        {RPCResult::Type::NUM, "currentblockweight", /*optional=*/true, "The block weight (excluding coinbase transaction) of the last assembled block (only present if a block was ever assembled)"},
424+                        {RPCResult::Type::NUM, "currentblocktx", /*optional=*/true, "The number of block transactions (excluding coinbase transaction) of the last assembled block (only present if a block was ever assembled)"},
    Sjors commented at 12:26 pm on January 7, 2025:
    76a95522b322c1a8ab1594ca8bf5ac99ab379c46 nit: (excluding coinbase), since the description already says “transactions”
    ismaelsadeeq commented at 7:34 pm on January 7, 2025:
    Done!
  61. Sjors commented at 12:29 pm on January 7, 2025: member
    Code reviewed ededdd13f3263df08f7878aa2514d3796436cbfb, almost there.
  62. ismaelsadeeq force-pushed on Jan 7, 2025
  63. ismaelsadeeq force-pushed on Jan 7, 2025
  64. DrahtBot added the label CI failed on Jan 7, 2025
  65. DrahtBot commented at 7:37 pm on January 7, 2025: contributor

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

  66. DrahtBot removed the label CI failed on Jan 7, 2025
  67. in doc/release-notes-31384.md:11 in 7452638559 outdated 
    0@@ -0,0 +1,17 @@
1+- Node and Mining
2+
3+---
4+
5+- PR #31384 fixed an issue where the coinbase transaction weight was being reserved in two different places.
6+  The fix ensures that the reservation happens in a single place.
7+
    Sjors commented at 10:39 am on January 8, 2025:

    You lost a line here:

    0- This PR also introduces a new startup option, `-maxcoinbaseweight` (default: `8,000` weight units).
  68. Sjors approved
  69. Sjors commented at 10:39 am on January 8, 2025: member
    ACK 7452638559e8ccbe00db5ef5a53cb21ffe27d337
  70. DrahtBot requested review from pinheadmz on Jan 8, 2025
  71. DrahtBot requested review from TheCharlatan on Jan 8, 2025
  72. ismaelsadeeq force-pushed on Jan 8, 2025
  73. in test/functional/mining_basic.py:258 in b55bb95d8d outdated 
    252+        # Reducing the weight by 2000 units will prevent 1 large transaction from fitting into the block.
253+        self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}", f"-blockmaxweight={custom_block_weight}"])
254+
255+        self.log.info("Testing the block template with custom -blockmaxweight to include 9 large and 2 normal transactions.")
256+        self.verify_block_template(
257+            expected_tx_count=11,
    pinheadmz commented at 8:01 pm on January 8, 2025:

    b55bb95d8d80175f30d2d4da5c6cfd72e5d8eedd

    nit: could be LARGE_TXS_COUNT + 1 to make it clear the difference from previous template

    ismaelsadeeq commented at 5:25 pm on January 9, 2025:

    I did this to address #31384 (review)

    Let me know if (LARGE_TXS_COUNT - 1) + NORMAL_TXS_COUNT is preferable.

  74. in src/init.cpp:653 in a45af6af4e outdated 
    649@@ -650,6 +650,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
650 
651 
652     argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d).", MAX_BLOCK_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
653+    argsman.AddArg("-maxcoinbaseweight=<n>", strprintf("Set the block maximum reserved coinbase transaction weight (default: %d).", node::BlockCreateOptions{}.coinbase_max_additional_weight), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
    pinheadmz commented at 8:17 pm on January 8, 2025:

    a45af6af4e7a59cd14ef6e16b8e5e180fa9d564d

    Question about this phrasing: what is “maximum” about it? If a user sets -maxcoinbaseweight=1000 then 1000 weight will always be reserved in block templates, and if the actual coinbase ends up being 50 weight instead of 1000 that extra weight will just be unused and the final block will be smaller than maximum size.

    Sjors commented at 10:30 am on January 9, 2025:

    Correct.

    “max” in the variable name refers to the maximum coinbase weight that a pool can add. So “maximum reservation” is perhaps not the best wording. Maybe try:

    0Reserve space to account for the largest coinbase transaction
1that mining software may add to the block.
    Sjors commented at 10:48 am on January 9, 2025:
    Perhaps -coinbaseweightreservation / -coinbasereservedweight is a better name.
    ismaelsadeeq commented at 5:27 pm on January 9, 2025:
    I’ve taken your suggestion @Sjors.
  75. in src/init.cpp:1030 in a45af6af4e outdated 
    1024@@ -1024,6 +1025,13 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1025         }
1026     }
1027 
1028+    if (args.IsArgSet("-maxcoinbaseweight")) {
1029+        const auto max_coinbase_weight = args.GetIntArg("-maxcoinbaseweight", node::BlockCreateOptions{}.coinbase_max_additional_weight);
1030+        if (max_coinbase_weight > MAX_BLOCK_WEIGHT) {
    pinheadmz commented at 8:19 pm on January 8, 2025:

    a45af6af4e7a59cd14ef6e16b8e5e180fa9d564d

    Should this test against args.GetIntArg("-blockmaxweight", MAX_BLOCK_WEIGHT) ? Or, what happens if a user sets -blockmaxweight=1000 -maxcoinbaseweight=4000 ?

    Sjors commented at 10:36 am on January 9, 2025:
    Current ClampOptions in node/miner.cpp has a comment that specifically permits -maxcoinbaseweight to exceed -blockmaxweight. It just means that all blocks will be empty. This seems like an unrealistic scenario anyway.
  76. in doc/release-notes-31384.md:11 in 2fb833e696 outdated 
     6+  The fix ensures that the reservation happens in a single place.
 7+
 8+- This PR also introduces a new startup option, `-maxcoinbaseweight` (default: `8,000` weight units).
 9+  This specifies how many weight units to reserve for the coinbase transaction.
10+- Upgrade note: the default of `-maxcoinbaseweight` ensures backward compatibility for users who did
11+  not override `-blockmaxweight`.
    pinheadmz commented at 8:22 pm on January 8, 2025:

    2fb833e69627a8f9b1c4b14b60d1df9903adf210

    You might want to mention the old default of 3996000 here. I was staring at blockmaxweight=4000000 on the following line for a minute trying to figure out what actually changed.

    Sjors commented at 10:29 am on January 9, 2025:

    Good point, maybe add:

    0The previous `-blockmaxweight` default was `3,996,000`. The block
1template construction code added an additional `4,000` padding.
2The new default `-maxcoinbaseweight` value was chosen to maintain
3that behaviour.
    ismaelsadeeq commented at 5:28 pm on January 9, 2025:
    I’ve taken this with a little modification.
  77. pinheadmz approved
  78. pinheadmz commented at 8:28 pm on January 8, 2025: member

    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

  79. DrahtBot requested review from Sjors on Jan 8, 2025
  80. in src/node/miner.cpp:103 in 2fb833e696 outdated 
    103-    nBlockWeight = m_options.coinbase_max_additional_weight;
104-    nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops;
105-
106     // These counters do not include coinbase tx
107+    nBlockWeight = 0;
108+    nBlockSigOpsCost = 0;
    luke-jr commented at 11:39 pm on January 8, 2025:
    This changes the meaning of getmininginfo fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.
    Sjors commented at 10:44 am on January 9, 2025:

    I agree we should not change the weight, currentblockweight and sigops fields in getblocktemplate and getmininginfo.

    We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.

    Additionally we should clarify the help text for these RPC methods that they include the coinbase reserved weight.

    It currently says: “as counted for purposes of block limits”, which is still a bit ambiguous so maybe add: “, including the coinbase reservation (see -maxcoinbaseweight).”

    ismaelsadeeq commented at 5:33 pm on January 9, 2025:

    This changes the meaning of getmininginfo fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.

    We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.

    I’ve reverted and included it but clarify the getmininginfo help text.

    I agree we should not change the weight, currentblockweight and sigops fields in getblocktemplate and getmininginfo.

    This only affects getmininginfo I think the weight and sigops results of getblocktemplate are for individual transactions in the block template.

    Sjors commented at 9:31 am on January 10, 2025:

    I think the weight and sigops results of getblocktemplate are for individual transactions in the block template.

    Ah that would make sense, since I tried to add those totals in an early version of #27433.

  81. in src/policy/policy.h:23 in 2fb833e696 outdated 
    18@@ -19,8 +19,6 @@ class CCoinsViewCache;
19 class CFeeRate;
20 class CScript;
21 
22-/** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/
23-static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
    luke-jr commented at 11:41 pm on January 8, 2025:
    This should remain logically separate from consensus constant.
    Sjors commented at 10:50 am on January 9, 2025:
    Seems reasonable to keep and just make it DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT};
    ismaelsadeeq commented at 5:34 pm on January 9, 2025:
    Done.
  82. luke-jr changes_requested
  83. Sjors commented at 10:56 am on January 9, 2025: member

    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.

  84. ismaelsadeeq force-pushed on Jan 9, 2025
  85. ismaelsadeeq commented at 5:41 pm on January 9, 2025: member

    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)

  86. in doc/release-notes-31384.md:6 in 7ab1344eb1 outdated 
    0@@ -0,0 +1,22 @@
1+- Node and Mining
2+
3+---
4+
5+- **PR #31384** fixed an issue where the coinbase transaction weight was being reserved in two separate places.
6+  Previously, the default `-blockmaxweight` was set to `4,000,000`. The block template generation algorithm should
    Sjors commented at 9:45 am on January 10, 2025:

    7ab1344eb13228998bcead7328d5cc0a905971d4

    -blockmaxweight was never 4,000,000 by default

    Maybe change this and the next two lines to:

    0When the node constructs a new block, e.g. for a `getblocktemplate` RPC call,
1it doesn't generate the coinbase transaction. Instead it reserves `4,000` weight
2units (WU) for it. Before this pull request, the default for `-blockmaxweight`
3was `3,996,000`, which effectively added another `4,000` weight units (WU) to
4this reservation, leading to a template size of `3,992,000`.
    ismaelsadeeq commented at 10:36 am on January 10, 2025:
    Taken, thanks.
  87. in doc/release-notes-31384.md:19 in 7ab1344eb1 outdated 
    14+
15+- **Upgrade Note:** The default `-coinbasereservedweight` ensures backward compatibility for users who did not override
16+  `-blockmaxweight` and relied on the previous behavior.
17+
18+- For users who do not rely on the previous behavior, setting `-coinbasereservedweight=4000`
19+  allows block templates of size `3,996,000` WU to be generated.
    Sjors commented at 9:54 am on January 10, 2025:

    I think think this wording is a bit too vague, how about:

    0Users who manually set `-blockmaxweight` to its maximum value `4,000,000` can lower
1`-coinbasereservedweight` to `4,000` to maintain the previous behaviour.
    ismaelsadeeq commented at 10:36 am on January 10, 2025:
    fixed
  88. in doc/release-notes-31384.md:21 in 7ab1344eb1 outdated 
    16+  `-blockmaxweight` and relied on the previous behavior.
17+
18+- For users who do not rely on the previous behavior, setting `-coinbasereservedweight=4000`
19+  allows block templates of size `3,996,000` WU to be generated.
20+
21+- Bitcoin Core will now **fail to start** if the `-blockmaxweight` or `-maxcoinbaseweight` init parameter exceeds
    Sjors commented at 9:57 am on January 10, 2025:
    -coinbasereservedweight
    ismaelsadeeq commented at 10:36 am on January 10, 2025:
    fixed
  89. Sjors approved
  90. Sjors commented at 10:08 am on January 10, 2025: member

    ACK 7ab1344eb13228998bcead7328d5cc0a905971d4

    Just some feedback on the release notes.

  91. DrahtBot requested review from pinheadmz on Jan 10, 2025
  92. ismaelsadeeq force-pushed on Jan 10, 2025
  93. Sjors commented at 10:45 am on January 10, 2025: member
    re-ACK dba211a9ceafc57a953efc757adfe09c0f3fdb14
  94. 0xB10C commented at 3:35 pm on January 10, 2025: contributor
    Concept ACK
  95. Sjors commented at 9:50 am on January 14, 2025: member

    @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

  96. 0xB10C commented at 9:51 am on January 14, 2025: contributor

    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

  97. ismaelsadeeq commented at 3:09 pm on January 14, 2025: member

    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.

  98. Sjors commented at 3:54 pm on January 14, 2025: member

    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)

  99. ismaelsadeeq force-pushed on Jan 14, 2025
  100. ismaelsadeeq force-pushed on Jan 14, 2025
  101. ismaelsadeeq commented at 11:14 pm on January 14, 2025: member

    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
  102. Sjors commented at 9:47 am on January 15, 2025: member

    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.

  103. in src/node/types.h:37 in a7d43645ea outdated 
    33@@ -34,11 +34,11 @@ struct BlockCreateOptions {
34      */
35     bool use_mempool{true};
36     /**
37-     * The maximum additional weight which the pool will add to the coinbase
38-     * scriptSig, witness and outputs. This must include any additional
39-     * weight needed for larger CompactSize encoded lengths.
40+     * The default reserved weight for the coinbase (scriptSig, witness and outputs)
    Sjors commented at 9:54 am on January 15, 2025:

    a7d43645ea353021b2143407ab06336105c82434: you can leave out “(scriptSig, witness and outputs)” as well as “This must include …”, they are left-overs from when this reflected CoinbaseOutputDataSize, see #31384 (comment)

    Suggested text:

    The default reserved weight for the block header, transaction count and coinbase transaction.

    ismaelsadeeq commented at 4:00 pm on January 16, 2025:
    Taken!
  104. in src/rpc/mining.cpp:422 in 9990552514 outdated 
    418@@ -419,8 +419,8 @@ static RPCHelpMan getmininginfo()
419                     RPCResult::Type::OBJ, "", "",
420                     {
421                         {RPCResult::Type::NUM, "blocks", "The current block"},
422-                        {RPCResult::Type::NUM, "currentblockweight", /*optional=*/true, "The block weight of the last assembled block (only present if a block was ever assembled)"},
423-                        {RPCResult::Type::NUM, "currentblocktx", /*optional=*/true, "The number of block transactions of the last assembled block (only present if a block was ever assembled)"},
424+                        {RPCResult::Type::NUM, "currentblockweight", /*optional=*/true, "The block weight (include coinbase reserved weight) of the last assembled block (only present if a block was ever assembled)"},
    Sjors commented at 10:01 am on January 15, 2025:
    9990552514328da45c19ed294c5bc79562aa12bc nit: “including”
    ismaelsadeeq commented at 4:01 pm on January 16, 2025:
    fixed, even better I clarified that it also included the block header and txs count.
  105. in src/init.cpp:1032 in 131eb4e630 outdated 
    1027+        const auto block_reserved_weight = args.GetIntArg("-blockreservedweight", node::BlockCreateOptions{}.block_reserved_weight);
1028+        if (block_reserved_weight > DEFAULT_BLOCK_MAX_WEIGHT) {
1029+            return InitError(strprintf(_("Specified -blockreservedweight (%d) exceeds consensus maximum block weight (%d)"), block_reserved_weight, DEFAULT_BLOCK_MAX_WEIGHT));
1030+        }
1031+        if (block_reserved_weight < node::BlockCreateOptions{}.minimum_block_reserved_weight) {
1032+            return InitError(strprintf(_("Specified -blockreservedweight (%d) is lower than minimum sane value of (%d)"), block_reserved_weight, node::BlockCreateOptions{}.minimum_block_reserved_weight));
    Sjors commented at 10:04 am on January 15, 2025:
    131eb4e630f958f1b5330bb9d170716c33655880: sane -> safety
    ismaelsadeeq commented at 4:01 pm on January 16, 2025:
    Done
  106. in src/node/types.h:50 in 131eb4e630 outdated 
    45+    /*
46+     * This is a sane minimum value for block_reserved_weight.
47+     * Reserving weight for coinbase transaction and block header
48+     * below this value will be prevented.
49+     */
50+    const uint32_t minimum_block_reserved_weight{2000};
    Sjors commented at 10:04 am on January 15, 2025:

    131eb4e630f958f1b5330bb9d170716c33655880: this should be a constant, not part of BlockCreateOptions.

    E.g. MINIMUM_BLOCK_RESERVED_WEIGHT{2000} defined next to DEFAULT_BLOCK_MAX_WEIGHT.

    ismaelsadeeq commented at 4:01 pm on January 16, 2025:
    Done
  107. in src/node/types.h:47 in 131eb4e630 outdated 
    40@@ -39,6 +41,13 @@ struct BlockCreateOptions {
41      * larger CompactSize encoded lengths.
42      */
43     size_t block_reserved_weight{8000};
44+
45+    /*
46+     * This is a sane minimum value for block_reserved_weight.
47+     * Reserving weight for coinbase transaction and block header
    Sjors commented at 10:08 am on January 15, 2025:
    I would leave out this sentence, or say “Setting a lower value is prevented at startup.”
    ismaelsadeeq commented at 4:02 pm on January 16, 2025:
    Done
  108. in src/node/types.h:46 in 131eb4e630 outdated 
    40@@ -39,6 +41,13 @@ struct BlockCreateOptions {
41      * larger CompactSize encoded lengths.
42      */
43     size_t block_reserved_weight{8000};
44+
45+    /*
46+     * This is a sane minimum value for block_reserved_weight.
    Sjors commented at 10:12 am on January 15, 2025:

    Maybe add:

    This accounts for the block header, var_int encoding of the transaction count and a minimally viable coinbase transaction. It adds an additional safety margin, because even with a thorough understanding of block serialization, it’s easy to make a costly mistake when trying to squeeze every last byte.

    ismaelsadeeq commented at 4:02 pm on January 16, 2025:
    Taken, thanks
  109. in doc/release-notes-31384.md:14 in ad1bc03245 outdated 
     9+ The mining code effectively added another `4,000` weight units (WU) to this reservation, leading to a template
10+ size of `3,992,000`.
11+
12+- The fix consolidates the reservation into a single place and introduces a new startup option,
13+  `-blockreservedweight` (default: `8,000` weight units). This option specifies the weight units
14+ reserved for the coinbase transaction. The default value of `-blockreservedweight` was chosen
    Sjors commented at 10:16 am on January 15, 2025:
    ad1bc03245181b00a25ea0182373eddae1c151e1: the header and
    ismaelsadeeq commented at 4:02 pm on January 16, 2025:
    Done
  110. Sjors commented at 10:16 am on January 15, 2025: member
    Code review ad1bc03245181b00a25ea0182373eddae1c151e1, mostly happy.
  111. 0xB10C commented at 2:13 pm on January 15, 2025: contributor

    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.

  112. wincss commented at 4:04 pm on January 15, 2025: none

    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.

  113. Sjors commented at 4:30 pm on January 15, 2025: member
    @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.
  114. miner: bugfix: fix duplicate weight reservation in block assembler 
    - This commit renamed coinbase_max_additional_weight to block_reserved_weight.

- Also clarify that the reservation is for block header, transaction count
  and coinbase transaction.
    0e8d06067c
  115. test: add `-blockmaxweight` startup option functional test 147bd093e3
  116. init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` 109c47abb0
  117. doc: rpc: improve `getmininginfo` help text 
    - The reserved weight of the coinbase transaction is an estimate and
  may not reflect the exact value; it can be lower.

- It should be clear that `currentblockweight` includes the reserved coinbase transaction weight.
  whereas `currentblocktx` does not account for the coinbase transaction count.

- Also clarify `m_last_block_num_txs` and `m_last_block_weight`
    baecdb9ee8
  118. miner: init: add `-blockreservedweight` startup option 
    - Prevent setting the value of `-blockreservedweight` below
  a safety value of 2000.
    01bdc27f43
  119. doc: add release notes fd3103142d
  120. ismaelsadeeq force-pushed on Jan 16, 2025
  121. ismaelsadeeq commented at 4:04 pm on January 16, 2025: member

    Forced pushed from ad1bc03245181b00a25ea0182373eddae1c151e1 to fd3103142de8bcb92b5db6a6501c4c66abc12abd see diff

    Changes

    1. Addressed @Sjors comments
  122. Sjors commented at 11:10 am on January 20, 2025: member
    ACK fd3103142de8bcb92b5db6a6501c4c66abc12abd
  123. DrahtBot requested review from 0xB10C on Jan 20, 2025


ismaelsadeeq DrahtBot Sjors pinheadmz darosior TheCharlatan luke-jr 0xB10C wincss


pinheadmz TheCharlatan 0xB10C

Labels
Mining

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me