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.

Sign up for GitHub

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

1568 some last mmio issues second part #1571

Merged
merged 9 commits into from
Nov 27, 2024
Merged

1568 some last mmio issues second part #1571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b23c78e
fix(mmuCall): extcodecopy of account under deployment
letypequividelespoubelles Nov 26, 2024
a235f33
fix: need to snapshot memory even if memorySpan empty
letypequividelespoubelles Nov 26, 2024
27d5a9d
Merge branch 'arith-dev' into 1568-some-last-mmio-issues
letypequividelespoubelles Nov 26, 2024
67ae06d
Olivier's cooment + ras
letypequividelespoubelles Nov 26, 2024
beaaddc
Merge branch '1568-some-last-mmio-issues' of github.com:Consensys/lin…
letypequividelespoubelles Nov 26, 2024
b326cdc
fix(modexp): NPE for some ref tests triggering MODEXP with huge arg
letypequividelespoubelles Nov 27, 2024
26491aa
Merge branch 'arith-dev' into 1568-some-last-mmio-issues
letypequividelespoubelles Nov 27, 2024
d9f8598
fix(modexp): NPE for huge arguments
letypequividelespoubelles Nov 27, 2024
da8f70c
fix: don't reject modexp with arg size of 512 bytes
letypequividelespoubelles Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...metization/src/main/java/net/consensys/linea/zktracer/module/hub/defer/DeferRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,8 @@ public void resolveUponContextReEntry(Hub hub, CallFrame callFrame) {
public void unscheduleForContextReEntry(ContextReEntryDefer defer, CallFrame callFrame) {
contextReEntryDefers.get(callFrame).remove(defer);
}

public void unscheduleForPostTransaction(PostTransactionDefer defer) {
postTransactionDefers.remove(defer);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As explained below I'm not convinced we need this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see other comment. We schedule for postTx at the creation of the callSection, when the (modexp) subsection is still not created, so we don't have the info at the time of the schedulling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So wouldn't we have to unscheduled the postTransaction stuff, too ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the postTx we unschedule !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The one we don't unSchedule (because it doesn't create NPE) is contextEntry

}
50 changes: 24 additions & 26 deletions ...ation/src/main/java/net/consensys/linea/zktracer/module/hub/section/call/CallSection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,6 @@ public class CallSection extends TraceSection
// row i+0
private final CallScenarioFragment scenarioFragment = new CallScenarioFragment();

public boolean isAbortingScenario() {
return scenarioFragment.getScenario().isAbortingScenario();
}

// last row
@Setter private ContextFragment finalContextFragment;

Expand Down Expand Up @@ -211,7 +207,7 @@ public CallSection(Hub hub, MessageFrame frame) {

value =
opCode.callHasValueArgument()
? Wei.of(currentFrame.frame().getStackItem(2).toUnsignedBigInteger())
? Wei.of(frame.getStackItem(2).toUnsignedBigInteger())
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved
: Wei.ZERO;

final CallOobCall oobCall = new CallOobCall();
Expand All @@ -224,13 +220,13 @@ public CallSection(Hub hub, MessageFrame frame) {
hub.defers().scheduleForPostTransaction(this);

// The CALL is now unexceptional and un-aborted
refineUndefinedScenario(hub);
CallScenarioFragment.CallScenario scenario = scenarioFragment.getScenario();
refineUndefinedScenario(hub, frame);
final CallScenarioFragment.CallScenario scenario = scenarioFragment.getScenario();
switch (scenario) {
case CALL_ABORT_WONT_REVERT -> abortingCall(hub);
case CALL_EOA_UNDEFINED -> eoaProcessing(hub);
case CALL_PRC_UNDEFINED -> prcProcessing(hub);
case CALL_SMC_UNDEFINED -> smcProcessing(hub, frame);
case CALL_PRC_UNDEFINED -> prcProcessing(hub);
case CALL_ABORT_WONT_REVERT -> abortingCall(hub);
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved
default -> throw new RuntimeException("Illegal CALL scenario");
}
}
Expand Down Expand Up @@ -313,21 +309,18 @@ private void abortingCall(Hub hub) {
*
* @param hub
*/
private void refineUndefinedScenario(Hub hub) {
private void refineUndefinedScenario(Hub hub, MessageFrame frame) {

final boolean aborts = hub.pch().abortingConditions().any();
if (aborts) {
scenarioFragment.setScenario(CALL_ABORT_WONT_REVERT);
return;
}

final WorldUpdater world = hub.currentFrame().frame().getWorldUpdater();
final WorldUpdater world = frame.getWorldUpdater();
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved
if (isPrecompile(calleeAddress)) {
precompileAddress = Optional.of(calleeAddress);
scenarioFragment.setScenario(CALL_PRC_UNDEFINED);

precompileSubsection =
ADDRESS_TO_PRECOMPILE.get(preOpcodeCalleeSnapshot.address()).apply(hub, this);
} else {
Optional.ofNullable(world.get(calleeAddress))
.ifPresentOrElse(
Expand Down Expand Up @@ -361,8 +354,17 @@ private void smcProcessing(Hub hub, MessageFrame frame) {
}

private void prcProcessing(Hub hub) {
precompileSubsection =
ADDRESS_TO_PRECOMPILE.get(preOpcodeCalleeSnapshot.address()).apply(hub, this);
hub.defers().scheduleForContextEntry(this);
hub.defers().scheduleForContextReEntry(this, hub.currentFrame());
// In case of arguments too large for MODEXP, transaction will be popped anyway, and resolving
// some defers will create NPE
if (precompileSubsection instanceof ModexpSubsection
&& ((ModexpSubsection) precompileSubsection).transactionWillBePopped) {
hub.defers().unscheduleForContextReEntry(this, hub.currentFrame());
hub.defers().unscheduleForPostTransaction(this);
}
Copy link
Collaborator

@OlivierBBB OlivierBBB Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't it make more sense to

  • schedule all the PrecompileSubsection's that aren't instanceof ModexpSubsection
  • only schedule a ModexpSubsection if !transactionWillBePopped ?

You are scheduling and unscheduling but you already have all requisite information to decide whether you will need to schedule at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do agree, but I find it way more readable to do it in any case, and undo it in some (very rare) case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

plus when we schedule for postTransaction, we still don't have the information as the precompile subsection is created after. So in any case we'll have to unschedule it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that both should be equivalent. Your call

}

@Override
Expand All @@ -375,7 +377,7 @@ public void resolvePostExecution(
@Override
public void resolveUponContextEntry(Hub hub) {

CallScenarioFragment.CallScenario scenario = scenarioFragment.getScenario();
final CallScenarioFragment.CallScenario scenario = scenarioFragment.getScenario();
checkState(scenario == CALL_SMC_UNDEFINED | scenario == CALL_PRC_UNDEFINED);

postOpcodeCallerSnapshot = preOpcodeCallerSnapshot.deepCopy();
Expand Down Expand Up @@ -462,33 +464,29 @@ public void resolveAtContextReEntry(Hub hub, CallFrame frame) {
}

case CALL_SMC_UNDEFINED -> {

// CALL_SMC_SUCCESS_XXX case
if (successBit) {
scenarioFragment.setScenario(CALL_SMC_SUCCESS_WONT_REVERT);
return;
}

AccountSnapshot beforeFailureCallerSnapshot =
final AccountSnapshot beforeFailureCallerSnapshot =
postOpcodeCallerSnapshot.deepCopy().setDeploymentInfo(hub);
AccountSnapshot afterFailureCallerSnapshot =
final AccountSnapshot afterFailureCallerSnapshot =
preOpcodeCallerSnapshot.deepCopy().setDeploymentInfo(hub);
AccountSnapshot beforeFailureCalleeSnapshot =
final AccountSnapshot beforeFailureCalleeSnapshot =
postOpcodeCalleeSnapshot.deepCopy().setDeploymentInfo(hub);
AccountSnapshot afterFailureCalleeSnapshot =
final AccountSnapshot afterFailureCalleeSnapshot =
preOpcodeCalleeSnapshot.deepCopy().setDeploymentInfo(hub).turnOnWarmth();

// CALL_SMC_FAILURE_XXX case
scenarioFragment.setScenario(CALL_SMC_FAILURE_WONT_REVERT);

if (isNonzeroValueSelfCall()) {
childContextExitCallerSnapshot.decrementBalanceBy(value);
reEntryCalleeSnapshot.decrementBalanceBy(value);
}

int childId = hub.currentFrame().childFrameIds().getLast();
CallFrame childFrame = hub.callStack().getById(childId);
int childContextRevertStamp = childFrame.revertStamp();
final int childId = hub.currentFrame().childFrameIds().getLast();
final CallFrame childFrame = hub.callStack().getById(childId);
final int childContextRevertStamp = childFrame.revertStamp();

final AccountFragment postReEntryCallerAccountFragment =
hub.factories()
Expand Down
8 changes: 5 additions & 3 deletions ...nsensys/linea/zktracer/module/hub/section/call/precompileSubsection/ModexpSubsection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class ModexpSubsection extends PrecompileSubsection {
public final ModexpMetadata modexpMetaData;
private ModexpPricingOobCall sixthOobCall;
private ImcFragment seventhImcFragment;
public boolean transactionWillBePopped = false;

public ModexpSubsection(final Hub hub, final CallSection callSection) {
super(hub, callSection);
Expand All @@ -62,19 +63,20 @@ public ModexpSubsection(final Hub hub, final CallSection callSection) {
.bbs()
.toUnsignedBigInteger()
.compareTo(BigInteger.valueOf(MODEXP_COMPONENT_BYTE_SIZE))
>= 0
> 0
|| modexpMetaData
.mbs()
.toUnsignedBigInteger()
.compareTo(BigInteger.valueOf(MODEXP_COMPONENT_BYTE_SIZE))
>= 0
> 0
|| modexpMetaData
.ebs()
.toUnsignedBigInteger()
.compareTo(BigInteger.valueOf(MODEXP_COMPONENT_BYTE_SIZE))
>= 0) {
> 0) {
hub.modexpEffectiveCall().addPrecompileLimit(Integer.MAX_VALUE);
hub.defers().unscheduleForContextReEntry(this, hub.currentFrame());
transactionWillBePopped = true;
return;
}

Expand Down
4 changes: 3 additions & 1 deletion ...sys/linea/zktracer/module/hub/section/call/precompileSubsection/PrecompileSubsection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,9 @@ public long returnDataSize() {
}

public Bytes rawCallerMemory() {
return callSection.getCallDataRange().getRawData();
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved
return getCallDataRange().isEmpty()
? getReturnAtRange().getRawData()
: getCallDataRange().getRawData();
}

public Bytes extractCallData() {
Expand Down
1 change: 0 additions & 1 deletion arithmetization/src/main/java/net/consensys/linea/zktracer/runtime/callstack/CallFrame.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ public boolean isMessageCall() {
@Getter private int byteCodeDeploymentNumber;
private EWord eCodeAddress = null; // memoization
@Getter private Bytecode code = Bytecode.EMPTY;
@Getter private int codeFragmentIndex = -1;
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved

// caller related information
@Getter private Address callerAddress = Address.ZERO;
Expand Down
10 changes: 5 additions & 5 deletions arithmetization/src/main/java/net/consensys/linea/zktracer/types/MemoryRange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,21 @@ public MemoryRange(long contextNumber) {
public MemoryRange(final long contextNumber, final Range range, final Bytes rawData) {
this.contextNumber = contextNumber;
this.range = range;
this.rawData = rawData;
this.rawData = range.isEmpty() ? Bytes.EMPTY : rawData;
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved
}

public MemoryRange(
final long contextNumber, final long offset, final long size, final Bytes rawData) {
this.contextNumber = contextNumber;
this.range = Range.fromOffsetAndSize(offset, size);
this.rawData = rawData;
this.rawData = isEmpty() ? Bytes.EMPTY : rawData;
}

public MemoryRange(final long contextNumber, final Range range, final MessageFrame frame) {
this.contextNumber = contextNumber;
this.range = range;
this.rawData =
(range.isEmpty()) ? Bytes.EMPTY : frame.shadowReadMemory(0, frame.memoryByteSize());
range.isEmpty() ? Bytes.EMPTY : frame.shadowReadMemory(0, frame.memoryByteSize());
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved
}

public long offset() {
Expand All @@ -103,7 +103,7 @@ public long contextNumber() {
}

public Bytes extract() {
return range.isEmpty()
return isEmpty()
? Bytes.EMPTY
: rightPaddedSlice(rawData, safeLongToInt(range.offset()), safeLongToInt(range.size()));
}
Expand All @@ -113,6 +113,6 @@ public boolean isEmpty() {
}

public MemoryRange snapshot() {
return new MemoryRange(this.contextNumber, this.range.snapshot(), this.rawData);
return new MemoryRange(contextNumber, range.snapshot(), rawData.copy());
letypequividelespoubelles marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading