-
Notifications
You must be signed in to change notification settings - Fork 37.9k
bugfix: miner: fix addPackageTxs
unsigned integer overflow
#33475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bugfix: miner: fix addPackageTxs
unsigned integer overflow
#33475
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33475. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, utACK b807dfc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code ACK b807dfc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
Github-Pull: bitcoin#33475 Rebased-From: b807dfc
Github-Pull: bitcoin#33475 Rebased-From: b807dfc
Backported to 30.x in #33473. |
This PR fixes an unsigned integer overflow in the
addPackageTxs
method of theBlockAssembler
.The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.
i.e, by starting with
-blockmaxweight=2000
,-blockreservedweight=2000
, or justblockmaxweight=2000
, and then calling the mining interfacecreateNewBlock
withblockReservedWeight
set to2000
.Instead of bailing out after going through transactions equivalent to
MAX_CONSECUTIVE_FAILURES
, the loop never breaks until all mempool transactions are visited.See #33421 (comment)
The fix avoids the overflow by using addition instead adding
BLOCK_FULL_ENOUGH_WEIGHT_DELTA
to the block weight and comparing it withm_options.nBlockMaxWeight
.Another alternative that preserves the same structure is to use
static_cast
. See c9530cf.This fix can be tested by cherry-picking the commits from #33421 without the static cast fix and running:
This is part of a larger inconsistency in how size/weight is represented in the codebase. It may be worth defining a dedicated type for size/weight.