Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: elliptic-curves integration, field agnostic sqrt/inverse hooks #2039

Merged
merged 53 commits into from
Feb 7, 2025
Merged

feat: elliptic-curves integration, field agnostic sqrt/inverse hooks #2039

merged 53 commits into from
Feb 7, 2025

Conversation

nhtyy
Copy link
Collaborator

@nhtyy nhtyy commented Feb 6, 2025 •
edited
Loading

This PR:

  • Implements ProjectivePoint, AffinePoint, and Scalar types for use with elliptic-curves traits.
  • Adds benchmarking for differences in cycles when paths in patch-testing are updated.
  • Removes committed lock files from patch-testing programs
  • Bumps dev version to 4.1.0

Todo:

  • Update docs with new patch links
  • Bump any usage of patches

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025 •
edited
Loading

Test Old New Diff
curve25519_dalek_test_decompressed_noncanonical 9263 9263 0.0000 %
secp256k1_program_test_recover_rand_lte_100 16583108 16574593 0.0513 %
k256_test_verify_rand_lte_100 461756374 24641305 94.6636 %
p256_test_recover_high_hash_high_recid 27808919 3815458 86.2797 %
curve25519_dalek_test_zero_msm 213931 213931 0.0000 %
curve25519_dalek_test_zero_mul 197994 197994 0.0000 %
rust_crypto_rsa_test_pkcs_verify_100 174645644 102105014 41.5359 %
bn_test_bn_test_fq_sqrt_100 989049 989049 0.0000 %
keccack_test_expected_digest_lte_100 2033521 2031988 0.0754 %
bls12_381_tests_test_sqrt_fp_100 1044124 1003959 3.8468 %
bls12_381_tests_test_sqrt_fp2_100 2084478 2034976 2.3748 %
curve25519_dalek_test_decompressed_expected_value 15693506 15387022 1.9529 %
rustcrypto_bigint_test_bigint_mul_mod_special 2332628 2332628 0.0000 %
curve25519_dalek_test_add_then_multiply 6847788 6372096 6.9467 %
k256_test_schnorr_verify 0 7010795 -inf %
curve25519_dalek_ng_test_zero_mul 197964 197964 0.0000 %
rustcrypto_bigint_test_bigint_mul_add_residue 2250012 2250012 0.0000 %
bls12_381_tests_test_inverse_fp_100 1607625 1607625 0.0000 %
bls12_381_tests_test_bls_add_100 15580049 15580049 0.0000 %
secp256k1_program_test_verify_rand_lte_100 179064023 178695251 0.2059 %
curve25519_dalek_test_ed25519_verify 32287391 32286522 0.0027 %
p256_test_recover_pubkey_infinity 417746 142888 65.7955 %
bn_test_bn_test_g1_double_100 624788 624809 -0.0034 %
bls12_381_tests_test_bls_double_100 9670245 9670245 0.0000 %
bls12_381_tests_test_inverse_fp2_100 3085079 3085079 0.0000 %
bn_test_bn_test_fr_inverse_100 990649 990649 0.0000 %
curve25519_dalek_ng_test_decompressed_noncanonical 204793 204793 0.0000 %
curve25519_dalek_ng_test_add_then_multiply 7176876 8004952 -11.5381 %
p256_test_recover_rand_lte_100 65992788 7225372 89.0513 %
p256_test_verify_rand_lte_100 1202567144 24901970 97.9293 %
sha_test_sha2_expected_digest_lte_100_times 3760428 3761654 -0.0326 %
k256_test_recover_high_hash_high_recid 10142486 2732034 73.0635 %
bn_test_bn_test_g1_add_100 811499 811485 0.0017 %
k256_test_recover_rand_lte_100 15581506 5743065 63.1418 %
bn_test_bn_test_fq_inverse_100 969449 969449 0.0000 %
k256_test_recover_pubkey_infinity 364553 129255 64.5443 %
curve25519_dalek_ng_test_zero_msm 217587 217587 0.0000 %
sha_test_sha3_expected_digest_lte_100_times 1793201 1793367 -0.0093 %

@nhtyy nhtyy force-pushed the n/fp_ops_hook branch 6 times, most recently from 4d8b992 to df0871b Compare February 7, 2025 17:27
ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
crates/zkvm/lib/src/ecdsa/projective.rs Outdated

/// Convert the projective point to an affine point.
///
/// Public as its used in patched crates.
Copy link
Member

Choose a reason for hiding this comment

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

No need for the "public" as it's used in patched crates comment.

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
crates/zkvm/lib/src/ecdsa/projective.rs Outdated

/// Check if the point is the identity point.
///
/// Public as its used in patched crates.
Copy link
Member

Choose a reason for hiding this comment

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

Comment unnecessary

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
crates/zkvm/lib/src/ecdsa/projective.rs Outdated
}
}

// todo: not actually true?
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove?

Copy link
Member

Choose a reason for hiding this comment

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

You did not fix this?

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
crates/zkvm/lib/src/ecdsa/projective.rs Outdated

impl<C: ECDSACurve> ConditionallySelectable for ProjectivePoint<C> {
fn conditional_select(a: &Self, b: &Self, choice: Choice) -> Self {
// Note: we dont care about constant time operations in the vm.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add a more clear and professional comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed comment and called ConditionalSelect on the inner affine type directly.

Copy link
Member

Choose a reason for hiding this comment

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

This comment is not up to date. Did you push the altest changes?

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/sha/Cargo.toml
@@ -17,7 +17,8 @@ sha3 = "=0.10.6"

[features]
prove = []
gpu = ["sp1-sdk/cuda"]
gpu = ["sp1-sdk/cuda"]

Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/secp256k1/Cargo.toml
@@ -15,6 +15,7 @@ sp1-test = { workspace = true }
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/p256/Cargo.toml
@@ -18,5 +18,6 @@ sp1-test = { workspace = true }
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/k256/Cargo.toml
@@ -18,5 +18,6 @@ sp1-test = { workspace = true }
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/curve25519-dalek/Cargo.toml
@@ -16,5 +16,6 @@ rand.workspace = true
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/curve25519-dalek-ng/Cargo.toml
@@ -15,5 +15,6 @@ rand.workspace = true
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/bn/Cargo.toml
@@ -16,5 +16,6 @@ substrate-bn = "0.6.0"
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/bls12-381/Cargo.toml
@@ -20,5 +20,6 @@ group = "0.13.0"
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani reviewed Feb 7, 2025
View reviewed changes
patch-testing/RustCrypto-rsa/Cargo.toml
@@ -16,5 +16,6 @@ rsa = { version = "0.9.7", features = ["std", "sha2", "serde"] }
prove = []
gpu = ["sp1-sdk/cuda"]


Copy link
Member

Choose a reason for hiding this comment

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

nit

ratankaliani
ratankaliani approved these changes Feb 7, 2025
View reviewed changes
Copy link
Member

@ratankaliani ratankaliani left a comment

Choose a reason for hiding this comment

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

LGTM, one last suggestion.

@nhtyy nhtyy merged commit e09ed7a into dev Feb 7, 2025
15 checks passed
@nhtyy nhtyy deleted the n/fp_ops_hook branch February 7, 2025 22:48
nhtyy added a commit that referenced this pull request Feb 8, 2025
@nhtyy @github-actions @ratankaliani
feat: elliptic-curves integration, field agnostic sqrt/inverse hooks (
285fb8e 
#2039)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ratan Kaliani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@ratankaliani ratankaliani ratankaliani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nhtyy @ratankaliani