Configuration argument handling improvements. #36861

jlopp commented at 5:54 PM on November 13, 2026: contributor

Several numeric configuration options now reject values that are out of range instead of silently wrapping or inverting. Previously a negative value could disable the intended limit entirely.

-dbbatchsize and -dbcrashratio must be non-negative. A negative -dbbatchsize wrapped to a huge value and disabled batch flushing, so a reindex could hold an unbounded batch in memory.
-limitclustersize, -limitancestorcount, -limitdescendantcount, -mempoolexpiry, -datacarriersize, -maxsendbuffer, -maxreceivebuffer, -bantime, -bytespersigop and -maxtipage must be non-negative.
-limitclustercount must be between 1 and 64.
-timeout must be positive. It previously fell back to the default silently, unlike the analogous -peertimeout, which already errored.

Zero remains valid wherever it already meant "disabled": -dbbatchsize=0, -limitclustersize=0, -datacarriersize=0, -mempoolexpiry=0, -maxtipage=0, -bytespersigop=0, -maxsendbuffer=0, -maxreceivebuffer=0 and -bantime=0.

The -help output for -bytespersigop, -datacarriersize, -dbbatchsize, -stopatheight and -checkblockindex now shows the =<n> placeholder for their value, matching how they are actually parsed.

DrahtBot commented at 5:54 PM on November 13, 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/36861
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36861.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36861#author-ralph

Reviews

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

Type	Reviewers
Concept ACK	l0rinc, ryanofsky
Approach NACK	sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

l0rinc commented at 6:55 PM on November 13, 2026: contributor

Concept ACK

There are lots of AI findings about these potential misconfigurations, so it would be good to close off as many of them as possible in one go.

A few closely related cases seem worth covering as well:

-timeout values above INT_MAX are narrowed before the new positivity check (similar ones were already attempted here and closed).
Very large -mempoolexpiry values can overflow when converted from hours to seconds.
A negative -txconfirmtarget wraps into the wallet's unsigned target and can make fee estimation fall back to -fallbackfee.
Negating an integer option is documented as setting it to 0, so -nolimitclustersize and -nobantime are equivalent to the zero values this PR explicitly accepts. -limitclustersize=0 makes every transaction oversized and prevents normal mempool admission, while -bantime=0 makes setban without an explicit duration create an immediately expired ban.

CI seems to fail because the new test loop binds a std::string reference to temporary strings.

DrahtBot added the label CI failed on Nov 13, 2026

DrahtBot commented at 6:58 PM on November 13, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task i686: https://github.com/bitcoin/bitcoin/actions/runs/43477395661/job/132064683071</sub> <sub>LLM reason (✨ experimental): The build failed because mempool_tests.cpp triggers a range-loop warning treated as an error: const std::string& binds to temporary strings.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

sedited commented at 7:56 PM on November 13, 2026: contributor

See #36763

But I think I'm Approach NACK at yet another piecemeal approach to a solution for this. This should be solved by the args handler itself, not with many different bug prone special cases.

Configuration argument handling improvements.

- Several numeric configuration options now reject values that are out of
  range instead of silently wrapping or inverting. Previously a negative value
  could disable the intended limit entirely.

  - `-dbbatchsize` and `-dbcrashratio` must be non-negative. A negative
    `-dbbatchsize` wrapped to a huge value and disabled batch flushing, so a
    reindex could hold an unbounded batch in memory.
  - `-limitclustersize`, `-limitancestorcount`, `-limitdescendantcount`,
    `-mempoolexpiry`, `-datacarriersize`, `-maxsendbuffer`, `-maxreceivebuffer`,
    `-bantime`, `-bytespersigop` and `-maxtipage` must be non-negative.
  - `-limitclustercount` must be between 1 and 64.
  - `-timeout` must be positive. It previously fell back to the default
    silently, unlike the analogous `-peertimeout`, which already errored.

  Zero remains valid wherever it already meant "disabled": `-dbbatchsize=0`,
  `-limitclustersize=0`, `-datacarriersize=0`, `-mempoolexpiry=0`,
  `-maxtipage=0`, `-bytespersigop=0`, `-maxsendbuffer=0`,
  `-maxreceivebuffer=0` and `-bantime=0`.

- The `-help` output for `-bytespersigop`, `-datacarriersize`, `-dbbatchsize`,
  `-stopatheight` and `-checkblockindex` now shows the `=<n>` placeholder for
  their value, matching how they are actually parsed.

1ab3f14858

jlopp force-pushed on Nov 13, 2026

ryanofsky commented at 10:56 PM on November 13, 2026: contributor

Concept ACK 1ab3f148582c21054dfc9323623edcf9768949f6 for parsing improvements here, but I'd recommend against increased use of the DISALLOW_NEGATION flag:

If your option can't accept zero values, DISALLOW_NEGATION is only a partial fix and may allow zeroes to slip through because you are only guarding against them being set ib a single path. Better to check for valid values one place instead of multiple.
It creates inconsistent UX where -option=0 and -nooption have different behaviors without a clear reason, when normal behavior is for them to behave the same.

DISALLOW_NEGATION was intended to used rarely and eventually removed (https://github.com/bitcoin/bitcoin/pull/22766#issuecomment-944924866) and this PR seems to increase usage by a lot. The other changes here look very good though.