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

secp256k1 precompiles: add, double and decompress + refactor #4

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

mcalancea
Copy link

@mcalancea mcalancea commented Jan 14, 2025

Note: this PR is done against Inversed's fork master, do not merge.

  • adds secp256k1_add, secp256k1_double and secp256k1_decompress precompiles with the same interface as sp1, but backed by the secp crate.
  • integrates appropriate dummy circuits for precompiles into the zkvm
  • added utils.rs with some utilities for manipulating memory segments of the VM.
  • tested the compatibility of the syscalls (+ keccak, done previously) with sp1 (see the syscalls.rs example file).
  • extended the docs pasted from sp1

Questions:

  • sp1 does one additional check for syscalls that only take one argument, namely that the second argument/register is 0. I haven't done this yet; do we want to? Or can we be more permissive?
  • sp1 does generally require alignment of the pointers along a 4-byte boundary. I think it may be helpful to require stricter alignment for arrays of larger types (for example for keccak which inputs u64s, it could be aligned to 8 bytes).

@mcalancea mcalancea requested a review from naure January 14, 2025 15:26
@mcalancea mcalancea force-pushed the mihai/secp256k1-add-double-syscalls branch 3 times, most recently from d59fdd3 to efb901d Compare January 17, 2025 16:55
@mcalancea mcalancea force-pushed the mihai/secp256k1-add-double-syscalls branch from efb901d to 0e168a9 Compare January 22, 2025 08:05
@mcalancea mcalancea force-pushed the mihai/secp256k1-add-double-syscalls branch 2 times, most recently from 2912a73 to b189845 Compare January 22, 2025 18:38
@mcalancea mcalancea force-pushed the mihai/secp256k1-add-double-syscalls branch from b189845 to 3df34fe Compare January 23, 2025 06:55
@mcalancea mcalancea changed the title secp256k1_add secp256k1 precompiles: add, double and decompress + refactor Jan 23, 2025
@naure
Copy link

naure commented Jan 30, 2025

the second argument/register is 0

For the purpose of compatibility and avoiding surprises I think we should do that too, with a comment to remember that it is the only reason.

alignment of the pointers along a 4-byte boundary

Ceno memory system works with 4-bytes words on 4-bytes alignment; there are no benefits with 8.

Copy link

@naure naure left a comment

Choose a reason for hiding this comment

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

Great work with batteries included!

  • Witgen & Runtime integration
  • Refactoring & Clarity
  • Specs & Refs & Comments
  • Tests & Examples & Compatibility

➜ I think there are some memory ops missing, see comments.

ceno_emul/src/syscalls/secp256k1.rs Outdated Show resolved Hide resolved
ceno_emul/src/syscalls/secp256k1.rs Outdated Show resolved Hide resolved
ceno_zkvm/src/instructions/riscv/dummy/dummy_ecall.rs Outdated Show resolved Hide resolved
ceno_zkvm/src/instructions/riscv/rv32im.rs Show resolved Hide resolved
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.

2 participants