-
Notifications
You must be signed in to change notification settings - Fork 33
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
1568 some last mmio issues second part #1571
1568 some last mmio issues second part #1571
Conversation
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
arithmetization/src/main/java/net/consensys/linea/zktracer/types/MemoryRange.java
Outdated
Show resolved
Hide resolved
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
…ea-tracer into 1568-some-last-mmio-issues
...sensys/linea/zktracer/module/hub/section/call/precompileSubsection/PrecompileSubsection.java
Show resolved
Hide resolved
arithmetization/src/main/java/net/consensys/linea/zktracer/types/MemoryRange.java
Show resolved
Hide resolved
arithmetization/src/main/java/net/consensys/linea/zktracer/types/MemoryRange.java
Show resolved
Hide resolved
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
arithmetization/src/main/java/net/consensys/linea/zktracer/types/MemoryRange.java
Outdated
Show resolved
Hide resolved
arithmetization/src/main/java/net/consensys/linea/zktracer/types/MemoryRange.java
Show resolved
Hide resolved
&& ((ModexpSubsection) precompileSubsection).transactionWillBePopped) { | ||
hub.defers().unscheduleForContextReEntry(this, hub.currentFrame()); | ||
hub.defers().unscheduleForPostTransaction(this); | ||
} |
There was a problem hiding this comment.
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'tinstanceof 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
...tization/src/main/java/net/consensys/linea/zktracer/module/hub/section/call/CallSection.java
Show resolved
Hide resolved
...tization/src/main/java/net/consensys/linea/zktracer/module/hub/section/call/CallSection.java
Show resolved
Hide resolved
...tization/src/main/java/net/consensys/linea/zktracer/module/hub/section/call/CallSection.java
Show resolved
Hide resolved
|
||
public void unscheduleForPostTransaction(PostTransactionDefer defer) { | ||
postTransactionDefers.remove(defer); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 !
There was a problem hiding this comment.
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
.../consensys/linea/zktracer/module/hub/section/call/precompileSubsection/ModexpSubsection.java
Outdated
Show resolved
Hide resolved
arithmetization/src/main/java/net/consensys/linea/zktracer/runtime/callstack/CallFrame.java
Show resolved
Hide resolved
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.