Skip to content

Minor Mulmod v2 refactor #10253

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
siladu merged 6 commits into from
Apr 19, 2026
Merged

Minor Mulmod v2 refactor #10253

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
66e15f2
Refactor stack index logic
siladu Apr 16, 2026
e2cab1d
Refactor test helper
siladu Apr 16, 2026
f935549
Remove redundant stack param
siladu Apr 17, 2026
cef643e
Inline mulmod method
siladu Apr 18, 2026
eec0607
Merge branch 'main' into mulmod-refactor
siladu Apr 18, 2026
a6c0500
compile error
siladu Apr 19, 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
26 changes: 16 additions & 10 deletions evm/src/main/java/org/hyperledger/besu/evm/v2/operation/MulModOperationV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,33 +50,39 @@ public Operation.OperationResult executeFixedCostOperation(
public static OperationResult staticOperation(final MessageFrame frame, final long[] stack) {
if (!frame.stackHasItems(3)) return UNDERFLOW_RESPONSE;
int top = frame.stackTopV2();
mulMod(stack, top);
// consumed three items and produced one item
frame.setTopV2(top - 2);
final int aOffset = (--top) << 2;
final int bOffset = (--top) << 2;
final int mOffset = (--top) << 2;

mulMod(stack, aOffset, bOffset, mOffset);

frame.setTopV2(++top);

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

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

Using side-effecting --top / ++top on the same variable makes the stack-height update fragile and harder to verify during future edits (e.g., inserting another read would silently change the resulting top). Consider using a separate stack pointer variable (e.g., int sp = frame.stackTopV2(); then decrement sp for offsets) and compute the final height explicitly (e.g., frame.setTopV2(sp + 1) after the three pops), avoiding pre-increment inside the setter.

Copilot uses AI. Check for mistakes.
return mulModSuccess;
}

/**
* Performs EVM MULMOD (modular multiplication) on the three top stack items.
* Performs EVM MULMOD (modular multiplication)
*
* <p>MULMOD: mulmod(a,b,m) = (a * b) mod m
* <p>MULMOD: mulmod(a, b, m) = (a * b) mod m
*
* <p>MULMOD: stack[top-3] = (stack[top-1] * stack[top-2]) mod stack[top-3].
*
* @param stack the flat limb array
* @param top current stack-top (item count)
* @param aOffset the stack offset of the first multiplicand
* @param bOffset the stack offset of the second multiplicand
* @param mOffset the stack offset of the modulus

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

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

The updated Javadoc is now offset-based, but it doesn’t explicitly state that aOffset/bOffset/mOffset are limb-array indices (already shifted by << 2), not “stack item indices”. Clarifying that these offsets are into the flat 4-limb backing array would reduce confusion for future maintainers and help prevent accidental double-shifting.

Suggested change
* @param aOffset the stack offset of the first multiplicand
* @param bOffset the stack offset of the second multiplicand
* @param mOffset the stack offset of the modulus
* @param aOffset the limb-array index of the first multiplicand in {@code stack}; this is an
* offset into the flat 4-limb backing array, not a stack item index
* @param bOffset the limb-array index of the second multiplicand in {@code stack}; this is an
* offset into the flat 4-limb backing array, not a stack item index
* @param mOffset the limb-array index of the modulus in {@code stack}; this is an offset into
* the flat 4-limb backing array, not a stack item index

Copilot uses AI. Check for mistakes.
*/
private static void mulMod(final long[] stack, final int top) {
final int aOffset = (top - 1) << 2;
final int bOffset = (top - 2) << 2;
final int mOffset = (top - 3) << 2;
private static void mulMod(

@lu-pinto lu-pinto Apr 17, 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.

nit: I don't get what's the benefit of this method, it's just creating another layer of indirection TBH. Wouldn't it be best to inline all of this logic in the operation directly? it doesn't feel like it increases complexity that much.
Anyway that being said it's more of a coding taste thing so won't block it.

@siladu siladu Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, inline is better cef643e

final long[] stack, final int aOffset, final int bOffset, final int mOffset) {
final UInt256 valueA =
new UInt256(stack[aOffset], stack[aOffset + 1], stack[aOffset + 2], stack[aOffset + 3]);
final UInt256 valueB =
new UInt256(stack[bOffset], stack[bOffset + 1], stack[bOffset + 2], stack[bOffset + 3]);
final UInt256 modulus =
new UInt256(stack[mOffset], stack[mOffset + 1], stack[mOffset + 2], stack[mOffset + 3]);

final UInt256 r = modulus.isZero() ? UInt256.ZERO : valueA.mulMod(valueB, modulus);

stack[mOffset] = r.u3();
stack[mOffset + 1] = r.u2();
stack[mOffset + 2] = r.u1();
Expand Down
9 changes: 2 additions & 7 deletions evm/src/test/java/org/hyperledger/besu/evm/v2/operation/MulModOperationV2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package org.hyperledger.besu.evm.v2.operation;

import static org.assertj.core.api.Assertions.assertThat;
import static org.hyperledger.besu.evm.v2.testutils.TestMessageFrameBuilderV2.getV2StackItem;

import org.hyperledger.besu.evm.UInt256;
import org.hyperledger.besu.evm.frame.ExceptionalHaltReason;
Expand Down Expand Up @@ -88,7 +89,7 @@ void mulModOperation(
expectedResult.equals("0x") || expectedResult.equals("0x0")
? UInt256.ZERO
: UInt256.fromBytesBE(Bytes32.fromHexStringLenient(expectedResult).toArrayUnsafe());
assertThat(getStackItem(frame, 0)).isEqualTo(expected);
assertThat(getV2StackItem(frame, 0)).isEqualTo(expected);
}

@Test
Expand Down Expand Up @@ -118,10 +119,4 @@ void mulModOperationUnderflowOnlyTwoItems() {
assertThat(result.getHaltReason()).isEqualTo(ExceptionalHaltReason.INSUFFICIENT_STACK_ITEMS);
assertThat(frame.stackTopV2()).isEqualTo(2);
}

private static UInt256 getStackItem(final MessageFrame frame, final int offset) {
final long[] s = frame.stackDataV2();
final int idx = (frame.stackTopV2() - 1 - offset) << 2;
return new UInt256(s[idx], s[idx + 1], s[idx + 2], s[idx + 3]);
}
}
13 changes: 13 additions & 0 deletions evm/src/test/java/org/hyperledger/besu/evm/v2/testutils/TestMessageFrameBuilderV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,19 @@ public MessageFrame build() {
return frame;
}

/**
* Reads a 256-bit word from the V2 stack at the given depth below the current top.
*
* @param frame the message frame with a V2 stack
* @param offset 0 for the topmost item, 1 for the item below, etc.
* @return the value as a {@link UInt256}
*/
public static UInt256 getV2StackItem(final MessageFrame frame, final int offset) {
final long[] s = frame.stackDataV2();
final int idx = (frame.stackTopV2() - 1 - offset) << 2;
Comment on lines +195 to +196

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

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

getV2StackItem will currently throw an ArrayIndexOutOfBoundsException (or compute a negative index) if offset is negative or offset >= frame.stackTopV2(). Since this is a shared test utility, it’s worth failing fast with a clear IllegalArgumentException (or an assertion) that validates offset >= 0 and offset < frame.stackTopV2() to make test failures easier to diagnose.

Suggested change
final long[] s = frame.stackDataV2();
final int idx = (frame.stackTopV2() - 1 - offset) << 2;
final int stackTop = frame.stackTopV2();
if (offset < 0 || offset >= stackTop) {
throw new IllegalArgumentException(
"Offset must be between 0 (inclusive) and stack size (exclusive): offset="
+ offset
+ ", stackTop="
+ stackTop);
}
final long[] s = frame.stackDataV2();
final int idx = (stackTop - 1 - offset) << 2;

Copilot uses AI. Check for mistakes.
return new UInt256(s[idx], s[idx + 1], s[idx + 2], s[idx + 3]);
}

private WorldUpdater createDefaultWorldUpdater() {
return new ToyWorld();
}
Expand Down
Loading