cannon: Support F_GETFD cmd to fcntl

[ajsutton]

Description

Fixes support for go 1.22+ in cannon single threaded.

MT cannon works with go1.22 but still fails with go 1.23 still because syscall 4325.

Tests

Updated fuzz test expectations.

Metadata

• cannon/op-program: Support building on go1.22 #12049

[codecov]

Codecov Report

Attention: Patch coverage is 37.50000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 74.87%. Comparing base (cd7e9d4) to head (32a6e3d).
Report is 6 commits behind head on develop.

Files with missing lines	Patch %	Lines
cannon/mipsevm/exec/mips_syscalls.go	12.50%	6 Missing and 1 partial ⚠️
cannon/mipsevm/versions/state.go	66.66%	2 Missing ⚠️
cannon/multicannon/exec.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #12050      +/-   ##
===========================================
- Coverage    75.19%   74.87%   -0.33%     
===========================================
  Files           49       49              
  Lines         3656     3665       +9     
===========================================
- Hits          2749     2744       -5     
- Misses         734      748      +14     
  Partials       173      173              

Flag	Coverage Δ
cannon-go-tests	74.87% <37.50%> (-0.33%)	⬇️

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

Files with missing lines	Coverage Δ
cannon/mipsevm/multithreaded/state.go	53.87% <ø> (+0.19%)	⬆️
cannon/mipsevm/singlethreaded/state.go	55.66% <ø> (+0.27%)	⬆️
cannon/mipsevm/versions/detect.go	85.71% <100.00%> (ø)
cannon/multicannon/exec.go	0.00% <0.00%> (ø)
cannon/mipsevm/versions/state.go	52.63% <66.66%> (+1.24%)	⬆️
cannon/mipsevm/exec/mips_syscalls.go	89.40% <12.50%> (-4.35%)	⬇️

... and 1 file with indirect coverage changes

[clabby]

LGTM. The manpage says the only FD flag is FD_CLOEXEC, which is paired up with the execve syscall. Cannon should never support that syscall, anyways, so should be good.

[Inphi]

The Cannon STF change looks good. But we need to create a new StateVersion since we're changing VM behavior.

I've created a PR that allows cannon to support multiple versions - #12072. I suggest rebasing on that and updating the cannon embeds so that we can continue to execute "old" cannon prior to this change.

[semgrep-app]

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

• packages/contracts-bedrock/scripts/DeployImplementations.s.sol
  • L473-476 - Triage
  • L769-772 - Triage

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

_{Ignore this finding from sol-style-input-arg-fmt.}

[ajsutton]

@Inphi I can dig into this but the test failures on this are interesting. We're dropping support for the original singlethreaded state but the tests are trying to dynamically generate JSON states for testing which is no longer supported. I think the answer is to add a testutil that can generate states of specific versions and formats for us and then we can hard code examples of older states (or possibly just fudge it - we're not expecting to be able to actually read the state since its not supported, just need to detect the version).

[ajsutton]

Also I'm inclined to say that we haven't released multithreaded cannon yet so we can just add getfd support for it without a new state version. We'll just copy in the old version for state version 0.

[Inphi]

@Inphi I can dig into this but the test failures on this are interesting. We're dropping support for the original singlethreaded state but the tests are trying to dynamically generate JSON states for testing which is no longer supported. I think the answer is to add a testutil that can generate states of specific versions and formats for us and then we can hard code examples of older states (or possibly just fudge it - we're not expecting to be able to actually read the state since its not supported, just need to detect the version).

@ajsutton We probably don't need to maintain tests for older state versions. The old VM has already been tested against the v0 state. I'd port tests using state files to use the latest states rather than keep the old ones around.

[ajsutton]

@ajsutton We probably don't need to maintain tests for older state versions. The old VM has already been tested against the v0 state. I'd port tests using state files to use the latest states rather than keep the old ones around.

@Inphi I think we need to keep the tests for op-challenger using JSON states and for detecting type 0 in Multicannon since that's important value. But agreed we can keep it focussed on that detection which should make it much easier and all the other tests focus on the new version.

[Inphi]

Right that was the last thing I forgot to deal with. The op-challenger's using cannon packages. I need to take a look to see if we can get rid of that.

[Inphi]

So the cannon state codec needs to be backwards compatible to support this op-challenger use - https://github.com/ethereum-optimism/optimism/blob/develop/op-challenger/game/fault/trace/cannon/state_converter.go#L37.
OTOH, we have the cannon witness subcommand which we never use. Could the op-challenger use that instead to get the info it needs?

[ajsutton]

ugh, that's a good point actually - op-challenger is actually parsing the state. For some of what it does it could use the witness command (and I have a patch around to do that) but it also needs to extract things like the last step. I think witness can convert a state to a proof which should be enough. I'll add it onto the yak shaving pile to get this merged... I have been quite keen to get all knowledge of cannon states out of challenger for a while so this is a forcing function.

[ajsutton]

hmm, actually I don't seem to have the final step done, but it should be implementing a StateConverter implementation that executes the witness subcommand.

[ajsutton]

And witness can't even generate a proof so we'll need to implement that because one of the reasons that challenger parses the state is to get the proof at the final step and it needs to actually get the step out, not just the claim in that case.

[ajsutton]

Converted to draft again until the compatibility issues are sorted. Its getting there, just a few more hairy yaks left....

[ajsutton]

I think what's left is:

• cannon: Output information about the state in JSON format from the witness command #12137
• op-challenger: Use witness subcommand instead of parsing cannon states #12141
• tagging a new cannon release to use to support the existing STF
• resolving all those conflicts
• updating the version detection tests to use a test utility that creates a file that looks enough like the old versions without actually having to still have the code for those old versions around.

[ajsutton]

@Inphi @clabby I've gone with calling the new version singlethreaded-2. I'm open to just calling all versions by just the state version number, but it felt useful to keep the singlethreaded/multithreaded information in the naming. I think the trailing number should always be the state version number even if the singlethreaded-* numbers then aren't sequential. ie we'll likely add multithreaded-3 next and then might have singlethreaded-4 etc.

If we do just use the StateVersion number we still need some way to name the constants in the code which is a big part of what drove me to make it singlethreaded-2.

[Inphi]

LGTM!

Flagging again that this breaks the recurring cannon-stf-job. This is what we want, but the job will notify @proofs-squad when it fails. There's a chicken-and-egg problem of needing a new cannon docker image to verify develop against the version 2 STF. So I'm fine with leaving cannon-stf-verify broken until we can fix.
We can ignore the notification or disable that job for a bit. Either works.

[ajsutton]

Ah thanks I'd forgotten about that job. I might disable it as part of this PR since we'll need to get a PR up and approved to fix it and we don't need to be notifying everyone in the mean time.

[ajsutton]

hmm, looking at the config I actually don't think it will notify us - it's a dependency of something that would, but that just means the next job won't run at all and so nothing notifies. That's worth testing to make sure we actually get notifications. But I'll merge this tomorrow when I have more time to get a fix up (or feel free to take over in whatever way you think is best if you prefer).

[Inphi]

Merging now. I'll fix the notifications alongside adding cannon-2 to cannon-stf-verify.