Skip to content

feat: RISCOF signature extraction for RISC-V architecture tests#52

Closed
codygunton wants to merge 3 commits intobrevis-network:mainfrom
codygunton:riscof
Closed

feat: RISCOF signature extraction for RISC-V architecture tests#52
codygunton wants to merge 3 commits intobrevis-network:mainfrom
codygunton:riscof

Conversation

@codygunton
Copy link

@codygunton codygunton commented Oct 16, 2025

The RISCOF framework is used for differential testing of RISC-V implementations against reference emulators such as the formally specified Sail model. The dashboard here tracks compliance using the canonical RISC-V architecture tests, executed via RISCOF, and in time we may add additional tests in a custom test suite.

In the RISCOF approach, the target implementation ("DUT") and the reference both dump a region of memory to a file (the "signatures") for a direct value-by-value comparison. This pull request adds the signature extraction mechanism for Pico.

Pico is compliant for every rv32im opcode except fence. This indicates a very high degree of compliance, for instance, the add test suite alone addresses over 500 edge cases. The fences must be unneeded due to the presence of only a single hart, but but they are apparently not properly treated as nops. This should not affect proving in most cases (cf the recent progress on RTP!), but it does complicate the use of certain tools such as fuzzers that may emit such instructions.

If this PR is accepted, the dashboard will be updated to run a nightly job looking for updates to the main branch, and running the tests in the case of a new commit. This is useful for catching regressions, and also for tracking any ISA changes that may occur.

@codygunton
Copy link
Author

Thanks @eason1981 for running my workflow. It looks like everything is fine except a linter error that I have fixed. This means I didn't break anything, but it also doesn't show my new work is correct, since I didn't add any tests of the new API endpoint. Is that alright? I think breakages should be rare, but ofc if you'd like help integrating into your own CI, I'd be happy to consult.

@eason1981
Copy link
Contributor

Thanks @eason1981 for running my workflow. It looks like everything is fine except a linter error that I have fixed.

Thanks for adding this sub command to pico-sdk CLI and fixing lint!

This means I didn't break anything, but it also doesn't show my new work is correct, since I didn't add any tests of the new API endpoint. Is that alright? I think breakages should be rare, but ofc if you'd like help integrating into your own CI, I'd be happy to consult.

I looked through the code, it looks great and shouldn’t break anything.
I suppose we could try running this new subcommand to generate the signatures, and then use the riscof CLI to produce the report in CI.
Could you please share the full command you used for running riscof with the generated signatures?
Or, do we have a better way to integrate it into the GitHub workflow? Thanks!

@codygunton
Copy link
Author

codygunton commented Oct 18, 2025

Great! I tried to make this RISCOF testing setup super portable and reproducible here: https://github.com/eth-act/zkevm-test-monitor. You should be able to execute ./run build pico and ./run test --arch pico. At the very least, this documents the RISCOF setup. The github actions that run this nightly for ZisK and Jolt are also available in there as a starting point.

Copy link
Contributor

@eason1981 eason1981 left a comment

Choose a reason for hiding this comment

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

LGTM

@codygunton
Copy link
Author

Is there anything more I can do to help this get merged?

@eason1981
Copy link
Contributor

Is there anything more I can do to help this get merged?

I believe that’s all for now. I’ll ping @succinctli for another review.

@codygunton
Copy link
Author

Will you please merge this?

@kaiwei-0
Copy link
Contributor

kaiwei-0 commented Dec 2, 2025

Will you please merge this?

I’ve just reviewed it and it looks good to me. The branch is a bit outdated, so I’ll update it with the latest changes and then proceed with merging.

@kaiwei-0
Copy link
Contributor

kaiwei-0 commented Dec 2, 2025

I’ve opened a follow-up PR here that rebases these changes on top of the latest pico (v1.1.9). @codygunton

@codygunton
Copy link
Author

The follow-up that @kaiwei-0 linked has been merged 🙏

@codygunton codygunton closed this Dec 2, 2025
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