Skip to content

Fix overflow bug in Currency.MulWithOverflow#420

Open
peterjan wants to merge 1 commit into
masterfrom
pj/fix-mul-currency
Open

Fix overflow bug in Currency.MulWithOverflow#420
peterjan wants to merge 1 commit into
masterfrom
pj/fix-mul-currency

Conversation

@peterjan

Copy link
Copy Markdown
Member

This PR fixes an overflow bug in Currency.MulWithOverflow. It was passing the carry from the first high-word add into the second add instead of counting it as overflow, so when that carry hit and the second one didn’t, it reported no overflow on a 129-bit product and returned a result off by 2^64. Net effect: Mul would silently wrap instead of panicking.

@peterjan peterjan self-assigned this Jul 10, 2026
@github-project-automation github-project-automation Bot moved this to In Progress in Sia Jul 10, 2026
@peterjan peterjan force-pushed the pj/fix-mul-currency branch from dace2bd to e94a834 Compare July 10, 2026 09:51
@peterjan peterjan marked this pull request as ready for review July 10, 2026 09:52
Copilot AI review requested due to automatic review settings July 10, 2026 09:53

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

Tip: disable this comment in your organization's Code Review settings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes incorrect overflow detection/handling in Currency.MulWithOverflow, preventing silent wraparound (and incorrect results) for certain 129-bit products.

Changes:

  • Corrected the 128-bit multiplication carry handling so the “middle-word” carry is counted as overflow rather than folded into the result.
  • Added a targeted regression test case for the 129-bit product scenario.
  • Added a changeset documenting the patch-level fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
types/currency.go Fixes MulWithOverflow carry propagation to correctly report overflow and preserve low 128-bit result.
types/types_test.go Adds a regression vector and a randomized stress test around MulWithOverflow.
.changeset/fix_overflow_bug_in_currencymulwithoverflow.md Records the change as a patch-level update via changeset metadata.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread types/types_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants