Skip to content

fix UInt256: Take result from addBack in mulSubOverflow #10078

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 3 commits into from
Mar 20, 2026
Merged

fix UInt256: Take result from addBack in mulSubOverflow #10078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb93bc8
Return result from addBack during mulSubOverflow instead of ignoring it
lu-pinto Mar 19, 2026
c127a10
add property test pattern for mulsub overflows
lu-pinto Mar 19, 2026
e6a0716
address review comments
lu-pinto Mar 20, 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
45 changes: 28 additions & 17 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 @@ -992,7 +992,7 @@ private QR256 mulSub(final long v3, final long v2, final long v1, final long v0,
return new QR256(q, new UInt256(z3, z2, z1, z0));
}

private UInt256 mulSubOverflow(final long v3, final long v2, final long v1, final long v0) {
private QR256 mulSubOverflow(final long v3, final long v2, final long v1, final long v0) {
// Overflow case: div2by1 quotient would be <1, 0>, but adjusts to <0, -1>
// <p1, p0> = -1 * u0 = <u0 - 1, -u0>
long res, borrow;
Expand All @@ -1011,21 +1011,22 @@ 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)
// q = MAX may still be 1 too high - need to pass q - 1 (-2L) to addBack; 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, 1L).r;
return addBack(z3, z2, z1, z0, -2L);
}
return new UInt256(z3, z2, z1, z0);
return new QR256(-1L, new UInt256(z3, z2, z1, z0));
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Mar 20, 2026

Choose a reason for hiding this comment

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

There is an extra allocation here and below, maybe worth checking gc an allocation numbers with JMH.

Copy link
Copy Markdown
Contributor Author

@lu-pinto lu-pinto Mar 20, 2026

Choose a reason for hiding this comment

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

the question is not if there is an extra allocation or not but how would you do it without it?

Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Mar 20, 2026

Choose a reason for hiding this comment

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

It is not a question but a statement. The hidden question is can we afford it or not ? so we need to benchmark it. I can see though a weak design in allocating twice on the returned result.

}

