Minor Mulmod v2 refactor

[siladu]

Takes stack "push", "pop" concept from #10216
i.e. (--top) and (++top)
also preemptive getV2StackItem test helper refactor from #10229

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors the MULMOD v2 implementation to use stack pointer pre-decrement/increment semantics and centralizes v2 stack item reading logic into a shared test utility.

Changes:

• Refactor MulModOperationV2 stack access to use --top offsets and ++top for the resulting stack height.
• Introduce a shared getV2StackItem helper in TestMessageFrameBuilderV2.
• Update MulModOperationV2Test to use the shared helper and remove the duplicate local implementation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
evm/src/test/java/org/hyperledger/besu/evm/v2/testutils/TestMessageFrameBuilderV2.java	Adds a reusable helper to read a UInt256 directly from the v2 flat stack array.
evm/src/test/java/org/hyperledger/besu/evm/v2/operation/MulModOperationV2Test.java	Switches tests to the shared helper and removes duplicated stack-reading code.
evm/src/main/java/org/hyperledger/besu/evm/v2/operation/MulModOperationV2.java	Updates MULMOD to compute operand offsets via stack-pointer decrement and sets the new stack top accordingly.

[Copilot]

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]

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]

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

[lu-pinto]

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]

Agree, inline is better cef643e