-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: TRM redesign #1936
feat: TRM redesign #1936
Conversation
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]>
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.
Pull Request Overview
This PR implements a redesign of the TRM module, refactoring the way WCP calls are handled and integrating a new approach for TRM operations. Key changes include:
- Introducing a new WcpCall class to encapsulate WCP instruction calls.
- Refactoring the Trm and TrmOperation classes to leverage the new WcpCall API and update loop conditions.
- Updating Hub to instantiate Trm with the required Wcp dependency.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
arithmetization/src/main/java/net/consensys/linea/zktracer/module/wcp/WcpCall.java | New class implementing WCP instruction calls with helper methods. |
arithmetization/src/main/java/net/consensys/linea/zktracer/module/trm/Trm.java | Refactored TRM module to use final parameters and pass Wcp to operations. |
arithmetization/src/main/java/net/consensys/linea/zktracer/module/trm/TrmOperation.java | Updated TRM operation logic to create WcpCall entries and adapt the tracing loop. |
arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/Hub.java | Updated instantiation of Trm to include the Wcp dependency. |
Files not reviewed (1)
- linea-constraints: Language not supported
arithmetization/src/main/java/net/consensys/linea/zktracer/module/trm/TrmOperation.java
Show resolved
Hide resolved
Signed-off-by: F Bojarski <[email protected]>
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
private final Bytes arg2Lo; | ||
private final boolean result; | ||
|
||
public WcpCall(Wcp wcp, byte instruction, Bytes arg1, Bytes arg2) { |
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 think we needed that method here as well https://github.com/Consensys/linea-tracer/blob/arith-dev/arithmetization/src/main/java/net/consensys/linea/zktracer/module/blockdata/BlockdataOperation.java#L264
Maybe we add a TODO to refacto and use that method after you merge 👌
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.
We need it not only for blockdata, but almost all modules calling WCP, so euc, mmio, txndata, hub, mxp, shakira, blakemodexp, ecData, oob, blockhash, soon rlpTxn ...
Signed-off-by: F Bojarski <[email protected]>
No description provided.