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

Add Oracle tests for MIPS #2945

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

martyall
Copy link
Contributor

@martyall martyall commented Jan 10, 2025

This PR consists of two commits

  • d98442c: Add cannon mips tests for oracle reads/writes:
    -- oracle.asm
    -- oracle_kzg.asm
    -- oracle_unaligned_read.asm
    -- oracle_unaligned_write.asm
  • c5095ad: Make modifications to SyscallWritePreimage handler to get tests to pass

The test was adapted from the cannon codebase version. None of these oracle based tests passed with the current implementation. The problem was with the SyscallWritePreimage syscall handler, which if you look at the test logs for d98442c you can see the errors of the form:

[2025-01-13T16:18:51Z DEBUG o1vm::elf_loader] The executable code starts at address 4194512, has size 304 bytes, and ends at address 4194815.
[2025-01-13T16:18:51Z DEBUG test_mips_elf::test_oracle] Asking oracle for key: 000000000000000000000000000000000000000000000000000000004a29921f
thread 'tests::open_mips_tests' panicked at 'Unknown key type prefix: 0', o1vm/tests/test_mips_elf.rs:62:22

If you want to run them locally, you can use the one-liner

> RUST_LOG=debug RUST_BACKTRACE=1 cargo test --features open_mips --test test_mips_elf -- --nocapture

The problem is that the current write handler isn't writing to the appropriate places in the register. I came up with a solution that fixes the problem with the cost of increasing the scratch space and the number of registers. If we're looking to "pay less" for the correct implementation, we can use this as a starting point.

@martyall martyall marked this pull request as ready for review January 10, 2025 21:09
@martyall martyall changed the title Add Oracle tests Add Oracle tests for MIPS Jan 10, 2025
@martyall martyall force-pushed the martin/add-oracle-tests-clean branch 2 times, most recently from 4bb3383 to 0800a90 Compare January 10, 2025 22:12
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 97.59036% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.59%. Comparing base (2136d6c) to head (c5095ad).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
o1vm/src/cannon.rs 0.00% 1 Missing ⚠️
o1vm/src/interpreters/mips/registers.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2945      +/-   ##
==========================================
- Coverage   76.61%   76.59%   -0.02%     
==========================================
  Files         255      255              
  Lines       60855    60791      -64     
==========================================
- Hits        46622    46564      -58     
+ Misses      14233    14227       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martyall martyall force-pushed the martin/add-oracle-tests-clean branch from 0800a90 to c5095ad Compare January 13, 2025 16:20
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.

1 participant