Bugfix: Miner: Don't reuse block_reserved_weight for "block is full enough to give up" weight delta

luke-jr commented at 7:45 am on April 26, 2025: member

PR #30356 incorrectly changed a constant of 4000 to m_options.coinbase_max_additional_weight in the check for when to give up finding another transaction to fill the block:

0             if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
1-                    m_options.nBlockMaxWeight - 4000) {
2+                    m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
3                 // Give up if we're close to full and haven't succeeded in a while
4                 break;
5             }

But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after MAX_CONSECUTIVE_FAILURES failed attempts.

It doesn’t seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.

Bugfix: Miner: Don't reuse block_reserved_weight for "block is full enough to give up" weight delta

PR #30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block:

```diff
             if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
-                    m_options.nBlockMaxWeight - 4000) {
+                    m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
                 // Give up if we're close to full and haven't succeeded in a while
                 break;
             }
```

But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts.

It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.

524f981bb8

DrahtBot commented at 7:45 am on April 26, 2025: 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/32355.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	darosior, ismaelsadeeq, achow101

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

darosior approved

darosior commented at 4:41 pm on April 26, 2025: member

utACK 524f981bb87319fdd6ff2ab4a932c4b4e31a7398

Makes sense and code looks good to me.

ismaelsadeeq approved

ismaelsadeeq commented at 12:29 pm on April 27, 2025: member

ACK 524f981bb87319fdd6ff2ab4a932c4b4e31a7398

Using the m_options.block_reserved_weight won’t cause issues, but generally, I think it’s best to separate concerns and make this a constexpr.

This also makes the mining code cleaner.

achow101 commented at 10:40 pm on April 29, 2025: member

ACK 524f981bb87319fdd6ff2ab4a932c4b4e31a7398

achow101 merged this on Apr 29, 2025

achow101 closed this on Apr 29, 2025

Sjors commented at 10:24 am on May 2, 2025: member

Concept ACK

If it wasn’t for the hardcoded minimum -blockreservedweight=2000, then if the user set it to zero this loop might never exit? In any case, it’s indeed an unrelated concept so deserves its own constant.

Bugfix: Miner: Don’t reuse block_reserved_weight for “block is full enough to give up” weight delta #32355

Code Coverage & Benchmarks

Reviews