Skip to content

feat: Use bytes API for underlying precompile library APIs#2705

Merged
rakita merged 24 commits intobluealloy:mainfrom
kevaundray:feat/precompile-bytes-api
Jul 21, 2025
Merged

feat: Use bytes API for underlying precompile library APIs#2705
rakita merged 24 commits intobluealloy:mainfrom
kevaundray:feat/precompile-bytes-api

Conversation

@kevaundray
Copy link
Contributor

@kevaundray kevaundray commented Jul 14, 2025

Motivation

Using revm inside of a zkVM involves patching certain cryptographic libraries using patch-crates.io -- this is quite brittle because one needs to ensure that the versions match up and long term we would like these to be patched using extern C.

One could implement extern C methods under a feature flag, however the more general strategy would be to have a pluggable architecture like alloy-rs/alloy#2634 -- This PR is a precursor to adding the pluggable architecture since the functions that are being plugged in shouldn't rely on the types of a particular library.

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 14, 2025

CodSpeed Performance Report

Merging #2705 will not alter performance

Comparing kevaundray:feat/precompile-bytes-api (7d3e169) with main (a494e38)

Summary

✅ 160 untouched benchmarks

@kevaundray
Copy link
Contributor Author

Pulled out of #2675

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

left few nits

let mut scalars = Vec::with_capacity(k);
let mut point_scalar_pairs: Vec<(G1Point, [u8; SCALAR_LENGTH])> = Vec::with_capacity(k);

for i in 0..k {
Copy link
Member

Choose a reason for hiding this comment

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

Would remove this for loop, the only thing that we do here is removal of the padding and adding creating new vec. And inside crypto provider you would again create an additional vec to transform the fields.

We can transmute it and send it padded and leave crypto provider to do it in the loop:

    let points = unsafe {
        mem::transmute::<&[u8], &[([u8; PADDED_G1_LENGTH], [u8; SCALAR_LENGTH])]>(&input)
    };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had initially done this to have a create a clean abstraction since the padding is an EVM specific operation and the cryptography modules don't have any context about the EVM -- If we can't avoid the extra allocation, I can use a transmute and remove padding in the cryptography module. Perhaps we could use an iterator?

Copy link
Contributor Author

@kevaundray kevaundray Jul 19, 2025

Choose a reason for hiding this comment

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

Removed the initial allocation here 7d3e169 -- what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have also rebased this ontop of #2706 to show what it looks like with CryptoProvider

Copy link
Member

Choose a reason for hiding this comment

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

Iterators are lazy so it makes sense that allocation is skipped here, it looks great. Would need to check just to be sure, but can do that later.

kevaundray and others added 7 commits July 19, 2025 20:07
Co-authored-by: rakita <rakita@users.noreply.github.com>
Co-authored-by: rakita <rakita@users.noreply.github.com>
Co-authored-by: rakita <rakita@users.noreply.github.com>
Co-authored-by: rakita <rakita@users.noreply.github.com>
/// Verify KZG proof.
#[inline]
pub fn verify_kzg_proof(commitment: &Bytes48, z: &Bytes32, y: &Bytes32, proof: &Bytes48) -> bool {
pub fn verify_kzg_proof(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removes the library specific types from the public API

@kevaundray kevaundray changed the title chore(experiment): Use bytes API for underlying precompile library APIs feat: Use bytes API for underlying precompile library APIs Jul 19, 2025
@kevaundray kevaundray marked this pull request as ready for review July 21, 2025 09:50
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@rakita rakita merged commit 066fc30 into bluealloy:main Jul 21, 2025
29 checks passed
This was referenced Jul 21, 2025
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