feat: Parse struct name in algebra + ECC init macros as string #1891
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- rename remaining structs - switch from num-bigint-dig to num-bigint
…weierstrass and edwards curves)
Co-authored-by: Jonathan Wang <[email protected]>
A bug involving opcode collisions between short Weierstrass and twisted Edwards curves was found. To fix this, CurveConfig was rewritten and separate opcodes were given to the two types of curves.
Commit: 5a3de18 |
CodSpeed WallTime Performance ReportMerging #1891 will not alter performanceComparing
|
jonathanpwang
added a commit
that referenced
this pull request
Aug 7, 2025
…1945) Cherry-picked from #1891 without the twisted Edwards macros. The `complex_init!` and `sw_init!` macros are updated so that instead of parsing a `Path`, it parses a `LitStr`. This makes it so that the `_init!` macros do not require any path imports in the file where it is called, which greatly improves the developer experience. The main files changed aside from test files and auto-generates `openvm_init.rs` files are: - extensions/algebra/complex-macros/src/lib.rs - extensions/ecc/sw-macros/src/lib.rs In both macros, the change essentially boils down to changing: (before) ```rust let items = input.parse_terminated(<Expr as Parse>::parse, Token![,])?; ``` to (after) ```rust let items = input.parse_terminated(<LitStr as Parse>::parse, Token![,])?; ``` and then associated type changes (mostly simplifications) in converting tokens to strings. There is one additional change needed only for the `sw_init!` macro case. Previously the linked function ```rust extern "C" fn #setup_extern_func() ``` defined by the `sw_init!` macro did use the path of the struct within the function implementation. After this PR, we avoid this import by moving that part of the logic into the `fn set_up_once()` function defined for the struct itself. This changed the function signature of the extern function to: ```rust extern "C" fn #setup_extern_func(uninit: *mut core::ffi::c_void, p1: *const u8, p2: *const u8) ``` [Here](https://www.diffchecker.com/0ncTMwe2/) is the diff of the code that was moved from the `#setup_extern_func()` into the `set_up_once()` function. --------- Co-authored-by: Avaneesh Kulkarni <[email protected]>
jonathanpwang
added a commit
that referenced
this pull request
Aug 7, 2025
…1945) Cherry-picked from #1891 without the twisted Edwards macros. The `complex_init!` and `sw_init!` macros are updated so that instead of parsing a `Path`, it parses a `LitStr`. This makes it so that the `_init!` macros do not require any path imports in the file where it is called, which greatly improves the developer experience. The main files changed aside from test files and auto-generates `openvm_init.rs` files are: - extensions/algebra/complex-macros/src/lib.rs - extensions/ecc/sw-macros/src/lib.rs In both macros, the change essentially boils down to changing: (before) ```rust let items = input.parse_terminated(<Expr as Parse>::parse, Token![,])?; ``` to (after) ```rust let items = input.parse_terminated(<LitStr as Parse>::parse, Token![,])?; ``` and then associated type changes (mostly simplifications) in converting tokens to strings. There is one additional change needed only for the `sw_init!` macro case. Previously the linked function ```rust extern "C" fn #setup_extern_func() ``` defined by the `sw_init!` macro did use the path of the struct within the function implementation. After this PR, we avoid this import by moving that part of the logic into the `fn set_up_once()` function defined for the struct itself. This changed the function signature of the extern function to: ```rust extern "C" fn #setup_extern_func(uninit: *mut core::ffi::c_void, p1: *const u8, p2: *const u8) ``` [Here](https://www.diffchecker.com/0ncTMwe2/) is the diff of the code that was moved from the `#setup_extern_func()` into the `set_up_once()` function. --------- Co-authored-by: Avaneesh Kulkarni <[email protected]>
|
You can merge #1945 and this branch into your twisted edwards branch |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we use the struct path as a shared id between the
*_declare!and*_init!macros so that they could agree on the extern func names. However, this requires the struct to be in scope (as well as some traits likeWeierstrassPointandIntMod). This isn't good for UX with the guest library re-org since these structs and traits are internal, and ideally should be invisible to the user.This PR modifies the
complex_init!,sw_init!, andte_init!macros to take in the struct name as a string. This requires some logic from the setup extern funcs to be moved to the ECC struct'sset_up_onceassociated function. In particular, the setup extern function is now just a wrapper on the custom asm instructions. It's purpose is simply to select the correct opcode (which is how it should have been originally).Note: this branch is based on
feat/edwards-curve-new-executionso #1858 should be reviewed and merged intofeat/new-executionfirst.TODO: remember to remove the
branch = ...fromexamples/algebra/Cargo.tomlandexamples/ecc/Cargo.tomlbefore merging.