Skip to content

1970 mxp redesign #1988

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

Open
wants to merge 33 commits into
base: arith-dev
Choose a base branch
from
Open

1970 mxp redesign #1988

wants to merge 33 commits into from

Conversation

lorenzogentile404
Copy link
Collaborator

@lorenzogentile404 lorenzogentile404 commented Apr 22, 2025

Redesign of the Mxp module for Cancun where MCOPY is added

The Mxp operation can follow 5 scenarii depending on the opcode. Each scenario executes wcp and euc computations and inherits from the previous scenario as computations are cumulative

1. MSize scenario

  • no computation

2. Trivial scenario

  • computes size1IsZero and size2IsZero

3. Mxpx scenario

  • computes size1IsZero and size2IsZero
  • computes mxpxExpression

4. State update with word pricing scenario

  • computes size1IsZero and size2IsZero
  • computes mxpxExpression
  • computes state update (wordsNew,cMemNew)
  • and extraGasCost for word pricing opcodes

5. State update with byte pricing scenario

  • computes size1IsZero and size2IsZero
  • computes mxpxExpression
  • computes state update (wordsNew,cMemNew)
  • and extraGasCost for byte pricing opcodes

@lorenzogentile404 lorenzogentile404 linked an issue Apr 22, 2025 that may be closed by this pull request
@amkCha amkCha force-pushed the 1970-mxp-redesign branch from 9e73108 to b14cc55 Compare April 30, 2025 16:03
Copy link
Collaborator

@letypequividelespoubelles letypequividelespoubelles left a comment

Choose a reason for hiding this comment

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

I'd suggest to have, for each Mxp instruction, a instMxpCall tht extends an abstract MxpCall where you compute only the needed stuff (see what has been done for OOB).

|| opCode.isCall();
}

// This is a copy and past from FrontierGasCalculator.java
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't we already have this somewhere where we compute the exceptions ?

@amkCha amkCha force-pushed the 1970-mxp-redesign branch from 257bec4 to a517c87 Compare May 7, 2025 19:56
@@ -52,7 +52,7 @@ public void memoryExpansionExceptionTest(boolean triggerRoob, OpCode opCode) {
MEMORY_EXPANSION_EXCEPTION,
bytecodeRunner.getHub().previousTraceSection().commonValues.tracedException());
assertTrue(bytecodeRunner.getHub().mxp().operations().getLast().getMxpCall().isMxpx());
assertEquals(triggerRoob, bytecodeRunner.getHub().mxp().operations().getLast().isRoob());
// assertEquals(triggerRoob, bytecodeRunner.getHub().mxp().operations().getLast().isRoob());
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lorenzogentile404 not sure why we comment this part ? Should we keep it while we run London version of Mxp ?

@amkCha
Copy link
Collaborator

amkCha commented May 7, 2025

I'd suggest to have, for each Mxp instruction, a instMxpCall tht extends an abstract MxpCall where you compute only the needed stuff (see what has been done for OOB).

Okay so to keep the computation in the Mxp folder and not have it in the Hub, I used the same logic but instead of an abstract MxpCall, I created an abstract class called MxpScenario and I derive the computation in each scenario that extends this class. Let me know if it makes it clear to read else I can always refactor to do it it with an MxpCall abstract class 👌

Copy link
Collaborator

@letypequividelespoubelles letypequividelespoubelles left a comment

Choose a reason for hiding this comment

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

just looked at the logic, not at the implementation

.pComputationArg2Hi(scenario.exoCalls.get(i).arg2Hi())
.pComputationArg2Lo(scenario.exoCalls.get(i).arg2Lo())
.pComputationResA(scenario.exoCalls.get(i).resultA().toLong())
.pComputationResB(scenario.exoCalls.get(i).resultB().toLong())
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might have sign issues :) we have conversion Bytes to unsignedLong somewhere

@amkCha amkCha force-pushed the 1970-mxp-redesign branch from 7f7fe8a to 9d67898 Compare May 12, 2025 13:44
@amkCha amkCha force-pushed the 1970-mxp-redesign branch from 62eec10 to c577ffc Compare May 14, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MXP redesign
3 participants