Skip to content
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.

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

Extensive OOGX tests involving memory expansion + gas estimation tool BytecodeRunner.runOnlyForGasCost + MSIZE (#1783) #1783

Merged
merged 32 commits into from
Feb 20, 2025

Conversation

lorenzogentile404
Copy link
Collaborator

@lorenzogentile404 lorenzogentile404 commented Feb 7, 2025

Closes #1685

  • BytecodeRunner.runOnlyForGasCost Method to calculate cost dynamically - based on LondonGasCalculator (Besu)
  • OOGX for opcodes with no mem expansion : list from opCodeToOpCodeDataMap.values() without opcodes with mem expansion cost
  • OOGX for opcodes with mem expansion cost : MSTORE, MSTORE8, MLOAD, CALLDATACOPY, CODECOPY, EXTCODECOPY, RETURN, RETURNDATACOPY, REVERT, CREATE, LOGX
  • MSIZE test: with non zero value and post memory expansion

Local benchmark done on 366 tests with

  • calculate gas cost manually 1min40s
  • calculate gas cost dynamically 1min30s
  • calculate gas cost by running BytecodeRunner.run twice 2min50s

Test coverage improvement
+7 lines covered
+4 conditions covered
Results
+0.3% condition coverage on Unit tests
+0.1% overall coverage score (with blockchain ref tests)

@lorenzogentile404 lorenzogentile404 changed the title Feat/nontrivial part1 OOGX tests using DynamicGasCostUtils Feb 7, 2025
@amkCha amkCha force-pushed the feat/nontrivial-part1 branch 4 times, most recently from 4fe31f2 to f449242 Compare February 18, 2025 09:21
@amkCha amkCha marked this pull request as ready for review February 18, 2025 16:15
@amkCha amkCha changed the title OOGX tests using DynamicGasCostUtils OOGX tests using BytecodeRunner.runOnlyForGasCost + MSIZE Feb 18, 2025
@amkCha amkCha force-pushed the feat/nontrivial-part1 branch from 7e06004 to af937c1 Compare February 18, 2025 16:29
.op(OpCode.MSIZE)
.push(0xFF)
.push(1) // expand memory
.op(OpCode.MSTORE)
Copy link
Collaborator

@OlivierBBB OlivierBBB Feb 19, 2025

Choose a reason for hiding this comment

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

Obvious remark: both MSTORE's expand memory, the first to size 0x20, the second to size 0x40.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, it felt better to go from a non-zero value to a non-zero value, but it's strictly the same behavior. Let me simplify it 👍

Comment on lines 154 to 163
bytecodeRunner.run(Wei.fromEth(1), gasCost + cornerCase, List.of(), calldata);
if (cornerCase == -1) {
assertEquals(
OUT_OF_GAS_EXCEPTION,
bytecodeRunner.getHub().previousTraceSection().commonValues.tracedException());
} else {
assertNotEquals(
OUT_OF_GAS_EXCEPTION,
bytecodeRunner.getHub().previousTraceSection().commonValues.tracedException());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This paragraph could be extracted into a method I imagine ? It's duplicated quite a number of times.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea, I created a Utils method 👌


@ParameterizedTest
@ValueSource(ints = {-1, 0, 1})
void outOfGasExceptionJumpi(int cornerCase) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, too, we have the opportunity to test simultaneous exceptions, in this case:

jumpException
outOfGasException

And it would be trivial to implement, just switch the second push to

    final Bytes bytecode =
        BytecodeCompiler.newProgram()
            .push(1) // pc = 0, 1
            .push(6) // pc = 2, 3
            // ...

Copy link
Collaborator

@OlivierBBB OlivierBBB Feb 19, 2025

Choose a reason for hiding this comment

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

It would make sense to create a new class for these MultiExceptionTests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which would include the RETURNDATACOPY double exception test mentioned earlier.

Copy link
Collaborator

@amkCha amkCha Feb 20, 2025

Choose a reason for hiding this comment

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

Issue created for MultiExceptionTests #1838

Copy link
Collaborator

@OlivierBBB OlivierBBB left a comment

Choose a reason for hiding this comment

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

Left several comments

@amkCha amkCha added the testing label Feb 20, 2025
@amkCha amkCha mentioned this pull request Feb 20, 2025
3 tasks
@amkCha amkCha force-pushed the feat/nontrivial-part1 branch from bb84d5e to ca1f209 Compare February 20, 2025 09:22
@amkCha amkCha requested a review from OlivierBBB February 20, 2025 09:30
Copy link
Collaborator

@OlivierBBB OlivierBBB left a comment

Choose a reason for hiding this comment

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

LGTM

@OlivierBBB OlivierBBB enabled auto-merge (squash) February 20, 2025 11:03
@OlivierBBB OlivierBBB changed the title OOGX tests using BytecodeRunner.runOnlyForGasCost + MSIZE Extensive OOGX tests involving memory expansion + gas estimation tool BytecodeRunner.runOnlyForGasCost + MSIZE (#1783) Feb 20, 2025
@OlivierBBB OlivierBBB disabled auto-merge February 20, 2025 11:05
@amkCha amkCha merged commit 145bb43 into arith-dev Feb 20, 2025
6 checks passed
@amkCha amkCha deleted the feat/nontrivial-part1 branch February 20, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nontrivial values tests for stack-consistency testing
3 participants