init: refuse -bytespersigop values that overflow #36538

fametrano commented at 7:13 AM on October 15, 2026: none

-bytespersigop is parsed as int64, stored as unsigned int, and multiplied by a transaction's sigop cost into the int32 sizes used by the mempool. Negative values wrap, values of 2^32 or more are truncated (4294967296 turns the sigop adjustment off), and large values overflow, so a debug build aborts on an Assume. The first commit refuses any value outside [0, 21474] at startup: up to 21474, a package of 25 transactions at the standard sigops limit still has an int32 size.

The second commit checks the sigops limit before a transaction is staged. A transaction that is otherwise standard can be far over that limit, and at 21474 one with about 100,000 sigops still overflows when staged. mempool_sigoplimit.py covers the range, a 25-transaction package at 21474, and that transaction.

DrahtBot added the label Validation on Oct 15, 2026

DrahtBot commented at 7:13 AM on October 15, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36538
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36538.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36538#author-ralph

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

maflcko commented at 7:29 AM on October 15, 2026: member

With a large -bytespersigop, its sigop-adjusted weight does not fit in the int32_t

Not sure this is a valid use-case, but the parsing is likely broken in this case anyway on this pull and master: It is parsed as i64, then assigned to u32, then used in a i32 calculation. Every step either truncates, wraps, or aborts. I'd say if this is fixed, it would be good to fix all of them.

fametrano commented at 9:14 PM on October 15, 2026: none

Agreed, fixed in 7d37a758b3: -bytespersigop outside [0, 21474] is now refused at startup, so it is stored without wrapping or truncation. 21474 is the largest value at which a package of 25 transactions at the standard sigops limit keeps an int32 size. A bound of 134217 would cover one transaction, but a package of 5 then aborts a debug build. I kept the sigops check before staging as a second commit, because a single transaction far over the limit still overflows at 21474.

DrahtBot added the label CI failed on Oct 15, 2026

fametrano commented at 6:09 AM on October 16, 2026: none

The "Windows native, VS" failure is the intermittent p2p_initial_headers_sync.py issue that #36545 fixes, so I think it is unrelated to this change.

init: refuse -bytespersigop values that overflow

-bytespersigop is parsed as int64_t and stored in an unsigned int, so
a negative value wraps and a value of 2^32 or more is truncated: -1
becomes 4294967295, and 4294967296 becomes 0, which silently turns off
the sigop adjustment.

The stored value is then multiplied by the sigop cost of a transaction
into sigop-adjusted sizes, and these are narrowed to int32_t: the weight
used by TxGraph, and the total vsize of a package in CheckFeeRate. A
standard transaction has a sigop cost of at most
MAX_STANDARD_TX_SIGOPS_COST (16000), so its vsize is at most
16000 * bytespersigop / WITNESS_SCALE_FACTOR (4), and a package holds at
most MAX_PACKAGE_COUNT (25) transactions. The package total fits in
int32_t while 25 * 4000 * bytespersigop <= INT32_MAX (2147483647), that
is up to 21474: 100000 * 21474 = 2147400000, while
100000 * 21475 = 2147500000 does not fit. Above that, a package of 25
transactions at the limit aborts a debug build on
Assume(virtual_bytes >= 0) in CFeeRate::GetFee; above 134217, so does a
single transaction, on Assume(feerate.size > 0) in
TxGraphImpl::AddTransaction.

Refuse at startup any value outside [0, 21474], before it is stored.

dc12f31518

validation: check sigops limit before staging

PreChecks stages a transaction in the mempool changeset before it
checks MAX_STANDARD_TX_SIGOPS_COST. The bound on -bytespersigop keeps
sigop-adjusted sizes in int32_t only for transactions within that
limit. A transaction that is otherwise standard can be far above it:
two P2WSH inputs with 50020 sigops each are 1365 vbytes. At
-bytespersigop=21474, 100040 * 21474 = 2148258960 exceeds INT32_MAX,
and staging it fails Assume(feerate.size > 0) in
TxGraphImpl::AddTransaction, which aborts a debug build.

Check the limit right after counting the sigops, before staging.

33b6803c40

DrahtBot removed the label CI failed on Oct 19, 2026

sedited commented at 11:05 AM on October 26, 2026: contributor

This still seems very heavy, while being a very incremental change to a single argument. I think if we go down this route further, we should do it comprehensively for all args that require a sane range of inputs. Closing this for now.

sedited closed this on Oct 26, 2026