Skip to content
This repository was archived by the owner on Jan 9, 2025. It is now read-only.

fix: addmod #1170

Merged
merged 2 commits into from
Jun 3, 2024
Merged

fix: addmod #1170

merged 2 commits into from
Jun 3, 2024

Conversation

enitrat
Copy link
Collaborator

@enitrat enitrat commented May 31, 2024

Time spent on this PR: 0.5d

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Resolves #

What is the new behavior?

  • Fixes a bug on addmod where if (a+b) overflowed, the carry was not handled

This change is Reviewable

Comment on lines +252 to +254
// result = (2**256) - (n - overflown_sum)
// <=> result = (2**256 - 1) - (n - overflown_sum - 1)
// as n > overflown_sum we can't have an underflow
Copy link
Member

Choose a reason for hiding this comment

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

this is not so easy to follow.

You could

let a = popped[0]
let b = popped[1]
let n = popped[2]

then using your notations (the names of the variable)

result = 2**256 * carry + sum = 2**256 + sum
 = 2**256 + sum - n = 2**256 - 1 - n + sum + 1

and this is because carry > 0 => n > 2**128 so 2**256 mod n = 2**256 - n

@ClementWalter ClementWalter merged commit f3faa28 into main Jun 3, 2024
7 checks passed
@ClementWalter ClementWalter deleted the fix/addmod branch June 3, 2024 11:46
matthieuauger pushed a commit to matthieuauger/kakarot that referenced this pull request Nov 9, 2024
* dev: move TRANSACTION_MAX_RETRIES to env

* undo to pub access identifier

* applied suggestions from code review

* fixed linting issues

---------

Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants