mining: include chunks that reach block limits #36156

Problem: BlockAssembler fills block templates with mempool chunks that fit the configured weight limit and the consensus sigops limit, but currently rejects a chunk when it brings either total exactly to its limit. Equality is valid because block_max_weight is a maximum and BIP 141 defines both consensus limits using ≤. The >= checks can therefore leave valid capacity unused and omit a fee-paying chunk. Mining option validation also permits block_max_weight == MAX_BLOCK_WEIGHT and coinbase_output_max_additional_sigops == MAX_BLOCK_SIGOPS_COST, while the chunk checks reject an accounted total equal to either limit. A 2016 review comment on the original package-selection PR raised the same inclusive-limit point.

Fix: Use strict greater-than comparisons so exact-limit chunks are included. Extend the existing chunk-limit test to cover exact fits and one-unit overages for both weight and sigops. Clarify how mining software reserves weight and sigops for the completed block.

This addresses the first issue in #35596.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

This seems backwards to me: the risk of a mistake here is that the block becomes invalid losing all possible reward from mining it, while the benefit of success is perhaps a single larger / additional transaction at the bottom of the block. Currently the bottom transaction in a block seems to pay less than 1000sats in total fees, so generously, that's less than 80c per block, as compared to the subsidy of around $240k per block.

Concept ACK

Looks correct to me.

This seems backwards to me: the risk of a mistake here is that the block becomes invalid losing all possible reward from mining it, while the benefit of success is perhaps a single larger / additional transaction at the bottom of the block.

This fix seems straightforward and correct, with good tests. Will the mistake come from the individual chunks sigops or weight accounting error? If indeed we do have that error, we still could end up creating an invalid block even without this fix.

If we are confident that our accounting of weight and sigops is correct without this PR, and that the block assembler will not create an invalid block, then fixing this issue is okay.

Currently the bottom transaction in a block seems to pay less than 1000sats in total fees, so generously, that's less than 80c per block, as compared to the subsidy of around $240k per block.

I think it's less about the revenue but more about making the block assembler code correct and reflect consensus allowed limits, and making it explicit that filled blocks may be created, are valid and will be accepted as part of the blockchain.

For me the more risky part is when we allow this minor bugs in the code, it incentivise pool companies to patch the code themselves to fix it, fwiw F2Pool did fix these bugs and have their custom binaries which create filled blocks.

For me the more risky part is when we allow this minor bugs in the code, it incentivise pool companies to patch the code themselves to fix it, fwiw F2Pool did fix these bugs and have their custom binaries which create filled blocks.

The issue you're referencing was in regards to 500vb of wasted space, versus this PR which is a delta of 1vb of space. The block you're referring to has a 2127 WU unused:

$ bitcoin-cli getblock 00000000000000000001eac8304428ea2fc5fc56e9ffefec848efde3c5a4f279 | jq 4e6-.weight
2127

It's certainly bad if mining pools introduce bugs in their software that invalidate blocks while trying to get to the very edge of valid behaviour, but at least it only impacts a single pool that way. I don't see why we would want to play the same stupid game and risk winning the same stupid prize. Off by one errors are common; removing a defensive buffer against that might be appropriate if there was a significant win to be had, but there isn't here.

Anyway, I've said my piece, I'll unsubscribe.

While looking into why the current check uses >=, I found a comment on the original package checks also suggesting >. Given that mining options already allow a block_max_weight of MAX_BLOCK_WEIGHT and a coinbase sigops reservation of MAX_BLOCK_SIGOPS_COST, I think accepting equality in chunk selection too would make these limits more consistent.

I understand the concern about risking an invalid block for such a small gain, so I added test cases one unit over each limit and clarified the existing reservations in the docs. I’d like to hear how others weigh that against the risk.