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

debug memory replay tests #1473

Merged
merged 27 commits into from
Nov 14, 2024

Conversation

letypequividelespoubelles
Copy link
Collaborator

No description provided.

Signed-off-by: F Bojarski <[email protected]>
@letypequividelespoubelles letypequividelespoubelles added the bug Something isn't working label Nov 4, 2024
@letypequividelespoubelles letypequividelespoubelles linked an issue Nov 4, 2024 that may be closed by this pull request
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
@letypequividelespoubelles letypequividelespoubelles marked this pull request as ready for review November 13, 2024 15:15
int stackLineCounter,
int nonStackLineCounter,
int mmuStamp,
int mxpStamp) {
this.commonFragmentValues = commonValues;
this.twoLineInstructionCounter = stackLineCounter == 1;
this.nonStackRowsCounter = nonStackLineCounter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should homogenize the names. In the spec it's NON_STACK_ROWS or NSR.

@@ -47,7 +47,7 @@ public class CommonFragmentValues {
public final HubProcessingPhase hubProcessingPhase;
public final int hubStamp;
public final CallStack callStack;
public final State.TxState.Stamps stamps; // for MMU and MXP stamps
public final State.TxState.Stamps stamps;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You provide the mmuStamp and mxpStamp separately in the CommonValuesFragment constructor. Are the State.TxState.Stamps dysfunctional ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this instruction missing altogether ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, it was in the CallDataLoadSection. But it was missing the postExecutiondefers to get the stack item directly from Besu

public Optional<Bytes> exoBytes() {
return exoIsRom ? Optional.of(hub.romLex().getCodeByMetadata(contract)) : Optional.empty();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this grabbing the currently executing bytecode in stead ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this was working well, except when the ROM is not set (ie in the case failure CREATE2), because we weren't able to get the initCode to hash in this case

@@ -62,47 +63,45 @@ public StackRamSection(Hub hub) {
// the unexceptional case
checkArgument(Exceptions.none(exceptions));

final EWord offset = EWord.of(hub.currentFrame().frame().getStackItem(0));
final CallFrame currentFrame = hub.currentFrame();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've never been a fan of the currentFrame terminology since we have two kinds of frames: CallFrame's and MessageFrame's. Maybe we can rename it to simply callFrame ?

.limb1(value.hi())
.limb2(value.lo())
.sourceRamBytes(Optional.of(currentRam));

Copy link
Collaborator

Choose a reason for hiding this comment

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

This block is just formatting, right ?

@@ -96,6 +96,8 @@ public PrecompileSubsection(final Hub hub, final CallSection callSection) {
this.callSection = callSection;
fragments = new ArrayList<>(maxNumberOfLines());

final MessageFrame messageFrame = hub.messageFrame();

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a very general comment: XxxSection's, with the exception of prewarming, initialization, finalization and skipping sections I believe, are all generated in the traceOpcode method of the hub. This method has as its input a (Besu) MessageFrame. This message frame is always "the right one". We also have the message frame in the hub's callFrame. So the question is: can and should we ax CallFrame.messageFrame in favour of always using the <tracing method> provided MessageFrame ?

Reasons for doing so: avoid synchronization issues. Kind of like the one we had wrt traceEndTransaction, where the worldUpdater of the HUB, which was implicitly used in the canonical AccountSnapshot constructor, was out of date and we should have been using the Besu provided one.

@letypequividelespoubelles letypequividelespoubelles merged commit 352b8b3 into arith-dev Nov 14, 2024
5 checks passed
@letypequividelespoubelles letypequividelespoubelles deleted the 1471-debug-memory-replaytests branch November 14, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug memory replayTests
2 participants