Duplicate coinbase transaction space reservation in CreateNewBlock #21950

1. 
   ariard commented at 7:07 pm on May 14, 2021: contributor

   In BlockAssembler::CreateNewBlock, we initiate nBlockWeight to 4000 WU, reserving space for coinbase transaction.

   Latter on, while iterating on transactions for block inclusion in addPackagesTxs, we verify that candidate package doesn’t overflow the max block weight. This check is done in TestPackage against nBlockMaxWeight. However, default value is DEFAULT_BLOCK_MAX_WEIGHT which is equal to MAX_BLOCK_WEIGHT - 4000 (L20 in src/policy/policy.cpp). So block weight assembled is starting at 4000 WU and can’t overflow 3996000 WU.

   Do we have duplicated coinbase transaction space reservation ? I can see this a safety margin to avoid creating invalid blocks if you create inflated coinbase transaction, at the price of few weight units leftover.

   If so, maybe we should leave it as it is but document this behavior, I was surprised hitting this while scratching unrelated tests.

   See comment in #11100

2. ariard added the label Bug on May 14, 2021
3. ariard renamed this:
   Duplicate coinbase transaction reservation in CreateNewBlock
   Duplicate coinbase transaction space reservation in CreateNewBlock
   on May 14, 2021
4. pinheadmz assigned pinheadmz on Jun 3, 2023
5. 
   pinheadmz commented at 7:13 pm on June 5, 2023: member

   This is actually pretty interesting: I quickly looked through the weight of the last 50-60 blocks and F2Pool is the ONLY pool generating blocks with total weight higher greater than 3,996,000. I started writing a unit test for this, it seems like that double coinbase reservation may actually put miners who rely on bitcoin core template generation at a disadvantage.

   And actually it looks like ALL of F2Pool’s recent blocks are actually > 3,996,000 in weight. They may be the only pool using their own software to create block templates?

6. 
   0xB10C commented at 11:58 pm on June 5, 2023: contributor

   And actually it looks like ALL of F2Pool’s recent blocks are actually > 3,996,000 in weight. They may be the only pool using their own software to create block templates?

   I’m fairly certain they use Bitcoin Core. You can control the max block weight with -blockmaxweight (default is 3996000 WU). Also, F2Pool is known for running with custom patches on top of Bitcoin Core to maximize fees: e.g. they reduced the number of sigops reserved for the coinbase. This bit them twice a few months ago due to not keeping that patch up-to-date.

   End of last year, Poolin also mined blocks with more than the default max block weight. See for example 762816. The EclipseMC pool mined a few, for example, 752015, the Huobi Pool with e.g. 710871, and the KuCoin Pool too with, for example, 751909.

7. 
   pinheadmz commented at 0:36 am on June 6, 2023: member

   @0xB10C thanks for these great insights. What do you think about the issue? The 4000 is reserved in two places:

   https://github.com/bitcoin/bitcoin/blob/f4a8269dfc144cc918570bdb870aa5143a11c1fe/src/node/miner.cpp#L97

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

   Should we remove one of these?

8. 
   0xB10C commented at 11:26 am on June 6, 2023: contributor

   I agree that reserving 4000 WU twice is confusing. However, I’m not familiar enough with the code to judge on which to remove.
9. 
   Sjors commented at 1:52 pm on November 29, 2024: member

   TIL about this, despite having authored #30356.

   I think -blockmaxweight should refer to the actual worst case block, i.e. assuming the pool uses all 4000 WU. So its default should be equal to MAX_BLOCK_WEIGHT. It’s documentation should clarify this.

   We do need to make sure that if a miner has been setting -blockmaxweight=4000000 manually, nothing breaks.

   An additional argument for having -blockmaxweight not adjust for the reservation, is that in Stratum v2 each connected Template Provider (i.e. a pool or an individual miner making its own templates) can specify how much space to reserve. So 4000 only applies to getblocktemplate users, whereas -blockmaxweight applies everywhere.

   It would be good to have a functional test for this.

   Some history:

   MAX_BLOCK_COST was initially introduced for a setting called -blockmaxcost as part of the main SegWit pull request #8149. It existed alongside -blockmaxsize and was effectively a way to limit the amount of witness data.

   At the time DEFAULT_BLOCK_MAX_SIZE was 750000, i.e. 3/4th of the maximum size. So DEFAULT_BLOCK_MAX_WEIGHT was set to 3 million, 3/4th of the maximum weight.

   Later on the combination of #8363 and #11100 replaced the confusing MAX_BLOCK_COST, -blockmaxsize and -blockmaxcost by just DEFAULT_BLOCK_MAX_WEIGHT and -blockmaxweight. That first PR kept the 3/4th of the maximum default.

   The second PR #11100 changed the default to 4 million. It also introduced the 4000 adjustment. Unfortunately increasing the default caused a bunch of arguing, which probably didn’t aid in review.

   However the mistake was caught in review, but considered not to be a big deal: #11100 (review) (PR description also links to this comment)

   I do think we should fix this, because the alternative is that people start patching this stuff themselves and make very expensive mistakes.

Contributors
ariard pinheadmz 0xB10C Sjors

Labels
Bug