fix: resolve signed integer overflow UB in CoinJoin priority and timeout by thepastaclaw · Pull Request #7236 · dashpay/dash

thepastaclaw · 2026-03-18T21:50:07Z

Summary

Fix two signed integer overflow UB issues in CoinJoin code, found during fuzz testing.

`CalculateAmountPriority` (common.h)

The return type is int but the computation -(nInputAmount / COIN) operates on
int64_t values. When nInputAmount is extremely large (e.g. near MAX_MONEY),
the result exceeds INT_MAX and the implicit narrowing to int is undefined
behavior under UBSan.

Fix: Clamp the int64_t result to [INT_MIN, INT_MAX] before returning.
This preserves the existing sort ordering for all realistic inputs while making
extreme values well-defined.

`IsTimeOutOfBounds` (coinjoin.cpp)

The expression current_time - nTime overflows when the two int64_t values
differ by more than INT64_MAX (e.g. one large positive, one large negative).

Fix: Compute the absolute difference using unsigned arithmetic, which is
well-defined for all inputs.

Validation

Both functions are non-consensus (CoinJoin sort priority and queue timeout only)
Neither overflow is exploitable — CoinJoin queue entries require valid MN signatures,
and the priority function only affects local sort order
The fixes preserve identical behavior for all realistic inputs
Found via UBSan-instrumented fuzz testing on the ci/fuzz-regression branch

thepastaclaw · 2026-03-18T21:50:15Z

@coderabbitai review

coderabbitai · 2026-03-18T21:50:22Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

github-actions · 2026-03-18T21:50:37Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2026-03-18T21:55:01Z

Walkthrough

The pull request contains two safety and optimization improvements to the coinjoin module. The first change simplifies timeout validation in IsTimeOutOfBounds by replacing a two-branch conditional check with a single unsigned difference computation that checks if the absolute difference between timestamps exceeds the threshold. The second change adds overflow prevention to CalculateAmountPriority by using std::clamp to constrain the computed value within valid integer range before casting. Both changes are localized modifications with no alterations to function signatures or public APIs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main change: fixing signed integer overflow undefined behavior in CoinJoin priority and timeout functions.
Description check	✅ Passed	The description is directly related to the changeset, providing detailed context about the two UB fixes, their causes, solutions, and validation rationale.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

📝 Coding Plan

Generate coding plan for human review comments

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/coinjoin/coinjoin.cpp`:
- Around line 57-61: Run clang-format on the indicated block (or the whole file)
to fix the whitespace/line-wrapping so the ternary expression and return
statement match project style; reformat the block containing current_time, nTime
and COINJOIN_QUEUE_TIMEOUT (the diff calculation and return) using your
project's clang-format config and commit the changes so the Clang Diff Format
Check passes.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 82b1eff3-1f1e-4510-a19f-8c06e26a762b

📥 Commits

Reviewing files that changed from the base of the PR and between 1d212a1 and 4248612.

📒 Files selected for processing (2)

src/coinjoin/coinjoin.cpp
src/coinjoin/common.h

coderabbitai · 2026-03-18T21:55:04Z

src/coinjoin/coinjoin.cpp

 {
-    return current_time - nTime > COINJOIN_QUEUE_TIMEOUT ||
-           nTime - current_time > COINJOIN_QUEUE_TIMEOUT;
+    const uint64_t diff = (current_time > nTime)
+                              ? static_cast<uint64_t>(current_time) - static_cast<uint64_t>(nTime)
+                              : static_cast<uint64_t>(nTime) - static_cast<uint64_t>(current_time);
+    return diff > static_cast<uint64_t>(COINJOIN_QUEUE_TIMEOUT);


⚠️ Potential issue | 🟡 Minor

Run clang-format on this block to unblock CI.

The pipeline reports formatting differences at Line 57 in this function. Please apply clang-format to this region/file so the Clang Diff Format Check passes.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[error] 57-57: Clang format differences detected in this file. Please run clang-format to fix formatting issues as shown by the diff.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/coinjoin.cpp` around lines 57 - 61, Run clang-format on the indicated block (or the whole file) to fix the whitespace/line-wrapping so the ternary expression and return statement match project style; reformat the block containing current_time, nTime and COINJOIN_QUEUE_TIMEOUT (the diff calculation and return) using your project's clang-format config and commit the changes so the Clang Diff Format Check passes.

CalculateAmountPriority in common.h could overflow when assigning a negated int64_t division result to an int return type with extreme CAmount values. Clamp the result to INT_MIN/INT_MAX before returning. IsTimeOutOfBounds in coinjoin.cpp could overflow on signed subtraction when current_time and nTime differ by more than INT64_MAX. Use unsigned arithmetic to compute the absolute difference safely. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

