Skip to content

Comments

cannon: Remove support for singlethreaded and 32-bit multithreaded versions#15443

Merged
ajsutton merged 31 commits intodevelopfrom
aj/remove-old-cannon
Apr 29, 2025
Merged

cannon: Remove support for singlethreaded and 32-bit multithreaded versions#15443
ajsutton merged 31 commits intodevelopfrom
aj/remove-old-cannon

Conversation

@ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Apr 17, 2025

Description

Governance has now approved 64 bit cannon so it should be used for all deployments going forward and we can remove support for the single threaded and 32-bit multithreaded versions. This is a pretty brute force removal - all the build tags are removed (though things like the Word alias are preserved). Lots of things still have 64 and multithreaded in the name which isn't really necessary anymore. We can choose if we want to do all those renames in follow up PRs.

Builds on #15487

Tests

Removed lots.

Metadata
Part of #13447

@codecov
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 45.95%. Comparing base (24080ea) to head (aa00e6a).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
cannon/mipsevm/testutil/evm.go 20.00% 3 Missing and 1 partial ⚠️
cannon/cmd/load_elf.go 0.00% 1 Missing ⚠️
cannon/mipsevm/tests/helpers.go 50.00% 1 Missing ⚠️
cannon/mipsevm/versions/state.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #15443      +/-   ##
===========================================
- Coverage    46.76%   45.95%   -0.81%     
===========================================
  Files         1286     1279       -7     
  Lines       106661   105478    -1183     
===========================================
- Hits         49875    48474    -1401     
- Misses       53262    53515     +253     
+ Partials      3524     3489      -35     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 64.87% <56.25%> (+7.31%) ⬆️
contracts-bedrock-tests 86.74% <ø> (-7.55%) ⬇️

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

Files with missing lines Coverage Δ
cannon/mipsevm/arch/arch64.go 100.00% <ø> (ø)
cannon/mipsevm/program/patch.go 93.75% <ø> (+26.58%) ⬆️
cannon/mipsevm/testutil/arch.go 65.71% <ø> (-6.38%) ⬇️
cannon/mipsevm/testutil/elf.go 100.00% <100.00%> (+19.04%) ⬆️
cannon/mipsevm/testutil/vmtests.go 100.00% <100.00%> (+9.00%) ⬆️
cannon/mipsevm/versions/version.go 80.00% <ø> (-1.49%) ⬇️
cannon/cmd/load_elf.go 0.00% <0.00%> (ø)
cannon/mipsevm/tests/helpers.go 81.66% <50.00%> (-2.65%) ⬇️
cannon/mipsevm/versions/state.go 52.63% <66.66%> (-8.37%) ⬇️
cannon/mipsevm/testutil/evm.go 69.49% <20.00%> (-1.10%) ⬇️

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ajsutton ajsutton force-pushed the aj/remove-old-cannon branch from dce24f2 to 2f55f1a Compare April 17, 2025 04:50
@ajsutton ajsutton force-pushed the aj/remove-old-cannon branch from 55e93a2 to 2178914 Compare April 17, 2025 20:51
@ajsutton ajsutton force-pushed the aj/remove-old-cannon branch 2 times, most recently from c4049bf to 7d8239a Compare April 22, 2025 01:20
@ajsutton
Copy link
Contributor Author

@mbaxter I think I've come unstuck on this one. It's failing in the contract tests (https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/87722/workflows/9d9cfc81-5c44-4ee6-953e-14234977d6b0/jobs/3455600) because of a memory alignment problem. I think that's because it's running MIPS2.t.sol which is the 32-bit cannon implementation but the FFI calls are winding up calling into 64-bit cannon code now (whereas previously it was 32-bit). I'm not really sure why they only started failing now - maybe because I rebased...

Any chance you could give me some hints? We could extend this to remove the 32-bit solidity implementations as well but we don't have a MIPS64.t.sol so not sure if we need to migrate some more solidity tests over to 64 bit to maintain coverage.

@mbaxter
Copy link
Contributor

mbaxter commented Apr 22, 2025

I think that's because it's running MIPS2.t.sol which is the 32-bit cannon implementation but the FFI calls are winding up calling into 64-bit cannon code now (whereas previously it was 32-bit).

Yeah - looks like the go-ffi script depends on the cannon64 build tags. Since we've removed the build tags, the 32-bit tests are failing because they are now using the 64-bit logic. I think we didn't see these earlier because the bedrock-go-tests were blocked because of other failing tests.

Looks like we will have to go ahead and remove the 32-bit solidity MIPS tests, which probably mean we should just go ahead and cut out the 32-bit contracts here as well.

we don't have a MIPS64.t.sol so not sure if we need to migrate some more solidity tests

