op-challenger: Asterisc Support with Refactoring #10094
Conversation
WalkthroughWalkthroughThe recent updates focus on integrating and enhancing support for the Asterisc trace type within the Optimism protocol's testing and configuration frameworks. These changes include adding new configurations, flags, and metrics specific to Asterisc, updating existing utilities, and introducing new testing and execution files for both Asterisc and Cannon trace types. The goal is to enhance the flexibility and capability of handling different trace types and network configurations efficiently. Changes
This summary highlights the significant changes, emphasizing the integration and enhancement of the Asterisc trace type, along with updates to existing functionalities and documentation to support the evolving requirements of the Optimism protocol. Recent Review DetailsConfiguration used: .coderabbit.yml Files ignored due to path filters (5)
Files selected for processing (38)
Files skipped from review as they are similar to previous changes (34)
Additional Context UsedLanguageTool (62)
Additional comments not posted (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
pcw109550
force-pushed
the
tip/pcw109550/op-challenger-asterisc-refactor
branch
from
68b3a2a
to
c24ac9b
Compare
pcw109550
force-pushed
the
tip/pcw109550/op-challenger-asterisc-refactor
branch
from
583736a
to
ceb1b24
Compare
pcw109550
force-pushed
the
tip/pcw109550/op-challenger-asterisc-refactor
branch
from
f8d9d5b
to
838d7d6
Compare
|
@refcell I removed asterisc's trace provider dependency of cannon by creating new |
Move gzip method to ioutil
Basic unit tests mirrored from cannon.
pcw109550
force-pushed
the
tip/pcw109550/op-challenger-asterisc-refactor
branch
from
291454f
to
a6d8361
Compare
|
@ajsutton No problem. I have gone back to having separate options again. Added tests. Also manually checked that the binary works like below. If both are not provided, If at least one between |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
c38ce09
TL; DR: Implemented op-challenger for asterisc with refactoring. Only unit test is passing. E2E tests will be implemented at asterisc repo.
Description
op-challenger provides a wrapper with FPVM binary, and it only supported cannon to play dispute game, providing traces. This PR adds a wrapper with yet another FPVM, asterisc.
Initial implementation(#10030) of op-challenger for asterisc had some duplicated codes, and even cyclic dependency from the nature that asterisc is maintained at separate repository.
This PR removes asterisc dependencies (removing code duplicates) by asking asterisc binary to already include the witness and stateHash in the state json. Related asterisc fixes: ethereum-optimism/asterisc#37, ethereum-optimism/asterisc#36. These FPVM fixes may be also added to cannon, to decouple FPVM and the challenger. It is FPVM's job to provide correct witness and stateHash.
I tried to avoid duplicate codes by exporting cannon tracer methods as much as possible. These shared logics can be refactored out. op-challenger for cannon is currently running on testnet, and I wanted to avoid code changes for cannon in this PR.
Refactoring Suggestion
FPVM abstraction
FPVM maintains its state, and witness and stateHash. In the FPVM state,
step,PC,Exitedwill be always be included, whatever FPVM we choose. Therefore, we may use below struct to parse state json from any FPVM.Upper fields are the only required data to perform op-challenger logic. I was able to implement asterisc tracer based on upper fields.
op-challenger can perform basic validation of this data. It can check the witness length since this may be differ by the VM type. Also, we may validate
exitCodewhich is included atStateHash.Metricer
Current
Metricerinterface requiresRecordCannonExecutionTime()andRecordAsteriscExecutionTime()to be implemented. I first thought to refactor these methods toRecordFPVMExecutionTime(), but it may break compatibility for prometheus.Flags
Most of the flags for asterisc binary is duplicated from cannon: (See ethereum-optimism/asterisc#8), and we may think all the duplicated flag codes(op-challenger/flags/flags.go) are unnecessary. However, I thought these flags are not included in the specs, so they are subject to change.
Tests
Unit tests for asterisc tracer is added. Basic flag sanity checking unit test is added.
There are no e2e tests added! These will be added to asterisc. Will refer to output_cannon_test.go.
To add more details about why e2e is implemented at asterisc, asterisc is expected to be maintained at separate repository. Asterisc pulls the monorepo(here!)
Summary by CodeRabbit
New Features
Asterisc, in the system.Refactor
Bug Fixes
Documentation