Skip to content

Use risc0-bigint2 FFI instead of risc0-zkvm sys_bigint2_n FFI#4

Merged
iddo-bentov merged 2 commits intorelease/v0.3.14from
iddo/accel
Mar 31, 2025
Merged

Use risc0-bigint2 FFI instead of risc0-zkvm sys_bigint2_n FFI#4
iddo-bentov merged 2 commits intorelease/v0.3.14from
iddo/accel

Conversation

@iddo-bentov
Copy link

Switch to risc0-bigint2 extern "C" functions (instead of zkvm-platform sys_bigint2_n).
There's also support for more efficient "unchecked" bigint2 calls, but it's deactivated.

@iddo-bentov iddo-bentov requested review from flaub and tzerrell March 31, 2025 19:45
@flaub
Copy link
Member

flaub commented Mar 31, 2025

Could you update the PR name to be more descriptive?

Copy link

@tzerrell tzerrell 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, glad to have this running with the bigint2 externs rather than through importing blobs. Also make the PR name change and I'll be ready to approve.

This does add some code maintenance complexity around needing to use the precompiler for the sake of an #ifdef __RISC0_UNCHECKED__ that is currently never actually used. Probably this is fine -- the code's already written, and we can always make another PR either utilize this unchecked #ifdef or take it out if it's causing maintenance issues.

Comment on lines +238 to +241
//if env::var("RISC0_UNCHECKED").is_ok() {
// cc.define("__RISC0_UNCHECKED__", None);
//}

Choose a reason for hiding this comment

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

Maybe add a comment explaining what this does. Perhaps something like this

Suggested change
//if env::var("RISC0_UNCHECKED").is_ok() {
// cc.define("__RISC0_UNCHECKED__", None);
//}
// // Allows the use of precompiles that are faster but which can be misused insecurely
//if env::var("RISC0_UNCHECKED").is_ok() {
// cc.define("__RISC0_UNCHECKED__", None);
//}

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I added this comment now.

@iddo-bentov iddo-bentov changed the title Iddo/accel Use risc0-bigint2 FFI instead of risc0-zkvm sys_bigint2_n FFI Mar 31, 2025
@iddo-bentov iddo-bentov merged commit 9ed9fb1 into release/v0.3.14 Mar 31, 2025
@iddo-bentov iddo-bentov deleted the iddo/accel branch March 31, 2025 23:17
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