thepastaclaw

Code Review

Clean, correct two-line fix for signed integer overflow UB in CoinJoin timeout and priority. Both arithmetic changes are mathematically sound and preserve original semantics. The only gap is the absence of regression tests exercising the extreme-value inputs that motivated these fixes.

Reviewed commit: ab4bea6

🟡 2 suggestion(s) | 💬 1 nitpick(s)

1 additional finding

💬 nitpick: Pre-existing: float-to-int implicit narrowing on denomination priority return

src/coinjoin/common.h (line 125)

Line 125 returns (float)COIN / *optDenom * 10000, which undergoes implicit float-to-int conversion. With current denominations the maximum result is ~10,000,000 (for the smallest denomination COIN/1000+1), safely within INT_MAX. But this is the same class of narrowing issue this PR fixes for the nondenom path. Not blocking since current values are safe, but worth noting for consistency.

🤖 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/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 60-62: No regression test for extreme timestamp values in IsTimeOutOfBounds
  The fix is correct: unsigned subtraction after a signed comparison produces the exact mathematical absolute difference for all int64_t pairs, including mixed-sign and extreme values. However, existing tests in coinjoin_queue_tests.cpp (deltas of ±15, ±60) and coinjoin_inouts_tests.cpp (delta of COINJOIN_QUEUE_TIMEOUT+1) only cover small time deltas around realistic timestamps. No test exercises the scenario that triggered the UBSan finding — e.g., current_time = INT64_MAX and nTime = INT64_MIN, or vice versa. A targeted test would prevent regressions and document the fix motivation.

In `src/coinjoin/common.h`:
- [SUGGESTION] lines 132-133: No regression test for CalculateAmountPriority clamp boundary
  The clamp is correct. Note that with valid money amounts, the old code would not have overflowed int: -(MAX_MONEY / COIN) = -21000000, well within INT_MIN..INT_MAX. The UB requires nInputAmount > INT_MAX * COIN (~2.1×10¹⁷), which exceeds MAX_MONEY but is representable in int64_t. Since CalculateAmountPriority accepts CAmount (int64_t) without a MoneyRange guard, fuzz testing can reach these values. There are no tests for this function at all — the function is constexpr, so a static_assert or compile-time test could verify the clamping directly.

thepastaclaw · 2026-03-20T02:52:02Z

src/coinjoin/coinjoin.cpp

+    const uint64_t diff = (current_time > nTime) ? static_cast<uint64_t>(current_time) - static_cast<uint64_t>(nTime)
+                                                 : static_cast<uint64_t>(nTime) - static_cast<uint64_t>(current_time);
+    return diff > static_cast<uint64_t>(COINJOIN_QUEUE_TIMEOUT);


🟡 Suggestion: No regression test for extreme timestamp values in IsTimeOutOfBounds

The fix is correct: unsigned subtraction after a signed comparison produces the exact mathematical absolute difference for all int64_t pairs, including mixed-sign and extreme values. However, existing tests in coinjoin_queue_tests.cpp (deltas of ±15, ±60) and coinjoin_inouts_tests.cpp (delta of COINJOIN_QUEUE_TIMEOUT+1) only cover small time deltas around realistic timestamps. No test exercises the scenario that triggered the UBSan finding — e.g., current_time = INT64_MAX and nTime = INT64_MIN, or vice versa. A targeted test would prevent regressions and document the fix motivation.

_{source: ['claude', 'codex']}

🤖 Fix this 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/coinjoin/coinjoin.cpp`: - [SUGGESTION] lines 60-62: No regression test for extreme timestamp values in IsTimeOutOfBounds The fix is correct: unsigned subtraction after a signed comparison produces the exact mathematical absolute difference for all int64_t pairs, including mixed-sign and extreme values. However, existing tests in coinjoin_queue_tests.cpp (deltas of ±15, ±60) and coinjoin_inouts_tests.cpp (delta of COINJOIN_QUEUE_TIMEOUT+1) only cover small time deltas around realistic timestamps. No test exercises the scenario that triggered the UBSan finding — e.g., current_time = INT64_MAX and nTime = INT64_MIN, or vice versa. A targeted test would prevent regressions and document the fix motivation.

thepastaclaw · 2026-03-20T02:52:02Z

src/coinjoin/common.h

+    const int64_t val = -(nInputAmount / COIN);
+    return int(std::clamp<int64_t>(val, std::numeric_limits<int>::min(), std::numeric_limits<int>::max()));


🟡 Suggestion: No regression test for CalculateAmountPriority clamp boundary

The clamp is correct. Note that with valid money amounts, the old code would not have overflowed int: -(MAX_MONEY / COIN) = -21000000, well within INT_MIN..INT_MAX. The UB requires nInputAmount > INT_MAX * COIN (~2.1×10¹⁷), which exceeds MAX_MONEY but is representable in int64_t. Since CalculateAmountPriority accepts CAmount (int64_t) without a MoneyRange guard, fuzz testing can reach these values. There are no tests for this function at all — the function is constexpr, so a static_assert or compile-time test could verify the clamping directly.

_{source: ['claude', 'codex']}

🤖 Fix this 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/coinjoin/common.h`: - [SUGGESTION] lines 132-133: No regression test for CalculateAmountPriority clamp boundary The clamp is correct. Note that with valid money amounts, the old code would not have overflowed int: -(MAX_MONEY / COIN) = -21000000, well within INT_MIN..INT_MAX. The UB requires nInputAmount > INT_MAX * COIN (~2.1×10¹⁷), which exceeds MAX_MONEY but is representable in int64_t. Since CalculateAmountPriority accepts CAmount (int64_t) without a MoneyRange guard, fuzz testing can reach these values. There are no tests for this function at all — the function is constexpr, so a static_assert or compile-time test could verify the clamping directly.

