Skip to content

Fix edge case for limb overflow in SMOD #9934

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.

Sign up for GitHub

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

Merged
lu-pinto merged 9 commits into from
Mar 6, 2026
Merged

Fix edge case for limb overflow in SMOD #9934

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c2d093e
fix edge case for limb overflow
daniellehrner Mar 2, 2026
65a71c2
tests do not trip all fixed cases
lu-pinto Mar 3, 2026
267ece3
Merge branch 'main' into bug/smod_limb_overflow
lu-pinto Mar 3, 2026
5230bf6
spotless
lu-pinto Mar 3, 2026
507b17d
Merge branch 'main' into bug/smod_limb_overflow
daniellehrner Mar 4, 2026
678fca5
Add missing reduceStep for higher limb in Modulus192
lu-pinto Mar 5, 2026
23203ee
spotless
lu-pinto Mar 5, 2026
b0d8d7c
Merge remote-tracking branch 'upstream/main' into bug/smod_limb_overflow
lu-pinto Mar 5, 2026
36b7ff5
Merge branch 'main' into bug/smod_limb_overflow
lu-pinto Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions evm/src/main/java/org/hyperledger/besu/evm/UInt256.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,10 @@ private UInt128 mulSubOverflow(final long v1, final long v0) {
long carry = u0 - 1 + ((Long.compareUnsigned(v0, z0) <= 0) ? 1 : 0);

long z1 = v1 + u1 - carry;
// q = MAX may still be 1 too high; check if result >= modulus (i.e. negative wrapped)
if (Long.compareUnsigned(z1, u1) > 0 || (z1 == u1 && Long.compareUnsigned(z0, u0) >= 0)) {
return addBack(z1, z0);
}
return new UInt128(z1, z0);
}

Expand Down Expand Up @@ -1427,6 +1431,13 @@ private UInt192 mulSubOverflow(final long v2, final long v1, final long v0) {
carry = u1 - 1 + ((Long.compareUnsigned(v1, res) < 0) ? 1 : 0);

long z2 = v2 - carry + u2 - borrow;
// q = MAX may still be 1 too high; check if result >= modulus (i.e. negative wrapped)
if (Long.compareUnsigned(z2, u2) > 0
|| (z2 == u2
&& (Long.compareUnsigned(z1, u1) > 0
|| (z1 == u1 && Long.compareUnsigned(z0, u0) >= 0)))) {
return addBack(z2, z1, z0);
}
return new UInt192(z2, z1, z0);
}

Expand Down Expand Up @@ -1660,6 +1671,15 @@ private UInt256 mulSubOverflow(final long v3, final long v2, final long v1, fina
carry = u2 - 1 + ((Long.compareUnsigned(v2, res) < 0) ? 1 : 0);

long z3 = v3 + u3 - carry - borrow;
// q = MAX may still be 1 too high; check if result >= modulus (i.e. negative wrapped)
if (Long.compareUnsigned(z3, u3) > 0
|| (z3 == u3
&& (Long.compareUnsigned(z2, u2) > 0
|| (z2 == u2
&& (Long.compareUnsigned(z1, u1) > 0
|| (z1 == u1 && Long.compareUnsigned(z0, u0) >= 0)))))) {
return addBack(z3, z2, z1, z0);
}

@lu-pinto lu-pinto Mar 2, 2026

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.

I think it should be possible to write this without branches - did you check if this produces branching at assembly level?

@lu-pinto lu-pinto Mar 3, 2026 •
edited
Loading

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.

Was thinking about comparing the last limb instead:

      long t1 = v3 + u3;
      long t2 = t1 - carry;
      long z3 = t2 - borrow;

      borrow =
          ((Long.compareUnsigned(t1, t2) < 0) ? 1L : 0L)
        + ((Long.compareUnsigned(t2, z3) < 0) ? 1L : 0L)
        - ((Long.compareUnsigned(t1, v3) < 0) ? 1L : 0L);
      if (borrow > 0) {
		return addBack(z3, z2, z1, z0);
      }

It passes all tests, but after consideration I think it's safer to go with your version as it seems safer to compare each limb one by one with the modulus.
Also from a performance perspective this is such a rare case that it doesn't really matter much.

@thomas-quadratic thomas-quadratic Mar 3, 2026

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.

Here a check that multipy substract with overflow gives a negative answer is sufficient, meaning there is a borrow, exactly like multiply substract operation.
I am not sure why you say it is safer, it is logically the same to check for a leftover borrow or to check the result has wrapped around, and it is similar to what we do in mulSub.

return new UInt256(z3, z2, z1, z0);
}

