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

bn254_syscalls #828

Merged
merged 29 commits into from
Feb 27, 2025
Merged

Conversation

mcalancea
Copy link
Collaborator

@mcalancea mcalancea commented Feb 7, 2025

Adds interface for precompiles pertaining to the bn254 elliptic curve. Mostly follows the structure of the existing precompiles.

Caveats:

  • This PR changes the mock ecall circuit to accept arbitrary memory addresses, as opposed to consecutive addresses as it does now. As discussed with @naure, this requires more attention; but I think the review can happen in parallel
  • The guest examples use debug messages for testing purposes. Debug messages are currently incompatible with proving. I'm trying to make the logging conditional via a flag in the input, but I need to debug some weird behavior.

@mcalancea mcalancea force-pushed the mihai/bn254-syscalls branch 3 times, most recently from 55df130 to fbf16d3 Compare February 10, 2025 14:38
@mcalancea mcalancea force-pushed the mihai/bn254-syscalls branch from fbf16d3 to a2f3d4e Compare February 10, 2025 14:40
@mcalancea mcalancea requested a review from naure February 10, 2025 14:54
@mcalancea mcalancea marked this pull request as ready for review February 10, 2025 14:54
@mcalancea mcalancea changed the title bn254_syscalls (wip) bn254_syscalls Feb 10, 2025
Copy link
Collaborator

@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!

See start_addr comment.

WriteMEM::construct_circuit(
cb,
start_addr.expr() + (i * WORD_SIZE) as u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The witness start_address could be removed, right?

bn254_fp2_add_config:
<LargeEcallDummy<E, Bn254Fp2AddSpec> as Instruction<E>>::InstructionConfig,
bn254_fp2_mul_config:
<LargeEcallDummy<E, Bn254Fp2MulSpec> as Instruction<E>>::InstructionConfig,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: Eventually the ecall circuit can be adapted to have a single circuit for all ecalls, or one circuit per common shape of arguments (1 or 2 pointers). That will be more efficient to verify.

(Not for this PR)

@@ -59,15 +58,15 @@ impl<E: ExtensionField, S: SyscallSpec> Instruction<E> for LargeEcallDummy<E, S>
.map(|i| {
let val_before = cb.create_witin(|| format!("mem_before_{}", i));
let val_after = cb.create_witin(|| format!("mem_after_{}", i));

let addr = cb.create_witin(|| format!("addr_{}", i));
Copy link
Collaborator

Choose a reason for hiding this comment

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

In principle it could also be two witnesses for the two pointer arguments (e.g. p and q) and then the consecutive addresses of each one.

(Not required in this PR)


fn main() {
let log_flag = true;
let log_state = |state: &[u32]| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool technique.

F: From<[Word; WORDS]>
+ Into<[Word; WORDS]>
+ std::ops::Add<Output = F>
+ std::ops::Mul<Output = F>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice factoring.

@mcalancea mcalancea added this pull request to the merge queue Feb 27, 2025
Merged via the queue into scroll-tech:master with commit cd38cee Feb 27, 2025
4 checks passed
@mcalancea mcalancea deleted the mihai/bn254-syscalls branch February 27, 2025 10:12
@mcalancea mcalancea linked an issue Mar 4, 2025 that may be closed by this pull request
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.

[Interface] bn254 curve
2 participants