private QR256 reduceStep(
final long v4, final long v3, final long v2, final long v1, final long v0, final long inv) {
if (v4 == u3) return new QR256(-1L, mulSubOverflow(v3, v2, v1, v0));
if (v4 == u3) return mulSubOverflow(v3, v2, v1, v0);
QR64 qr = div2by1(v4, v3, u3, inv);
if (qr.q != 0) return mulSub(qr.r, v2, v1, v0, qr.q);
return new QR256(0, new UInt256(v3, v2, v1, v0));
Expand Down Expand Up @@ -1175,8 +1176,10 @@ private static QR64 div2by1(final long x1, final long x0, final long y, final lo
// --------------------------------------------------------------------------
// endregion

// region Records
// --------------------------------------------------------------------------
// region Quotient / Remainder types
// These types are used to store the result of division. Due to the nature of the algorithm
// (div2by1) quotient (q) can't
// ever exceed 64 bits while remainder (r) can range from 64 to 256 bits.
private record QR64(long q, long r) {}

private record QR128(long q, UInt128 r) {}
Expand All @@ -1185,6 +1188,11 @@ private record QR192(long q, UInt192 r) {}

private record QR256(long q, UInt256 r) {}

// --------------------------------------------------------------------------
// endregion

// region UInt* types
// --------------------------------------------------------------------------
record UInt64(long u0) {
UInt64 shiftLeft(final int shift) {
return (shift == 0) ? this : new UInt64(u0 << shift);
Expand Down Expand Up @@ -1414,22 +1422,23 @@ private QR128 mulSub(final long x1, final long x0, final long q) {
return new QR128(q, new UInt128(z1, z0));
}

private UInt128 mulSubOverflow(final long v1, final long v0) {
private QR128 mulSubOverflow(final long v1, final long v0) {
// Overflow case: div2by1 quotient would be <1, 0>, but adjusts to <0, MAX>
// <p1, p0> = -1 * u0 = <u0 - 1, -u0>
long z0 = v0 + u0;
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)
// q = MAX may still be 1 too high - need to pass q - 1 (-2L) to addBack; 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, 1L).r;
Copy link
Copy Markdown
Contributor

@siladu siladu Mar 19, 2026

Choose a reason for hiding this comment

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

Was this 1L acting is a dummy value before? If not why is it OK to change it?

Copy link
Copy Markdown
Contributor Author

@lu-pinto lu-pinto Mar 20, 2026

Choose a reason for hiding this comment

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

yes this was wrong before. While in mulSubOverflow the quotient wraps around the long value so q = MAX -> 0xfff..fff so -1 is the right value while we are inside mulSubOverflow - see comment at the top of the method for computing the remainder:

      // Overflow case: div2by1 quotient would be <1, 0>, but adjusts to <0, MAX>
      // <p1, p0> = -1 * u0 = <u0 - 1, -u0>

While going into addBack the quotient estimation is off by 1 so we need to decrement before jumping into addBack.

return addBack(z1, z0, -2L);
Copy link
Copy Markdown
Contributor

@macfarla macfarla Mar 20, 2026

Choose a reason for hiding this comment

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

think the -2L arg is non-obvious, worth a comment

Copy link
Copy Markdown
Contributor Author

@lu-pinto lu-pinto Mar 20, 2026

Choose a reason for hiding this comment

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

now it has been fixed it seems obvious and don't really know how to best explain in few lines but added a comment now, hopefully that helps.

}
return new UInt128(z1, z0);
return new QR128(-1L, new UInt128(z1, z0));
}

private QR128 reduceStep(final long v2, final long v1, final long v0, final long inv) {
if (v2 == u1) return new QR128(-1L, mulSubOverflow(v1, v0));
if (v2 == u1) return mulSubOverflow(v1, v0);
QR64 qr = div2by1(v2, v1, u1, inv);
if (qr.q != 0) return mulSub(qr.r, v0, qr.q);
return new QR128(0, new UInt128(qr.r, v0));
Expand Down Expand Up @@ -1640,7 +1649,7 @@ private QR192 mulSub(final long v2, final long v1, final long v0, final long q)
return new QR192(q, new UInt192(z2, z1, z0));
}

private UInt192 mulSubOverflow(final long v2, final long v1, final long v0) {
private QR192 mulSubOverflow(final long v2, final long v1, final long v0) {
// Overflow case: div2by1 quotient would be <1, 0>, but adjusts to <0, -1>
// <p1, p0> = -1 * u0 = <u0 - 1, -u0>
long z0 = v0 + u0;
Expand All @@ -1652,19 +1661,21 @@ 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)
// q = MAX may still be 1 too high - need to pass q - 1 (-2L) to addBack; 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, 1L).r;

return addBack(z2, z1, z0, -2L);
}
return new UInt192(z2, z1, z0);
return new QR192(-1L, new UInt192(z2, z1, z0));
}

private QR192 reduceStep(
final long v3, final long v2, final long v1, final long v0, final long inv) {
if (v3 == u2) return new QR192(-1L, mulSubOverflow(v2, v1, v0));
if (v3 == u2) return mulSubOverflow(v2, v1, v0);
QR64 qr = div2by1(v3, v2, u2, inv);
if (qr.q != 0) return mulSub(qr.r, v1, v0, qr.q);
return new QR192(0, new UInt192(v2, v1, v0));
Expand Down
32 changes: 21 additions & 11 deletions evm/src/test/java/org/hyperledger/besu/evm/UInt256PropertyBasedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ Arbitrary<byte[]> bytes0to64_shaped() {
Tuple.of(4, Arbitraries.of(Pattern.ALL_ZERO)),
Tuple.of(4, Arbitraries.of(Pattern.ALL_FF)),
Tuple.of(4, Arbitraries.of(Pattern.LIMB_SIGN_BITS)),
Tuple.of(4, Arbitraries.of(Pattern.FIXED_TOP_LIMBS)),
Tuple.of(10, Arbitraries.of(Pattern.RANDOM)));

return lengths.flatMap(
Expand Down Expand Up @@ -129,29 +130,38 @@ private enum Pattern {
ALL_ZERO,
ALL_FF,
LIMB_SIGN_BITS,
FIXED_TOP_LIMBS,
RANDOM
}

private static byte[] applyPattern(final byte[] bytes, final Pattern pat) {
switch (pat) {
case ALL_ZERO:
return switch (pat) {
case ALL_ZERO -> {
Arrays.fill(bytes, (byte) 0x00);
return bytes;
case ALL_FF:
yield bytes;
}
case ALL_FF -> {
Arrays.fill(bytes, (byte) 0xFF);
return bytes;
case LIMB_SIGN_BITS:
yield bytes;
}
case LIMB_SIGN_BITS -> {
Arrays.fill(bytes, (byte) 0x00);
forceMsbAtIndexIfPresent(bytes, 0);
forceMsbAtIndexIfPresent(bytes, 8);
forceMsbAtIndexIfPresent(bytes, 16);
forceMsbAtIndexIfPresent(bytes, 24);
forceMsbAtIndexIfPresent(bytes, bytes.length - 1);
return bytes;
case RANDOM:
default:
return bytes;
}
yield bytes;
}
case FIXED_TOP_LIMBS -> {
int size = (int) Math.ceil(bytes.length / 8D) * 8;
final byte[] newArray = new byte[size];
System.arraycopy(bytes, 0, newArray, size - bytes.length, bytes.length);
Arrays.fill(newArray, 0, 8, (byte) 0x80);
yield newArray;
}
case RANDOM -> bytes;
};
}

private static void forceMsbAtIndexIfPresent(final byte[] bytes, final int index) {
Expand Down
14 changes: 13 additions & 1 deletion 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 @@ -601,7 +601,19 @@ static Collection<Object[]> testCases() {
"0x0700b2d7adda7612da7f95"
},
{"0xbf1256135bb3f72de074d0f237", "0x8b63235ac1765530"},
{"0x5b35862b0027a502b1d4cbc4a09e25", "0x932542f4003763"}
{"0x5b35862b0027a502b1d4cbc4a09e25", "0x932542f4003763"},
{
// Multiply and subtract overflows and we need to decrement quotient estimation -
// UInt192 case
"0x8200000000000000000000000000000000000000000000000000000000000000",
"0x8200000000000000fe000004000000ffff000000fffff700"
},
{
// Multiply and subtract overflows and we need to decrement quotient estimation -
// UInt128 case
"0x820000000000000000000000000000000000000000000000",
"0x8200000000000000fe00000000000001"
},
})
.flatMap(
inputs ->
Expand Down
Loading