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

cannon: Multi VM executor #12072

Merged
merged 20 commits into from
Sep 25, 2024
Merged

cannon: Multi VM executor #12072

merged 20 commits into from
Sep 25, 2024

Conversation

Inphi
Copy link
Contributor

@Inphi Inphi commented Sep 23, 2024

image

Multicannon is a cannon wrapper that can be used to load and run Cannon states using a variety of Cannon FPVM implementations. This solves the issue of supporting older versions of the Cannon FPVM STF, without introducing branching logic into the STF. See ethereum-optimism/design-docs#88 for more details on the rationale.

Multicannon mimics the same CLI as regular cannon, and in a couple subcommands, requires an additional --version flag to be specified where it cannot autodetect the VM implementation to use. The --version flag is used to locate the appropriate cannon FPVM program to use.

The run subcommand autodetects the appropriate VM implementation to use by inspecting the input state file. The StateVersion in the state file indicates the Cannon VM implementation to use.

Multicannon also supports a new list subcommand that displays the embedded cannon VMs and their versions.

Annoyances

An embedded cannon binary must exist for every version supported by multicannon. This is wasteful in the case of single-threaded and multi-threaded VMs as a single cannon VM implementation supports both using the --type flag.

Adding additional Cannon VMs

Easiest way to do this is using the op-stack-go Dockerfile. Whenever the Cannon VM STF is altered, a new StateVersion must be created. A Github tag can then be created for the change. This tag can be referenced when building the op-stack-go docker image to build an older version of cannon into the cannon/multicannon/embeds directory. When multicannon is built, it becomes aware of all cannon VM binaries found in the embeds directory.

cannon/multicannon/exec.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 31.75182% with 187 lines in your changes missing coverage. Please review.

Project coverage is 68.14%. Comparing base (9178d5f) to head (77d6d37).
Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
cannon/multicannon/exec.go 0.00% 51 Missing ⚠️
cannon/multicannon/list.go 0.00% 36 Missing ⚠️
cannon/multicannon/main.go 0.00% 21 Missing ⚠️
cannon/multicannon/run.go 0.00% 16 Missing ⚠️
cannon/multicannon/load_elf.go 0.00% 15 Missing ⚠️
cannon/multicannon/witness.go 0.00% 15 Missing ⚠️
cannon/multicannon/util.go 45.45% 10 Missing and 2 partials ⚠️
cannon/mipsevm/versions/state.go 37.50% 10 Missing ⚠️
cannon/cmd/load_elf.go 69.23% 8 Missing ⚠️
cannon/mipsevm/versions/detect.go 85.71% 2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (9178d5f) and HEAD (77d6d37). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (9178d5f) HEAD (77d6d37)
cannon-go-tests 2 1
Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #12072       +/-   ##
============================================
- Coverage    79.10%   68.14%   -10.97%     
============================================
  Files           41       54       +13     
  Lines         3437     4119      +682     
