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

Improve legibility and methodology for testing opcode circuits #331

Open
bgillesp opened this issue Oct 7, 2024 · 4 comments
Open

Improve legibility and methodology for testing opcode circuits #331

bgillesp opened this issue Oct 7, 2024 · 4 comments

Comments

@bgillesp
Copy link
Collaborator

bgillesp commented Oct 7, 2024

Currently, RISC-V opcode circuits are tested individually in a mock prover using hand-crafted operations added to ceno_zkvm/src/scheme/mock_prover.rs, and (sometimes) using the real prover in a single example program at ceno_zkvm/examples/riscv_opcodes.rs. The methodology for implementing this testing could likely use some improvements in terms of:

  • Legibility, e.g. providing a more usable interface for specifying and implementing tests for circuits
  • Coverage, e.g. ensuring that all implemented RISC-V functionality is tested in both a mock prover and a real example program
@bgillesp
Copy link
Collaborator Author

bgillesp commented Oct 7, 2024

See also comments by @matthiasgoergens posted here

@naure
Copy link
Collaborator

naure commented Oct 10, 2024

The point is that this is how the spec is written. So we can tell the exact data being tested without intermediate code.

But yes it is getting out of hand.

  • We could add convenience constructors to DecodedInstruction for instance. Then do include tests for the test utilities tying it back to the raw values from the spec.
  • The constants in mock_prover.rs could be replaced by some functions that give the instruction and the PC together.

image

@KimiWu123
Copy link
Contributor

KimiWu123 commented Oct 15, 2024

I submitted a short PR #395 to address the constants in mock_prover.rs, and would like to raise some discussion and hear your feedback. If this PR makes sense to you, I'll keep working on all the opcode tests.

cc. @hero78119

KimiWu123 added a commit that referenced this issue Oct 18, 2024
This PR is trying to remove `MOCK_PROGRAM` and `MOCK_PC_XXX` in
`MockProver`, related to part of #331.

The idea is to 
1. move `load_program_table` out from `load_table` so that we can pass
any opcodes we would like to test, and
2. leverage existing `DecodedInstruction::from_raw()` to reassemble the
opcode format for `MockProver`.

---------

Co-authored-by: naure <[email protected]>
@matthiasgoergens
Copy link
Collaborator

The point is that this is how the spec is written. So we can tell the exact data being tested without intermediate code.

If you want to test against the spec like that, you should just use the official RiscV tests.

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

No branches or pull requests

4 participants