thepastaclaw

Code Review

Test-only follow-up commit adding comprehensive regression tests for both UB fixes. All prior review suggestions are now addressed: queue_timestamp_extreme_values covers INT64_MIN/INT64_MAX edge cases for IsTimeOutOfBounds, and calculate_amount_priority_clamp (plus two static_asserts) verifies the clamping boundary for CalculateAmountPriority. Tests are well-structured and exercise the exact scenarios that motivated the fixes.

Reviewed commit: 1053f37

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

thepastaclaw · 2026-03-20T09:20:11Z

Updated per knst's feedback: replaced magic numbers in tests with symbolic expressions (-(MAX_MONEY / COIN), -INT_MAX) so the expected values are self-documenting.

thepastaclaw

Incremental Review (push 4cc88eaf vs prior reviewed 1053f373)

Delta: 1 file changed — src/test/coinjoin_queue_tests.cpp (4 insertions, 5 deletions)

Test-only commit replacing hardcoded magic numbers with symbolic constants: -21000000 → -(MAX_MONEY / COIN), -2147483647 → -INT_MAX. Pure readability improvement, no behavioral change.

Prior review stands — no new findings. ✅

coderabbitai bot reviewed Mar 18, 2026

View reviewed changes

thepastaclaw force-pushed the fix-coinjoin-ub branch from 4248612 to dbfbbe6 Compare March 18, 2026 22:09

thepastaclaw force-pushed the fix-coinjoin-ub branch from cb601cd to ab4bea6 Compare March 19, 2026 21:34

thepastaclaw commented Mar 20, 2026

View reviewed changes

thepastaclaw force-pushed the fix-coinjoin-ub branch from 1053f37 to 4ef1992 Compare March 20, 2026 03:44

test: add regression tests for CoinJoin UB fixes

4cc88ea

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

thepastaclaw force-pushed the fix-coinjoin-ub branch from 4ef1992 to 4cc88ea Compare March 20, 2026 09:20

thepastaclaw commented Mar 20, 2026

View reviewed changes

ci: re-trigger after runner infrastructure failure

f969cc5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve signed integer overflow UB in CoinJoin priority and timeout#7236

fix: resolve signed integer overflow UB in CoinJoin priority and timeout#7236
thepastaclaw wants to merge 3 commits intodashpay:developfrom
thepastaclaw:fix-coinjoin-ub

thepastaclaw commented Mar 18, 2026

Uh oh!

thepastaclaw commented Mar 18, 2026

Uh oh!

coderabbitai bot commented Mar 18, 2026

Uh oh!

github-actions bot commented Mar 18, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Mar 18, 2026

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Mar 18, 2026

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Mar 20, 2026

Uh oh!

thepastaclaw Mar 20, 2026

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw commented Mar 20, 2026

Uh oh!

thepastaclaw left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		const int64_t val = -(nInputAmount / COIN);
		return int(std::clamp<int64_t>(val, std::numeric_limits<int>::min(), std::numeric_limits<int>::max()));

Conversation

thepastaclaw commented Mar 18, 2026

Summary

CalculateAmountPriority (common.h)

IsTimeOutOfBounds (coinjoin.cpp)

Validation

Uh oh!

thepastaclaw commented Mar 18, 2026

Uh oh!

coderabbitai bot commented Mar 18, 2026

Uh oh!

github-actions bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Mar 18, 2026

Walkthrough

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

1 additional finding

Uh oh!

thepastaclaw Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw commented Mar 20, 2026

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

`CalculateAmountPriority` (common.h)

`IsTimeOutOfBounds` (coinjoin.cpp)

github-actions bot commented Mar 18, 2026 •

edited

Loading