We moved away from solidity tests because they're hard to work with and provide less coverage (no way to do differential testing in solidity). These tests have been implemented as differential tests in cannon/mipsevm/tests. So, there's no need to migrate anything else - everything should be covered there.

@ajsutton ajsutton force-pushed the aj/remove-old-cannon branch from de9d230 to 6c3e3ca Compare April 23, 2025 00:53
@ajsutton ajsutton changed the base branch from develop to mbax/cannon-solidity-feature-toggles April 23, 2025 03:14
@ajsutton
Copy link
Contributor Author

Ok, got this passing everything by deleting the 32-bit solidity implementations and their tests as well and changing mipsVersion from 1 to 6 in about a bajillion places. We're going to need to follow up with some way to avoid having such an insane number of places hard code the latest mips version, but I think this PR is already way too big. The test failures show very clearly what needs to be updated when the state version updates, it's just time consuming to go around them all.

Base automatically changed from mbax/cannon-solidity-feature-toggles to develop April 23, 2025 17:09
@ajsutton ajsutton force-pushed the aj/remove-old-cannon branch from 6a4524f to 64800e1 Compare April 23, 2025 22:11
@ajsutton
Copy link
Contributor Author

The failing tests are all ones that connect to an external node and something seems to have changed with that node causing failures. Getting similar failures on develop as well: https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/87917/workflows/b9c2ee0b-e389-4d01-92ca-8513fea99541/jobs/3462290

@ajsutton ajsutton marked this pull request as ready for review April 23, 2025 22:37
@ajsutton ajsutton requested review from a team as code owners April 23, 2025 22:37
@ajsutton ajsutton requested review from mbaxter and mds1 April 23, 2025 22:37
@mbaxter
Copy link
Contributor

mbaxter commented Apr 28, 2025

lgtm - if it's helpful, I rebased and pushed a few more small fixes here.

@ajsutton ajsutton force-pushed the aj/remove-old-cannon branch from 64800e1 to e14b4c5 Compare April 28, 2025 20:12
Copy link
Contributor

@axelKingsley axelKingsley left a comment

Choose a reason for hiding this comment

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

LGTM, I focused on the circle ci file, as the rest has had attention from the proofs team. Left one question but otherwise G2G.

@ajsutton ajsutton force-pushed the aj/remove-old-cannon branch from 622c7af to aa00e6a Compare April 28, 2025 23:26
@ajsutton ajsutton enabled auto-merge April 28, 2025 23:27
@ajsutton ajsutton added this pull request to the merge queue Apr 28, 2025
Merged via the queue into develop with commit 69d4677 Apr 29, 2025
51 checks passed
@ajsutton ajsutton deleted the aj/remove-old-cannon branch April 29, 2025 00:04
iquidus pushed a commit to Layr-Labs/optimism that referenced this pull request Jul 24, 2025
…rsions (ethereum-optimism#15443)

* cannon: Remove support for singlethreaded and 32-bit multithreaded cannon versions.

* Update build-prestates.sh to handle some versions not having a cannon32 variant.

* Stop building 32 bit prestate

* op-e2e: Remove st cannon tests.

Switch default to cannon MT.

* cannon: Remove cannon stf diff check for singlethreaded vm.

There is no singlethreaded vm in develop anymore so no need to check its compatible.

* Don't copy out st cannon meta.json

* Stop analysis op-program for 32-bit compatibility.

* Remove mipsevm tests. They were only run for 32bit.

* op-e2e: Always use cannon mt prestates and variant.

* cannon: There are no states that can be JSON marshalled.

* cannon: Remove go patching.

* ci: Remove build step for MIPS tests.

* cannon: Remove conditional build comment, restore test helper methods.

* ci: Only run cannon lint and test for 64 bit.

* Remove RunVMTests_OpenMips - the OpenMIPS tests were removed and this test is always skipped on 64-bit.

* contracts: Only build single ffi - no need for a separate cannon64 build.

* cannon: Remove 32-bit solidity implementations.

* cannon: Load MIPS64 abi

* cannon: Remove support for 32bit vms when loading artifacts for tests.

* cannon: Update deployment scripts to only support cannon MT.

* cannon: Update mips version in a lot of places.

* Fix another mipsVersion

* Format

* Fix more mipsVersions

* Fix test

* Fix mipsVersion for TestImplementations

* Cut cannon64 --tags

* Don't build op-program for mips32

* Simplify cannon diff task - assume 64-bit

* op-program: Don't copy 32-bit elf

* op-e2e: Use cannon64 as default VM.
Sanitize op-program 64-bit.

---------

Co-authored-by: mbaxter <meredith@oplabs.co>
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