Expand Down
40 changes: 40 additions & 0 deletions evm/src/test/java/org/hyperledger/besu/evm/UInt256Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,46 @@ public void mulMod() {
}
}

@Test
public void modWithOverflowQuotientEstimate() {

@lu-pinto lu-pinto Mar 2, 2026

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.

All 3 tests only target UInt192::mulSubOverflow. Please write up additional tests that can test the other 2 cases.

// When the dividend's leading limb equals the modulus's leading limb, the trial quotient
// overflows and is clamped to 2^64-1. Verify correctness for each Modulus size.

// Modulus192 path (b.u3==0, b.u2!=0)
UInt256 a1 =
UInt256.fromBytesBE(
new BigInteger("7effffff8000000000000000000000000000000000000000d900000000000001", 16)
.toByteArray());
UInt256 b1 =
UInt256.fromBytesBE(
new BigInteger("7effffff800000007effffff800000008000ff0000010000", 16).toByteArray());
BigInteger expected1 = new BigInteger("7effffff800000007dff00feffff0001d901fe0000020001", 16);
assertThat(new BigInteger(1, a1.mod(b1).toBytesBE())).isEqualTo(expected1);
assertThat(new BigInteger(1, a1.signedMod(b1).toBytesBE())).isEqualTo(expected1);

// Modulus128 path (b.u3==0, b.u2==0, b.u1!=0)
UInt256 a2 =
UInt256.fromBytesBE(new BigInteger("ffffffffffffffff0000000000000001", 16).toByteArray());
UInt256 b2 =
UInt256.fromBytesBE(new BigInteger("ffffffffffffffff0000000000000000", 16).toByteArray());
BigInteger expected2 = new BigInteger("1", 16);
assertThat(new BigInteger(1, a2.mod(b2).toBytesBE())).isEqualTo(expected2);

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

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

The Modulus128 path test only covers mod, but unlike the Modulus192 and Modulus256 tests, it does not also test signedMod. For consistency and to ensure the edge case is exercised in the signed path as well, a corresponding assertThat(new BigInteger(1, a2.signedMod(b2).toBytesBE())).isEqualTo(expected2) assertion should be added here (the Modulus192 test on line 562 does include both, and both values are non-negative so the result is the same).

Suggested change
assertThat(new BigInteger(1, a2.mod(b2).toBytesBE())).isEqualTo(expected2);
assertThat(new BigInteger(1, a2.mod(b2).toBytesBE())).isEqualTo(expected2);
assertThat(new BigInteger(1, a2.signedMod(b2).toBytesBE())).isEqualTo(expected2);

Copilot uses AI. Check for mistakes.

// Modulus256 path (b.u3!=0)
UInt256 a3 =
UInt256.fromBytesBE(
new BigInteger("7effffff800000000000000000000000000000000000000000000000000000ff", 16)
.toByteArray());
UInt256 b3 =
UInt256.fromBytesBE(
new BigInteger("7effffff800000007effffff80000000800000000000000080000000000000ff", 16)
.toByteArray());
BigInteger aBI3 = new BigInteger(1, a3.toBytesBE());
BigInteger bBI3 = new BigInteger(1, b3.toBytesBE());
BigInteger expected3 = aBI3.mod(bBI3);

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

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

The expected value for the Modulus256 test case is derived from BigInteger.mod at runtime rather than being a hardcoded constant. If there is a bug in BigInteger (extremely unlikely) or a mistake in constructing a3/b3, the test would pass even with a wrong result. For a regression test, it is better to use a pre-computed hardcoded expected value (as is done for the 192-bit and 128-bit paths) so the test anchors the correct answer independently.

Suggested change
BigInteger aBI3 = new BigInteger(1, a3.toBytesBE());
BigInteger bBI3 = new BigInteger(1, b3.toBytesBE());
BigInteger expected3 = aBI3.mod(bBI3);
// a3 < b3, so a3 mod b3 == a3. Precomputed expected value:
BigInteger expected3 =
new BigInteger("7effffff800000000000000000000000000000000000000000000000000000ff", 16);

Copilot uses AI. Check for mistakes.
assertThat(new BigInteger(1, a3.mod(b3).toBytesBE())).isEqualTo(expected3);
}

@Test
public void signedMod() {
final Random random = new Random(432);
Expand Down