fix: avoid int8_t truncation of -parbls before clamping#7454
Conversation
The bls_threads lambda assigned args.GetIntArg("-parbls", ...) (int64_t)
into an int8_t before adding GetNumCores() and clamping. A large -parbls
value such as 200 truncates/wraps to a negative int8_t, wrongly taking the
"leave that many cores free" branch, and threads += GetNumCores() can
overflow int8_t. Use an int intermediate and cast to int8_t only after the
final clamp to [0, MAX_BLSCHECK_THREADS], matching the sibling -par handling.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit 7e35ad8) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ce02b3e0f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /*dash_dbs_in_memory=*/false, | ||
| /*bls_threads=*/[&args]() -> int8_t { | ||
| int8_t threads = args.GetIntArg("-parbls", llmq::DEFAULT_BLSCHECK_THREADS); | ||
| int threads = args.GetIntArg("-parbls", llmq::DEFAULT_BLSCHECK_THREADS); |
There was a problem hiding this comment.
When -parbls is set above INT_MAX, GetIntArg() has already parsed a valid int64_t, but this assignment narrows it to int before the clamp. On normal 32-bit-int builds a value such as -parbls=2147483681 becomes negative, takes the auto/leave-cores-free path, and ends up clamped near 0 instead of to MAX_BLSCHECK_THREADS, so the truncation bug still exists for larger inputs. Keeping threads as int64_t through the adjustment/clamp avoids that remaining wraparound before the final int8_t cast.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7e35ad8. The value now stays in int64_t through adjustment and clamping; the clamp is applied before subtracting one so INT64_MIN cannot underflow either. A full dashd build passed, and regtest runs with -parbls=2147483681 and INT64_MAX both reported the configured maximum of 33 threads.
Walkthrough
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
At exact head 7e35ad8, the -parbls lambda correctly retains int64_t through GetIntArg, adjustment, and clamping, narrowing to int8_t only after the result is proven within [0,33]. The prior truncation blocker is fixed. Two suggestions remain: add regression test coverage for large -parbls values, and squash the two-commit stack since the second commit only widens an intermediate type that never independently shipped.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/init.cpp`:
- [SUGGESTION] src/init.cpp:2066-2077: Add regression coverage for large -parbls values
No automated test exercises the boundary behavior this lambda implements — a repo-wide search for `parbls` only turns up src/init.cpp, src/llmq/options.h, and the man pages, no test file. A functional startup test (or a unit test that extracts the clamp logic) should cover at least `-parbls=200`, `-parbls=2147483648` (INT32_MAX+1), and `-parbls=9223372036854775807` (INT64_MAX), asserting the effective BLS thread count is clamped to 33.
- [SUGGESTION] src/init.cpp:2066-2077: Squash 7e35ad8b311 into 6ce02b3e0f0 — the follow-up only widens an intermediate that never shipped
6ce02b3 changed the lambda's intermediate from int8_t to a plain int to fix the original truncation bug, but int (32-bit) is still narrower than the int64_t range GetIntArg can return for -parbls. 7e35ad8b311 immediately re-widens the identical code path to int64_t because int was still insufficient. Since neither intermediate state was ever released, this is one logical fix split into two commits — squash them into a single commit so the permanent history reflects only the correct final implementation.
| /*bls_threads=*/[&args]() -> int8_t { | ||
| int8_t threads = args.GetIntArg("-parbls", llmq::DEFAULT_BLSCHECK_THREADS); | ||
| int64_t threads = args.GetIntArg("-parbls", llmq::DEFAULT_BLSCHECK_THREADS); | ||
| if (threads <= 0) { | ||
| // -parbls=0 means autodetect (number of cores - 1 validator threads) | ||
| // -parbls=-n means "leave n cores free" (number of cores - n - 1 validator threads) | ||
| threads += GetNumCores(); | ||
| } | ||
| // Subtract 1 because the main thread counts towards the par threads | ||
| return std::clamp<int8_t>(threads - 1, 0, llmq::MAX_BLSCHECK_THREADS); | ||
| const int64_t adjusted_threads = std::clamp<int64_t>( | ||
| threads, 1, int64_t{llmq::MAX_BLSCHECK_THREADS} + 1) - 1; | ||
| return static_cast<int8_t>(adjusted_threads); | ||
| }(), |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for large -parbls values
No automated test exercises the boundary behavior this lambda implements — a repo-wide search for parbls only turns up src/init.cpp, src/llmq/options.h, and the man pages, no test file. A functional startup test (or a unit test that extracts the clamp logic) should cover at least -parbls=200, -parbls=2147483648 (INT32_MAX+1), and -parbls=9223372036854775807 (INT64_MAX), asserting the effective BLS thread count is clamped to 33.
source: ['codex']
| /*bls_threads=*/[&args]() -> int8_t { | ||
| int8_t threads = args.GetIntArg("-parbls", llmq::DEFAULT_BLSCHECK_THREADS); | ||
| int64_t threads = args.GetIntArg("-parbls", llmq::DEFAULT_BLSCHECK_THREADS); | ||
| if (threads <= 0) { | ||
| // -parbls=0 means autodetect (number of cores - 1 validator threads) | ||
| // -parbls=-n means "leave n cores free" (number of cores - n - 1 validator threads) | ||
| threads += GetNumCores(); | ||
| } | ||
| // Subtract 1 because the main thread counts towards the par threads | ||
| return std::clamp<int8_t>(threads - 1, 0, llmq::MAX_BLSCHECK_THREADS); | ||
| const int64_t adjusted_threads = std::clamp<int64_t>( | ||
| threads, 1, int64_t{llmq::MAX_BLSCHECK_THREADS} + 1) - 1; | ||
| return static_cast<int8_t>(adjusted_threads); | ||
| }(), |
There was a problem hiding this comment.
🟡 Suggestion: Squash 7e35ad8 into 6ce02b3 — the follow-up only widens an intermediate that never shipped
6ce02b3 changed the lambda's intermediate from int8_t to a plain int to fix the original truncation bug, but int (32-bit) is still narrower than the int64_t range GetIntArg can return for -parbls. 7e35ad8 immediately re-widens the identical code path to int64_t because int was still insufficient. Since neither intermediate state was ever released, this is one logical fix split into two commits — squash them into a single commit so the permanent history reflects only the correct final implementation.
source: ['claude', 'codex']
Issue being fixed or feature implemented
-parblsis parsed as anint64_t, but it was narrowed toint8_tbefore the value was adjusted and clamped. Values above 127, such as-parbls=200, could therefore wrap negative and take the "leave cores free" path instead of clamping to the configured maximum.What was done?
Keep the BLS verification thread count in
int64_twhile applying the autodetection adjustment and clamp. Clamp the pre-subtraction value to[1, MAX_BLSCHECK_THREADS + 1], then subtract one and cast toint8_t. This preserves the intended[0, MAX_BLSCHECK_THREADS]result across the full acceptedint64_targument range without signed underflow.How Has This Been Tested?
src/dashdandsrc/dash-clion macOS arm64.-parbls=2147483681and-parbls=9223372036854775807; both reported 33 BLS verification threads, the configured maximum.git diff --check.Breaking Changes
None.
Checklist: