Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: ADDMOD performs operations MOD 2^256 #695

Closed
Tracked by #28 ...
greged93 opened this issue Aug 24, 2023 · 2 comments · Fixed by #865
Closed
Tracked by #28 ...

bug: ADDMOD performs operations MOD 2^256 #695

greged93 opened this issue Aug 24, 2023 · 2 comments · Fixed by #865
Assignees

Comments

@greged93
Copy link
Contributor

Bug Report

Kakarot version: ee6458a

Current behavior:
The add mod opcode performs addition of two uint256, without handling the one bit carry returned by the uint256_add. This causes the addition to be performed modulo 2^256, which shouldn't be the case (see Ethereum yellow paper opcode ADDMOD).

Expected behavior:

// Stack input:
// 0 - a: number.
// 1 - b: number.
// 1 - c: modulo.
let stack = ctx.stack;
let (stack, popped) = Stack.pop_n(self=stack, n=3);
let a = popped[0];
let b = popped[1];
let c = popped[2];
// Compute the addition
let (result, _) = uint256_add(a, b);
// Compute the modulo
let (_, rem) = uint256_unsigned_div_rem(result, c);

The add mod opcode should return the operation (a+b)%c, without subjecting the intermediate result a+b to modulo 2^256. However, since the bit carry from uint256_add isn't used, the operation is performed modulo 2^256.

Steps to reproduce:

RUN_SPECIFIC_TESTS=addmod_d10g0v0_Shanghai cargo test -p ef-testing --features ef-tests -- --nocapture

This test will fail due to the incorrect addition performed modulo 2^256:
https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/VMTests/vmArithmeticTest/addmodFiller.yml#L109

Indeed, in our case, the performed operation is: (-1 + -2) % 5 = ((0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff + 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe) % 2^256) % 5 = (0x1fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffd % 2^256) % 5 = 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffd % 5 = 3.

Failing output should be:

[!] Case /Users/greg/code/rust/ef-tests/crates/ef-testing/ethereum-tests/BlockchainTests/GeneralStateTests/VMTests/vmArithmeticTest/addmod.json failed (description: addmod): Test failed: failed test addmod_d10g0v0_Shanghai: expected storage value 0x0000000000000000000000000000000000000000000000000000000000000004, got 0x0000000000000000000000000000000000000000000000000000000000000003
@Eikix
Copy link
Member

Eikix commented Sep 8, 2023

A duplicate of #678, but let's keep this one, it's more detailed

@feltroidprime
Copy link
Contributor

feltroidprime commented Nov 30, 2023

can do add mod in 15 steps for 10 XMR if you want

@enitrat enitrat self-assigned this Jan 8, 2024
@enitrat enitrat mentioned this issue Jan 8, 2024
7 tasks
ClementWalter pushed a commit that referenced this issue Jan 8, 2024
<!--- Please provide a general summary of your changes in the title
above -->

<!-- Give an estimate of the time you spent on this PR in terms of work
days.
Did you spend 0.5 days on this PR or rather 2 days?  -->

Time spent on this PR: 0.5d

## Pull request type

<!-- Please try to limit your pull request to one type,
submit multiple pull requests if needed. -->

Please check the type of change your PR introduces:

- [x] 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?

<!-- Please describe the current behavior that you are modifying,
or link to a relevant issue. -->

Resolves #695 Resolves #691 

## What is the new behavior?

<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Fixed the addmod implementation
-
-
@github-project-automation github-project-automation bot moved this from 🆕 Backlog to ✅ Done in Kakarot on Starknet Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants