Skip to content

refactor: guest bindings #1613

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 56 commits into
base: release-v1.2.1-rc.0
Choose a base branch
from

Conversation

Avaneesh-axiom
Copy link
Contributor

@Avaneesh-axiom Avaneesh-axiom commented Apr 30, 2025

This PR removes parts of the guest libraries for each extension. These libraries are to be moved into a new repo as part of the guest library re-org.

Summary of changes for each extension:

  • algebra: minor changes
  • ecc: the re-export of halo2curves was removed, also minor changes
  • pairing: the traits, the halo2curves_shims module, along with a minimal set of constants remain in the openvm repo, while the bn254 and bls12_381 implementations were moved out
  • bigint: the U256 and I256 structs were deleted. Only Rust bindings remain.
  • sha256: the sha256 and set_sha256 functions were deleted, leaving behind only the Rust binding
  • keccak256: the keccak256 and set_keccak256 functions were deleted, leaving behind only the Rust binding

Notes:

  • the k256 and p256 modules of the ecc extension were not deleted as they are useful for testing the ecc macros and traits. For this reason, the ecc tests were also not deleted
  • the algebra tests were also not deleted for the same reason
  • the mod_sqrt implementation was moved from the ecc extension to the algebra extension

Benchmarks and examples are now outdated since they depend on guest libraries that no longer exist. These need to be redesigned and rewritten to use the new guest libraries. I wasn't able to easily port the benchmarks and examples because depending on the new guest libraries creates weird linker issues (I'm guessing due to double-importing openvm).

TODO:

  • switch all deps to main after merge
  • uncomment ecdsa test

Closes INT-3788

@Avaneesh-axiom Avaneesh-axiom force-pushed the refactor/guest-library-reorg-final branch 5 times, most recently from 1027e21 to 88bf100 Compare May 1, 2025 01:33
@Avaneesh-axiom Avaneesh-axiom marked this pull request as ready for review May 1, 2025 03:48
@Avaneesh-axiom Avaneesh-axiom force-pushed the refactor/guest-library-reorg-final branch 8 times, most recently from 56207ac to 63931ab Compare May 7, 2025 20:07
@jonathanpwang jonathanpwang force-pushed the feat/binding-primitive-libs branch from 2a47052 to c9c0989 Compare May 8, 2025 06:46
@jonathanpwang jonathanpwang force-pushed the refactor/guest-library-reorg-final branch from 63931ab to 6fbc163 Compare May 8, 2025 06:59
@Avaneesh-axiom Avaneesh-axiom force-pushed the refactor/guest-library-reorg-final branch from cd66383 to 828553b Compare May 9, 2025 15:22

This comment has been minimized.

@Avaneesh-axiom Avaneesh-axiom force-pushed the refactor/guest-library-reorg-final branch from 56c28d0 to 1d30696 Compare May 9, 2025 19:21

This comment has been minimized.

@Avaneesh-axiom Avaneesh-axiom force-pushed the refactor/guest-library-reorg-final branch 2 times, most recently from fabaddf to 656670c Compare May 9, 2025 19:38
@jonathanpwang jonathanpwang changed the title refactor: Guest Library Re-org refactor: guest bindings May 9, 2025
Base automatically changed from feat/binding-primitive-libs to release-v1.2.1-rc.0 May 13, 2025 01:23
@jonathanpwang jonathanpwang force-pushed the refactor/guest-library-reorg-final branch from 656670c to bea1fb1 Compare May 13, 2025 01:29
@Avaneesh-axiom Avaneesh-axiom force-pushed the refactor/guest-library-reorg-final branch from bea1fb1 to 4357ece Compare May 13, 2025 20:57
Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

LGTM, but don't merge because we will merge the other PR into this one

This comment has been minimized.

use halo2curves_axiom::ff;

use crate::{field::Field, DivAssignUnsafe, DivUnsafe};

impl<'a, F: ff::Field> DivUnsafe<&'a F> for F {
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't really matter, but what stopped you from doing a blanket implementation for all ff::Field?

let mut is_less = 0u8.into();
// Iterate over limbs in little endian order and retain the result of the last non-equal comparison.
for (x_limb, p_limb) in self.0.iter().zip(<Self as ::openvm_algebra_guest::IntMod>::MODULUS.iter()) {
if x_limb < p_limb {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is technically not constant time because of branching

@jonathanpwang
Copy link
Contributor

It is weird to keep k256.rs in ecc/guest since there is a lot of code duplication.

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

Two follow-ups, but let's merge this first:

  • constant time algorithms (constant time doesn't really make sense in zkvm context I think, but let's revisit)
  • redundant code for k256.rs

This comment has been minimized.

Copy link

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (-23 [-1.9%]) 1,164 334,086 17,676,626 - - -
fibonacci (-37 [-1.5%]) 2,473 1,500,277 50,589,503 - - -
regex (+399 [+5.4%]) 7,764 4,165,226 166,511,152 - - -
ecrecover (+591 [+42.5%]) 1,980 (+339097 [+117.2%]) 628,544 (+15107986 [+104.4%]) 29,578,172 - - -
pairing (-94 [-2.1%]) 4,403 (+36828 [+2.0%]) 1,857,264 (+1216224 [+1.3%]) 97,048,631 - - -

Commit: fa869e0

Benchmark Workflow

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.

3 participants