Skip to content

Implement BatchNormalize for NonIdentity #1896

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
Merged
merged 8 commits into from
Jun 13, 2025
Merged

Implement BatchNormalize for NonIdentity #1896

merged 8 commits into from
Jun 13, 2025

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jun 8, 2025 •
edited
Loading

As discussed in #1889. I will add some tests in elliptic-curves as well.

Resolves #1889.
Companion PR: RustCrypto/elliptic-curves#1248.

@daxpedda daxpedda mentioned this pull request Jun 8, 2025
Closed
tarcieri
tarcieri reviewed Jun 11, 2025
View reviewed changes
tarcieri
tarcieri reviewed Jun 11, 2025
View reviewed changes
@daxpedda daxpedda force-pushed the non-identity-batch-normalize branch from 1c8e9df to e2c3a8f Compare June 12, 2025 09:08
@daxpedda daxpedda mentioned this pull request Jun 12, 2025
Closed
daxpedda
daxpedda commented Jun 12, 2025
View reviewed changes
@daxpedda
Copy link
Contributor Author

daxpedda commented Jun 12, 2025 •
edited
Loading

I completely removed any usage of mem::transmute() and mem::transmute_copy() and opted for safer alternatives.

One notably change: instead of mem::transmute_copy() for fixed arrays, I used array::map(). This seems to be optimized to a memcpy by compilers (Godbolt). Additionally this gets rid of one unsafe call.

@daxpedda daxpedda requested a review from tarcieri June 12, 2025 09:21
tarcieri
tarcieri reviewed Jun 12, 2025
View reviewed changes
daxpedda
daxpedda commented Jun 12, 2025
View reviewed changes
@daxpedda daxpedda requested a review from tarcieri June 12, 2025 16:39
@daxpedda daxpedda force-pushed the non-identity-batch-normalize branch 2 times, most recently from 225884b to e3e5f8f Compare June 12, 2025 16:41
@daxpedda daxpedda force-pushed the non-identity-batch-normalize branch from e3e5f8f to 0a351c7 Compare June 12, 2025 21:29
tarcieri
tarcieri reviewed Jun 12, 2025
View reviewed changes
tarcieri
tarcieri reviewed Jun 12, 2025
View reviewed changes
@daxpedda daxpedda requested a review from tarcieri June 13, 2025 00:12
@daxpedda daxpedda force-pushed the non-identity-batch-normalize branch from 21d93b8 to ced7993 Compare June 13, 2025 00:14
@daxpedda daxpedda force-pushed the non-identity-batch-normalize branch from ced7993 to 6edc705 Compare June 13, 2025 00:14
tarcieri
tarcieri reviewed Jun 13, 2025
View reviewed changes
@tarcieri
Copy link
Member

@daxpedda can you also add cargo careful and cargo miri to CI, similar to these?

https://github.com/RustCrypto/crypto-bigint/blob/dff9eb8/.github/workflows/crypto-bigint.yml#L98-L105

https://github.com/RustCrypto/crypto-bigint/blob/dff9eb8/.github/workflows/crypto-bigint.yml#L133-L147

It would also be good to find a way to exercise this code with tests somewhere in this repo, possibly leveraging MockCurve.

tarcieri
tarcieri approved these changes Jun 13, 2025
View reviewed changes
Copy link
Member

@tarcieri tarcieri 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 now. I can handle the CI changes as a followup.

@daxpedda
Copy link
Contributor Author

Looks good now. I can handle the CI changes as a followup.

Already on it. Trying to add a test right now.

tarcieri
tarcieri reviewed Jun 13, 2025
View reviewed changes
elliptic-curve/src/point/non_identity.rs

#[allow(unsafe_code)]
// SAFETY: `NonIdentity` is `repr(transparent)`.
let points: &[P; N] = unsafe { &*points.as_ptr().cast() };
Copy link
Member

Choose a reason for hiding this comment

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

Curious if you could extract an AsRef impl here, but perhaps I can experiment with that myself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently not, because array is a foreign type. Same applies to slices.

I guess we could add it as a method?

@daxpedda daxpedda requested a review from tarcieri June 13, 2025 18:46
@tarcieri tarcieri merged commit f24c2ae into RustCrypto:master Jun 13, 2025
14 checks passed
tarcieri pushed a commit to RustCrypto/elliptic-curves that referenced this pull request Jun 13, 2025
@daxpedda
Update elliptic-curve (#1257)
b9103a6 
* Update to changes in `BatchNormalize` (RustCrypto/traits#1896)
* Update to hash2curve changes (RustCrypto/traits#1901)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tarcieri tarcieri Awaiting requested review from tarcieri

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement BatchNormalize for NonIdentity

2 participants

@daxpedda @tarcieri