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

pull ismaelsadeeq wants to merge 4 commits into bitcoin:master from ismaelsadeeq:11-2024-fix-duplicate-coinbase-reservation-bug changing 8 files +126 −11
  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.

    At a minimum, if we don’t want to update this, we should reserve 8,000 weight units in src/policy/policy.h so it will be clear that it is Bitcoin Core policy to reserve 8,000 weight units for the coinbase transaction.

  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 pinheadmz, Sjors

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  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:71 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.
  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. miner: bugfix: remove duplicate space reservation for coinbase tx e1114bcc0d
  23. ismaelsadeeq force-pushed on Dec 4, 2024
  24. test: add `-blockmaxweight` startup option functional test b52aad748e
  25. init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` c638b4fb66
  26. doc: add release notes f7a2f125c7
  27. ismaelsadeeq force-pushed on Dec 4, 2024
  28. DrahtBot removed the label Needs rebase on Dec 5, 2024
  29. 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.

  30. in doc/release-notes-31384.md:5 in f7a2f125c7 
    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.
  31. 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.

  32. in src/node/miner.cpp:74 in f7a2f125c7 
    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
  33. pinheadmz approved
  34. 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

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

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

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

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


ismaelsadeeq DrahtBot Sjors pinheadmz darosior

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: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me