Skip to content

feat: openvm precompile #46

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: openvm precompile #46

wants to merge 7 commits into from

Conversation

lightsing
Copy link
Member

@lightsing lightsing commented Jun 19, 2025

This pr adds openvm precompile support behind a openvm gate.

Some files are copied from revm with almost no modification since revm did expose those as public, should be easy to update.

openvm support are copied from axiom-crypto/revm.

Also refactors some constant definition avoiding magic number.

@frisitano
Copy link
Collaborator

Not a review, just a general comment about the approach. This PR leaks information about the prover into the executor, which, in my opinion, results in a worse separation of concerns. Are we happy to do this?

@lightsing
Copy link
Member Author

Not a review, just a general comment about the approach. This PR leaks information about the prover into the executor, which, in my opinion, results in a worse separation of concerns. Are we happy to do this?

This is necessary to run the prover with openvm precompile circuit.

no matter merge or not, we are need to use this.

@lightsing lightsing marked this pull request as ready for review June 25, 2025 02:12
lispc
lispc previously approved these changes Jun 25, 2025
@lightsing lightsing marked this pull request as draft June 25, 2025 02:16
# Conflicts:
#	src/precompile/blake2.rs
#	src/precompile/bn128.rs
#	src/precompile/hash.rs
#	src/precompile/modexp.rs
@lightsing lightsing marked this pull request as ready for review June 25, 2025 02:19
@Thegaram
Copy link
Contributor

Not a review, just a general comment about the approach. This PR leaks information about the prover into the executor, which, in my opinion, results in a worse separation of concerns. Are we happy to do this?

I had the same concern. @lispc agreed but he said currently it's not easy to do, so to avoid delaying the upgrade, we should merge this, and maybe refactor later.

@Thegaram Thegaram requested review from frisitano and greged93 June 25, 2025 06:16
@frisitano
Copy link
Collaborator

I had the same concern. @lispc agreed but he said currently it's not easy to do, so to avoid delaying the upgrade, we should merge this, and maybe refactor later.

I agree with your proposal. Let's merge as is and then look to refactor in the future. One way we can approach this is by extracting scroll-revm-precompiles to an independent crate and then implementing a patch for it in the prover repos. I'm also not hugely fond of the crate patch pattern, but it is something we can consider in the future.

Copy link
Collaborator

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

Looks good. I'm mindful that this feature is light on testing. Do you have any ideas about how we can test the different implementations (feature flag configurations) to ensure all implementations are consistent and correct?

@lispc
Copy link

lispc commented Jun 25, 2025

oh @frisitano Peter just suggested another solution for this. He suggest inside zk guest program, we just replace the "handler" of Precompiles struct. So using that way, it is more easy to maintain. We are trying that method. So let us hold this PR for a while. Likely we don't need to add the openvm feature and codepath here.

@frisitano
Copy link
Collaborator

oh @frisitano Peter just suggested another solution for this. He suggest inside zk guest program, we just replace the "handler" of Precompiles struct. So using that way, it is more easy to maintain. We are trying that method. So let us hold this PR for a while. Likely we don't need to add the openvm feature and codepath here.

I think this is the cleanest approach, but you may need to make changes to the reth ScrollBlockExecutor to support exposing methods to replace the precompiles. Let me know how you get on.

@lispc
Copy link

lispc commented Jun 25, 2025

oh @frisitano Peter just suggested another solution for this. He suggest inside zk guest program, we just replace the "handler" of Precompiles struct. So using that way, it is more easy to maintain. We are trying that method. So let us hold this PR for a while. Likely we don't need to add the openvm feature and codepath here.

I think this is the cleanest approach, but you may need to make changes to the reth ScrollBlockExecutor to support exposing methods to replace the precompiles. Let me know how you get on.

yes. cc @lightsing

Comment on lines +39 to +43
if input.len() < FQ_LEN {
Err(PrecompileError::Bn128FieldPointNotAMember)
} else {
Fp::from_be_bytes(&input[..32]).ok_or(PrecompileError::Bn128FieldPointNotAMember)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if input.len() < FQ_LEN {
Err(PrecompileError::Bn128FieldPointNotAMember)
} else {
Fp::from_be_bytes(&input[..32]).ok_or(PrecompileError::Bn128FieldPointNotAMember)
}
if input.len() < FQ_LEN {
Err(PrecompileError::Bn128FieldPointNotAMember)
}
Fp::from_be_bytes(&input[..32]).ok_or(PrecompileError::Bn128FieldPointNotAMember)

Bn254::msm(&[fr], &[p])
}

/// pairing_check performs a pairing check on a list of G1 and G2 point pairs and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// pairing_check performs a pairing check on a list of G1 and G2 point pairs and
/// Performs a pairing check on a list of G1 and G2 point pairs and

if pairs.is_empty() {
return true;
}
let (g1_points, g2_points): (Vec<_>, Vec<_>) = pairs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let (g1_points, g2_points): (Vec<_>, Vec<_>) = pairs
let (g1_points, g2_points) = pairs

p * fr
}

/// pairing_check performs a pairing check on a list of G1 and G2 point pairs and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// pairing_check performs a pairing check on a list of G1 and G2 point pairs and
/// Performs a pairing check on a list of G1 and G2 point pairs and

result.into_affine()
}

/// pairing_check performs a pairing check on a list of G1 and G2 point pairs and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// pairing_check performs a pairing check on a list of G1 and G2 point pairs and
/// Performs a pairing check on a list of G1 and G2 point pairs and

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.

5 participants