Skip to content

Fix mulDiv at the u256 boundary with full-width divisor (closes #81)#87

Merged
koko1123 merged 1 commit into
mainfrom
fix/muldiv-u256-boundary
Jun 10, 2026
Merged

Fix mulDiv at the u256 boundary with full-width divisor (closes #81)#87
koko1123 merged 1 commit into
mainfrom
fix/muldiv-u256-boundary

Conversation

@koko1123

@koko1123 koko1123 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

What

mulDiv(MAX, MAX, MAX) should equal MAX ((2^256-1)^2 / (2^256-1) = 2^256-1) but returned a slightly smaller value.

Isolated: the overflow path (taken when a*b exceeds u256) used a limb-native Knuth Algorithm D divider (divWide) that mis-handled a full multi-limb divisor. mulDiv(MAX, 2, 2) was correct (small divisor); the all-limbs-set divisor was not.

Fix

Replaced the overflow path's mulWide+divWide with native u512: product = @as(u512,a) * @as(u512,b), quotient = product / denominator, return null if it exceeds u256. Simple and provably correct. Removed the now-dead buggy divWide.

The benchmarked u128 fast path (18ns) and the medium u256 path are untouched -- only the rare a*b-overflows-u256 branch changed, so DEX-math performance is unaffected.

Verification

The mulDiv overflow intermediate test (un-quarantined) passes, all DEX V2/V3 math tests still pass (they exercise mulDiv heavily). Suite: 773 pass, 2 skip. 0.16.0 and 0.17-dev both green.

Closes #81

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Corrected the mulDiv calculation to return accurate results in edge cases with full-width divisor overflow.

mulDiv(MAX, MAX, MAX) should equal MAX but returned a slightly smaller
value. The overflow path used a limb-native Knuth Algorithm D divider
(divWide) that mis-handled a full multi-limb divisor. Isolated:
mulDiv(MAX, 2, 2) was correct (small divisor) but the all-limbs-set
divisor case was not.

Replace the overflow path's mulWide+divWide with native u512:
product = a*b in u512, quotient = product/denominator, null if it
exceeds u256. Simple and provably correct. The fast u128 path (the
benchmarked 18ns hot path) and the medium u256 path are unchanged --
only the rare a*b-overflows-u256 branch is affected. Removed the now
dead, buggy divWide.

Surfaced by the test-harness fix (#84).
@vercel

vercel Bot commented Jun 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
eth-zig Ready Ready Preview, Comment Jun 10, 2026 6:31pm

Request Review

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b76f1cb0-806d-40ae-b181-fafb411a8d3c

📥 Commits

Reviewing files that changed from the base of the PR and between 2658200 and 3248b81.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • src/uint256.zig

📝 Walkthrough

Walkthrough

The PR fixes a boundary-case bug in mulDiv(a, b, denominator) where multiplying two u256 values at maximum results in a 512-bit intermediate that requires full-width division. The implementation now uses Zig's native u512 arithmetic instead of limb-native wide division, eliminating the incorrect result for edge cases like mulDiv(MAX, MAX, MAX). The previously-skipped overflow-intermediate test is enabled to validate the correction, and the changelog documents the fix.

Changes

mulDiv Overflow Boundary Correction

Layer / File(s) Summary
mulDiv overflow path and divWide removal
src/uint256.zig
Removes the divWide private helper implementing limb-native wide division. Rewrites mulDiv's overflow path to compute full-precision quotients directly using native u512 multiplication/division, returning null when the quotient exceeds MAX.
Test validation
src/uint256.zig
Unskips the mulDiv overflow intermediate test by removing its early error.SkipZigTest return, enabling MAX-boundary overflow assertions to run.
Changelog documentation
CHANGELOG.md
Documents the mulDiv correctness fix for the u256-overflow-with-full-width-divisor boundary case and notes the switch to native u512 arithmetic.

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • StrobeLabs/eth.zig#4: Both PRs modify src/uint256.zig's mulDiv implementation—main PR fixes the specific full-width divisor overflow path by switching to native u512 arithmetic, while PR #4 adds/optimizes the mulDiv 512-bit intermediate approach—so the changes are directly connected at the mulDiv code level.

A rabbit hops through numbers vast and wide,
Finds u512 where division hides,
MAX meets MAX without a fright,
The overflow boundary shines so bright! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: fixing mulDiv behavior at the u256 boundary when the divisor is full-width, directly matching the primary objective of the PR.
Linked Issues check ✅ Passed The PR successfully addresses all coding requirements from issue #81: fixes the mulDiv edge-case for full-width divisors, preserves existing performance paths, and restores the previously quarantined test.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the mulDiv overflow path and removing the problematic divWide helper; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/muldiv-u256-boundary

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

@koko1123
koko1123 merged commit 1ec181c into main Jun 10, 2026
14 checks passed
@koko1123
koko1123 deleted the fix/muldiv-u256-boundary branch June 10, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

uint256.mulDiv(MAX, MAX, MAX) wrong at the u256 boundary

1 participant