============================================
+ Hits          2719     2807       +88     
- Misses         548     1134      +586     
- Partials       170      178        +8     
Flag Coverage Δ
cannon-go-tests 68.14% <31.75%> (-10.97%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cannon/cmd/run.go 7.84% <100.00%> (ø)
cannon/cmd/witness.go 44.00% <100.00%> (ø)
cannon/mipsevm/versions/detect.go 85.71% <85.71%> (ø)
cannon/cmd/load_elf.go 26.38% <69.23%> (ø)
cannon/mipsevm/versions/state.go 59.72% <37.50%> (-6.35%) ⬇️
cannon/multicannon/util.go 45.45% <45.45%> (ø)
cannon/multicannon/load_elf.go 0.00% <0.00%> (ø)
cannon/multicannon/witness.go 0.00% <0.00%> (ø)
cannon/multicannon/run.go 0.00% <0.00%> (ø)
cannon/multicannon/main.go 0.00% <0.00%> (ø)
... and 2 more

... and 3 files with indirect coverage changes

cannon/multicannon/list.go Fixed Show fixed Hide fixed
@Inphi Inphi requested review from ajsutton and mbaxter September 24, 2024 14:44
@Inphi Inphi marked this pull request as ready for review September 24, 2024 14:44
@Inphi Inphi requested review from a team as code owners September 24, 2024 14:44
Copy link
Contributor

semgrep-app bot commented Sep 24, 2024

Semgrep found 1 sol-style-return-arg-fmt finding:

  • packages/contracts-bedrock/src/libraries/Blueprint.sol

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

Copy link
Contributor

@mbaxter mbaxter left a comment

Choose a reason for hiding this comment

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

looks good! just a few questions

cannon/multicannon/util.go Outdated Show resolved Hide resolved
cannon/mipsevm/versions/detect.go Show resolved Hide resolved
cannon/multicannon/load_elf.go Outdated Show resolved Hide resolved
cannon/multicannon/util.go Outdated Show resolved Hide resolved
cannon/multicannon/witness.go Outdated Show resolved Hide resolved
@Inphi Inphi requested a review from mbaxter September 24, 2024 17:12
cannon/cmd/load_elf.go Outdated Show resolved Hide resolved
cannon/multicannon/list.go Show resolved Hide resolved
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM generally. I suspect we should just skip the .git folder access in docker for now since we aren't (yet) supporting old versions and can revisit how best to do that. And it's probably worth following up to work out if we can be more flexible with CLI arg changes but it won't be an issue for now.

Overall I think this is a useful tool in the toolbox and enables the really neat 64bit migration approach you came up with so worth having. I'm not sure it should always be the approach we use for compatibility to be honest and a fork in the STF might be a better approach in simple cases (just like we support multiple hard forks in op-node). At the moment the two approaches are a bit mutually exclusive since we need a separate binary for every state version but we could investigate ways to avoid that in a follow up if we need to.

The thing that makes me nervous about just using a fixed old version for backwards compatibility is if there's some reason we do need to make changes to it like fixing a bug, tweaking a CLI arg etc that doesn't affect the state outcome but is needed for some reason. We could potentially create a fork to do the fix on but it gets messy. I think we can work to improve things over time though if we need to - right now this is a really good way to unblock the GET_FD change and the 64bit migration.

cannon/mipsevm/versions/detect.go Show resolved Hide resolved
cannon/multicannon/exec.go Show resolved Hide resolved
cannon/multicannon/run.go Outdated Show resolved Hide resolved
ops/docker/op-stack-go/Dockerfile.dockerignore Outdated Show resolved Hide resolved
Copy link
Contributor

semgrep-app bot commented Sep 25, 2024

Semgrep found 2 sol-style-input-arg-fmt findings:

Inputs to functions must be prepended with an underscore (_)

Ignore this finding from sol-style-input-arg-fmt.

@Inphi
Copy link
Contributor Author

Inphi commented Sep 25, 2024

Merging as soon as CI passes.

@Inphi Inphi enabled auto-merge September 25, 2024 04:33
@Inphi Inphi added this pull request to the merge queue Sep 25, 2024
Merged via the queue into develop with commit 56502dd Sep 25, 2024
66 checks passed
@Inphi Inphi deleted the inphi/multicannon branch September 25, 2024 04:43
This was referenced Sep 26, 2024
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
* cannon: Multi VM executor

* fix run subcmd arg fwding

* fix mt prestate

* add list subcmd; multicannon in op-stack-go

* remove cannon-latest

* safer strconv

* lint

* include .gitkeep in embed

* fix .git copy

* add detect.go tests

* add nosemgrep

* review comments

* list filtering

* add note to MIPS.sol in version stf ref

* use fork-exec

* minimal flag parsing

* load old cannon binaries from docker images

* note

* --help flag defaults

* remove redundant copy from cannon-builder-0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants