Add MulOperationV2#10291
Conversation
Uses UInt256.mul same as MulOperationOptimized and stack_long_array Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces a v2 implementation of the EVM MUL opcode using the long[]-backed v2 stack, along with tests and a JMH benchmark to validate/capture the performance improvement.
Changes:
- Added
MulOperationV2implementingMULfor the v2 long[] stack layout. - Wired opcode
0x02to the v2MULimplementation in the v2 execution loop. - Added unit tests and a JMH benchmark for
MulOperationV2.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| evm/src/test/java/org/hyperledger/besu/evm/v2/operation/MulOperationV2Test.java | Adds correctness/stack-behavior/gas-cost tests for v2 MUL. |
| evm/src/main/java/org/hyperledger/besu/evm/v2/operation/MulOperationV2.java | Implements the v2 MUL operation using the long[] stack representation. |
| evm/src/main/java/org/hyperledger/besu/evm/EVM.java | Routes opcode 0x02 to MulOperationV2.staticOperation during v2 execution. |
| ethereum/core/src/jmh/java/org/hyperledger/besu/ethereum/vm/operations/v2/MulOperationBenchmarkV2.java | Adds a JMH benchmark harness for the v2 MUL operation. |
| */ | ||
| public class MulOperationV2 extends AbstractFixedCostOperationV2 { | ||
|
|
||
| private static final OperationResult MUL_SUCCESS = new OperationResult(5, null); |
There was a problem hiding this comment.
The operation is constructed with a GasCalculator and AbstractFixedCostOperationV2 is given gasCalculator.getLowTierGasCost(), but executeFixedCostOperation() returns a static OperationResult hardcoding gas cost 5. This makes execute(...) ignore the provided gas calculator (e.g., custom calculators or future changes). Prefer deriving the OperationResult gas cost from the calculator (e.g., store a per-instance OperationResult built from getLowTierGasCost()), or adjust the API so the static path can receive the intended gas cost.
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
|
|
||
| assertThat(frame.stackTopV2()).isEqualTo(2); | ||
| assertThat(getV2StackItem(frame, 0)).isEqualTo(UInt256.fromInt(8)); // 4 * 2 | ||
| assertThat(getV2StackItem(frame, 1)).isEqualTo(UInt256.fromBytesBE(untouched.toArrayUnsafe())); |
| Arguments.of( | ||
| "0x1000000000000001200000000000000230000000000000034000000000000004", | ||
| "0x1000000000000001100000000000000110000000000000011000000000000001", | ||
| "0x490000000000000b2700000000000009e4000000000000078000000000000004")); |
There was a problem hiding this comment.
can you add these two cases :
- x * 1 = x
- x * y = y * x
There was a problem hiding this comment.
Already covered by property_mul_identity and property_mul_commutative
| final Operation.OperationResult result = operation.execute(frame, null); | ||
|
|
||
| assertThat(result.getGasCost()).isEqualTo(gasCalculator.getLowTierGasCost()); | ||
| } |
There was a problem hiding this comment.
Can you include a test for OutOfGas, and an other test for OutOfGas even stackunderflow error, ex. https://github.com/besu-eth/besu/pull/10298/changes#diff-3bee54d9bf3b24b510bf547be393a3a29960b9b273f0a2b8f5a5b07ab2679e20R101-R107
ahamlat
left a comment
There was a problem hiding this comment.
Few comments on the tests but nothing blocking
| import org.hyperledger.besu.evm.operation.Operation; | ||
| import org.hyperledger.besu.evm.v2.operation.MulOperationV2; | ||
|
|
||
| public class MulOperationBenchmarkV2 extends BinaryOperationBenchmarkV2 { |
There was a problem hiding this comment.
I would wait for the changes on the Sub operation PR to avoid doing another PR to change this to BinaryArithmeticOperationBenchmarkV2 ?
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
* Refactor and fixes for JMH benchmarks regarding signed values (#10269) Summary of changes: - There was a lot of duplicate setup code in all of the arithmetic opcodes (DIV, SDIV, MOD, ...). All this copy pasting didn't help and the definition of enums with the byte sizes manually creates a source for errors and duplicate effort. - Names are also now consistent within each benchmark following OPCODE_BIT-SIZE_BIT-SIZE structure for easy copy pasting of results and interpretation. - Removed enum case definition in most cases to avoid redundancy. - Some issues were also fixed around negation of inputs for testing signed opcodes (SMOD was missing negation!). Inputs are now correctly negated and compared with their absolute values for swapping them. - Generation of 256 bit negative numbers is now limited to 255 bits to make sure we leave MSB for two complement representation. * Publish besu-evm as an API dependency from plugin-api (#10262) Signed-off-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> * Header download progress (#10275) * Save and resume header download progress on pipeline restart Track the lowest imported block number in ImportHeadersStep and persist it to ChainSyncState so that backward header downloads can resume from where they left off after an error, rather than restarting from the pivot. - Add ChainSyncState.withHeaderProgress() to update the header progress - Track lowestImportedBlock in ImportHeadersStep - Return BackwardHeaderPipelineResult record from pipeline factory - Call saveHeaderProgress() in SnapSyncChainDownloader on error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> * Enforce EIP-7928 BAL item budget per transaction in processing and mining (#10250) Signed-off-by: Karim Taam <karim.t2am@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Take empty block period seconds out of experimental (#10264) * Take empty block period seconds out of experimental Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Tidy up changelog Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Typo Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Review comments Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> --------- Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Add ChaindId, Coinbase, Gaslimit and PrevRandao to EVM v2 (#10298) * Add ChaindId, Coinbase, Gaslimit and PrevRandao to EVM v2 Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> * Address comments Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> --------- Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> * Rename InvalidSystemCallAddressException to SystemCallNoCodeAtAddressException (#10305) * Rename InvalidSystemCallAddressException to SystemCallNoCodeAtAddressException The exception is thrown when no code exists at the address, not because the address is invalid. Updated all usages. Fixes #10281 Signed-off-by: Liberty S <694522458@qq.com> * Fix missing reference to renamed exception Signed-off-by: Liberty S <694522458@qq.com> --------- Signed-off-by: Liberty S <694522458@qq.com> Co-authored-by: daniellehrner <daniel.lehrner@consensys.net> * Publish Guava as an API dependency from plugin-api (#10248) Signed-off-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> * clean stop bws if world state unavailable (#10021) * clean stop bws if world state unavailable * immutable field Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> * Reject Status with mismatched protocolVersion vs layout (#10241) * Reject Status with mismatched protocolVersion vs layout A Besu peer build (seen in production as `besu/v26.2-develop-73d07f9`) advertises eth/69 in Hello but sends the eth/68 Status layout `[version, networkId, totalDifficulty, bestHash, genesisHash, forkId]` with `version=69` stamped on the wire. Spec-strict EL clients (e.g. Nimbus) reject this with `protocol breach` every 30 seconds. Besu's current decoder uses shape auto-detection (checks whether the fourth element is a list) and only discovers the inconsistency inside the EthStatus constructor via `checkArgument`, which throws `IllegalArgumentException`. That type is not caught by `EthProtocolManager.handleStatusMessage`'s `try/catch (RLPException)`, so the exception escapes the message dispatcher instead of producing a clean `SUBPROTOCOL_TRIGGERED_UNPARSABLE_STATUS` disconnect. Validate version/layout consistency inline in `EthStatus.readFrom` and throw `RLPException` instead. Add tests for both mismatch directions, including the exact malformed bytes captured from the broken peer in bal-devnet-3. Signed-off-by: qu0b <st3f4n.s@gmail.com> Signed-off-by: qu0b <stefan@starflinger.eu> * Remove redundant comment on shape/version enforcement Addresses review from @pinges: the code is self-explanatory. Signed-off-by: Stefan <stefan@starflinger.eu> --------- Signed-off-by: qu0b <st3f4n.s@gmail.com> Signed-off-by: qu0b <stefan@starflinger.eu> Signed-off-by: Stefan <stefan@starflinger.eu> Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> * Flaky BackwardSyncContextTest: remove broken Awaitility pattern (#10303) * Fix flaky BackwardSyncContextTest by removing broken Awaitility pattern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Matilda-Clerke <matilda.clerke@consensys.net> * Add MulOperationV2 (#10291) Add MulOperationV2, units and benchmark Uses UInt256.mul same as MulOperationOptimized Signed-off-by: Simon Dudley <simon.dudley@consensys.net> * Feat/reenable dynamic cpsb calculation (#10295) * reenable dynamic costPerStateByte calculation Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * fix AbstractBlockProcessorIntegrationTest Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> --------- Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * add eip 7976 to Amsterdam (#10296) Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * Add EIP-7981 to bal-devnet-4 (#10297) * add eip 7981 to Amsterdam Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * fix AbstractBlockProcessorIntegrationTest Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> --------- Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> --------- Signed-off-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> Signed-off-by: Karim Taam <karim.t2am@gmail.com> Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> Signed-off-by: Liberty S <694522458@qq.com> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: qu0b <st3f4n.s@gmail.com> Signed-off-by: qu0b <stefan@starflinger.eu> Signed-off-by: Stefan <stefan@starflinger.eu> Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> Co-authored-by: Luis Pinto <luis.pinto@consensys.net> Co-authored-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Karim Taam <karim.t2am@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Matt Whitehead <matthew.whitehead@kaleido.io> Co-authored-by: ahamlat <ameziane.hamlat@consensys.net> Co-authored-by: Liberty-Swine <694522458@qq.com> Co-authored-by: Stefan <22667037+qu0b@users.noreply.github.com> Co-authored-by: Matilda-Clerke <matilda.clerke@consensys.net> Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
* Refactor and fixes for JMH benchmarks regarding signed values (besu-eth#10269) Summary of changes: - There was a lot of duplicate setup code in all of the arithmetic opcodes (DIV, SDIV, MOD, ...). All this copy pasting didn't help and the definition of enums with the byte sizes manually creates a source for errors and duplicate effort. - Names are also now consistent within each benchmark following OPCODE_BIT-SIZE_BIT-SIZE structure for easy copy pasting of results and interpretation. - Removed enum case definition in most cases to avoid redundancy. - Some issues were also fixed around negation of inputs for testing signed opcodes (SMOD was missing negation!). Inputs are now correctly negated and compared with their absolute values for swapping them. - Generation of 256 bit negative numbers is now limited to 255 bits to make sure we leave MSB for two complement representation. * Publish besu-evm as an API dependency from plugin-api (besu-eth#10262) Signed-off-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> * Header download progress (besu-eth#10275) * Save and resume header download progress on pipeline restart Track the lowest imported block number in ImportHeadersStep and persist it to ChainSyncState so that backward header downloads can resume from where they left off after an error, rather than restarting from the pivot. - Add ChainSyncState.withHeaderProgress() to update the header progress - Track lowestImportedBlock in ImportHeadersStep - Return BackwardHeaderPipelineResult record from pipeline factory - Call saveHeaderProgress() in SnapSyncChainDownloader on error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> * Enforce EIP-7928 BAL item budget per transaction in processing and mining (besu-eth#10250) Signed-off-by: Karim Taam <karim.t2am@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Take empty block period seconds out of experimental (besu-eth#10264) * Take empty block period seconds out of experimental Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Tidy up changelog Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Typo Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Review comments Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> --------- Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Add ChaindId, Coinbase, Gaslimit and PrevRandao to EVM v2 (besu-eth#10298) * Add ChaindId, Coinbase, Gaslimit and PrevRandao to EVM v2 Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> * Address comments Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> --------- Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> * Rename InvalidSystemCallAddressException to SystemCallNoCodeAtAddressException (besu-eth#10305) * Rename InvalidSystemCallAddressException to SystemCallNoCodeAtAddressException The exception is thrown when no code exists at the address, not because the address is invalid. Updated all usages. Fixes besu-eth#10281 Signed-off-by: Liberty S <694522458@qq.com> * Fix missing reference to renamed exception Signed-off-by: Liberty S <694522458@qq.com> --------- Signed-off-by: Liberty S <694522458@qq.com> Co-authored-by: daniellehrner <daniel.lehrner@consensys.net> * Publish Guava as an API dependency from plugin-api (besu-eth#10248) Signed-off-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> * clean stop bws if world state unavailable (besu-eth#10021) * clean stop bws if world state unavailable * immutable field Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> * Reject Status with mismatched protocolVersion vs layout (besu-eth#10241) * Reject Status with mismatched protocolVersion vs layout A Besu peer build (seen in production as `besu/v26.2-develop-73d07f9`) advertises eth/69 in Hello but sends the eth/68 Status layout `[version, networkId, totalDifficulty, bestHash, genesisHash, forkId]` with `version=69` stamped on the wire. Spec-strict EL clients (e.g. Nimbus) reject this with `protocol breach` every 30 seconds. Besu's current decoder uses shape auto-detection (checks whether the fourth element is a list) and only discovers the inconsistency inside the EthStatus constructor via `checkArgument`, which throws `IllegalArgumentException`. That type is not caught by `EthProtocolManager.handleStatusMessage`'s `try/catch (RLPException)`, so the exception escapes the message dispatcher instead of producing a clean `SUBPROTOCOL_TRIGGERED_UNPARSABLE_STATUS` disconnect. Validate version/layout consistency inline in `EthStatus.readFrom` and throw `RLPException` instead. Add tests for both mismatch directions, including the exact malformed bytes captured from the broken peer in bal-devnet-3. Signed-off-by: qu0b <st3f4n.s@gmail.com> Signed-off-by: qu0b <stefan@starflinger.eu> * Remove redundant comment on shape/version enforcement Addresses review from @pinges: the code is self-explanatory. Signed-off-by: Stefan <stefan@starflinger.eu> --------- Signed-off-by: qu0b <st3f4n.s@gmail.com> Signed-off-by: qu0b <stefan@starflinger.eu> Signed-off-by: Stefan <stefan@starflinger.eu> Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> * Flaky BackwardSyncContextTest: remove broken Awaitility pattern (besu-eth#10303) * Fix flaky BackwardSyncContextTest by removing broken Awaitility pattern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Matilda-Clerke <matilda.clerke@consensys.net> * Add MulOperationV2 (besu-eth#10291) Add MulOperationV2, units and benchmark Uses UInt256.mul same as MulOperationOptimized Signed-off-by: Simon Dudley <simon.dudley@consensys.net> * Feat/reenable dynamic cpsb calculation (besu-eth#10295) * reenable dynamic costPerStateByte calculation Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * fix AbstractBlockProcessorIntegrationTest Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> --------- Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * add eip 7976 to Amsterdam (besu-eth#10296) Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * Add EIP-7981 to bal-devnet-4 (besu-eth#10297) * add eip 7981 to Amsterdam Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> * fix AbstractBlockProcessorIntegrationTest Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> --------- Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> --------- Signed-off-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> Signed-off-by: Karim Taam <karim.t2am@gmail.com> Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> Signed-off-by: Liberty S <694522458@qq.com> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: qu0b <st3f4n.s@gmail.com> Signed-off-by: qu0b <stefan@starflinger.eu> Signed-off-by: Stefan <stefan@starflinger.eu> Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: daniellehrner <daniel.lehrner@consensys.net> Co-authored-by: Luis Pinto <luis.pinto@consensys.net> Co-authored-by: Alejandro <26930485+alejandroGM0@users.noreply.github.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Karim Taam <karim.t2am@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Matt Whitehead <matthew.whitehead@kaleido.io> Co-authored-by: ahamlat <ameziane.hamlat@consensys.net> Co-authored-by: Liberty-Swine <694522458@qq.com> Co-authored-by: Stefan <22667037+qu0b@users.noreply.github.com> Co-authored-by: Matilda-Clerke <matilda.clerke@consensys.net> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Part of #10131
Uses UInt256.mul same as MulOperationOptimized and stack_long_array
stack_long_array